[Intel-gfx] [PATCH 02/19] drm/i915/execlists: Refactor common engine setup
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu May 5 10:18:41 UTC 2016
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 at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Dave Gordon <david.s.gordon at intel.com>
> ---
> 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);
> }
>
> /**
>
More information about the Intel-gfx
mailing list