| Message ID | 1446629584-10008-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 04, 2015 at 11:33:04AM +0200, Mika Kuoppala wrote: > We have a timed release of a forcewake when using > I915_READ/WRITE macros. wait_for() macro will go to quite > long sleep if the first read doesn't satisfy the condition for > successful exit. With these two interacting, it is possible that > we lose the forcewake during the wait_for() and the subsequent read > will reaquire forcewake. > > Further experiments with skl shows that when we lose forcewake, > we lose the reset request we submitted. So this register > is not power context saved. > > Grab forcewakes for all engines before starting to request for > resets so that all requests stay valid for the duration of reset > requisition across all the engines. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92774 > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Tested-by: Tomi Sarvela <tomix.p.sarvela@intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index f0f97b2..5a6e7f1b 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1483,6 +1483,8 @@ static int gen8_do_reset(struct drm_device *dev) > struct intel_engine_cs *engine; > int i; > /* If the power well sleeps during the reset, the reset * request may be dropped and never completes (causing -EIO). */ > + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); I wonder if this would be more useful in intel_gpu_reset(), it won't hurt any other platforms and may prevent similar snafu in future? -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Wed, Nov 04, 2015 at 11:33:04AM +0200, Mika Kuoppala wrote: >> We have a timed release of a forcewake when using >> I915_READ/WRITE macros. wait_for() macro will go to quite >> long sleep if the first read doesn't satisfy the condition for >> successful exit. With these two interacting, it is possible that >> we lose the forcewake during the wait_for() and the subsequent read >> will reaquire forcewake. >> >> Further experiments with skl shows that when we lose forcewake, >> we lose the reset request we submitted. So this register >> is not power context saved. >> >> Grab forcewakes for all engines before starting to request for >> resets so that all requests stay valid for the duration of reset >> requisition across all the engines. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92774 >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Tested-by: Tomi Sarvela <tomix.p.sarvela@intel.com> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> drivers/gpu/drm/i915/intel_uncore.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >> index f0f97b2..5a6e7f1b 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -1483,6 +1483,8 @@ static int gen8_do_reset(struct drm_device *dev) >> struct intel_engine_cs *engine; >> int i; >> > /* If the power well sleeps during the reset, the reset > * request may be dropped and never completes (causing -EIO). > */ >> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > > I wonder if this would be more useful in intel_gpu_reset(), it won't > hurt any other platforms and may prevent similar snafu in future? I had similar thoughts but chickened out due to forcewake_reset ending up inside the call chain on such setup. Now with a refreshing my memory, the forcewake_reset should handle the 'restore' case. -Mika > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index f0f97b2..5a6e7f1b 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1483,6 +1483,8 @@ static int gen8_do_reset(struct drm_device *dev) struct intel_engine_cs *engine; int i; + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); + for_each_ring(engine, dev_priv, i) { I915_WRITE(RING_RESET_CTL(engine->mmio_base), _MASKED_BIT_ENABLE(RESET_CTL_REQUEST_RESET)); @@ -1497,6 +1499,8 @@ static int gen8_do_reset(struct drm_device *dev) } } + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); + return gen6_do_reset(dev); not_ready: @@ -1504,6 +1508,8 @@ not_ready: I915_WRITE(RING_RESET_CTL(engine->mmio_base), _MASKED_BIT_DISABLE(RESET_CTL_REQUEST_RESET)); + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); + return -EIO; }