[RFC v2 0/2] backlight: pwm_bl: support linear brightness to human eye

[RFC v2 0/2] backlight: pwm_bl: support linear brightness to human eye

[Enric Balletbo i Serra]

Dear all,

This patch series is a second RFC to know your opinion about the way to
solve the problems exposed in the first RFC [1]

The first patch what tries to solve is the problem of granularity for high
resolution PWMs. The idea is simple interpolate between 2 brightness values
so we can have a high PWM duty cycle (a 16 bits PWM is up to 65535 possible
steps) without having to list out every possible value in the dts. I think
that this patch is required to not break backward compability, to be more
flexible and also extend the functionality to be able to use high resolution
PWM with enough steps to have a good UI experience in userspace.

The second patch is a bit more ambicious, the idea is let decide the driver
the brightness-levels required in function of the PWM resolution. To do this
we use a static table filled with the CIE 1931 algorithm values to convert
brightness to PWM duty cycle.

More detailed info is available in the commit message of every patch.

A couple of questions come to my mind:

- What's the amoung of steps we really need? Currently there is 1024 steps
  for a resolution of 16 bits, and 37 steps for a PWM with 8 bits of
  resolution. That seems to work well with both 16 bits and 8 bits. Tested
  on a Samsung Chromebook Plus (16 bits) and a SL50 device (8 bits)

- What about 32 bits or greather resolutions? We need to support it?
  I did a quick look and seems nobody requires it, but I don't really know.
  For now if we set more than 16 bits of resolution the driver triggers
  an error.

Waiting for your feedback.

Chances since v1:
- Add linear interpolation for high resolution PWM.
- Get rid of fixed point calculations and use a table instead.

[1] http://www.spinics.net/lists/devicetree/msg193262.html

Best regards,

Enric Balletbo i Serra (2):
  backlight: pwm_bl: linear interpolation between values of
    brightness-levels
  backlight: pwm_bl: compute brightness of LED linearly to human eye.

 .../bindings/leds/backlight/pwm-backlight.txt      |   2 +
 drivers/video/backlight/pwm_bl.c                   | 209 ++++++++++++++++++---
 include/linux/pwm_backlight.h                      |   3 +
 3 files changed, 191 insertions(+), 23 deletions(-)

-- 
2.9.3

[RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Enric Balletbo i Serra]

Setting use-linear-interpolation in the dts will allow you to have linear
interpolation between values of brightness-levels.

There are now 256 between each of the values of brightness-levels. If
something is requested halfway between 2 values, we'll use linear
interpolation.

This way a high resolution pwm duty cycle can be used without having to
list out every possible value in the dts. This system also allows for
gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").

Patch based on the Alexandru M Stan work done for ChromeOS kernels.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
 drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
 include/linux/pwm_backlight.h                      |  2 +
 3 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 764db86..7c48f20 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -17,6 +17,8 @@ Optional properties:
                "pwms" property (see PWM binding[0])
   - enable-gpios: contains a single GPIO specifier for the GPIO which enables
                   and disables the backlight (see GPIO binding[1])
