| Message ID | 20190610233739.29477-2-mka@chromium.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] dt-bindings: pwm-backlight: Add 'max-brightness' property | expand |
On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote: > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED > linearly to human eye") uses pwm_period / hweight32(pwm_period) as > as heuristic to determine the number of brightness levels when the DT > doesn't provide a brightness level table. This heuristic is broken > and can result in excessively large brightness tables. > > Instead of using the heuristic try to retrieve the number of > brightness levels from the device tree (property 'max-brightness' > + 1). If the value is not specified use a default of 256 levels. > > Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye") I don't think this one is suitable for stable. I'm pretty sure the heuristics works well for many boards, and you just replaced it with another heuristics ("256"). > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> Acked-by: Pavel Machek <pavel@ucw.cz>
On Mon, Jun 10, 2019 at 04:37:39PM -0700, Matthias Kaehlcke wrote: > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED > linearly to human eye") uses pwm_period / hweight32(pwm_period) as > as heuristic to determine the number of brightness levels when the DT > doesn't provide a brightness level table. This heuristic is broken > and can result in excessively large brightness tables. > > Instead of using the heuristic try to retrieve the number of > brightness levels from the device tree (property 'max-brightness' > + 1). If the value is not specified use a default of 256 levels. I'll look at the code tomorrow but why 256? To me it feels simultaneously too big for a simple 8-bit PWM and too small for animated backlight effects. I certainly agree that an override could be useful but I'm not clear why deriving a default based on the period is bogus (and the description is merely concerned about uselessly big tables). /* * Once we have 4096 levels there's little point going much higher... * neither interactive sliders nor animation benefits from having * more values in the table. */ max_brightness = min(DIV_ROUND_UP(period, ffs(period), 4096); Daniel. > > Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye") > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/video/backlight/pwm_bl.c | 59 ++++++++++++-------------------- > 1 file changed, 21 insertions(+), 38 deletions(-) > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c > index fb45f866b923..2913cbe9cfcb 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -194,38 +194,19 @@ int pwm_backlight_brightness_default(struct device *dev, > struct platform_pwm_backlight_data *data, > unsigned int period) > { > - unsigned int counter = 0; > - unsigned int i, n; > + unsigned int i; > + unsigned int nlevels = data->max_brightness + 1; > u64 retval; > > - /* > - * Count the number of bits needed to represent the period number. The > - * number of bits is used to calculate the number of levels used for the > - * brightness-levels table, the purpose of this calculation is have a > - * pre-computed table with enough levels to get linear brightness > - * perception. The period is divided by the number of bits so for a > - * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM > - * we have 65535 / 16 = 4096 brightness levels. > - * > - * Note that this method is based on empirical testing on different > - * devices with PWM of 8 and 16 bits of resolution. > - */ > - n = period; > - while (n) { > - counter += n % 2; > - n >>= 1; > - } > - > - data->max_brightness = DIV_ROUND_UP(period, counter); > - data->levels = devm_kcalloc(dev, data->max_brightness, > + data->levels = devm_kcalloc(dev, nlevels, > sizeof(*data->levels), GFP_KERNEL); > if (!data->levels) > return -ENOMEM; > > /* Fill the table using the cie1931 algorithm */ > - for (i = 0; i < data->max_brightness; i++) { > + for (i = 0; i < nlevels; i++) { > retval = cie1931((i * PWM_LUMINANCE_SCALE) / > - data->max_brightness, PWM_LUMINANCE_SCALE) * > + nlevels, PWM_LUMINANCE_SCALE) * > period; > retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE); > if (retval > UINT_MAX) > @@ -233,8 +214,7 @@ int pwm_backlight_brightness_default(struct device *dev, > data->levels[i] = (unsigned int)retval; > } > > - data->dft_brightness = data->max_brightness / 2; > - data->max_brightness--; > + data->dft_brightness = nlevels / 2; > > return 0; > } > @@ -272,8 +252,13 @@ static int pwm_backlight_parse_dt(struct device *dev, > * set a default table of brightness levels will be used. > */ > prop = of_find_property(node, "brightness-levels", &length); > - if (!prop) > + if (!prop) { > + if (of_property_read_u32(node, "max-brightness", > + &data->max_brightness)) > + data->max_brightness = 255; > + > return 0; > + } > > data->max_brightness = length / sizeof(u32); > > @@ -565,13 +550,10 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > pb->levels = data->levels; > } > - } else if (!data->max_brightness) { > + } else if (node) { > /* > - * If no brightness levels are provided and max_brightness is > - * not set, use the default brightness table. For the DT case, > - * max_brightness is set to 0 when brightness levels is not > - * specified. For the non-DT case, max_brightness is usually > - * set to some value. > + * If no brightness levels are provided use the default > + * brightness table. > */ > > /* Get the PWM period (in nanoseconds) */ > @@ -591,12 +573,13 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > pb->levels = data->levels; > } > - } else { > - /* > - * That only happens for the non-DT case, where platform data > - * sets the max_brightness value. > - */ > + } else if (data->max_brightness) { > + /* non-DT case, max_brightness value set in platform data. */ > pb->scale = data->max_brightness; > + } else { > + dev_err(&pdev->dev, "max brightness is not specified\n"); > + ret = -EINVAL; > + goto err_alloc; > } > > pb->lth_brightness = data->lth_brightness * (state.period / pb->scale); > -- > 2.22.0.rc2.383.gf4fbbf30c2-goog >
Hi Daniel, On Tue, Jun 11, 2019 at 04:33:14PM +0100, Daniel Thompson wrote: > On Mon, Jun 10, 2019 at 04:37:39PM -0700, Matthias Kaehlcke wrote: > > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED > > linearly to human eye") uses pwm_period / hweight32(pwm_period) as > > as heuristic to determine the number of brightness levels when the DT > > doesn't provide a brightness level table. This heuristic is broken > > and can result in excessively large brightness tables. > > > > Instead of using the heuristic try to retrieve the number of > > brightness levels from the device tree (property 'max-brightness' > > + 1). If the value is not specified use a default of 256 levels. > > I'll look at the code tomorrow but why 256? > > To me it feels simultaneously too big for a simple 8-bit PWM and too > small for animated backlight effects. I agree there is no one-size-fits-it-all default, 256 seemed like a possible compromise. > I certainly agree that an override could be useful but I'm not clear why > deriving a default based on the period is bogus (and the description is > merely concerned about uselessly big tables). Maybe it's not necessarily bogus, but the current heuristic that counts the number of set bits (hweight()) in the period certainly is. IIUC the period provides a clue about the PWM resolution, because it would be hard/impossible to accomodate the high resolution in shorter periods. > /* > * Once we have 4096 levels there's little point going much higher... > * neither interactive sliders nor animation benefits from having > * more values in the table. > */ > max_brightness = min(DIV_ROUND_UP(period, ffs(period), 4096); I was also considering something along these lines, but wasn't sure if there is indeed a relation between the period and the PWM resolution. I take your suggestion as a confirmation :)
Hi Pavel, On Tue, Jun 11, 2019 at 12:18:43PM +0200, Pavel Machek wrote: > On Mon 2019-06-10 16:37:39, Matthias Kaehlcke wrote: > > Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED > > linearly to human eye") uses pwm_period / hweight32(pwm_period) as > > as heuristic to determine the number of brightness levels when the DT > > doesn't provide a brightness level table. This heuristic is broken > > and can result in excessively large brightness tables. > > > > Instead of using the heuristic try to retrieve the number of > > brightness levels from the device tree (property 'max-brightness' > > + 1). If the value is not specified use a default of 256 levels. > > > > Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye") > > I don't think this one is suitable for stable. I'm pretty sure the > heuristics works well for many boards, and you just replaced it with > another heuristics ("256"). whether the patch is suitable for stable/upstream is certainly debatable, in any case I'd argue the current heuristic is bogus and works by accident or at a cost: nlevels = period / hweight(period) w/ period = 131071 ns (0x1FFFF) nlevels = 131071 / 17 = 7710 w/ period = 131072 ns (0x20000) nlevels = 131072 / 1 = 131072 and some PWMs use significantly higher periods like 1 ms or 10 ms.
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index fb45f866b923..2913cbe9cfcb 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -194,38 +194,19 @@ int pwm_backlight_brightness_default(struct device *dev, struct platform_pwm_backlight_data *data, unsigned int period) { - unsigned int counter = 0; - unsigned int i, n; + unsigned int i; + unsigned int nlevels = data->max_brightness + 1; u64 retval; - /* - * Count the number of bits needed to represent the period number. The - * number of bits is used to calculate the number of levels used for the - * brightness-levels table, the purpose of this calculation is have a - * pre-computed table with enough levels to get linear brightness - * perception. The period is divided by the number of bits so for a - * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM - * we have 65535 / 16 = 4096 brightness levels. - * - * Note that this method is based on empirical testing on different - * devices with PWM of 8 and 16 bits of resolution. - */ - n = period; - while (n) { - counter += n % 2; - n >>= 1; - } - - data->max_brightness = DIV_ROUND_UP(period, counter); - data->levels = devm_kcalloc(dev, data->max_brightness, + data->levels = devm_kcalloc(dev, nlevels, sizeof(*data->levels), GFP_KERNEL); if (!data->levels) return -ENOMEM; /* Fill the table using the cie1931 algorithm */ - for (i = 0; i < data->max_brightness; i++) { + for (i = 0; i < nlevels; i++) { retval = cie1931((i * PWM_LUMINANCE_SCALE) / - data->max_brightness, PWM_LUMINANCE_SCALE) * + nlevels, PWM_LUMINANCE_SCALE) * period; retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE); if (retval > UINT_MAX) @@ -233,8 +214,7 @@ int pwm_backlight_brightness_default(struct device *dev, data->levels[i] = (unsigned int)retval; } - data->dft_brightness = data->max_brightness / 2; - data->max_brightness--; + data->dft_brightness = nlevels / 2; return 0; } @@ -272,8 +252,13 @@ static int pwm_backlight_parse_dt(struct device *dev, * set a default table of brightness levels will be used. */ prop = of_find_property(node, "brightness-levels", &length); - if (!prop) + if (!prop) { + if (of_property_read_u32(node, "max-brightness", + &data->max_brightness)) + data->max_brightness = 255; + return 0; + } data->max_brightness = length / sizeof(u32); @@ -565,13 +550,10 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->levels = data->levels; } - } else if (!data->max_brightness) { + } else if (node) { /* - * If no brightness levels are provided and max_brightness is - * not set, use the default brightness table. For the DT case, - * max_brightness is set to 0 when brightness levels is not - * specified. For the non-DT case, max_brightness is usually - * set to some value. + * If no brightness levels are provided use the default + * brightness table. */ /* Get the PWM period (in nanoseconds) */ @@ -591,12 +573,13 @@ static int pwm_backlight_probe(struct platform_device *pdev) pb->levels = data->levels; } - } else { - /* - * That only happens for the non-DT case, where platform data - * sets the max_brightness value. - */ + } else if (data->max_brightness) { + /* non-DT case, max_brightness value set in platform data. */ pb->scale = data->max_brightness; + } else { + dev_err(&pdev->dev, "max brightness is not specified\n"); + ret = -EINVAL; + goto err_alloc; } pb->lth_brightness = data->lth_brightness * (state.period / pb->scale);
Commit 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye") uses pwm_period / hweight32(pwm_period) as as heuristic to determine the number of brightness levels when the DT doesn't provide a brightness level table. This heuristic is broken and can result in excessively large brightness tables. Instead of using the heuristic try to retrieve the number of brightness levels from the device tree (property 'max-brightness' + 1). If the value is not specified use a default of 256 levels. Fixes: 88ba95bedb79 ("backlight: pwm_bl: Compute brightness of LED linearly to human eye") Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/video/backlight/pwm_bl.c | 59 ++++++++++++-------------------- 1 file changed, 21 insertions(+), 38 deletions(-)