| Message ID | 1412030973-16738-2-git-send-email-ullysses.a.eoff@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 29, 2014 at 03:49:32PM -0700, U. Artie Eoff wrote: > Improper truncated integer division in the scale() function causes > actual_brightness != brightness. This (partial) work-around should be > sufficient for a majority of use-cases, but it is by no means a complete > solution. > > TODO: Determine how best to scale "user" values to "hw" values, and > vice-versa, when the ranges are of different sizes. That would be a > buggy scenario even with this work-around. > > The issue was introduced in the following (v3.17-rc1) commit: > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > macro > > v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani) > -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien) > -v1 and v2 originally authored by Joe Konno. > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> Is there some bug report, internal jira, mailing list reference or similar about this? Note that at least for OTC jira tasks we now want them to be added to commit message with e.g. OTC-Jria: VIZ-4932 And I guess I should merge patch 2 before patch 1, right? Cheers, Daniel > --- > drivers/gpu/drm/i915/intel_panel.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index f17ada3..f7da913 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -398,6 +398,9 @@ intel_panel_detect(struct drm_device *dev) > } > } > > +#define DIV_ROUND_CLOSEST_ULL(ll, d) \ > +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; }) > + > /** > * scale - scale values from one range to another > * > @@ -419,9 +422,8 @@ static uint32_t scale(uint32_t source_val, > source_val = clamp(source_val, source_min, source_max); > > /* avoid overflows */ > - target_val = (uint64_t)(source_val - source_min) * > - (target_max - target_min); > - do_div(target_val, source_max - source_min); > + target_val = DIV_ROUND_CLOSEST_ULL((uint64_t)(source_val - source_min) * > + (target_max - target_min), source_max - source_min); > target_val += target_min; > > return target_val; > -- > 1.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2014-09-30 at 10:04 +0200, Daniel Vetter wrote: > On Mon, Sep 29, 2014 at 03:49:32PM -0700, U. Artie Eoff wrote: > > Improper truncated integer division in the scale() function causes > > actual_brightness != brightness. This (partial) work-around should be > > sufficient for a majority of use-cases, but it is by no means a complete > > solution. > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > vice-versa, when the ranges are of different sizes. That would be a > > buggy scenario even with this work-around. > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > macro > > > > v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani) > > -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien) > > -v1 and v2 originally authored by Joe Konno. > > > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > > Is there some bug report, internal jira, mailing list reference or similar > about this? > > Note that at least for OTC jira tasks we now want them to be added to > commit message with e.g. > > OTC-Jria: VIZ-4932 > Yes, the OTC-Jira task is: VIZ-4395. I'll resubmit with amended commit message. > And I guess I should merge patch 2 before patch 1, right? No, patch 1 before patch 2. Thanks, --U. Artie > > Cheers, Daniel > > > --- > > drivers/gpu/drm/i915/intel_panel.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > > index f17ada3..f7da913 100644 > > --- a/drivers/gpu/drm/i915/intel_panel.c > > +++ b/drivers/gpu/drm/i915/intel_panel.c > > @@ -398,6 +398,9 @@ intel_panel_detect(struct drm_device *dev) > > } > > } > > > > +#define DIV_ROUND_CLOSEST_ULL(ll, d) \ > > +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; }) > > + > > /** > > * scale - scale values from one range to another > > * > > @@ -419,9 +422,8 @@ static uint32_t scale(uint32_t source_val, > > source_val = clamp(source_val, source_min, source_max); > > > > /* avoid overflows */ > > - target_val = (uint64_t)(source_val - source_min) * > > - (target_max - target_min); > > - do_div(target_val, source_max - source_min); > > + target_val = DIV_ROUND_CLOSEST_ULL((uint64_t)(source_val - source_min) * > > + (target_max - target_min), source_max - source_min); > > target_val += target_min; > > > > return target_val; > > -- > > 1.9.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Tue, Sep 30, 2014 at 02:58:54PM +0000, Eoff, Ullysses A wrote: > On Tue, 2014-09-30 at 10:04 +0200, Daniel Vetter wrote: > > On Mon, Sep 29, 2014 at 03:49:32PM -0700, U. Artie Eoff wrote: > > > Improper truncated integer division in the scale() function causes > > > actual_brightness != brightness. This (partial) work-around should be > > > sufficient for a majority of use-cases, but it is by no means a complete > > > solution. > > > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > > vice-versa, when the ranges are of different sizes. That would be a > > > buggy scenario even with this work-around. > > > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > > macro > > > > > > v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani) > > > -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien) > > > -v1 and v2 originally authored by Joe Konno. > > > > > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > > > > Is there some bug report, internal jira, mailing list reference or similar > > about this? > > > > Note that at least for OTC jira tasks we now want them to be added to > > commit message with e.g. > > > > OTC-Jria: VIZ-4932 > > > > Yes, the OTC-Jira task is: VIZ-4395. I'll resubmit with amended commit > message. > > > And I guess I should merge patch 2 before patch 1, right? > > No, patch 1 before patch 2. Oh, I didn't notice that your first add a duplicated version of the macro and then unify it. That's a bit backwards ... -Daniel
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Tuesday, September 30, 2014 9:32 AM > To: Eoff, Ullysses A > Cc: daniel@ffwll.ch; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA > > On Tue, Sep 30, 2014 at 02:58:54PM +0000, Eoff, Ullysses A wrote: > > On Tue, 2014-09-30 at 10:04 +0200, Daniel Vetter wrote: > > > On Mon, Sep 29, 2014 at 03:49:32PM -0700, U. Artie Eoff wrote: > > > > Improper truncated integer division in the scale() function causes > > > > actual_brightness != brightness. This (partial) work-around should be > > > > sufficient for a majority of use-cases, but it is by no means a complete > > > > solution. > > > > > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > > > vice-versa, when the ranges are of different sizes. That would be a > > > > buggy scenario even with this work-around. > > > > > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > > > macro > > > > > > > > v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani) > > > > -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien) > > > > -v1 and v2 originally authored by Joe Konno. > > > > > > > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > > > > > > Is there some bug report, internal jira, mailing list reference or similar > > > about this? > > > > > > Note that at least for OTC jira tasks we now want them to be added to > > > commit message with e.g. > > > > > > OTC-Jria: VIZ-4932 > > > > > > > Yes, the OTC-Jira task is: VIZ-4395. I'll resubmit with amended commit > > message. > > > > > And I guess I should merge patch 2 before patch 1, right? > > > > No, patch 1 before patch 2. > > Oh, I didn't notice that your first add a duplicated version of the macro > and then unify it. That's a bit backwards ... The reason for doing it this way is so we can cherry-pick patch 1 into the chromeos 3.10 kernel cleanly (with some other backlight patches). That is, patch 1 is decoupled from the changes in intel_display.c. Hope that makes sense. U. Artie > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Sep 30, 2014 at 04:52:43PM +0000, Eoff, Ullysses A wrote: > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > > Sent: Tuesday, September 30, 2014 9:32 AM > > To: Eoff, Ullysses A > > Cc: daniel@ffwll.ch; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA > > > > On Tue, Sep 30, 2014 at 02:58:54PM +0000, Eoff, Ullysses A wrote: > > > On Tue, 2014-09-30 at 10:04 +0200, Daniel Vetter wrote: > > > > On Mon, Sep 29, 2014 at 03:49:32PM -0700, U. Artie Eoff wrote: > > > > > Improper truncated integer division in the scale() function causes > > > > > actual_brightness != brightness. This (partial) work-around should be > > > > > sufficient for a majority of use-cases, but it is by no means a complete > > > > > solution. > > > > > > > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > > > > vice-versa, when the ranges are of different sizes. That would be a > > > > > buggy scenario even with this work-around. > > > > > > > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > > > > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > > > > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > > > > macro > > > > > > > > > > v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani) > > > > > -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien) > > > > > -v1 and v2 originally authored by Joe Konno. > > > > > > > > > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > > > > > > > > Is there some bug report, internal jira, mailing list reference or similar > > > > about this? > > > > > > > > Note that at least for OTC jira tasks we now want them to be added to > > > > commit message with e.g. > > > > > > > > OTC-Jria: VIZ-4932 > > > > > > > > > > Yes, the OTC-Jira task is: VIZ-4395. I'll resubmit with amended commit > > > message. > > > > > > > And I guess I should merge patch 2 before patch 1, right? > > > > > > No, patch 1 before patch 2. > > > > Oh, I didn't notice that your first add a duplicated version of the macro > > and then unify it. That's a bit backwards ... > > The reason for doing it this way is so we can cherry-pick patch 1 into the > chromeos 3.10 kernel cleanly (with some other backlight patches). > That is, patch 1 is decoupled from the changes in intel_display.c. > Hope that makes sense. Yeah. Would be good to mention that in the commit message, e.g. "For easier backporting Duplicate the division macro here and refactor the code in a follow-up patch". I'll add that when merging if you don't resend. -Daniel
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Tuesday, September 30, 2014 10:23 AM > To: Eoff, Ullysses A > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA > > On Tue, Sep 30, 2014 at 04:52:43PM +0000, Eoff, Ullysses A wrote: > > > -----Original Message----- > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > > > Sent: Tuesday, September 30, 2014 9:32 AM > > > To: Eoff, Ullysses A > > > Cc: daniel@ffwll.ch; intel-gfx@lists.freedesktop.org > > > Subject: Re: [Intel-gfx] [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA > > > > > > On Tue, Sep 30, 2014 at 02:58:54PM +0000, Eoff, Ullysses A wrote: > > > > On Tue, 2014-09-30 at 10:04 +0200, Daniel Vetter wrote: > > > > > On Mon, Sep 29, 2014 at 03:49:32PM -0700, U. Artie Eoff wrote: > > > > > > Improper truncated integer division in the scale() function causes > > > > > > actual_brightness != brightness. This (partial) work-around should be > > > > > > sufficient for a majority of use-cases, but it is by no means a complete > > > > > > solution. > > > > > > > > > > > > TODO: Determine how best to scale "user" values to "hw" values, and > > > > > > vice-versa, when the ranges are of different sizes. That would be a > > > > > > buggy scenario even with this work-around. > > > > > > > > > > > > The issue was introduced in the following (v3.17-rc1) commit: > > > > > > > > > > > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > > > > > > > > > > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > > > > > > macro > > > > > > > > > > > > v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani) > > > > > > -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien) > > > > > > -v1 and v2 originally authored by Joe Konno. > > > > > > > > > > > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > > > > > > > > > > Is there some bug report, internal jira, mailing list reference or similar > > > > > about this? > > > > > > > > > > Note that at least for OTC jira tasks we now want them to be added to > > > > > commit message with e.g. > > > > > > > > > > OTC-Jria: VIZ-4932 > > > > > > > > > > > > > Yes, the OTC-Jira task is: VIZ-4395. I'll resubmit with amended commit > > > > message. > > > > > > > > > And I guess I should merge patch 2 before patch 1, right? > > > > > > > > No, patch 1 before patch 2. > > > > > > Oh, I didn't notice that your first add a duplicated version of the macro > > > and then unify it. That's a bit backwards ... > > > > The reason for doing it this way is so we can cherry-pick patch 1 into the > > chromeos 3.10 kernel cleanly (with some other backlight patches). > > That is, patch 1 is decoupled from the changes in intel_display.c. > > Hope that makes sense. > > Yeah. Would be good to mention that in the commit message, e.g. "For > easier backporting Duplicate the division macro here and refactor the code > in a follow-up patch". I'll add that when merging if you don't resend. Agreed. I'll let you amend the message while merging if you don't mind. I'll get the process right in another round ;-) Thanks for your support! U. Artie > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Reviewed-By: Joe Konno <joe.konno@intel.com> On 09/29/2014 03:49 PM, U. Artie Eoff wrote: > Improper truncated integer division in the scale() function causes > actual_brightness != brightness. This (partial) work-around should be > sufficient for a majority of use-cases, but it is by no means a complete > solution. > > TODO: Determine how best to scale "user" values to "hw" values, and > vice-versa, when the ranges are of different sizes. That would be a > buggy scenario even with this work-around. > > The issue was introduced in the following (v3.17-rc1) commit: > > 6dda730 drm/i915: respect the VBT minimum backlight brightness > > v2: (thanks to Chris Wilson) clarify commit message, use rounded division > macro > > v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani) > -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien) > -v1 and v2 originally authored by Joe Konno. > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > --- > drivers/gpu/drm/i915/intel_panel.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index f17ada3..f7da913 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -398,6 +398,9 @@ intel_panel_detect(struct drm_device *dev) > } > } > > +#define DIV_ROUND_CLOSEST_ULL(ll, d) \ > +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; }) > + > /** > * scale - scale values from one range to another > * > @@ -419,9 +422,8 @@ static uint32_t scale(uint32_t source_val, > source_val = clamp(source_val, source_min, source_max); > > /* avoid overflows */ > - target_val = (uint64_t)(source_val - source_min) * > - (target_max - target_min); > - do_div(target_val, source_max - source_min); > + target_val = DIV_ROUND_CLOSEST_ULL((uint64_t)(source_val - source_min) * > + (target_max - target_min), source_max - source_min); > target_val += target_min; > > return target_val;
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index f17ada3..f7da913 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -398,6 +398,9 @@ intel_panel_detect(struct drm_device *dev) } } +#define DIV_ROUND_CLOSEST_ULL(ll, d) \ +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; }) + /** * scale - scale values from one range to another * @@ -419,9 +422,8 @@ static uint32_t scale(uint32_t source_val, source_val = clamp(source_val, source_min, source_max); /* avoid overflows */ - target_val = (uint64_t)(source_val - source_min) * - (target_max - target_min); - do_div(target_val, source_max - source_min); + target_val = DIV_ROUND_CLOSEST_ULL((uint64_t)(source_val - source_min) * + (target_max - target_min), source_max - source_min); target_val += target_min; return target_val;
Improper truncated integer division in the scale() function causes actual_brightness != brightness. This (partial) work-around should be sufficient for a majority of use-cases, but it is by no means a complete solution. TODO: Determine how best to scale "user" values to "hw" values, and vice-versa, when the ranges are of different sizes. That would be a buggy scenario even with this work-around. The issue was introduced in the following (v3.17-rc1) commit: 6dda730 drm/i915: respect the VBT minimum backlight brightness v2: (thanks to Chris Wilson) clarify commit message, use rounded division macro v3: -DIV_ROUND_CLOSEST() fails to build with CONFIG_X86_32=y. (Jani) -Use DIV_ROUND_CLOSEST_ULL() instead. (Damien) -v1 and v2 originally authored by Joe Konno. Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> --- drivers/gpu/drm/i915/intel_panel.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)