Re: [PATCH v4] drm/i915: avoid processing spurious/shared interrupts in low-power states

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 24 Feb 2015, Imre Deak <imre.deak@intel.com> 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@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)
>
> v4:
> - drm_irq_uninstall() implies synchronize_irq(), so no need to call it
>   explicitly (Daniel)
>
> Reference: https://lkml.org/lkml/2015/2/11/205
> Reported-and-bisected-by: Klaus Ethgen <Klaus@Ethgen.ch>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pushed to drm-intel-fixes, cc: stable, thanks for the patch and review.

BR,
Jani.


> ---
>  drivers/gpu/drm/i915/i915_irq.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9073119..1561248 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 (;;) {
> @@ -4517,6 +4538,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
>

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