Re: [PATCH v3 1/4] drm/i915: Ack interrupts before handling them (GEN5 - GEN7)

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3486 bytes --]

On Mon, 2014-06-16 at 16:10 +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> Otherwise, we might receive a new interrupt before we have time to ack the first
> one, eventually missing it.
> 
> According to BSPec, the right order should be:
> 
> 1 - Disable Master Interrupt Control.
> 2 - Find the source(s) of the interrupt.
> 3 - Clear the Interrupt Identity bits (IIR).
> 4 - Process the interrupt(s) that had bits set in the IIRs.
> 5 - Re-enable Master Interrupt Control.
> 
> Without an atomic XCHG operation with mmio space, the above merely reduces the window
> in which we can miss an interrupt (especially when you consider how heavyweight the
> I915_READ/I915_WRITE operations are).

I can see how we can miss a second, third etc. back-to-back interrupt,
but that's always a problem with edge triggered interrupts. But the
rearranging done in this patchset closes the race where we are left with
a pending interrupt flag without ever calling its handler.

> We maintain the "disable SDE interrupts when handling" hack since apparently it works.
> 
> Spotted by Bob Beckett <robert.beckett@intel.com>.
> 
> v2: Add warning to commit message and comments to the code as per Chris Wilson's request.
> v3: Improve the source comments.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

I couldn't spot any problems, so on all 4 patches:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5522cbf..a68f68c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2136,6 +2136,14 @@ static void ivb_display_irq_handler(struct drm_device *dev, u32 de_iir)
>  	}
>  }
>  
> +/*
> + * To handle irqs with the minimum potential races with fresh interrupts, we:
> + * 1 - Disable Master Interrupt Control.
> + * 2 - Find the source(s) of the interrupt.
> + * 3 - Clear the Interrupt Identity bits (IIR).
> + * 4 - Process the interrupt(s) that had bits set in the IIRs.
> + * 5 - Re-enable Master Interrupt Control.
> + */
>  static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = arg;
> @@ -2163,32 +2171,34 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  		POSTING_READ(SDEIER);
>  	}
>  
> +	/* Find, clear, then process each source of interrupt */
> +
>  	gt_iir = I915_READ(GTIIR);
>  	if (gt_iir) {
> +		I915_WRITE(GTIIR, gt_iir);
> +		ret = IRQ_HANDLED;
>  		if (INTEL_INFO(dev)->gen >= 6)
>  			snb_gt_irq_handler(dev, dev_priv, gt_iir);
>  		else
>  			ilk_gt_irq_handler(dev, dev_priv, gt_iir);
> -		I915_WRITE(GTIIR, gt_iir);
> -		ret = IRQ_HANDLED;
>  	}
>  
>  	de_iir = I915_READ(DEIIR);
>  	if (de_iir) {
> +		I915_WRITE(DEIIR, de_iir);
> +		ret = IRQ_HANDLED;
>  		if (INTEL_INFO(dev)->gen >= 7)
>  			ivb_display_irq_handler(dev, de_iir);
>  		else
>  			ilk_display_irq_handler(dev, de_iir);
> -		I915_WRITE(DEIIR, de_iir);
> -		ret = IRQ_HANDLED;
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		u32 pm_iir = I915_READ(GEN6_PMIIR);
>  		if (pm_iir) {
> -			gen6_rps_irq_handler(dev_priv, pm_iir);
>  			I915_WRITE(GEN6_PMIIR, pm_iir);
>  			ret = IRQ_HANDLED;
> +			gen6_rps_irq_handler(dev_priv, pm_iir);
>  		}
>  	}
>  


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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