Re: [PATCH] drm/i915: Fix runtime pm inbalance due to reg I/O forcewake dance

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

On Fri, Mar 14, 2014 at 08:48:46AM +0100, Daniel Vetter wrote:
> This regression has been introduced in
> 
> commit 8232644ccf099548710843e97360a3fcd6d28e04
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Mar 5 12:00:39 2014 +0000
> 
>     drm/i915: Convert the forcewake worker into a timer func
> 
> which started to use the delayed forcewake put also for the register
> I/O forcewake dance. But this conflicts functionally with the
> (reviewed in parallel)
> 
> commit 6d88064edcfc5e5893371f7c06b9f3078dc1edf6
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Fri Feb 21 17:58:29 2014 -0300
> 
>     drm/i915: put runtime PM only when we actually release force_wake
> 
> which moved the runtime pm put calls into the delayed forcewake put
> function to avoid an inversion between these. The problem is that the
> register I/O function do _not_ grab a runtime pm reference, hence
> dropping it is a bug.
> 
> So split the timer into two to re-balance the runtime pm refcounting.
> The tricky bit to ensure is that the _raw timer doesn't run after
> we've runtime-suspended the device. After all it only has an implicit
> runtime pm reference provided by its caller. But the del_timer_sync in
> the runtime suspend code will ensure this.
> 
> Add a comment to document this all.

Yuck. Why don't we just stick the runtime pm put into
gen6_gt_force_wake_put() and let Chris's timer cancellation +
forcewake reset fixes take care of things?

> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76151
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  9 +++++++++
>  drivers/gpu/drm/i915/intel_uncore.c | 14 ++++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 70fbe904016f..c2789702b9a5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -511,7 +511,16 @@ struct intel_uncore {
>  	unsigned fw_rendercount;
>  	unsigned fw_mediacount;
>  
> +	/*
> +	 * Delayed forcewake put timers. The _raw one is for register I/O
> +	 * functions which don't grab a runtime pm reference, instead presuming
> +	 * that someone else holds that already.
> +	 *
> +	 * Synchronization with runtime pm actually suspended the device happens
> +	 * in the runtime suspend path in intel_uncore_forcewake_reset.
> +	 */
>  	struct timer_list force_wake_timer;
> +	struct timer_list force_wake_timer_raw;
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index e6bb421a3dbd..8010e2caf821 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -293,7 +293,7 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> -static void gen6_force_wake_timer(unsigned long arg)
> +static void gen6_force_wake_timer_raw(unsigned long arg)
>  {
>  	struct drm_i915_private *dev_priv = (void *)arg;
>  	unsigned long irqflags;
> @@ -304,6 +304,13 @@ 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);
> +}
> +
> +static void gen6_force_wake_timer(unsigned long arg)
> +{
> +	struct drm_i915_private *dev_priv = (void *)arg;
> +
> +	gen6_force_wake_timer_raw(arg);
>  
>  	intel_runtime_pm_put(dev_priv);
>  }
> @@ -314,6 +321,7 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  	unsigned long irqflags;
>  
>  	del_timer_sync(&dev_priv->uncore.force_wake_timer);
> +	del_timer_sync(&dev_priv->uncore.force_wake_timer_raw);
>  
>  	/* Hold uncore.lock across reset to prevent any register access
>  	 * with forcewake not set correctly
> @@ -542,7 +550,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
>  						      FORCEWAKE_ALL); \
>  		dev_priv->uncore.forcewake_count++; \
> -		mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \
> +		mod_timer_pinned(&dev_priv->uncore.force_wake_timer_raw, \
>  				 jiffies + 1); \
>  	} \
>  	val = __raw_i915_read##x(dev_priv, reg); \
> @@ -726,6 +734,8 @@ void intel_uncore_init(struct drm_device *dev)
>  
>  	setup_timer(&dev_priv->uncore.force_wake_timer,
>  		    gen6_force_wake_timer, (unsigned long)dev_priv);
> +	setup_timer(&dev_priv->uncore.force_wake_timer_raw,
> +		    gen6_force_wake_timer_raw, (unsigned long)dev_priv);
>  
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
> -- 
> 1.8.5.2

-- 
Ville Syrjälä
Intel OTC