| Message ID | 5365E935.4020703@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, May 04, 2014 at 03:16:05PM +0800, Aaron Lu wrote: > On 04/28/2014 09:41 PM, Daniel Vetter wrote: > > 64bit divisions won't compile on 32bit. You need one of the DO_DIV macros, > > or whatever they're called again. I pain, I know ;-) > > Thanks for the correction, here is an updated patch :-) > > From: Aaron Lu <aaron.lu@intel.com> > Date: Mon, 28 Apr 2014 11:02:52 +0800 > Subject: [PATCH v2] drm/i915: restore backlight precision when converting from > ACPI > > When we set backlight on behalf of ACPI opregion, we will convert the > backlight value in the 0-255 range defined in opregion to the actual > hardware level. Commit 22505b82a2 (drm/i915: avoid brightness overflow > when doing scale) is meant to fix the overflow problem when doing the > conversion, but it also caused a problem that the converted hardware > level doesn't quite represent the intended value: say user wants maximum > backlight level(255 in opregion's range), then we will calculate the > actual hardware level to be: level = freq / max * level, where freq is > the hardware's max backlight level(937 on an user's box), and max and > level are all 255. The converted value should be 937 but the above > calculation will yield 765. > > To fix this issue, just use 64 bits to do the calculation to keep the > precision and avoid overflow at the same time. > > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=72491 > Reported-by: Nico Schottelius <nico-bugzilla.kernel.org@schottelius.org> > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > --- > v2: use do_div as reminded by Daniel. > > drivers/gpu/drm/i915/intel_panel.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index a953b081ee38..8725917a3d0d 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -492,6 +492,7 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > enum pipe pipe = intel_get_pipe_from_connector(connector); > u32 freq; > unsigned long flags; > + u64 n; > > if (!panel->backlight.present || pipe == INVALID_PIPE) > return; > @@ -502,10 +503,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, > > /* scale to hardware max, but be careful to not overflow */ > freq = panel->backlight.max; > - if (freq < max) > - level = level * freq / max; > - else > - level = freq / max * level; > + n = level * freq; 32b * 32b = 32b n = (u64)level * freq; to avoid overflow as you claim. Also this still has the same rounding error as before. -Chris
On 05/04/2014 03:22 PM, Chris Wilson wrote: > On Sun, May 04, 2014 at 03:16:05PM +0800, Aaron Lu wrote: >> On 04/28/2014 09:41 PM, Daniel Vetter wrote: >>> 64bit divisions won't compile on 32bit. You need one of the DO_DIV macros, >>> or whatever they're called again. I pain, I know ;-) >> >> Thanks for the correction, here is an updated patch :-) >> >> From: Aaron Lu <aaron.lu@intel.com> >> Date: Mon, 28 Apr 2014 11:02:52 +0800 >> Subject: [PATCH v2] drm/i915: restore backlight precision when converting from >> ACPI >> >> When we set backlight on behalf of ACPI opregion, we will convert the >> backlight value in the 0-255 range defined in opregion to the actual >> hardware level. Commit 22505b82a2 (drm/i915: avoid brightness overflow >> when doing scale) is meant to fix the overflow problem when doing the >> conversion, but it also caused a problem that the converted hardware >> level doesn't quite represent the intended value: say user wants maximum >> backlight level(255 in opregion's range), then we will calculate the >> actual hardware level to be: level = freq / max * level, where freq is >> the hardware's max backlight level(937 on an user's box), and max and >> level are all 255. The converted value should be 937 but the above >> calculation will yield 765. >> >> To fix this issue, just use 64 bits to do the calculation to keep the >> precision and avoid overflow at the same time. >> >> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=72491 >> Reported-by: Nico Schottelius <nico-bugzilla.kernel.org@schottelius.org> >> Signed-off-by: Aaron Lu <aaron.lu@intel.com> >> --- >> v2: use do_div as reminded by Daniel. >> >> drivers/gpu/drm/i915/intel_panel.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c >> index a953b081ee38..8725917a3d0d 100644 >> --- a/drivers/gpu/drm/i915/intel_panel.c >> +++ b/drivers/gpu/drm/i915/intel_panel.c >> @@ -492,6 +492,7 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, >> enum pipe pipe = intel_get_pipe_from_connector(connector); >> u32 freq; >> unsigned long flags; >> + u64 n; >> >> if (!panel->backlight.present || pipe == INVALID_PIPE) >> return; >> @@ -502,10 +503,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, >> >> /* scale to hardware max, but be careful to not overflow */ >> freq = panel->backlight.max; >> - if (freq < max) >> - level = level * freq / max; >> - else >> - level = freq / max * level; >> + n = level * freq; > > 32b * 32b = 32b > > n = (u64)level * freq; to avoid overflow as you claim. Ah...yes, my fault, thanks. > > Also this still has the same rounding error as before. I didn't get this, care to explain?
On Sun, May 04, 2014 at 03:31:01PM +0800, Aaron Lu wrote: > On 05/04/2014 03:22 PM, Chris Wilson wrote: > > Also this still has the same rounding error as before. > > I didn't get this, care to explain? The calculation you use, truncates, rather than say round to nearest, would is the same discrepancy in your changelog. -Chris
On 05/04/2014 06:41 PM, Chris Wilson wrote: > On Sun, May 04, 2014 at 03:31:01PM +0800, Aaron Lu wrote: >> On 05/04/2014 03:22 PM, Chris Wilson wrote: >>> Also this still has the same rounding error as before. >> >> I didn't get this, care to explain? > > The calculation you use, truncates, rather than say round to nearest, > would is the same discrepancy in your changelog. This patch is meant to fix the problem that when user specify the max backlight level in acpi_video interface, we will get the hardware max level too in i915. With this patch, the hardware level we will get is: level * freq / max = 255 * 937 / 255 = 937, which is correct. For other values, the truncates is OK: 254 * 937 / 255 = 933 253 * 937 / 255 = 929 ... ...
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index a953b081ee38..8725917a3d0d 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -492,6 +492,7 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, enum pipe pipe = intel_get_pipe_from_connector(connector); u32 freq; unsigned long flags; + u64 n; if (!panel->backlight.present || pipe == INVALID_PIPE) return; @@ -502,10 +503,9 @@ void intel_panel_set_backlight(struct intel_connector *connector, u32 level, /* scale to hardware max, but be careful to not overflow */ freq = panel->backlight.max; - if (freq < max) - level = level * freq / max; - else - level = freq / max * level; + n = level * freq; + do_div(n, max); + level = n; panel->backlight.level = level; if (panel->backlight.device)