[Intel-gfx] [PATCH] drm/i915/execlists: Refactor common engine setup
Dave Gordon
david.s.gordon at intel.com
Thu Apr 28 16:12:28 UTC 2016
On 28/04/16 14:47, 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.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> ---
>
> I'm not too happy about using the function parameters with lots of
> similar types, very easy to muddle them up. I'm sending this all because
> it places one initialiser in a common location rather than per-engine!
> -Chris
>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 157 +++++++++++++++++++--------------------
> 1 file changed, 78 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 874c2515f9d4..a4a4f8eaf000 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,28 @@ 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 struct intel_engine_cs *
> +logical_ring_setup(struct drm_device *dev, int id,
> + const char *name,
> + unsigned exec_id,
> + unsigned guc_id,
> + unsigned mmio_base,
> + unsigned irq)
> {
> 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;
Eeep! Isn't this the "engine initialised and ready for use" 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);
>
> - tasklet_init(&engine->irq_tasklet,
> - intel_lrc_irq_handler, (unsigned long)engine);
> + engine->name = name,
> + engine->id = id;
> + engine->exec_id = exec_id;
> + engine->guc_id = guc_id;
> + engine->mmio_base = mmio_base;
>
> - 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 +2001,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, irq);
> +
> + 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 +2058,17 @@ 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;
> 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);
> + engine = logical_ring_setup(dev, RCS, "render ring",
> + I915_EXEC_RENDER,
> + GUC_RENDER_ENGINE,
> + RENDER_RING_BASE,
> + GEN8_RCS_IRQ_SHIFT);
We could turn this collection of arguments into a struct, and create a
static readonly table of them. That would eliminate the risk of mixing
them up!
const struct engine_info engines[] =
{ /* name ID execflag GuC ID MMIO
base IRQ shift */
{ "render", RCS, I915_EXEC_RENDER, GUC_RENDER_ENGINE,
RENDER_RING_BASE, GEN8_RCS_IRQ_SHIFT },
{ "bsd", BSD, I915_EXEC_BSD, GUC_VIDEO_ENGINE,
GEN6_BSD_RING_BASE, GEN8_VCS1_IRQ_SHIFT),
...
};
And since all the correspondences would be defined in one table, it
would be much easier to check than hunting around for multiple function
calls.
.Dave.
> 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 +2079,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 +2094,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 +2104,54 @@ 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_default_irqs(engine, GEN8_VCS1_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VCS, "bsd ring",
> + I915_EXEC_BSD,
> + GUC_VIDEO_ENGINE,
> + GEN6_BSD_RING_BASE,
> + GEN8_VCS1_IRQ_SHIFT);
>
> - 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_default_irqs(engine, GEN8_VCS2_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VCS2, "bsd2 ring",
> + I915_EXEC_BSD,
> + GUC_VIDEO_ENGINE2,
> + GEN8_BSD2_RING_BASE,
> + GEN8_VCS2_IRQ_SHIFT);
>
> - 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;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_BCS_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, BCS, "blitter ring",
> + I915_EXEC_BLT,
> + GUC_BLITTER_ENGINE,
> + BLT_RING_BASE,
> + GEN8_BCS_IRQ_SHIFT);
>
> - 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;
> + struct intel_engine_cs *engine;
>
> - logical_ring_default_irqs(engine, GEN8_VECS_IRQ_SHIFT);
> - logical_ring_default_vfuncs(dev, engine);
> + engine = logical_ring_setup(dev, VECS, "video enhancement ring",
> + I915_EXEC_VEBOX,
> + GUC_VIDEOENHANCE_ENGINE,
> + VEBOX_RING_BASE,
> + GEN8_VECS_IRQ_SHIFT);
>
> - return logical_ring_init(dev, engine);
> + return logical_ring_init(engine);
> }
>
> /**
>
More information about the Intel-gfx
mailing list