+  - use-linear-interpolation: set this propriety to enable linear interpolation
+                              between each of the values of brightness-levels.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9bd1768..59b1bfb 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -24,6 +24,8 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
+#define NSTEPS	256
+
 struct pwm_bl_data {
 	struct pwm_device	*pwm;
 	struct device		*dev;
@@ -35,6 +37,7 @@ struct pwm_bl_data {
 	struct gpio_desc	*enable_gpio;
 	unsigned int		scale;
 	bool			legacy;
+	bool			piecewise;
 	int			(*notify)(struct device *,
 					  int brightness);
 	void			(*notify_after)(struct device *,
@@ -76,17 +79,36 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 	pb->enabled = false;
 }
 
-static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
+static int scale(struct pwm_bl_data *pb, int x)
 {
 	unsigned int lth = pb->lth_brightness;
+
+	return (x * (pb->period - lth) / pb->scale) + lth;
+}
+
+static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
+{
+	int coarse = brightness / NSTEPS;
+	int fine = brightness % NSTEPS;
 	int duty_cycle;
 
-	if (pb->levels)
-		duty_cycle = pb->levels[brightness];
-	else
-		duty_cycle = brightness;
+	if (pb->levels) {
+		if (pb->piecewise) {
+			duty_cycle = scale(pb, pb->levels[coarse]);
+			if (fine > 0)
+				duty_cycle += (scale(pb, pb->levels[coarse + 1])
+					       - scale(pb, pb->levels[coarse]))
+					       * fine / NSTEPS;
+			dev_dbg(pb->dev, "brightness=%d coarse=%d fine=%d\n",
+				brightness, coarse, fine);
+		} else {
+			duty_cycle = scale(pb, pb->levels[brightness]);
+		}
+	} else {
+		duty_cycle = scale(pb, brightness);
+	}
 
-	return (duty_cycle * (pb->period - lth) / pb->scale) + lth;
+	return duty_cycle;
 }
 
 static int pwm_backlight_update_status(struct backlight_device *bl)
@@ -149,11 +171,11 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	if (!prop)
 		return -EINVAL;
 
-	data->max_brightness = length / sizeof(u32);
+	data->levels_count = length / sizeof(u32);
 
 	/* read brightness levels from DT property */
-	if (data->max_brightness > 0) {
-		size_t size = sizeof(*data->levels) * data->max_brightness;
+	if (data->levels_count > 0) {
+		size_t size = sizeof(*data->levels) * data->levels_count;
 
 		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
 		if (!data->levels)
@@ -161,7 +183,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
 
 		ret = of_property_read_u32_array(node, "brightness-levels",
 						 data->levels,
-						 data->max_brightness);
+						 data->levels_count);
 		if (ret < 0)
 			return ret;
 
@@ -170,10 +192,18 @@ static int pwm_backlight_parse_dt(struct device *dev,
 		if (ret < 0)
 			return ret;
 
+		data->piecewise = of_property_read_bool(node,
+						    "use-linear-interpolation");
+
 		data->dft_brightness = value;
-		data->max_brightness--;
+		data->levels_count--;
 	}
 
+	if (data->piecewise)
+		data->max_brightness = data->levels_count * NSTEPS;
+	else
+		data->max_brightness = data->levels_count;
+
 	data->enable_gpio = -EINVAL;
 	return 0;
 }
@@ -258,7 +288,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (data->levels) {
 		unsigned int i;
 
-		for (i = 0; i <= data->max_brightness; i++)
+		for (i = 0; i <= data->levels_count; i++)
 			if (data->levels[i] > pb->scale)
 				pb->scale = data->levels[i];
 
@@ -272,6 +302,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	pb->exit = data->exit;
 	pb->dev = &pdev->dev;
 	pb->enabled = false;
+	pb->piecewise = data->piecewise;
 
 	pb->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
 						  GPIOD_ASIS);
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index e8afbd7..444a91b 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -14,6 +14,8 @@ struct platform_pwm_backlight_data {
 	unsigned int lth_brightness;
 	unsigned int pwm_period_ns;
 	unsigned int *levels;
+	unsigned int levels_count;
+	bool piecewise;
 	/* TODO remove once all users are switched to gpiod_* API */
 	int enable_gpio;
 	int (*init)(struct device *dev);
-- 
2.9.3

Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Rob Herring]

On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote:
> Setting use-linear-interpolation in the dts will allow you to have linear
> interpolation between values of brightness-levels.
> 
> There are now 256 between each of the values of brightness-levels. If
> something is requested halfway between 2 values, we'll use linear
> interpolation.
> 
> This way a high resolution pwm duty cycle can be used without having to
> list out every possible value in the dts. 

I thought we already had a way to do that.

> This system also allows for
> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").
> 
> Patch based on the Alexandru M Stan work done for ChromeOS kernels.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>  .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
>  drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
>  include/linux/pwm_backlight.h                      |  2 +
>  3 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..7c48f20 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,8 @@ Optional properties:
>                 "pwms" property (see PWM binding[0])
>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>                    and disables the backlight (see GPIO binding[1])
> +  - use-linear-interpolation: set this propriety to enable linear interpolation
> +                              between each of the values of brightness-levels.

Isn't this really just whether you allow values not listed because what 
other kind of interpolation would you do? Seems like you should just be 
able to query the resolution of the PWM and decide if it can take more 
values than listed.

Rob

Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Enric Balletbo Serra]

Hi Rob,

2017-11-20 19:58 GMT+01:00 Rob Herring <robh@kernel.org>:
> On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote:
>> Setting use-linear-interpolation in the dts will allow you to have linear
>> interpolation between values of brightness-levels.
>>
>> There are now 256 between each of the values of brightness-levels. If
>> something is requested halfway between 2 values, we'll use linear
>> interpolation.
>>
>> This way a high resolution pwm duty cycle can be used without having to
>> list out every possible value in the dts.
>
> I thought we already had a way to do that.
>
>> This system also allows for
>> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").
>>
>> Patch based on the Alexandru M Stan work done for ChromeOS kernels.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>  .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
>>  drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
>>  include/linux/pwm_backlight.h                      |  2 +
>>  3 files changed, 47 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> index 764db86..7c48f20 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> @@ -17,6 +17,8 @@ Optional properties:
>>                 "pwms" property (see PWM binding[0])
>>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>>                    and disables the backlight (see GPIO binding[1])
>> +  - use-linear-interpolation: set this propriety to enable linear interpolation
>> +                              between each of the values of brightness-levels.
>
> Isn't this really just whether you allow values not listed because what
> other kind of interpolation would you do?

Yes, the idea behind this is just allow values not listed.

> Seems like you should just be
> able to query the resolution of the PWM and decide if it can take more
> values than listed.
>

Without using a new DT propriety and let decide the driver when use
intepolation and when not? Shouldn't this break current behavior then?
You can have a high resolution PWM but I'm not sure if you want allow
always more values than listed.

Regards,
 Enric


> Rob

Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Doug Anderson]

