[Intel-gfx] [PATCH v2] drm/i915/execlists: Refactor common engine setup

[Daniel Vetter]

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?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch