[Intel-gfx] [PATCH 2/2] drm/i915: Protect engine request list with spinlock

[Chris Wilson]

On Tue, Feb 24, 2015 at 01:23:27PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > On Tue, Feb 24, 2015 at 11:39:08AM +0100, Daniel Vetter wrote:
> >> On Tue, Feb 24, 2015 at 08:31:18AM +0000, Chris Wilson wrote:
> >> > On Tue, Feb 24, 2015 at 12:58:19AM +0100, Daniel Vetter wrote:
> >> > > On Thu, Feb 19, 2015 at 04:41:12PM +0000, Chris Wilson wrote:
> >> > > > On Thu, Feb 19, 2015 at 06:18:55PM +0200, Mika Kuoppala wrote:
> >> > > > > There are multiple players interested in the ring->request_list
> >> > > > > state. Request submission can happen in kernel or user context,
> >> > > > > idle worker is going through request list to free items. And then there
> >> > > > > is hangcheck worker which tries to figure out if particular ring is
> >> > > > > healthy by peeking at the request list among other things. And if
> >> > > > > judged stuck by hangcheck, error state is colleted. Which in turns
> >> > > > > needs access to ring->request_list.
> >> > > > 
> >> > > > We have discussed this before. Hangcheck does not need the lock so long
> >> > > > as it is serialised with deletion. List processing with hangcheck during
> >> > > > concurrent addition is safe.
> >> > > > 
> >> > > > For example, I expect the request locking to look like
> >> > > > 
> >> > > > http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_gem_request.c#n691
> >> > > 
> >> > > I think longer-term with per-engine reset and fun stuff like that we
> >> > > probably want the spinlock, just to avoid too many headaches with locking
> >> > > auditing. For the execbuf fastpath it should just be one more spinlock per
> >> > > ioctl, so hopefully bearable.
> >> > 
> >> > But it is not even the locking bug that breaks capture, so what's the
> >> > point?
> >> 
> >> Oh I've read the patch as general prep work for more finegrained reset
> >> support not as a fix for the referenced bug. I guess the bug is just the
> >> usual incoherent seqno/irq thing that's been plagueing us ever since gen6?
> >
> > I presumed Mika wants to fix that hangcheck and capture may explode as
> > requests are completed concurrently. The bug that I expect will remain
> > is that we peek at the bo without locks during capture.
> > -Chris
> >
> 
> What I think is the failure mode on [1] is:
> 
> Request gets added to the ring but not yet
> into the ring->request_list, gpu finishes it and updates
> the hw status page. Hangcheck runs and sees that request_list
> does not contain the supposed request and complains
> that the hangcheck was activated on idle ring.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=88651

Oh, the easiest way to prevent that was to force hangcheck to go through
two passes before declaring a missed interrupt - and that there were no
interrupts in the meantime.

http://cgit.freedesktop.org/~ickle/linux-2.6/tree/drivers/gpu/drm/i915/i915_irq.c#n2973
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre