[PATCH] drm/i915: Reset forcewake before suspend

[PATCH] drm/i915: Reset forcewake before suspend

[Chris Wilson]

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

Re: [PATCH] drm/i915: Reset forcewake before suspend

[Daniel Vetter]

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

Re: [PATCH] drm/i915: Reset forcewake before suspend

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Reset forcewake before suspend

[Chris Wilson]

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

[PATCH] drm/i915: Consolidate forcewake resetting to a single function

[Chris Wilson]

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

Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function

[Mika Kuoppala]

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

Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function

[Chris Wilson]

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

Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function

[Mika Kuoppala]

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

Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function

[Daniel Vetter]

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

Re: [PATCH] drm/i915: Consolidate forcewake resetting to a single function

[Daniel Vetter]

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