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@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
---
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);
}
/**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
