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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Dave Gordon <david.s.gordon@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.

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

> -	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);
>
> -	INIT_LIST_HEAD(&engine->buffers);
> -	INIT_LIST_HEAD(&engine->execlist_queue);
> -	spin_lock_init(&engine->execlist_lock);
> +	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;
>
> -	tasklet_init(&engine->irq_tasklet,
> -		     intel_lrc_irq_handler, (unsigned long)engine);
> -
> -	logical_ring_init_platform_invariants(engine);
> +	/* Intentionally left blank. */
> +	engine->buffer = NULL;
>
>   	fw_domains = intel_uncore_forcewake_for_reg(dev_priv,
>   						    RING_ELSP(engine),
> @@ -2004,6 +2041,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;
> @@ -2036,22 +2098,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;
> @@ -2062,8 +2114,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;
> @@ -2079,7 +2129,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);
>   	}
> @@ -2089,70 +2139,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);
>   }
>
>   /**
>

Otherwise I like how this looks, just engine->dev bugging me!

Regards,

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