* [PATCH 0/3] Some forcewake fixes
@ 2012-08-24 19:31 Ben Widawsky
2012-08-24 19:31 ` [PATCH 1/3] drm/i915: Extract forcewake ack timeout Ben Widawsky
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Ben Widawsky @ 2012-08-24 19:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky, Matthew Garret
I have no proof that any of these are relevant other than a designer in the
know told me we should be doing this. Furthermore if Mathew's hang is truly on
the very first read to the FORCEWAKE_ACK, these patches almost surely will
*not* help :-(
Ben Widawsky (3):
drm/i915: Extract forcewake ack timeout
drm/i915: Change forcewake timeout to 2ms
drm/i915: Never read FORCEWAKE
drivers/gpu/drm/i915/intel_pm.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
--
1.7.12
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] drm/i915: Extract forcewake ack timeout 2012-08-24 19:31 [PATCH 0/3] Some forcewake fixes Ben Widawsky @ 2012-08-24 19:31 ` Ben Widawsky 2012-08-24 19:31 ` [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms Ben Widawsky 2012-08-24 19:31 ` [PATCH 3/3] drm/i915: Never read FORCEWAKE Ben Widawsky 2 siblings, 0 replies; 10+ messages in thread From: Ben Widawsky @ 2012-08-24 19:31 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky It's used all over the place, and we want to be able to play around with the value, apparently. Note that it doesn't touch other timeouts of the same value (like gtfifo, and thread C0 wait). Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/intel_pm.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c0721ff..f42c142 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -31,6 +31,8 @@ #include "../../../platform/x86/intel_ips.h" #include <linux/module.h> +#define FORCEWAKE_ACK_TIMEOUT_US 500 + /* FBC, or Frame Buffer Compression, is a technique employed to compress the * framebuffer contents in-memory, aiming at reducing the required bandwidth * during in-memory transfers and, therefore, reduce the power packet. @@ -3968,13 +3970,15 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) else forcewake_ack = FORCEWAKE_ACK; - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, 500)) + if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, + FORCEWAKE_ACK_TIMEOUT_US)) DRM_ERROR("Force wake wait timed out\n"); I915_WRITE_NOTRACE(FORCEWAKE, 1); POSTING_READ(FORCEWAKE); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), 500)) + if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), + FORCEWAKE_ACK_TIMEOUT_US)) DRM_ERROR("Force wake wait timed out\n"); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -3989,13 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) else forcewake_ack = FORCEWAKE_MT_ACK; - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, 500)) + if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, + FORCEWAKE_ACK_TIMEOUT_US)) DRM_ERROR("Force wake wait timed out\n"); I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); POSTING_READ(FORCEWAKE_MT); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), 500)) + if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), + FORCEWAKE_ACK_TIMEOUT_US)) DRM_ERROR("Force wake wait timed out\n"); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -4082,7 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv) I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff); POSTING_READ(FORCEWAKE_VLV); - if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), 500)) + if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), + FORCEWAKE_ACK_TIMEOUT_US)) DRM_ERROR("Force wake wait timed out\n"); __gen6_gt_wait_for_thread_c0(dev_priv); -- 1.7.12 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms 2012-08-24 19:31 [PATCH 0/3] Some forcewake fixes Ben Widawsky 2012-08-24 19:31 ` [PATCH 1/3] drm/i915: Extract forcewake ack timeout Ben Widawsky @ 2012-08-24 19:31 ` Ben Widawsky 2012-08-27 6:59 ` Jani Nikula 2012-08-24 19:31 ` [PATCH 3/3] drm/i915: Never read FORCEWAKE Ben Widawsky 2 siblings, 1 reply; 10+ messages in thread From: Ben Widawsky @ 2012-08-24 19:31 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky A designer familiar with the hardware has stated that the forcewake timeout can theoretically be as high as a little over 1ms. Therefore we modify our code to use 2ms (appropriate fudge and because we don't want to round down). Hopefully this can't prevent spurious timeouts. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f42c142..2a8468d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -31,7 +31,7 @@ #include "../../../platform/x86/intel_ips.h" #include <linux/module.h> -#define FORCEWAKE_ACK_TIMEOUT_US 500 +#define FORCEWAKE_ACK_TIMEOUT_MS 2 /* FBC, or Frame Buffer Compression, is a technique employed to compress the * framebuffer contents in-memory, aiming at reducing the required bandwidth @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) else forcewake_ack = FORCEWAKE_ACK; - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR("Force wake wait timed out\n"); I915_WRITE_NOTRACE(FORCEWAKE, 1); POSTING_READ(FORCEWAKE); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR("Force wake wait timed out\n"); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -3993,15 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) else forcewake_ack = FORCEWAKE_MT_ACK; - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR("Force wake wait timed out\n"); I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); POSTING_READ(FORCEWAKE_MT); - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR("Force wake wait timed out\n"); __gen6_gt_wait_for_thread_c0(dev_priv); @@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv) I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff); POSTING_READ(FORCEWAKE_VLV); - if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), - FORCEWAKE_ACK_TIMEOUT_US)) + if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), + FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR("Force wake wait timed out\n"); __gen6_gt_wait_for_thread_c0(dev_priv); -- 1.7.12 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms 2012-08-24 19:31 ` [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms Ben Widawsky @ 2012-08-27 6:59 ` Jani Nikula 2012-08-28 15:51 ` Ben Widawsky 0 siblings, 1 reply; 10+ messages in thread From: Jani Nikula @ 2012-08-27 6:59 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, Ben Widawsky On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote: > A designer familiar with the hardware has stated that the forcewake > timeout can theoretically be as high as a little over 1ms. Therefore we > modify our code to use 2ms (appropriate fudge and because we don't want > to round down). > > Hopefully this can't prevent spurious timeouts. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index f42c142..2a8468d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -31,7 +31,7 @@ > #include "../../../platform/x86/intel_ips.h" > #include <linux/module.h> > > -#define FORCEWAKE_ACK_TIMEOUT_US 500 > +#define FORCEWAKE_ACK_TIMEOUT_MS 2 > > /* FBC, or Frame Buffer Compression, is a technique employed to compress the > * framebuffer contents in-memory, aiming at reducing the required bandwidth > @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) > else > forcewake_ack = FORCEWAKE_ACK; > > - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, > - FORCEWAKE_ACK_TIMEOUT_US)) > + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, > + FORCEWAKE_ACK_TIMEOUT_MS)) Superficially this looks okay, but the implementation of wait_for_atomic() not so. As a surprise, this change drops cpu_relax() from the busy loop, even thought the timeout is potentially much longer. The quick fix here would be to just use 2000 us with wait_for_atomic_us(), but we should do something about wait_for_atomic() too. Luckily it's only ever used at one place. BR, Jani. > DRM_ERROR("Force wake wait timed out\n"); > > I915_WRITE_NOTRACE(FORCEWAKE, 1); > POSTING_READ(FORCEWAKE); > > - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), > - FORCEWAKE_ACK_TIMEOUT_US)) > + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), > + FORCEWAKE_ACK_TIMEOUT_MS)) > DRM_ERROR("Force wake wait timed out\n"); > > __gen6_gt_wait_for_thread_c0(dev_priv); > @@ -3993,15 +3993,15 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) > else > forcewake_ack = FORCEWAKE_MT_ACK; > > - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, > - FORCEWAKE_ACK_TIMEOUT_US)) > + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, > + FORCEWAKE_ACK_TIMEOUT_MS)) > DRM_ERROR("Force wake wait timed out\n"); > > I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); > POSTING_READ(FORCEWAKE_MT); > > - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), > - FORCEWAKE_ACK_TIMEOUT_US)) > + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), > + FORCEWAKE_ACK_TIMEOUT_MS)) > DRM_ERROR("Force wake wait timed out\n"); > > __gen6_gt_wait_for_thread_c0(dev_priv); > @@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv) > I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff); > POSTING_READ(FORCEWAKE_VLV); > > - if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), > - FORCEWAKE_ACK_TIMEOUT_US)) > + if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), > + FORCEWAKE_ACK_TIMEOUT_MS)) > DRM_ERROR("Force wake wait timed out\n"); > > __gen6_gt_wait_for_thread_c0(dev_priv); > -- > 1.7.12 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms 2012-08-27 6:59 ` Jani Nikula @ 2012-08-28 15:51 ` Ben Widawsky 2012-08-28 16:00 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Ben Widawsky @ 2012-08-28 15:51 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx On 2012-08-26 23:59, Jani Nikula wrote: > On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote: >> A designer familiar with the hardware has stated that the forcewake >> timeout can theoretically be as high as a little over 1ms. Therefore >> we >> modify our code to use 2ms (appropriate fudge and because we don't >> want >> to round down). >> >> Hopefully this can't prevent spurious timeouts. >> >> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> >> --- >> drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c >> b/drivers/gpu/drm/i915/intel_pm.c >> index f42c142..2a8468d 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -31,7 +31,7 @@ >> #include "../../../platform/x86/intel_ips.h" >> #include <linux/module.h> >> >> -#define FORCEWAKE_ACK_TIMEOUT_US 500 >> +#define FORCEWAKE_ACK_TIMEOUT_MS 2 >> >> /* FBC, or Frame Buffer Compression, is a technique employed to >> compress the >> * framebuffer contents in-memory, aiming at reducing the required >> bandwidth >> @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct >> drm_i915_private *dev_priv) >> else >> forcewake_ack = FORCEWAKE_ACK; >> >> - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == >> 0, >> - FORCEWAKE_ACK_TIMEOUT_US)) >> + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, >> + FORCEWAKE_ACK_TIMEOUT_MS)) > > Superficially this looks okay, but the implementation of > wait_for_atomic() not so. As a surprise, this change drops > cpu_relax() > from the busy loop, even thought the timeout is potentially much > longer. > > The quick fix here would be to just use 2000 us with > wait_for_atomic_us(), but we should do something about > wait_for_atomic() > too. Luckily it's only ever used at one place. > > BR, > Jani. Hmm, dare I say, I think this is a bug in _wait_for. Without spending too much brain power on this, I believe the compiler can screw us over here. No matter the bug, cpu_relax is still probably desirable, unless there is some newer coolness here? I shall insert a patch before this to do the cpu_relax in _wait_for. Nice catch. Ben > > >> DRM_ERROR("Force wake wait timed out\n"); >> >> I915_WRITE_NOTRACE(FORCEWAKE, 1); >> POSTING_READ(FORCEWAKE); >> >> - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), >> - FORCEWAKE_ACK_TIMEOUT_US)) >> + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), >> + FORCEWAKE_ACK_TIMEOUT_MS)) >> DRM_ERROR("Force wake wait timed out\n"); >> >> __gen6_gt_wait_for_thread_c0(dev_priv); >> @@ -3993,15 +3993,15 @@ static void >> __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) >> else >> forcewake_ack = FORCEWAKE_MT_ACK; >> >> - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == >> 0, >> - FORCEWAKE_ACK_TIMEOUT_US)) >> + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, >> + FORCEWAKE_ACK_TIMEOUT_MS)) >> DRM_ERROR("Force wake wait timed out\n"); >> >> I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); >> POSTING_READ(FORCEWAKE_MT); >> >> - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1), >> - FORCEWAKE_ACK_TIMEOUT_US)) >> + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), >> + FORCEWAKE_ACK_TIMEOUT_MS)) >> DRM_ERROR("Force wake wait timed out\n"); >> >> __gen6_gt_wait_for_thread_c0(dev_priv); >> @@ -4088,8 +4088,8 @@ static void vlv_force_wake_get(struct >> drm_i915_private *dev_priv) >> I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff); >> POSTING_READ(FORCEWAKE_VLV); >> >> - if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), >> - FORCEWAKE_ACK_TIMEOUT_US)) >> + if (wait_for_atomic((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), >> + FORCEWAKE_ACK_TIMEOUT_MS)) >> DRM_ERROR("Force wake wait timed out\n"); >> >> __gen6_gt_wait_for_thread_c0(dev_priv); >> -- >> 1.7.12 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms 2012-08-28 15:51 ` Ben Widawsky @ 2012-08-28 16:00 ` Daniel Vetter 2012-08-28 16:07 ` Ben Widawsky 0 siblings, 1 reply; 10+ messages in thread From: Daniel Vetter @ 2012-08-28 16:00 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > On 2012-08-26 23:59, Jani Nikula wrote: >> >> On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote: >>> >>> A designer familiar with the hardware has stated that the forcewake >>> timeout can theoretically be as high as a little over 1ms. Therefore we >>> modify our code to use 2ms (appropriate fudge and because we don't want >>> to round down). >>> >>> Hopefully this can't prevent spurious timeouts. >>> >>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> >>> --- >>> drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++----------- >>> 1 file changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_pm.c >>> b/drivers/gpu/drm/i915/intel_pm.c >>> index f42c142..2a8468d 100644 >>> --- a/drivers/gpu/drm/i915/intel_pm.c >>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>> @@ -31,7 +31,7 @@ >>> #include "../../../platform/x86/intel_ips.h" >>> #include <linux/module.h> >>> >>> -#define FORCEWAKE_ACK_TIMEOUT_US 500 >>> +#define FORCEWAKE_ACK_TIMEOUT_MS 2 >>> >>> /* FBC, or Frame Buffer Compression, is a technique employed to compress >>> the >>> * framebuffer contents in-memory, aiming at reducing the required >>> bandwidth >>> @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct >>> drm_i915_private *dev_priv) >>> else >>> forcewake_ack = FORCEWAKE_ACK; >>> >>> - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) == >>> 0, >>> - FORCEWAKE_ACK_TIMEOUT_US)) >>> + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == 0, >>> + FORCEWAKE_ACK_TIMEOUT_MS)) >> >> >> Superficially this looks okay, but the implementation of >> wait_for_atomic() not so. As a surprise, this change drops cpu_relax() >> from the busy loop, even thought the timeout is potentially much longer. >> >> The quick fix here would be to just use 2000 us with >> wait_for_atomic_us(), but we should do something about wait_for_atomic() >> too. Luckily it's only ever used at one place. >> >> BR, >> Jani. > > > Hmm, dare I say, I think this is a bug in _wait_for. Without spending too > much brain power on this, I believe the compiler can screw us over here. No > matter the bug, cpu_relax is still probably desirable, unless there is some > newer coolness here? I shall insert a patch before this to do the cpu_relax > in _wait_for. The original idea behind wiat_for_us was that we use udelay and really limit ourselves to that us timeout (since jiffies is too coarse). Now that the timeout for forcewake is 2ms (gawk!) I think we can stop bothering to pretend that this should timeout quickly and drop the _us variant (but still keep the cpu relax imo). -Daniel -- Daniel Vetter daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms 2012-08-28 16:00 ` Daniel Vetter @ 2012-08-28 16:07 ` Ben Widawsky 2012-08-28 16:27 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Ben Widawsky @ 2012-08-28 16:07 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On 2012-08-28 09:00, Daniel Vetter wrote: > On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky <ben@bwidawsk.net> > wrote: >> On 2012-08-26 23:59, Jani Nikula wrote: >>> >>> On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote: >>>> >>>> A designer familiar with the hardware has stated that the >>>> forcewake >>>> timeout can theoretically be as high as a little over 1ms. >>>> Therefore we >>>> modify our code to use 2ms (appropriate fudge and because we don't >>>> want >>>> to round down). >>>> >>>> Hopefully this can't prevent spurious timeouts. >>>> >>>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> >>>> --- >>>> drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++----------- >>>> 1 file changed, 11 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c >>>> b/drivers/gpu/drm/i915/intel_pm.c >>>> index f42c142..2a8468d 100644 >>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>> @@ -31,7 +31,7 @@ >>>> #include "../../../platform/x86/intel_ips.h" >>>> #include <linux/module.h> >>>> >>>> -#define FORCEWAKE_ACK_TIMEOUT_US 500 >>>> +#define FORCEWAKE_ACK_TIMEOUT_MS 2 >>>> >>>> /* FBC, or Frame Buffer Compression, is a technique employed to >>>> compress >>>> the >>>> * framebuffer contents in-memory, aiming at reducing the >>>> required >>>> bandwidth >>>> @@ -3970,15 +3970,15 @@ static void >>>> __gen6_gt_force_wake_get(struct >>>> drm_i915_private *dev_priv) >>>> else >>>> forcewake_ack = FORCEWAKE_ACK; >>>> >>>> - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & >>>> 1) == >>>> 0, >>>> - FORCEWAKE_ACK_TIMEOUT_US)) >>>> + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) >>>> == 0, >>>> + FORCEWAKE_ACK_TIMEOUT_MS)) >>> >>> >>> Superficially this looks okay, but the implementation of >>> wait_for_atomic() not so. As a surprise, this change drops >>> cpu_relax() >>> from the busy loop, even thought the timeout is potentially much >>> longer. >>> >>> The quick fix here would be to just use 2000 us with >>> wait_for_atomic_us(), but we should do something about >>> wait_for_atomic() >>> too. Luckily it's only ever used at one place. >>> >>> BR, >>> Jani. >> >> >> Hmm, dare I say, I think this is a bug in _wait_for. Without >> spending too >> much brain power on this, I believe the compiler can screw us over >> here. No >> matter the bug, cpu_relax is still probably desirable, unless there >> is some >> newer coolness here? I shall insert a patch before this to do the >> cpu_relax >> in _wait_for. > > The original idea behind wiat_for_us was that we use udelay and > really > limit ourselves to that us timeout (since jiffies is too coarse). Now > that the timeout for forcewake is 2ms (gawk!) I think we can stop > bothering to pretend that this should timeout quickly and drop the > _us > variant (but still keep the cpu relax imo). > -Daniel Unless I'm confused, you're acking what I was planning? -- Ben Widawsky, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms 2012-08-28 16:07 ` Ben Widawsky @ 2012-08-28 16:27 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2012-08-28 16:27 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Tue, Aug 28, 2012 at 6:07 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > On 2012-08-28 09:00, Daniel Vetter wrote: >> >> On Tue, Aug 28, 2012 at 5:51 PM, Ben Widawsky <ben@bwidawsk.net> wrote: >>> >>> On 2012-08-26 23:59, Jani Nikula wrote: >>>> >>>> >>>> On Fri, 24 Aug 2012, Ben Widawsky <ben@bwidawsk.net> wrote: >>>>> >>>>> >>>>> A designer familiar with the hardware has stated that the forcewake >>>>> timeout can theoretically be as high as a little over 1ms. Therefore we >>>>> modify our code to use 2ms (appropriate fudge and because we don't want >>>>> to round down). >>>>> >>>>> Hopefully this can't prevent spurious timeouts. >>>>> >>>>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++----------- >>>>> 1 file changed, 11 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c >>>>> b/drivers/gpu/drm/i915/intel_pm.c >>>>> index f42c142..2a8468d 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>>> @@ -31,7 +31,7 @@ >>>>> #include "../../../platform/x86/intel_ips.h" >>>>> #include <linux/module.h> >>>>> >>>>> -#define FORCEWAKE_ACK_TIMEOUT_US 500 >>>>> +#define FORCEWAKE_ACK_TIMEOUT_MS 2 >>>>> >>>>> /* FBC, or Frame Buffer Compression, is a technique employed to >>>>> compress >>>>> the >>>>> * framebuffer contents in-memory, aiming at reducing the required >>>>> bandwidth >>>>> @@ -3970,15 +3970,15 @@ static void __gen6_gt_force_wake_get(struct >>>>> drm_i915_private *dev_priv) >>>>> else >>>>> forcewake_ack = FORCEWAKE_ACK; >>>>> >>>>> - if (wait_for_atomic_us((I915_READ_NOTRACE(forcewake_ack) & 1) >>>>> == >>>>> 0, >>>>> - FORCEWAKE_ACK_TIMEOUT_US)) >>>>> + if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1) == >>>>> 0, >>>>> + FORCEWAKE_ACK_TIMEOUT_MS)) >>>> >>>> >>>> >>>> Superficially this looks okay, but the implementation of >>>> wait_for_atomic() not so. As a surprise, this change drops cpu_relax() >>>> from the busy loop, even thought the timeout is potentially much longer. >>>> >>>> The quick fix here would be to just use 2000 us with >>>> wait_for_atomic_us(), but we should do something about wait_for_atomic() >>>> too. Luckily it's only ever used at one place. >>>> >>>> BR, >>>> Jani. >>> >>> >>> >>> Hmm, dare I say, I think this is a bug in _wait_for. Without spending too >>> much brain power on this, I believe the compiler can screw us over here. >>> No >>> matter the bug, cpu_relax is still probably desirable, unless there is >>> some >>> newer coolness here? I shall insert a patch before this to do the >>> cpu_relax >>> in _wait_for. >> >> >> The original idea behind wiat_for_us was that we use udelay and really >> limit ourselves to that us timeout (since jiffies is too coarse). Now >> that the timeout for forcewake is 2ms (gawk!) I think we can stop >> bothering to pretend that this should timeout quickly and drop the _us >> variant (but still keep the cpu relax imo). >> -Daniel > > > Unless I'm confused, you're acking what I was planning? If what you're planing is to fix up wait_for_atomic to look like wait_for_us and the ditch the _us variant, yep, acked. -Daniel -- Daniel Vetter daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] drm/i915: Never read FORCEWAKE 2012-08-24 19:31 [PATCH 0/3] Some forcewake fixes Ben Widawsky 2012-08-24 19:31 ` [PATCH 1/3] drm/i915: Extract forcewake ack timeout Ben Widawsky 2012-08-24 19:31 ` [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms Ben Widawsky @ 2012-08-24 19:31 ` Ben Widawsky 2012-08-24 19:48 ` Daniel Vetter 2 siblings, 1 reply; 10+ messages in thread From: Ben Widawsky @ 2012-08-24 19:31 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky The same designer from the previous patch has told us to never read FORCEWAKE. We only do this for the POSTING_READ(), so simply change that to something within the same cacheline (for no reason in particular other than it sounds nice). In the _mt case we can leverage the gtfifodbg check for the POSTING_READ. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/intel_pm.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2a8468d..83ec02c 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3975,7 +3975,7 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) DRM_ERROR("Force wake wait timed out\n"); I915_WRITE_NOTRACE(FORCEWAKE, 1); - POSTING_READ(FORCEWAKE); + POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */ if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), FORCEWAKE_ACK_TIMEOUT_MS)) @@ -3998,7 +3998,7 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) DRM_ERROR("Force wake wait timed out\n"); I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); - POSTING_READ(FORCEWAKE_MT); + POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */ if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), FORCEWAKE_ACK_TIMEOUT_MS)) @@ -4035,14 +4035,12 @@ void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv) static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv) { I915_WRITE_NOTRACE(FORCEWAKE, 0); - POSTING_READ(FORCEWAKE); gen6_gt_check_fifodbg(dev_priv); } static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv) { I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_DISABLE(1)); - POSTING_READ(FORCEWAKE_MT); gen6_gt_check_fifodbg(dev_priv); } -- 1.7.12 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm/i915: Never read FORCEWAKE 2012-08-24 19:31 ` [PATCH 3/3] drm/i915: Never read FORCEWAKE Ben Widawsky @ 2012-08-24 19:48 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2012-08-24 19:48 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx On Fri, Aug 24, 2012 at 12:31:11PM -0700, Ben Widawsky wrote: > The same designer from the previous patch has told us to never read > FORCEWAKE. We only do this for the POSTING_READ(), so simply change that > to something within the same cacheline (for no reason in particular > other than it sounds nice). In the _mt case we can leverage > the gtfifodbg check for the POSTING_READ. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> This partially reverts commit 6af2d180f82151cf3d58952e35a4f96e45bc453a Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Thu Jul 26 16:24:50 2012 +0200 drm/i915: fix forcewake related hangs on snb I guess the commit message should mention that. Also, please add a comment it the _put functions that the check_fifodbg serves as a posting read (and that we have experimental evidence suggesting that we should readback something "nearby"). btw, can you please ask your nice designer whether he has any clue whether that "nearby" posting_read has an effect? Thanks, Daniel > --- > drivers/gpu/drm/i915/intel_pm.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2a8468d..83ec02c 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3975,7 +3975,7 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv) > DRM_ERROR("Force wake wait timed out\n"); > > I915_WRITE_NOTRACE(FORCEWAKE, 1); > - POSTING_READ(FORCEWAKE); > + POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */ > > if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), > FORCEWAKE_ACK_TIMEOUT_MS)) > @@ -3998,7 +3998,7 @@ static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv) > DRM_ERROR("Force wake wait timed out\n"); > > I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1)); > - POSTING_READ(FORCEWAKE_MT); > + POSTING_READ(ECOBUS); /* something from same cacheline, but !FORCEWAKE */ > > if (wait_for_atomic((I915_READ_NOTRACE(forcewake_ack) & 1), > FORCEWAKE_ACK_TIMEOUT_MS)) > @@ -4035,14 +4035,12 @@ void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv) > static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv) > { > I915_WRITE_NOTRACE(FORCEWAKE, 0); > - POSTING_READ(FORCEWAKE); > gen6_gt_check_fifodbg(dev_priv); > } > > static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv) > { > I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_DISABLE(1)); > - POSTING_READ(FORCEWAKE_MT); > gen6_gt_check_fifodbg(dev_priv); > } > > -- > 1.7.12 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-08-28 16:27 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-08-24 19:31 [PATCH 0/3] Some forcewake fixes Ben Widawsky 2012-08-24 19:31 ` [PATCH 1/3] drm/i915: Extract forcewake ack timeout Ben Widawsky 2012-08-24 19:31 ` [PATCH 2/3] drm/i915: Change forcewake timeout to 2ms Ben Widawsky 2012-08-27 6:59 ` Jani Nikula 2012-08-28 15:51 ` Ben Widawsky 2012-08-28 16:00 ` Daniel Vetter 2012-08-28 16:07 ` Ben Widawsky 2012-08-28 16:27 ` Daniel Vetter 2012-08-24 19:31 ` [PATCH 3/3] drm/i915: Never read FORCEWAKE Ben Widawsky 2012-08-24 19:48 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.