* [PATCH] drm/i915: Reset forcewake before suspend @ 2014-03-13 11:05 Chris Wilson 2014-03-13 11:26 ` Daniel Vetter 2014-03-13 11:29 ` Ville Syrjälä 0 siblings, 2 replies; 10+ messages in thread From: Chris Wilson @ 2014-03-13 11:05 UTC (permalink / raw) To: intel-gfx Now that we regularly defer the forcewake dance to a timer func, it is likely to fire after we disable the device during suspend. This generates an oops as we detect inconsistency in the hardware state. So before suspend, we want to complete the outstanding dance and generally sanitize the registers before handing back to the BIOS. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 1 + drivers/gpu/drm/i915/intel_uncore.c | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 658fe24961eb..5a0d34c47885 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -467,6 +467,7 @@ static int i915_drm_freeze(struct drm_device *dev) i915_save_state(dev); intel_opregion_fini(dev); + intel_uncore_fini(dev); console_lock(); intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 7861d97600e1..361d1eae3cfd 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -805,6 +805,10 @@ void intel_uncore_fini(struct drm_device *dev) /* Paranoia: make sure we have disabled everything before we exit. */ intel_uncore_sanitize(dev); intel_uncore_forcewake_reset(dev); + + dev_priv->uncore.forcewake_count = 0; + dev_priv->uncore.fw_rendercount = 0; + dev_priv->uncore.fw_mediacount = 0; } static const struct register_whitelist { -- 1.9.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Reset forcewake before suspend 2014-03-13 11:05 [PATCH] drm/i915: Reset forcewake before suspend Chris Wilson @ 2014-03-13 11:26 ` Daniel Vetter 2014-03-13 11:29 ` Ville Syrjälä 1 sibling, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2014-03-13 11:26 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Thu, Mar 13, 2014 at 11:05:02AM +0000, Chris Wilson wrote: > Now that we regularly defer the forcewake dance to a timer func, it is > likely to fire after we disable the device during suspend. This > generates an oops as we detect inconsistency in the hardware state. So > before suspend, we want to complete the outstanding dance and generally > sanitize the registers before handing back to the BIOS. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Queued for -next, thanks for the patch. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.c | 1 + > drivers/gpu/drm/i915/intel_uncore.c | 4 ++++ > 2 files changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 658fe24961eb..5a0d34c47885 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -467,6 +467,7 @@ static int i915_drm_freeze(struct drm_device *dev) > i915_save_state(dev); > > intel_opregion_fini(dev); > + intel_uncore_fini(dev); > > console_lock(); > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 7861d97600e1..361d1eae3cfd 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -805,6 +805,10 @@ void intel_uncore_fini(struct drm_device *dev) > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev); > intel_uncore_forcewake_reset(dev); > + > + dev_priv->uncore.forcewake_count = 0; > + dev_priv->uncore.fw_rendercount = 0; > + dev_priv->uncore.fw_mediacount = 0; > } > > static const struct register_whitelist { > -- > 1.9.0 > > _______________________________________________ > 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] 10+ messages in thread
* Re: [PATCH] drm/i915: Reset forcewake before suspend 2014-03-13 11:05 [PATCH] drm/i915: Reset forcewake before suspend Chris Wilson 2014-03-13 11:26 ` Daniel Vetter @ 2014-03-13 11:29 ` Ville Syrjälä 2014-03-13 11:57 ` Chris Wilson 1 sibling, 1 reply; 10+ messages in thread From: Ville Syrjälä @ 2014-03-13 11:29 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Thu, Mar 13, 2014 at 11:05:02AM +0000, Chris Wilson wrote: > Now that we regularly defer the forcewake dance to a timer func, it is > likely to fire after we disable the device during suspend. This > generates an oops as we detect inconsistency in the hardware state. So > before suspend, we want to complete the outstanding dance and generally > sanitize the registers before handing back to the BIOS. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> I was just looking a this a bit myself and came to the same conclusion. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> There is however one slight issue which I missed when the reviewing the forcewake timer patch. intel_uncore_fini() will cancel the timer before calling intel_uncore_sanitize(), but if that one will ever do something that needs forcewake, we're going to run into the same problem again. Actually it looks like gen6_disable_rps() will result in a forcewake get on BDW due to GEN6_RC_CONTROL no being shadowed. So it looks like we should just move the del_timer_sync() to happen after intel_uncore_sanitize() and this issue would be fixed. > --- > drivers/gpu/drm/i915/i915_drv.c | 1 + > drivers/gpu/drm/i915/intel_uncore.c | 4 ++++ > 2 files changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 658fe24961eb..5a0d34c47885 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -467,6 +467,7 @@ static int i915_drm_freeze(struct drm_device *dev) > i915_save_state(dev); > > intel_opregion_fini(dev); > + intel_uncore_fini(dev); > > console_lock(); > intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 7861d97600e1..361d1eae3cfd 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -805,6 +805,10 @@ void intel_uncore_fini(struct drm_device *dev) > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev); > intel_uncore_forcewake_reset(dev); > + > + dev_priv->uncore.forcewake_count = 0; > + dev_priv->uncore.fw_rendercount = 0; > + dev_priv->uncore.fw_mediacount = 0; > } > > static const struct register_whitelist { > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Reset forcewake before suspend 2014-03-13 11:29 ` Ville Syrjälä @ 2014-03-13 11:57 ` Chris Wilson 2014-03-13 12:00 ` [PATCH] drm/i915: Consolidate forcewake resetting to a single function Chris Wilson 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2014-03-13 11:57 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Thu, Mar 13, 2014 at 01:29:17PM +0200, Ville Syrjälä wrote: > On Thu, Mar 13, 2014 at 11:05:02AM +0000, Chris Wilson wrote: > > Now that we regularly defer the forcewake dance to a timer func, it is > > likely to fire after we disable the device during suspend. This > > generates an oops as we detect inconsistency in the hardware state. So > > before suspend, we want to complete the outstanding dance and generally > > sanitize the registers before handing back to the BIOS. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > I was just looking a this a bit myself and came to the same conclusion. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > There is however one slight issue which I missed when the reviewing the > forcewake timer patch. intel_uncore_fini() will cancel the timer before > calling intel_uncore_sanitize(), but if that one will ever do something > that needs forcewake, we're going to run into the same problem again. > Actually it looks like gen6_disable_rps() will result in a forcewake > get on BDW due to GEN6_RC_CONTROL no being shadowed. So it looks like we > should just move the del_timer_sync() to happen after > intel_uncore_sanitize() and this issue would be fixed. I'd like to move it all to forcewake_reset(), just requires a little bit of fiddling to maintain the user forcewake across gen6_do_reset. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915: Consolidate forcewake resetting to a single function 2014-03-13 11:57 ` Chris Wilson @ 2014-03-13 12:00 ` Chris Wilson 2014-03-13 13:00 ` Mika Kuoppala 2014-03-13 13:24 ` Mika Kuoppala 0 siblings, 2 replies; 10+ messages in thread From: Chris Wilson @ 2014-03-13 12:00 UTC (permalink / raw) To: intel-gfx We have two paths that try to reset the forcewake registers back to known good values, with slightly different semantics and levels of paranoia. Combine the two by passing a parameter to either restore the forcewake status or to clear our bookkeeping, and raise the paranoia level to max. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_uncore.c | 81 +++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 40 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 361d1eae3cfd..e6bb421a3dbd 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -308,9 +308,17 @@ static void gen6_force_wake_timer(unsigned long arg) intel_runtime_pm_put(dev_priv); } -static void intel_uncore_forcewake_reset(struct drm_device *dev) +static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) { struct drm_i915_private *dev_priv = dev->dev_private; + unsigned long irqflags; + + del_timer_sync(&dev_priv->uncore.force_wake_timer); + + /* Hold uncore.lock across reset to prevent any register access + * with forcewake not set correctly + */ + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); if (IS_VALLEYVIEW(dev)) vlv_force_wake_reset(dev_priv); @@ -319,6 +327,35 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev) if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_GEN8(dev)) __gen7_gt_force_wake_mt_reset(dev_priv); + + if (restore) { /* If reset with a user forcewake, try to restore */ + unsigned fw = 0; + + if (IS_VALLEYVIEW(dev)) { + if (dev_priv->uncore.fw_rendercount) + fw |= FORCEWAKE_RENDER; + + if (dev_priv->uncore.fw_mediacount) + fw |= FORCEWAKE_MEDIA; + } else { + if (dev_priv->uncore.forcewake_count) + fw = FORCEWAKE_ALL; + } + + if (fw) + dev_priv->uncore.funcs.force_wake_get(dev_priv, fw); + + if (IS_GEN6(dev) || IS_GEN7(dev)) + dev_priv->uncore.fifo_count = + __raw_i915_read32(dev_priv, GTFIFOCTL) & + GT_FIFO_FREE_ENTRIES_MASK; + } else { + dev_priv->uncore.forcewake_count = 0; + dev_priv->uncore.fw_rendercount = 0; + dev_priv->uncore.fw_mediacount = 0; + } + + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); } void intel_uncore_early_sanitize(struct drm_device *dev) @@ -344,7 +381,7 @@ void intel_uncore_early_sanitize(struct drm_device *dev) __raw_i915_write32(dev_priv, GTFIFODBG, __raw_i915_read32(dev_priv, GTFIFODBG)); - intel_uncore_forcewake_reset(dev); + intel_uncore_forcewake_reset(dev, false); } void intel_uncore_sanitize(struct drm_device *dev) @@ -798,17 +835,9 @@ void intel_uncore_init(struct drm_device *dev) void intel_uncore_fini(struct drm_device *dev) { - struct drm_i915_private *dev_priv = dev->dev_private; - - del_timer_sync(&dev_priv->uncore.force_wake_timer); - /* Paranoia: make sure we have disabled everything before we exit. */ intel_uncore_sanitize(dev); - intel_uncore_forcewake_reset(dev); - - dev_priv->uncore.forcewake_count = 0; - dev_priv->uncore.fw_rendercount = 0; - dev_priv->uncore.fw_mediacount = 0; + intel_uncore_forcewake_reset(dev, false); } static const struct register_whitelist { @@ -957,13 +986,6 @@ static int gen6_do_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; int ret; - unsigned long irqflags; - u32 fw_engine = 0; - - /* Hold uncore.lock across reset to prevent any register access - * with forcewake not set correctly - */ - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); /* Reset the chip */ @@ -976,29 +998,8 @@ static int gen6_do_reset(struct drm_device *dev) /* Spin waiting for the device to ack the reset request */ ret = wait_for((__raw_i915_read32(dev_priv, GEN6_GDRST) & GEN6_GRDOM_FULL) == 0, 500); - intel_uncore_forcewake_reset(dev); - - /* If reset with a user forcewake, try to restore */ - if (IS_VALLEYVIEW(dev)) { - if (dev_priv->uncore.fw_rendercount) - fw_engine |= FORCEWAKE_RENDER; - - if (dev_priv->uncore.fw_mediacount) - fw_engine |= FORCEWAKE_MEDIA; - } else { - if (dev_priv->uncore.forcewake_count) - fw_engine = FORCEWAKE_ALL; - } - - if (fw_engine) - dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine); + intel_uncore_forcewake_reset(dev, true); - if (IS_GEN6(dev) || IS_GEN7(dev)) - dev_priv->uncore.fifo_count = - __raw_i915_read32(dev_priv, GTFIFOCTL) & - GT_FIFO_FREE_ENTRIES_MASK; - - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); return ret; } -- 1.9.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function 2014-03-13 12:00 ` [PATCH] drm/i915: Consolidate forcewake resetting to a single function Chris Wilson @ 2014-03-13 13:00 ` Mika Kuoppala 2014-03-13 13:10 ` Chris Wilson 2014-03-13 13:24 ` Mika Kuoppala 1 sibling, 1 reply; 10+ messages in thread From: Mika Kuoppala @ 2014-03-13 13:00 UTC (permalink / raw) To: Chris Wilson, intel-gfx Chris Wilson <chris@chris-wilson.co.uk> writes: > We have two paths that try to reset the forcewake registers back to > known good values, with slightly different semantics and levels of > paranoia. Combine the two by passing a parameter to either restore the > forcewake status or to clear our bookkeeping, and raise the paranoia > level to max. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_uncore.c | 81 +++++++++++++++++++------------------ > 1 file changed, 41 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 361d1eae3cfd..e6bb421a3dbd 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -308,9 +308,17 @@ static void gen6_force_wake_timer(unsigned long arg) > intel_runtime_pm_put(dev_priv); > } > > -static void intel_uncore_forcewake_reset(struct drm_device *dev) > +static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned long irqflags; > + > + del_timer_sync(&dev_priv->uncore.force_wake_timer); > + > + /* Hold uncore.lock across reset to prevent any register access > + * with forcewake not set correctly > + */ > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > if (IS_VALLEYVIEW(dev)) > vlv_force_wake_reset(dev_priv); > @@ -319,6 +327,35 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev) > > if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_GEN8(dev)) > __gen7_gt_force_wake_mt_reset(dev_priv); > + > + if (restore) { /* If reset with a user forcewake, try to restore */ > + unsigned fw = 0; > + > + if (IS_VALLEYVIEW(dev)) { > + if (dev_priv->uncore.fw_rendercount) > + fw |= FORCEWAKE_RENDER; > + > + if (dev_priv->uncore.fw_mediacount) > + fw |= FORCEWAKE_MEDIA; > + } else { > + if (dev_priv->uncore.forcewake_count) > + fw = FORCEWAKE_ALL; > + } > + > + if (fw) > + dev_priv->uncore.funcs.force_wake_get(dev_priv, fw); > + > + if (IS_GEN6(dev) || IS_GEN7(dev)) > + dev_priv->uncore.fifo_count = > + __raw_i915_read32(dev_priv, GTFIFOCTL) & > + GT_FIFO_FREE_ENTRIES_MASK; > + } else { > + dev_priv->uncore.forcewake_count = 0; > + dev_priv->uncore.fw_rendercount = 0; > + dev_priv->uncore.fw_mediacount = 0; > + } > + > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > } > > void intel_uncore_early_sanitize(struct drm_device *dev) > @@ -344,7 +381,7 @@ void intel_uncore_early_sanitize(struct drm_device *dev) > __raw_i915_write32(dev_priv, GTFIFODBG, > __raw_i915_read32(dev_priv, GTFIFODBG)); > > - intel_uncore_forcewake_reset(dev); > + intel_uncore_forcewake_reset(dev, false); > } > > void intel_uncore_sanitize(struct drm_device *dev) > @@ -798,17 +835,9 @@ void intel_uncore_init(struct drm_device *dev) > > void intel_uncore_fini(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - > - del_timer_sync(&dev_priv->uncore.force_wake_timer); > - > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev); > - intel_uncore_forcewake_reset(dev); > - > - dev_priv->uncore.forcewake_count = 0; > - dev_priv->uncore.fw_rendercount = 0; > - dev_priv->uncore.fw_mediacount = 0; > + intel_uncore_forcewake_reset(dev, false); > } > > static const struct register_whitelist { > @@ -957,13 +986,6 @@ static int gen6_do_reset(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > - unsigned long irqflags; > - u32 fw_engine = 0; > - > - /* Hold uncore.lock across reset to prevent any register access > - * with forcewake not set correctly > - */ > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > /* Reset the chip */ > > @@ -976,29 +998,8 @@ static int gen6_do_reset(struct drm_device *dev) > /* Spin waiting for the device to ack the reset request */ > ret = wait_for((__raw_i915_read32(dev_priv, GEN6_GDRST) & GEN6_GRDOM_FULL) == 0, 500); We used to go through the reset with uncore lock held, which is not the case anymore. Will it create problems? -Mika > - intel_uncore_forcewake_reset(dev); > - > - /* If reset with a user forcewake, try to restore */ > - if (IS_VALLEYVIEW(dev)) { > - if (dev_priv->uncore.fw_rendercount) > - fw_engine |= FORCEWAKE_RENDER; > - > - if (dev_priv->uncore.fw_mediacount) > - fw_engine |= FORCEWAKE_MEDIA; > - } else { > - if (dev_priv->uncore.forcewake_count) > - fw_engine = FORCEWAKE_ALL; > - } > - > - if (fw_engine) > - dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine); > + intel_uncore_forcewake_reset(dev, true); > > - if (IS_GEN6(dev) || IS_GEN7(dev)) > - dev_priv->uncore.fifo_count = > - __raw_i915_read32(dev_priv, GTFIFOCTL) & > - GT_FIFO_FREE_ENTRIES_MASK; > - > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > return ret; > } > > -- > 1.9.0 > > _______________________________________________ > 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] drm/i915: Consolidate forcewake resetting to a single function 2014-03-13 13:00 ` Mika Kuoppala @ 2014-03-13 13:10 ` Chris Wilson 2014-03-13 17:21 ` Daniel Vetter 0 siblings, 1 reply; 10+ messages in thread From: Chris Wilson @ 2014-03-13 13:10 UTC (permalink / raw) To: Mika Kuoppala; +Cc: intel-gfx On Thu, Mar 13, 2014 at 03:00:58PM +0200, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > @@ -957,13 +986,6 @@ static int gen6_do_reset(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > int ret; > > - unsigned long irqflags; > > - u32 fw_engine = 0; > > - > > - /* Hold uncore.lock across reset to prevent any register access > > - * with forcewake not set correctly > > - */ > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > /* Reset the chip */ > > > > @@ -976,29 +998,8 @@ static int gen6_do_reset(struct drm_device *dev) > > /* Spin waiting for the device to ack the reset request */ > > ret = wait_for((__raw_i915_read32(dev_priv, GEN6_GDRST) & GEN6_GRDOM_FULL) == 0, 500); > > We used to go through the reset with uncore lock held, > which is not the case anymore. Will it create problems? I don't think so. gen6_do_reset() is itself serialised and there should be no other access to GDRST anyway, so from that perspective we won't trigger hw errors. So the only thing that the uncore lock was protecting in this function is the forcewake dance. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function 2014-03-13 13:10 ` Chris Wilson @ 2014-03-13 17:21 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2014-03-13 17:21 UTC (permalink / raw) To: Chris Wilson, Mika Kuoppala, intel-gfx On Thu, Mar 13, 2014 at 01:10:37PM +0000, Chris Wilson wrote: > On Thu, Mar 13, 2014 at 03:00:58PM +0200, Mika Kuoppala wrote: > > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > @@ -957,13 +986,6 @@ static int gen6_do_reset(struct drm_device *dev) > > > { > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > int ret; > > > - unsigned long irqflags; > > > - u32 fw_engine = 0; > > > - > > > - /* Hold uncore.lock across reset to prevent any register access > > > - * with forcewake not set correctly > > > - */ > > > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > > > /* Reset the chip */ > > > > > > @@ -976,29 +998,8 @@ static int gen6_do_reset(struct drm_device *dev) > > > /* Spin waiting for the device to ack the reset request */ > > > ret = wait_for((__raw_i915_read32(dev_priv, GEN6_GDRST) & GEN6_GRDOM_FULL) == 0, 500); > > > > We used to go through the reset with uncore lock held, > > which is not the case anymore. Will it create problems? > > I don't think so. gen6_do_reset() is itself serialised and there should > be no other access to GDRST anyway, so from that perspective we won't > trigger hw errors. So the only thing that the uncore lock was protecting > in this function is the forcewake dance. Yes, protecting the forcewake dance was the only reason to hold the uncore lock over gen6 reset. As long as we hold the uncore lock while restoring any forcewake references after reset we're good, no need to shove the entire reset under that spinlock. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function 2014-03-13 12:00 ` [PATCH] drm/i915: Consolidate forcewake resetting to a single function Chris Wilson 2014-03-13 13:00 ` Mika Kuoppala @ 2014-03-13 13:24 ` Mika Kuoppala 2014-03-13 17:22 ` Daniel Vetter 1 sibling, 1 reply; 10+ messages in thread From: Mika Kuoppala @ 2014-03-13 13:24 UTC (permalink / raw) To: Chris Wilson, intel-gfx Chris Wilson <chris@chris-wilson.co.uk> writes: > We have two paths that try to reset the forcewake registers back to > known good values, with slightly different semantics and levels of > paranoia. Combine the two by passing a parameter to either restore the > forcewake status or to clear our bookkeeping, and raise the paranoia > level to max. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 81 +++++++++++++++++++------------------ > 1 file changed, 41 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 361d1eae3cfd..e6bb421a3dbd 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -308,9 +308,17 @@ static void gen6_force_wake_timer(unsigned long arg) > intel_runtime_pm_put(dev_priv); > } > > -static void intel_uncore_forcewake_reset(struct drm_device *dev) > +static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned long irqflags; > + > + del_timer_sync(&dev_priv->uncore.force_wake_timer); > + > + /* Hold uncore.lock across reset to prevent any register access > + * with forcewake not set correctly > + */ > + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > if (IS_VALLEYVIEW(dev)) > vlv_force_wake_reset(dev_priv); > @@ -319,6 +327,35 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev) > > if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_GEN8(dev)) > __gen7_gt_force_wake_mt_reset(dev_priv); > + > + if (restore) { /* If reset with a user forcewake, try to restore */ > + unsigned fw = 0; > + > + if (IS_VALLEYVIEW(dev)) { > + if (dev_priv->uncore.fw_rendercount) > + fw |= FORCEWAKE_RENDER; > + > + if (dev_priv->uncore.fw_mediacount) > + fw |= FORCEWAKE_MEDIA; > + } else { > + if (dev_priv->uncore.forcewake_count) > + fw = FORCEWAKE_ALL; > + } > + > + if (fw) > + dev_priv->uncore.funcs.force_wake_get(dev_priv, fw); > + > + if (IS_GEN6(dev) || IS_GEN7(dev)) > + dev_priv->uncore.fifo_count = > + __raw_i915_read32(dev_priv, GTFIFOCTL) & > + GT_FIFO_FREE_ENTRIES_MASK; > + } else { > + dev_priv->uncore.forcewake_count = 0; > + dev_priv->uncore.fw_rendercount = 0; > + dev_priv->uncore.fw_mediacount = 0; > + } > + > + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > } > > void intel_uncore_early_sanitize(struct drm_device *dev) > @@ -344,7 +381,7 @@ void intel_uncore_early_sanitize(struct drm_device *dev) > __raw_i915_write32(dev_priv, GTFIFODBG, > __raw_i915_read32(dev_priv, GTFIFODBG)); > > - intel_uncore_forcewake_reset(dev); > + intel_uncore_forcewake_reset(dev, false); > } > > void intel_uncore_sanitize(struct drm_device *dev) > @@ -798,17 +835,9 @@ void intel_uncore_init(struct drm_device *dev) > > void intel_uncore_fini(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - > - del_timer_sync(&dev_priv->uncore.force_wake_timer); > - > /* Paranoia: make sure we have disabled everything before we exit. */ > intel_uncore_sanitize(dev); > - intel_uncore_forcewake_reset(dev); > - > - dev_priv->uncore.forcewake_count = 0; > - dev_priv->uncore.fw_rendercount = 0; > - dev_priv->uncore.fw_mediacount = 0; > + intel_uncore_forcewake_reset(dev, false); > } > > static const struct register_whitelist { > @@ -957,13 +986,6 @@ static int gen6_do_reset(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > - unsigned long irqflags; > - u32 fw_engine = 0; > - > - /* Hold uncore.lock across reset to prevent any register access > - * with forcewake not set correctly > - */ > - spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > /* Reset the chip */ > > @@ -976,29 +998,8 @@ static int gen6_do_reset(struct drm_device *dev) > /* Spin waiting for the device to ack the reset request */ > ret = wait_for((__raw_i915_read32(dev_priv, GEN6_GDRST) & GEN6_GRDOM_FULL) == 0, 500); > > - intel_uncore_forcewake_reset(dev); > - > - /* If reset with a user forcewake, try to restore */ > - if (IS_VALLEYVIEW(dev)) { > - if (dev_priv->uncore.fw_rendercount) > - fw_engine |= FORCEWAKE_RENDER; > - > - if (dev_priv->uncore.fw_mediacount) > - fw_engine |= FORCEWAKE_MEDIA; > - } else { > - if (dev_priv->uncore.forcewake_count) > - fw_engine = FORCEWAKE_ALL; > - } > - > - if (fw_engine) > - dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine); > + intel_uncore_forcewake_reset(dev, true); > > - if (IS_GEN6(dev) || IS_GEN7(dev)) > - dev_priv->uncore.fifo_count = > - __raw_i915_read32(dev_priv, GTFIFOCTL) & > - GT_FIFO_FREE_ENTRIES_MASK; > - > - spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > return ret; > } > > -- > 1.9.0 > > _______________________________________________ > 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] drm/i915: Consolidate forcewake resetting to a single function 2014-03-13 13:24 ` Mika Kuoppala @ 2014-03-13 17:22 ` Daniel Vetter 0 siblings, 0 replies; 10+ messages in thread From: Daniel Vetter @ 2014-03-13 17:22 UTC (permalink / raw) To: Mika Kuoppala; +Cc: intel-gfx On Thu, Mar 13, 2014 at 03:24:23PM +0200, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > We have two paths that try to reset the forcewake registers back to > > known good values, with slightly different semantics and levels of > > paranoia. Combine the two by passing a parameter to either restore the > > forcewake status or to clear our bookkeeping, and raise the paranoia > > level to max. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-03-13 17:22 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-13 11:05 [PATCH] drm/i915: Reset forcewake before suspend Chris Wilson 2014-03-13 11:26 ` Daniel Vetter 2014-03-13 11:29 ` Ville Syrjälä 2014-03-13 11:57 ` Chris Wilson 2014-03-13 12:00 ` [PATCH] drm/i915: Consolidate forcewake resetting to a single function Chris Wilson 2014-03-13 13:00 ` Mika Kuoppala 2014-03-13 13:10 ` Chris Wilson 2014-03-13 17:21 ` Daniel Vetter 2014-03-13 13:24 ` Mika Kuoppala 2014-03-13 17:22 ` 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.