Message ID | 20171116141151.21171-1-enric.balletbo@collabora.com |
---|---|
Headers | show |
Series | backlight: pwm_bl: support linear brightness to human eye | expand |
Hi, On Thu, Nov 16, 2017 at 6:11 AM, Enric Balletbo i Serra <enric.balletbo@collabora.com> 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? > 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. > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > 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: 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). > + 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... -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@collabora.com> 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). >> 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. > > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> 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: > > 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). > > >> + 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... > > -Doug > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Nov 30, 2017 at 3:27 AM, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > > 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@collabora.com> 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. OK, cool. I didn't remember anyone actually confirming that they had checked that this was the case, but that's probably just my bad memory and failures at searching through history. I don't have any objections to the idea if people are convinced it's a good enough approximation. :) It would be kinda nice if something could go in the commit message, like: This method will work in any cases where linearly scaling the PWM duty cycle causes a roughly linear scaling of the luminance of the backlight. :) -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, 2017-11-30 12:27 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>: > > > 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@collabora.com> 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@collabora.com> >>> --- >>> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On Thu, Nov 30, 2017 at 10:34 AM, Enric Balletbo Serra <eballetbo@gmail.com> wrote: > Hi, > > 2017-11-30 12:27 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>: >> >> >> 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@collabora.com> 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. The thing I was thinking was that it might be very common to have a min PWM duty cycle. It would be a shame if patch set #2 was almost a perfect fit but you were forced to specify a table in the device tree (to get your output to be linear w/ respect to human perception) just because you had a minimum duty cycle. >>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>>> --- >>>> 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] Personally I'd love to see that little bit of code in the commit message to help explain where the table came from, though I'm known for putting PhD theses in my commit messages... ;) >>>> + 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 16, 2017 at 03:11:51PM +0100, Enric Balletbo i Serra 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. > > 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. > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > drivers/video/backlight/pwm_bl.c | 160 +++++++++++++++++++++++++++++++++++---- > include/linux/pwm_backlight.h | 1 + > 2 files changed, 147 insertions(+), 14 deletions(-) > > 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. > + */ > +const unsigned int cie1931_table[1024] = { I'm a little surprised to see such a large table. I thought we'd be able to use the linear interpolation logic to smooth a smaller table. > + 0, 7, 14, 21, 28, 35, 43, 50, 57, 64, 71, 78, 85, 92, 99, 106, 114, 121, > + 128, 135, 142, 149, 156, 163, 170, 177, 185, 192, 199, 206, 213, 220, > + 227, 234, 241, 248, 256, 263, 270, 277, 284, 291, 298, 305, 312, 319, > + 327, 334, 341, 348, 355, 362, 369, 376, 383, 390, 398, 405, 412, 419, > + 426, 433, 440, 447, 454, 461, 469, 476, 483, 490, 497, 504, 511, 518, > + 525, 532, 540, 547, 554, 561, 568, 575, 582, 589, 596, 603, 610, 618, > + 625, 633, 640, 648, 655, 663, 671, 679, 687, 695, 703, 711, 719, 727, > + 735, 744, 752, 761, 769, 778, 786, 795, 804, 813, 822, 831, 840, 849, > + 858, 867, 876, 886, 895, 905, 914, 924, 934, 943, 953, 963, 973, 983, > + 993, 1004, 1014, 1024, 1034, 1045, 1055, 1066, 1077, 1087, 1098, 1109, > + 1120, 1131, 1142, 1153, 1165, 1176, 1187, 1199, 1210, 1222, 1234, 1245, > + 1257, 1269, 1281, 1293, 1305, 1318, 1330, 1342, 1355, 1367, 1380, 1392, > + 1405, 1418, 1431, 1444, 1457, 1470, 1483, 1497, 1510, 1523, 1537, 1551, > + 1564, 1578, 1592, 1606, 1620, 1634, 1648, 1662, 1677, 1691, 1706, 1720, > + 1735, 1750, 1765, 1780, 1795, 1810, 1825, 1840, 1855, 1871, 1886, 1902, > + 1918, 1933, 1949, 1965, 1981, 1997, 2014, 2030, 2046, 2063, 2079, 2096, > + 2113, 2130, 2146, 2163, 2181, 2198, 2215, 2232, 2250, 2267, 2285, 2303, > + 2321, 2338, 2356, 2375, 2393, 2411, 2429, 2448, 2466, 2485, 2504, 2523, > + 2542, 2561, 2580, 2599, 2618, 2638, 2657, 2677, 2697, 2716, 2736, 2756, > + 2776, 2796, 2817, 2837, 2858, 2878, 2899, 2920, 2941, 2961, 2983, 3004, > + 3025, 3046, 3068, 3089, 3111, 3133, 3155, 3177, 3199, 3221, 3243, 3266, > + 3288, 3311, 3333, 3356, 3379, 3402, 3425, 3448, 3472, 3495, 3519, 3542, > + 3566, 3590, 3614, 3638, 3662, 3686, 3711, 3735, 3760, 3784, 3809, 3834, > + 3859, 3884, 3910, 3935, 3960, 3986, 4012, 4037, 4063, 4089, 4115, 4142, > + 4168, 4194, 4221, 4248, 4274, 4301, 4328, 4356, 4383, 4410, 4438, 4465, > + 4493, 4521, 4549, 4577, 4605, 4633, 4661, 4690, 4719, 4747, 4776, 4805, > + 4834, 4863, 4893, 4922, 4952, 4981, 5011, 5041, 5071, 5101, 5131, 5162, > + 5192, 5223, 5254, 5285, 5316, 5347, 5378, 5409, 5441, 5472, 5504, 5536, > + 5568, 5600, 5632, 5664, 5697, 5729, 5762, 5795, 5828, 5861, 5894, 5928, > + 5961, 5995, 6028, 6062, 6096, 6130, 6164, 6199, 6233, 6268, 6302, 6337, > + 6372, 6407, 6442, 6478, 6513, 6549, 6585, 6621, 6657, 6693, 6729, 6765, > + 6802, 6839, 6875, 6912, 6949, 6986, 7024, 7061, 7099, 7137, 7174, 7212, > + 7250, 7289, 7327, 7366, 7404, 7443, 7482, 7521, 7560, 7600, 7639, 7679, > + 7718, 7758, 7798, 7838, 7879, 7919, 7960, 8000, 8041, 8082, 8123, 8165, > + 8206, 8248, 8289, 8331, 8373, 8415, 8457, 8500, 8542, 8585, 8628, 8671, > + 8714, 8757, 8800, 8844, 8887, 8931, 8975, 9019, 9064, 9108, 9152, 9197, > + 9242, 9287, 9332, 9377, 9423, 9468, 9514, 9560, 9606, 9652, 9698, 9745, > + 9791, 9838, 9885, 9932, 9979, 10026, 10074, 10121, 10169, 10217, 10265, > + 10313, 10362, 10410, 10459, 10508, 10557, 10606, 10655, 10704, 10754, > + 10804, 10854, 10904, 10954, 11004, 11055, 11105, 11156, 11207, 11258, > + 11310, 11361, 11413, 11464, 11516, 11568, 11620, 11673, 11725, 11778, > + 11831, 11884, 11937, 11990, 12044, 12097, 12151, 12205, 12259, 12314, > + 12368, 12423, 12477, 12532, 12587, 12643, 12698, 12754, 12809, 12865, > + 12921, 12977, 13034, 13090, 13147, 13204, 13261, 13318, 13375, 13433, > + 13491, 13548, 13606, 13665, 13723, 13781, 13840, 13899, 13958, 14017, > + 14077, 14136, 14196, 14256, 14316, 14376, 14436, 14497, 14557, 14618, > + 14679, 14740, 14802, 14863, 14925, 14987, 15049, 15111, 15173, 15236, > + 15299, 15362, 15425, 15488, 15551, 15615, 15679, 15743, 15807, 15871, > + 15935, 16000, 16065, 16130, 16195, 16260, 16326, 16392, 16457, 16523, > + 16590, 16656, 16723, 16789, 16856, 16923, 16991, 17058, 17126, 17194, > + 17261, 17330, 17398, 17467, 17535, 17604, 17673, 17742, 17812, 17881, > + 17951, 18021, 18091, 18162, 18232, 18303, 18374, 18445, 18516, 18588, > + 18659, 18731, 18803, 18875, 18947, 19020, 19093, 19166, 19239, 19312, > + 19385, 19459, 19533, 19607, 19681, 19755, 19830, 19905, 19980, 20055, > + 20130, 20206, 20281, 20357, 20433, 20510, 20586, 20663, 20740, 20817, > + 20894, 20971, 21049, 21127, 21205, 21283, 21361, 21440, 21519, 21598, > + 21677, 21756, 21836, 21915, 21995, 22075, 22156, 22236, 22317, 22398, > + 22479, 22560, 22642, 22723, 22805, 22887, 22969, 23052, 23135, 23217, > + 23300, 23384, 23467, 23551, 23635, 23719, 23803, 23887, 23972, 24057, > + 24142, 24227, 24313, 24398, 24484, 24570, 24656, 24743, 24829, 24916, > + 25003, 25091, 25178, 25266, 25354, 25442, 25530, 25618, 25707, 25796, > + 25885, 25974, 26064, 26153, 26243, 26334, 26424, 26514, 26605, 26696, > + 26787, 26879, 26970, 27062, 27154, 27246, 27338, 27431, 27524, 27617, > + 27710, 27804, 27897, 27991, 28085, 28179, 28274, 28369, 28463, 28559, > + 28654, 28749, 28845, 28941, 29037, 29134, 29230, 29327, 29424, 29521, > + 29619, 29717, 29815, 29913, 30011, 30110, 30208, 30307, 30406, 30506, > + 30605, 30705, 30805, 30906, 31006, 31107, 31208, 31309, 31410, 31512, > + 31614, 31716, 31818, 31920, 32023, 32126, 32229, 32332, 32436, 32540, > + 32644, 32748, 32852, 32957, 33062, 33167, 33272, 33378, 33484, 33590, > + 33696, 33802, 33909, 34016, 34123, 34230, 34338, 34446, 34554, 34662, > + 34770, 34879, 34988, 35097, 35206, 35316, 35426, 35536, 35646, 35757, > + 35867, 35978, 36090, 36201, 36313, 36425, 36537, 36649, 36762, 36874, > + 36987, 37101, 37214, 37328, 37442, 37556, 37671, 37785, 37900, 38015, > + 38131, 38246, 38362, 38478, 38594, 38711, 38828, 38945, 39062, 39179, > + 39297, 39415, 39533, 39651, 39770, 39889, 40008, 40127, 40247, 40367, > + 40487, 40607, 40728, 40848, 40969, 41091, 41212, 41334, 41456, 41578, > + 41700, 41823, 41946, 42069, 42193, 42316, 42440, 42564, 42689, 42813, > + 42938, 43063, 43189, 43314, 43440, 43566, 43692, 43819, 43946, 44073, > + 44200, 44328, 44456, 44584, 44712, 44840, 44969, 45098, 45227, 45357, > + 45487, 45617, 45747, 45877, 46008, 46139, 46270, 46402, 46534, 46666, > + 46798, 46930, 47063, 47196, 47329, 47463, 47596, 47730, 47865, 47999, > + 48134, 48269, 48404, 48540, 48675, 48811, 48948, 49084, 49221, 49358, > + 49495, 49633, 49771, 49909, 50047, 50185, 50324, 50463, 50603, 50742, > + 50882, 51022, 51162, 51303, 51444, 51585, 51726, 51868, 52010, 52152, > + 52294, 52437, 52580, 52723, 52867, 53010, 53154, 53299, 53443, 53588, > + 53733, 53878, 54024, 54169, 54315, 54462, 54608, 54755, 54902, 55050, > + 55197, 55345, 55493, 55642, 55790, 55939, 56089, 56238, 56388, 56538, > + 56688, 56839, 56989, 57140, 57292, 57443, 57595, 57747, 57900, 58053, > + 58205, 58359, 58512, 58666, 58820, 58974, 59129, 59284, 59439, 59594, > + 59750, 59906, 60062, 60218, 60375, 60532, 60689, 60847, 61005, 61163, > + 61321, 61480, 61639, 61798, 61957, 62117, 62277, 62437, 62598, 62759, > + 62920, 63081, 63243, 63405, 63567, 63729, 63892, 64055, 64219, 64382, > + 64546, 64710, 64875, 65039, 65204, 65369, 65535, > +}; > + > struct pwm_bl_data { > struct pwm_device *pwm; > struct device *dev; > @@ -38,6 +144,7 @@ struct pwm_bl_data { > unsigned int scale; > bool legacy; > bool piecewise; > + bool use_lth_table; Again, I didn't think we'd need to special case the lookup table except in the probe method. It's "just" a built-in levels table (ideally reinforced with with code to figure out how many steps to interpolate). > int (*notify)(struct device *, > int brightness); > void (*notify_after)(struct device *, > @@ -104,6 +211,8 @@ static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > } else { > duty_cycle = scale(pb, pb->levels[brightness]); > } > + } else if (pb->use_lth_table) { > + duty_cycle = cie1931_table[brightness]; > } else { > duty_cycle = scale(pb, brightness); > } > @@ -169,9 +278,9 @@ static int pwm_backlight_parse_dt(struct device *dev, > /* determine the number of brightness levels */ > prop = of_find_property(node, "brightness-levels", &length); > if (!prop) > - return -EINVAL; > - > - data->levels_count = length / sizeof(u32); > + data->use_lth_table = true; > + else > + data->levels_count = length / sizeof(u32); > > /* read brightness levels from DT property */ > if (data->levels_count > 0) { > @@ -181,24 +290,28 @@ static int pwm_backlight_parse_dt(struct device *dev, > if (!data->levels) > return -ENOMEM; > > - ret = of_property_read_u32_array(node, "brightness-levels", > - data->levels, > - data->levels_count); > - if (ret < 0) > - return ret; > - > - ret = of_property_read_u32(node, "default-brightness-level", > - &value); > - if (ret < 0) > - return ret; > + if (prop) { > + ret = of_property_read_u32_array(node, > + "brightness-levels", > + data->levels, > + data->levels_count); > + if (ret < 0) > + return ret; > + } > > data->piecewise = of_property_read_bool(node, > "use-linear-interpolation"); > > - data->dft_brightness = value; > data->levels_count--; > } > > + ret = of_property_read_u32(node, "default-brightness-level", > + &value); > + if (ret < 0) > + return ret; > + > + data->dft_brightness = value; > + > if (data->piecewise) > data->max_brightness = data->levels_count * NSTEPS; > else > @@ -260,8 +373,10 @@ static int pwm_backlight_probe(struct platform_device *pdev) > struct backlight_device *bl; > struct device_node *node = pdev->dev.of_node; > struct pwm_bl_data *pb; > + struct pwm_state state; > struct pwm_args pargs; > int ret; > + int i; > > if (!data) { > ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); > @@ -303,6 +418,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) > pb->dev = &pdev->dev; > pb->enabled = false; > pb->piecewise = data->piecewise; > + pb->use_lth_table = data->use_lth_table; > > pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", > GPIOD_ASIS); > @@ -361,6 +477,22 @@ static int pwm_backlight_probe(struct platform_device *pdev) > > dev_dbg(&pdev->dev, "got pwm for backlight\n"); > > + if (pb->use_lth_table) { > + /* Get PWM resolution */ > + pwm_get_state(pb->pwm, &state); > + if (state.period > 65535) { > + dev_err(&pdev->dev, "PWM resolution not supported\n"); > + goto err_alloc; > + } > + /* Find the number of steps based on the PWM resolution */ > + for (i = 0; i < ARRAY_SIZE(cie1931_table); i++) > + if (cie1931_table[i] >= state.period) { > + data->max_brightness = i; > + break; > + } > + pb->scale = data->max_brightness; > + } > + > /* > * FIXME: pwm_apply_args() should be removed when switching to > * the atomic PWM API. > diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h > index 444a91b..4ec3b0d 100644 > --- a/include/linux/pwm_backlight.h > +++ b/include/linux/pwm_backlight.h > @@ -15,6 +15,7 @@ struct platform_pwm_backlight_data { > unsigned int pwm_period_ns; > unsigned int *levels; > unsigned int levels_count; > + bool use_lth_table; Why not just "if (!levels)"? > bool piecewise; > /* TODO remove once all users are switched to gpiod_* API */ > int enable_gpio; > -- > 2.9.3 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! > 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] Can we just generate the table on the fly? Should not be hard to do in fixed point, right? Pavel
Hi Daniel, 2017-12-15 15:51 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>: > On Thu, Nov 16, 2017 at 03:11:51PM +0100, Enric Balletbo i Serra 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. >> >> 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. >> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> drivers/video/backlight/pwm_bl.c | 160 +++++++++++++++++++++++++++++++++++---- >> include/linux/pwm_backlight.h | 1 + >> 2 files changed, 147 insertions(+), 14 deletions(-) >> >> 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. >> + */ >> +const unsigned int cie1931_table[1024] = { > > I'm a little surprised to see such a large table. I thought we'd be able > to use the linear interpolation logic to smooth a smaller table. > oh, I didn't catch that you wanted use linear interpolation for that table too, that table is directly the output of the CIE 1931 algorithm. And yes 1024 step looks like lots of steps but based on the tests 1024 steps for a 16 bit resolution PWM is reasonable, I guess (though it's a personal perception and opinion as I don't know how to calculate the number of really needed steps). > >> + 0, 7, 14, 21, 28, 35, 43, 50, 57, 64, 71, 78, 85, 92, 99, 106, 114, 121, >> + 128, 135, 142, 149, 156, 163, 170, 177, 185, 192, 199, 206, 213, 220, >> + 227, 234, 241, 248, 256, 263, 270, 277, 284, 291, 298, 305, 312, 319, >> + 327, 334, 341, 348, 355, 362, 369, 376, 383, 390, 398, 405, 412, 419, >> + 426, 433, 440, 447, 454, 461, 469, 476, 483, 490, 497, 504, 511, 518, >> + 525, 532, 540, 547, 554, 561, 568, 575, 582, 589, 596, 603, 610, 618, >> + 625, 633, 640, 648, 655, 663, 671, 679, 687, 695, 703, 711, 719, 727, >> + 735, 744, 752, 761, 769, 778, 786, 795, 804, 813, 822, 831, 840, 849, >> + 858, 867, 876, 886, 895, 905, 914, 924, 934, 943, 953, 963, 973, 983, >> + 993, 1004, 1014, 1024, 1034, 1045, 1055, 1066, 1077, 1087, 1098, 1109, >> + 1120, 1131, 1142, 1153, 1165, 1176, 1187, 1199, 1210, 1222, 1234, 1245, >> + 1257, 1269, 1281, 1293, 1305, 1318, 1330, 1342, 1355, 1367, 1380, 1392, >> + 1405, 1418, 1431, 1444, 1457, 1470, 1483, 1497, 1510, 1523, 1537, 1551, >> + 1564, 1578, 1592, 1606, 1620, 1634, 1648, 1662, 1677, 1691, 1706, 1720, >> + 1735, 1750, 1765, 1780, 1795, 1810, 1825, 1840, 1855, 1871, 1886, 1902, >> + 1918, 1933, 1949, 1965, 1981, 1997, 2014, 2030, 2046, 2063, 2079, 2096, >> + 2113, 2130, 2146, 2163, 2181, 2198, 2215, 2232, 2250, 2267, 2285, 2303, >> + 2321, 2338, 2356, 2375, 2393, 2411, 2429, 2448, 2466, 2485, 2504, 2523, >> + 2542, 2561, 2580, 2599, 2618, 2638, 2657, 2677, 2697, 2716, 2736, 2756, >> + 2776, 2796, 2817, 2837, 2858, 2878, 2899, 2920, 2941, 2961, 2983, 3004, >> + 3025, 3046, 3068, 3089, 3111, 3133, 3155, 3177, 3199, 3221, 3243, 3266, >> + 3288, 3311, 3333, 3356, 3379, 3402, 3425, 3448, 3472, 3495, 3519, 3542, >> + 3566, 3590, 3614, 3638, 3662, 3686, 3711, 3735, 3760, 3784, 3809, 3834, >> + 3859, 3884, 3910, 3935, 3960, 3986, 4012, 4037, 4063, 4089, 4115, 4142, >> + 4168, 4194, 4221, 4248, 4274, 4301, 4328, 4356, 4383, 4410, 4438, 4465, >> + 4493, 4521, 4549, 4577, 4605, 4633, 4661, 4690, 4719, 4747, 4776, 4805, >> + 4834, 4863, 4893, 4922, 4952, 4981, 5011, 5041, 5071, 5101, 5131, 5162, >> + 5192, 5223, 5254, 5285, 5316, 5347, 5378, 5409, 5441, 5472, 5504, 5536, >> + 5568, 5600, 5632, 5664, 5697, 5729, 5762, 5795, 5828, 5861, 5894, 5928, >> + 5961, 5995, 6028, 6062, 6096, 6130, 6164, 6199, 6233, 6268, 6302, 6337, >> + 6372, 6407, 6442, 6478, 6513, 6549, 6585, 6621, 6657, 6693, 6729, 6765, >> + 6802, 6839, 6875, 6912, 6949, 6986, 7024, 7061, 7099, 7137, 7174, 7212, >> + 7250, 7289, 7327, 7366, 7404, 7443, 7482, 7521, 7560, 7600, 7639, 7679, >> + 7718, 7758, 7798, 7838, 7879, 7919, 7960, 8000, 8041, 8082, 8123, 8165, >> + 8206, 8248, 8289, 8331, 8373, 8415, 8457, 8500, 8542, 8585, 8628, 8671, >> + 8714, 8757, 8800, 8844, 8887, 8931, 8975, 9019, 9064, 9108, 9152, 9197, >> + 9242, 9287, 9332, 9377, 9423, 9468, 9514, 9560, 9606, 9652, 9698, 9745, >> + 9791, 9838, 9885, 9932, 9979, 10026, 10074, 10121, 10169, 10217, 10265, >> + 10313, 10362, 10410, 10459, 10508, 10557, 10606, 10655, 10704, 10754, >> + 10804, 10854, 10904, 10954, 11004, 11055, 11105, 11156, 11207, 11258, >> + 11310, 11361, 11413, 11464, 11516, 11568, 11620, 11673, 11725, 11778, >> + 11831, 11884, 11937, 11990, 12044, 12097, 12151, 12205, 12259, 12314, >> + 12368, 12423, 12477, 12532, 12587, 12643, 12698, 12754, 12809, 12865, >> + 12921, 12977, 13034, 13090, 13147, 13204, 13261, 13318, 13375, 13433, >> + 13491, 13548, 13606, 13665, 13723, 13781, 13840, 13899, 13958, 14017, >> + 14077, 14136, 14196, 14256, 14316, 14376, 14436, 14497, 14557, 14618, >> + 14679, 14740, 14802, 14863, 14925, 14987, 15049, 15111, 15173, 15236, >> + 15299, 15362, 15425, 15488, 15551, 15615, 15679, 15743, 15807, 15871, >> + 15935, 16000, 16065, 16130, 16195, 16260, 16326, 16392, 16457, 16523, >> + 16590, 16656, 16723, 16789, 16856, 16923, 16991, 17058, 17126, 17194, >> + 17261, 17330, 17398, 17467, 17535, 17604, 17673, 17742, 17812, 17881, >> + 17951, 18021, 18091, 18162, 18232, 18303, 18374, 18445, 18516, 18588, >> + 18659, 18731, 18803, 18875, 18947, 19020, 19093, 19166, 19239, 19312, >> + 19385, 19459, 19533, 19607, 19681, 19755, 19830, 19905, 19980, 20055, >> + 20130, 20206, 20281, 20357, 20433, 20510, 20586, 20663, 20740, 20817, >> + 20894, 20971, 21049, 21127, 21205, 21283, 21361, 21440, 21519, 21598, >> + 21677, 21756, 21836, 21915, 21995, 22075, 22156, 22236, 22317, 22398, >> + 22479, 22560, 22642, 22723, 22805, 22887, 22969, 23052, 23135, 23217, >> + 23300, 23384, 23467, 23551, 23635, 23719, 23803, 23887, 23972, 24057, >> + 24142, 24227, 24313, 24398, 24484, 24570, 24656, 24743, 24829, 24916, >> + 25003, 25091, 25178, 25266, 25354, 25442, 25530, 25618, 25707, 25796, >> + 25885, 25974, 26064, 26153, 26243, 26334, 26424, 26514, 26605, 26696, >> + 26787, 26879, 26970, 27062, 27154, 27246, 27338, 27431, 27524, 27617, >> + 27710, 27804, 27897, 27991, 28085, 28179, 28274, 28369, 28463, 28559, >> + 28654, 28749, 28845, 28941, 29037, 29134, 29230, 29327, 29424, 29521, >> + 29619, 29717, 29815, 29913, 30011, 30110, 30208, 30307, 30406, 30506, >> + 30605, 30705, 30805, 30906, 31006, 31107, 31208, 31309, 31410, 31512, >> + 31614, 31716, 31818, 31920, 32023, 32126, 32229, 32332, 32436, 32540, >> + 32644, 32748, 32852, 32957, 33062, 33167, 33272, 33378, 33484, 33590, >> + 33696, 33802, 33909, 34016, 34123, 34230, 34338, 34446, 34554, 34662, >> + 34770, 34879, 34988, 35097, 35206, 35316, 35426, 35536, 35646, 35757, >> + 35867, 35978, 36090, 36201, 36313, 36425, 36537, 36649, 36762, 36874, >> + 36987, 37101, 37214, 37328, 37442, 37556, 37671, 37785, 37900, 38015, >> + 38131, 38246, 38362, 38478, 38594, 38711, 38828, 38945, 39062, 39179, >> + 39297, 39415, 39533, 39651, 39770, 39889, 40008, 40127, 40247, 40367, >> + 40487, 40607, 40728, 40848, 40969, 41091, 41212, 41334, 41456, 41578, >> + 41700, 41823, 41946, 42069, 42193, 42316, 42440, 42564, 42689, 42813, >> + 42938, 43063, 43189, 43314, 43440, 43566, 43692, 43819, 43946, 44073, >> + 44200, 44328, 44456, 44584, 44712, 44840, 44969, 45098, 45227, 45357, >> + 45487, 45617, 45747, 45877, 46008, 46139, 46270, 46402, 46534, 46666, >> + 46798, 46930, 47063, 47196, 47329, 47463, 47596, 47730, 47865, 47999, >> + 48134, 48269, 48404, 48540, 48675, 48811, 48948, 49084, 49221, 49358, >> + 49495, 49633, 49771, 49909, 50047, 50185, 50324, 50463, 50603, 50742, >> + 50882, 51022, 51162, 51303, 51444, 51585, 51726, 51868, 52010, 52152, >> + 52294, 52437, 52580, 52723, 52867, 53010, 53154, 53299, 53443, 53588, >> + 53733, 53878, 54024, 54169, 54315, 54462, 54608, 54755, 54902, 55050, >> + 55197, 55345, 55493, 55642, 55790, 55939, 56089, 56238, 56388, 56538, >> + 56688, 56839, 56989, 57140, 57292, 57443, 57595, 57747, 57900, 58053, >> + 58205, 58359, 58512, 58666, 58820, 58974, 59129, 59284, 59439, 59594, >> + 59750, 59906, 60062, 60218, 60375, 60532, 60689, 60847, 61005, 61163, >> + 61321, 61480, 61639, 61798, 61957, 62117, 62277, 62437, 62598, 62759, >> + 62920, 63081, 63243, 63405, 63567, 63729, 63892, 64055, 64219, 64382, >> + 64546, 64710, 64875, 65039, 65204, 65369, 65535, >> +}; >> + >> struct pwm_bl_data { >> struct pwm_device *pwm; >> struct device *dev; >> @@ -38,6 +144,7 @@ struct pwm_bl_data { >> unsigned int scale; >> bool legacy; >> bool piecewise; >> + bool use_lth_table; > > Again, I didn't think we'd need to special case the lookup table except > in the probe method. It's "just" a built-in levels table (ideally > reinforced with with code to figure out how many steps to interpolate). > Ok. > >> int (*notify)(struct device *, >> int brightness); >> void (*notify_after)(struct device *, >> @@ -104,6 +211,8 @@ static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) >> } else { >> duty_cycle = scale(pb, pb->levels[brightness]); >> } >> + } else if (pb->use_lth_table) { >> + duty_cycle = cie1931_table[brightness]; >> } else { >> duty_cycle = scale(pb, brightness); >> } >> @@ -169,9 +278,9 @@ static int pwm_backlight_parse_dt(struct device *dev, >> /* determine the number of brightness levels */ >> prop = of_find_property(node, "brightness-levels", &length); >> if (!prop) >> - return -EINVAL; >> - >> - data->levels_count = length / sizeof(u32); >> + data->use_lth_table = true; >> + else >> + data->levels_count = length / sizeof(u32); >> >> /* read brightness levels from DT property */ >> if (data->levels_count > 0) { >> @@ -181,24 +290,28 @@ static int pwm_backlight_parse_dt(struct device *dev, >> if (!data->levels) >> return -ENOMEM; >> >> - ret = of_property_read_u32_array(node, "brightness-levels", >> - data->levels, >> - data->levels_count); >> - if (ret < 0) >> - return ret; >> - >> - ret = of_property_read_u32(node, "default-brightness-level", >> - &value); >> - if (ret < 0) >> - return ret; >> + if (prop) { >> + ret = of_property_read_u32_array(node, >> + "brightness-levels", >> + data->levels, >> + data->levels_count); >> + if (ret < 0) >> + return ret; >> + } >> >> data->piecewise = of_property_read_bool(node, >> "use-linear-interpolation"); >> >> - data->dft_brightness = value; >> data->levels_count--; >> } >> >> + ret = of_property_read_u32(node, "default-brightness-level", >> + &value); >> + if (ret < 0) >> + return ret; >> + >> + data->dft_brightness = value; >> + >> if (data->piecewise) >> data->max_brightness = data->levels_count * NSTEPS; >> else >> @@ -260,8 +373,10 @@ static int pwm_backlight_probe(struct platform_device *pdev) >> struct backlight_device *bl; >> struct device_node *node = pdev->dev.of_node; >> struct pwm_bl_data *pb; >> + struct pwm_state state; >> struct pwm_args pargs; >> int ret; >> + int i; >> >> if (!data) { >> ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); >> @@ -303,6 +418,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) >> pb->dev = &pdev->dev; >> pb->enabled = false; >> pb->piecewise = data->piecewise; >> + pb->use_lth_table = data->use_lth_table; >> >> pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", >> GPIOD_ASIS); >> @@ -361,6 +477,22 @@ static int pwm_backlight_probe(struct platform_device *pdev) >> >> dev_dbg(&pdev->dev, "got pwm for backlight\n"); >> >> + if (pb->use_lth_table) { >> + /* Get PWM resolution */ >> + pwm_get_state(pb->pwm, &state); >> + if (state.period > 65535) { >> + dev_err(&pdev->dev, "PWM resolution not supported\n"); >> + goto err_alloc; >> + } >> + /* Find the number of steps based on the PWM resolution */ >> + for (i = 0; i < ARRAY_SIZE(cie1931_table); i++) >> + if (cie1931_table[i] >= state.period) { >> + data->max_brightness = i; >> + break; >> + } >> + pb->scale = data->max_brightness; >> + } >> + >> /* >> * FIXME: pwm_apply_args() should be removed when switching to >> * the atomic PWM API. >> diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h >> index 444a91b..4ec3b0d 100644 >> --- a/include/linux/pwm_backlight.h >> +++ b/include/linux/pwm_backlight.h >> @@ -15,6 +15,7 @@ struct platform_pwm_backlight_data { >> unsigned int pwm_period_ns; >> unsigned int *levels; >> unsigned int levels_count; >> + bool use_lth_table; > > Why not just "if (!levels)"? > Yep, should work. >> bool piecewise; >> /* TODO remove once all users are switched to gpiod_* API */ >> int enable_gpio; >> -- >> 2.9.3 >> If it's ok I'll send a first version (no RFC) of the patchet adding your comments. Thanks, Enric -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Pavel, 2017-12-15 21:57 GMT+01:00 Pavel Machek <pavel@ucw.cz>: > Hi! > >> 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] > > Can we just generate the table on the fly? Should not be hard to do in > fixed point, right? This was discussed a bit in previous RFC which had the code to generate the table on the fly, see [1]. The use of a fixed table or an on the fly table is something that I'll let the maintainers to decide. I've no strong opinion on use the on the fly table if someone takes care to review deeply the fixed point maths :) [1] https://lkml.org/lkml/2017/9/4/335 Regards, Enric > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 2017-12-18 11:40:59, Enric Balletbo Serra wrote: > Hi Pavel, > > 2017-12-15 21:57 GMT+01:00 Pavel Machek <pavel@ucw.cz>: > > Hi! > > > >> 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] > > > > Can we just generate the table on the fly? Should not be hard to do in > > fixed point, right? > > This was discussed a bit in previous RFC which had the code to > generate the table on the fly, see [1]. The use of a fixed table or an > on the fly table is something that I'll let the maintainers to decide. > I've no strong opinion on use the on the fly table if someone takes > care to review deeply the fixed point maths :) You are free to pre-compute the table at boot. And you can even compare the built-in and pre-computed table at boot, to make sure you made no mistakes :-). Pavel
On Mon, Dec 18, 2017 at 11:27:22AM +0100, Enric Balletbo Serra wrote: > Hi Daniel, > > 2017-12-15 15:51 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>: > > On Thu, Nov 16, 2017 at 03:11:51PM +0100, Enric Balletbo i Serra 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. > >> > >> 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. > >> > >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > >> --- > >> drivers/video/backlight/pwm_bl.c | 160 +++++++++++++++++++++++++++++++++++---- > >> include/linux/pwm_backlight.h | 1 + > >> 2 files changed, 147 insertions(+), 14 deletions(-) > >> > >> 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. > >> + */ > >> +const unsigned int cie1931_table[1024] = { > > > > I'm a little surprised to see such a large table. I thought we'd be able > > to use the linear interpolation logic to smooth a smaller table. > > > > oh, I didn't catch that you wanted use linear interpolation for that > table too, that table is directly the output of the CIE 1931 > algorithm. And yes 1024 step looks like lots of steps but based on > the tests 1024 steps for a 16 bit resolution PWM is reasonable, I > guess (though it's a personal perception and opinion as I don't know > how to calculate the number of really needed steps). I think two different values on the userspace side should always map to different values on the kernel side. This should make it possible to calculate the maximal number of steps by scaling up the table to the PWM resolution and then scanning for the smallest interval between table steps. Once we have a maximal value we could either use it directly or we might want to push it through min(calculated_max, 1024), on the assumption that even for animated changes to backlight level that 1024 is a sensible limit[1] [1] I think it is... I'd be interested to know of Doug A. shares this view.. > > > >> + 0, 7, 14, 21, 28, 35, 43, 50, 57, 64, 71, 78, 85, 92, 99, 106, 114, 121, > >> + 128, 135, 142, 149, 156, 163, 170, 177, 185, 192, 199, 206, 213, 220, > >> + 227, 234, 241, 248, 256, 263, 270, 277, 284, 291, 298, 305, 312, 319, > >> + 327, 334, 341, 348, 355, 362, 369, 376, 383, 390, 398, 405, 412, 419, > >> + 426, 433, 440, 447, 454, 461, 469, 476, 483, 490, 497, 504, 511, 518, > >> + 525, 532, 540, 547, 554, 561, 568, 575, 582, 589, 596, 603, 610, 618, > >> + 625, 633, 640, 648, 655, 663, 671, 679, 687, 695, 703, 711, 719, 727, > >> + 735, 744, 752, 761, 769, 778, 786, 795, 804, 813, 822, 831, 840, 849, > >> + 858, 867, 876, 886, 895, 905, 914, 924, 934, 943, 953, 963, 973, 983, > >> + 993, 1004, 1014, 1024, 1034, 1045, 1055, 1066, 1077, 1087, 1098, 1109, > >> + 1120, 1131, 1142, 1153, 1165, 1176, 1187, 1199, 1210, 1222, 1234, 1245, > >> + 1257, 1269, 1281, 1293, 1305, 1318, 1330, 1342, 1355, 1367, 1380, 1392, > >> + 1405, 1418, 1431, 1444, 1457, 1470, 1483, 1497, 1510, 1523, 1537, 1551, > >> + 1564, 1578, 1592, 1606, 1620, 1634, 1648, 1662, 1677, 1691, 1706, 1720, > >> + 1735, 1750, 1765, 1780, 1795, 1810, 1825, 1840, 1855, 1871, 1886, 1902, > >> + 1918, 1933, 1949, 1965, 1981, 1997, 2014, 2030, 2046, 2063, 2079, 2096, > >> + 2113, 2130, 2146, 2163, 2181, 2198, 2215, 2232, 2250, 2267, 2285, 2303, > >> + 2321, 2338, 2356, 2375, 2393, 2411, 2429, 2448, 2466, 2485, 2504, 2523, > >> + 2542, 2561, 2580, 2599, 2618, 2638, 2657, 2677, 2697, 2716, 2736, 2756, > >> + 2776, 2796, 2817, 2837, 2858, 2878, 2899, 2920, 2941, 2961, 2983, 3004, > >> + 3025, 3046, 3068, 3089, 3111, 3133, 3155, 3177, 3199, 3221, 3243, 3266, > >> + 3288, 3311, 3333, 3356, 3379, 3402, 3425, 3448, 3472, 3495, 3519, 3542, > >> + 3566, 3590, 3614, 3638, 3662, 3686, 3711, 3735, 3760, 3784, 3809, 3834, > >> + 3859, 3884, 3910, 3935, 3960, 3986, 4012, 4037, 4063, 4089, 4115, 4142, > >> + 4168, 4194, 4221, 4248, 4274, 4301, 4328, 4356, 4383, 4410, 4438, 4465, > >> + 4493, 4521, 4549, 4577, 4605, 4633, 4661, 4690, 4719, 4747, 4776, 4805, > >> + 4834, 4863, 4893, 4922, 4952, 4981, 5011, 5041, 5071, 5101, 5131, 5162, > >> + 5192, 5223, 5254, 5285, 5316, 5347, 5378, 5409, 5441, 5472, 5504, 5536, > >> + 5568, 5600, 5632, 5664, 5697, 5729, 5762, 5795, 5828, 5861, 5894, 5928, > >> + 5961, 5995, 6028, 6062, 6096, 6130, 6164, 6199, 6233, 6268, 6302, 6337, > >> + 6372, 6407, 6442, 6478, 6513, 6549, 6585, 6621, 6657, 6693, 6729, 6765, > >> + 6802, 6839, 6875, 6912, 6949, 6986, 7024, 7061, 7099, 7137, 7174, 7212, > >> + 7250, 7289, 7327, 7366, 7404, 7443, 7482, 7521, 7560, 7600, 7639, 7679, > >> + 7718, 7758, 7798, 7838, 7879, 7919, 7960, 8000, 8041, 8082, 8123, 8165, > >> + 8206, 8248, 8289, 8331, 8373, 8415, 8457, 8500, 8542, 8585, 8628, 8671, > >> + 8714, 8757, 8800, 8844, 8887, 8931, 8975, 9019, 9064, 9108, 9152, 9197, > >> + 9242, 9287, 9332, 9377, 9423, 9468, 9514, 9560, 9606, 9652, 9698, 9745, > >> + 9791, 9838, 9885, 9932, 9979, 10026, 10074, 10121, 10169, 10217, 10265, > >> + 10313, 10362, 10410, 10459, 10508, 10557, 10606, 10655, 10704, 10754, > >> + 10804, 10854, 10904, 10954, 11004, 11055, 11105, 11156, 11207, 11258, > >> + 11310, 11361, 11413, 11464, 11516, 11568, 11620, 11673, 11725, 11778, > >> + 11831, 11884, 11937, 11990, 12044, 12097, 12151, 12205, 12259, 12314, > >> + 12368, 12423, 12477, 12532, 12587, 12643, 12698, 12754, 12809, 12865, > >> + 12921, 12977, 13034, 13090, 13147, 13204, 13261, 13318, 13375, 13433, > >> + 13491, 13548, 13606, 13665, 13723, 13781, 13840, 13899, 13958, 14017, > >> + 14077, 14136, 14196, 14256, 14316, 14376, 14436, 14497, 14557, 14618, > >> + 14679, 14740, 14802, 14863, 14925, 14987, 15049, 15111, 15173, 15236, > >> + 15299, 15362, 15425, 15488, 15551, 15615, 15679, 15743, 15807, 15871, > >> + 15935, 16000, 16065, 16130, 16195, 16260, 16326, 16392, 16457, 16523, > >> + 16590, 16656, 16723, 16789, 16856, 16923, 16991, 17058, 17126, 17194, > >> + 17261, 17330, 17398, 17467, 17535, 17604, 17673, 17742, 17812, 17881, > >> + 17951, 18021, 18091, 18162, 18232, 18303, 18374, 18445, 18516, 18588, > >> + 18659, 18731, 18803, 18875, 18947, 19020, 19093, 19166, 19239, 19312, > >> + 19385, 19459, 19533, 19607, 19681, 19755, 19830, 19905, 19980, 20055, > >> + 20130, 20206, 20281, 20357, 20433, 20510, 20586, 20663, 20740, 20817, > >> + 20894, 20971, 21049, 21127, 21205, 21283, 21361, 21440, 21519, 21598, > >> + 21677, 21756, 21836, 21915, 21995, 22075, 22156, 22236, 22317, 22398, > >> + 22479, 22560, 22642, 22723, 22805, 22887, 22969, 23052, 23135, 23217, > >> + 23300, 23384, 23467, 23551, 23635, 23719, 23803, 23887, 23972, 24057, > >> + 24142, 24227, 24313, 24398, 24484, 24570, 24656, 24743, 24829, 24916, > >> + 25003, 25091, 25178, 25266, 25354, 25442, 25530, 25618, 25707, 25796, > >> + 25885, 25974, 26064, 26153, 26243, 26334, 26424, 26514, 26605, 26696, > >> + 26787, 26879, 26970, 27062, 27154, 27246, 27338, 27431, 27524, 27617, > >> + 27710, 27804, 27897, 27991, 28085, 28179, 28274, 28369, 28463, 28559, > >> + 28654, 28749, 28845, 28941, 29037, 29134, 29230, 29327, 29424, 29521, > >> + 29619, 29717, 29815, 29913, 30011, 30110, 30208, 30307, 30406, 30506, > >> + 30605, 30705, 30805, 30906, 31006, 31107, 31208, 31309, 31410, 31512, > >> + 31614, 31716, 31818, 31920, 32023, 32126, 32229, 32332, 32436, 32540, > >> + 32644, 32748, 32852, 32957, 33062, 33167, 33272, 33378, 33484, 33590, > >> + 33696, 33802, 33909, 34016, 34123, 34230, 34338, 34446, 34554, 34662, > >> + 34770, 34879, 34988, 35097, 35206, 35316, 35426, 35536, 35646, 35757, > >> + 35867, 35978, 36090, 36201, 36313, 36425, 36537, 36649, 36762, 36874, > >> + 36987, 37101, 37214, 37328, 37442, 37556, 37671, 37785, 37900, 38015, > >> + 38131, 38246, 38362, 38478, 38594, 38711, 38828, 38945, 39062, 39179, > >> + 39297, 39415, 39533, 39651, 39770, 39889, 40008, 40127, 40247, 40367, > >> + 40487, 40607, 40728, 40848, 40969, 41091, 41212, 41334, 41456, 41578, > >> + 41700, 41823, 41946, 42069, 42193, 42316, 42440, 42564, 42689, 42813, > >> + 42938, 43063, 43189, 43314, 43440, 43566, 43692, 43819, 43946, 44073, > >> + 44200, 44328, 44456, 44584, 44712, 44840, 44969, 45098, 45227, 45357, > >> + 45487, 45617, 45747, 45877, 46008, 46139, 46270, 46402, 46534, 46666, > >> + 46798, 46930, 47063, 47196, 47329, 47463, 47596, 47730, 47865, 47999, > >> + 48134, 48269, 48404, 48540, 48675, 48811, 48948, 49084, 49221, 49358, > >> + 49495, 49633, 49771, 49909, 50047, 50185, 50324, 50463, 50603, 50742, > >> + 50882, 51022, 51162, 51303, 51444, 51585, 51726, 51868, 52010, 52152, > >> + 52294, 52437, 52580, 52723, 52867, 53010, 53154, 53299, 53443, 53588, > >> + 53733, 53878, 54024, 54169, 54315, 54462, 54608, 54755, 54902, 55050, > >> + 55197, 55345, 55493, 55642, 55790, 55939, 56089, 56238, 56388, 56538, > >> + 56688, 56839, 56989, 57140, 57292, 57443, 57595, 57747, 57900, 58053, > >> + 58205, 58359, 58512, 58666, 58820, 58974, 59129, 59284, 59439, 59594, > >> + 59750, 59906, 60062, 60218, 60375, 60532, 60689, 60847, 61005, 61163, > >> + 61321, 61480, 61639, 61798, 61957, 62117, 62277, 62437, 62598, 62759, > >> + 62920, 63081, 63243, 63405, 63567, 63729, 63892, 64055, 64219, 64382, > >> + 64546, 64710, 64875, 65039, 65204, 65369, 65535, > >> +}; > >> + > >> struct pwm_bl_data { > >> struct pwm_device *pwm; > >> struct device *dev; > >> @@ -38,6 +144,7 @@ struct pwm_bl_data { > >> unsigned int scale; > >> bool legacy; > >> bool piecewise; > >> + bool use_lth_table; > > > > Again, I didn't think we'd need to special case the lookup table except > > in the probe method. It's "just" a built-in levels table (ideally > > reinforced with with code to figure out how many steps to interpolate). > > > > Ok. > > > > >> int (*notify)(struct device *, > >> int brightness); > >> void (*notify_after)(struct device *, > >> @@ -104,6 +211,8 @@ static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness) > >> } else { > >> duty_cycle = scale(pb, pb->levels[brightness]); > >> } > >> + } else if (pb->use_lth_table) { > >> + duty_cycle = cie1931_table[brightness]; > >> } else { > >> duty_cycle = scale(pb, brightness); > >> } > >> @@ -169,9 +278,9 @@ static int pwm_backlight_parse_dt(struct device *dev, > >> /* determine the number of brightness levels */ > >> prop = of_find_property(node, "brightness-levels", &length); > >> if (!prop) > >> - return -EINVAL; > >> - > >> - data->levels_count = length / sizeof(u32); > >> + data->use_lth_table = true; > >> + else > >> + data->levels_count = length / sizeof(u32); > >> > >> /* read brightness levels from DT property */ > >> if (data->levels_count > 0) { > >> @@ -181,24 +290,28 @@ static int pwm_backlight_parse_dt(struct device *dev, > >> if (!data->levels) > >> return -ENOMEM; > >> > >> - ret = of_property_read_u32_array(node, "brightness-levels", > >> - data->levels, > >> - data->levels_count); > >> - if (ret < 0) > >> - return ret; > >> - > >> - ret = of_property_read_u32(node, "default-brightness-level", > >> - &value); > >> - if (ret < 0) > >> - return ret; > >> + if (prop) { > >> + ret = of_property_read_u32_array(node, > >> + "brightness-levels", > >> + data->levels, > >> + data->levels_count); > >> + if (ret < 0) > >> + return ret; > >> + } > >> > >> data->piecewise = of_property_read_bool(node, > >> "use-linear-interpolation"); > >> > >> - data->dft_brightness = value; > >> data->levels_count--; > >> } > >> > >> + ret = of_property_read_u32(node, "default-brightness-level", > >> + &value); > >> + if (ret < 0) > >> + return ret; > >> + > >> + data->dft_brightness = value; > >> + > >> if (data->piecewise) > >> data->max_brightness = data->levels_count * NSTEPS; > >> else > >> @@ -260,8 +373,10 @@ static int pwm_backlight_probe(struct platform_device *pdev) > >> struct backlight_device *bl; > >> struct device_node *node = pdev->dev.of_node; > >> struct pwm_bl_data *pb; > >> + struct pwm_state state; > >> struct pwm_args pargs; > >> int ret; > >> + int i; > >> > >> if (!data) { > >> ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); > >> @@ -303,6 +418,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) > >> pb->dev = &pdev->dev; > >> pb->enabled = false; > >> pb->piecewise = data->piecewise; > >> + pb->use_lth_table = data->use_lth_table; > >> > >> pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable", > >> GPIOD_ASIS); > >> @@ -361,6 +477,22 @@ static int pwm_backlight_probe(struct platform_device *pdev) > >> > >> dev_dbg(&pdev->dev, "got pwm for backlight\n"); > >> > >> + if (pb->use_lth_table) { > >> + /* Get PWM resolution */ > >> + pwm_get_state(pb->pwm, &state); > >> + if (state.period > 65535) { > >> + dev_err(&pdev->dev, "PWM resolution not supported\n"); > >> + goto err_alloc; > >> + } > >> + /* Find the number of steps based on the PWM resolution */ > >> + for (i = 0; i < ARRAY_SIZE(cie1931_table); i++) > >> + if (cie1931_table[i] >= state.period) { > >> + data->max_brightness = i; > >> + break; > >> + } > >> + pb->scale = data->max_brightness; > >> + } > >> + > >> /* > >> * FIXME: pwm_apply_args() should be removed when switching to > >> * the atomic PWM API. > >> diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h > >> index 444a91b..4ec3b0d 100644 > >> --- a/include/linux/pwm_backlight.h > >> +++ b/include/linux/pwm_backlight.h > >> @@ -15,6 +15,7 @@ struct platform_pwm_backlight_data { > >> unsigned int pwm_period_ns; > >> unsigned int *levels; > >> unsigned int levels_count; > >> + bool use_lth_table; > > > > Why not just "if (!levels)"? > > > > Yep, should work. > > >> bool piecewise; > >> /* TODO remove once all users are switched to gpiod_* API */ > >> int enable_gpio; > >> -- > >> 2.9.3 > >> > > If it's ok I'll send a first version (no RFC) of the patchet adding > your comments. Yes, I think this is more than practical enough to lose the RFC. Daniel. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 18, 2017 at 11:40:59AM +0100, Enric Balletbo Serra wrote: > Hi Pavel, > > 2017-12-15 21:57 GMT+01:00 Pavel Machek <pavel@ucw.cz>: > > Hi! > > > >> 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] > > > > Can we just generate the table on the fly? Should not be hard to do in > > fixed point, right? > > This was discussed a bit in previous RFC which had the code to > generate the table on the fly, see [1]. The use of a fixed table or an > on the fly table is something that I'll let the maintainers to decide. > I've no strong opinion on use the on the fly table if someone takes > care to review deeply the fixed point maths :) The last time we discussed this we concluded we would introduce linear interpolation to make it easier enlarge the small tables we typically see in devicetree. Having done that it seemed attractive (at least to me) to reuse any interpolation code we get and then simply provide a "sane" default look up table for use by DT authors who don't really know how to map PWM on/ off times to luminance. I did review the original fixed point code for the first RFC. IIRC some of the low level functions *looked* they could overflow but, on closer inspection, were never actually overflowed in practice due to the number ranges used by the callers. To be honest part of the attraction of a LUT instead was that I wouldn't have to closely review nor ensure all the fixed point code was properly commented ;-) . The other item in favour of LUT was that (as Doug A. pointed out) PWM duty-cycle to luminance is not strictly linear. Whilst at the moment I am OK to discount this effect it is possible we might want to combined the luminance to human perception values with values read out from graphs in a datasheet. Enric: Having said all that I don't want to give you a really heavy handed steer here. If you think the code was cleaner or clearer when using the formulae then I'd be happy to review the fixed point code. Daniel. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Dec 18, 2017 at 5:31 AM, Daniel Thompson <daniel.thompson@linaro.org> wrote: > I think two different values on the userspace side should always map to > different values on the kernel side. This is what I thought originally, but I believe I've convinced myself that this contradicts other goals and therefore needs to be relaxed. Specifically: Goal #1: A linear adjustment in the number exposed to userspace should result in a linear increase in human perceived brightness. Goal #2: Don't needlessly throw away precision available to the hardware. For instance, if the hardware only supports 64, 128 or 256 levels, it seems like a worthy goal to make sure that userspace can access each of these brightness levels. So if we accept that #1 and #2 are goals, the only solution is to expose a larger "virtual" space and have more than one user-exposed value map to the same actual brightness. As a very simple example, let's say we have a backlight that allows 8 levels: 0 = black 1 = 20% user brightness 2 = 40% user brightness 3 = 60% user brightness 4 = 75% user brightness 5 = 85% user brightness 6 = 90% user brightness 7 = 95% user brightness 8 = 100% user brightness What should we do here? We certainly couldn't expose 8 levels to the user since that would be very non-linear. What about if we exposed 6 levels? We could do: 0%, 20%, 40%, 60%, 85%, 100% That's mostly linear, but the 85% is a little wrong. We've also thrown away the ability for the user to access 90% and 95%, which seems non-ideal. IMHO better in this case is is to expose 101 values to userspace (including 0 and 100) and accept the fact that when the user specifies 10% and 11% that it won't change anything in the hardware. Now, I suppose that throwing away a few values if a PWM has 65536 levels is maybe not the end of the world, but I guess it also depends a lot on which levels you're throwing out. If we have this: 0 = black 1 = 5% user brightness 2 = 10% user brightness 65534 = 99.99% user brightness 65536 = 100% user brightness If we kept things linear (and didn't duplicate) in this case, we'd only expose 21 different level. 0, 5%, 10%, ..., 95%, 100%. IMHO it's better to duplicate. Once we've accepted duplication, I'd say it's easier to just pick a number of levels (4096?) and expose that to the user. If we wanted to be more friendly to the user, we could perhaps somehow expose the actual value, too. For instance, in the above example with 8 levels if the user set the brightness to "11", we could somehow expose to userspace that the brightness actually became "20". > This should make it possible > to calculate the maximal number of steps by scaling up the table to the > PWM resolution and then scanning for the smallest interval between > table steps. > > Once we have a maximal value we could either use it directly or we > might want to push it through min(calculated_max, 1024), on the > assumption that even for animated changes to backlight level that > 1024 is a sensible limit[1] > > > [1] I think it is... I'd be interested to know of Doug A. shares this > view.. I'm not not an expert at all, I just pretend sometimes (though usually I don't even pretend and just bask in my ignorance). Somehow it sticks in my mind that 4096 would be a good value, but I have no real evidence to back that up. ...but, that being said, let's see if I can come up with an excuse for 4096. My overall goal is that you want to be able to adjust the brightness without the user noticing each step. Users can definitely notice a step at 256 levels since that's widely quoted as roughly the number of levels that the human can perceive. I'd double it to be sure (hey, people pay for 48-bit color, don't they?), so let's say that a user could perceive 512 steps. My initial thought is that you'd want to animate, maybe 8 steps, between each perceivable point, which would get to 4096. ...but now that I say it, it does seem like technically you could get away with moving 1/1024, waiting, then the moving another 1/1024. In theory, the user shouldn't notice each step and it should be just as smooth as making 8 steps. ...so I guess I've convinced myself that 1024 should be enough. If I were designing it I'd probably still pick 4096 anyway just because I see no downsides and I could sorta believe that somehow my argument is wrong, but I won't yell if you pick 1024. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 18, 2017 at 08:46:09AM -0800, Doug Anderson wrote: > Hi, > > On Mon, Dec 18, 2017 at 5:31 AM, Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > I think two different values on the userspace side should always map to > > different values on the kernel side. > > This is what I thought originally, but I believe I've convinced myself > that this contradicts other goals and therefore needs to be relaxed. > Specifically: > > Goal #1: A linear adjustment in the number exposed to userspace should > result in a linear increase in human perceived brightness. > > Goal #2: Don't needlessly throw away precision available to the > hardware. For instance, if the hardware only supports 64, 128 or 256 > levels, it seems like a worthy goal to make sure that userspace can > access each of these brightness levels. > > > So if we accept that #1 and #2 are goals, I'm not sure that I accept goal #1 for highly constrained hardware that is physically capable only of a very few steps. I think adopting Goal #1 favours the slider use-case too much over the hot-key use case. If you linearise a tiny space then the hot-key risks doing nothing then pressed. It's not that I don't think this is a real problem but I think it is one that must be solved in the ABI (e.g. by communicating the typical curve to userspace and revealing true hardware steps). > the only solution is to > expose a larger "virtual" space and have more than one user-exposed > value map to the same actual brightness. As a very simple example, > let's say we have a backlight that allows 8 levels: > > 0 = black > 1 = 20% user brightness > 2 = 40% user brightness > 3 = 60% user brightness > 4 = 75% user brightness > 5 = 85% user brightness > 6 = 90% user brightness > 7 = 95% user brightness > 8 = 100% user brightness Note that these patches are for the PWM backlight; these steps seem unlikely even for an 8-bit PWM. That leads us to a difficult question. When presented with a low-bit PWM then are automatic curves the right tool? With such low steps we probably need to compromise linearity to some extent (and maybe the DT author may be forced to tune for slider versus hotkey depending on what our form-factor is). Daniel. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html