Re: [RFC v2 2/2] backlight: pwm_bl: compute brightness of LED linearly to human eye.

[Enric Balletbo Serra]

Hi,

2017-11-30 12:27 GMT+01:00 Daniel Thompson <daniel.thompson@xxxxxxxxxx>:
>
>
> On 30/11/17 00:44, Doug Anderson wrote:
>>
>> Hi,
>>
>> On Thu, Nov 16, 2017 at 6:11 AM, Enric Balletbo i Serra
>> <enric.balletbo@xxxxxxxxxxxxx> wrote:
>>>
>>> When you want to change the brightness using a PWM signal, one thing you
>>> need to consider is how human perceive the brightness. Human perceive the
>>> brightness change non-linearly, we have better sensitivity at low
>>> luminance than high luminance, so to achieve perceived linear dimming,
>>> the
>>> brightness must be matches to the way our eyes behave. The CIE 1931
>>> lightness formula is what actually describes how we perceive light.
>>>
>>> This patch adds support to compute the brightness levels based on a
>>> static
>>> table filled with the numbers provided by the CIE 1931 algorithm, for now
>>> it only supports PWM resolutions up to 65535 (16 bits) with 1024 steps.
>>> Lower PWM resolutions are implemented using the same curve but with less
>>> steps, e.g. For a PWM resolution of 256 (8 bits) we have 37 steps.
>>
>>
>> Your patch assumes that the input to your formula (luminance, I think)
>> scales linearly with PWM duty cycle.  I don't personally know this,
>> but has anyone confirmed it's common in reality, or at least is a
>> close enough approximation of reality?
>
>
> Isn't this the loop we went round for v1?
>
> We do know that its not linear, however the graphs from a couple of example
> devices didn't look too scary and nobody has proposed a better formula.
>
> At this point the linear interpolation code in patch 1 allows people with
> especially alinear devices to express suitable brightness curves.
>
> However we also know that many DT authors choose not to create good
> brightness tables for their devices... and we'd rather they used allowed the
> kernel to choose a model than to use no model at all.
>
>
> Daniel.
>
>
>
> Enric: BTW sorry I haven't replied so far. That's mostly because
>        these looked more "real" and that I should pay them close
>        attention (which requires time I haven't had spare to
>        consume yet).
>

No problem. It also took me some time to send v2 because of was busy
with other things :)

>
>
>>> The calculation of the duty cycle using the CIE 1931 algorithm is enabled
>>> by
>>> default when you do not define the 'brightness-levels' propriety in your
>>> device tree.
>>
>>
>> One note is that you probably still want at least a "min" duty cycle.
>> I seem to remember some PWM backlights don't work well when the duty
>> cycle is too low and it would still be nice to be able to use your
>> table.
>>

Right, iirc that is needed on some veyron devices, and this is another
reason why the first patch needs to support to be able to describe
corrected gamma curves and not only linear-interpolation between 2
points. I'd say that:

1. For low and high resolution PWMs,  if you know nothing about PWM
backlight, patch 2 can provide to the user a default table that should
work.
2. If you need something more specific you should use the old way
(aka, use brightness levels table)
    2.1 If you have a high resolution PWM you might be interested on
enable interpolation.

>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>>> ---
>>>   drivers/video/backlight/pwm_bl.c | 160
>>> +++++++++++++++++++++++++++++++++++----
>>>   include/linux/pwm_backlight.h    |   1 +
>>>   2 files changed, 147 insertions(+), 14 deletions(-)
>>
>>
>> Something I'd like to see in a patch somewhere in this series is a way
>> to expose the backlight "units" to userspace.  As far as I know right
>> now the backlight exposed to userspace is "unitless", but it would be
>> nice for userspace to query that the backlight is now linear to human
>> perception.  For old code, it could always expose the unit as
>> "unknown".
>>
>>
>>> diff --git a/drivers/video/backlight/pwm_bl.c
>>> b/drivers/video/backlight/pwm_bl.c
>>> index 59b1bfb..ea96358 100644
>>> --- a/drivers/video/backlight/pwm_bl.c
>>> +++ b/drivers/video/backlight/pwm_bl.c
>>> @@ -26,6 +26,112 @@
>>>
>>>   #define NSTEPS 256
>>>
>>> +/*
>>> + * CIE lightness to PWM conversion table. The CIE 1931 lightness formula
>>> is what
>>> + * actually describes how we perceive light:
>>> + *
>>> + *          Y = (L* / 902.3)           if L* â 0.08856
>>> + *          Y = ((L* + 16) / 116)^3    if L* > 0.08856
>>> + *
>>> + * Where Y is the luminance (output) between 0.0 and 1.0, and L* is the
>>> + * lightness (input) between 0 and 100.
>>
>>
>> Just because I'm stupid and not 100% sure, I think:
>>

Just because I'm also stupid and I'm still learning :) I'll try to
clarify a bit more that ...


>> luminance = the amount of light coming out of the screen
>> lightness = how bright a human perceives the screen to be
>>
>> Is that right?  If so could you add it to the comments?  So "output"
>> here is the output to the PWM and "input" is the input from userspace
>> (and thus should be expressed in terms of human perception).
>>

Yes, I think that how you describe luminance and lightness is right,
and sounds good improve the doc.

To be clear the correction table for PWM values can be calculated with
this code.

OUTPUT_SIZE = 65535      # Output integer size
INPUT_SIZE = 2047

def cie1931(L):
    L = L*100.0
    if L <= 8:
        return (L/902.3)
    else:
        return ((L+16.0)/116.0)**3

x = range(0,int(INPUT_SIZE+1))
y = [int(round(cie1931(float(L)/INPUT_SIZE)*(OUTPUT_SIZE))) for L in x]


>>
>>> +       0, 7, 14, 21, 28, 35, 43, 50, 57, 64, 71, 78, 85, 92, 99, 106,
>>> 114, 121,
>>
>>
>> Seems like you could save space (and nicely use the previous patch) by
>> using the linear interpolation code from the previous patch, since
>>
>> 0 + 7 = 7
>> + 7 = 14
>> + 7 = 21
>> + 7 = 28
>> + 7 = 35
>>
>> ...and it would likely be OK to keep going and be slight off, so:
>>
>> + 7 = 42
>> + 7 = 49
>> + 7 = 56
>> + 7 = 63
>> + 7 = 70
>> ...
>> ...
>>
>> In other words it seems like you're just providing a default table...
>>

Yes, I think that's the Daniel idea ;)

>> -Doug
>>
>

Best regards,
 - Enric