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

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

[Chris Wilson]

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);
+}
+
 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

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

[Ville Syrjälä]

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

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

[Ben Widawsky]

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.

I don't understand how or why but from casual powertop observation, this
workqueue uses between 4x and 50x or the nearest other i915 workqueue
(i915_gem_retire_work_handler). On my x240...

> 
> 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);
> +}
> +
>  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

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Chris Wilson]

On Sun, Feb 23, 2014 at 12:12:25PM -0800, Ben Widawsky wrote:
> 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.
> 
> I don't understand how or why but from casual powertop observation, this
> workqueue uses between 4x and 50x or the nearest other i915 workqueue
> (i915_gem_retire_work_handler). On my x240...

What does that mean? We expect this to be frequently hit since we use it
so often, but retire_work_handler is only run every couple of seconds to
trim our lists (in case of userspace failure).

The idea is to have the forcewake reset run during the next scheduler event,
so using a workqueue was probably the wrong choice and perhaps we should have
used a timer instead?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Ben Widawsky]

On Mon, Feb 24, 2014 at 08:30:22AM +0000, Chris Wilson wrote:
> On Sun, Feb 23, 2014 at 12:12:25PM -0800, Ben Widawsky wrote:
> > 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.
> > 
> > I don't understand how or why but from casual powertop observation, this
> > workqueue uses between 4x and 50x or the nearest other i915 workqueue
> > (i915_gem_retire_work_handler). On my x240...
> 
> What does that mean? We expect this to be frequently hit since we use it
> so often, but retire_work_handler is only run every couple of seconds to
> trim our lists (in case of userspace failure).
> 
> The idea is to have the forcewake reset run during the next scheduler event,
> so using a workqueue was probably the wrong choice and perhaps we should have
> used a timer instead?
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

I just posted it for the sake of potentially starting a conversation. I
assume we always expected power consumption to go up since [it would
seem at least] we're usually just deferring the release of the last
put(). But then we have the race to idle argument in there as well.

In any event, I wasn't asking for any change, was just surprised by the
data.


-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915: Convert the forcewake worker into a timer func

[Chris Wilson]

We don't want to suffer scheduling delay when turning off the GPU after
waking it up to touch registers. Ideally, we only want to keep the GPU
awake for the register access sequence, with a single forcewake dance on
the first access and release immediately after the last. We set a timer
on the first access so that we only dance once and on the next scheduler
tick, we drop the forcewake again.

This moves the cleanup routine from the common i915 workqueue to a timer
func so that we don't anger powertop, and drop the forcewake again
quicker.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h     |    2 +-
 drivers/gpu/drm/i915/intel_uncore.c |   15 ++++++---------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eb31c45..9416b03 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -508,7 +508,7 @@ struct intel_uncore {
 	unsigned fw_rendercount;
 	unsigned fw_mediacount;
 
-	struct delayed_work force_wake_work;
+	struct timer_list force_wake_timer;
 };
 
 #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 c628414..140a9f0 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -289,10 +289,8 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-static void gen6_force_wake_work(struct work_struct *work)
+static void gen6_force_wake_timer(struct drm_i915_private *dev_priv)
 {
-	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);
@@ -405,9 +403,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	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);
+		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
+				 jiffies + 1);
 	}
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
@@ -681,8 +678,8 @@ 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);
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);
 
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
@@ -794,7 +791,7 @@ 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);
+	del_timer_sync(&dev_priv->uncore.force_wake_timer);
 
 	/* Paranoia: make sure we have disabled everything before we exit. */
 	intel_uncore_sanitize(dev);
-- 
1.7.9.5

Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

[Ville Syrjälä]

On Fri, Feb 28, 2014 at 09:02:38AM +0000, Chris Wilson wrote:
> We don't want to suffer scheduling delay when turning off the GPU after
> waking it up to touch registers. Ideally, we only want to keep the GPU
> awake for the register access sequence, with a single forcewake dance on
> the first access and release immediately after the last. We set a timer
> on the first access so that we only dance once and on the next scheduler
> tick, we drop the forcewake again.
> 
> This moves the cleanup routine from the common i915 workqueue to a timer
> func so that we don't anger powertop, and drop the forcewake again
> quicker.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |    2 +-
>  drivers/gpu/drm/i915/intel_uncore.c |   15 ++++++---------
>  2 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index eb31c45..9416b03 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -508,7 +508,7 @@ struct intel_uncore {
>  	unsigned fw_rendercount;
>  	unsigned fw_mediacount;
>  
> -	struct delayed_work force_wake_work;
> +	struct timer_list force_wake_timer;
>  };
>  
>  #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 c628414..140a9f0 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -289,10 +289,8 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> -static void gen6_force_wake_work(struct work_struct *work)
> +static void gen6_force_wake_timer(struct drm_i915_private *dev_priv)
>  {
> -	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);
> @@ -405,9 +403,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	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);
> +		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> +				 jiffies + 1);
>  	}
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);

Should we use the timer to delay the forcewake release from register
read/write functions too? Or maybe do it only for register
read/write since gen6_gt_force_wake_get/put are rather rare and I
wouldn't expect significant forcewake ping-pong after the put.

>  
> @@ -681,8 +678,8 @@ 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);
> +	setup_timer(&dev_priv->uncore.force_wake_timer,
> +		    gen6_force_wake_timer, (unsigned long)dev_priv);
>  
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
> @@ -794,7 +791,7 @@ 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);
> +	del_timer_sync(&dev_priv->uncore.force_wake_timer);
>  
>  	/* Paranoia: make sure we have disabled everything before we exit. */
>  	intel_uncore_sanitize(dev);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

[Chris Wilson]

On Fri, Feb 28, 2014 at 11:16:33AM +0200, Ville Syrjälä wrote:
> Should we use the timer to delay the forcewake release from register
> read/write functions too? Or maybe do it only for register
> read/write since gen6_gt_force_wake_get/put are rather rare and I
> wouldn't expect significant forcewake ping-pong after the put.

Yes, yes it should.
/me hides under a rock.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Convert the forcewake worker into a timer func

[Chris Wilson]

We don't want to suffer scheduling delay when turning off the GPU after
waking it up to touch registers. Ideally, we only want to keep the GPU
awake for the register access sequence, with a single forcewake dance on
the first access and release immediately after the last. We set a timer
on the first access so that we only dance once and on the next scheduler
tick, we drop the forcewake again.

This moves the cleanup routine from the common i915 workqueue to a timer
func so that we don't anger powertop, and drop the forcewake again
quicker.

v2: Enable the deferred force_wake_put for regular register reads as
well.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_uncore.c | 34 +++++++++++++++-------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b22765192018..8af8e0dd3943 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -508,7 +508,7 @@ struct intel_uncore {
 	unsigned fw_rendercount;
 	unsigned fw_mediacount;
 
-	struct delayed_work force_wake_work;
+	struct timer_list force_wake_timer;
 };
 
 #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 c62841404c82..8ee171178bfe 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -289,10 +289,8 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-static void gen6_force_wake_work(struct work_struct *work)
+static void gen6_force_wake_timer(struct drm_i915_private *dev_priv)
 {
-	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);
@@ -405,9 +403,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	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);
+		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
+				 jiffies + 1);
 	}
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
@@ -484,17 +481,15 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 static u##x \
 gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	REG_READ_HEADER(x); \
-	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		if (dev_priv->uncore.forcewake_count == 0) \
-			dev_priv->uncore.funcs.force_wake_get(dev_priv, \
-							FORCEWAKE_ALL); \
-		val = __raw_i915_read##x(dev_priv, reg); \
-		if (dev_priv->uncore.forcewake_count == 0) \
-			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
-							FORCEWAKE_ALL); \
-	} else { \
-		val = __raw_i915_read##x(dev_priv, reg); \
+	if (dev_priv->uncore.forcewake_count == 0 && \
+	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
+		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, \
+				 jiffies + 1); \
 	} \
+	val = __raw_i915_read##x(dev_priv, reg); \
 	REG_READ_FOOTER; \
 }
 
@@ -681,8 +676,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);
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    (void (*)(unsigned long))gen6_force_wake_timer,
+		    (unsigned long)dev_priv);
 
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
@@ -794,7 +790,7 @@ 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);
+	del_timer_sync(&dev_priv->uncore.force_wake_timer);
 
 	/* Paranoia: make sure we have disabled everything before we exit. */
 	intel_uncore_sanitize(dev);
-- 
1.9.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

[Ben Widawsky]

On Fri, Feb 28, 2014 at 06:44:03PM +0000, Chris Wilson wrote:
> We don't want to suffer scheduling delay when turning off the GPU after
> waking it up to touch registers. Ideally, we only want to keep the GPU
> awake for the register access sequence, with a single forcewake dance on
> the first access and release immediately after the last. We set a timer
> on the first access so that we only dance once and on the next scheduler
> tick, we drop the forcewake again.
> 
> This moves the cleanup routine from the common i915 workqueue to a timer
> func so that we don't anger powertop, and drop the forcewake again
> quicker.
> 
> v2: Enable the deferred force_wake_put for regular register reads as
> well.

So I went to test this patch today. Oddly this isn't showing up in any
of my profiles on the latest drm-intel-nightly. I just tested today, and
Daniel made it a 3.14-rc4 based thing today. It was still present on
3.13.0. Actually, i915_gem_retire_work_handler is the only one even
showing up in powertop, which is how it should be IMO.

Therefore I'm not sure what we should do with this.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c | 34 +++++++++++++++-------------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b22765192018..8af8e0dd3943 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -508,7 +508,7 @@ struct intel_uncore {
>  	unsigned fw_rendercount;
>  	unsigned fw_mediacount;
>  
> -	struct delayed_work force_wake_work;
> +	struct timer_list force_wake_timer;
>  };
>  
>  #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 c62841404c82..8ee171178bfe 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -289,10 +289,8 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> -static void gen6_force_wake_work(struct work_struct *work)
> +static void gen6_force_wake_timer(struct drm_i915_private *dev_priv)
>  {
> -	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);
> @@ -405,9 +403,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	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);
> +		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> +				 jiffies + 1);
>  	}
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  
> @@ -484,17 +481,15 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  static u##x \
>  gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	REG_READ_HEADER(x); \
> -	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> -							FORCEWAKE_ALL); \
> -		val = __raw_i915_read##x(dev_priv, reg); \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
> -							FORCEWAKE_ALL); \
> -	} else { \
> -		val = __raw_i915_read##x(dev_priv, reg); \
> +	if (dev_priv->uncore.forcewake_count == 0 && \
> +	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> +		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, \
> +				 jiffies + 1); \
>  	} \
> +	val = __raw_i915_read##x(dev_priv, reg); \
>  	REG_READ_FOOTER; \
>  }
>  
> @@ -681,8 +676,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);
> +	setup_timer(&dev_priv->uncore.force_wake_timer,
> +		    (void (*)(unsigned long))gen6_force_wake_timer,
> +		    (unsigned long)dev_priv);
>  
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
> @@ -794,7 +790,7 @@ 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);
> +	del_timer_sync(&dev_priv->uncore.force_wake_timer);
>  
>  	/* Paranoia: make sure we have disabled everything before we exit. */
>  	intel_uncore_sanitize(dev);
> -- 
> 1.9.0
> 

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

[Chris Wilson]

