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

Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> · Thu, 5 May 2016 11:18:41 +0100

Hi,

On 05/05/16 10:15, 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.
v3: Sanitize the engine's IMR to disable any potential interrupt before
we are ready (enabled in init_hw).

Same as before - I don't like hardware access in this code path since we 
otherwise have it split into a later init_hw phase. And I don't like 
engine->dev being used for intel_engine_initialized.

On retrospect, interrupt vs engine->irq_queue race is already there now, 
for the render ring at least. So maybe just drop the IMR bit which would 
make the patch pure refactoring and can have my R-b then.

Regards,

Tvrtko

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 | 180 +++++++++++++++++++++------------------
  1 file changed, 97 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d8763524319d..8106316ce56f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1918,8 +1918,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;
@@ -1930,7 +1929,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;
  	}
@@ -1941,6 +1940,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
@@ -1961,31 +1961,72 @@ 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)
  {
+	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->id = id;
+	engine->name = info->name;
+	engine->exec_id = info->exec_id;
+	engine->guc_id = info->guc_id;
+	engine->mmio_base = info->mmio_base;

-	engine->dev = dev;
-	INIT_LIST_HEAD(&engine->active_list);
-	INIT_LIST_HEAD(&engine->request_list);
-	i915_gem_batch_pool_init(dev, &engine->batch_pool);
-	init_waitqueue_head(&engine->irq_queue);
+	/* disable interrupts to this engine before we install ourselves */
+	I915_WRITE_IMR(engine, ~0);
+	POSTING_READ(RING_IMR(engine->mmio_base));

-	INIT_LIST_HEAD(&engine->buffers);
-	INIT_LIST_HEAD(&engine->execlist_queue);
-	spin_lock_init(&engine->execlist_lock);
-
-	tasklet_init(&engine->irq_tasklet,
-		     intel_lrc_irq_handler, (unsigned long)engine);
+	engine->dev = dev;

-	logical_ring_init_platform_invariants(engine);
+	/* Intentionally left blank. */
+	engine->buffer = NULL;

  	fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
  						    RING_ELSP(engine),
@@ -2001,6 +2042,31 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)

  	engine->fw_domains = fw_domains;

+	INIT_LIST_HEAD(&engine->active_list);
+	INIT_LIST_HEAD(&engine->request_list);
+	INIT_LIST_HEAD(&engine->buffers);
+	INIT_LIST_HEAD(&engine->execlist_queue);
+	spin_lock_init(&engine->execlist_lock);
+
+	tasklet_init(&engine->irq_tasklet,
+		     intel_lrc_irq_handler, (unsigned long)engine);
+
+	logical_ring_init_platform_invariants(engine);
+	logical_ring_default_vfuncs(engine);
+	logical_ring_default_irqs(engine, info->irq_shift);
+
+	intel_engine_init_hangcheck(engine);
+	i915_gem_batch_pool_init(engine->dev, &engine->batch_pool);
+
+	return engine;
+}
+
+static int
+logical_ring_init(struct intel_engine_cs *engine)
+{
+	struct intel_context *dctx = to_i915(engine->dev)->kernel_context;
+	int ret;
+
  	ret = i915_cmd_parser_init_ring(engine);
  	if (ret)
  		goto error;
@@ -2033,22 +2099,12 @@ error:

  static int logical_render_ring_init(struct drm_device *dev)
  {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[RCS];
+	struct intel_engine_cs *engine = logical_ring_setup(dev, RCS);
  	int ret;

-	engine->name = "render ring";
-	engine->id = RCS;
-	engine->exec_id = I915_EXEC_RENDER;
-	engine->guc_id = GUC_RENDER_ENGINE;
-	engine->mmio_base = RENDER_RING_BASE;
-
-	logical_ring_default_irqs(engine, GEN8_RCS_IRQ_SHIFT);
  	if (HAS_L3_DPF(dev))
  		engine->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;

-	logical_ring_default_vfuncs(dev, engine);
-
  	/* Override some for render ring. */
  	if (INTEL_INFO(dev)->gen >= 9)
  		engine->init_hw = gen9_init_render_ring;
@@ -2059,8 +2115,6 @@ static int logical_render_ring_init(struct drm_device *dev)
  	engine->emit_flush = gen8_emit_flush_render;
  	engine->emit_request = gen8_emit_request_render;

-	engine->dev = dev;
-
  	ret = intel_init_pipe_control(engine);
  	if (ret)
  		return ret;
@@ -2076,7 +2130,7 @@ static int logical_render_ring_init(struct drm_device *dev)
  			  ret);
  	}

-	ret = logical_ring_init(dev, engine);
+	ret = logical_ring_init(engine);
  	if (ret) {
  		lrc_destroy_wa_ctx_obj(engine);
  	}
@@ -2086,70 +2140,30 @@ static int logical_render_ring_init(struct drm_device *dev)

  static int logical_bsd_ring_init(struct drm_device *dev)
  {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[VCS];
-
-	engine->name = "bsd ring";
-	engine->id = VCS;
-	engine->exec_id = I915_EXEC_BSD;
-	engine->guc_id = GUC_VIDEO_ENGINE;
-	engine->mmio_base = GEN6_BSD_RING_BASE;
+	struct intel_engine_cs *engine = logical_ring_setup(dev, VCS);

-	logical_ring_default_irqs(engine, GEN8_VCS1_IRQ_SHIFT);
-	logical_ring_default_vfuncs(dev, engine);
-
-	return logical_ring_init(dev, engine);
+	return logical_ring_init(engine);
  }

  static int logical_bsd2_ring_init(struct drm_device *dev)
  {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[VCS2];
-
-	engine->name = "bsd2 ring";
-	engine->id = VCS2;
-	engine->exec_id = I915_EXEC_BSD;
-	engine->guc_id = GUC_VIDEO_ENGINE2;
-	engine->mmio_base = GEN8_BSD2_RING_BASE;
+	struct intel_engine_cs *engine = logical_ring_setup(dev, VCS2);

-	logical_ring_default_irqs(engine, GEN8_VCS2_IRQ_SHIFT);
-	logical_ring_default_vfuncs(dev, engine);
-
-	return logical_ring_init(dev, engine);
+	return logical_ring_init(engine);
  }

  static int logical_blt_ring_init(struct drm_device *dev)
  {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[BCS];
-
-	engine->name = "blitter ring";
-	engine->id = BCS;
-	engine->exec_id = I915_EXEC_BLT;
-	engine->guc_id = GUC_BLITTER_ENGINE;
-	engine->mmio_base = BLT_RING_BASE;
-
-	logical_ring_default_irqs(engine, GEN8_BCS_IRQ_SHIFT);
-	logical_ring_default_vfuncs(dev, engine);
+	struct intel_engine_cs *engine = logical_ring_setup(dev, BCS);

-	return logical_ring_init(dev, engine);
+	return logical_ring_init(engine);
  }

  static int logical_vebox_ring_init(struct drm_device *dev)
  {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_engine_cs *engine = &dev_priv->engine[VECS];
-
-	engine->name = "video enhancement ring";
-	engine->id = VECS;
-	engine->exec_id = I915_EXEC_VEBOX;
-	engine->guc_id = GUC_VIDEOENHANCE_ENGINE;
-	engine->mmio_base = VEBOX_RING_BASE;
-
-	logical_ring_default_irqs(engine, GEN8_VECS_IRQ_SHIFT);
-	logical_ring_default_vfuncs(dev, engine);
+	struct intel_engine_cs *engine = logical_ring_setup(dev, VECS);

-	return logical_ring_init(dev, engine);
+	return logical_ring_init(engine);
  }

  /**

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







