[PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[Enric Balletbo i Serra]

Dear all,

This series is a third patchset integrating the requested changes.

The first and second 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 thirth and fourth 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 create a brightness-levels 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.

Both functionalities were tested on a Samsung Chromebook Plus (that has
a 16 bits PWM) and a SL50 device (with a 8 bits PWM)

Waiting for your feedback.

Best regards,

Enric Balletbo i Serra (4):
  backlight: pwm_bl: linear interpolation between brightness-levels
  dt-bindings: pwm-backlight: add a num-interpolation-steps property.
  backlight: pwm_bl: compute brightness of LED linearly to human eye.
  dt-bindings: pwm-backlight: move brightness-levels to optional.

 .../bindings/leds/backlight/pwm-backlight.txt      |  34 ++-
 drivers/video/backlight/pwm_bl.c                   | 232 +++++++++++++++++++--
 2 files changed, 246 insertions(+), 20 deletions(-)

-- 
2.15.1

[PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Enric Balletbo i Serra]

Setting num-interpolated-steps in the dts will allow you to have linear
interpolation between values of brightness-levels. 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.

The most simple example is interpolate between two brightness values a
number of steps, this can be done setting the following in the dts:

  brightness-levels = <0 65535>;
  num-interpolated-steps = <1024>;
  default-brightness-level = <512>;

This will create a brightness-level table with the following values:

  <0 63 126 189 252 315 378 441 ... 64260 64323 64386 64449 65535>

Another use case can be describe a gamma corrected curve, as we have
better sensitivity at low luminance than high luminance we probably
want have smaller steps for low brightness levels values and bigger
steps for high brightness levels values. This can be achieved with
the following in the dts:

  brightness-levels = <0 4096 65535>;
  num-interpolated-steps = <1024>;
  default-brightness-level = <512>;

This will create a brightness-levels table with the following values:

  <0 4 8 12 16 20 ... 4096 4156 4216 4276 ... 65535>

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v2:
Requested by Daniel Thompson:
 - Use a devres alloc for table creatiion and then just swap pointers.
 - No need to use calloc because the loop initializes every element.
Changes since v1:
- None.

 drivers/video/backlight/pwm_bl.c | 83 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 8e3f1245f5c5..f0a108ab570a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -147,7 +147,11 @@ static int pwm_backlight_parse_dt(struct device *dev,
 				  struct platform_pwm_backlight_data *data)
 {
 	struct device_node *node = dev->of_node;
+	unsigned int num_levels = 0;
+	unsigned int levels_count;
+	unsigned int num_steps;
 	struct property *prop;
+	unsigned int *table;
 	int length;
 	u32 value;
 	int ret;
@@ -167,6 +171,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
 	/* read brightness levels from DT property */
 	if (data->max_brightness > 0) {
 		size_t size = sizeof(*data->levels) * data->max_brightness;
+		unsigned int i, j, n = 0;
 
 		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
 		if (!data->levels)
@@ -184,6 +189,84 @@ static int pwm_backlight_parse_dt(struct device *dev,
 			return ret;
 
 		data->dft_brightness = value;
+
+		/*
+		 * This property is optional, if is set enables linear
+		 * interpolation between each of the values of brightness levels
+		 * and creates a new pre-computed table.
+		 */
+		of_property_read_u32(node, "num-interpolated-steps",
+				     &num_steps);
+
+		/*
+		 * Make sure that there is at least two entries in the
+		 * brightness-levels table, otherwise we can't interpolate
+		 * between two points.
+		 */
+		if (num_steps) {
+			if (data->max_brightness < 2) {
+				dev_err(dev, "can't interpolate\n");
+				return -EINVAL;
+			}
+
+			/*
+			 * Recalculate the number of brightness levels, now
+			 * taking in consideration the number of interpolated
+			 * steps between two levels.
+			 */
+			for (i = 0; i < data->max_brightness - 1; i++) {
+				if ((data->levels[i + 1] - data->levels[i]) /
+				   num_steps)
+					num_levels += num_steps;
+				else
+					num_levels++;
+			}
+			num_levels++;
+			dev_dbg(dev, "new number of brightness levels: %d\n",
+				num_levels);
+
+			/*
+			 * Create a new table of brightness levels with all the
+			 * interpolated steps.
+			 */
+			size = sizeof(*table) * num_levels;
+			table = devm_kzalloc(dev, size, GFP_KERNEL);
+			if (!table)
+				return -ENOMEM;
+
+			/* Fill the interpolated table. */
+			levels_count = 0;
+			for (i = 0; i < data->max_brightness - 1; i++) {
+				value = data->levels[i];
+				n = (data->levels[i + 1] - value) / num_steps;
+				if (n > 0) {
+					for (j = 0; j < num_steps; j++) {
+						table[levels_count] = value;
+						value += n;
+						levels_count++;
+					}
+				} else {
+					table[levels_count] = data->levels[i];
+					levels_count++;
+				}
+			}
+			table[levels_count] = data->levels[i];
+
+			/*
+			 * As we use interpolation lets remove current
+			 * brightness levels table and replace for the
+			 * new interpolated table.
+			 */
+			devm_kfree(dev, data->levels);
+			data->levels = table;
+
+			/*
+			 * Reassign max_brightness value to the new total number
+			 * of brightness levels.
+			 */
+			data->max_brightness = num_levels;
+		}
+
 		data->max_brightness--;
 	}
 
-- 
2.15.1

[PATCH v3 2/4] dt-bindings: pwm-backlight: add a num-interpolation-steps property.

[Enric Balletbo i Serra]

The num-interpolated-steps property specifies the number of
interpolated steps between each value of brightness-level table. This is
useful for high resolution PWMs to not have to list out every possible
value in the brightness-level array.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
---
Changes since v2:
- Add Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Changes since v1:
- Add an example with a small but realistic curve. Suggested by Rob and
  Daniel

 .../bindings/leds/backlight/pwm-backlight.txt         | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index 310810906613..ce9b5746b774 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -21,6 +21,11 @@ Optional properties:
                           and enabling the backlight using GPIO.
   - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
                       and setting PWM value to 0.
+  - num-interpolated-steps: Number of interpolated steps between each value
+                            of brightness-levels table. This way a high
+                            resolution pwm duty cycle can be used without
+                            having to list out every possible value in the
+                            brightness-level array.
 
 [0]: Documentation/devicetree/bindings/pwm/pwm.txt
 [1]: Documentation/devicetree/bindings/gpio/gpio.txt
@@ -39,3 +44,17 @@ Example:
 		post-pwm-on-delay-ms = <10>;
 		pwm-off-delay-ms = <10>;
 	};
+
+Example using num-interpolation-steps:
+
+	backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 0 5000000>;
+
+		brightness-levels = <0 2048 4096 8192 16384 65535>;
+		num-interpolated-steps = <2048>;
+		default-brightness-level = <4096>;
+
+		power-supply = <&vdd_bl_reg>;
+		enable-gpios = <&gpio 58 0>;
+	};
-- 
2.15.1

[PATCH v3 3/4] 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 computes a default table with the brightness levels filled
with the numbers provided by the CIE 1931 algorithm, the number of the
brightness levels is calculated based on the PWM resolution.

The calculation of the table 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>
---
Changes since v2:
- None
Changes since v1:
- Fix an horrible and unjustifiable mistake a missing { fortunately
  catched very quick.

 drivers/video/backlight/pwm_bl.c | 149 +++++++++++++++++++++++++++++++++++----
 1 file changed, 136 insertions(+), 13 deletions(-)

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index f0a108ab570a..d047d875f251 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -143,6 +143,107 @@ static const struct backlight_ops pwm_backlight_ops = {
 };
 
 #ifdef CONFIG_OF
+#define PWM_LUMINANCE_SCALE	10000 /* luminance scale */
+
+/* An integer based power function */
+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, the amount of light coming out of the screen, and
+ * is a number between 0.0 and 1.0; and L* is the lightness, how bright a human
+ * perceives the screen to be, and is a number between 0 and 100.
+ *
+ * The following function does the fixed point maths needed to implement the
+ * above formula.
+ */
+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 default correction table for PWM values to create linear brightness
+ * for LED based backlights using the CIE1931 algorithm.
+ */
+static
+int pwm_backlight_brightness_default(struct device *dev,
+				     struct platform_pwm_backlight_data *data,
+				     unsigned int period)
+{
+	unsigned int counter = 0;
+	unsigned int i, n;
+	u64 retval;
+
+	/*
+	 * Count the number of bits needed to represent the period number. The
+	 * number of bits is used to calculate the number of levels used for the
+	 * brightness-levels table, the purpose of this calculation is have a
+	 * pre-computed table with enough levels to get linear brightness
+	 * perception. The period is divided by the number of bits so for a
+	 * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM
+	 * we have 65535 / 16 = 4096 brightness levels.
+	 *
+	 * Note that this method is based on empirical testing on different
+	 * devices with PWM of 8 and 16 bits of resolution.
+	 */
+	n = period;
+	while (n) {
+		counter += n % 2;
+		n >>= 1;
+	}
+
+	data->max_brightness = DIV_ROUND_UP(period, counter);
+	data->levels = devm_kcalloc(dev, data->max_brightness,
+				    sizeof(*data->levels), GFP_KERNEL);
+	if (!data->levels)
+		return -ENOMEM;
+
+	/* Fill the table using the cie1931 algorithm */
+	for (i = 0; i < data->max_brightness; i++) {
+		retval = cie1931((i * PWM_LUMINANCE_SCALE) /
+				 data->max_brightness, PWM_LUMINANCE_SCALE) *
+				 period;
+		retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE);
+		if (retval > UINT_MAX)
+			return -EINVAL;
+		data->levels[i] = (unsigned int)retval;
+	}
+
+	data->dft_brightness = data->max_brightness / 2;
+	data->max_brightness--;
+
+	return 0;
+}
+
 static int pwm_backlight_parse_dt(struct device *dev,
 				  struct platform_pwm_backlight_data *data)
 {
@@ -161,10 +262,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
 
 	memset(data, 0, sizeof(*data));
 
-	/* determine the number of brightness levels */
+	/*
+	 * Determine the number of brightness levels, if this property is not
+	 * set a default table of brightness levels will be used.
+	 */
 	prop = of_find_property(node, "brightness-levels", &length);
 	if (!prop)
-		return -EINVAL;
+		return 0;
 
 	data->max_brightness = length / sizeof(u32);
 
@@ -294,6 +398,14 @@ static int pwm_backlight_parse_dt(struct device *dev,
 {
 	return -ENODEV;
 }
+
+static
+int pwm_backlight_brightness_default(struct device *dev,
+				     struct platform_pwm_backlight_data *data,
+				     unsigned int period)
+{
+	return -ENODEV;
+}
 #endif
 
 static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
@@ -334,7 +446,9 @@ 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;
+	unsigned int i;
 	int ret;
 
 	if (!data) {
@@ -359,17 +473,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
-	if (data->levels) {
-		unsigned int i;
-
-		for (i = 0; i <= data->max_brightness; i++)
-			if (data->levels[i] > pb->scale)
-				pb->scale = data->levels[i];
-
-		pb->levels = data->levels;
-	} else
-		pb->scale = data->max_brightness;
-
 	pb->notify = data->notify;
 	pb->notify_after = data->notify_after;
 	pb->check_fb = data->check_fb;
@@ -436,6 +539,26 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "got pwm for backlight\n");
 
+	if (!data->levels) {
+		/* Get the PWM period (in nanoseconds) */
+		pwm_get_state(pb->pwm, &state);
+
+		ret = pwm_backlight_brightness_default(&pdev->dev, data,
+						       state.period);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"failed to setup default brightness table\n");
+			goto err_alloc;
+		}
+	}
+
+	for (i = 0; i <= data->max_brightness; i++) {
+		if (data->levels[i] > pb->scale)
+			pb->scale = data->levels[i];
+
+		pb->levels = data->levels;
+	}
+
 	/*
 	 * FIXME: pwm_apply_args() should be removed when switching to
 	 * the atomic PWM API.
-- 
2.15.1

[PATCH v3 4/4] dt-bindings: pwm-backlight: move brightness-levels to optional.

[Enric Balletbo i Serra]

The patch 'backlight: pwm_bl: compute brightness of LED linearly to
human eye' introduced a default brightness-levels table that is used
when brightness-levels is not available in the dts. So move
brightness-levels and default-brightness-level to be optional.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
---
Changes since v2:
- Add Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Changes since v1:
- Fix a couple of typos as requested by Rob.
  - s/availablel/available/
  - s/move move/move/
  - "so move..." should be a new sentence.
- Add Reviewed-by: Rob Herring <robh@kernel.org>

 .../devicetree/bindings/leds/backlight/pwm-backlight.txt  | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
index ce9b5746b774..64fa2fbd98c9 100644
--- a/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
+++ b/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt
@@ -3,13 +3,6 @@ pwm-backlight bindings
 Required properties:
   - compatible: "pwm-backlight"
   - pwms: OF device-tree PWM specification (see PWM binding[0])
-  - brightness-levels: Array of distinct brightness levels. Typically these
-      are in the range from 0 to 255, but any range starting at 0 will do.
-      The actual brightness level (PWM duty cycle) will be interpolated
-      from these values. 0 means a 0% duty cycle (darkest/off), while the
-      last value in the array represents a 100% duty cycle (brightest).
-  - default-brightness-level: the default brightness level (index into the
-      array defined by the "brightness-levels" property)
   - power-supply: regulator for supply voltage
 
 Optional properties:
@@ -21,6 +14,14 @@ Optional properties:
                           and enabling the backlight using GPIO.
   - pwm-off-delay-ms: Delay in ms between disabling the backlight using GPIO
                       and setting PWM value to 0.
+  - brightness-levels: Array of distinct brightness levels. Typically these
+                       are in the range from 0 to 255, but any range starting at
+                       0 will do. The actual brightness level (PWM duty cycle)
+                       will be interpolated from these values. 0 means a 0% duty
+                       cycle (darkest/off), while the last value in the array
+                       represents a 100% duty cycle (brightest).
+  - default-brightness-level: The default brightness level (index into the
+                              array defined by the "brightness-levels" property).
   - num-interpolated-steps: Number of interpolated steps between each value
                             of brightness-levels table. This way a high
                             resolution pwm duty cycle can be used without
-- 
2.15.1

Re: [PATCH v3 2/4] dt-bindings: pwm-backlight: add a num-interpolation-steps property.

[Rob Herring]

On Thu, Feb 08, 2018 at 12:30:30PM +0100, Enric Balletbo i Serra wrote:
> The num-interpolated-steps property specifies the number of
> interpolated steps between each value of brightness-level table. This is
> useful for high resolution PWMs to not have to list out every possible
> value in the brightness-level array.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
> Changes since v2:
> - Add Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Changes since v1:
> - Add an example with a small but realistic curve. Suggested by Rob and
>   Daniel
> 
>  .../bindings/leds/backlight/pwm-backlight.txt         | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[Enric Balletbo Serra]

Hi Daniel,

Gentle ping for this series, there is any possibility you have a
chance to review it? Let me know if you want I change something.

Thanks,
  Enric

2018-02-08 12:30 GMT+01:00 Enric Balletbo i Serra
<enric.balletbo@collabora.com>:
> Dear all,
>
> This series is a third patchset integrating the requested changes.
>
> The first and second 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 thirth and fourth 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 create a brightness-levels 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.
>
> Both functionalities were tested on a Samsung Chromebook Plus (that has
> a 16 bits PWM) and a SL50 device (with a 8 bits PWM)
>
> Waiting for your feedback.
>
> Best regards,
>
> Enric Balletbo i Serra (4):
>   backlight: pwm_bl: linear interpolation between brightness-levels
>   dt-bindings: pwm-backlight: add a num-interpolation-steps property.
>   backlight: pwm_bl: compute brightness of LED linearly to human eye.
>   dt-bindings: pwm-backlight: move brightness-levels to optional.
>
>  .../bindings/leds/backlight/pwm-backlight.txt      |  34 ++-
>  drivers/video/backlight/pwm_bl.c                   | 232 +++++++++++++++++++--
>  2 files changed, 246 insertions(+), 20 deletions(-)
>
> --
> 2.15.1
>

Re: [PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[Daniel Thompson]

On Mon, Mar 19, 2018 at 05:04:31PM +0100, Enric Balletbo Serra wrote:
> Hi Daniel,
> 
> Gentle ping for this series, there is any possibility you have a
> chance to review it? Let me know if you want I change something.

I haven't got it in my TODO backlog... which means either I mistakenly
deleted it when it went through originally or that I deliberately deleted
it because I thought a v4 was coming along soon.

I could go diving through the archives if I need to but were there other
pending changes for this patchset?


Daniel.


> 
> Thanks,
>   Enric
> 
> 2018-02-08 12:30 GMT+01:00 Enric Balletbo i Serra
> <enric.balletbo@collabora.com>:
> > Dear all,
> >
> > This series is a third patchset integrating the requested changes.
> >
> > The first and second 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 thirth and fourth 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 create a brightness-levels 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.
> >
> > Both functionalities were tested on a Samsung Chromebook Plus (that has
> > a 16 bits PWM) and a SL50 device (with a 8 bits PWM)
> >
> > Waiting for your feedback.
> >
> > Best regards,
> >
> > Enric Balletbo i Serra (4):
> >   backlight: pwm_bl: linear interpolation between brightness-levels
> >   dt-bindings: pwm-backlight: add a num-interpolation-steps property.
> >   backlight: pwm_bl: compute brightness of LED linearly to human eye.
> >   dt-bindings: pwm-backlight: move brightness-levels to optional.
> >
> >  .../bindings/leds/backlight/pwm-backlight.txt      |  34 ++-
> >  drivers/video/backlight/pwm_bl.c                   | 232 +++++++++++++++++++--
> >  2 files changed, 246 insertions(+), 20 deletions(-)
> >
> > --
> > 2.15.1
> >

Re: [PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[Enric Balletbo Serra]

Hi Daniel,


2018-03-20 12:22 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>:
> On Mon, Mar 19, 2018 at 05:04:31PM +0100, Enric Balletbo Serra wrote:
>> Hi Daniel,
>>
>> Gentle ping for this series, there is any possibility you have a
>> chance to review it? Let me know if you want I change something.
>
> I haven't got it in my TODO backlog... which means either I mistakenly
> deleted it when it went through originally or that I deliberately deleted
> it because I thought a v4 was coming along soon.
>
> I could go diving through the archives if I need to but were there other
> pending changes for this patchset?
>

Unless I am missing something there isn't pending changes requested.

1/4 I addressed your latest comments in this version.
2/4 Has been acked by Rob Herring
3/4 I did not receive feedback but iirc we agreed to use a pre-computed table.
4/4 Is already acked by you.

Regards,
 Enric

>
> Daniel.
>
>
>>
>> Thanks,
>>   Enric
>>
>> 2018-02-08 12:30 GMT+01:00 Enric Balletbo i Serra
>> <enric.balletbo@collabora.com>:
>> > Dear all,
>> >
>> > This series is a third patchset integrating the requested changes.
>> >
>> > The first and second 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 thirth and fourth 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 create a brightness-levels 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.
>> >
>> > Both functionalities were tested on a Samsung Chromebook Plus (that has
>> > a 16 bits PWM) and a SL50 device (with a 8 bits PWM)
>> >
>> > Waiting for your feedback.
>> >
>> > Best regards,
>> >
>> > Enric Balletbo i Serra (4):
>> >   backlight: pwm_bl: linear interpolation between brightness-levels
>> >   dt-bindings: pwm-backlight: add a num-interpolation-steps property.
>> >   backlight: pwm_bl: compute brightness of LED linearly to human eye.
>> >   dt-bindings: pwm-backlight: move brightness-levels to optional.
>> >
>> >  .../bindings/leds/backlight/pwm-backlight.txt      |  34 ++-
>> >  drivers/video/backlight/pwm_bl.c                   | 232 +++++++++++++++++++--
>> >  2 files changed, 246 insertions(+), 20 deletions(-)
>> >
>> > --
>> > 2.15.1
>> >

Re: [PATCH v3 1/4] backlight: pwm_bl: linear interpolation between brightness-levels

[Daniel Thompson]

On Thu, Feb 08, 2018 at 12:30:29PM +0100, Enric Balletbo i Serra wrote:
> Setting num-interpolated-steps in the dts will allow you to have linear
> interpolation between values of brightness-levels. 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.
> 
> The most simple example is interpolate between two brightness values a
> number of steps, this can be done setting the following in the dts:
> 
>   brightness-levels = <0 65535>;
>   num-interpolated-steps = <1024>;
>   default-brightness-level = <512>;
> 
> This will create a brightness-level table with the following values:
> 
>   <0 63 126 189 252 315 378 441 ... 64260 64323 64386 64449 65535>
> 
> Another use case can be describe a gamma corrected curve, as we have
> better sensitivity at low luminance than high luminance we probably
> want have smaller steps for low brightness levels values and bigger
> steps for high brightness levels values. This can be achieved with
> the following in the dts:
> 
>   brightness-levels = <0 4096 65535>;
>   num-interpolated-steps = <1024>;
>   default-brightness-level = <512>;
> 
> This will create a brightness-levels table with the following values:
> 
>   <0 4 8 12 16 20 ... 4096 4156 4216 4276 ... 65535>
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

> ---
> Changes since v2:
> Requested by Daniel Thompson:
>  - Use a devres alloc for table creatiion and then just swap pointers.
>  - No need to use calloc because the loop initializes every element.
> Changes since v1:
> - None.
> 
>  drivers/video/backlight/pwm_bl.c | 83 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 8e3f1245f5c5..f0a108ab570a 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -147,7 +147,11 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  				  struct platform_pwm_backlight_data *data)
>  {
>  	struct device_node *node = dev->of_node;
> +	unsigned int num_levels = 0;
> +	unsigned int levels_count;
> +	unsigned int num_steps;
>  	struct property *prop;
> +	unsigned int *table;
>  	int length;
>  	u32 value;
>  	int ret;
> @@ -167,6 +171,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  	/* read brightness levels from DT property */
>  	if (data->max_brightness > 0) {
>  		size_t size = sizeof(*data->levels) * data->max_brightness;
> +		unsigned int i, j, n = 0;
>  
>  		data->levels = devm_kzalloc(dev, size, GFP_KERNEL);
>  		if (!data->levels)
> @@ -184,6 +189,84 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  			return ret;
>  
>  		data->dft_brightness = value;
> +
> +		/*
> +		 * This property is optional, if is set enables linear
> +		 * interpolation between each of the values of brightness levels
> +		 * and creates a new pre-computed table.
> +		 */
> +		of_property_read_u32(node, "num-interpolated-steps",
> +				     &num_steps);
> +
> +		/*
> +		 * Make sure that there is at least two entries in the
> +		 * brightness-levels table, otherwise we can't interpolate
> +		 * between two points.
> +		 */
> +		if (num_steps) {
> +			if (data->max_brightness < 2) {
> +				dev_err(dev, "can't interpolate\n");
> +				return -EINVAL;
> +			}
> +
> +			/*
> +			 * Recalculate the number of brightness levels, now
> +			 * taking in consideration the number of interpolated
> +			 * steps between two levels.
> +			 */
> +			for (i = 0; i < data->max_brightness - 1; i++) {
> +				if ((data->levels[i + 1] - data->levels[i]) /
> +				   num_steps)
> +					num_levels += num_steps;
> +				else
> +					num_levels++;
> +			}
> +			num_levels++;
> +			dev_dbg(dev, "new number of brightness levels: %d\n",
> +				num_levels);
> +
> +			/*
> +			 * Create a new table of brightness levels with all the
> +			 * interpolated steps.
> +			 */
> +			size = sizeof(*table) * num_levels;
> +			table = devm_kzalloc(dev, size, GFP_KERNEL);
> +			if (!table)
> +				return -ENOMEM;
> +
> +			/* Fill the interpolated table. */
> +			levels_count = 0;
> +			for (i = 0; i < data->max_brightness - 1; i++) {
> +				value = data->levels[i];
> +				n = (data->levels[i + 1] - value) / num_steps;
> +				if (n > 0) {
> +					for (j = 0; j < num_steps; j++) {
> +						table[levels_count] = value;
> +						value += n;
> +						levels_count++;
> +					}
> +				} else {
> +					table[levels_count] = data->levels[i];
> +					levels_count++;
> +				}
> +			}
> +			table[levels_count] = data->levels[i];
> +
> +			/*
> +			 * As we use interpolation lets remove current
> +			 * brightness levels table and replace for the
> +			 * new interpolated table.
> +			 */
> +			devm_kfree(dev, data->levels);
> +			data->levels = table;
> +
> +			/*
> +			 * Reassign max_brightness value to the new total number
> +			 * of brightness levels.
> +			 */
> +			data->max_brightness = num_levels;
> +		}
> +
>  		data->max_brightness--;
>  	}
>  
> -- 
> 2.15.1
> 

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

[Daniel Thompson]

On Thu, Feb 08, 2018 at 12:30:31PM +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 computes a default table with the brightness levels filled
> with the numbers provided by the CIE 1931 algorithm, the number of the
> brightness levels is calculated based on the PWM resolution.
> 
> The calculation of the table 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>

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>

> ---
> Changes since v2:
> - None
> Changes since v1:
> - Fix an horrible and unjustifiable mistake a missing { fortunately
>   catched very quick.
> 
>  drivers/video/backlight/pwm_bl.c | 149 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 136 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index f0a108ab570a..d047d875f251 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -143,6 +143,107 @@ static const struct backlight_ops pwm_backlight_ops = {
>  };
>  
>  #ifdef CONFIG_OF
> +#define PWM_LUMINANCE_SCALE	10000 /* luminance scale */
> +
> +/* An integer based power function */
> +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, the amount of light coming out of the screen, and
> + * is a number between 0.0 and 1.0; and L* is the lightness, how bright a human
> + * perceives the screen to be, and is a number between 0 and 100.
> + *
> + * The following function does the fixed point maths needed to implement the
> + * above formula.
> + */
> +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 default correction table for PWM values to create linear brightness
> + * for LED based backlights using the CIE1931 algorithm.
> + */
> +static
> +int pwm_backlight_brightness_default(struct device *dev,
> +				     struct platform_pwm_backlight_data *data,
> +				     unsigned int period)
> +{
> +	unsigned int counter = 0;
> +	unsigned int i, n;
> +	u64 retval;
> +
> +	/*
> +	 * Count the number of bits needed to represent the period number. The
> +	 * number of bits is used to calculate the number of levels used for the
> +	 * brightness-levels table, the purpose of this calculation is have a
> +	 * pre-computed table with enough levels to get linear brightness
> +	 * perception. The period is divided by the number of bits so for a
> +	 * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM
> +	 * we have 65535 / 16 = 4096 brightness levels.
> +	 *
> +	 * Note that this method is based on empirical testing on different
> +	 * devices with PWM of 8 and 16 bits of resolution.
> +	 */
> +	n = period;
> +	while (n) {
> +		counter += n % 2;
> +		n >>= 1;
> +	}
> +
> +	data->max_brightness = DIV_ROUND_UP(period, counter);
> +	data->levels = devm_kcalloc(dev, data->max_brightness,
> +				    sizeof(*data->levels), GFP_KERNEL);
> +	if (!data->levels)
> +		return -ENOMEM;
> +
> +	/* Fill the table using the cie1931 algorithm */
> +	for (i = 0; i < data->max_brightness; i++) {
> +		retval = cie1931((i * PWM_LUMINANCE_SCALE) /
> +				 data->max_brightness, PWM_LUMINANCE_SCALE) *
> +				 period;
> +		retval = DIV_ROUND_CLOSEST_ULL(retval, PWM_LUMINANCE_SCALE);
> +		if (retval > UINT_MAX)
> +			return -EINVAL;
> +		data->levels[i] = (unsigned int)retval;
> +	}
> +
> +	data->dft_brightness = data->max_brightness / 2;
> +	data->max_brightness--;
> +
> +	return 0;
> +}
> +
>  static int pwm_backlight_parse_dt(struct device *dev,
>  				  struct platform_pwm_backlight_data *data)
>  {
> @@ -161,10 +262,13 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  
>  	memset(data, 0, sizeof(*data));
>  
> -	/* determine the number of brightness levels */
> +	/*
> +	 * Determine the number of brightness levels, if this property is not
> +	 * set a default table of brightness levels will be used.
> +	 */
>  	prop = of_find_property(node, "brightness-levels", &length);
>  	if (!prop)
> -		return -EINVAL;
> +		return 0;
>  
>  	data->max_brightness = length / sizeof(u32);
>  
> @@ -294,6 +398,14 @@ static int pwm_backlight_parse_dt(struct device *dev,
>  {
>  	return -ENODEV;
>  }
> +
> +static
> +int pwm_backlight_brightness_default(struct device *dev,
> +				     struct platform_pwm_backlight_data *data,
> +				     unsigned int period)
> +{
> +	return -ENODEV;
> +}
>  #endif
>  
>  static int pwm_backlight_initial_power_state(const struct pwm_bl_data *pb)
> @@ -334,7 +446,9 @@ 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;
> +	unsigned int i;
>  	int ret;
>  
>  	if (!data) {
> @@ -359,17 +473,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  		goto err_alloc;
>  	}
>  
> -	if (data->levels) {
> -		unsigned int i;
> -
> -		for (i = 0; i <= data->max_brightness; i++)
> -			if (data->levels[i] > pb->scale)
> -				pb->scale = data->levels[i];
> -
> -		pb->levels = data->levels;
> -	} else
> -		pb->scale = data->max_brightness;
> -
>  	pb->notify = data->notify;
>  	pb->notify_after = data->notify_after;
>  	pb->check_fb = data->check_fb;
> @@ -436,6 +539,26 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  
>  	dev_dbg(&pdev->dev, "got pwm for backlight\n");
>  
> +	if (!data->levels) {
> +		/* Get the PWM period (in nanoseconds) */
> +		pwm_get_state(pb->pwm, &state);
> +
> +		ret = pwm_backlight_brightness_default(&pdev->dev, data,
> +						       state.period);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to setup default brightness table\n");
> +			goto err_alloc;
> +		}
> +	}
> +
> +	for (i = 0; i <= data->max_brightness; i++) {
> +		if (data->levels[i] > pb->scale)
> +			pb->scale = data->levels[i];
> +
> +		pb->levels = data->levels;
> +	}
> +
>  	/*
>  	 * FIXME: pwm_apply_args() should be removed when switching to
>  	 * the atomic PWM API.
> -- 
> 2.15.1
> 

Re: [PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[Daniel Thompson]

On Tue, Mar 20, 2018 at 01:13:20PM +0100, Enric Balletbo Serra wrote:
> Hi Daniel,
> 
> 
> 2018-03-20 12:22 GMT+01:00 Daniel Thompson <daniel.thompson@linaro.org>:
> > On Mon, Mar 19, 2018 at 05:04:31PM +0100, Enric Balletbo Serra wrote:
> >> Hi Daniel,
> >>
> >> Gentle ping for this series, there is any possibility you have a
> >> chance to review it? Let me know if you want I change something.
> >
> > I haven't got it in my TODO backlog... which means either I mistakenly
> > deleted it when it went through originally or that I deliberately deleted
> > it because I thought a v4 was coming along soon.
> >
> > I could go diving through the archives if I need to but were there other
> > pending changes for this patchset?
> >
> 
> Unless I am missing something there isn't pending changes requested.
> 
> 1/4 I addressed your latest comments in this version.
> 2/4 Has been acked by Rob Herring
> 3/4 I did not receive feedback but iirc we agreed to use a pre-computed table.
> 4/4 Is already acked by you.

You weren't missing anything... and I've just dived through my mail
history and retrieved the patchset so I can review it.

You should now have an ack from me on every patch. Sorry it has taken 
so long.


Daniel.


> 
> Regards,
>  Enric
> 
> >
> > Daniel.
> >
> >
> >>
> >> Thanks,
> >>   Enric
> >>
> >> 2018-02-08 12:30 GMT+01:00 Enric Balletbo i Serra
> >> <enric.balletbo@collabora.com>:
> >> > Dear all,
> >> >
> >> > This series is a third patchset integrating the requested changes.
> >> >
> >> > The first and second 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 thirth and fourth 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 create a brightness-levels 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.
> >> >
> >> > Both functionalities were tested on a Samsung Chromebook Plus (that has
> >> > a 16 bits PWM) and a SL50 device (with a 8 bits PWM)
> >> >
> >> > Waiting for your feedback.
> >> >
> >> > Best regards,
> >> >
> >> > Enric Balletbo i Serra (4):
> >> >   backlight: pwm_bl: linear interpolation between brightness-levels
> >> >   dt-bindings: pwm-backlight: add a num-interpolation-steps property.
> >> >   backlight: pwm_bl: compute brightness of LED linearly to human eye.
> >> >   dt-bindings: pwm-backlight: move brightness-levels to optional.
> >> >
> >> >  .../bindings/leds/backlight/pwm-backlight.txt      |  34 ++-
> >> >  drivers/video/backlight/pwm_bl.c                   | 232 +++++++++++++++++++--
> >> >  2 files changed, 246 insertions(+), 20 deletions(-)
> >> >
> >> > --
> >> > 2.15.1
> >> >

Re: [PATCH v3 0/4] backlight: pwm_bl: support linear interpolation and brightness to human eye

[Lee Jones]

On Thu, 08 Feb 2018, Enric Balletbo i Serra wrote:

> Dear all,
> 
> This series is a third patchset integrating the requested changes.
> 
> The first and second 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 thirth and fourth 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 create a brightness-levels 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.
> 
> Both functionalities were tested on a Samsung Chromebook Plus (that has
> a 16 bits PWM) and a SL50 device (with a 8 bits PWM)
> 
> Waiting for your feedback.

Looks like you now have some positive feedback. :)

Could you please collect all of your received Acks and re-post this
set as a [RESEND] please?

> Best regards,
> 
> Enric Balletbo i Serra (4):
>   backlight: pwm_bl: linear interpolation between brightness-levels
>   dt-bindings: pwm-backlight: add a num-interpolation-steps property.
>   backlight: pwm_bl: compute brightness of LED linearly to human eye.
>   dt-bindings: pwm-backlight: move brightness-levels to optional.
> 
>  .../bindings/leds/backlight/pwm-backlight.txt      |  34 ++-
>  drivers/video/backlight/pwm_bl.c                   | 232 +++++++++++++++++++--
>  2 files changed, 246 insertions(+), 20 deletions(-)


-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Matthias Kaehlcke]

Hi Enric,

some comments inline, a bit late, but I just tested this on veyron
minnie.

On Thu, Feb 08, 2018 at 12:30:31PM +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 computes a default table with the brightness levels filled
> with the numbers provided by the CIE 1931 algorithm, the number of the
> brightness levels is calculated based on the PWM resolution.
> 
> The calculation of the table 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>
>
> ...
>
> +static
> +int pwm_backlight_brightness_default(struct device *dev,
> +				     struct platform_pwm_backlight_data *data,
> +				     unsigned int period)
> +{
> +	unsigned int counter = 0;
> +	unsigned int i, n;
> +	u64 retval;
> +
> +	/*
> +	 * Count the number of bits needed to represent the period number. The
> +	 * number of bits is used to calculate the number of levels used for the
> +	 * brightness-levels table, the purpose of this calculation is have a
> +	 * pre-computed table with enough levels to get linear brightness
> +	 * perception. The period is divided by the number of bits so for a
> +	 * 8-bit PWM we have 255 / 8 = 32 brightness levels or for a 16-bit PWM
> +	 * we have 65535 / 16 = 4096 brightness levels.
> +	 *
> +	 * Note that this method is based on empirical testing on different
> +	 * devices with PWM of 8 and 16 bits of resolution.
> +	 */
> +	n = period;
> +	while (n) {
> +		counter += n % 2;
> +		n >>= 1;
> +	}

I don't quite follow the heuristics above. Are you sure the number of
PWM bits can be infered from the period? What if the period value (in
ns) doesn't directly correspond to a register value? And even if it
did, counting the number of set bits (the above loops is a
re-implementation of ffs()) doesn't really result in the dividers
mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
in a divider of 1, i.e. 32768 brighness levels.

On veyron minnie the period is 1000000 ns, which results in 142858
levels (1000000 / 7)!

Not sure if there is a clean solution using heuristics, a DT property
specifying the number of levels could be an alternative. This could
also be useful to limit the number of (mostly) redundant levels, even
the intended max of 4096 seems pretty high.

Another (not directly related) observation is that on minnie the
actual brightness at a nominal 50% is close to 0 (duty cycle ~3%). I
haven't tested with other devices, but I wonder if it would make
sense to have an option to drop the bottom N% of levels, since the
near 0 brightness in the lower 50% probably isn't very useful in most
use cases, but maybe it looks different on other devices.

Cheers

Matthias

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

[Pavel Machek]

Hi!

> > +	 * Note that this method is based on empirical testing on different
> > +	 * devices with PWM of 8 and 16 bits of resolution.
> > +	 */
> > +	n = period;
> > +	while (n) {
> > +		counter += n % 2;
> > +		n >>= 1;
> > +	}
> 
> I don't quite follow the heuristics above. Are you sure the number of
> PWM bits can be infered from the period? What if the period value (in
> ns) doesn't directly correspond to a register value? And even if it
> did, counting the number of set bits (the above loops is a
> re-implementation of ffs()) doesn't really result in the dividers
> mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
> in a divider of 1, i.e. 32768 brighness levels.
> 
> On veyron minnie the period is 1000000 ns, which results in 142858
> levels (1000000 / 7)!
> 
> Not sure if there is a clean solution using heuristics, a DT property
> specifying the number of levels could be an alternative. This could
> also be useful to limit the number of (mostly) redundant levels, even
> the intended max of 4096 seems pretty high.
> 
> Another (not directly related) observation is that on minnie the
> actual brightness at a nominal 50% is close to 0 (duty cycle ~3%). I
> haven't tested with other devices, but I wonder if it would make
> sense to have an option to drop the bottom N% of levels, since the
> near 0 brightness in the lower 50% probably isn't very useful in most
> use cases, but maybe it looks different on other devices.

Eye percieves logarithm(duty cycle), mostly, and I find very low brightness
levels quite useful when trying to use machine in dark room.

But yes, specifying if brightness is linear or exponential would be quite
useful.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Enric Balletbo i Serra]

Hi Matthias,

On 8/6/19 23:02, Pavel Machek wrote:
> Hi!
> 
>>> +	 * Note that this method is based on empirical testing on different
>>> +	 * devices with PWM of 8 and 16 bits of resolution.
>>> +	 */
>>> +	n = period;
>>> +	while (n) {
>>> +		counter += n % 2;
>>> +		n >>= 1;
>>> +	}
>>
>> I don't quite follow the heuristics above. Are you sure the number of
>> PWM bits can be infered from the period? What if the period value (in
>> ns) doesn't directly correspond to a register value? And even if it
>> did, counting the number of set bits (the above loops is a
>> re-implementation of ffs()) doesn't really result in the dividers
>> mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
>> in a divider of 1, i.e. 32768 brighness levels.
>>

Right, I think that only works on the cases that we only have one pwm cell, and
looks like during my tests I did only tests on devices with one pwm cell :-(

And as you point the code is broken for other cases (pwm-cells > 1)

>> On veyron minnie the period is 1000000 ns, which results in 142858
>> levels (1000000 / 7)!
>>
>> Not sure if there is a clean solution using heuristics, a DT property
>> specifying the number of levels could be an alternative. This could
>> also be useful to limit the number of (mostly) redundant levels, even
>> the intended max of 4096 seems pretty high.
>>

Looking again looks like we _can not_ deduce the number of bits of a pwm, it is
not exposed at all, so I think we will need to end adding a property to specify
this. Something similar to what leds-pwm binding does, it has:

max-brightness : Maximum brightness possible for the LED


Enric

>> Another (not directly related) observation is that on minnie the
>> actual brightness at a nominal 50% is close to 0 (duty cycle ~3%). I
>> haven't tested with other devices, but I wonder if it would make
>> sense to have an option to drop the bottom N% of levels, since the
>> near 0 brightness in the lower 50% probably isn't very useful in most
>> use cases, but maybe it looks different on other devices.
> 
> Eye percieves logarithm(duty cycle), mostly, and I find very low brightness
> levels quite useful when trying to use machine in dark room.
> 
> But yes, specifying if brightness is linear or exponential would be quite
> useful.
> 									Pavel
> 

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

[Matthias Kaehlcke]

Hi Enric

On Mon, Jun 10, 2019 at 12:00:02PM +0200, Enric Balletbo i Serra wrote:
> Hi Matthias,
> 
> On 8/6/19 23:02, Pavel Machek wrote:
> > Hi!
> > 
> >>> +	 * Note that this method is based on empirical testing on different
> >>> +	 * devices with PWM of 8 and 16 bits of resolution.
> >>> +	 */
> >>> +	n = period;
> >>> +	while (n) {
> >>> +		counter += n % 2;
> >>> +		n >>= 1;
> >>> +	}
> >>
> >> I don't quite follow the heuristics above. Are you sure the number of
> >> PWM bits can be infered from the period? What if the period value (in
> >> ns) doesn't directly correspond to a register value? And even if it
> >> did, counting the number of set bits (the above loops is a
> >> re-implementation of ffs()) doesn't really result in the dividers
> >> mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
> >> in a divider of 1, i.e. 32768 brighness levels.
> >>
> 
> Right, I think that only works on the cases that we only have one pwm cell, and
> looks like during my tests I did only tests on devices with one pwm cell :-(
> 
> And as you point the code is broken for other cases (pwm-cells > 1)
> 
> >> On veyron minnie the period is 1000000 ns, which results in 142858
> >> levels (1000000 / 7)!
> >>
> >> Not sure if there is a clean solution using heuristics, a DT property
> >> specifying the number of levels could be an alternative. This could
> >> also be useful to limit the number of (mostly) redundant levels, even
> >> the intended max of 4096 seems pretty high.
> >>
> 
> Looking again looks like we _can not_ deduce the number of bits of a pwm, it is
> not exposed at all, so I think we will need to end adding a property to specify
> this. Something similar to what leds-pwm binding does, it has:
> 
> max-brightness : Maximum brightness possible for the LED

Thanks for the confirmation that I didn't just miss some clever trick.

I also think that some kind of DT property is needed, I'll try to come
up with a reasonable name, keeping in mind that some devices might not
want to use the entire range of levels.

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

[Matthias Kaehlcke]

Hi Pavel,

On Sat, Jun 08, 2019 at 11:02:26PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > +	 * Note that this method is based on empirical testing on different
> > > +	 * devices with PWM of 8 and 16 bits of resolution.
> > > +	 */
> > > +	n = period;
> > > +	while (n) {
> > > +		counter += n % 2;
> > > +		n >>= 1;
> > > +	}
> > 
> > I don't quite follow the heuristics above. Are you sure the number of
> > PWM bits can be infered from the period? What if the period value (in
> > ns) doesn't directly correspond to a register value? And even if it
> > did, counting the number of set bits (the above loops is a
> > re-implementation of ffs()) doesn't really result in the dividers
> > mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
> > in a divider of 1, i.e. 32768 brighness levels.
> > 
> > On veyron minnie the period is 1000000 ns, which results in 142858
> > levels (1000000 / 7)!
> > 
> > Not sure if there is a clean solution using heuristics, a DT property
> > specifying the number of levels could be an alternative. This could
> > also be useful to limit the number of (mostly) redundant levels, even
> > the intended max of 4096 seems pretty high.
> > 
> > Another (not directly related) observation is that on minnie the
> > actual brightness at a nominal 50% is close to 0 (duty cycle ~3%). I
> > haven't tested with other devices, but I wonder if it would make
> > sense to have an option to drop the bottom N% of levels, since the
> > near 0 brightness in the lower 50% probably isn't very useful in most
> > use cases, but maybe it looks different on other devices.
> 
> Eye percieves logarithm(duty cycle), mostly, and I find very low brightness
> levels quite useful when trying to use machine in dark room.

I realized that the brightness level display on Chrome OS (= my test
device) is non-linear, and it isn't actually the lower 50% of levels
that is near 0 brightness, but 'only' about 20%.

> But yes, specifying if brightness is linear or exponential would be quite
> useful.

Agreed, this could help userspace with displaying a reasonable
brightness level.

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

[Enric Balletbo i Serra]

Hi Matthias,

On 10/6/19 22:39, Matthias Kaehlcke wrote:
> Hi Enric
> 
> On Mon, Jun 10, 2019 at 12:00:02PM +0200, Enric Balletbo i Serra wrote:
>> Hi Matthias,
>>
>> On 8/6/19 23:02, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> +	 * Note that this method is based on empirical testing on different
>>>>> +	 * devices with PWM of 8 and 16 bits of resolution.
>>>>> +	 */
>>>>> +	n = period;
>>>>> +	while (n) {
>>>>> +		counter += n % 2;
>>>>> +		n >>= 1;
>>>>> +	}
>>>>
>>>> I don't quite follow the heuristics above. Are you sure the number of
>>>> PWM bits can be infered from the period? What if the period value (in
>>>> ns) doesn't directly correspond to a register value? And even if it
>>>> did, counting the number of set bits (the above loops is a
>>>> re-implementation of ffs()) doesn't really result in the dividers
>>>> mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
>>>> in a divider of 1, i.e. 32768 brighness levels.
>>>>
>>
>> Right, I think that only works on the cases that we only have one pwm cell, and
>> looks like during my tests I did only tests on devices with one pwm cell :-(
>>
>> And as you point the code is broken for other cases (pwm-cells > 1)
>>
>>>> On veyron minnie the period is 1000000 ns, which results in 142858
>>>> levels (1000000 / 7)!
>>>>
>>>> Not sure if there is a clean solution using heuristics, a DT property
>>>> specifying the number of levels could be an alternative. This could
>>>> also be useful to limit the number of (mostly) redundant levels, even
>>>> the intended max of 4096 seems pretty high.
>>>>
>>
>> Looking again looks like we _can not_ deduce the number of bits of a pwm, it is
>> not exposed at all, so I think we will need to end adding a property to specify
>> this. Something similar to what leds-pwm binding does, it has:
>>
>> max-brightness : Maximum brightness possible for the LED
> 
> Thanks for the confirmation that I didn't just miss some clever trick.
> 
> I also think that some kind of DT property is needed, I'll try to come
> up with a reasonable name, keeping in mind that some devices might not
> want to use the entire range of levels.
> 

Note that, If I remember correctly, the original idea behind all these patches
was provide a default curve with enough points following the  CIE 1931 formula
(which describes how we perceive light). When default doesn't work for your
hardware, you could play and define your own curve using the
num-interpolated-steps property I.e:

 brightness-levels = <0 2048 4096 8192 16384 65535>;
 num-interpolated-steps = <2048>;
 default-brightness-level = <4096>;

Or even expose all the possible levels, like you do with your chromeos kernel.

 brightness-levels = <0 65535>;
 num-interpolated-steps = <65535>;
 default-brightness-level = <4096>;

The above should work independently of the bug found (that of course needs to be
fixed)

Enric

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

[Matthias Kaehlcke]

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

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

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

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

[Daniel Thompson]

On Mon, Jun 10, 2019 at 01:52:33PM -0700, Matthias Kaehlcke wrote:
> Hi Pavel,
> 
> On Sat, Jun 08, 2019 at 11:02:26PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > > +	 * Note that this method is based on empirical testing on different
> > > > +	 * devices with PWM of 8 and 16 bits of resolution.
> > > > +	 */
> > > > +	n = period;
> > > > +	while (n) {
> > > > +		counter += n % 2;
> > > > +		n >>= 1;
> > > > +	}
> > > 
> > > I don't quite follow the heuristics above. Are you sure the number of
> > > PWM bits can be infered from the period? What if the period value (in
> > > ns) doesn't directly correspond to a register value? And even if it
> > > did, counting the number of set bits (the above loops is a
> > > re-implementation of ffs()) doesn't really result in the dividers
> > > mentioned in the comment. E.g. a period of 32768 ns (0x8000) results
> > > in a divider of 1, i.e. 32768 brighness levels.
> > > 
> > > On veyron minnie the period is 1000000 ns, which results in 142858
> > > levels (1000000 / 7)!
> > > 
> > > Not sure if there is a clean solution using heuristics, a DT property
> > > specifying the number of levels could be an alternative. This could
> > > also be useful to limit the number of (mostly) redundant levels, even
> > > the intended max of 4096 seems pretty high.
> > > 
> > > Another (not directly related) observation is that on minnie the
> > > actual brightness at a nominal 50% is close to 0 (duty cycle ~3%). I
> > > haven't tested with other devices, but I wonder if it would make
> > > sense to have an option to drop the bottom N% of levels, since the
> > > near 0 brightness in the lower 50% probably isn't very useful in most
> > > use cases, but maybe it looks different on other devices.
> > 
> > Eye percieves logarithm(duty cycle), mostly, and I find very low brightness
> > levels quite useful when trying to use machine in dark room.
> 
> I realized that the brightness level display on Chrome OS (= my test
> device) is non-linear, and it isn't actually the lower 50% of levels
> that is near 0 brightness, but 'only' about 20%.
> 
> > But yes, specifying if brightness is linear or exponential would be quite
> > useful.
> 
> Agreed, this could help userspace with displaying a reasonable
> brightness level.

This is a long standing flaw in the backlight interfaces. AFAIK generic
userspaces end up with a (flawed) heuristic.

Basically devices with a narrow range of choices can be assumed to be
logarithmic (since anything linear with a narrow range of choices is
junk anyway and the slider will never feel right).

On the other side though we can only really make a guess.

Systems are coming along that allow us to animate the change of
brightness (part of the reason for interpolated tables is to
permit smooth animation rather than because the user explicitly wants
to set the brightness to exactly 1117). These systems are often
logarithmic but with a wide range of values.


Daniel.

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

[Brian Norris]

On Tue, Jun 11, 2019 at 3:49 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> This is a long standing flaw in the backlight interfaces. AFAIK generic
> userspaces end up with a (flawed) heuristic.

Bingo! Would be nice if we could start to fix this long-standing flaw.

> Basically devices with a narrow range of choices can be assumed to be
> logarithmic

That's (almost, see below) exactly what we have.

(And this is what Matthias is fighting against, now that we're
implementing both "large number of data points" and "pre-curved" at
the same time. We will have to either adapt the heuristic, or else
adapt our device trees to fit the heuristic.)

> Systems are coming along that allow us to animate the change of
> brightness (part of the reason for interpolated tables is to
> permit smooth animation rather than because the user explicitly wants
> to set the brightness to exactly 1117).

Chrome OS has done this for a long time. So "coming along" is a bit late ;)

Also, I believe Chrome OS will do animation/smoothing for all tables
(small or large) where it can: even for the small tables.

> These systems are often
> logarithmic but with a wide range of values.

NB: Chrome OS happens to use a polynomial formula (exponent = 2 or
0.5, depending on how you look at it), not logarithmic. You can see it
in all its (non)glory here:

https://chromium.googlesource.com/chromiumos/platform2/+/ee015853b227cf265491bd80ccf096b188490529/power_manager/powerd/policy/internal_backlight_controller.cc#451

Regards,
Brian

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

[Matthias Kaehlcke]

On Tue, Jun 11, 2019 at 09:55:30AM -0700, Brian Norris wrote:
> On Tue, Jun 11, 2019 at 3:49 AM Daniel Thompson
> <daniel.thompson@linaro.org> wrote:
> > This is a long standing flaw in the backlight interfaces. AFAIK generic
> > userspaces end up with a (flawed) heuristic.
> 
> Bingo! Would be nice if we could start to fix this long-standing flaw.

Agreed!

How could a fix look like, a sysfs attribute? Would a boolean value
like 'logarithmic_scale' or 'linear_scale' be enough or could more
granularity be needed?

The new attribute could be optional (it only exists if explicitly
specified by the driver) or be set to a default based on a heuristic
if not specified and be 'fixed' on a case by case basis. The latter
might violate "don't break userspace" though, so I'm not sure it's a
good idea.

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

[Pavel Machek]

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

On Tue 2019-06-11 15:30:19, Matthias Kaehlcke wrote:
> On Tue, Jun 11, 2019 at 09:55:30AM -0700, Brian Norris wrote:
> > On Tue, Jun 11, 2019 at 3:49 AM Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > > This is a long standing flaw in the backlight interfaces. AFAIK generic
> > > userspaces end up with a (flawed) heuristic.
> > 
> > Bingo! Would be nice if we could start to fix this long-standing flaw.
> 
> Agreed!
> 
> How could a fix look like, a sysfs attribute? Would a boolean value
> like 'logarithmic_scale' or 'linear_scale' be enough or could more
> granularity be needed?

I'd expect attribute "scale" with values "linear" or "logarithmic".

> The new attribute could be optional (it only exists if explicitly
> specified by the driver) or be set to a default based on a heuristic
> if not specified and be 'fixed' on a case by case basis. The latter
> might violate "don't break userspace" though, so I'm not sure it's a
> good idea.

I'd do it only when we explicitely know. We don't want it to be buggy.

And I guess we should decide what interface we really want? (Linear?
Logarithmic?) And make new drivers do that.

Best regards,
									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: [PATCH v3 3/4] backlight: pwm_bl: compute brightness of LED linearly to human eye.

[Daniel Thompson]

On Tue, Jun 11, 2019 at 03:30:19PM -0700, Matthias Kaehlcke wrote:
> On Tue, Jun 11, 2019 at 09:55:30AM -0700, Brian Norris wrote:
> > On Tue, Jun 11, 2019 at 3:49 AM Daniel Thompson
> > <daniel.thompson@linaro.org> wrote:
> > > This is a long standing flaw in the backlight interfaces. AFAIK generic
> > > userspaces end up with a (flawed) heuristic.
> > 
> > Bingo! Would be nice if we could start to fix this long-standing flaw.
> 
> Agreed!
> 
> How could a fix look like, a sysfs attribute? Would a boolean value
> like 'logarithmic_scale' or 'linear_scale' be enough or could more
> granularity be needed?

Certainly "linear" (this device will work more or less correctly if the
userspace applies perceptual curves). Not sure about logarithmic since
what is actually useful is something that is "perceptually linear"
(logarithmic is merely a way to approximate that).

I do wonder about a compatible string like most-detailed to
least-detailed description. This for a PWM with the auto-generated
tables we'd see something like:

cie-1991,perceptual,non-linear

For something that is non-linear but we are not sure what its tables are
we can offer just "non-linear".

> 
> The new attribute could be optional (it only exists if explicitly
> specified by the driver) or be set to a default based on a heuristic
> if not specified and be 'fixed' on a case by case basis. The latter
> might violate "don't break userspace" though, so I'm not sure it's a
> good idea.

I think we should avoid any heuristic! There are several drivers and we
may not be able to work through all of them and make the correct
decision.

Instead one valid value for the sysfs should be "unknown" and this be
the default for drivers we have not analysed (this also makes it easy to
introduce change here).

We should only set the property to something else for drivers that have
been reviewed.

There could be a special case for pwm_bl.c in that I'm prepared to
assume that the hardware components downstream of the PWM have a
roughly linear response and that if the user provided tables that their
function is to provide a perceptually comfortable response.


Daniel.

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

[Matthias Kaehlcke]

Hi Daniel,

On Wed, Jun 12, 2019 at 12:03:25PM +0100, Daniel Thompson wrote:
> On Tue, Jun 11, 2019 at 03:30:19PM -0700, Matthias Kaehlcke wrote:
> > On Tue, Jun 11, 2019 at 09:55:30AM -0700, Brian Norris wrote:
> > > On Tue, Jun 11, 2019 at 3:49 AM Daniel Thompson
> > > <daniel.thompson@linaro.org> wrote:
> > > > This is a long standing flaw in the backlight interfaces. AFAIK generic
> > > > userspaces end up with a (flawed) heuristic.
> > > 
> > > Bingo! Would be nice if we could start to fix this long-standing flaw.
> > 
> > Agreed!
> > 
> > How could a fix look like, a sysfs attribute? Would a boolean value
> > like 'logarithmic_scale' or 'linear_scale' be enough or could more
> > granularity be needed?
> 
> Certainly "linear" (this device will work more or less correctly if the
> userspace applies perceptual curves). Not sure about logarithmic since
> what is actually useful is something that is "perceptually linear"
> (logarithmic is merely a way to approximate that).
> 
> I do wonder about a compatible string like most-detailed to
> least-detailed description. This for a PWM with the auto-generated
> tables we'd see something like:
> 
> cie-1991,perceptual,non-linear
> 
> For something that is non-linear but we are not sure what its tables are
> we can offer just "non-linear".

Thanks for the feedback!

It seems clear that we want a string for the added flexibility. I can
work on a patch with the compatible string like description you
suggested and we can discuss in the review if we want to go with that
or prefer something else.

> > The new attribute could be optional (it only exists if explicitly
> > specified by the driver) or be set to a default based on a heuristic
> > if not specified and be 'fixed' on a case by case basis. The latter
> > might violate "don't break userspace" though, so I'm not sure it's a
> > good idea.
> 
> I think we should avoid any heuristic! There are several drivers and we
> may not be able to work through all of them and make the correct
> decision.

Agreed

> Instead one valid value for the sysfs should be "unknown" and this be
> the default for drivers we have not analysed (this also makes it easy to
> introduce change here).

An "unknown" value sounds good, it allows userspace to just do what it
did/would hace done before this attribute existed.

> We should only set the property to something else for drivers that have
> been reviewed.
> 
> There could be a special case for pwm_bl.c in that I'm prepared to
> assume that the hardware components downstream of the PWM have a
> roughly linear response and that if the user provided tables that their
> function is to provide a perceptually comfortable response.

Unfortunately this isn't universally true :(

At least several Chrome OS devices use a linear brightness scale and
userspace does the transformation in the animated slider. A quick
'git grep -A10 brightness-levels arch' suggests that there are
multiple other devices/platforms using a linear scale.

We could treat devices with a predefined brightness table as
"unknown", unless there is a (new optional) DT property that indicates
the type of the scale.

Cheers

Matthias

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

[Daniel Thompson]

On Wed, Jun 12, 2019 at 12:26:42PM -0700, Matthias Kaehlcke wrote:
> Hi Daniel,
> 
> On Wed, Jun 12, 2019 at 12:03:25PM +0100, Daniel Thompson wrote:
> > On Tue, Jun 11, 2019 at 03:30:19PM -0700, Matthias Kaehlcke wrote:
> > > On Tue, Jun 11, 2019 at 09:55:30AM -0700, Brian Norris wrote:
> > > > On Tue, Jun 11, 2019 at 3:49 AM Daniel Thompson
> > > > <daniel.thompson@linaro.org> wrote:
> > > > > This is a long standing flaw in the backlight interfaces. AFAIK generic
> > > > > userspaces end up with a (flawed) heuristic.
> > > > 
> > > > Bingo! Would be nice if we could start to fix this long-standing flaw.
> > > 
> > > Agreed!
> > > 
> > > How could a fix look like, a sysfs attribute? Would a boolean value
> > > like 'logarithmic_scale' or 'linear_scale' be enough or could more
> > > granularity be needed?
> > 
> > Certainly "linear" (this device will work more or less correctly if the
> > userspace applies perceptual curves). Not sure about logarithmic since
> > what is actually useful is something that is "perceptually linear"
> > (logarithmic is merely a way to approximate that).
> > 
> > I do wonder about a compatible string like most-detailed to
> > least-detailed description. This for a PWM with the auto-generated
> > tables we'd see something like:
> > 
> > cie-1991,perceptual,non-linear
> > 
> > For something that is non-linear but we are not sure what its tables are
> > we can offer just "non-linear".
> 
> Thanks for the feedback!
> 
> It seems clear that we want a string for the added flexibility. I can
> work on a patch with the compatible string like description you
> suggested and we can discuss in the review if we want to go with that
> or prefer something else.

Great, other important thing if we did decide to go this route is there
must be some devices with multiple strings on day 1 (such as the cie-1991
example above).

Whatever we say the ABI is, if we end up with established userspace
components that strcmp("linear", ...) and there are no early counter
examples then we could get stuck without the option to add more
precise tokens as we learn more.


> > > The new attribute could be optional (it only exists if explicitly
> > > specified by the driver) or be set to a default based on a heuristic
> > > if not specified and be 'fixed' on a case by case basis. The latter
> > > might violate "don't break userspace" though, so I'm not sure it's a
> > > good idea.
> > 
> > I think we should avoid any heuristic! There are several drivers and we
> > may not be able to work through all of them and make the correct
> > decision.
> 
> Agreed
> 
> > Instead one valid value for the sysfs should be "unknown" and this be
> > the default for drivers we have not analysed (this also makes it easy to
> > introduce change here).
> 
> An "unknown" value sounds good, it allows userspace to just do what it
> did/would hace done before this attribute existed.
> 
> > We should only set the property to something else for drivers that have
> > been reviewed.
> > 
> > There could be a special case for pwm_bl.c in that I'm prepared to
> > assume that the hardware components downstream of the PWM have a
> > roughly linear response and that if the user provided tables that their
> > function is to provide a perceptually comfortable response.
> 
> Unfortunately this isn't universally true :(
> 
> At least several Chrome OS devices use a linear brightness scale and
> userspace does the transformation in the animated slider. A quick
> 'git grep -A10 brightness-levels arch' suggests that there are
> multiple other devices/platforms using a linear scale.

Good point.

Any clue whether the tables are "stupid" (e.g. offer a poor user experience
with notchy feeling backlight response) or whether they work because
some of the downstream componentry has a non-linear response?


> We could treat devices with a predefined brightness table as
> "unknown", unless there is a (new optional) DT property that indicates
> the type of the scale.

If we have an "unknown" and we don't know then I guess I just claimed
that's what we have to do for cases we don't understand.

For pwm_bl it would be easy to study the table and calculate how far from the
line the centre point is... although that bringing back heuristics into
the picture, albeit more useful ones.

As I said... I'd be OK for the pwm_bl to take a few liberties because it
is so different from the fully fledged LED driver drivers but we don't
need to go crazy ;-)


Daniel.

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

[Matthias Kaehlcke]

On Wed, Jun 12, 2019 at 08:47:57PM +0100, Daniel Thompson wrote:
> On Wed, Jun 12, 2019 at 12:26:42PM -0700, Matthias Kaehlcke wrote:
> > Hi Daniel,
> > 
> > On Wed, Jun 12, 2019 at 12:03:25PM +0100, Daniel Thompson wrote:
> > > On Tue, Jun 11, 2019 at 03:30:19PM -0700, Matthias Kaehlcke wrote:
> > > > On Tue, Jun 11, 2019 at 09:55:30AM -0700, Brian Norris wrote:
> > > > > On Tue, Jun 11, 2019 at 3:49 AM Daniel Thompson
> > > > > <daniel.thompson@linaro.org> wrote:
> > > > > > This is a long standing flaw in the backlight interfaces. AFAIK generic
> > > > > > userspaces end up with a (flawed) heuristic.
> > > > > 
> > > > > Bingo! Would be nice if we could start to fix this long-standing flaw.
> > > > 
> > > > Agreed!
> > > > 
> > > > How could a fix look like, a sysfs attribute? Would a boolean value
> > > > like 'logarithmic_scale' or 'linear_scale' be enough or could more
> > > > granularity be needed?
> > > 
> > > Certainly "linear" (this device will work more or less correctly if the
> > > userspace applies perceptual curves). Not sure about logarithmic since
> > > what is actually useful is something that is "perceptually linear"
> > > (logarithmic is merely a way to approximate that).
> > > 
> > > I do wonder about a compatible string like most-detailed to
> > > least-detailed description. This for a PWM with the auto-generated
> > > tables we'd see something like:
> > > 
> > > cie-1991,perceptual,non-linear
> > > 
> > > For something that is non-linear but we are not sure what its tables are
> > > we can offer just "non-linear".
> > 
> > Thanks for the feedback!
> > 
> > It seems clear that we want a string for the added flexibility. I can
> > work on a patch with the compatible string like description you
> > suggested and we can discuss in the review if we want to go with that
> > or prefer something else.
> 
> Great, other important thing if we did decide to go this route is there
> must be some devices with multiple strings on day 1 (such as the cie-1991
> example above).

Ok, I can add this to the PWM backlight driver (obviously with the
actual handling of the new attribute in the core).

> Whatever we say the ABI is, if we end up with established userspace
> components that strcmp("linear", ...) and there are no early counter
> examples then we could get stuck without the option to add more
> precise tokens as we learn more.

Indeed, we need userspace to understand this isn't necessarily a
'simple' string.

> > > > The new attribute could be optional (it only exists if explicitly
> > > > specified by the driver) or be set to a default based on a heuristic
> > > > if not specified and be 'fixed' on a case by case basis. The latter
> > > > might violate "don't break userspace" though, so I'm not sure it's a
> > > > good idea.
> > > 
> > > I think we should avoid any heuristic! There are several drivers and we
> > > may not be able to work through all of them and make the correct
> > > decision.
> > 
> > Agreed
> > 
> > > Instead one valid value for the sysfs should be "unknown" and this be
> > > the default for drivers we have not analysed (this also makes it easy to
> > > introduce change here).
> > 
> > An "unknown" value sounds good, it allows userspace to just do what it
> > did/would hace done before this attribute existed.
> > 
> > > We should only set the property to something else for drivers that have
> > > been reviewed.
> > > 
> > > There could be a special case for pwm_bl.c in that I'm prepared to
> > > assume that the hardware components downstream of the PWM have a
> > > roughly linear response and that if the user provided tables that their
> > > function is to provide a perceptually comfortable response.
> > 
> > Unfortunately this isn't universally true :(
> > 
> > At least several Chrome OS devices use a linear brightness scale and
> > userspace does the transformation in the animated slider. A quick
> > 'git grep -A10 brightness-levels arch' suggests that there are
> > multiple other devices/platforms using a linear scale.
> 
> Good point.
> 
> Any clue whether the tables are "stupid" (e.g. offer a poor user experience
> with notchy feeling backlight response) or whether they work because
> some of the downstream componentry has a non-linear response?

Sorry, I don't know details about any of these devices, except some of
the Chrome OS ones.

> > We could treat devices with a predefined brightness table as
> > "unknown", unless there is a (new optional) DT property that indicates
> > the type of the scale.
> 
> If we have an "unknown" and we don't know then I guess I just claimed
> that's what we have to do for cases we don't understand.
> 
> For pwm_bl it would be easy to study the table and calculate how far from the
> line the centre point is... although that bringing back heuristics into
> the picture, albeit more useful ones.

True, distinguishing between 'linear' and 'non-linear' shouldn't be a
big deal.

> As I said... I'd be OK for the pwm_bl to take a few liberties because it
> is so different from the fully fledged LED driver drivers but we don't
> need to go crazy ;-)

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

[Pavel Machek]

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

Hi!

> > Certainly "linear" (this device will work more or less correctly if the
> > userspace applies perceptual curves). Not sure about logarithmic since
> > what is actually useful is something that is "perceptually linear"
> > (logarithmic is merely a way to approximate that).
> > 
> > I do wonder about a compatible string like most-detailed to
> > least-detailed description. This for a PWM with the auto-generated
> > tables we'd see something like:
> > 
> > cie-1991,perceptual,non-linear
> > 
> > For something that is non-linear but we are not sure what its tables are
> > we can offer just "non-linear".
> 
> Thanks for the feedback!
> 
> It seems clear that we want a string for the added flexibility. I can
> work on a patch with the compatible string like description you
> suggested and we can discuss in the review if we want to go with that
> or prefer something else.

Compatible-like string seems overly complicated.

> > Instead one valid value for the sysfs should be "unknown" and this be
> > the default for drivers we have not analysed (this also makes it easy to
> > introduce change here).
> 
> An "unknown" value sounds good, it allows userspace to just do what it
> did/would hace done before this attribute existed.

What about simply not presenting the attribute when we don't have the
information?
									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: [PATCH v3 3/4] backlight: pwm_bl: compute brightness of LED linearly to human eye.

[Matthias Kaehlcke]

Hi Pavel,

On Mon, Jun 17, 2019 at 03:01:51PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > Certainly "linear" (this device will work more or less correctly if the
> > > userspace applies perceptual curves). Not sure about logarithmic since
> > > what is actually useful is something that is "perceptually linear"
> > > (logarithmic is merely a way to approximate that).
> > > 
> > > I do wonder about a compatible string like most-detailed to
> > > least-detailed description. This for a PWM with the auto-generated
> > > tables we'd see something like:
> > > 
> > > cie-1991,perceptual,non-linear
> > > 
> > > For something that is non-linear but we are not sure what its tables are
> > > we can offer just "non-linear".
> > 
> > Thanks for the feedback!
> > 
> > It seems clear that we want a string for the added flexibility. I can
> > work on a patch with the compatible string like description you
> > suggested and we can discuss in the review if we want to go with that
> > or prefer something else.
> 
> Compatible-like string seems overly complicated.

I see the merit in the sense that it allows to provide more precision
for if userspace wants/needs it, without requiring userspace to know all
possible (future) options. If userspace wants to keep things simple it
can just check for check for "s == 'non-linear'" and
"s.ends_with(',non-linear')"

In any case, I posted a first version of the patch:

https://lore.kernel.org/patchwork/patch/1088760/

Maybe best to center the discussion there?

> > > Instead one valid value for the sysfs should be "unknown" and this be
> > > the default for drivers we have not analysed (this also makes it easy to
> > > introduce change here).
> > 
> > An "unknown" value sounds good, it allows userspace to just do what it
> > did/would hace done before this attribute existed.
> 
> What about simply not presenting the attribute when we don't have the
> information?

I'm open to either, I mentioned it earlier and Daniel seemed to prefer
the 'unknown' value so I went with it in the first version (it's also
slightly less code).

Cheers

Matthias