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

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

[Enric Balletbo i Serra]

Dear all,

This patch series is a first RFC to know your opinion about implement
support to create brightness levels tables dinamically. I tried to argue
in every patch the specific reasons we think this can be interesting, to
sumup, the idea behind these patches is be able to pass via device tree
two parameters to the driver so it can calculate the brightness levels
based on the CIE 1931 lightness formula, which is what actually describes
how we perceive light.

I think that at least the maths involved can be improved, and I've still
some doubts. With current code if you create a table with a max PWM
value of 255 and 127 steps, the first numbers are repeated so I'm thinking
that maybe we should skip/remove the repeated values. i.e. have a table
like this,

[0, 1, 2, 3  ...  235, 240, 245, 250, 255]

instead of

[0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3  ...  235, 240, 245, 250, 255]

Well, I know there are things to improve but lets see your feedback first
before dedicate more time on it. The patches were tested on a couple of
devices but I'll test on more devices meanwhile we discuss about it.

Best regards,

Enric Balletbo i Serra (2):
  dt-bindings: pwm-backlight: add brightness-levels-scale property
  backlight: pwm_bl: compute brightness of LED linearly to human eye.

 .../bindings/leds/backlight/pwm-backlight.txt      |  21 ++++
 drivers/video/backlight/pwm_bl.c                   | 111 +++++++++++++++++++--
 2 files changed, 123 insertions(+), 9 deletions(-)

-- 
2.9.3

[RFC 1/2] dt-bindings: pwm-backlight: add brightness-levels-scale property

[Enric Balletbo i Serra]

Brightness is not perceived linearly; rather, it typically follows some
kind of parabolic curve. We can support this by skipping values in the
brightness-levels array in a pseudo-quadratic curve. Typically we used
less than 256 levels, which yields no more than 1KiB of memory in our
device tree. But, we've noticed that on some devices the backlight
performs much smoother at lower ranges if we have more than 256-levels of
granularity. On kevin device, for example, if we support all 64K, that
will waste us at least 256KiB in our device tree.

Let's avoid to waste memory and have a huge table of numbers in our device
tree of numbers by adding a brightness-levels-scale property to let the
driver compute the brightness levels based on one algorithm and their
property parameters.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 .../bindings/leds/backlight/pwm-backlight.txt       | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 764db86..11c5583 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -17,6 +17,16 @@ 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])
+  - brightness-levels-scale: this can be used instead of 'brightness-levels',
+                             to represent the perceived luminance. So rather
+                             than specifying 'brightness-levels = <0
+                             1 2 ... 65535>', one can simply say
+                             'brightness-levels-scale = <255 65535>', where the
+                             first number is the max number of levels and the
+                             second number is the max PWM value that represent a
+                             100% duty cycle (brightest). The result is a
+                             correction table for PWM values to create linear
+                             brightness based on the CIE1931 algorithm.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
@@ -33,3 +43,14 @@ Example:
 		power-supply = <&vdd_bl_reg>;
 		enable-gpios = <&gpio 58 0>;
 	};
+
+	backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 0 5000000>;
+
+		brightness-levels-scale = <255 65535>;
+		default-brightness-level = <128>;
+
+		power-supply = <&vdd_bl_reg>;
+		enable-gpios = <&gpio 58 0>;
+	};
-- 
2.9.3

[RFC 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 dinamically based
on this algorithm. For example, the definition of the following property
in your device tree,

 brightness-levels-scale = <16 255>

is equivalent to,

 brightness-levels = <0 2 4 7 11 17 25 35 47 62 79 99 123 150 181 216 255>;

It does not make much sense use the new property for few levels of
granularity, as you can really use the brightness-levels property with the
table hardcoded, but, if we have more than 256-levels of granularity you
might prefer use the new property instead of put a huge table in your
device tree.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 drivers/video/backlight/pwm_bl.c | 111 +++++++++++++++++++++++++++++++++++----
 1 file changed, 102 insertions(+), 9 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 1261400..7d87c0d 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -130,6 +130,72 @@ static const struct backlight_ops pwm_backlight_ops = {
 };
 
 #ifdef CONFIG_OF
+
+#define PWM_LUMINANCE_SCALE	10000 /* luminance scale */
+
+static u64 int_pow(u64 base, int exp)
+{
+	u64 result = 1;
+
+	while (exp) {
+		if (exp & 1)
+			result *= base;
+		exp >>= 1;
+		base *= base;
+	}
+
+	return result;
+}
+
+/*
+ * CIE lightness to PWM conversion.
+ *
+ * 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.
+ */
+static u64 cie1931(unsigned int lightness, unsigned int scale)
+{
+	u64 retval;
+
+	lightness *= 100;
+	if (lightness <= (8 * scale)) {
+		retval = DIV_ROUND_CLOSEST_ULL(lightness * 10, 9023);
+	} else {
+		retval = int_pow((lightness + (16 * scale)) / 116, 3);
+		retval = DIV_ROUND_CLOSEST_ULL(retval, (scale * scale));
+	}
+
+	return retval;
+}
+
+/*
+ * Create a correction table for PWM values to create linear brightness for a
+ * LED based on the CIE1931 algorithm.
+ */
+static int pwm_backlight_brightness(unsigned int *levels,
+				    unsigned int input_size,
+				    unsigned int output_size,
+				    unsigned int scale)
+{
+	u64 retval;
+	int i;
+
+	for (i = 0; i < input_size + 1; i++) {
+		retval = cie1931((i * scale) / input_size, scale) * output_size;
+		retval = DIV_ROUND_CLOSEST_ULL(retval, scale);
+		if (retval > UINT_MAX)
+			return -EINVAL;
+		levels[i] = (unsigned int)retval;
+	}
+
+	return 0;
+}
+
 static int pwm_backlight_parse_dt(struct device *dev,
 				  struct platform_pwm_backlight_data *data)
 {
@@ -146,10 +212,19 @@ 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->max_brightness = length / sizeof(u32);
+	if (!prop) {
+		/* total number of brightness levels */
+		ret = of_property_read_u32_index(node,
+						 "brightness-levels-scale",
+						 0, &value);
+		if (ret < 0)
+			return ret;
+		if (value > INT_MAX)
+			return -EINVAL;
+		data->max_brightness = value;
+	} else {
+		data->max_brightness = length / sizeof(u32);
+	}
 
 	/* read brightness levels from DT property */
 	if (data->max_brightness > 0) {
@@ -159,11 +234,29 @@ 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->max_brightness);
-		if (ret < 0)
-			return ret;
+		if (prop) {
+			ret = of_property_read_u32_array(node,
+							 "brightness-levels",
+							 data->levels,
+							 data->max_brightness);
+			if (ret < 0)
+				return ret;
+		} else {
+			ret = of_property_read_u32_index(node,
+						"brightness-levels-scale",
+						1, &value);
+			if (ret < 0)
+				return ret;
+			if (value > INT_MAX)
+				return -EINVAL;
+
+			ret = pwm_backlight_brightness(data->levels,
+						       data->max_brightness,
+						       value,
+						       PWM_LUMINANCE_SCALE);
+			if (ret)
+				return ret;
+		}
 
 		ret = of_property_read_u32(node, "default-brightness-level",
 					   &value);
-- 
2.9.3

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

[Daniel Thompson]

On 04/09/17 16:35, Enric Balletbo i Serra wrote:
> Dear all,
> 
> This patch series is a first RFC to know your opinion about implement
> support to create brightness levels tables dinamically. I tried to argue
> in every patch the specific reasons we think this can be interesting, to
> sumup, the idea behind these patches is be able to pass via device tree
> two parameters to the driver so it can calculate the brightness levels
> based on the CIE 1931 lightness formula, which is what actually describes
> how we perceive light.
> 
> I think that at least the maths involved can be improved, and I've still
> some doubts. With current code if you create a table with a max PWM
> value of 255 and 127 steps, the first numbers are repeated so I'm thinking > that maybe we should skip/remove the repeated values. i.e. have a table
> like this,
> 
> [0, 1, 2, 3  ...  235, 240, 245, 250, 255]
> 
> instead of
> 
> [0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3  ...  235, 240, 245, 250, 255]
> 
> Well, I know there are things to improve but lets see your feedback first
> before dedicate more time on it. The patches were tested on a couple of
> devices but I'll test on more devices meanwhile we discuss about it.

I'm not fully decided on this one but my initial reaction isn't to 
question the concept so much as to ask why the number of levels should 
go in the devicetree at all! We could just make brightness-levels 
optional and get the driver to pick sane curves by default.

I'm sure we can debate what "sane" means for a couple of e-mails yet but 
in principle, given it knows the PWM max counter value, the driver 
should be able to calculate the "right" number of steps too. If we have 
that your core code remains but we don't have to complexify the device

<strawman>
Basically we prefer X (?100 like some of the Intel DRM drivers do for 
connector properties?) steps but we reduce the number of steps if the 
PWM is rather course and we can't get sufficiently different steps.
</strawman>

I guess the summary of what I'm saying is that if we can 
programmatically derive brightness curves then the number of steps is 
not really a property of the hardware and doesn't belong in devicetree.


Daniel.

Re: [RFC 1/2] dt-bindings: pwm-backlight: add brightness-levels-scale property

[Daniel Thompson]

On 04/09/17 16:35, Enric Balletbo i Serra wrote:
> Brightness is not perceived linearly; rather, it typically follows some
> kind of parabolic curve. We can support this by skipping values in the
> brightness-levels array in a pseudo-quadratic curve. Typically we used
> less than 256 levels, which yields no more than 1KiB of memory in our
> device tree. But, we've noticed that on some devices the backlight
> performs much smoother at lower ranges if we have more than 256-levels of
> granularity. On kevin device, for example, if we support all 64K, that
> will waste us at least 256KiB in our device tree.
> 
> Let's avoid to waste memory and have a huge table of numbers in our device
> tree of numbers by adding a brightness-levels-scale property to let the
> driver compute the brightness levels based on one algorithm and their
> property parameters.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>   .../bindings/leds/backlight/pwm-backlight.txt       | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..11c5583 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,16 @@ 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])
> +  - brightness-levels-scale: this can be used instead of 'brightness-levels',
> +                             to represent the perceived luminance. So rather
> +                             than specifying 'brightness-levels = <0
> +                             1 2 ... 65535>', one can simply say
> +                             'brightness-levels-scale = <255 65535>', where the
> +                             first number is the max number of levels and the
> +                             second number is the max PWM value that represent a
> +                             100% duty cycle (brightest). The result is a
> +                             correction table for PWM values to create linear
> +                             brightness based on the CIE1931 algorithm.

