Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup
Date: Tue, 10 May 2016 08:50:24 +0100	[thread overview]
Message-ID: <20160510075024.GA24535@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20160510074605.GJ27098@phenom.ffwll.local>

On Tue, May 10, 2016 at 09:46:05AM +0200, Daniel Vetter wrote:
> On Mon, May 09, 2016 at 11:41:41AM +0100, Chris Wilson wrote:
> > On Mon, May 09, 2016 at 09:58:20AM +0200, Daniel Vetter wrote:
> > > On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote:
> > > > This is sanitize. We do enable it in engine->init_hw(), but the point
> > > > raised by Ville earlier in his review of GT irq handling is that nobody
> > > > currently disables the ring IMR before use. Here we have a
> > > > chicken-and-egg problem, do we duplicate knowledge of available engines
> > > > (and their mmio_base) in irq preinstall/sanitize or do we do the engine
> > > > specific mmio in engine initialisation? The problem Turslin was raising
> > > > was that on future enabling, somebody had enabled the engine IRQ before
> > > > the engines were initialised (i.e. had completely disregarded the
> > > > current init_hw sequence). Plonking it in i915_irq.c is not foolproof
> > > > either!
> > > 
> > > Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level
> > > interrupts, but for GT stuff all masked. In init_hw we could clear that
> > > then, and before init_hw no one should call ring->get_irq to enable it and
> > > potentially cause havoc. Or still too fragile in your opinion?
> > 
> > The race is if we get an interrupt inside init_engine, after we set
> > engine->dev but before we setup the state for the irq handler. (Note the
> > race isn't strictly just dev, everything we touch inside the irq handler
> > gives arise to a potential ordering issue.)
> 
> But how does this happen? Assuming we did mask all the higher bits
> correctly beforehand ... Is this just theoretical (in which case I think
> cleanup in init_hw is totally fine), or did it go kaboom already?

We haven't masked all the bits correctly beforehand. We don't today, and
tomorrow someone thought it would be fun to enable the GT interrupts
before the engines were initialised. Still, it can only go bang if the
interrupt was asserted - so a particularly interesting bios or kexec /
hibernation scenario.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx