Re: [PATCH 4/5] drm/i915: move gen6 rps handling to workqueue

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 20 Apr 2011 16:53:18 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> The render P-state handling code requires reading from a GT register.
> This means that FORCEWAKE must be written to, a resource which is shared
> and should be protected by struct_mutex.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |    1 +
>  drivers/gpu/drm/i915/i915_drv.h |    4 +++
>  drivers/gpu/drm/i915/i915_irq.c |   47 +++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_reg.h |    5 +++-
>  4 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..855355e 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2025,6 +2025,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	spin_lock_init(&dev_priv->irq_lock);
>  	spin_lock_init(&dev_priv->error_lock);
> +	spin_lock_init(&dev_priv->rps_lock);
>  
>  	if (IS_MOBILE(dev) || !IS_GEN2(dev))
>  		dev_priv->num_pipe = 2;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1cd5a76..4faa7e5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -676,6 +676,10 @@ typedef struct drm_i915_private {
>  
>  	bool mchbar_need_disable;
>  
> +	struct work_struct rps_work;
> +	spinlock_t rps_lock;
> +	u32 pm_iir;
> +
>  	u8 cur_delay;
>  	u8 min_delay;
>  	u8 max_delay;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c6062c3..2d9f751 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -367,22 +367,30 @@ static void notify_ring(struct drm_device *dev,
>  		  jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
>  }
>  
> -static void gen6_pm_irq_handler(struct drm_device *dev)
> +static void gen6_pm_rps_work(struct work_struct *work)
>  {
> -	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> +	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> +						    rps_work);
>  	u8 new_delay = dev_priv->cur_delay;
> -	u32 pm_iir;
> +	u32 pm_iir, pm_imr;
> +
> +	spin_lock_irq(&dev_priv->rps_lock);
> +	pm_iir = dev_priv->pm_iir;
> +	dev_priv->pm_iir = 0;
> +	pm_imr = I915_READ(GEN6_PMIMR);
> +	spin_unlock_irq(&dev_priv->rps_lock);
>  
> -	pm_iir = I915_READ(GEN6_PMIIR);
>  	if (!pm_iir)
>  		return;
>  
> +	mutex_lock(&dev_priv->dev->struct_mutex);
>  	if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
>  		if (dev_priv->cur_delay != dev_priv->max_delay)
>  			new_delay = dev_priv->cur_delay + 1;
>  		if (new_delay > dev_priv->max_delay)
>  			new_delay = dev_priv->max_delay;
>  	} else if (pm_iir & (GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT)) {
> +		gen6_gt_force_wake_get(dev_priv);
>  		if (dev_priv->cur_delay != dev_priv->min_delay)
>  			new_delay = dev_priv->cur_delay - 1;
>  		if (new_delay < dev_priv->min_delay) {
> @@ -396,13 +404,18 @@ static void gen6_pm_irq_handler(struct drm_device *dev)
>  			I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
>  				   I915_READ(GEN6_RP_INTERRUPT_LIMITS) & ~0x3f0000);
>  		}
> +		gen6_gt_force_wake_put(dev_priv);
>  	}
>  
> -	gen6_set_rps(dev, new_delay);
> +	gen6_set_rps(dev_priv->dev, new_delay);
>  	dev_priv->cur_delay = new_delay;
> +	mutex_unlock(&dev_priv->dev->struct_mutex);
>  
> -	I915_WRITE(GEN6_PMIIR, pm_iir);
> -}
> +	/*
> +	 * Lock not held here, because clearing is non-destructive, and
> +	 * the interrupt handler is the only other place where it is written.
> +	 */
> +	I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir); }

But does this register use __gen6_gt_wait_for_fifo? *That* requires a
lock. I'm starting to see a need for I915_READ_IRQ, I915_WRITE_IRQ, so
that the circumstances are explicit and we acknowledge them when modifying
the read/write routines.

>  static void pch_irq_handler(struct drm_device *dev)
>  {
> @@ -525,13 +538,28 @@ static irqreturn_t ironlake_irq_handler(struct drm_device *dev)
>  		i915_handle_rps_change(dev);
>  	}
>  
> -	if (IS_GEN6(dev))
> -		gen6_pm_irq_handler(dev);
> +	if (IS_GEN6(dev) && pm_iir)  {
> +		if (pm_iir & (GEN6_PM_DEFERRED_EVENTS)) {

	if (IS_GEN6(dev) && pm_iir & GEN6_PM_DEFERRED_EVENTS) {

roll the two ifs into one to remove a surplus level of nesting and kill
the redundant brackets!

> +			unsigned long flags;
> +			/* Make sure no new interrupts come in */
> +			spin_lock_irqsave(&dev_priv->rps_lock, flags);
> +			I915_WRITE(GEN6_PMIMR, pm_iir);
> +
> +			/* Add the new ones */
> +			BUG_ON(dev_priv->pm_iir & pm_iir);

Bah. The comments are absolutely useless. Really. What you need to
describe is how the use of IMR and IIR is split between the interrupt
handler and the tasklet.

Or maybe they did their job, because I had to go back and read much more
carefully what you were doing...

> +			dev_priv->pm_iir |= pm_iir;
> +			spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
> +
> +			/* queue it up */
> +			queue_work(dev_priv->wq, &dev_priv->rps_work);
> +		}
> +	}
>  
>  	/* should clear PCH hotplug event before clear CPU irq */
>  	I915_WRITE(SDEIIR, pch_iir);
>  	I915_WRITE(GTIIR, gt_iir);
>  	I915_WRITE(DEIIR, de_iir);
> +	I915_WRITE(GEN6_PMIIR, pm_iir);
>  
>  done:
>  	I915_WRITE(DEIER, de_ier);
> @@ -1658,6 +1686,7 @@ void i915_driver_irq_preinstall(struct drm_device * dev)
>  
>  	INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
>  	INIT_WORK(&dev_priv->error_work, i915_error_work_func);
> +	INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>  
>  	if (HAS_PCH_SPLIT(dev)) {
>  		ironlake_irq_preinstall(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f39ac3a..9774a2e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3386,7 +3386,7 @@
>  #define GEN6_PMINTRMSK				0xA168
>  
>  #define GEN6_PMISR				0x44020
> -#define GEN6_PMIMR				0x44024
> +#define GEN6_PMIMR				0x44024 /* Protected by rps_lock */
>  #define GEN6_PMIIR				0x44028
>  #define GEN6_PMIER				0x4402C
>  #define  GEN6_PM_MBOX_EVENT			(1<<25)
> @@ -3396,6 +3396,9 @@
>  #define  GEN6_PM_RP_DOWN_THRESHOLD		(1<<4)
>  #define  GEN6_PM_RP_UP_EI_EXPIRED		(1<<2)
>  #define  GEN6_PM_RP_DOWN_EI_EXPIRED		(1<<1)
> +#define  GEN6_PM_DEFERRED_EVENTS		(GEN6_PM_RP_UP_THRESHOLD | \
> +						 GEN6_PM_RP_DOWN_THRESHOLD | \
> +						 GEN6_PM_RP_DOWN_TIMEOUT)
>  
>  #define GEN6_PCODE_MAILBOX			0x138124
>  #define   GEN6_PCODE_READY			(1<<31)
> -- 
> 1.7.3.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre