[Intel-gfx] [PATCH 05/10] drm/i915: also disable south interrupts when handling them

[Daniel Vetter]

On Fri, Feb 08, 2013 at 05:35:16PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> From the docs:
>   "Only the rising edge of the PCH Display interrupt will cause the
>   North Display IIR (DEIIR) PCH Display Interrupt even bit to be set,
>   so all PCH Display Interrupts, including back to back interrupts,
>   must be cleared before a new PCH Display interrupt can cause DEIIR
>   to be set".
> 
> The current code works fine because we don't get many interrupts, but
> if we enable the PCH FIFO underrun interrupts we'll start getting so
> many interrupts that at some point new PCH interrupts won't cause
> DEIIR to be set.
> 
> The initial implementation I tried was to turn the code that checks
> SDEIIR into a loop, but we can still get interrupts even after the
> loop is done (and before the irq handler finishes), so we have to
> either disable the interrupts or mask them. In the end I concluded
> that just disabling the PCH interrupts is enough, you don't even need
> the loop, so this is what this patch implements. I've tested it and it
> passes the 2 "PCH FIFO underrun interrupt storms" I can reproduce:
> the "ironlake_crtc_disable" case and the "wrong watermarks" case.
> 
> In other words, here's how to reproduce the problem fixed by this
> patch:
>   1 - Enable PCH FIFO underrun interrupts (SERR_INT on SNB+)
>   2 - Boot the machine
>   3 - While booting we'll get tons of PCH FIFO underrun interrupts
>   4 - Plug a new monitor
>   5 - Run xrandr, notice it won't detect the new monitor
>   6 - Read SDEIIR and notice it's not 0 while DEIIR is 0
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

This smells fishy. Looking at the code, clearing the SDEIIR before the
DEIIR looks strange. If we immediately get another south interrupt, it
will propagate to the DEIIR. But since we currently processing south
interrupts the corresponding bit in DEIIR is set, so when we clear DEIIR
later on, we kill that south interrupt. And since it's then once lost,
we'll lose all subsequent ones, too.

So have you tried clearing DEIIR before clearing any of the subordinate
registers?

The patch itself is imo wrong, since afaik the hw will drop any interrupts
if we disable them. Hence any south interrupt happening while we are in
the irq handler will effectively be lost.

And for the underrun reporting irq storm, I guess we need to first have
infrastructure to block out interrupts while the pipe is going down before
we can enable them. Assuming that pipe underruns are expected in that
case.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f096ad9..500fd65 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -701,7 +701,7 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> -	u32 de_iir, gt_iir, de_ier, pm_iir;
> +	u32 de_iir, gt_iir, de_ier, pm_iir, sde_ier;
>  	irqreturn_t ret = IRQ_NONE;
>  	int i;
>  
> @@ -711,6 +711,10 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>  
> +	sde_ier = I915_READ(SDEIER);
> +	I915_WRITE(SDEIER, 0);
> +	POSTING_READ(SDEIER);
> +
>  	gt_iir = I915_READ(GTIIR);
>  	if (gt_iir) {
>  		snb_gt_irq_handler(dev, dev_priv, gt_iir);
> @@ -759,6 +763,8 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  
>  	I915_WRITE(DEIER, de_ier);
>  	POSTING_READ(DEIER);
> +	I915_WRITE(SDEIER, sde_ier);
> +	POSTING_READ(SDEIER);
>  
>  	return ret;
>  }
> @@ -778,7 +784,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	struct drm_device *dev = (struct drm_device *) arg;
>  	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>  	int ret = IRQ_NONE;
> -	u32 de_iir, gt_iir, de_ier, pm_iir;
> +	u32 de_iir, gt_iir, de_ier, pm_iir, sde_ier;
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
> @@ -787,6 +793,10 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>  	POSTING_READ(DEIER);
>  
> +	sde_ier = I915_READ(SDEIER);
> +	I915_WRITE(SDEIER, 0);
> +	POSTING_READ(SDEIER);
> +
>  	de_iir = I915_READ(DEIIR);
>  	gt_iir = I915_READ(GTIIR);
>  	pm_iir = I915_READ(GEN6_PMIIR);
> @@ -849,6 +859,8 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  done:
>  	I915_WRITE(DEIER, de_ier);
>  	POSTING_READ(DEIER);
> +	I915_WRITE(SDEIER, sde_ier);
> +	POSTING_READ(SDEIER);
>  
>  	return ret;
>  }
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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