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

[Chris Wilson]

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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre