Re: [PATCH] drm/i915: Rebalance runtime pm vs forcewake

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Mar 14, 2014 at 08:37:15AM +0000, Chris Wilson wrote:
> ---
>  drivers/gpu/drm/i915/i915_drv.c     | 2 +-
>  drivers/gpu/drm/i915/intel_uncore.c | 9 ++-------
>  2 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5a0d34c47885..3fbf8aa8d119 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -845,11 +845,11 @@ static int i915_runtime_suspend(struct device *device)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	WARN_ON(!HAS_RUNTIME_PM(dev));
> -	assert_force_wake_inactive(dev_priv);

Why is this necessary? Also I've already pushed a pile of other patches on
top of all this, so I think a full commit is better. Also gives us an
excuse to document our flailing here a bit better in a neat commit message
... Imo we should also mention that the forcewake_put here isn't really
perf critical any more (if this is really the case).

Cheers, Daniel
>  
>  	DRM_DEBUG_KMS("Suspending device\n");
>  
>  	i915_gem_release_all_mmaps(dev_priv);
> +	intel_uncore_fini(dev);
>  
>  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
>  	dev_priv->pm.suspended = true;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index e6bb421a3dbd..527527382361 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -304,8 +304,6 @@ static void gen6_force_wake_timer(unsigned long arg)
>  	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);
> -
> -	intel_runtime_pm_put(dev_priv);
>  }
>  
>  static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> @@ -439,7 +437,6 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>  void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  {
>  	unsigned long irqflags;
> -	bool delayed = false;
>  
>  	if (!dev_priv->uncore.funcs.force_wake_put)
>  		return;
> @@ -450,19 +447,17 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  		goto out;
>  	}
>  
> -
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	if (--dev_priv->uncore.forcewake_count == 0) {
>  		dev_priv->uncore.forcewake_count++;
> -		delayed = true;
>  		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
>  				 jiffies + 1);
>  	}
> +
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  
>  out:
> -	if (!delayed)
> -		intel_runtime_pm_put(dev_priv);
> +	intel_runtime_pm_put(dev_priv);
>  }
>  
>  void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
> -- 
> 1.9.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch