public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [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
* [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 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
* 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