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

[Daniel Vetter]

On Fri, Apr 29, 2016 at 11:22:59AM +0100, Chris Wilson wrote:
> On Fri, Apr 29, 2016 at 11:11:20AM +0100, Tvrtko Ursulin wrote:
> > 
> > On 29/04/16 11:00, Chris Wilson wrote:
> > >On Fri, Apr 29, 2016 at 10:50:02AM +0100, Tvrtko Ursulin wrote:
> > >>
> > >>On 29/04/16 10:39, Chris Wilson wrote:
> > >>>On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote:
> > >>>>On 29/04/16 10:15, Chris Wilson wrote:
> > >>>>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > >>>>>index 2e0eaa9fa240..2c94072ab085 100644
> > >>>>>--- a/drivers/gpu/drm/i915/intel_lrc.c
> > >>>>>+++ b/drivers/gpu/drm/i915/intel_lrc.c
> > >>>>>@@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> > >>>>>         struct intel_engine_cs *engine = &dev_priv->engine[id];
> > >>>>>         enum forcewake_domains fw_domains;
> > >>>>>
> > >>>>>-       engine->dev = dev;
> > >>>>>-
> > >>>>>         engine->id = id;
> > >>>>>         engine->name = info->name;
> > >>>>>         engine->exec_id = info->exec_id;
> > >>>>>         engine->guc_id = info->guc_id;
> > >>>>>         engine->mmio_base = info->mmio_base;
> > >>>>>
> > >>>>>+       /* disable interrupts to this engine before we install ourselves*/
> > >>>>>+       I915_WRITE_IMR(engine, ~0);
> > >>>>>+
> > >>>>>+       engine->dev = dev;
> > >>>>>+
> > >>>>>         /* Intentionally left blank. */
> > >>>>>         engine->buffer = NULL;
> > >>>>>
> > >>>>>Make sense?
> > >>>>
> > >>>>Not the most elegant because all the hw access we have so far is in
> > >>>>engine->init_hw. Why can't we just make intel_engine_initialized
> > >>>>return false until the very last thing in engine constructors?
> > >>>
> > >>>In my defence sanitizing the hw before we are ready is common practice
> > >>>across the driver. The unfun part is that irq install is before gem_init
> > >>>(because modeset init wants irq enabled for GMBUS/dp-aux). gem init
> > >>>itself could be split up and moved around so that the setup and init_hw
> > >>>phases are separate (which would be next on the init ordering hitlist I
> > >>>hope).
> > >>>
> > >>>I want engine->dev/engine->i915 set early so we can use it during setup
> > >>>and init-hw and so that for_each_engine() works as expected in that
> > >>>time.
> > >>
> > >>Why wouldn't an explicit engine->initialized flag solve that? You
> > >>could keep setting engine->dev early (as it should be) and then set
> > >>engine->initialized at the end of per-engine constructors.
> > >
> > >Because it becomes irrelevant very shortly. The only interesting
> > >question remaining is whether or not we should be sanitizing the IMR
> > >first. It has been suggested elsewhere (in Ville's review of the GT
> > >interrupt handling) that doing the sanitization would be useful.
> > 
> > How come it becomes irrelevant? Will there not be
> > intel_engine_initialized? Because as long as there is, imho it makes
> > sense not to use engine->dev for it.
> 
> The only argument here is how to handle an interrupt left over from
> before the driver loads. At all other times engine->dev means precisely
> that. I do not agree that you need to duplicate the state.

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