Re: [PATCH] rpm

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 10, 2014 at 05:43:15PM +0100, Chris Wilson wrote:
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 20 ++------------
>  drivers/gpu/drm/i915/intel_lrc.c     | 21 ++-------------
>  drivers/gpu/drm/i915/intel_uncore.c  | 52 +++++++++++++++---------------------
>  4 files changed, 27 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5f35048..a72d8b8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return 0;
>  
> +	intel_runtime_pm_get(dev_priv);
>  	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>  
>  	return 0;
> @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>  		return 0;
>  
>  	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> +	intel_runtime_pm_put(dev_priv);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 794ad8f..fafd202 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
> -	unsigned long irqflags;
>  
>  	val = I915_READ(LCPLL_CTL);
>  
> @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  	/*
>  	 * Make sure we're not on PC8 state before disabling PC8, otherwise
>  	 * we'll hang the machine. To prevent PC8 state, just enable force_wake.
> -	 *
> -	 * The other problem is that hsw_restore_lcpll() is called as part of
> -	 * the runtime PM resume sequence, so we can't just call
> -	 * gen6_gt_force_wake_get() because that function calls
> -	 * intel_runtime_pm_get(), and we can't change the runtime PM refcount
> -	 * while we are on the resume sequence. So to solve this problem we have
> -	 * to call special forcewake code that doesn't touch runtime PM and
> -	 * doesn't enable the forcewake delayed work.
>  	 */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -	if (dev_priv->uncore.forcewake_count++ == 0)
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>  
>  	if (val & LCPLL_POWER_DOWN_ALLOW) {
>  		val &= ~LCPLL_POWER_DOWN_ALLOW;
> @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  			DRM_ERROR("Switching back to LCPLL failed\n");
>  	}
>  
> -	/* See the big comment above. */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -	if (--dev_priv->uncore.forcewake_count == 0)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 6f1dd00..aeaa1bc 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>  	struct drm_i915_private *dev_priv = engine->i915;
>  	uint64_t tmp;
>  	uint32_t desc[4];
> -	unsigned long flags;
>  
>  	/* XXX: You must always write both descriptors in the order below. */
>  
> @@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>  	desc[1] = upper_32_bits(tmp);
>  	desc[0] = lower_32_bits(tmp);
>  
> -	/* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes
> -	 * are in progress.
> -	 *
> -	 * The other problem is that we can't just call gen6_gt_force_wake_get()
> -	 * because that function calls intel_runtime_pm_get(), which might sleep.
> -	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
> -	 */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (dev_priv->uncore.forcewake_count++ == 0)
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> -
> +	gen6_gt_force_wake_get(dev_priv, engine->power_domains);
>  	I915_WRITE(RING_ELSP(engine), desc[1]);
>  	I915_WRITE(RING_ELSP(engine), desc[0]);
>  	I915_WRITE(RING_ELSP(engine), desc[3]);
> @@ -280,12 +268,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>  
>  	/* ELSP is a wo register, so use another nearby reg for posting instead */
>  	POSTING_READ(RING_EXECLIST_STATUS(engine));
> -
> -	/* Release Force Wakeup (see the big comment above). */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (--dev_priv->uncore.forcewake_count == 0)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> +	gen6_gt_force_wake_put(dev_priv, engine->power_domains);
>  }
>  
>  static u16 next_tag(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c99d5ef..3b3d3e0 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -24,6 +24,8 @@
>  #include "i915_drv.h"
>  #include "intel_drv.h"
>  
> +#include <linux/pm_runtime.h>
> +
>  #define FORCEWAKE_ACK_TIMEOUT_MS 2
>  
>  #define __raw_i915_read8(dev_priv__, reg__) readb((dev_priv__)->regs + (reg__))
> @@ -258,10 +260,6 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  
>  static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>  {
> -	unsigned long irqflags;
> -
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
>  	if (fw_engine & FORCEWAKE_RENDER &&
>  	    dev_priv->uncore.fw_rendercount++ != 0)
>  		fw_engine &= ~FORCEWAKE_RENDER;
> @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>  
>  	if (fw_engine)
>  		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> -
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
>  static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  {
> -	unsigned long irqflags;
> -
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
>  	if (fw_engine & FORCEWAKE_RENDER) {
>  		WARN_ON(!dev_priv->uncore.fw_rendercount);
>  		if (--dev_priv->uncore.fw_rendercount != 0)
> @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  
>  	if (fw_engine)
>  		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine);
> -
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
>  static void gen6_force_wake_timer(unsigned long arg)

Looks like you forgot to kill the rpm_put() from the timer. And then we
also need to make sure the timer is approriately cancelled when
runtime suspending the device.

-- 
Ville Syrjälä
Intel OTC