[Intel-gfx] [PATCH 10/12] drm/i915: Merge legacy+execlists context structs
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon May 23 10:26:13 UTC 2016
On 22/05/16 14:02, Chris Wilson wrote:
> struct intel_context contains two substructs, one for the legacy RCS and
> one for every execlists engine. Since legacy RCS is a subset of the
> execlists engine support, just combine the two substructs.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++-------------
> drivers/gpu/drm/i915/i915_drv.h | 7 ---
> drivers/gpu/drm/i915/i915_gem_context.c | 75 +++++++++++++++++++--------------
> drivers/gpu/drm/i915/intel_lrc.c | 26 ------------
> drivers/gpu/drm/i915/intel_lrc.h | 1 -
> 5 files changed, 55 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 945fe4710b37..30cb26fe2fa9 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -199,13 +199,6 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
> seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
> }
>
> -static void describe_ctx(struct seq_file *m, struct i915_gem_context *ctx)
> -{
> - seq_putc(m, ctx->legacy_hw_ctx.initialized ? 'I' : 'i');
> - seq_putc(m, ctx->remap_slice ? 'R' : 'r');
> - seq_putc(m, ' ');
> -}
> -
> static int i915_gem_object_list_info(struct seq_file *m, void *data)
> {
> struct drm_info_node *node = m->private;
> @@ -2001,7 +1994,6 @@ static int i915_context_status(struct seq_file *m, void *unused)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_engine_cs *engine;
> struct i915_gem_context *ctx;
> - enum intel_engine_id id;
> int ret;
>
> ret = mutex_lock_interruptible(&dev->struct_mutex);
> @@ -2009,10 +2001,6 @@ static int i915_context_status(struct seq_file *m, void *unused)
> return ret;
>
> list_for_each_entry(ctx, &dev_priv->context_list, link) {
> - if (!i915.enable_execlists &&
> - ctx->legacy_hw_ctx.rcs_state == NULL)
> - continue;
> -
> seq_printf(m, "HW context %u ", ctx->hw_id);
> if (IS_ERR(ctx->file_priv)) {
> seq_puts(m, "(deleted) ");
> @@ -2029,25 +2017,18 @@ static int i915_context_status(struct seq_file *m, void *unused)
> } else
> seq_puts(m, "(kernel) ");
>
> - describe_ctx(m, ctx);
> + seq_putc(m, ctx->remap_slice ? 'R' : 'r');
> + seq_putc(m, '\n');
>
> - if (i915.enable_execlists) {
> + for_each_engine(engine, dev_priv) {
> + struct intel_context *ce = &ctx->engine[engine->id];
> + seq_printf(m, "%s: ", engine->name);
> + seq_putc(m, ce->initialised ? 'I' : 'i');
> + if (ce->state)
> + describe_obj(m, ce->state);
> + if (ce->ringbuf)
> + describe_ctx_ringbuf(m, ce->ringbuf);
> seq_putc(m, '\n');
> - for_each_engine_id(engine, dev_priv, id) {
> - struct drm_i915_gem_object *ctx_obj =
> - ctx->engine[id].state;
> - struct intel_ringbuffer *ringbuf =
> - ctx->engine[id].ringbuf;
> -
> - seq_printf(m, "%s: ", engine->name);
> - if (ctx_obj)
> - describe_obj(m, ctx_obj);
> - if (ringbuf)
> - describe_ctx_ringbuf(m, ringbuf);
> - seq_putc(m, '\n');
> - }
> - } else {
> - describe_obj(m, ctx->legacy_hw_ctx.rcs_state);
> }
>
> seq_putc(m, '\n');
> @@ -2062,10 +2043,10 @@ static void i915_dump_lrc_obj(struct seq_file *m,
> struct i915_gem_context *ctx,
> struct intel_engine_cs *engine)
> {
> + struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
> struct page *page;
> uint32_t *reg_state;
> int j;
> - struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
> unsigned long ggtt_offset = 0;
>
> seq_printf(m, "CONTEXT: %s %u\n", engine->name, ctx->hw_id);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d3d2538d17e8..259a0ee7a601 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -862,13 +862,6 @@ struct i915_gem_context {
> /* Unique identifier for this context, used by the hw for tracking */
> unsigned hw_id;
>
> - /* Legacy ring buffer submission */
> - struct {
> - struct drm_i915_gem_object *rcs_state;
> - bool initialized;
> - } legacy_hw_ctx;
> -
> - /* Execlists */
> struct intel_context {
> struct drm_i915_gem_object *state;
> struct intel_ringbuffer *ringbuf;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 51e83a79ab36..9847235997b9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -152,13 +152,11 @@ static void i915_gem_context_clean(struct i915_gem_context *ctx)
> void i915_gem_context_free(struct kref *ctx_ref)
> {
> struct i915_gem_context *ctx = container_of(ctx_ref, typeof(*ctx), ref);
> + int i;
>
> lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> trace_i915_context_free(ctx);
>
> - if (i915.enable_execlists)
> - intel_lr_context_free(ctx);
> -
> /*
> * This context is going away and we need to remove all VMAs still
> * around. This is to handle imported shared objects for which
> @@ -168,8 +166,19 @@ void i915_gem_context_free(struct kref *ctx_ref)
>
> i915_ppgtt_put(ctx->ppgtt);
>
> - if (ctx->legacy_hw_ctx.rcs_state)
> - drm_gem_object_unreference(&ctx->legacy_hw_ctx.rcs_state->base);
> + for (i = 0; i < I915_NUM_ENGINES; i++) {
> + struct intel_context *ce = &ctx->engine[i];
> +
> + if (!ce->state)
> + continue;
> +
> + WARN_ON(ce->pin_count);
> + if (ce->ringbuf)
> + intel_ringbuffer_free(ce->ringbuf);
> +
> + drm_gem_object_unreference(&ce->state->base);
> + }
> +
> list_del(&ctx->link);
>
> ida_simple_remove(&ctx->i915->context_hw_ida, ctx->hw_id);
> @@ -266,7 +275,7 @@ __create_hw_context(struct drm_device *dev,
> ret = PTR_ERR(obj);
> goto err_out;
> }
> - ctx->legacy_hw_ctx.rcs_state = obj;
> + ctx->engine[RCS].state = obj;
> }
>
> /* Default context will never have a file_priv */
> @@ -335,8 +344,11 @@ static void i915_gem_context_unpin(struct i915_gem_context *ctx,
> if (i915.enable_execlists) {
> intel_lr_context_unpin(ctx, engine);
> } else {
> - if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)
> - i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
> + struct intel_context *ce = &ctx->engine[engine->id];
> +
> + if (ce->state)
> + i915_gem_object_ggtt_unpin(ce->state);
> +
> i915_gem_context_put(ctx);
> }
> }
> @@ -400,7 +412,7 @@ int i915_gem_context_init(struct drm_device *dev)
> return PTR_ERR(ctx);
> }
>
> - if (ctx->legacy_hw_ctx.rcs_state) {
> + if (ctx->engine[RCS].state) {
Maybe now put a comment saying this is for legacy now, to make the
asymmetry in ctx pinning paths between the two stick out more.
> int ret;
>
> /* We may need to do things with the shrinker which
> @@ -410,7 +422,7 @@ int i915_gem_context_init(struct drm_device *dev)
> * be available. To avoid this we always pin the default
> * context.
> */
> - ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state,
> + ret = i915_gem_obj_ggtt_pin(ctx->engine[RCS].state,
> get_context_alignment(dev_priv), 0);
> if (ret) {
> DRM_ERROR("Failed to pinned default global context (error %d)\n",
> @@ -435,15 +447,17 @@ void i915_gem_context_lost(struct drm_i915_private *dev_priv)
> lockdep_assert_held(&dev_priv->dev->struct_mutex);
>
> for_each_engine(engine, dev_priv) {
> - if (engine->last_context == NULL)
> - continue;
> + if (engine->last_context) {
> + i915_gem_context_unpin(engine->last_context, engine);
> + engine->last_context = NULL;
> + }
>
> - i915_gem_context_unpin(engine->last_context, engine);
> - engine->last_context = NULL;
> + /* Force the GPU state to be reinitialised on enabling */
> + dev_priv->kernel_context->engine[engine->id].initialised =
> + engine->init_context == NULL;
> }
>
> /* Force the GPU state to be reinitialised on enabling */
> - dev_priv->kernel_context->legacy_hw_ctx.initialized = false;
> dev_priv->kernel_context->remap_slice = ALL_L3_SLICES(dev_priv);
> }
>
> @@ -454,8 +468,8 @@ void i915_gem_context_fini(struct drm_device *dev)
>
> lockdep_assert_held(&dev->struct_mutex);
>
> - if (dctx->legacy_hw_ctx.rcs_state)
> - i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> + if (!i915.enable_execlists && dctx->engine[RCS].state)
> + i915_gem_object_ggtt_unpin(dctx->engine[RCS].state);
>
> i915_gem_context_put(dctx);
> dev_priv->kernel_context = NULL;
> @@ -562,7 +576,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> intel_ring_emit(engine, MI_NOOP);
> intel_ring_emit(engine, MI_SET_CONTEXT);
> intel_ring_emit(engine,
> - i915_gem_obj_ggtt_offset(req->ctx->legacy_hw_ctx.rcs_state) |
> + i915_gem_obj_ggtt_offset(req->ctx->engine[RCS].state) |
> flags);
> /*
> * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
> @@ -639,7 +653,7 @@ static inline bool skip_rcs_switch(struct i915_hw_ppgtt *ppgtt,
> if (to->remap_slice)
> return false;
>
> - if (!to->legacy_hw_ctx.initialized)
> + if (!to->engine[RCS].initialised)
> return false;
>
> if (ppgtt && (intel_engine_flag(engine) & ppgtt->pd_dirty_rings))
> @@ -704,7 +718,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> return 0;
>
> /* Trying to pin first makes error handling easier. */
> - ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state,
> + ret = i915_gem_obj_ggtt_pin(to->engine[RCS].state,
> get_context_alignment(engine->i915),
> 0);
> if (ret)
> @@ -727,7 +741,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> *
> * XXX: We need a real interface to do this instead of trickery.
> */
> - ret = i915_gem_object_set_to_gtt_domain(to->legacy_hw_ctx.rcs_state, false);
> + ret = i915_gem_object_set_to_gtt_domain(to->engine[RCS].state, false);
> if (ret)
> goto unpin_out;
>
> @@ -742,7 +756,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> goto unpin_out;
> }
>
> - if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to))
> + if (!to->engine[RCS].initialised || i915_gem_context_is_default(to))
> /* NB: If we inhibit the restore, the context is not allowed to
> * die because future work may end up depending on valid address
> * space. This means we must enforce that a page table load
> @@ -766,8 +780,8 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> * MI_SET_CONTEXT instead of when the next seqno has completed.
> */
> if (from != NULL) {
> - from->legacy_hw_ctx.rcs_state->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> - i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->legacy_hw_ctx.rcs_state), req);
> + from->engine[RCS].state->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> + i915_vma_move_to_active(i915_gem_obj_to_ggtt(from->engine[RCS].state), req);
> /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
> * whole damn pipeline, we don't need to explicitly mark the
> * object dirty. The only exception is that the context must be
> @@ -775,10 +789,10 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> * able to defer doing this until we know the object would be
> * swapped, but there is no way to do that yet.
> */
> - from->legacy_hw_ctx.rcs_state->dirty = 1;
> + from->engine[RCS].state->dirty = 1;
>
> /* obj is kept alive until the next request by its active ref */
> - i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
> + i915_gem_object_ggtt_unpin(from->engine[RCS].state);
> i915_gem_context_put(from);
> }
> engine->last_context = i915_gem_context_get(to);
> @@ -812,19 +826,19 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> to->remap_slice &= ~(1<<i);
> }
>
> - if (!to->legacy_hw_ctx.initialized) {
> + if (!to->engine[RCS].initialised) {
> if (engine->init_context) {
> ret = engine->init_context(req);
> if (ret)
> return ret;
> }
> - to->legacy_hw_ctx.initialized = true;
> + to->engine[RCS].initialised = true;
> }
>
> return 0;
>
> unpin_out:
> - i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state);
> + i915_gem_object_ggtt_unpin(to->engine[RCS].state);
> return ret;
> }
>
> @@ -848,8 +862,7 @@ int i915_switch_context(struct drm_i915_gem_request *req)
> WARN_ON(i915.enable_execlists);
> lockdep_assert_held(&req->i915->dev->struct_mutex);
>
> - if (engine->id != RCS ||
> - req->ctx->legacy_hw_ctx.rcs_state == NULL) {
> + if (!req->ctx->engine[engine->id].state) {
> struct i915_gem_context *to = req->ctx;
> struct i915_hw_ppgtt *ppgtt =
> to->ppgtt ?: req->i915->mm.aliasing_ppgtt;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 4b7c9680b097..558f069cd6d8 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2413,31 +2413,6 @@ populate_lr_context(struct i915_gem_context *ctx,
> }
>
> /**
> - * intel_lr_context_free() - free the LRC specific bits of a context
> - * @ctx: the LR context to free.
> - *
> - * The real context freeing is done in i915_gem_context_free: this only
> - * takes care of the bits that are LRC related: the per-engine backing
> - * objects and the logical ringbuffer.
> - */
> -void intel_lr_context_free(struct i915_gem_context *ctx)
> -{
> - int i;
> -
> - for (i = I915_NUM_ENGINES; --i >= 0; ) {
> - struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf;
> - struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state;
> -
> - if (!ctx_obj)
> - continue;
> -
> - WARN_ON(ctx->engine[i].pin_count);
> - intel_ringbuffer_free(ringbuf);
> - drm_gem_object_unreference(&ctx_obj->base);
> - }
> -}
> -
> -/**
> * intel_lr_context_size() - return the size of the context for an engine
> * @ring: which engine to find the context size for
> *
> @@ -2497,7 +2472,6 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> struct intel_ringbuffer *ringbuf;
> int ret;
>
> - WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL);
> WARN_ON(ce->state);
>
> context_size = round_up(intel_lr_context_size(engine), 4096);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index eb2e1d157fed..a8db42a9c50f 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -101,7 +101,6 @@ static inline void intel_logical_ring_emit_reg(struct intel_ringbuffer *ringbuf,
>
> struct i915_gem_context;
>
> -void intel_lr_context_free(struct i915_gem_context *ctx);
> uint32_t intel_lr_context_size(struct intel_engine_cs *engine);
> void intel_lr_context_unpin(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine);
>
Looks good to me.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list