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

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

On Mon, Aug 26, 2013 at 12:06:43PM +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.
> 
> v2: Fix teardown ordering - unmap the regs after turning off forcewake,
> and make sure we do turn off forcewake - both found by Ville.
> 
> Note: I have no claims for improved performance, stablity or power
> comsumption for this patch. We should not be hitting the registers often
> enough for this to improve benchmarks, but given the nature of our hw it
> is likely to improve long term stability.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_dma.c     |  6 ++++--
>  drivers/gpu/drm/i915/i915_drv.h     |  3 +++
>  drivers/gpu/drm/i915/intel_uncore.c | 30 ++++++++++++++++++++++++++++--
>  3 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 883990f..97a6e22 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1787,8 +1787,6 @@ int i915_driver_unload(struct drm_device *dev)
>  	list_del(&dev_priv->gtt.base.global_link);
>  	WARN_ON(!list_empty(&dev_priv->vm_list));
>  	drm_mm_takedown(&dev_priv->gtt.base.mm);
> -	if (dev_priv->regs != NULL)
> -		pci_iounmap(dev->pdev, dev_priv->regs);
>  
>  	drm_vblank_cleanup(dev);
>  
> @@ -1800,6 +1798,10 @@ int i915_driver_unload(struct drm_device *dev)
>  
>  	dev_priv->gtt.base.cleanup(&dev_priv->gtt.base);
>  
> +	intel_uncore_fini(dev);
> +	if (dev_priv->regs != NULL)
> +		pci_iounmap(dev->pdev, dev_priv->regs);
> +
>  	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 8649f1c..462cc7f 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;
> +
> +	flush_delayed_work(&dev_priv->uncore.force_wake_work);

A bit orthogonal, but maybe we should also add
'WARN_ON(dev_priv->uncore.forcewake_count)' just to make sure we
don't leave forcewake enabled by accident? Or if we're really
paranoid, maybe even 'if (WARN_ON(...)) force_wake_reset()'.

But anyways, the patch looks like it should do what it claims, and
I can't see other bugs, so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +}
> +
>  static void intel_uncore_forcewake_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -306,8 +328,12 @@ 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) {
> +		dev_priv->uncore.forcewake_count++;
> +		mod_delayed_work(dev_priv->wq,
> +				 &dev_priv->uncore.force_wake_work,
> +				 1);
> +	}
>  	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