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: Mon, 9 May 2016 08:45:16 +0100	[thread overview]
Message-ID: <20160509074516.GB12277@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20160509070233.GM27098@phenom.ffwll.local>

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!
-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