[Intel-gfx] [PATCH v2 1/4] drm/i915: Ack interrupts before handling them (GEN5 - GEN7)

[Chris Wilson]

On Mon, Jun 16, 2014 at 03:09:24PM +0100, oscar.mateo at intel.com wrote:
> From: Oscar Mateo <oscar.mateo at 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).
> 
> We maintain the "disable SDE interrupts when handling" hack since apparently it works.
> 
> Spotted by Bob Beckett <robert.beckett at intel.com>.
> 
> v2: Add warning to commit message and comments to the code as per Chris Wilson's request.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5522cbf..7e9fba0 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2147,7 +2147,7 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  	 * do any I915_{READ,WRITE}. */
>  	intel_uncore_check_errors(dev);
>  
> -	/* disable master interrupt before clearing iir  */
> +	/* 1 - Disable master interrupt */

Sorry to be a nuisance, but I was thinking of more of theory of
operation block at the start of the function as well.

/* To handle irqs with the minimum of 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.
 */

Since we have a number of similar irq handlers this can be in a comment
block in just the first handler. And then we leave condensed reminders
in each function. Actually it may work best with this t.o.p. repeated
each time.

>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>  	POSTING_READ(DEIER);
> @@ -2163,37 +2163,43 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg)
>  		POSTING_READ(SDEIER);
>  	}
>  
> +	/* 2 - Find the source(s) of the interrupt. */

/* Find, clear, then process each source of interrupt */

Then drop 3, 4 since we have the overview block and the ordering reminder
here.
>  	gt_iir = I915_READ(GTIIR);
>  	if (gt_iir) {
> +		/* 3 - Clear the Interrupt Identity bits (IIR) before trying to
> +		 * handle the interrupt (we have the IIR safely stored) */
> +		I915_WRITE(GTIIR, gt_iir);
> +		ret = IRQ_HANDLED;
> +		/* 4 - Process the interrupt(s) that had bits set in the IIRs. */
>  		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);
>  		}
>  	}
>  
> +	/* 5 - Re-enable Master Interrupt Control */
/* Finally re-enable the master interrupt */
>  	I915_WRITE(DEIER, de_ier);
>  	POSTING_READ(DEIER);
> +
>  	if (!HAS_PCH_NOP(dev)) {
>  		I915_WRITE(SDEIER, sde_ier);
>  		POSTING_READ(SDEIER);

-- 
Chris Wilson, Intel Open Source Technology Centre