Even if we do keep this property (see first e-mail) this second value 
seems pointless to me; can't the driver just use the actual PWM max 
counter value for this?


Daniel.


>   
>   [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>   [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -33,3 +43,14 @@ Example:
>   		power-supply = <&vdd_bl_reg>;
>   		enable-gpios = <&gpio 58 0>;
>   	};
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm 0 5000000>;
> +
> +		brightness-levels-scale = <255 65535>;
> +		default-brightness-level = <128>;
> +
> +		power-supply = <&vdd_bl_reg>;
> +		enable-gpios = <&gpio 58 0>;
> +	};
> 

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

[Daniel Thompson]

On 05/09/17 12:05, Daniel Thompson wrote:
> On 04/09/17 16:35, Enric Balletbo i Serra wrote:
>> Dear all,
>>
>> This patch series is a first RFC to know your opinion about implement
>> support to create brightness levels tables dinamically. I tried to argue
>> in every patch the specific reasons we think this can be interesting, to
>> sumup, the idea behind these patches is be able to pass via device tree
>> two parameters to the driver so it can calculate the brightness levels
>> based on the CIE 1931 lightness formula, which is what actually describes
>> how we perceive light.
>>
>> I think that at least the maths involved can be improved, and I've still
>> some doubts. With current code if you create a table with a max PWM
>> value of 255 and 127 steps, the first numbers are repeated so I'm 
>> thinking > that maybe we should skip/remove the repeated values. i.e. 
>> have a table
>> like this,
>>
>> [0, 1, 2, 3  ...  235, 240, 245, 250, 255]
>>
>> instead of
>>
>> [0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3  ...  235, 240, 245, 250, 255]
>>
>> Well, I know there are things to improve but lets see your feedback first
>> before dedicate more time on it. The patches were tested on a couple of
>> devices but I'll test on more devices meanwhile we discuss about it.
> 
> I'm not fully decided on this one but my initial reaction isn't to 
> question the concept so much as to ask why the number of levels should 
> go in the devicetree at all! We could just make brightness-levels 
> optional and get the driver to pick sane curves by default.
> 
> I'm sure we can debate what "sane" means for a couple of e-mails yet but 
> in principle, given it knows the PWM max counter value, the driver 
> should be able to calculate the "right" number of steps too. If we have 
> that your core code remains but we don't have to complexify the device

... tree

Sorry. ;-)


Daniel.


> 
> <strawman>
> Basically we prefer X (?100 like some of the Intel DRM drivers do for 
> connector properties?) steps but we reduce the number of steps if the 
> PWM is rather course and we can't get sufficiently different steps.
> </strawman>
> 
> I guess the summary of what I'm saying is that if we can 
> programmatically derive brightness curves then the number of steps is 
> not really a property of the hardware and doesn't belong in devicetree.
> 
> 
> Daniel.

Re: [RFC 1/2] dt-bindings: pwm-backlight: add brightness-levels-scale property

[Guenter Roeck]

On Mon, Sep 4, 2017 at 8:35 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Brightness is not perceived linearly; rather, it typically follows some
> kind of parabolic curve. We can support this by skipping values in the
> brightness-levels array in a pseudo-quadratic curve. Typically we used
> less than 256 levels, which yields no more than 1KiB of memory in our
> device tree. But, we've noticed that on some devices the backlight
> performs much smoother at lower ranges if we have more than 256-levels of
> granularity. On kevin device, for example, if we support all 64K, that
> will waste us at least 256KiB in our device tree.
>
> Let's avoid to waste memory and have a huge table of numbers in our device
> tree of numbers by adding a brightness-levels-scale property to let the
> driver compute the brightness levels based on one algorithm and their
> property parameters.
>

Given Daniel's comments, would it possibly make sense to provide
something like "brightness-level-range = <0, 65536>" instead and let
the driver handle details ?

Thanks,
Guenter

> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>  .../bindings/leds/backlight/pwm-backlight.txt       | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> index 764db86..11c5583 100644
> --- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> +++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
> @@ -17,6 +17,16 @@ 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])
> +  - brightness-levels-scale: this can be used instead of 'brightness-levels',
> +                             to represent the perceived luminance. So rather
> +                             than specifying 'brightness-levels = <0
> +                             1 2 ... 65535>', one can simply say
> +                             'brightness-levels-scale = <255 65535>', where the
> +                             first number is the max number of levels and the
> +                             second number is the max PWM value that represent a
> +                             100% duty cycle (brightest). The result is a
> +                             correction table for PWM values to create linear
> +                             brightness based on the CIE1931 algorithm.
>
>  [0]: Documentation/devicetree/bindings/pwm/pwm.txt
>  [1]: Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -33,3 +43,14 @@ Example:
>                 power-supply = <&vdd_bl_reg>;
>                 enable-gpios = <&gpio 58 0>;
>         };
> +
> +       backlight {
> +               compatible = "pwm-backlight";
> +               pwms = <&pwm 0 5000000>;
> +
> +               brightness-levels-scale = <255 65535>;
> +               default-brightness-level = <128>;
> +
> +               power-supply = <&vdd_bl_reg>;
> +               enable-gpios = <&gpio 58 0>;
> +       };
> --
> 2.9.3
>

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

[Jingoo Han]

On Tuesday, September 5, 2017 7:06 AM, Daniel Thompson wrote:
> 
> On 04/09/17 16:35, Enric Balletbo i Serra wrote:
> > Dear all,
> >
> > This patch series is a first RFC to know your opinion about implement
> > support to create brightness levels tables dinamically. I tried to argue
> > in every patch the specific reasons we think this can be interesting, to
> > sumup, the idea behind these patches is be able to pass via device tree
> > two parameters to the driver so it can calculate the brightness levels
> > based on the CIE 1931 lightness formula, which is what actually
> describes
> > how we perceive light.
> >
> > I think that at least the maths involved can be improved, and I've still
> > some doubts. With current code if you create a table with a max PWM
> > value of 255 and 127 steps, the first numbers are repeated so I'm
> thinking > that maybe we should skip/remove the repeated values. i.e. have
> a table
> > like this,
> >
> > [0, 1, 2, 3  ...  235, 240, 245, 250, 255]
> >
> > instead of
> >
> > [0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3  ...  235, 240, 245, 250, 255]
> >
> > Well, I know there are things to improve but lets see your feedback
> first
> > before dedicate more time on it. The patches were tested on a couple of
> > devices but I'll test on more devices meanwhile we discuss about it.
> 
> I'm not fully decided on this one but my initial reaction isn't to
> question the concept so much as to ask why the number of levels should
> go in the devicetree at all! We could just make brightness-levels
> optional and get the driver to pick sane curves by default.
> 
> I'm sure we can debate what "sane" means for a couple of e-mails yet but
> in principle, given it knows the PWM max counter value, the driver
> should be able to calculate the "right" number of steps too. If we have
> that your core code remains but we don't have to complexify the device
> 
> <strawman>
> Basically we prefer X (?100 like some of the Intel DRM drivers do for
> connector properties?) steps but we reduce the number of steps if the
> PWM is rather course and we can't get sufficiently different steps.
> </strawman>
> 
> I guess the summary of what I'm saying is that if we can
> programmatically derive brightness curves then the number of steps is
> not really a property of the hardware and doesn't belong in devicetree.

Yep, I agree with Daniel's opinion. I cannot find the reason
this feature can be added to the device tree.

In my opinion, this feature can be handled by upper user level layer,
not backlight framework level. However, we can discuss this topic to
find how to handle it.

Best regards,
Jingoo Han

> 
> 
> Daniel.

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

[Doug Anderson]

Hi,

On Tue, Sep 5, 2017 at 9:34 AM, Jingoo Han <jingoohan1@gmail.com> wrote:
> On Tuesday, September 5, 2017 7:06 AM, Daniel Thompson wrote:
>>
>> On 04/09/17 16:35, Enric Balletbo i Serra wrote:
>> > Dear all,
>> >
>> > This patch series is a first RFC to know your opinion about implement
>> > support to create brightness levels tables dinamically. I tried to argue
>> > in every patch the specific reasons we think this can be interesting, to
>> > sumup, the idea behind these patches is be able to pass via device tree
>> > two parameters to the driver so it can calculate the brightness levels
>> > based on the CIE 1931 lightness formula, which is what actually
>> describes
>> > how we perceive light.
>> >
>> > I think that at least the maths involved can be improved, and I've still
>> > some doubts. With current code if you create a table with a max PWM
>> > value of 255 and 127 steps, the first numbers are repeated so I'm
>> thinking > that maybe we should skip/remove the repeated values. i.e. have
>> a table
>> > like this,
>> >
>> > [0, 1, 2, 3  ...  235, 240, 245, 250, 255]
>> >
>> > instead of
>> >
>> > [0, 0, 0, 1, 1, 1, 1, 2, 2, 2, 2, 2, 3  ...  235, 240, 245, 250, 255]
>> >
>> > Well, I know there are things to improve but lets see your feedback
>> first
>> > before dedicate more time on it. The patches were tested on a couple of
>> > devices but I'll test on more devices meanwhile we discuss about it.
>>
>> I'm not fully decided on this one but my initial reaction isn't to
>> question the concept so much as to ask why the number of levels should
>> go in the devicetree at all! We could just make brightness-levels
>> optional and get the driver to pick sane curves by default.
>>
>> I'm sure we can debate what "sane" means for a couple of e-mails yet but
>> in principle, given it knows the PWM max counter value, the driver
>> should be able to calculate the "right" number of steps too. If we have
>> that your core code remains but we don't have to complexify the device
>>
>> <strawman>
>> Basically we prefer X (?100 like some of the Intel DRM drivers do for
>> connector properties?) steps but we reduce the number of steps if the
>> PWM is rather course and we can't get sufficiently different steps.
>> </strawman>
>>
>> I guess the summary of what I'm saying is that if we can
>> programmatically derive brightness curves then the number of steps is
>> not really a property of the hardware and doesn't belong in devicetree.
>
> Yep, I agree with Daniel's opinion. I cannot find the reason
> this feature can be added to the device tree.
>
> In my opinion, this feature can be handled by upper user level layer,
> not backlight framework level. However, we can discuss this topic to
> find how to handle it.

Warning ahead of time: I'm not an expert.  If something I'm saying is
blatantly wrong (or even a little wrong), please correct me.

--

I'd agree that I don't think we should land Enric's series as-is.
...but I think something has been missing from the discussion so far:
the fact that the backlight driver doesn't necessarily increase light
output (in Watts) linearly in response to a linear increase in PWM
duty cycle.

I think that we can all agree that the end result is to be able to
have a backlight that is easy to scale linearly with the human
perception of brightness (aka in Lumens).  Right?  So how do we get
there?

For PWM backlight, there are two inputs to the system:

1. PWM Frequency

2. PWM Duty Cycle

I think we all know that in (almost?) all cases scaling PWM Duty Cycle
linearly doesn't result in a linear increase of perceived brightness
in Lumens.


So far in these discussions folks have been assuming that if we just
apply cie1931 to the PWM Duty Cycle then we're done and we have
perceived brightness in Lumens.  ...but I think that's not quite
right.  There are more factors.  Let's use the datasheet for a random
backlight driver, like RT8561A.  There appears to be a public
datasheet at <http://www.richtek.com/assets/product_file/RT8561A/DS8561A-02.pdf>.

A) There may be a non-linear curve between PWM Duty Cycle and LED
Current (mA).  The particular curve is different based on mode
(Digital Ctrl vs. Analog Ctrl) and also PWM Frequency.  Sometimes this
curve is nearly linear for large parts of the curve but not the whole
curve.  Sometimes even though the curve is nearly linear there is an
offset (AKA 10% duty cycle could still produce nearly zero light
output).

B) There may be a non-linear curve between LED current and light
output in Watts (I think?).

C) The human perception model means there is a non-linear curve
between light output in Watts and human perceived brightness in
Lumens.


So A and B are hardware dependent and _do_ belong in the device tree (IMHO).


...then the question is whether the device tree should specify the
curve so that the Watts scales linearly (and then the kernel adjust
for human perception) or so that Lumens scales linearly (which is
already adjusted for human perception).

Historically I believe the device tree has always wanted it so that
Lumens scales linearly.  So I guess the "we don't do anything" answer
is that the device tree should help account for for A + B + C.


If someone can show that it's useful (for some reason) to specify "A +
B" and have the cie1931 transformation happen in the kernel then we
can look at that.  I don't personally know if this is a savings or
not.  It depends on whether "A + B" is somehow easier to find (from
datasheets?) or is somehow more likely to be more linear.

--

So you could say: the current device tree already says that we want to
define a table that accounts for A + B + C, so why do we need to do
anything?

...we need to do something because currently the only way to specify
the curve in the device tree is to specify every single point in the
curve.  Then you're only allows to set the backlight to one of those
exact points.  That's bad because:

1. Specifying every point is a big waste of space.  Specifying 256
points in the device tree wastes 1K.

2. Ideally you'd want to specify more than 256 points.  If you bump a
backlight up by 1 / 256 you can notice it and it seems jerky.  If you
change the software to instead do 16 increases each by 1 / 4096 then
it is a much smoother transition.  ...but specifying 4096 points in
the device tree would waste 16 K (!).


...one proposal to fix this is to add some way to specify
piecewise-linear in the device tree.  Using piecewise linear you can
specify a nearly arbitrary curve with not too many points.  The idea
here is that you're not limiting yourself to only selecting the points
on the curve.

Hopefully Enric can try prototyping this up...

--

One last note is that it would be nice to find some way to make it
easier for people to get this right.  I will take responsibility and
admit that I've been involved in device trees that just specified a
linear curve from 1 to 256.  That's not quite right, but it's never
been terribly easy to measure the correct curve (and never super
critical).  On Chrome OS (where I've been working) historically I
believe that the cie1931 transformation has historically happened in
userspace, so effectively above I've asserted that "A + B" was linear
enough and then we've done "C" programmatically.

I'm not saying what we've done in the past is terribly correct, but I
am saying that we should definitely take into account making this very
easy for people to get right.  Possibly this could be as simple as
documenting a good / cheap light meter and how exactly to generate a
table...


-Doug

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

[Daniel Thompson]

On 07/09/17 19:04, Doug Anderson wrote:
 > I'd agree that I don't think we should land Enric's series as-is.
 > ...but I think something has been missing from the discussion so far:
 > the fact that the backlight driver doesn't necessarily increase light
 > output (in Watts) linearly in response to a linear increase in PWM
 > duty cycle.
 >
 > I think that we can all agree that the end result is to be able to
 > have a backlight that is easy to scale linearly with the human
 > perception of brightness (aka in Lumens).  Right?  So how do we get
 > there?

I'd suggest we avoid talking about watts (measure of power, not limited 
to visible light) and lumens (measure of visible light, preceptually 
weighted for colour but *not* for perceived brightness).


> So far in these discussions folks have been assuming that if we just
> apply cie1931 to the PWM Duty Cycle then we're done and we have
> perceived brightness in Lumens.  ...but I think that's not quite
> right.  There are more factors.  Let's use the datasheet for a random
> backlight driver, like RT8561A.  There appears to be a public
> datasheet at <http://www.richtek.com/assets/product_file/RT8561A/DS8561A-02.pdf>.
> 
> A) There may be a non-linear curve between PWM Duty Cycle and LED
> Current (mA).  The particular curve is different based on mode
> (Digital Ctrl vs. Analog Ctrl) and also PWM Frequency.  Sometimes this
> curve is nearly linear for large parts of the curve but not the whole
> curve.  Sometimes even though the curve is nearly linear there is an
> offset (AKA 10% duty cycle could still produce nearly zero light
> output).
> 
> B) There may be a non-linear curve between LED current and light
> output in Watts (I think?).
> 
> C) The human perception model means there is a non-linear curve
> between light output in Watts and human perceived brightness in
> Lumens.
>
> So A and B are hardware dependent and _do_ belong in the device tree (IMHO).

You forgot to model how to screen size and its maximum light output of 
the backlight impact pupil dilation ;-).

Or... putting it another way, A and B are only relevant if they help us 
eliminate significant sources of error.


> ...then the question is whether the device tree should specify the
> curve so that the Watts scales linearly (and then the kernel adjust
> for human perception) or so that Lumens scales linearly (which is
> already adjusted for human perception).
> 
> Historically I believe the device tree has always wanted it so that
> Lumens scales linearly.  So I guess the "we don't do anything" answer
> is that the device tree should help account for for A + B + C.

I would interpret the history slightly differently (although I'm not an 
authoritative historian here).

There is a problem with the backlight interfaces (but entirely unrelated 
to Enric'c patch). The units the backlight users are not defined and 
varies from driver to driver.

The userspace has never been able to tell but since most PC backlight 
controls are perceptually weighted (through "magic" in the BIOS) we 
didn't really discover the problems until lots of embedded folks had 
added backlight drivers and many of these used linear controls.

Anyhow we're stuck where we are... and we probably shouldn't bog down 
discussion of it (since Enric's patch only affects one of the many drivers.


> ...one proposal to fix this is to add some way to specify
> piecewise-linear in the device tree.  Using piecewise linear you can
> specify a nearly arbitrary curve with not too many points.  The idea
> here is that you're not limiting yourself to only selecting the points
> on the curve.
 > Hopefully Enric can try prototyping this up...

You mean have an additional property that allows the driver to linearly 
interpolate between steps in the brightness-levels table (so it can 
provide more steps than are described)?

Sounds OK to me although I'm still interested in whether an automatic 
table can be "right enough"...


> One last note is that it would be nice to find some way to make it
> easier for people to get this right.

... and this is why.

One great aspect of Enric's current work is that it has the potential to 
allow the driver to get it "right enough" with little effort by the DT 
author.

Having said that allowing the driver to interpolate and having a 
reference table in the driver (to allow brightness-levels to be 
optional) would do the same thing and spare me having to review all the 
fixed point maths for the CIE 1931 mappings as it evolves ;-)


> I will take responsibility and admit that I've been involved in
> device trees that just specified a > linear curve from 1 to 256.  That's
 > not quite right, but it's never
> been terribly easy to measure the correct curve (and never super
> critical).  On Chrome OS (where I've been working) historically I
> believe that the cie1931 transformation has historically happened in
> userspace, so effectively above I've asserted that "A + B" was linear
> enough and then we've done "C" programmatically.

Right now I've been assuming the best a userspace can do is:

  - Assume the backlight is perceptually weighted (since IIRC most x86
    PCs are)

  - Use quirks tables (and perhaps also take a sneaky peak to identify
    "lazy" brightness-level tables in the DT) and do some kind of
    linear-to-perceptual mapping (where CIE 1931 is as good a model as
    anything else)

Is ChromeOS doing anything like this is it just a bit of userspace 
configuration to decide whether or not to apply a perceptual model?


> I'm not saying what we've done in the past is terribly correct, but I
> am saying that we should definitely take into account making this very
> easy for people to get right.  Possibly this could be as simple as
> documenting a good / cheap light meter and how exactly to generate a
> table...

Again no objections... but TBH I just don't see many DT authors actually 
breaking out the light meter and doing it.


Daniel.

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

[Doug Anderson]

Hi,

On Fri, Sep 8, 2017 at 4:18 AM, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> On 07/09/17 19:04, Doug Anderson wrote:
>> I'd agree that I don't think we should land Enric's series as-is.
>> ...but I think something has been missing from the discussion so far:
>> the fact that the backlight driver doesn't necessarily increase light
>> output (in Watts) linearly in response to a linear increase in PWM
>> duty cycle.
>>
>> I think that we can all agree that the end result is to be able to
>> have a backlight that is easy to scale linearly with the human
>> perception of brightness (aka in Lumens).  Right?  So how do we get
>> there?
>
> I'd suggest we avoid talking about watts (measure of power, not limited to
> visible light) and lumens (measure of visible light, preceptually weighted
> for colour but *not* for perceived brightness).

Ah, OK.  I thought lumens accounted for perceived brightness?  I was
having a hard time following all the stuff I could find online about
this.  Mostly I thought I saw people talking about Luminous Flux
(measured in Lumens), but maybe I was mistaken.


>> So far in these discussions folks have been assuming that if we just
>> apply cie1931 to the PWM Duty Cycle then we're done and we have
>> perceived brightness in Lumens.  ...but I think that's not quite
>> right.  There are more factors.  Let's use the datasheet for a random
>> backlight driver, like RT8561A.  There appears to be a public
>> datasheet at
>> <http://www.richtek.com/assets/product_file/RT8561A/DS8561A-02.pdf>.
>>
>> A) There may be a non-linear curve between PWM Duty Cycle and LED
>> Current (mA).  The particular curve is different based on mode
>> (Digital Ctrl vs. Analog Ctrl) and also PWM Frequency.  Sometimes this
>> curve is nearly linear for large parts of the curve but not the whole
>> curve.  Sometimes even though the curve is nearly linear there is an
>> offset (AKA 10% duty cycle could still produce nearly zero light
>> output).
>>
>> B) There may be a non-linear curve between LED current and light
>> output in Watts (I think?).
>>
>> C) The human perception model means there is a non-linear curve
>> between light output in Watts and human perceived brightness in
>> Lumens.
>>
>> So A and B are hardware dependent and _do_ belong in the device tree
>> (IMHO).
>
>
> You forgot to model how to screen size and its maximum light output of the
> backlight impact pupil dilation ;-).

