* [PATCH 0/2] drm/i915: WA intel_backlight scale math @ 2014-09-29 22:49 U. Artie Eoff 2014-09-29 22:49 ` [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA U. Artie Eoff 2014-09-29 22:49 ` [PATCH 2/2] drm/i915: Move DIV_ROUND_CLOSEST_ULL macro to header U. Artie Eoff 0 siblings, 2 replies; 11+ messages in thread From: U. Artie Eoff @ 2014-09-29 22:49 UTC (permalink / raw) To: intel-gfx The first patch is version 3 of the original patch authored by Joe Konno. Joe Konno is off-the-grid for the next week or so. Thus, I'm submitting version 3 to keep the momentum going on this. I elected to split v3 up into two patches so that the first can be cherry-picked easier (i.e. out of context of intel_display.c). U. Artie Eoff (2): drm/i915: intel_backlight scale() math WA drm/i915: Move DIV_ROUND_CLOSEST_ULL macro to header drivers/gpu/drm/i915/intel_display.c | 3 --- drivers/gpu/drm/i915/intel_drv.h | 3 +++ drivers/gpu/drm/i915/intel_panel.c | 5 ++--- 3 files changed, 5 insertions(+), 6 deletions(-) -- 1.9.3 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA 2014-09-29 22:49 [PATCH 0/2] drm/i915: WA intel_backlight scale math U. Artie Eoff @ 2014-09-29 22:49 ` U. Artie Eoff 2014-09-30 8:04 ` Daniel Vetter 2014-10-06 14:14 ` Joe Konno 2014-09-29 22:49 ` [PATCH 2/2] drm/i915: Move DIV_ROUND_CLOSEST_ULL macro to header U. Artie Eoff 1 sibling, 2 replies; 11+ messages in thread From: U. Artie Eoff @ 2014-09-29 22:49 UTC (permalink / raw) To: intel-gfx 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; -- 1.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA 2014-09-29 22:49 ` [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA U. Artie Eoff @ 2014-09-30 8:04 ` Daniel Vetter 2014-09-30 14:58 ` Eoff, Ullysses A 2014-10-06 14:14 ` Joe Konno 1 sibling, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2014-09-30 8:04 UTC (permalink / raw) To: U. Artie Eoff; +Cc: intel-gfx 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA 2014-09-30 8:04 ` Daniel Vetter @ 2014-09-30 14:58 ` Eoff, Ullysses A 2014-09-30 16:31 ` Daniel Vetter 0 siblings, 1 reply; 11+ messages in thread From: Eoff, Ullysses A @ 2014-09-30 14:58 UTC (permalink / raw) To: daniel@ffwll.ch; +Cc: intel-gfx@lists.freedesktop.org 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA 2014-09-30 14:58 ` Eoff, Ullysses A @ 2014-09-30 16:31 ` Daniel Vetter 2014-09-30 16:52 ` Eoff, Ullysses A 0 siblings, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2014-09-30 16:31 UTC (permalink / raw) To: Eoff, Ullysses A; +Cc: intel-gfx@lists.freedesktop.org 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA 2014-09-30 16:31 ` Daniel Vetter @ 2014-09-30 16:52 ` Eoff, Ullysses A 2014-09-30 17:23 ` Daniel Vetter 0 siblings, 1 reply; 11+ messages in thread From: Eoff, Ullysses A @ 2014-09-30 16:52 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org > -----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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA 2014-09-30 16:52 ` Eoff, Ullysses A @ 2014-09-30 17:23 ` Daniel Vetter 2014-09-30 21:18 ` Eoff, Ullysses A 0 siblings, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2014-09-30 17:23 UTC (permalink / raw) To: Eoff, Ullysses A; +Cc: intel-gfx@lists.freedesktop.org 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA 2014-09-30 17:23 ` Daniel Vetter @ 2014-09-30 21:18 ` Eoff, Ullysses A 0 siblings, 0 replies; 11+ messages in thread From: Eoff, Ullysses A @ 2014-09-30 21:18 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org > -----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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA 2014-09-29 22:49 ` [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA U. Artie Eoff 2014-09-30 8:04 ` Daniel Vetter @ 2014-10-06 14:14 ` Joe Konno 1 sibling, 0 replies; 11+ messages in thread From: Joe Konno @ 2014-10-06 14:14 UTC (permalink / raw) To: intel-gfx 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/i915: Move DIV_ROUND_CLOSEST_ULL macro to header 2014-09-29 22:49 [PATCH 0/2] drm/i915: WA intel_backlight scale math U. Artie Eoff 2014-09-29 22:49 ` [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA U. Artie Eoff @ 2014-09-29 22:49 ` U. Artie Eoff 2014-10-06 14:15 ` Joe Konno 1 sibling, 1 reply; 11+ messages in thread From: U. Artie Eoff @ 2014-09-29 22:49 UTC (permalink / raw) To: intel-gfx Move the duplicated DIV_ROUND_CLOSEST_ULL macro into the intel_drv.h header file so that it can be shared between intel_display.c and intel_panel.c. Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 3 --- drivers/gpu/drm/i915/intel_drv.h | 3 +++ drivers/gpu/drm/i915/intel_panel.c | 3 --- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5b05ddb..db800f2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -73,9 +73,6 @@ static const uint32_t intel_cursor_formats[] = { DRM_FORMAT_ARGB8888, }; -#define DIV_ROUND_CLOSEST_ULL(ll, d) \ -({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; }) - static void intel_increase_pllclock(struct drm_device *dev, enum pipe pipe); static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1b72c15..8401ae3 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -36,6 +36,9 @@ #include <drm/drm_dp_mst_helper.h> #include <drm/drm_rect.h> +#define DIV_ROUND_CLOSEST_ULL(ll, d) \ +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; }) + /** * _wait_for - magic (register) wait macro * diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c index f7da913..fade0dd 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -398,9 +398,6 @@ 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 * -- 1.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/i915: Move DIV_ROUND_CLOSEST_ULL macro to header 2014-09-29 22:49 ` [PATCH 2/2] drm/i915: Move DIV_ROUND_CLOSEST_ULL macro to header U. Artie Eoff @ 2014-10-06 14:15 ` Joe Konno 0 siblings, 0 replies; 11+ messages in thread From: Joe Konno @ 2014-10-06 14:15 UTC (permalink / raw) To: intel-gfx Reviewed-By: Joe Konno <joe.konno@intel.com> On 09/29/2014 03:49 PM, U. Artie Eoff wrote: > Move the duplicated DIV_ROUND_CLOSEST_ULL macro into the intel_drv.h > header file so that it can be shared between intel_display.c > and intel_panel.c. > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 3 --- > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > drivers/gpu/drm/i915/intel_panel.c | 3 --- > 3 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 5b05ddb..db800f2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -73,9 +73,6 @@ static const uint32_t intel_cursor_formats[] = { > DRM_FORMAT_ARGB8888, > }; > > -#define DIV_ROUND_CLOSEST_ULL(ll, d) \ > -({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; }) > - > static void intel_increase_pllclock(struct drm_device *dev, > enum pipe pipe); > static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 1b72c15..8401ae3 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -36,6 +36,9 @@ > #include <drm/drm_dp_mst_helper.h> > #include <drm/drm_rect.h> > > +#define DIV_ROUND_CLOSEST_ULL(ll, d) \ > +({ unsigned long long _tmp = (ll)+(d)/2; do_div(_tmp, d); _tmp; }) > + > /** > * _wait_for - magic (register) wait macro > * > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c > index f7da913..fade0dd 100644 > --- a/drivers/gpu/drm/i915/intel_panel.c > +++ b/drivers/gpu/drm/i915/intel_panel.c > @@ -398,9 +398,6 @@ 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 > * ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-10-06 14:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-29 22:49 [PATCH 0/2] drm/i915: WA intel_backlight scale math U. Artie Eoff 2014-09-29 22:49 ` [PATCH 1/2 v3] drm/i915: intel_backlight scale() math WA U. Artie Eoff 2014-09-30 8:04 ` Daniel Vetter 2014-09-30 14:58 ` Eoff, Ullysses A 2014-09-30 16:31 ` Daniel Vetter 2014-09-30 16:52 ` Eoff, Ullysses A 2014-09-30 17:23 ` Daniel Vetter 2014-09-30 21:18 ` Eoff, Ullysses A 2014-10-06 14:14 ` Joe Konno 2014-09-29 22:49 ` [PATCH 2/2] drm/i915: Move DIV_ROUND_CLOSEST_ULL macro to header U. Artie Eoff 2014-10-06 14:15 ` Joe Konno
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox