I was trying to adjust the brightness for a new chromebook: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2... Like a lot of panels, the low end needs to be cropped, and now that we have the interpolation stuff I wanted to make use of it and bake in even the curve.
I found the behavior a little unintuitive and non-linear. See patch 1 for a suggested fix for this.
Unfortunatelly a few veyron dts files were relying on this (perhaps weird) behavior. Those devices also want a minimum brightness. The issue is that they also want the 0% point for turning off the display. https://github.com/torvalds/linux/commit/6233269bce47bd450196a671ab28eb1ec5e...
So the idea here is to change those dts files to only say <3 255> (patch 3), and add in a virtual 0% point at the bottom of the scale (patch 2).
We have to do this conditionally because it seems some devices like to have the scale inverted: % git grep "brightness-levels\s*=\s*<\s*[1-9]"|cat arch/arm/boot/dts/tegra124-apalis-eval.dts: brightness-levels = <255 231 223 207 191 159 127 0>;
Alexandru Stan (3): backlight: pwm_bl: Fix interpolation backlight: pwm_bl: Artificially add 0% during interpolation ARM: dts: rockchip: Remove 0 point in backlight
arch/arm/boot/dts/rk3288-veyron-jaq.dts | 2 +- arch/arm/boot/dts/rk3288-veyron-minnie.dts | 2 +- arch/arm/boot/dts/rk3288-veyron-tiger.dts | 2 +- drivers/video/backlight/pwm_bl.c | 78 +++++++++++----------- 4 files changed, 42 insertions(+), 42 deletions(-)
Whenever num-interpolated-steps was larger than the distance between 2 consecutive brightness levels the table would get really discontinuous. The slope of the interpolation would stick with integers only and if it was 0 the whole line segment would get skipped.
Example settings: brightness-levels = <0 1 2 4 8 16 32 64 128 256>; num-interpolated-steps = <16>;
The distances between 1 2 4 and 8 would be 1, and only starting with 16 it would start to interpolate properly.
Let's change it so there's always interpolation happening, even if there's no enough points available (read: values in the table would appear more than once). This should match the expected behavior much more closely.
Reviewed-by: Douglas Anderson dianders@chromium.org Reviewed-by: Matthias Kaehlcke mka@chromium.org Signed-off-by: Alexandru Stan amstan@chromium.org ---
drivers/video/backlight/pwm_bl.c | 70 ++++++++++++++------------------ 1 file changed, 31 insertions(+), 39 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 82b8d7594701..5193a72305a2 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -235,8 +235,7 @@ 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_levels; unsigned int num_steps = 0; struct property *prop; unsigned int *table; @@ -265,12 +264,11 @@ static int pwm_backlight_parse_dt(struct device *dev, if (!prop) return 0;
- data->max_brightness = length / sizeof(u32); + num_levels = length / sizeof(u32);
/* 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; + if (num_levels > 0) { + size_t size = sizeof(*data->levels) * num_levels;
data->levels = devm_kzalloc(dev, size, GFP_KERNEL); if (!data->levels) @@ -278,7 +276,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
ret = of_property_read_u32_array(node, "brightness-levels", data->levels, - data->max_brightness); + num_levels); if (ret < 0) return ret;
@@ -303,7 +301,13 @@ static int pwm_backlight_parse_dt(struct device *dev, * between two points. */ if (num_steps) { - if (data->max_brightness < 2) { + unsigned int num_input_levels = num_levels; + unsigned int i; + u32 x1, x2, x; + u32 y1, y2; + s64 dx, dy; + + if (num_input_levels < 2) { dev_err(dev, "can't interpolate\n"); return -EINVAL; } @@ -313,14 +317,7 @@ static int pwm_backlight_parse_dt(struct device *dev, * 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++; + num_levels = (num_input_levels - 1) * num_steps + 1; dev_dbg(dev, "new number of brightness levels: %d\n", num_levels);
@@ -332,24 +329,25 @@ static int pwm_backlight_parse_dt(struct device *dev, 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++; + /* + * Fill the interpolated table[x] = y + * by draw lines between each (x1, y1) to (x2, y2). + */ + dx = num_steps; + for (i = 0; i < num_input_levels - 1; i++) { + x1 = i * dx; + x2 = x1 + dx; + y1 = data->levels[i]; + y2 = data->levels[i + 1]; + dy = y2 - y1; + + for (x = x1; x < x2; x++) { + table[x] = y1 + + div_s64(dy * (x - x1), dx); } } - table[levels_count] = data->levels[i]; + /* Fill in the last point, since no line starts here. */ + table[x2] = y2;
/* * As we use interpolation lets remove current @@ -358,15 +356,9 @@ static int pwm_backlight_parse_dt(struct device *dev, */ 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--; + data->max_brightness = num_levels - 1; }
return 0;
On Mon, Jul 20, 2020 at 09:25:20PM -0700, Alexandru Stan wrote:
Whenever num-interpolated-steps was larger than the distance between 2 consecutive brightness levels the table would get really discontinuous. The slope of the interpolation would stick with integers only and if it was 0 the whole line segment would get skipped.
Example settings: brightness-levels = <0 1 2 4 8 16 32 64 128 256>; num-interpolated-steps = <16>;
The distances between 1 2 4 and 8 would be 1, and only starting with 16 it would start to interpolate properly.
Let's change it so there's always interpolation happening, even if there's no enough points available (read: values in the table would appear more than once). This should match the expected behavior much more closely.
Reviewed-by: Douglas Anderson dianders@chromium.org Reviewed-by: Matthias Kaehlcke mka@chromium.org Signed-off-by: Alexandru Stan amstan@chromium.org
Apologies for the delay. Patch 2/3 meant I had some thinking to do... and then the holiday's took their toll.
Overall this looks good, just some quibbles about broken 64-bit maths.
drivers/video/backlight/pwm_bl.c | 70 ++++++++++++++------------------ 1 file changed, 31 insertions(+), 39 deletions(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 82b8d7594701..5193a72305a2 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -235,8 +235,7 @@ 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_levels; unsigned int num_steps = 0; struct property *prop; unsigned int *table;
@@ -265,12 +264,11 @@ static int pwm_backlight_parse_dt(struct device *dev, if (!prop) return 0;
- data->max_brightness = length / sizeof(u32);
num_levels = length / sizeof(u32);
/* 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;
if (num_levels > 0) {
size_t size = sizeof(*data->levels) * num_levels;data->levels = devm_kzalloc(dev, size, GFP_KERNEL); if (!data->levels)
@@ -278,7 +276,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
ret = of_property_read_u32_array(node, "brightness-levels", data->levels,
data->max_brightness);
num_levels);@@ -303,7 +301,13 @@ static int pwm_backlight_parse_dt(struct device *dev, * between two points. */ if (num_steps) {
if (data->max_brightness < 2) {
unsigned int num_input_levels = num_levels;unsigned int i;u32 x1, x2, x;u32 y1, y2;s64 dx, dy;
dx should be 32-bit. It will be truncated to 32-bit when passed to div_s64() so this type is actively misleading about how the maths works.
if (num_input_levels < 2) { dev_err(dev, "can't interpolate\n"); return -EINVAL; }@@ -313,14 +317,7 @@ static int pwm_backlight_parse_dt(struct device *dev, * 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;elsenum_levels++;}num_levels++;
num_levels = (num_input_levels - 1) * num_steps + 1; dev_dbg(dev, "new number of brightness levels: %d\n", num_levels);@@ -332,24 +329,25 @@ static int pwm_backlight_parse_dt(struct device *dev, 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++;
/** Fill the interpolated table[x] = y* by draw lines between each (x1, y1) to (x2, y2).*/dx = num_steps;for (i = 0; i < num_input_levels - 1; i++) {x1 = i * dx;x2 = x1 + dx;y1 = data->levels[i];y2 = data->levels[i + 1];dy = y2 - y1;
This is an u32 expression being assigned to a s64. I could be rusty on my fixed point maths but won't this promote too late for the 64-bitness of dy to be useful?
Daniel.
for (x = x1; x < x2; x++) {table[x] = y1 +div_s64(dy * (x - x1), dx); } }
table[levels_count] = data->levels[i];
/* Fill in the last point, since no line starts here. */table[x2] = y2; /* * As we use interpolation lets remove current@@ -358,15 +356,9 @@ static int pwm_backlight_parse_dt(struct device *dev, */ 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--;
data->max_brightness = num_levels - 1;}
return 0;
-- 2.27.0
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
Signed-off-by: Alexandru Stan amstan@chromium.org ---
drivers/video/backlight/pwm_bl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 5193a72305a2..b24711ddf504 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -349,6 +349,14 @@ static int pwm_backlight_parse_dt(struct device *dev, /* Fill in the last point, since no line starts here. */ table[x2] = y2;
+ /* + * If we don't start at 0 yet we're increasing, assume + * the dts wanted to crop the low end of the range, so + * insert a 0 to provide a display off mode. + */ + if (table[0] > 0 && table[0] < table[num_levels - 1]) + table[0] = 0; + /* * As we use interpolation lets remove current * brightness levels table and replace for the
On Mon, Jul 20, 2020 at 09:25:21PM -0700, Alexandru Stan wrote:
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
Signed-off-by: Alexandru Stan amstan@chromium.org
drivers/video/backlight/pwm_bl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 5193a72305a2..b24711ddf504 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -349,6 +349,14 @@ static int pwm_backlight_parse_dt(struct device *dev, /* Fill in the last point, since no line starts here. */ table[x2] = y2;
/** If we don't start at 0 yet we're increasing, assume* the dts wanted to crop the low end of the range, so* insert a 0 to provide a display off mode.*/if (table[0] > 0 && table[0] < table[num_levels - 1])table[0] = 0;
Isn't that what the enable/disable switch in backlights are for? There's lots of backligh drivers (mostly the firmware variety) where setting the backlight to 0 does not shut it off, it's just the lowest setting.
But I've not been involved in the details of these discussions. -Daniel
/* * As we use interpolation lets remove current * brightness levels table and replace for the-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Aug 07, 2020 at 10:21:13AM +0200, daniel@ffwll.ch wrote:
On Mon, Jul 20, 2020 at 09:25:21PM -0700, Alexandru Stan wrote:
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
Signed-off-by: Alexandru Stan amstan@chromium.org
drivers/video/backlight/pwm_bl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 5193a72305a2..b24711ddf504 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -349,6 +349,14 @@ static int pwm_backlight_parse_dt(struct device *dev, /* Fill in the last point, since no line starts here. */ table[x2] = y2;
/** If we don't start at 0 yet we're increasing, assume* the dts wanted to crop the low end of the range, so* insert a 0 to provide a display off mode.*/if (table[0] > 0 && table[0] < table[num_levels - 1])table[0] = 0;Isn't that what the enable/disable switch in backlights are for? There's lots of backligh drivers (mostly the firmware variety) where setting the backlight to 0 does not shut it off, it's just the lowest setting.
But I've not been involved in the details of these discussions.
It's been a long standing complaint that the backlight drivers are not consistent w.r.t. whether 0 means off or lowest. The most commonly used backlights (ACPI in particular) do not adopt 0 means off but lots of specific drivers do.
IMHO what is "right" depends on the display technology. For displays that are essentially black when the backlight is off and become difficult or impossible to read I'm a little dubious about standardizing on zero means off. There are situations when zero means off does make sense however. For example front-lit or transflexive displays are readable when the "backlight" is off and on these displays it would make sense.
Daniel.
On Mon, Jul 20, 2020 at 09:25:21PM -0700, Alexandru Stan wrote:
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
For backlights it is not defined that 0 means off and, to be honest, 0 means off is actually rather weird for anything except transflexive or front lit reflective displays[1]. There is a problem on several systems that when the backlight slider is reduced to zero you can't see the screen properly to turn it back up. This patch looks like it would make that problem worse by hurting systems with will written device trees.
There is some nasty legacy here: some backlight displays that are off at zero and that sucks because userspace doesn't know whether zero is off or lowest possible setting.
Nevertheless perhaps a better way to handle this case is for 0 to map to 5% power and for the userspace to turn the backlight on/off as final step in an animated backlight fade out (and one again for a fade in).
Daniel.
Signed-off-by: Alexandru Stan amstan@chromium.org
drivers/video/backlight/pwm_bl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 5193a72305a2..b24711ddf504 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -349,6 +349,14 @@ static int pwm_backlight_parse_dt(struct device *dev, /* Fill in the last point, since no line starts here. */ table[x2] = y2;
/** If we don't start at 0 yet we're increasing, assume* the dts wanted to crop the low end of the range, so* insert a 0 to provide a display off mode.*/if (table[0] > 0 && table[0] < table[num_levels - 1])table[0] = 0;/* * As we use interpolation lets remove current * brightness levels table and replace for the-- 2.27.0
On Fri, Sep 04, 2020 at 12:38:22PM +0100, Daniel Thompson wrote:
On Mon, Jul 20, 2020 at 09:25:21PM -0700, Alexandru Stan wrote:
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
For backlights it is not defined that 0 means off and, to be honest, 0 means off is actually rather weird for anything except transflexive or front lit reflective displays[1]. There is a problem on several systems that when the backlight slider is reduced to zero you can't see the screen properly to turn it back up. This patch looks like it would make that problem worse by hurting systems with will written device trees.
There is some nasty legacy here: some backlight displays that are off at zero and that sucks because userspace doesn't know whether zero is off or lowest possible setting.
Nevertheless perhaps a better way to handle this case is for 0 to map to 5% power and for the userspace to turn the backlight on/off as final step in an animated backlight fade out (and one again for a fade in).
Afaik chromeos encodes "0 means off" somewhere in there stack. We've gotten similar patches for the i915 backlight driver when we started obeying the panel's lower limit in our pwm backlight driver thing that's sometimes used instead of acpi.
There's also the problem that with fancy panels with protocol (dsi, edp, ...) shutting of the backlight completely out of the proper power sequence hangs the panel (for some panels at least), so providing a backlight off that doesn't go through the drm modeset sequence isn't always possible.
It's a bit a mess indeed :-/ -Daniel
Daniel.
Signed-off-by: Alexandru Stan amstan@chromium.org
drivers/video/backlight/pwm_bl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 5193a72305a2..b24711ddf504 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -349,6 +349,14 @@ static int pwm_backlight_parse_dt(struct device *dev, /* Fill in the last point, since no line starts here. */ table[x2] = y2;
/** If we don't start at 0 yet we're increasing, assume* the dts wanted to crop the low end of the range, so* insert a 0 to provide a display off mode.*/if (table[0] > 0 && table[0] < table[num_levels - 1])table[0] = 0;/* * As we use interpolation lets remove current * brightness levels table and replace for the-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Sep 07, 2020 at 09:50:18AM +0200, Daniel Vetter wrote:
On Fri, Sep 04, 2020 at 12:38:22PM +0100, Daniel Thompson wrote:
On Mon, Jul 20, 2020 at 09:25:21PM -0700, Alexandru Stan wrote:
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
For backlights it is not defined that 0 means off and, to be honest, 0 means off is actually rather weird for anything except transflexive or front lit reflective displays[1]. There is a problem on several systems that when the backlight slider is reduced to zero you can't see the screen properly to turn it back up. This patch looks like it would make that problem worse by hurting systems with will written device trees.
There is some nasty legacy here: some backlight displays that are off at zero and that sucks because userspace doesn't know whether zero is off or lowest possible setting.
Nevertheless perhaps a better way to handle this case is for 0 to map to 5% power and for the userspace to turn the backlight on/off as final step in an animated backlight fade out (and one again for a fade in).
Afaik chromeos encodes "0 means off" somewhere in there stack. We've gotten similar patches for the i915 backlight driver when we started obeying the panel's lower limit in our pwm backlight driver thing that's sometimes used instead of acpi.
Out of interest... were they accepted?
I did took a quick look at intel_panel.c and didn't see anything that appeared to be special casing zero but I thought I might double check.
Daniel.
There's also the problem that with fancy panels with protocol (dsi, edp, ...) shutting of the backlight completely out of the proper power sequence hangs the panel (for some panels at least), so providing a backlight off that doesn't go through the drm modeset sequence isn't always possible.
It's a bit a mess indeed :-/ -Daniel
Daniel.
Signed-off-by: Alexandru Stan amstan@chromium.org
drivers/video/backlight/pwm_bl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 5193a72305a2..b24711ddf504 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -349,6 +349,14 @@ static int pwm_backlight_parse_dt(struct device *dev, /* Fill in the last point, since no line starts here. */ table[x2] = y2;
/** If we don't start at 0 yet we're increasing, assume* the dts wanted to crop the low end of the range, so* insert a 0 to provide a display off mode.*/if (table[0] > 0 && table[0] < table[num_levels - 1])table[0] = 0;/* * As we use interpolation lets remove current * brightness levels table and replace for the-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, Sep 9, 2020 at 4:45 PM Daniel Thompson daniel.thompson@linaro.org wrote:
On Mon, Sep 07, 2020 at 09:50:18AM +0200, Daniel Vetter wrote:
On Fri, Sep 04, 2020 at 12:38:22PM +0100, Daniel Thompson wrote:
On Mon, Jul 20, 2020 at 09:25:21PM -0700, Alexandru Stan wrote:
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
For backlights it is not defined that 0 means off and, to be honest, 0 means off is actually rather weird for anything except transflexive or front lit reflective displays[1]. There is a problem on several systems that when the backlight slider is reduced to zero you can't see the screen properly to turn it back up. This patch looks like it would make that problem worse by hurting systems with will written device trees.
There is some nasty legacy here: some backlight displays that are off at zero and that sucks because userspace doesn't know whether zero is off or lowest possible setting.
Nevertheless perhaps a better way to handle this case is for 0 to map to 5% power and for the userspace to turn the backlight on/off as final step in an animated backlight fade out (and one again for a fade in).
Afaik chromeos encodes "0 means off" somewhere in there stack. We've gotten similar patches for the i915 backlight driver when we started obeying the panel's lower limit in our pwm backlight driver thing that's sometimes used instead of acpi.
Out of interest... were they accepted?
I did took a quick look at intel_panel.c and didn't see anything that appeared to be special casing zero but I thought I might double check.
I don't think so. Just figured I bring this up since it might explain why this is coming back again from an @chromium.com address. -Daniel
Daniel.
There's also the problem that with fancy panels with protocol (dsi, edp, ...) shutting of the backlight completely out of the proper power sequence hangs the panel (for some panels at least), so providing a backlight off that doesn't go through the drm modeset sequence isn't always possible.
It's a bit a mess indeed :-/ -Daniel
Daniel.
Signed-off-by: Alexandru Stan amstan@chromium.org
drivers/video/backlight/pwm_bl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 5193a72305a2..b24711ddf504 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -349,6 +349,14 @@ static int pwm_backlight_parse_dt(struct device *dev, /* Fill in the last point, since no line starts here. */ table[x2] = y2;
/** If we don't start at 0 yet we're increasing, assume* the dts wanted to crop the low end of the range, so* insert a 0 to provide a display off mode.*/if (table[0] > 0 && table[0] < table[num_levels - 1])table[0] = 0;/* * As we use interpolation lets remove current * brightness levels table and replace for the-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 09, 2020 at 05:03:37PM +0200, Daniel Vetter wrote:
On Wed, Sep 9, 2020 at 4:45 PM Daniel Thompson daniel.thompson@linaro.org wrote:
On Mon, Sep 07, 2020 at 09:50:18AM +0200, Daniel Vetter wrote:
On Fri, Sep 04, 2020 at 12:38:22PM +0100, Daniel Thompson wrote:
On Mon, Jul 20, 2020 at 09:25:21PM -0700, Alexandru Stan wrote:
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
For backlights it is not defined that 0 means off and, to be honest, 0 means off is actually rather weird for anything except transflexive or front lit reflective displays[1]. There is a problem on several systems that when the backlight slider is reduced to zero you can't see the screen properly to turn it back up. This patch looks like it would make that problem worse by hurting systems with will written device trees.
There is some nasty legacy here: some backlight displays that are off at zero and that sucks because userspace doesn't know whether zero is off or lowest possible setting.
Nevertheless perhaps a better way to handle this case is for 0 to map to 5% power and for the userspace to turn the backlight on/off as final step in an animated backlight fade out (and one again for a fade in).
Afaik chromeos encodes "0 means off" somewhere in there stack. We've gotten similar patches for the i915 backlight driver when we started obeying the panel's lower limit in our pwm backlight driver thing that's sometimes used instead of acpi.
Out of interest... were they accepted?
I did took a quick look at intel_panel.c and didn't see anything that appeared to be special casing zero but I thought I might double check.
I don't think so. Just figured I bring this up since it might explain why this is coming back again from an @chromium.com address.
Thanks to Alexandru pointing at the right code it's clear this got fix, over 5 years ago:
https://source.chromium.org/chromiumos/_/chromium/chromiumos/platform2/+/6e8...
power: Write to bl_power when backlight reaches or leaves 0. Make InternalBacklight write FB_BLANK_UNBLANK to bl_power just before setting the internal backlight brightness to a nonzero value, and FB_BLANK_POWERDOWN to bl_power just before setting the brightness to 0. This is needed for hardware that interprets a brightness level of 0 as meaning "dim" rather than "off". BUG=chromium:396218 TEST=added unit tests Change-Id: I914e333ab41db623564d5a67beac89f1a62bce9d Reviewed-on: https://chromium-review.googlesource.com/311010 Commit-Ready: Dan Erat derat@chromium.org Tested-by: Dan Erat derat@chromium.org Reviewed-by: Dan Erat derat@chromium.org
Cheers, Daniel
There's also the problem that with fancy panels with protocol (dsi, edp, ...) shutting of the backlight completely out of the proper power sequence hangs the panel (for some panels at least), so providing a backlight off that doesn't go through the drm modeset sequence isn't always possible.
It's a bit a mess indeed :-/ -Daniel
Daniel.
Signed-off-by: Alexandru Stan amstan@chromium.org
drivers/video/backlight/pwm_bl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 5193a72305a2..b24711ddf504 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -349,6 +349,14 @@ static int pwm_backlight_parse_dt(struct device *dev, /* Fill in the last point, since no line starts here. */ table[x2] = y2;
/** If we don't start at 0 yet we're increasing, assume* the dts wanted to crop the low end of the range, so* insert a 0 to provide a display off mode.*/if (table[0] > 0 && table[0] < table[num_levels - 1])table[0] = 0;/* * As we use interpolation lets remove current * brightness levels table and replace for the-- 2.27.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Fri, Sep 4, 2020 at 4:38 AM Daniel Thompson daniel.thompson@linaro.org wrote:
On Mon, Jul 20, 2020 at 09:25:21PM -0700, Alexandru Stan wrote:
Some displays need the low end of the curve cropped in order to make them happy. In that case we still want to have the 0% point, even though anything between 0% and 5%(example) would be skipped.
For backlights it is not defined that 0 means off and, to be honest, 0 means off is actually rather weird for anything except transflexive or front lit reflective displays[1]. There is a problem on several systems that when the backlight slider is reduced to zero you can't see the screen properly to turn it back up. This patch looks like it would make that problem worse by hurting systems with will written device trees.
There is some nasty legacy here: some backlight displays that are off at zero and that sucks because userspace doesn't know whether zero is off or lowest possible setting.
Nevertheless perhaps a better way to handle this case is for 0 to map to 5% power and for the userspace to turn the backlight on/off as final step in an animated backlight fade out (and one again for a fade in).
Hello
Apologies for my delay. Thanks for the responses!
Yeah, I felt pretty sketchy about this 0% patch as well. But I figured it's better to send my suggestion and get corrected than lose the fixed interpolation patch.
Turns out there's no reason to need 2/3. I was mistaken: echo "4" > /sys/devices/platform/backlight/backlight/backlight/bl_power seems to work just fine to turn the backlight off, nothing special about my device (pwm comes from cros_ec). Chrome OS user space already makes full use of that knob (https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/pl...) I wanted to try X11 on the same device but I haven't gotten to it yet, perhaps I'll post my results in the next cover letter. So it seems I didn't have to worry about "not breaking userspace" on the existing devices.
I'll respin this patch set: keep 1/3 and 3/3, remove 2/3, and potentially add another one to update trogdor's dtsi (since that's where I want this fixed linear interpolation to happen).
Thank you, Alexandru Stan
On Mon, Jul 20, 2020 at 9:27 PM Alexandru Stan amstan@chromium.org wrote:
I was trying to adjust the brightness for a new chromebook: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2... Like a lot of panels, the low end needs to be cropped, and now that we have the interpolation stuff I wanted to make use of it and bake in even the curve.
I found the behavior a little unintuitive and non-linear. See patch 1 for a suggested fix for this.
Unfortunatelly a few veyron dts files were relying on this (perhaps weird) behavior. Those devices also want a minimum brightness. The issue is that they also want the 0% point for turning off the display. https://github.com/torvalds/linux/commit/6233269bce47bd450196a671ab28eb1ec5e...
So the idea here is to change those dts files to only say <3 255> (patch 3), and add in a virtual 0% point at the bottom of the scale (patch 2).
We have to do this conditionally because it seems some devices like to have the scale inverted: % git grep "brightness-levels\s*=\s*<\s*[1-9]"|cat arch/arm/boot/dts/tegra124-apalis-eval.dts: brightness-levels = <255 231 223 207 191 159 127 0>;
Alexandru Stan (3): backlight: pwm_bl: Fix interpolation backlight: pwm_bl: Artificially add 0% during interpolation ARM: dts: rockchip: Remove 0 point in backlight
arch/arm/boot/dts/rk3288-veyron-jaq.dts | 2 +- arch/arm/boot/dts/rk3288-veyron-minnie.dts | 2 +- arch/arm/boot/dts/rk3288-veyron-tiger.dts | 2 +- drivers/video/backlight/pwm_bl.c | 78 +++++++++++----------- 4 files changed, 42 insertions(+), 42 deletions(-)
-- 2.27.0
Hello,
Friendly ping. Let me know if you would like me to make any changes to my patches.
Thanks, Alexandru M Stan
dri-devel@lists.freedesktop.org
-
Alexandru M Stan -
Alexandru Stan
-
Daniel Thompson -
Daniel Vetter -
danielļ¼ ffwll.ch