Silly me...  Oops, I also forgot to account for the absolute humidity
of the room.  Do you think we can require all backlights come with a
humidity sensor?


> Or... putting it another way, A and B are only relevant if they help us
> eliminate significant sources of error.

Right.  ...so your point is we can't model everything and we just need
to choose what's important.

I'll agree that "B" above might not be worth modelling (though I don't
know).  ...but I think we need to do _something_ about A.

>From the datasheet I point at looking at "Figure 8. LED Current vs.
ACTL PWM Dimming Duty Cycle", it seems like we at least need to do
something to account for the curve if we happen to be running at 30
kHz for whatever reason.  Specifically if we do no other work then any
duty cycle below 8% will result in no brightness.  Eyeballing the
graph 10% duty cycle looks to be about 2% current.

One option to solve this type of problem is to to specify a minimum
offset and assume things are linear after that offset.  That might
work, but it also might prevent you from accessing some of those nice
low brightness points.  Historically I have been frustrated when in
dark rooms that I couldn't set the brightness to be dim enough...

The whole piecewise linear concept is that maybe you'd specify the
curve (in terms of milliPercent) like this (values found by measuring
datasheet curve with a ruler):

<0 0>
<10000 1800> # 10% duty cycle gives 1.8% current
<12000 4300> # 12% duty cycle gives 4.3% current
<17000 10000> # 17% duty cycle gives 10% current
<93000 90000> # 93% duty cycle gives 90% current
<100000 100000> # 100% duty cycle gives 100% current


