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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
> +}
> +
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
