Re: [PATCH v2 1/3] backlight: pwm_bl: Fix interpolation
On Wed, 14 Oct 2020 at 23:55, Geert Uytterhoeven wrote:
>
> Hi Alexandru,
>
> On Tue, Oct 13, 2020 at 1:57 PM 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.
> >
> > Signed-off-by: Alexandru Stan
>
> Thanks for your patch!
Thanks for your reply!
I'm sorry I haven't replied earlier. Looks like your reply was marked as spam.
Rest be assured my spam filter has been disciplined! :D
>
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -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 * ((s64)x - x1),
> > dx);
>
> Yummy, 64-by-32 divisions.
> Shouldn't this use a rounded division?
It won't hurt. But it really doesn't make much of a difference either way.
>
> Nevertheless, I think it would be worthwhile to implement this using
> a (modified) Bresenham algorithm, avoiding multiplications and
> divisions, and possibly increasing accuracy as well.
>
> https://en.wikipedia.org/wiki/Bresenham%27s_line_algorithm
Sure, it might be a little faster to use Bresenham's line algorithm.
Looks like to implement it I would have to deal with some fixed point
math and still have to do divisions occasionally.
I don't think performance is critical here, the values get calculated
only once when the driver loads, and the algorithm's accuracy
improvements might be at most 1 LSB.
Meanwhile the formula I already implemented is almost the same as the
formulas found at
https://en.wikipedia.org/wiki/Linear_interpolation#:~:text=gives
I would like to keep it as is, as straightforward as possible.
>
> > }
> > }
> > - table[levels_count] = data->levels[i];
> > + /* Fill in the last point, since no line starts
> > here. */
> > + table[x2] = y2;
> >
> >
[PATCH v3 0/3] PWM backlight interpolation adjustments
I was trying to adjust the brightness-levels for the trogdor boards: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2291209 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(-) -- 2.28.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 3/3] backlight: pwm_bl: Fix interpolation
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
---
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",
[PATCH v2 0/3] PWM backlight interpolation adjustments
I was trying to adjust the brightness-levels for the trogdor boards: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2291209 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 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): backlight: pwm_bl: Fix interpolation ARM: dts: rockchip: veyron: Remove 0 point from brightness-levels arm64: dts: qcom: trogdor: Add brightness-levels 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(-) -- 2.28.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/3] backlight: pwm_bl: Fix interpolation
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.
Signed-off-by: Alexandru Stan
---
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..3e77f6b73fd9 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;
-
[PATCH 2/3] backlight: pwm_bl: Artificially add 0% during interpolation
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 --- 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
[PATCH 0/3] PWM backlight interpolation adjustments
I was trying to adjust the brightness for a new chromebook: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2291209 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/6233269bce47bd450196a671ab28eb1ec5eb88d9#diff-e401ae20091bbfb311a062c464f4f47fL23 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] backlight: pwm_bl: Fix interpolation
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
Reviewed-by: Matthias Kaehlcke
Signed-off-by: Alexandru Stan
---
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;
