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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote:
> On Mon, May 09, 2016 at 09:02:33AM +0200, Daniel Vetter wrote:
> > On Mon, May 02, 2016 at 11:58:38AM +0100, Chris Wilson wrote:
> > > On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote:
> > > > Imo the low-level irq clearing should all be done in the relevant irq
> > > > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can
> > > > have a bikeshed whether the enginer IMR enable/disable functions should be
> > > > together with the clearing or not, placing them in either file is fine.
> > > > But since we already clear the higher-level IMR registers in i915_irq.c we
> > > > might as well clear the low-level ones too in i915_irq.c.
> > > 
> > > That's tricky since that is done before the engines - so how do we get
> > > the various bases to i915_irq.c without duplication? Enabling the irq
> > > for the engines is part of init_hw, so correspondingly putting the
> > > early disable into init looks fine to me.
> > 
> > I just don't particularly like that we have hw access in a function that
> > thus far (and at a glance still after this patch) only sets up software
> > state. I'll probably lead to some ugly surprise. That's why I'd like to
> > move this either to engine->init_hw or to i915_irq.c.
> 
> 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?

Indeed putting it into i915_irq.c seems to not be great since it splits
the gt per-engine mask reg handling too far.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx