Re: [PATCH] drm/i915: Delay the relase of the forcewake by a jiffie

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

On Sun, Aug 25, 2013 at 08:45:34PM +0100, Chris Wilson wrote:
> Obtaining the forcwake requires expensive and time consuming
> serialisation. And we often try to obtain the forcewake multiple times
> in very quick succession. We can reduce the overhead of these sequences
> by delaying the forcewake release, and so not hammer the hw quite so
> hard.
> 
> I was hoping this would help with the spurious
> [drm:__gen6_gt_force_wake_mt_get] *ERROR* Timed out waiting for forcewake old ack to clear.
> found on Haswell. Alas not.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_dma.c     |  2 ++
>  drivers/gpu/drm/i915/i915_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_uncore.c | 33 +++++++++++++++++++++++++++++++--
>  3 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 883990f..c0fb23e 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1800,6 +1800,8 @@ int i915_driver_unload(struct drm_device *dev)
>  
>  	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
>  
> +	intel_uncore_fini(dev);
> +

We've already unmapped the registers.

>  	if (dev_priv->slab)
>  		kmem_cache_destroy(dev_priv->slab);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a6354c3..8c93d93 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -406,6 +406,8 @@ struct intel_uncore {
>  
>  	unsigned fifo_count;
>  	unsigned forcewake_count;
> +
> +	struct delayed_work force_wake_work;
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> @@ -1792,6 +1794,7 @@ extern void intel_uncore_early_sanitize(struct drm_device *dev);
>  extern void intel_uncore_init(struct drm_device *dev);
>  extern void intel_uncore_clear_errors(struct drm_device *dev);
>  extern void intel_uncore_check_errors(struct drm_device *dev);
> +extern void intel_uncore_fini(struct drm_device *dev);
>  
>  void
>  i915_enable_pipestat(drm_i915_private_t *dev_priv, int pipe, u32 mask);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 8f5bc86..50fdad7 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -204,6 +204,18 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv)
>  	gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> +static void gen6_force_wake_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, typeof(*dev_priv), uncore.force_wake_work.work);
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +	if (--dev_priv->uncore.forcewake_count == 0)
> +		dev_priv->uncore.funcs.force_wake_put(dev_priv);
> +	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +}
> +
>  void intel_uncore_early_sanitize(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -216,6 +228,9 @@ void intel_uncore_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	INIT_DELAYED_WORK(&dev_priv->uncore.force_wake_work,
> +			  gen6_force_wake_work);
> +
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = vlv_force_wake_get;
>  		dev_priv->uncore.funcs.force_wake_put = vlv_force_wake_put;
> @@ -261,6 +276,13 @@ 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;
> +
> +	cancel_delayed_work_sync(&dev_priv->uncore.force_wake_work);

Aren't we potentially leaking a forcewake here? Or do we clear it
forcefully somewhere else?

> +}
> +
>  void intel_uncore_sanitize(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -301,8 +323,15 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>  	unsigned long irqflags;
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -	if (--dev_priv->uncore.forcewake_count == 0)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv);
> +	if (--dev_priv->uncore.forcewake_count == 0) {
> +		if (dev_priv->uncore.force_wake_work.work.func) {
> +			dev_priv->uncore.forcewake_count++;
> +			mod_delayed_work(dev_priv->wq,
> +					 &dev_priv->uncore.force_wake_work,
> +					 1);
> +		} else
> +			dev_priv->uncore.funcs.force_wake_put(dev_priv);
> +	}
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC