* [PATCH] drm/i915: rc6 residency (fix the fix)
@ 2012-04-20 18:50 Ben Widawsky
2012-04-20 19:24 ` Chris Wilson
2012-04-22 15:49 ` Chris Wilson
0 siblings, 2 replies; 11+ messages in thread
From: Ben Widawsky @ 2012-04-20 18:50 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-20 18:50 [PATCH] drm/i915: rc6 residency (fix the fix) Ben Widawsky
@ 2012-04-20 19:24 ` Chris Wilson
2012-04-20 19:27 ` Daniel Vetter
2012-04-22 15:49 ` Chris Wilson
1 sibling, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2012-04-20 19:24 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky, Ben Widawsky
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-20 19:24 ` Chris Wilson
@ 2012-04-20 19:27 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2012-04-20 19:27 UTC (permalink / raw)
To: Chris Wilson; +Cc: Ben Widawsky, intel-gfx, Ben Widawsky
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-20 18:50 [PATCH] drm/i915: rc6 residency (fix the fix) Ben Widawsky
2012-04-20 19:24 ` Chris Wilson
@ 2012-04-22 15:49 ` Chris Wilson
2012-04-22 17:35 ` Ben Widawsky
1 sibling, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2012-04-22 15:49 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky, Ben Widawsky
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-22 15:49 ` Chris Wilson
@ 2012-04-22 17:35 ` Ben Widawsky
2012-04-22 17:39 ` Chris Wilson
0 siblings, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2012-04-22 17:35 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-22 17:35 ` Ben Widawsky
@ 2012-04-22 17:39 ` Chris Wilson
2012-04-22 18:02 ` Ben Widawsky
2012-04-23 2:57 ` Ben Widawsky
0 siblings, 2 replies; 11+ messages in thread
From: Chris Wilson @ 2012-04-22 17:39 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Ben Widawsky
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-22 17:39 ` Chris Wilson
@ 2012-04-22 18:02 ` Ben Widawsky
2012-04-22 18:11 ` Chris Wilson
2012-04-23 2:57 ` Ben Widawsky
1 sibling, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2012-04-22 18:02 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-22 18:02 ` Ben Widawsky
@ 2012-04-22 18:11 ` Chris Wilson
0 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2012-04-22 18:11 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, Ben Widawsky
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-22 17:39 ` Chris Wilson
2012-04-22 18:02 ` Ben Widawsky
@ 2012-04-23 2:57 ` Ben Widawsky
2012-04-23 7:38 ` Daniel Vetter
1 sibling, 1 reply; 11+ messages in thread
From: Ben Widawsky @ 2012-04-23 2:57 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
2012-04-23 2:57 ` Ben Widawsky
@ 2012-04-23 7:38 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2012-04-23 7:38 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: rc6 residency (fix the fix)
@ 2012-04-22 18:27 Ben Widawsky
0 siblings, 0 replies; 11+ messages in thread
From: Ben Widawsky @ 2012-04-22 18:27 UTC (permalink / raw)
To: chris; +Cc: intel-gfx, benjamin.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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-04-23 7:37 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-20 18:50 [PATCH] drm/i915: rc6 residency (fix the fix) Ben Widawsky
2012-04-20 19:24 ` Chris Wilson
2012-04-20 19:27 ` Daniel Vetter
2012-04-22 15:49 ` Chris Wilson
2012-04-22 17:35 ` Ben Widawsky
2012-04-22 17:39 ` Chris Wilson
2012-04-22 18:02 ` Ben Widawsky
2012-04-22 18:11 ` Chris Wilson
2012-04-23 2:57 ` Ben Widawsky
2012-04-23 7:38 ` Daniel Vetter
-- strict thread matches above, loose matches on Subject: below --
2012-04-22 18:27 Ben Widawsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox