Re: [PATCH v3 3/4] backlight: pwm_bl: compute brightness of LED linearly to human eye.

[Matthias Kaehlcke <mka@chromium.org>]

On Mon, Jun 10, 2019 at 11:02:27PM +0200, Enric Balletbo i Serra wrote:
> Hi Matthias,
> 
> On 10/6/19 22:39, Matthias Kaehlcke wrote:
> > Hi Enric
> > 
> > On Mon, Jun 10, 2019 at 12:00:02PM +0200, Enric Balletbo i Serra wrote:
> >> Hi Matthias,
> >>
> >> On 8/6/19 23:02, Pavel Machek wrote:
> >>> Hi!
> >>>
> >>>>> +	 * 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;
> >>>>> +	}
> >>>>
> >>>> I don't quite follow the heuristics above. Are you sure the number of
> >>>> PWM bits can be infered from the period? What if the period value (in
> >>>> ns) doesn't directly correspond to a register value? And even if it
> >>>> did, counting the number of set bits (the above loops is a
> >>>> re-implementation of ffs()) doesn't really result in the dividers
> >>>> mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
> >>>> in a divider of 1, i.e. 32768 brighness levels.
> >>>>
> >>
> >> Right, I think that only works on the cases that we only have one pwm cell, and
> >> looks like during my tests I did only tests on devices with one pwm cell :-(
> >>
> >> And as you point the code is broken for other cases (pwm-cells > 1)
> >>
> >>>> On veyron minnie the period is 1000000 ns, which results in 142858
> >>>> levels (1000000 / 7)!
> >>>>
> >>>> Not sure if there is a clean solution using heuristics, a DT property
> >>>> specifying the number of levels could be an alternative. This could
> >>>> also be useful to limit the number of (mostly) redundant levels, even
> >>>> the intended max of 4096 seems pretty high.
> >>>>
> >>
> >> Looking again looks like we _can not_ deduce the number of bits of a pwm, it is
> >> not exposed at all, so I think we will need to end adding a property to specify
> >> this. Something similar to what leds-pwm binding does, it has:
> >>
> >> max-brightness : Maximum brightness possible for the LED
> > 
> > Thanks for the confirmation that I didn't just miss some clever trick.
> > 
> > I also think that some kind of DT property is needed, I'll try to come
> > up with a reasonable name, keeping in mind that some devices might not
> > want to use the entire range of levels.
> > 
> 
> Note that, If I remember correctly, the original idea behind all these patches
> was provide a default curve with enough points following the  CIE 1931 formula
> (which describes how we perceive light). When default doesn't work for your
> hardware, you could play and define your own curve using the
> num-interpolated-steps property I.e:
> 
>  brightness-levels = <0 2048 4096 8192 16384 65535>;
>  num-interpolated-steps = <2048>;
>  default-brightness-level = <4096>;
> 
> Or even expose all the possible levels, like you do with your chromeos kernel.
> 
>  brightness-levels = <0 65535>;
>  num-interpolated-steps = <65535>;
>  default-brightness-level = <4096>;
> 
> The above should work independently of the bug found (that of course needs to be
> fixed)

Thanks for the info, indeed (keep) using a custom curve could be an
option.

I'm learning about the corresponding user space component of Chrome OS
as we speak. It looks like it's possible to specify the minimum
brightness level to use, which should do the trick, also making the OS
aware of the exponential nature of the backlight levels might help.