[Intel-gfx] [PATCH v3] drm/i915: avoid processing spurious/shared interrupts in low-power states

[Daniel Vetter]

On Mon, Feb 23, 2015 at 04:31:49PM +0200, Imre Deak wrote:
> Atm, it's possible that the interrupt handler is called when the device
> is in D3 or some other low-power state. It can be due to another device
> that is still in D0 state and shares the interrupt line with i915, or on
> some platforms there could be spurious interrupts even without sharing
> the interrupt line. The latter case was reported by Klaus Ethgen using a
> Lenovo x61p machine (gen 4). He noticed this issue via a system
> suspend/resume hang and bisected it to the following commit:
> 
> commit e11aa362308f5de467ce355a2a2471321b15a35c
> Author: Jesse Barnes <jbarnes at virtuousgeek.org>
> Date:   Wed Jun 18 09:52:55 2014 -0700
> 
>     drm/i915: use runtime irq suspend/resume in freeze/thaw
> 
> This is a problem, since in low-power states IIR will always read
> 0xffffffff resulting in an endless IRQ servicing loop.
> 
> Fix this by handling interrupts only when the driver explicitly enables
> them and so it's guaranteed that the interrupt registers return a valid
> value.
> 
> Note that this issue existed even before the above commit, since during
> runtime suspend/resume we never unregistered the handler.
> 
> v2:
> - clarify the purpose of smp_mb() vs. synchronize_irq() in the
>   code comment (Chris)
> 
> v3:
> - no need for an explicit smp_mb(), we can assume that synchronize_irq()
>   and the mmio read/writes in the install hooks provide for this (Daniel)
> - remove code comment as the remaining synchronize_irq() is self
>   explanatory (Daniel)
> 
> Reference: https://lkml.org/lkml/2015/2/11/205
> Reported-and-bisected-by: Klaus Ethgen <Klaus at Ethgen.ch>
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9073119..612c9c0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1889,6 +1889,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  	u32 iir, gt_iir, pm_iir;
>  	irqreturn_t ret = IRQ_NONE;
>  
> +	if (!intel_irqs_enabled(dev_priv))
> +		return IRQ_NONE;
> +
>  	while (true) {
>  		/* Find, clear, then process each source of interrupt */
>  
> @@ -1933,6 +1936,9 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg)
>  	u32 master_ctl, iir;
>  	irqreturn_t ret = IRQ_NONE;
>  
> +	if (!intel_irqs_enabled(dev_priv))
> +		return IRQ_NONE;
> +
>  	for (;;) {
>  		master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL;
>  		iir = I915_READ(VLV_IIR);
> @@ -2205,6 +2211,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
>  	irqreturn_t ret = IRQ_NONE;
>  
> +	if (!intel_irqs_enabled(dev_priv))
> +		return IRQ_NONE;
> +
>  	/* We get interrupts on unclaimed registers, so check for this before we
>  	 * do any I915_{READ,WRITE}. */
>  	intel_uncore_check_errors(dev);
> @@ -2276,6 +2285,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>  	enum pipe pipe;
>  	u32 aux_mask = GEN8_AUX_CHANNEL_A;
>  
> +	if (!intel_irqs_enabled(dev_priv))
> +		return IRQ_NONE;
> +
>  	if (IS_GEN9(dev))
>  		aux_mask |=  GEN9_AUX_CHANNEL_B | GEN9_AUX_CHANNEL_C |
>  			GEN9_AUX_CHANNEL_D;
> @@ -3768,6 +3780,9 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
>  		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
>  		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
>  
> +	if (!intel_irqs_enabled(dev_priv))
> +		return IRQ_NONE;
> +
>  	iir = I915_READ16(IIR);
>  	if (iir == 0)
>  		return IRQ_NONE;
> @@ -3948,6 +3963,9 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
>  	int pipe, ret = IRQ_NONE;
>  
> +	if (!intel_irqs_enabled(dev_priv))
> +		return IRQ_NONE;
> +
>  	iir = I915_READ(IIR);
>  	do {
>  		bool irq_received = (iir & ~flip_mask) != 0;
> @@ -4168,6 +4186,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  		I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT |
>  		I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
>  
> +	if (!intel_irqs_enabled(dev_priv))
> +		return IRQ_NONE;
> +
>  	iir = I915_READ(IIR);
>  
>  	for (;;) {
> @@ -4504,6 +4525,7 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
>  	drm_irq_uninstall(dev_priv->dev);
>  	intel_hpd_cancel_work(dev_priv);
>  	dev_priv->pm.irqs_enabled = false;
> +	synchronize_irq(dev_priv->dev->irq);

drm_irq_uninstall calls free_irq which means there's no way at all for our
handler to still run. The synchronize_irq here is hence redundnant. With
that hunk removed this is

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

>  }
>  
>  /**
> @@ -4517,6 +4539,7 @@ void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv)
>  {
>  	dev_priv->dev->driver->irq_uninstall(dev_priv->dev);
>  	dev_priv->pm.irqs_enabled = false;
> +	synchronize_irq(dev_priv->dev->irq);
>  }
>  
>  /**
> -- 
> 2.1.0
> 

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