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

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	oscar.mateo@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/4] drm/i915: Ack interrupts before handling them (GEN5 - GEN7)
Date: Mon, 16 Jun 2014 20:10:42 +0200	[thread overview]
Message-ID: <20140616181042.GH5821@phenom.ffwll.local> (raw)
In-Reply-To: <20140616120737.GA7320@nuc-i3427.alporthouse.com>

On Mon, Jun 16, 2014 at 01:07:37PM +0100, Chris Wilson wrote:
> On Mon, Jun 16, 2014 at 12:30:26PM +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.
> 
> Without a atomic xchg operation with mmio space, we merely reduce the
> window. This is even more apparent when you consider how heavyweight
> those I915_READ/I915_WRITE are.

I think the correct description of the race is:

If we handle the irq source before resetting the irq signal bit then
there's a race between the handling and the resetting where a new
interrupt can get lost. This is important where interrupts can happen
quickly and have some status (like seqno or other completion number)
attached.

If we on the other hand handle the irq (i.e. read the seqno/cookie) after
clearing the interrupt we might get a spurious interrupt occasionally
(i.e. one for a cookie value we've seen already), but we'll never miss the
interrupt for a cookie update.

This fix has the potential to explain tons of the missed interrupt fun
we've seen in the past years all over the place (render, rps, ...).
-Daniel

> 
> > According to BSPec, the right order should be:
> > 
> > 1 - Disable Master Interrupt Control.
> > 2 - Find the source(s) of the interrupt and clear the Interrupt Identity bits (IIR)
> > 4 - Process the interrupt(s) that had bits set in the IIRs.
> > 5 - Re-enable Master Interrupt Control.
> > 
> > We maintain the "disable SDE interrupts when handling" hack since apparently it works.
> > 
> > Spotted by Bob Beckett <robert.beckett@intel.com>.
> > 
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> 
> I'd like the changelog slightly clarified, and the notes here would be
> useful in the code as well to describe the theory of operation in
> handling IRQ.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@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