>> ...then the question is whether the device tree should specify the
>> curve so that the Watts scales linearly (and then the kernel adjust
>> for human perception) or so that Lumens scales linearly (which is
>> already adjusted for human perception).
>>
>> Historically I believe the device tree has always wanted it so that
>> Lumens scales linearly.  So I guess the "we don't do anything" answer
>> is that the device tree should help account for for A + B + C.
>
>
> I would interpret the history slightly differently (although I'm not an
> authoritative historian here).
>
> There is a problem with the backlight interfaces (but entirely unrelated to
> Enric'c patch). The units the backlight users are not defined and varies
> from driver to driver.
>
> The userspace has never been able to tell but since most PC backlight
> controls are perceptually weighted (through "magic" in the BIOS) we didn't
> really discover the problems until lots of embedded folks had added
> backlight drivers and many of these used linear controls.
>
> Anyhow we're stuck where we are... and we probably shouldn't bog down
> discussion of it (since Enric's patch only affects one of the many drivers.

See below discussion of how Chrome OS userspace deals with this today,
but it feels like we need to come up with a less hacky solution...


>> ...one proposal to fix this is to add some way to specify
>> piecewise-linear in the device tree.  Using piecewise linear you can
>> specify a nearly arbitrary curve with not too many points.  The idea
>> here is that you're not limiting yourself to only selecting the points
>> on the curve.
>
>> Hopefully Enric can try prototyping this up...
>
> You mean have an additional property that allows the driver to linearly
> interpolate between steps in the brightness-levels table (so it can provide
> more steps than are described)?
>
> Sounds OK to me although I'm still interested in whether an automatic table
> can be "right enough"...

My secret plan was that Enric could implement piecewise linear
support, which really should be a very simple bit of code.  This would
allow people to model the non-linearlity of their hardware if they
wanted but would add very little overhead if they didn't want to.  AKA
the lazy way to specify your "curve":

  <0 0>
  <100000 100000> # 100% duty cycle gives 100% current

...and of course you could just assume this "curve" if nothing was specified...

...then there's the question of whether or not Enric's human
perception work should be applied atop this or if the super-cool-elite
status of pieceiwse linear subsumes the need.  Said another way: once
you have piecewise linear it would be pretty easy to manually apply
the human perception model to your curve before putting it in the
device tree...

I guess the cop out answer is to add an attribute like
"apply-human-perception-factor".  The presence of this attribute will
cause Enric's code to run.  The lack of this attribute will cause it
not to run.  Thus if you assume that hardware response is nearly
linear (without counting human perception) and you want human
perceived output, you'd do:

  <0 0>
  <100000 100000> # 100% duty cycle gives 100% current
  apply-human-perception-factor


If you want to go whole-hog and get out the light meter, it's probably
pretty easy to just specify the full curve _after_ applying the human
factor...


> One great aspect of Enric's current work is that it has the potential to
> allow the driver to get it "right enough" with little effort by the DT
> author.
>
> Having said that allowing the driver to interpolate and having a reference
> table in the driver (to allow brightness-levels to be optional) would do the
> same thing and spare me having to review all the fixed point maths for the
> CIE 1931 mappings as it evolves ;-)

Right.  Essentially you'd be making the lazy solution currently
implied in many Chrome OS boards (assume mostly linear hardware
response) easier to do.


> Right now I've been assuming the best a userspace can do is:
>
>  - Assume the backlight is perceptually weighted (since IIRC most x86
>    PCs are)
>
>  - Use quirks tables (and perhaps also take a sneaky peak to identify
>    "lazy" brightness-level tables in the DT) and do some kind of
>    linear-to-perceptual mapping (where CIE 1931 is as good a model as
>    anything else)
>
> Is ChromeOS doing anything like this is it just a bit of userspace
> configuration to decide whether or not to apply a perceptual model?

Right now Chrome OS has what's a bit of a hack.  You can see the code at:

https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/powerd/policy/internal_backlight_controller.cc

Basically if the backlight has more than 100
(kMinLevelsForNonLinearMapping) levels then it does some conversions
to adjust things to be exponential.  I guess the idea is that the
magic BIOS devices usually have < 100 levels and PWM backlights have
more?


The hope was that we could adjust the kernel to be consistent and then
fix userspace to just drive everything the same way...


-Doug

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

[Enric Balletbo Serra]

Hi,

2017-09-08 19:39 GMT+02:00 Doug Anderson <dianders@google.com>:
> Hi,
>
> On Fri, Sep 8, 2017 at 4:18 AM, Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
>> On 07/09/17 19:04, Doug Anderson wrote:
>>> I'd agree that I don't think we should land Enric's series as-is.
>>> ...but I think something has been missing from the discussion so far:
>>> the fact that the backlight driver doesn't necessarily increase light
>>> output (in Watts) linearly in response to a linear increase in PWM
>>> duty cycle.
>>>
>>> I think that we can all agree that the end result is to be able to
>>> have a backlight that is easy to scale linearly with the human
>>> perception of brightness (aka in Lumens).  Right?  So how do we get
>>> there?
>>
>> I'd suggest we avoid talking about watts (measure of power, not limited to
>> visible light) and lumens (measure of visible light, preceptually weighted
>> for colour but *not* for perceived brightness).
>
> Ah, OK.  I thought lumens accounted for perceived brightness?  I was
> having a hard time following all the stuff I could find online about
> this.  Mostly I thought I saw people talking about Luminous Flux
> (measured in Lumens), but maybe I was mistaken.
>
>
>>> So far in these discussions folks have been assuming that if we just
>>> apply cie1931 to the PWM Duty Cycle then we're done and we have
>>> perceived brightness in Lumens.  ...but I think that's not quite
>>> right.  There are more factors.  Let's use the datasheet for a random
>>> backlight driver, like RT8561A.  There appears to be a public
>>> datasheet at
>>> <http://www.richtek.com/assets/product_file/RT8561A/DS8561A-02.pdf>.
>>>
>>> A) There may be a non-linear curve between PWM Duty Cycle and LED
>>> Current (mA).  The particular curve is different based on mode
>>> (Digital Ctrl vs. Analog Ctrl) and also PWM Frequency.  Sometimes this
>>> curve is nearly linear for large parts of the curve but not the whole
>>> curve.  Sometimes even though the curve is nearly linear there is an
>>> offset (AKA 10% duty cycle could still produce nearly zero light
>>> output).
>>>
>>> B) There may be a non-linear curve between LED current and light
>>> output in Watts (I think?).
>>>
>>> C) The human perception model means there is a non-linear curve
>>> between light output in Watts and human perceived brightness in
>>> Lumens.
>>>
>>> So A and B are hardware dependent and _do_ belong in the device tree
>>> (IMHO).
>>
>>
>> You forgot to model how to screen size and its maximum light output of the
>> backlight impact pupil dilation ;-).
>
> Silly me...  Oops, I also forgot to account for the absolute humidity
> of the room.  Do you think we can require all backlights come with a
> humidity sensor?
>
>
>> Or... putting it another way, A and B are only relevant if they help us
>> eliminate significant sources of error.
>
> Right.  ...so your point is we can't model everything and we just need
> to choose what's important.
>
> I'll agree that "B" above might not be worth modelling (though I don't
> know).  ...but I think we need to do _something_ about A.
>
> From the datasheet I point at looking at "Figure 8. LED Current vs.
> ACTL PWM Dimming Duty Cycle", it seems like we at least need to do
> something to account for the curve if we happen to be running at 30
> kHz for whatever reason.  Specifically if we do no other work then any
> duty cycle below 8% will result in no brightness.  Eyeballing the
> graph 10% duty cycle looks to be about 2% current.
>
> One option to solve this type of problem is to to specify a minimum
> offset and assume things are linear after that offset.  That might
> work, but it also might prevent you from accessing some of those nice
> low brightness points.  Historically I have been frustrated when in
> dark rooms that I couldn't set the brightness to be dim enough...
>
> The whole piecewise linear concept is that maybe you'd specify the
> curve (in terms of milliPercent) like this (values found by measuring
> datasheet curve with a ruler):
>
> <0 0>
> <10000 1800> # 10% duty cycle gives 1.8% current
> <12000 4300> # 12% duty cycle gives 4.3% current
> <17000 10000> # 17% duty cycle gives 10% current
> <93000 90000> # 93% duty cycle gives 90% current
> <100000 100000> # 100% duty cycle gives 100% current
>
>
>>> ...then the question is whether the device tree should specify the
>>> curve so that the Watts scales linearly (and then the kernel adjust
>>> for human perception) or so that Lumens scales linearly (which is
>>> already adjusted for human perception).
>>>
>>> Historically I believe the device tree has always wanted it so that
>>> Lumens scales linearly.  So I guess the "we don't do anything" answer
>>> is that the device tree should help account for for A + B + C.
>>
>>
>> I would interpret the history slightly differently (although I'm not an
>> authoritative historian here).
>>
>> There is a problem with the backlight interfaces (but entirely unrelated to
>> Enric'c patch). The units the backlight users are not defined and varies
>> from driver to driver.
>>

Based on this seems reasonable maintain current implementation to not
break backward compability. Even, I think makes sense improve current
implementation by adding somekind of piecewise linear concept to the
brightness levels, similar to Doug's suggestion. So if we want, i.e,
256 levels or more, instead of specify the full table in the DT we can
only specify some points in DT but the driver can expose to userspace
more steps (how many?) between two brightness levels. Of course, this
doesn't makes the live of the future users easier but I think will
make the live of the current users of this interface more flexible
(specially when you want lots of levels)

Then, to make the user live easier, there is the thing about human
perception, we can move brightness-levels to be optional and fall to
apply the human perception code if it's not specified. Here the thing
and point of discussion is, if the cie1931 is the right algorithm to
do the 'magic' in the driver. From what I investigated seems that is
but I might be wrong.

Seems reasonable apply both solutions? I can send a second RFC with
both approaches.

>> The userspace has never been able to tell but since most PC backlight
>> controls are perceptually weighted (through "magic" in the BIOS) we didn't
>> really discover the problems until lots of embedded folks had added
>> backlight drivers and many of these used linear controls.
>>
>> Anyhow we're stuck where we are... and we probably shouldn't bog down
>> discussion of it (since Enric's patch only affects one of the many drivers.
>
> See below discussion of how Chrome OS userspace deals with this today,
> but it feels like we need to come up with a less hacky solution...
>
>
>>> ...one proposal to fix this is to add some way to specify
>>> piecewise-linear in the device tree.  Using piecewise linear you can
>>> specify a nearly arbitrary curve with not too many points.  The idea
>>> here is that you're not limiting yourself to only selecting the points
>>> on the curve.
>>
>>> Hopefully Enric can try prototyping this up...
>>
>> You mean have an additional property that allows the driver to linearly
>> interpolate between steps in the brightness-levels table (so it can provide
>> more steps than are described)?
>>
>> Sounds OK to me although I'm still interested in whether an automatic table
>> can be "right enough"...
>
> My secret plan was that Enric could implement piecewise linear
> support, which really should be a very simple bit of code.  This would
> allow people to model the non-linearlity of their hardware if they
> wanted but would add very little overhead if they didn't want to.  AKA
> the lazy way to specify your "curve":
>
>   <0 0>
>   <100000 100000> # 100% duty cycle gives 100% current
>
> ...and of course you could just assume this "curve" if nothing was specified...
>
> ...then there's the question of whether or not Enric's human
> perception work should be applied atop this or if the super-cool-elite
> status of pieceiwse linear subsumes the need.  Said another way: once
> you have piecewise linear it would be pretty easy to manually apply
> the human perception model to your curve before putting it in the
> device tree...
>
> I guess the cop out answer is to add an attribute like
> "apply-human-perception-factor".  The presence of this attribute will
> cause Enric's code to run.  The lack of this attribute will cause it
> not to run.  Thus if you assume that hardware response is nearly
> linear (without counting human perception) and you want human
> perceived output, you'd do:
>
>   <0 0>
>   <100000 100000> # 100% duty cycle gives 100% current
>   apply-human-perception-factor
>
>
> If you want to go whole-hog and get out the light meter, it's probably
> pretty easy to just specify the full curve _after_ applying the human
> factor...
>
>
>> One great aspect of Enric's current work is that it has the potential to
>> allow the driver to get it "right enough" with little effort by the DT
>> author.
>>
>> Having said that allowing the driver to interpolate and having a reference
>> table in the driver (to allow brightness-levels to be optional) would do the
>> same thing and spare me having to review all the fixed point maths for the
>> CIE 1931 mappings as it evolves ;-)
>
> Right.  Essentially you'd be making the lazy solution currently
> implied in many Chrome OS boards (assume mostly linear hardware
> response) easier to do.
>
>
>> Right now I've been assuming the best a userspace can do is:
>>
>>  - Assume the backlight is perceptually weighted (since IIRC most x86
>>    PCs are)
>>
>>  - Use quirks tables (and perhaps also take a sneaky peak to identify
>>    "lazy" brightness-level tables in the DT) and do some kind of
>>    linear-to-perceptual mapping (where CIE 1931 is as good a model as
>>    anything else)
>>
>> Is ChromeOS doing anything like this is it just a bit of userspace
>> configuration to decide whether or not to apply a perceptual model?
>
> Right now Chrome OS has what's a bit of a hack.  You can see the code at:
>
> https://chromium.googlesource.com/chromiumos/platform2/+/master/power_manager/powerd/policy/internal_backlight_controller.cc
>
> Basically if the backlight has more than 100
> (kMinLevelsForNonLinearMapping) levels then it does some conversions
> to adjust things to be exponential.  I guess the idea is that the
> magic BIOS devices usually have < 100 levels and PWM backlights have
> more?
>
>
> The hope was that we could adjust the kernel to be consistent and then
> fix userspace to just drive everything the same way...
>
>
> -Doug

Best regards,
 Enric.

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

[Doug Anderson]

Hi,

On Thu, Sep 14, 2017 at 3:46 AM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> Based on this seems reasonable maintain current implementation to not
> break backward compability. Even, I think makes sense improve current
> implementation by adding somekind of piecewise linear concept to the
> brightness levels, similar to Doug's suggestion. So if we want, i.e,
> 256 levels or more, instead of specify the full table in the DT we can
> only specify some points in DT but the driver can expose to userspace
> more steps (how many?) between two brightness levels.

It seems sane to me.  Personally I'd say that if you're using
piecewise linear you just pick a number of levels to expose, perhaps
16383, or 32767, or 65535) and expose that many levels for everyone.
It's possible that bumping the brightness up by "1" will not actually
change a hardware register, but that seems like it would be fine,
right?

Probably you'd want to require some sort of dt change to enable
piecewise linear since it seems plausible that you could break
existing boards if you started interpolating.

> Of course, this
> doesn't makes the live of the future users easier but I think will
> make the live of the current users of this interface more flexible
> (specially when you want lots of levels)
>
> Then, to make the user live easier, there is the thing about human
> perception, we can move brightness-levels to be optional and fall to
> apply the human perception code if it's not specified. Here the thing
> and point of discussion is, if the cie1931 is the right algorithm to
> do the 'magic' in the driver. From what I investigated seems that is
> but I might be wrong.

I don't personally know, so hopefully someone else can comment.


-Doug

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

[Daniel Thompson]

On 14/09/17 11:46, Enric Balletbo Serra wrote:
>>>> So far in these discussions folks have been assuming that if we just
>>>> apply cie1931 to the PWM Duty Cycle then we're done and we have
>>>> perceived brightness in Lumens.  ...but I think that's not quite
>>>> right.  There are more factors.  Let's use the datasheet for a random
>>>> backlight driver, like RT8561A.  There appears to be a public
>>>> datasheet at
>>>> <http://www.richtek.com/assets/product_file/RT8561A/DS8561A-02.pdf>.
>>>>
>>>> A) There may be a non-linear curve between PWM Duty Cycle and LED
>>>> Current (mA).  The particular curve is different based on mode
>>>> (Digital Ctrl vs. Analog Ctrl) and also PWM Frequency.  Sometimes this
>>>> curve is nearly linear for large parts of the curve but not the whole
>>>> curve.  Sometimes even though the curve is nearly linear there is an
>>>> offset (AKA 10% duty cycle could still produce nearly zero light
>>>> output).
>>>>
>>>> B) There may be a non-linear curve between LED current and light
>>>> output in Watts (I think?).
>>>>
>>>> C) The human perception model means there is a non-linear curve
>>>> between light output in Watts and human perceived brightness in
>>>> Lumens.
>>>>
>>>> So A and B are hardware dependent and _do_ belong in the device tree
>>>> (IMHO).
>>>
>>>
>>> You forgot to model how to screen size and its maximum light output of the
>>> backlight impact pupil dilation ;-).
>>
>> Silly me...  Oops, I also forgot to account for the absolute humidity
>> of the room.  Do you think we can require all backlights come with a
>> humidity sensor?
>>
>>
>>> Or... putting it another way, A and B are only relevant if they help us
>>> eliminate significant sources of error.
>>
>> Right.  ...so your point is we can't model everything and we just need
>> to choose what's important.
>>
>> I'll agree that "B" above might not be worth modelling (though I don't
>> know).  ...but I think we need to do _something_ about A.
>>
>>  From the datasheet I point at looking at "Figure 8. LED Current vs.
>> ACTL PWM Dimming Duty Cycle", it seems like we at least need to do
>> something to account for the curve if we happen to be running at 30
>> kHz for whatever reason.  Specifically if we do no other work then any
>> duty cycle below 8% will result in no brightness.  Eyeballing the
>> graph 10% duty cycle looks to be about 2% current.
>>
>> One option to solve this type of problem is to to specify a minimum
>> offset and assume things are linear after that offset.  That might
>> work, but it also might prevent you from accessing some of those nice
>> low brightness points.  Historically I have been frustrated when in
>> dark rooms that I couldn't set the brightness to be dim enough...
>>
>> The whole piecewise linear concept is that maybe you'd specify the
>> curve (in terms of milliPercent) like this (values found by measuring
>> datasheet curve with a ruler):
>>
>> <0 0>
>> <10000 1800> # 10% duty cycle gives 1.8% current
>> <12000 4300> # 12% duty cycle gives 4.3% current
>> <17000 10000> # 17% duty cycle gives 10% current
>> <93000 90000> # 93% duty cycle gives 90% current
>> <100000 100000> # 100% duty cycle gives 100% current
>>
>>
>>>> ...then the question is whether the device tree should specify the
>>>> curve so that the Watts scales linearly (and then the kernel adjust
>>>> for human perception) or so that Lumens scales linearly (which is
>>>> already adjusted for human perception).
>>>>
>>>> Historically I believe the device tree has always wanted it so that
>>>> Lumens scales linearly.  So I guess the "we don't do anything" answer
>>>> is that the device tree should help account for for A + B + C.
>>>
>>>
>>> I would interpret the history slightly differently (although I'm not an
>>> authoritative historian here).
>>>
>>> There is a problem with the backlight interfaces (but entirely unrelated to
>>> Enric'c patch). The units the backlight users are not defined and varies
>>> from driver to driver.
>>>
> 
> Based on this seems reasonable maintain current implementation to not
> break backward compability. Even, I think makes sense improve current
> implementation by adding somekind of piecewise linear concept to the
> brightness levels, similar to Doug's suggestion. So if we want, i.e,
> 256 levels or more, instead of specify the full table in the DT we can
> only specify some points in DT but the driver can expose to userspace
> more steps (how many?) between two brightness levels. Of course, this
> doesn't makes the live of the future users easier but I think will
> make the live of the current users of this interface more flexible
> (specially when you want lots of levels).

Ideally I'd like the driver to derive the number of steps based on the 
PWM resolution it discovers (I don't entirely agree that having a large 
portion of the slider map to no change is a good thing... we should be 
able to estimate the smallest useful step size).

Having said that, I'm open to suggestions about why we cannot make such 
an estimate.


> Then, to make the user live easier, there is the thing about human
> perception, we can move brightness-levels to be optional and fall to
> apply the human perception code if it's not specified. Here the thing
> and point of discussion is, if the cie1931 is the right algorithm to
> do the 'magic' in the driver. From what I investigated seems that is
> but I might be wrong.

It's certainly more correct than linear ;-) .

Actually I don't recall you commenting on the idea that we could ditch 
the fixed point code and simply have a default table built into the 
driver that can be used if there is no brightness-levels property 
(interpreted by the same piecewise linear code as everything else).

I suspect such a table could be fairly small.


> Seems reasonable apply both solutions? I can send a second RFC with
> both approaches.

Works for me.


Daniel.

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

[Enric Balletbo Serra]

Hi Daniel,

2017-09-18 18:00 GMT+02:00 Daniel Thompson <daniel.thompson@linaro.org>:
> On 14/09/17 11:46, Enric Balletbo Serra wrote:
>>>>>
>>>>> So far in these discussions folks have been assuming that if we just
>>>>> apply cie1931 to the PWM Duty Cycle then we're done and we have
>>>>> perceived brightness in Lumens.  ...but I think that's not quite
>>>>> right.  There are more factors.  Let's use the datasheet for a random
>>>>> backlight driver, like RT8561A.  There appears to be a public
>>>>> datasheet at
>>>>> <http://www.richtek.com/assets/product_file/RT8561A/DS8561A-02.pdf>.
>>>>>
>>>>> A) There may be a non-linear curve between PWM Duty Cycle and LED
>>>>> Current (mA).  The particular curve is different based on mode
>>>>> (Digital Ctrl vs. Analog Ctrl) and also PWM Frequency.  Sometimes this
>>>>> curve is nearly linear for large parts of the curve but not the whole
>>>>> curve.  Sometimes even though the curve is nearly linear there is an
>>>>> offset (AKA 10% duty cycle could still produce nearly zero light
>>>>> output).
>>>>>
>>>>> B) There may be a non-linear curve between LED current and light
>>>>> output in Watts (I think?).
>>>>>
>>>>> C) The human perception model means there is a non-linear curve
>>>>> between light output in Watts and human perceived brightness in
>>>>> Lumens.
>>>>>
>>>>> So A and B are hardware dependent and _do_ belong in the device tree
>>>>> (IMHO).
>>>>
>>>>
>>>>
>>>> You forgot to model how to screen size and its maximum light output of
>>>> the
>>>> backlight impact pupil dilation ;-).
>>>
>>>
>>> Silly me...  Oops, I also forgot to account for the absolute humidity
>>> of the room.  Do you think we can require all backlights come with a
>>> humidity sensor?
>>>
>>>
>>>> Or... putting it another way, A and B are only relevant if they help us
>>>> eliminate significant sources of error.
>>>
>>>
>>> Right.  ...so your point is we can't model everything and we just need
>>> to choose what's important.
>>>
>>> I'll agree that "B" above might not be worth modelling (though I don't
>>> know).  ...but I think we need to do _something_ about A.
>>>
>>>  From the datasheet I point at looking at "Figure 8. LED Current vs.
>>> ACTL PWM Dimming Duty Cycle", it seems like we at least need to do
>>> something to account for the curve if we happen to be running at 30
>>> kHz for whatever reason.  Specifically if we do no other work then any
>>> duty cycle below 8% will result in no brightness.  Eyeballing the
>>> graph 10% duty cycle looks to be about 2% current.
>>>
>>> One option to solve this type of problem is to to specify a minimum
>>> offset and assume things are linear after that offset.  That might
>>> work, but it also might prevent you from accessing some of those nice
>>> low brightness points.  Historically I have been frustrated when in
>>> dark rooms that I couldn't set the brightness to be dim enough...
>>>
>>> The whole piecewise linear concept is that maybe you'd specify the
>>> curve (in terms of milliPercent) like this (values found by measuring
>>> datasheet curve with a ruler):
>>>
>>> <0 0>
>>> <10000 1800> # 10% duty cycle gives 1.8% current
>>> <12000 4300> # 12% duty cycle gives 4.3% current
>>> <17000 10000> # 17% duty cycle gives 10% current
>>> <93000 90000> # 93% duty cycle gives 90% current
>>> <100000 100000> # 100% duty cycle gives 100% current
>>>
>>>
>>>>> ...then the question is whether the device tree should specify the
>>>>> curve so that the Watts scales linearly (and then the kernel adjust
>>>>> for human perception) or so that Lumens scales linearly (which is
>>>>> already adjusted for human perception).
>>>>>
>>>>> Historically I believe the device tree has always wanted it so that
>>>>> Lumens scales linearly.  So I guess the "we don't do anything" answer
>>>>> is that the device tree should help account for for A + B + C.
>>>>
>>>>
>>>>
>>>> I would interpret the history slightly differently (although I'm not an
>>>> authoritative historian here).
>>>>
>>>> There is a problem with the backlight interfaces (but entirely unrelated
>>>> to
>>>> Enric'c patch). The units the backlight users are not defined and varies
>>>> from driver to driver.
>>>>
>>
>> Based on this seems reasonable maintain current implementation to not
>> break backward compability. Even, I think makes sense improve current
>> implementation by adding somekind of piecewise linear concept to the
>> brightness levels, similar to Doug's suggestion. So if we want, i.e,
>> 256 levels or more, instead of specify the full table in the DT we can
>> only specify some points in DT but the driver can expose to userspace
>> more steps (how many?) between two brightness levels. Of course, this
>> doesn't makes the live of the future users easier but I think will
>> make the live of the current users of this interface more flexible
>> (specially when you want lots of levels).
>
>
> Ideally I'd like the driver to derive the number of steps based on the PWM
> resolution it discovers (I don't entirely agree that having a large portion
> of the slider map to no change is a good thing... we should be able to
> estimate the smallest useful step size).
>
> Having said that, I'm open to suggestions about why we cannot make such an
> estimate.
>
>
>> Then, to make the user live easier, there is the thing about human
>> perception, we can move brightness-levels to be optional and fall to
>> apply the human perception code if it's not specified. Here the thing
>> and point of discussion is, if the cie1931 is the right algorithm to
>> do the 'magic' in the driver. From what I investigated seems that is
>> but I might be wrong.
>
>
> It's certainly more correct than linear ;-) .
>
> Actually I don't recall you commenting on the idea that we could ditch the
> fixed point code and simply have a default table built into the driver that
> can be used if there is no brightness-levels property (interpreted by the
> same piecewise linear code as everything else).
>

Sounds a good idea, was on my mind improve the fixed point code, but
this is better, indeed. I'm not sure if one table will be enough
though, maybe we need a table for every PWM resolution? I'll try to do
some tests on some different boards.

Enric

> I suspect such a table could be fairly small.
>
>
>> Seems reasonable apply both solutions? I can send a second RFC with
>> both approaches.
>
>
> Works for me.
>
>
> Daniel.