On Sun, Mar 02, 2014 at 05:01:26PM -0800, Ben Widawsky wrote:
> On Fri, Feb 28, 2014 at 06:44:03PM +0000, Chris Wilson wrote:
> > We don't want to suffer scheduling delay when turning off the GPU after
> > waking it up to touch registers. Ideally, we only want to keep the GPU
> > awake for the register access sequence, with a single forcewake dance on
> > the first access and release immediately after the last. We set a timer
> > on the first access so that we only dance once and on the next scheduler
> > tick, we drop the forcewake again.
> > 
> > This moves the cleanup routine from the common i915 workqueue to a timer
> > func so that we don't anger powertop, and drop the forcewake again
> > quicker.
> > 
> > v2: Enable the deferred force_wake_put for regular register reads as
> > well.
> 
> So I went to test this patch today. Oddly this isn't showing up in any
> of my profiles on the latest drm-intel-nightly. I just tested today, and
> Daniel made it a 3.14-rc4 based thing today. It was still present on
> 3.13.0. Actually, i915_gem_retire_work_handler is the only one even
> showing up in powertop, which is how it should be IMO.
> 
> Therefore I'm not sure what we should do with this.

>From one perspective, that sounds promising - if it doesn't cause
powertop to start a panic. This patch is what I wanted for the
forcewake-off delay in the first place, so it is worth
investigating as it is more likely to have an effect. There are still
the regular dropped mmio writes being reported in downstream bugzillas.
:|
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

[Ville Syrjälä]

On Fri, Feb 28, 2014 at 06:44:03PM +0000, Chris Wilson wrote:
> We don't want to suffer scheduling delay when turning off the GPU after
> waking it up to touch registers. Ideally, we only want to keep the GPU
> awake for the register access sequence, with a single forcewake dance on
> the first access and release immediately after the last. We set a timer
> on the first access so that we only dance once and on the next scheduler
> tick, we drop the forcewake again.
> 
> This moves the cleanup routine from the common i915 workqueue to a timer
> func so that we don't anger powertop, and drop the forcewake again
> quicker.
> 
> v2: Enable the deferred force_wake_put for regular register reads as
> well.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c | 34 +++++++++++++++-------------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b22765192018..8af8e0dd3943 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -508,7 +508,7 @@ struct intel_uncore {
>  	unsigned fw_rendercount;
>  	unsigned fw_mediacount;
>  
> -	struct delayed_work force_wake_work;
> +	struct timer_list force_wake_timer;
>  };
>  
>  #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 c62841404c82..8ee171178bfe 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -289,10 +289,8 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> -static void gen6_force_wake_work(struct work_struct *work)
> +static void gen6_force_wake_timer(struct drm_i915_private *dev_priv)
>  {
> -	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);
> @@ -405,9 +403,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	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);
> +		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> +				 jiffies + 1);

This could expire more or less immediately, but it should be fine. We'd
just end up doing two forcewake_get()s instead of one, which should
still be better than >2 if the theory of the timer holds.

>  	}
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  
> @@ -484,17 +481,15 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  static u##x \
>  gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	REG_READ_HEADER(x); \
> -	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> -							FORCEWAKE_ALL); \
> -		val = __raw_i915_read##x(dev_priv, reg); \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
> -							FORCEWAKE_ALL); \
> -	} else { \
> -		val = __raw_i915_read##x(dev_priv, reg); \
> +	if (dev_priv->uncore.forcewake_count == 0 && \
> +	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> +		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, \
> +				 jiffies + 1); \
>  	} \
> +	val = __raw_i915_read##x(dev_priv, reg); \
>  	REG_READ_FOOTER; \
>  }
>  
> @@ -681,8 +676,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);
> +	setup_timer(&dev_priv->uncore.force_wake_timer,
> +		    (void (*)(unsigned long))gen6_force_wake_timer,

I'd prefer to do the required casting in gen6_force_wake_timer(). Seems
a bit less error prone since we'd at least get type checking for the
function pointer.

> +		    (unsigned long)dev_priv);
>  
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
> @@ -794,7 +790,7 @@ 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);
> +	del_timer_sync(&dev_priv->uncore.force_wake_timer);

This could leave force wake enabled.

>  
>  	/* Paranoia: make sure we have disabled everything before we exit. */
>  	intel_uncore_sanitize(dev);
> -- 
> 1.9.0

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

[Chris Wilson]

On Mon, Mar 03, 2014 at 04:46:20PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 28, 2014 at 06:44:03PM +0000, Chris Wilson wrote:
> > @@ -405,9 +403,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >  	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);
> > +		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> > +				 jiffies + 1);
> 
> This could expire more or less immediately, but it should be fine. We'd
> just end up doing two forcewake_get()s instead of one, which should
> still be better than >2 if the theory of the timer holds.

When does the timer actually fire? I am presuming that the callbacks are
run from a soft-interrupt at the next scheduling point. Using a pinned
timer here should mean that the earlier that it could fire is after this
sequence of register writes (or at least in the next mutex_lock() etc).
At any rate, we only care that we reduce the number of times we do the
forcewake dance.

> > @@ -794,7 +790,7 @@ 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);
> > +	del_timer_sync(&dev_priv->uncore.force_wake_timer);
> 
> This could leave force wake enabled.

Bah humbug. Not our problem ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Convert the forcewake worker into a timer func

[Chris Wilson]

We don't want to suffer scheduling delay when turning off the GPU after
waking it up to touch registers. Ideally, we only want to keep the GPU
awake for the register access sequence, with a single forcewake dance on
the first access and release immediately after the last. We set a timer
on the first access so that we only dance once and on the next scheduler
tick, we drop the forcewake again.

This moves the cleanup routine from the common i915 workqueue to a timer
func so that we don't anger powertop, and drop the forcewake again
quicker.

v2: Enable the deferred force_wake_put for regular register reads as
    well.
v3: Beautification and make sure we disable forcewake when shutting
    down.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_uncore.c | 35 ++++++++++++++++-------------------
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b0ab6330b294..bb9ea026310c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -508,7 +508,7 @@ struct intel_uncore {
 	unsigned fw_rendercount;
 	unsigned fw_mediacount;
 
-	struct delayed_work force_wake_work;
+	struct timer_list force_wake_timer;
 };
 
 #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 d1e9d63953c5..e5b59ac5151a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -289,10 +289,9 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
-static void gen6_force_wake_work(struct work_struct *work)
+static void gen6_force_wake_timer(unsigned long arg)
 {
-	struct drm_i915_private *dev_priv =
-		container_of(work, typeof(*dev_priv), uncore.force_wake_work.work);
+	struct drm_i915_private *dev_priv = (void *)arg;
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
@@ -405,9 +404,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 	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);
+		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
+				 jiffies + 1);
 	}
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
@@ -484,17 +482,15 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 static u##x \
 gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	REG_READ_HEADER(x); \
-	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		if (dev_priv->uncore.forcewake_count == 0) \
-			dev_priv->uncore.funcs.force_wake_get(dev_priv, \
-							FORCEWAKE_ALL); \
-		val = __raw_i915_read##x(dev_priv, reg); \
-		if (dev_priv->uncore.forcewake_count == 0) \
-			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
-							FORCEWAKE_ALL); \
-	} else { \
-		val = __raw_i915_read##x(dev_priv, reg); \
+	if (dev_priv->uncore.forcewake_count == 0 && \
+	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
+		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, \
+				 jiffies + 1); \
 	} \
+	val = __raw_i915_read##x(dev_priv, reg); \
 	REG_READ_FOOTER; \
 }
 
@@ -682,8 +678,8 @@ 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);
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);
 
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
@@ -795,10 +791,11 @@ 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);
+	del_timer_sync(&dev_priv->uncore.force_wake_timer);
 
 	/* Paranoia: make sure we have disabled everything before we exit. */
 	intel_uncore_sanitize(dev);
+	intel_uncore_forcewake_reset(dev);
 }
 
 static const struct register_whitelist {
-- 
1.9.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

[Ville Syrjälä]

On Wed, Mar 05, 2014 at 12:00:39PM +0000, Chris Wilson wrote:
> We don't want to suffer scheduling delay when turning off the GPU after
> waking it up to touch registers. Ideally, we only want to keep the GPU
> awake for the register access sequence, with a single forcewake dance on
> the first access and release immediately after the last. We set a timer
> on the first access so that we only dance once and on the next scheduler
> tick, we drop the forcewake again.
> 
> This moves the cleanup routine from the common i915 workqueue to a timer
> func so that we don't anger powertop, and drop the forcewake again
> quicker.
> 
> v2: Enable the deferred force_wake_put for regular register reads as
>     well.
> v3: Beautification and make sure we disable forcewake when shutting
>     down.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks good.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

The next question will be if we want this for VLV too, but that's going
to be a bit more comlicated due to the dual forcewake counts. I guess we
can leave that alone for now.

> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c | 35 ++++++++++++++++-------------------
>  2 files changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b0ab6330b294..bb9ea026310c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -508,7 +508,7 @@ struct intel_uncore {
>  	unsigned fw_rendercount;
>  	unsigned fw_mediacount;
>  
> -	struct delayed_work force_wake_work;
> +	struct timer_list force_wake_timer;
>  };
>  
>  #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 d1e9d63953c5..e5b59ac5151a 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -289,10 +289,9 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
> -static void gen6_force_wake_work(struct work_struct *work)
> +static void gen6_force_wake_timer(unsigned long arg)
>  {
> -	struct drm_i915_private *dev_priv =
> -		container_of(work, typeof(*dev_priv), uncore.force_wake_work.work);
> +	struct drm_i915_private *dev_priv = (void *)arg;
>  	unsigned long irqflags;
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> @@ -405,9 +404,8 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  	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);
> +		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> +				 jiffies + 1);
>  	}
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  
> @@ -484,17 +482,15 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  static u##x \
>  gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	REG_READ_HEADER(x); \
> -	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> -							FORCEWAKE_ALL); \
> -		val = __raw_i915_read##x(dev_priv, reg); \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
> -							FORCEWAKE_ALL); \
> -	} else { \
> -		val = __raw_i915_read##x(dev_priv, reg); \
> +	if (dev_priv->uncore.forcewake_count == 0 && \
> +	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> +		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, \
> +				 jiffies + 1); \
>  	} \
> +	val = __raw_i915_read##x(dev_priv, reg); \
>  	REG_READ_FOOTER; \
>  }
>  
> @@ -682,8 +678,8 @@ 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);
> +	setup_timer(&dev_priv->uncore.force_wake_timer,
> +		    gen6_force_wake_timer, (unsigned long)dev_priv);
>  
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
> @@ -795,10 +791,11 @@ 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);
> +	del_timer_sync(&dev_priv->uncore.force_wake_timer);
>  
>  	/* Paranoia: make sure we have disabled everything before we exit. */
>  	intel_uncore_sanitize(dev);
> +	intel_uncore_forcewake_reset(dev);
>  }
>  
>  static const struct register_whitelist {
> -- 
> 1.9.0

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Convert the forcewake worker into a timer func

[Daniel Vetter]

On Wed, Mar 05, 2014 at 02:21:19PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 05, 2014 at 12:00:39PM +0000, Chris Wilson wrote:
> > We don't want to suffer scheduling delay when turning off the GPU after
> > waking it up to touch registers. Ideally, we only want to keep the GPU
> > awake for the register access sequence, with a single forcewake dance on
> > the first access and release immediately after the last. We set a timer
> > on the first access so that we only dance once and on the next scheduler
> > tick, we drop the forcewake again.
> > 
> > This moves the cleanup routine from the common i915 workqueue to a timer
> > func so that we don't anger powertop, and drop the forcewake again
> > quicker.
> > 
> > v2: Enable the deferred force_wake_put for regular register reads as
> >     well.
> > v3: Beautification and make sure we disable forcewake when shutting
> >     down.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ben Widawsky <ben@bwidawsk.net>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Looks good.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The next question will be if we want this for VLV too, but that's going
> to be a bit more comlicated due to the dual forcewake counts. I guess we
> can leave that alone for now.

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