I was trying to adjust the brightness-levels for the trogdor boards: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2... Like on a lot of panels, trogdor's 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 that's customary to have on chromebooks.
I found the current behavior of the pwm_bl driver a little unintuitive and non-linear. See patch 1 for a suggested fix for this.
A few veyron dts files were relying on this (perhaps weird) behavior. Those devices also want a minimum brightness like trogdor, so changed them to use the new way.
Finally, given that trogdor's dts is part of linux-next now, add the brightness-levels to it, since that's the original reason I was looking at this.
Changes in v3: - Reordered patches, since both dts changes will work just fine even before the driver change. - Rewrote a bit of the commit message to describe the new policy, as Daniel suggested. - Removed redundant s64 for something that's always positive
Changes in v2: - Fixed type promotion in the driver - Removed "backlight: pwm_bl: Artificially add 0% during interpolation", userspace works just fine without it because it already knows how to use bl_power for turning off the display. - Added brightness-levels to trogdor as well, now the dts is upstream.
Alexandru Stan (3): ARM: dts: rockchip: veyron: Remove 0 point from brightness-levels arm64: dts: qcom: trogdor: Add brightness-levels backlight: pwm_bl: Fix interpolation
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 +- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 9 +++ drivers/video/backlight/pwm_bl.c | 70 +++++++++----------- 5 files changed, 43 insertions(+), 42 deletions(-)
The previous behavior was a little unexpected, its properties/problems: 1. It was designed to generate strictly increasing values (no repeats) 2. It had quantization errors when calculating step size. Resulting in unexpected jumps near the end of some segments.
Example settings: brightness-levels = <0 1 2 4 8 16 32 64 128 256>; num-interpolated-steps = <16>;
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.
The distances between 1 2 4 and 8 would be 1 (property #1 fighting us), and only starting with 16 it would start to interpolate properly.
Property #1 is not enough. The goal here is more than just monotonically increasing. We should still care about the shape of the curve. Repeated points might be desired if we're in the part of the curve where we want to go slow (aka slope near 0).
Problem #2 is plainly a bug. Imagine if the 64 entry was 63 instead, the calculated slope on the 32-63 segment will be almost half as it should be.
The most expected and simplest algorithm for interpolation is linear interpolation, which would handle both problems. Let's just implement that!
Take pairs of points from the brightness-levels array and linearly interpolate between them. On the X axis (what userspace sees) we'll now have equally sized intervals (num-interpolated-steps sized, as opposed to before where we were at the mercy of quantization).
END
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 dfc760830eb9..e48fded3e414 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -230,8 +230,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; @@ -260,12 +259,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) @@ -273,7 +271,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;
@@ -298,7 +296,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, dx; + u32 y1, y2; + s64 dy; + + if (num_input_levels < 2) { dev_err(dev, "can't interpolate\n"); return -EINVAL; } @@ -308,14 +312,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);
@@ -327,24 +324,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 = (s64)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 @@ -353,15 +351,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 Wed, Oct 21, 2020 at 10:04:45PM -0700, Alexandru Stan wrote:
The previous behavior was a little unexpected, its properties/problems:
- It was designed to generate strictly increasing values (no repeats)
- It had quantization errors when calculating step size. Resulting in
unexpected jumps near the end of some segments.
Example settings: brightness-levels = <0 1 2 4 8 16 32 64 128 256>; num-interpolated-steps = <16>;
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.
The distances between 1 2 4 and 8 would be 1 (property #1 fighting us), and only starting with 16 it would start to interpolate properly.
Property #1 is not enough. The goal here is more than just monotonically increasing. We should still care about the shape of the curve. Repeated points might be desired if we're in the part of the curve where we want to go slow (aka slope near 0).
Problem #2 is plainly a bug. Imagine if the 64 entry was 63 instead, the calculated slope on the 32-63 segment will be almost half as it should be.
The most expected and simplest algorithm for interpolation is linear interpolation, which would handle both problems. Let's just implement that!
Take pairs of points from the brightness-levels array and linearly interpolate between them. On the X axis (what userspace sees) we'll now have equally sized intervals (num-interpolated-steps sized, as opposed to before where we were at the mercy of quantization).
END
INTERESTING.
I guess this a copy 'n paste error from some internal log book? Better removed... but I won't lose sleep over it.
Signed-off-by: Alexandru Stan amstan@chromium.org
I've waited a bit to see how strong the feelings were w.r.t. getting rid of the division from the table initialization. It was something I was aware of during an earlier review but it was below my personal nitpicking threshold (which could be badly calibrated... hence waiting). However it's all been quiet so:
Reviewed-by: Daniel Thompson daniel.thompson@linaro.org
Daniel.
On Wed, Oct 28, 2020 at 8:12 AM Daniel Thompson daniel.thompson@linaro.org wrote:
On Wed, Oct 21, 2020 at 10:04:45PM -0700, Alexandru Stan wrote:
The previous behavior was a little unexpected, its properties/problems:
- It was designed to generate strictly increasing values (no repeats)
- It had quantization errors when calculating step size. Resulting in
unexpected jumps near the end of some segments.
Example settings: brightness-levels = <0 1 2 4 8 16 32 64 128 256>; num-interpolated-steps = <16>;
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.
The distances between 1 2 4 and 8 would be 1 (property #1 fighting us), and only starting with 16 it would start to interpolate properly.
Property #1 is not enough. The goal here is more than just monotonically increasing. We should still care about the shape of the curve. Repeated points might be desired if we're in the part of the curve where we want to go slow (aka slope near 0).
Problem #2 is plainly a bug. Imagine if the 64 entry was 63 instead, the calculated slope on the 32-63 segment will be almost half as it should be.
The most expected and simplest algorithm for interpolation is linear interpolation, which would handle both problems. Let's just implement that!
Take pairs of points from the brightness-levels array and linearly interpolate between them. On the X axis (what userspace sees) we'll now have equally sized intervals (num-interpolated-steps sized, as opposed to before where we were at the mercy of quantization).
END
INTERESTING.
I guess this a copy 'n paste error from some internal log book? Better removed... but I won't lose sleep over it.
Sorry! Yeah, I mistakenly duplicated the "END" line in patman.
Signed-off-by: Alexandru Stan amstan@chromium.org
I've waited a bit to see how strong the feelings were w.r.t. getting rid of the division from the table initialization. It was something I was aware of during an earlier review but it was below my personal nitpicking threshold (which could be badly calibrated... hence waiting). However it's all been quiet so:
Reviewed-by: Daniel Thompson daniel.thompson@linaro.org
Daniel.
Alexandru Stan (amstan)
On Wed, 21 Oct 2020, Alexandru Stan wrote:
The previous behavior was a little unexpected, its properties/problems:
- It was designed to generate strictly increasing values (no repeats)
- It had quantization errors when calculating step size. Resulting in
unexpected jumps near the end of some segments.
Example settings: brightness-levels = <0 1 2 4 8 16 32 64 128 256>; num-interpolated-steps = <16>;
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.
The distances between 1 2 4 and 8 would be 1 (property #1 fighting us), and only starting with 16 it would start to interpolate properly.
Property #1 is not enough. The goal here is more than just monotonically increasing. We should still care about the shape of the curve. Repeated points might be desired if we're in the part of the curve where we want to go slow (aka slope near 0).
Problem #2 is plainly a bug. Imagine if the 64 entry was 63 instead, the calculated slope on the 32-63 segment will be almost half as it should be.
The most expected and simplest algorithm for interpolation is linear interpolation, which would handle both problems. Let's just implement that!
Take pairs of points from the brightness-levels array and linearly interpolate between them. On the X axis (what userspace sees) we'll now have equally sized intervals (num-interpolated-steps sized, as opposed to before where we were at the mercy of quantization).
END
I removed this.
Signed-off-by: Alexandru Stan amstan@chromium.org
drivers/video/backlight/pwm_bl.c | 70 ++++++++++++++------------------ 1 file changed, 31 insertions(+), 39 deletions(-)
Applied, thanks.
On Wed, 21 Oct 2020 22:04:42 -0700, Alexandru Stan wrote:
I was trying to adjust the brightness-levels for the trogdor boards: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2... Like on a lot of panels, trogdor's 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 that's customary to have on chromebooks.
I found the current behavior of the pwm_bl driver a little unintuitive and non-linear. See patch 1 for a suggested fix for this.
[...]
Applied, thanks!
[1/1] ARM: dts: rockchip: Remove 0 point from brightness-levels on rk3288-veyron commit: 225c59b9235a421cdb219be5fbc13126a49714a6
Best regards,
dri-devel@lists.freedesktop.org
-
Alexandru M Stan -
Alexandru Stan
-
Daniel Thompson -
Heiko Stuebner -
Lee Jones