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

[Chris Wilson]

On Fri, Apr 29, 2016 at 10:04:35AM +0100, Tvrtko Ursulin wrote:
> 
> On 28/04/16 18:35, Chris Wilson wrote:
> >Move all of the constant assignments up front and into a common
> >function. This is primarily to ensure the backpointers are set as early
> >as possible for later use during initialisation.
> >
> >v2: Use a constant struct so that all the similar values are set
> >together.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> >Cc: Dave Gordon <david.s.gordon at intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_lrc.c | 176 +++++++++++++++++++++------------------
> >  1 file changed, 93 insertions(+), 83 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> >index 874c2515f9d4..2e0eaa9fa240 100644
> >--- a/drivers/gpu/drm/i915/intel_lrc.c
> >+++ b/drivers/gpu/drm/i915/intel_lrc.c
> >@@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> >  }
> >
> >  static void
> >-logical_ring_default_vfuncs(struct drm_device *dev,
> >-			    struct intel_engine_cs *engine)
> >+logical_ring_default_vfuncs(struct intel_engine_cs *engine)
> >  {
> >  	/* Default vfuncs which can be overriden by each engine. */
> >  	engine->init_hw = gen8_init_common_ring;
> >@@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev,
> >  	engine->emit_bb_start = gen8_emit_bb_start;
> >  	engine->get_seqno = gen8_get_seqno;
> >  	engine->set_seqno = gen8_set_seqno;
> >-	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> >+	if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) {
> >  		engine->irq_seqno_barrier = bxt_a_seqno_barrier;
> >  		engine->set_seqno = bxt_a_set_seqno;
> >  	}
> >@@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift)
> >  {
> >  	engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> >  	engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> >+	init_waitqueue_head(&engine->irq_queue);
> >  }
> >
> >  static int
> >@@ -1964,31 +1964,68 @@ lrc_setup_hws(struct intel_engine_cs *engine,
> >  	return 0;
> >  }
> >
> >-static int
> >-logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)
> >+static const struct logical_ring_info {
> >+	const char *name;
> >+	unsigned exec_id;
> >+	unsigned guc_id;
> >+	u32 mmio_base;
> >+	unsigned irq_shift;
> >+} logical_rings[] = {
> >+	[RCS] = {
> >+		.name = "render ring",
> >+		.exec_id = I915_EXEC_RENDER,
> >+		.guc_id = GUC_RENDER_ENGINE,
> >+		.mmio_base = RENDER_RING_BASE,
> >+		.irq_shift = GEN8_RCS_IRQ_SHIFT,
> >+	},
> >+	[BCS] = {
> >+		.name = "blitter ring",
> >+		.exec_id = I915_EXEC_BLT,
> >+		.guc_id = GUC_BLITTER_ENGINE,
> >+		.mmio_base = BLT_RING_BASE,
> >+		.irq_shift = GEN8_BCS_IRQ_SHIFT,
> >+	},
> >+	[VCS] = {
> >+		.name = "bsd ring",
> >+		.exec_id = I915_EXEC_BSD,
> >+		.guc_id = GUC_VIDEO_ENGINE,
> >+		.mmio_base = GEN6_BSD_RING_BASE,
> >+		.irq_shift = GEN8_VCS1_IRQ_SHIFT,
> >+	},
> >+	[VCS2] = {
> >+		.name = "bsd2 ring",
> >+		.exec_id = I915_EXEC_BSD,
> >+		.guc_id = GUC_VIDEO_ENGINE2,
> >+		.mmio_base = GEN8_BSD2_RING_BASE,
> >+		.irq_shift = GEN8_VCS2_IRQ_SHIFT,
> >+	},
> >+	[VECS] = {
> >+		.name = "video enhancement ring",
> >+		.exec_id = I915_EXEC_VEBOX,
> >+		.guc_id = GUC_VIDEOENHANCE_ENGINE,
> >+		.mmio_base = VEBOX_RING_BASE,
> >+		.irq_shift = GEN8_VECS_IRQ_SHIFT,
> >+	},
> >+};
> >+
> >+static struct intel_engine_cs *
> >+logical_ring_setup(struct drm_device *dev, enum intel_engine_id id)
> >  {
> 
> Would dev_priv be better? Just to gradually move towards the correct
> state of things.

I have a patch queued up to do engine->i915 (1 KiB in object code
reduction) next.

> >+	const struct logical_ring_info *info = &logical_rings[id];
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >-	struct intel_context *dctx = dev_priv->kernel_context;
> >+	struct intel_engine_cs *engine = &dev_priv->engine[id];
> >  	enum forcewake_domains fw_domains;
> >-	int ret;
> >-
> >-	/* Intentionally left blank. */
> >-	engine->buffer = NULL;
> >
> >  	engine->dev = dev;
> 
> Looking at usages of intel_engine_initialized... one potential
> danger scenario would be interrupt noise during driver load end in
> notify ring and explodes. Sounds very unlikely but theoretically it
> is a regression compared to where engine->dev initialization was
> before.
> 
> We should really move away from engine->dev for this and just add an
> explicit flag.

Hmm. not that but I think we really should be sanitizing the irq here
and enabling them last.

Like:

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

-- 
Chris Wilson, Intel Open Source Technology Centre