Re: [PATCH 2/2] backlight: pwm_bl: Get number of brightness levels for CIE 1931 from the device tree

[Daniel Thompson]

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
>