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

Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> · Fri, 29 Apr 2016 10:25:41 +0100

On 29/04/16 10:15, Chris Wilson wrote:
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@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
Cc: Dave Gordon <david.s.gordon@xxxxxxxxx>
---
  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?

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?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx







