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@xxxxxxxxxxxx>
> ---
> 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
>