Hi,

On Mon, Nov 27, 2017 at 3:21 AM, Enric Balletbo Serra
<eballetbo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Rob,
>
> 2017-11-20 19:58 GMT+01:00 Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>> On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote:
>>> Setting use-linear-interpolation in the dts will allow you to have linear
>>> interpolation between values of brightness-levels.
>>>
>>> There are now 256 between each of the values of brightness-levels. If
>>> something is requested halfway between 2 values, we'll use linear
>>> interpolation.
>>>
>>> This way a high resolution pwm duty cycle can be used without having to
>>> list out every possible value in the dts.
>>
>> I thought we already had a way to do that.
>>
>>> This system also allows for
>>> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").
>>>
>>> Patch based on the Alexandru M Stan work done for ChromeOS kernels.
>>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> ---
>>>  .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
>>>  drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
>>>  include/linux/pwm_backlight.h                      |  2 +
>>>  3 files changed, 47 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> index 764db86..7c48f20 100644
>>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> @@ -17,6 +17,8 @@ Optional properties:
>>>                 "pwms" property (see PWM binding[0])
>>>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>>>                    and disables the backlight (see GPIO binding[1])
>>> +  - use-linear-interpolation: set this propriety to enable linear interpolation
>>> +                              between each of the values of brightness-levels.
>>
>> Isn't this really just whether you allow values not listed because what
>> other kind of interpolation would you do?
>
> Yes, the idea behind this is just allow values not listed.
>
>> Seems like you should just be
>> able to query the resolution of the PWM and decide if it can take more
>> values than listed.
>>
>
> Without using a new DT propriety and let decide the driver when use
> intepolation and when not? Shouldn't this break current behavior then?
> You can have a high resolution PWM but I'm not sure if you want allow
> always more values than listed.

Right.  The assumption is that it's _possible_ that only the exact
values listed are valid duty cycles.  We don't want to break old
hardware by suddenly allowing other duty cycles to be used.  However,
it's expected that on most hardware you can simply use any duty cycle
between any of the specified ones and it should be fine.  Anyone
specifying this property would say that's OK for their hardware.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Doug Anderson]

Hi,

On Mon, Nov 27, 2017 at 3:21 AM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> Hi Rob,
>
> 2017-11-20 19:58 GMT+01:00 Rob Herring <robh@kernel.org>:
>> On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote:
>>> Setting use-linear-interpolation in the dts will allow you to have linear
>>> interpolation between values of brightness-levels.
>>>
>>> There are now 256 between each of the values of brightness-levels. If
>>> something is requested halfway between 2 values, we'll use linear
>>> interpolation.
>>>
>>> This way a high resolution pwm duty cycle can be used without having to
>>> list out every possible value in the dts.
>>
>> I thought we already had a way to do that.
>>
>>> This system also allows for
>>> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").
>>>
>>> Patch based on the Alexandru M Stan work done for ChromeOS kernels.
>>>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> ---
>>>  .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
>>>  drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
>>>  include/linux/pwm_backlight.h                      |  2 +
>>>  3 files changed, 47 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> index 764db86..7c48f20 100644
>>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>>> @@ -17,6 +17,8 @@ Optional properties:
>>>                 "pwms" property (see PWM binding[0])
>>>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>>>                    and disables the backlight (see GPIO binding[1])
>>> +  - use-linear-interpolation: set this propriety to enable linear interpolation
>>> +                              between each of the values of brightness-levels.
>>
>> Isn't this really just whether you allow values not listed because what
>> other kind of interpolation would you do?
>
> Yes, the idea behind this is just allow values not listed.
>
>> Seems like you should just be
>> able to query the resolution of the PWM and decide if it can take more
>> values than listed.
>>
>
> Without using a new DT propriety and let decide the driver when use
> intepolation and when not? Shouldn't this break current behavior then?
> You can have a high resolution PWM but I'm not sure if you want allow
> always more values than listed.

