Re: [PATCH] drm/i915: rc6 residency (fix the fix)

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Ben Widawsky]

[-- Attachment #1.1: Type: text/plain, Size: 883 bytes --]

Google fail.  I'd like to go back to my 32b machine and try to reproduce this.  your patch seems fine to me though. 

-------- Original message --------
Subject: Re: [PATCH] drm/i915: rc6 residency (fix the fix) 
From: Chris Wilson <chris@chris-wilson.co.uk> 
To: Ben Widawsky <ben@bwidawsk.net> 
CC: intel-gfx@lists.freedesktop.org,Ben Widawsky <benjamin.widawsky@intel.com> 

On Sun, 22 Apr 2012 11:02:32 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Does div_round64 work for you? I think that is the best solution.

Only exists in drivers/cpuidle/governors/menu.c, and still would need a
do_div to avoid the subsequent 64-bit division.

The choice appears to be either perform the divide by 1000 using do_div,
or note that a u32 * 128 / 1000 is also a u32 and so cast the result of
DIV_ROUND_UP_ULL back to u32.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #1.2: Type: text/html, Size: 1134 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Daniel Vetter]

On Sun, Apr 22, 2012 at 07:57:33PM -0700, Ben Widawsky wrote:
> On Sun, 22 Apr 2012 18:39:23 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Sun, 22 Apr 2012 10:35:29 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > On Sun, 22 Apr 2012 16:49:53 +0100
> > > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > 
> > > > On Fri, 20 Apr 2012 11:50:01 -0700, Ben Widawsky
> > > > <ben@bwidawsk.net> wrote:
> > > > > Chris' fix for my 32b breakage was incorrect. do_div returns a
> > > > > remainder. Go back to a divide macro which is more 32b friendly.
> > > > > 
> > > > > Tested on x86-64.
> > > > > 
> > > > > This has only been compile tested on 32b systems.
> > > > Doesn't compile on my 32-bit systems.
> > > > 
> > > > Still does a 64-bit divide.
> > > 
> > > This is weird. I don't understand what's broken exactly. It should
> > > be dividing an unsigned long long, which the 32b compiler should
> > > have no issue with. The macro itself expands to do_div which was
> > > your original fix.
> > > 
> > > What is the error message you get?
> > Undefined reference to __udivid3.
> > 
> > The reason is that the result of DIV_ROUND_UP_ULL is a 64-bit value,
> > which we then proceed to divide by 100.
> > -Chris
> > 
> 
> For the sake of picking nits, this is a linker fail, not a compiler one.
> The only reason I make note of it is I was only compiling the .ko when
> I did my testing.
> 
> So lesson learned, can't we just:

Done.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Ben Widawsky]

On Sun, 22 Apr 2012 18:39:23 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Sun, 22 Apr 2012 10:35:29 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > On Sun, 22 Apr 2012 16:49:53 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > On Fri, 20 Apr 2012 11:50:01 -0700, Ben Widawsky
> > > <ben@bwidawsk.net> wrote:
> > > > Chris' fix for my 32b breakage was incorrect. do_div returns a
> > > > remainder. Go back to a divide macro which is more 32b friendly.
> > > > 
> > > > Tested on x86-64.
> > > > 
> > > > This has only been compile tested on 32b systems.
> > > Doesn't compile on my 32-bit systems.
> > > 
> > > Still does a 64-bit divide.
> > 
> > This is weird. I don't understand what's broken exactly. It should
> > be dividing an unsigned long long, which the 32b compiler should
> > have no issue with. The macro itself expands to do_div which was
> > your original fix.
> > 
> > What is the error message you get?
> Undefined reference to __udivid3.
> 
> The reason is that the result of DIV_ROUND_UP_ULL is a 64-bit value,
> which we then proceed to divide by 100.
> -Chris
> 

For the sake of picking nits, this is a linker fail, not a compiler one.
The only reason I make note of it is I was only compiling the .ko when
I did my testing.

So lesson learned, can't we just:

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index bd6350b..79f8344 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -40,7 +40,7 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
                return 0;
 
        raw_time = I915_READ(reg) * 128ULL;
-       return DIV_ROUND_UP_ULL(raw_time, 1000) / 100;
+       return DIV_ROUND_UP_ULL(raw_time, 100000);
 }
 
 static ssize_t

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Chris Wilson]

On Sun, 22 Apr 2012 11:02:32 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Does div_round64 work for you? I think that is the best solution.

Only exists in drivers/cpuidle/governors/menu.c, and still would need a
do_div to avoid the subsequent 64-bit division.

The choice appears to be either perform the divide by 1000 using do_div,
or note that a u32 * 128 / 1000 is also a u32 and so cast the result of
DIV_ROUND_UP_ULL back to u32.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Ben Widawsky]

On Sun, 22 Apr 2012 18:39:23 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Sun, 22 Apr 2012 10:35:29 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > On Sun, 22 Apr 2012 16:49:53 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > On Fri, 20 Apr 2012 11:50:01 -0700, Ben Widawsky
> > > <ben@bwidawsk.net> wrote:
> > > > Chris' fix for my 32b breakage was incorrect. do_div returns a
> > > > remainder. Go back to a divide macro which is more 32b friendly.
> > > > 
> > > > Tested on x86-64.
> > > > 
> > > > This has only been compile tested on 32b systems.
> > > Doesn't compile on my 32-bit systems.
> > > 
> > > Still does a 64-bit divide.
> > 
> > This is weird. I don't understand what's broken exactly. It should
> > be dividing an unsigned long long, which the 32b compiler should
> > have no issue with. The macro itself expands to do_div which was
> > your original fix.
> > 
> > What is the error message you get?
> Undefined reference to __udivid3.
> 
> The reason is that the result of DIV_ROUND_UP_ULL is a 64-bit value,
> which we then proceed to divide by 100.
> -Chris
> 

Does div_round64 work for you? I think that is the best solution.

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Chris Wilson]

On Sun, 22 Apr 2012 10:35:29 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Sun, 22 Apr 2012 16:49:53 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Fri, 20 Apr 2012 11:50:01 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > Chris' fix for my 32b breakage was incorrect. do_div returns a
> > > remainder. Go back to a divide macro which is more 32b friendly.
> > > 
> > > Tested on x86-64.
> > > 
> > > This has only been compile tested on 32b systems.
> > Doesn't compile on my 32-bit systems.
> > 
> > Still does a 64-bit divide.
> 
> This is weird. I don't understand what's broken exactly. It should be
> dividing an unsigned long long, which the 32b compiler should have no
> issue with. The macro itself expands to do_div which was your original
> fix.
> 
> What is the error message you get?
Undefined reference to __udivid3.

The reason is that the result of DIV_ROUND_UP_ULL is a 64-bit value,
which we then proceed to divide by 100.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Ben Widawsky]

On Sun, 22 Apr 2012 16:49:53 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 20 Apr 2012 11:50:01 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > Chris' fix for my 32b breakage was incorrect. do_div returns a
> > remainder. Go back to a divide macro which is more 32b friendly.
> > 
> > Tested on x86-64.
> > 
> > This has only been compile tested on 32b systems.
> Doesn't compile on my 32-bit systems.
> 
> Still does a 64-bit divide.

This is weird. I don't understand what's broken exactly. It should be
dividing an unsigned long long, which the 32b compiler should have no
issue with. The macro itself expands to do_div which was your original
fix.

What is the error message you get?

>  
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48756
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_sysfs.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> > b/drivers/gpu/drm/i915/i915_sysfs.c index f1b5108..bd6350b 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -39,8 +39,8 @@ static u32 calc_residency(struct drm_device *dev,
> > const u32 reg) if (!intel_enable_rc6(dev))
> >  		return 0;
> >  
> > -	raw_time = I915_READ(reg) * 128ULL + 500;
> > -	return do_div(raw_time, 100000);
> > +	raw_time = I915_READ(reg) * 128ULL;
> > +	return DIV_ROUND_UP_ULL(raw_time, 1000) / 100;
> return (u32)DIV_ROUND_UP_ULL(raw_time, 1000) / 100;
> -Chris
> 

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Chris Wilson]

On Fri, 20 Apr 2012 11:50:01 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Chris' fix for my 32b breakage was incorrect. do_div returns a
> remainder. Go back to a divide macro which is more 32b friendly.
> 
> Tested on x86-64.
> 
> This has only been compile tested on 32b systems.
Doesn't compile on my 32-bit systems.

Still does a 64-bit divide.
 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48756
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_sysfs.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index f1b5108..bd6350b 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -39,8 +39,8 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
>  	if (!intel_enable_rc6(dev))
>  		return 0;
>  
> -	raw_time = I915_READ(reg) * 128ULL + 500;
> -	return do_div(raw_time, 100000);
> +	raw_time = I915_READ(reg) * 128ULL;
> +	return DIV_ROUND_UP_ULL(raw_time, 1000) / 100;
return (u32)DIV_ROUND_UP_ULL(raw_time, 1000) / 100;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Daniel Vetter]

On Fri, Apr 20, 2012 at 08:24:04PM +0100, Chris Wilson wrote:
> On Fri, 20 Apr 2012 11:50:01 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Chris' fix for my 32b breakage was incorrect. do_div returns a
> > remainder. Go back to a divide macro which is more 32b friendly.
> > 
> > Tested on x86-64.
> > 
> > This has only been compile tested on 32b systems.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48756
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> Sincere-apologies: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Had I looked for the equivalent 64bit DIV_ROUND_UP, I could have saved
> myself some embarrassment.
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: rc6 residency (fix the fix)

[Chris Wilson]

On Fri, 20 Apr 2012 11:50:01 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Chris' fix for my 32b breakage was incorrect. do_div returns a
> remainder. Go back to a divide macro which is more 32b friendly.
> 
> Tested on x86-64.
> 
> This has only been compile tested on 32b systems.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48756
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
Sincere-apologies: Chris Wilson <chris@chris-wilson.co.uk>

Had I looked for the equivalent 64bit DIV_ROUND_UP, I could have saved
myself some embarrassment.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: rc6 residency (fix the fix)

[Ben Widawsky]

Chris' fix for my 32b breakage was incorrect. do_div returns a
remainder. Go back to a divide macro which is more 32b friendly.

Tested on x86-64.

This has only been compile tested on 32b systems.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48756
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_sysfs.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index f1b5108..bd6350b 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -39,8 +39,8 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg)
 	if (!intel_enable_rc6(dev))
 		return 0;
 
-	raw_time = I915_READ(reg) * 128ULL + 500;
-	return do_div(raw_time, 100000);
+	raw_time = I915_READ(reg) * 128ULL;
+	return DIV_ROUND_UP_ULL(raw_time, 1000) / 100;
 }
 
 static ssize_t
-- 
1.7.10