Right.  The assumption is that it's _possible_ that only the exact
values listed are valid duty cycles.  We don't want to break old
hardware by suddenly allowing other duty cycles to be used.  However,
it's expected that on most hardware you can simply use any duty cycle
between any of the specified ones and it should be fine.  Anyone
specifying this property would say that's OK for their hardware.

-Doug

Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Doug Anderson]

Hi,

On Thu, Nov 16, 2017 at 6:11 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Setting use-linear-interpolation in the dts will allow you to have linear
> interpolation between values of brightness-levels.
>
> There are now 256 between each of the values of brightness-levels. If
> something is requested halfway between 2 values, we'll use linear
> interpolation.
>
> This way a high resolution pwm duty cycle can be used without having to
> list out every possible value in the dts. This system also allows for
> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").
>
> Patch based on the Alexandru M Stan work done for ChromeOS kernels.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>  .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
>  drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
>  include/linux/pwm_backlight.h                      |  2 +
>  3 files changed, 47 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..7c48f20 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,8 @@ Optional properties:
>                 "pwms" property (see PWM binding[0])
>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>                    and disables the backlight (see GPIO binding[1])
> +  - use-linear-interpolation: set this propriety to enable linear interpolation
> +                              between each of the values of brightness-levels.
>
>  [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>  [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 9bd1768..59b1bfb 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -24,6 +24,8 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>
> +#define NSTEPS 256

I'm not sure this is quite the ideal way to specify it.  I could sorta
imagine wanting to specify just:

  brightness-levels = <0 65535>

...and in such a case you'll only give 256 steps in between.  256
isn't quite granular enough and the human eye can notice each step.  I
could fake it by putting this in the device tree:

  brightness-levels = <0 4095 8191 ... ... 61439 65535>

...but that's kinda silly.  I'd rather just say that when we're using
interpolation we just say that there will be a certain number of steps
(like 32768).  Is there really a huge advantage of picking 256 steps
between each specified value instead of just picking a fixed number of
brightness levels?

Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Daniel Thompson]

On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote:
> 
> Setting use-linear-interpolation in the dts will allow you to have linear
> interpolation between values of brightness-levels.
> 
> There are now 256 between each of the values of brightness-levels. If
> something is requested halfway between 2 values, we'll use linear
> interpolation.

Why 256?
> 
> This way a high resolution pwm duty cycle can be used without having to
> list out every possible value in the dts. This system also allows for
> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").
> 
> Patch based on the Alexandru M Stan work done for ChromeOS kernels.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>  .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
>  drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
>  include/linux/pwm_backlight.h                      |  2 +
>  3 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..7c48f20 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,8 @@ Optional properties:
>                 "pwms" property (see PWM binding[0])
>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>                    and disables the backlight (see GPIO binding[1])
> +  - use-linear-interpolation: set this propriety to enable linear interpolation
> +                              between each of the values of brightness-levels.

Deciding whether or not this deployment of interpolation is a property
of the hardware is a finely balanced judgement. On the whole I conclude
that since the lookup table is a property of the hardware so too is the
deployment of interpolation.

Following up on the "why 256?" comment. IMHO either the number of
interpolated steps should be calculated from the underlying PWM
resolution or it could simply be specified in the DT (e.g. replace
use-linear-interpolation with num-interpolated-steps).


>  [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>  [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 9bd1768..59b1bfb 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -24,6 +24,8 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  
> +#define NSTEPS	256
> +
>  struct pwm_bl_data {
>  	struct pwm_device	*pwm;
>  	struct device		*dev;
> @@ -35,6 +37,7 @@ struct pwm_bl_data {
>  	struct gpio_desc	*enable_gpio;
>  	unsigned int		scale;
>  	bool			legacy;
> +	bool			piecewise;
>  	int			(*notify)(struct device *,
>  					  int brightness);
>  	void			(*notify_after)(struct device *,
> @@ -76,17 +79,36 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>  	pb->enabled = false;
>  }
>  
> -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
> +static int scale(struct pwm_bl_data *pb, int x)
>  {
>  	unsigned int lth = pb->lth_brightness;
> +
> +	return (x * (pb->period - lth) / pb->scale) + lth;
> +}
> +
> +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
> +{
> +	int coarse = brightness / NSTEPS;
> +	int fine = brightness % NSTEPS;
>  	int duty_cycle;
>  
> -	if (pb->levels)
> -		duty_cycle = pb->levels[brightness];
> -	else
> -		duty_cycle = brightness;
> +	if (pb->levels) {
> +		if (pb->piecewise) {
> +			duty_cycle = scale(pb, pb->levels[coarse]);
> +			if (fine > 0)
> +				duty_cycle += (scale(pb, pb->levels[coarse + 1])
> +					       - scale(pb, pb->levels[coarse]))
> +					       * fine / NSTEPS;
> +			dev_dbg(pb->dev, "brightness=%d coarse=%d fine=%d\n",
> +				brightness, coarse, fine);
> +		} else {
> +			duty_cycle = scale(pb, pb->levels[brightness]);
> +		}
> +	} else {
> +		duty_cycle = scale(pb, brightness);
> +	}
>  
> -	return (duty_cycle * (pb->period - lth) / pb->scale) + lth;
> +	return duty_cycle;
>  }
>  
>  static int pwm_backlight_update_status(struct backlight_device *bl)
> @@ -149,11 +171,11 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  	if (!prop)
>  		return -EINVAL;
>  
> -	data->max_brightness = length / sizeof(u32);
> +	data->levels_count = length / sizeof(u32);
>  
>  	/* read brightness levels from DT property */
> -	if (data->max_brightness > 0) {
> -		size_t size = sizeof(*data->levels) * data->max_brightness;
> +	if (data->levels_count > 0) {
> +		size_t size = sizeof(*data->levels) * data->levels_count;
>  
>  		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
>  		if (!data->levels)
> @@ -161,7 +183,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  
>  		ret = of_property_read_u32_array(node, "brightness-levels",
>  						 data->levels,
> -						 data->max_brightness);
> +						 data->levels_count);
>  		if (ret < 0)
>  			return ret;
>  
> @@ -170,10 +192,18 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  		if (ret < 0)
>  			return ret;
>  
> +		data->piecewise = of_property_read_bool(node,
> +						    "use-linear-interpolation");
> +
>  		data->dft_brightness = value;
> -		data->max_brightness--;
> +		data->levels_count--;
>  	}
>  
> +	if (data->piecewise)
> +		data->max_brightness = data->levels_count * NSTEPS;
> +	else
> +		data->max_brightness = data->levels_count;

I think we lost a -1 here?


Daniel.

Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Enric Balletbo Serra]

Hi Daniel,

2017-12-15 15:40 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>:
> On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote:
>>
>> Setting use-linear-interpolation in the dts will allow you to have linear
>> interpolation between values of brightness-levels.
>>
>> There are now 256 between each of the values of brightness-levels. If
>> something is requested halfway between 2 values, we'll use linear
>> interpolation.
>
> Why 256?

To be honest there isn't a strong reason, I thought that 256 was a
good value because is the minimum number of steps possible (8 bits
pwm). But yeah, guess the discussion is more if this value should be
calculated, or specified in the the DT more than the value itself.

>>
>> This way a high resolution pwm duty cycle can be used without having to
>> list out every possible value in the dts. This system also allows for
>> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").
>>
>> Patch based on the Alexandru M Stan work done for ChromeOS kernels.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>  .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
>>  drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
>>  include/linux/pwm_backlight.h                      |  2 +
>>  3 files changed, 47 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> index 764db86..7c48f20 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
>> @@ -17,6 +17,8 @@ Optional properties:
>>                 "pwms" property (see PWM binding[0])
>>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>>                    and disables the backlight (see GPIO binding[1])
>> +  - use-linear-interpolation: set this propriety to enable linear interpolation
>> +                              between each of the values of brightness-levels.
>
> Deciding whether or not this deployment of interpolation is a property
> of the hardware is a finely balanced judgement. On the whole I conclude
> that since the lookup table is a property of the hardware so too is the
> deployment of interpolation.
>
> Following up on the "why 256?" comment. IMHO either the number of
> interpolated steps should be calculated from the underlying PWM
> resolution or it could simply be specified in the DT (e.g. replace
> use-linear-interpolation with num-interpolated-steps).
>

Personally I like the idea to have the possibility to specify the
number of interpolated steps in the DT, I think that will be more
flexible for the user. If it's ok let me send a first version using
num-interpolated-steps, and continue the discussion about if makes
sense to have that in the DT or not.

>
>>  [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>>  [1]: Documentation/devicetree/bindings/gpio/gpio.txt
>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>> index 9bd1768..59b1bfb 100644
>> --- a/drivers/video/backlight/pwm_bl.c
>> +++ b/drivers/video/backlight/pwm_bl.c
>> @@ -24,6 +24,8 @@
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/slab.h>
>>
>> +#define NSTEPS       256
>> +
>>  struct pwm_bl_data {
>>       struct pwm_device       *pwm;
>>       struct device           *dev;
>> @@ -35,6 +37,7 @@ struct pwm_bl_data {
>>       struct gpio_desc        *enable_gpio;
>>       unsigned int            scale;
>>       bool                    legacy;
>> +     bool                    piecewise;
>>       int                     (*notify)(struct device *,
>>                                         int brightness);
>>       void                    (*notify_after)(struct device *,
>> @@ -76,17 +79,36 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>>       pb->enabled = false;
>>  }
>>
>> -static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
>> +static int scale(struct pwm_bl_data *pb, int x)
>>  {
>>       unsigned int lth = pb->lth_brightness;
>> +
>> +     return (x * (pb->period - lth) / pb->scale) + lth;
>> +}
>> +
>> +static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
>> +{
>> +     int coarse = brightness / NSTEPS;
>> +     int fine = brightness % NSTEPS;
>>       int duty_cycle;
>>
>> -     if (pb->levels)
>> -             duty_cycle = pb->levels[brightness];
>> -     else
>> -             duty_cycle = brightness;
>> +     if (pb->levels) {
>> +             if (pb->piecewise) {
>> +                     duty_cycle = scale(pb, pb->levels[coarse]);
>> +                     if (fine > 0)
>> +                             duty_cycle += (scale(pb, pb->levels[coarse + 1])
>> +                                            - scale(pb, pb->levels[coarse]))
>> +                                            * fine / NSTEPS;
>> +                     dev_dbg(pb->dev, "brightness=%d coarse=%d fine=%d\n",
>> +                             brightness, coarse, fine);
>> +             } else {
>> +                     duty_cycle = scale(pb, pb->levels[brightness]);
>> +             }
>> +     } else {
>> +             duty_cycle = scale(pb, brightness);
>> +     }
>>
>> -     return (duty_cycle * (pb->period - lth) / pb->scale) + lth;
>> +     return duty_cycle;
>>  }
>>
>>  static int pwm_backlight_update_status(struct backlight_device *bl)
>> @@ -149,11 +171,11 @@ static int pwm_backlight_parse_dt(struct device *dev,
>>       if (!prop)
>>               return -EINVAL;
>>
>> -     data->max_brightness = length / sizeof(u32);
>> +     data->levels_count = length / sizeof(u32);
>>
>>       /* read brightness levels from DT property */
>> -     if (data->max_brightness > 0) {
>> -             size_t size = sizeof(*data->levels) * data->max_brightness;
>> +     if (data->levels_count > 0) {
>> +             size_t size = sizeof(*data->levels) * data->levels_count;
>>
>>               data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
>>               if (!data->levels)
>> @@ -161,7 +183,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
>>
>>               ret = of_property_read_u32_array(node, "brightness-levels",
>>                                                data->levels,
>> -                                              data->max_brightness);
>> +                                              data->levels_count);
>>               if (ret < 0)
>>                       return ret;
>>
>> @@ -170,10 +192,18 @@ static int pwm_backlight_parse_dt(struct device *dev,
>>               if (ret < 0)
>>                       return ret;
>>
>> +             data->piecewise = of_property_read_bool(node,
>> +                                                 "use-linear-interpolation");
>> +
>>               data->dft_brightness = value;
>> -             data->max_brightness--;
>> +             data->levels_count--;
>>       }
>>
>> +     if (data->piecewise)
>> +             data->max_brightness = data->levels_count * NSTEPS;
>> +     else
>> +             data->max_brightness = data->levels_count;
>
> I think we lost a -1 here?
>

Good catch, I think so.

Regards,
 Enric

>
> Daniel.

Re: [RFC v2 1/2] backlight: pwm_bl: linear interpolation between values of brightness-levels

[Daniel Thompson]

On Mon, Dec 18, 2017 at 10:47:24AM +0100, Enric Balletbo Serra wrote:
> Hi Daniel,
> 
> 2017-12-15 15:40 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>:
> > On Thu, Nov 16, 2017 at 03:11:50PM +0100, Enric Balletbo i Serra wrote:
> >>
> >> Setting use-linear-interpolation in the dts will allow you to have linear
> >> interpolation between values of brightness-levels.
> >>
> >> There are now 256 between each of the values of brightness-levels. If
> >> something is requested halfway between 2 values, we'll use linear
> >> interpolation.
> >
> > Why 256?
> 
> To be honest there isn't a strong reason, I thought that 256 was a
> good value because is the minimum number of steps possible (8 bits
> pwm). But yeah, guess the discussion is more if this value should be
> calculated, or specified in the the DT more than the value itself.
> 
> >>
> >> This way a high resolution pwm duty cycle can be used without having to
> >> list out every possible value in the dts. This system also allows for
> >> gamma corrected values (eg: "brightness-levels = <0 2 4 8 16 32>;").
> >>
> >> Patch based on the Alexandru M Stan work done for ChromeOS kernels.
> >>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >>  .../bindings/leds/backlight/pwm-backlight.txt      |  2 +
> >>  drivers/video/backlight/pwm_bl.c                   | 55 +++++++++++++++++-----
> >>  include/linux/pwm_backlight.h                      |  2 +
> >>  3 files changed, 47 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> >> index 764db86..7c48f20 100644
> >> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> >> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> >> @@ -17,6 +17,8 @@ Optional properties:
> >>                 "pwms" property (see PWM binding[0])
> >>    - enable-gpios: contains a single GPIO specifier for the GPIO which enables
> >>                    and disables the backlight (see GPIO binding[1])
> >> +  - use-linear-interpolation: set this propriety to enable linear interpolation
> >> +                              between each of the values of brightness-levels.
> >
> > Deciding whether or not this deployment of interpolation is a property
> > of the hardware is a finely balanced judgement. On the whole I conclude
> > that since the lookup table is a property of the hardware so too is the
> > deployment of interpolation.
> >
> > Following up on the "why 256?" comment. IMHO either the number of
> > interpolated steps should be calculated from the underlying PWM
> > resolution or it could simply be specified in the DT (e.g. replace
> > use-linear-interpolation with num-interpolated-steps).
> >
> 
> Personally I like the idea to have the possibility to specify the
> number of interpolated steps in the DT, I think that will be more
> flexible for the user. If it's ok let me send a first version using
> num-interpolated-steps, and continue the discussion about if makes
> sense to have that in the DT or not.

It's ok from my side.


Daniel.

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

[Enric Balletbo i Serra]

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-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
 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] = {
+	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;
 	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;
 	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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Enric Balletbo i Serra]

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] = {
+	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;
 	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;
 	bool piecewise;
 	/* TODO remove once all users are switched to gpiod_* API */
 	int enable_gpio;
-- 
2.9.3

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

[Doug Anderson]

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

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

[Daniel Thompson]

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-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> 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-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>> ---
>>   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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Thompson]

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
> 

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

[Doug Anderson]

Hi,

On Thu, Nov 30, 2017 at 3:27 AM, Daniel Thompson
<daniel.thompson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Doug Anderson]

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

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> 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-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> ---
>>>   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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

[Doug Anderson]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Enric Balletbo Serra]

Hi Pavel,

2017-12-15 21:57 GMT+01:00 Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>:
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Enric Balletbo Serra]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Daniel Thompson]

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.

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

[Daniel Thompson]

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
> 

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

[Enric Balletbo Serra]

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

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

[Daniel Thompson]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> >> ---
> >>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Thompson]

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.

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

[Doug Anderson]

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.

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

[Daniel Thompson]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Daniel Thompson]

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.