[Intel-gfx] [PATCH 3/9] drm/i915: Refactor execlists default context pinning
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Apr 19 09:16:29 UTC 2016
On 19/04/16 07:49, Chris Wilson wrote:
> Refactor pinning and unpinning of contexts, such that the default
> context for an engine is pinned during initialisation and unpinned
> during teardown (pinning of the context handles the reference counting).
> Thus we can eliminate the special case handling of the default context
> that was required to mask that it was not being pinned normally.
>
> v2: Rebalance context_queue after rebasing.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 5 +-
> drivers/gpu/drm/i915/i915_gem_request.c | 6 +-
> drivers/gpu/drm/i915/intel_lrc.c | 103 ++++++++++++--------------------
> 3 files changed, 41 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index bd409bfa6e61..a07c3478f0d8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2108,9 +2108,8 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
> return ret;
>
> list_for_each_entry(ctx, &dev_priv->context_list, link)
> - if (ctx != dev_priv->kernel_context)
> - for_each_engine(engine, dev_priv)
> - i915_dump_lrc_obj(m, ctx, engine);
> + for_each_engine(engine, dev_priv)
> + i915_dump_lrc_obj(m, ctx, engine);
>
> mutex_unlock(&dev->struct_mutex);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index a6f3e6e53b3b..33aacf1725dd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -644,10 +644,8 @@ void i915_gem_request_free(struct kref *req_ref)
> i915_gem_request_remove_from_client(req);
>
> if (ctx) {
> - if (i915.enable_execlists) {
> - if (ctx != to_i915(req)->kernel_context)
> - intel_lr_context_unpin(ctx, req->engine);
> - }
> + if (i915.enable_execlists)
> + intel_lr_context_unpin(ctx, req->engine);
>
> i915_gem_context_unreference(ctx);
> }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index cec112449b31..79ac6f06a502 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -585,9 +585,7 @@ static void execlists_context_queue(struct drm_i915_gem_request *request)
> struct drm_i915_gem_request *cursor;
> int num_elements = 0;
>
> - if (request->ctx != to_i915(request)->kernel_context)
> - intel_lr_context_pin(request->ctx, engine);
> -
> + intel_lr_context_pin(request->ctx, request->engine);
> i915_gem_request_reference(request);
>
> spin_lock_bh(&engine->execlist_lock);
> @@ -688,10 +686,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> return ret;
> }
>
> - if (request->ctx != to_i915(request)->kernel_context)
> - ret = intel_lr_context_pin(request->ctx, request->engine);
> -
> - return ret;
> + return intel_lr_context_pin(request->ctx, request->engine);
> }
>
> static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
> @@ -768,12 +763,8 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
> if (engine->last_context != request->ctx) {
> if (engine->last_context)
> intel_lr_context_unpin(engine->last_context, engine);
> - if (request->ctx != to_i915(request)->kernel_context) {
> - intel_lr_context_pin(request->ctx, engine);
> - engine->last_context = request->ctx;
> - } else {
> - engine->last_context = NULL;
> - }
> + intel_lr_context_pin(request->ctx, engine);
> + engine->last_context = request->ctx;
> }
>
> if (dev_priv->guc.execbuf_client)
> @@ -996,12 +987,7 @@ void intel_execlists_retire_requests(struct intel_engine_cs *engine)
> spin_unlock_bh(&engine->execlist_lock);
>
> list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) {
> - struct intel_context *ctx = req->ctx;
> - struct drm_i915_gem_object *ctx_obj =
> - ctx->engine[engine->id].state;
> -
> - if (ctx_obj && (ctx != to_i915(req)->kernel_context))
> - intel_lr_context_unpin(ctx, engine);
> + intel_lr_context_unpin(req->ctx, engine);
>
> list_del(&req->execlist_link);
> i915_gem_request_unreference(req);
> @@ -1046,22 +1032,25 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
> return 0;
> }
>
> -static int intel_lr_context_do_pin(struct intel_context *ctx,
> - struct intel_engine_cs *engine)
> +static int intel_lr_context_pin(struct intel_context *ctx,
> + struct intel_engine_cs *engine)
> {
> struct drm_i915_private *dev_priv = ctx->i915;
> - struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
> - struct intel_ringbuffer *ringbuf = ctx->engine[engine->id].ringbuf;
> + struct drm_i915_gem_object *ctx_obj;
> + struct intel_ringbuffer *ringbuf;
> void *vaddr;
> - u32 *lrc_reg_state;
> + uint32_t *lrc_reg_state;
I prefer u32, and you preferred u64 in "drm/i915: Replace the pinned
context address with its unique ID". :) Doesn't matter really, it is too
late for consistency anyway.
> int ret;
>
> - WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
> + lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> + if (ctx->engine[engine->id].pin_count++)
> + return 0;
>
> + ctx_obj = ctx->engine[engine->id].state;
> ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
> PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> if (ret)
> - return ret;
> + goto err;
>
> vaddr = i915_gem_object_pin_map(ctx_obj);
> if (IS_ERR(vaddr)) {
> @@ -1071,10 +1060,12 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
>
> lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
>
> + ringbuf = ctx->engine[engine->id].ringbuf;
> ret = intel_pin_and_map_ringbuffer_obj(engine->dev, ringbuf);
> if (ret)
> goto unpin_map;
>
> + i915_gem_context_reference(ctx);
> ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
> intel_lr_context_descriptor_update(ctx, engine);
> lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start;
> @@ -1085,31 +1076,13 @@ static int intel_lr_context_do_pin(struct intel_context *ctx,
> if (i915.enable_guc_submission)
> I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>
> - return ret;
> + return 0;
>
> unpin_map:
> i915_gem_object_unpin_map(ctx_obj);
> unpin_ctx_obj:
> i915_gem_object_ggtt_unpin(ctx_obj);
> -
> - return ret;
> -}
> -
> -static int intel_lr_context_pin(struct intel_context *ctx,
> - struct intel_engine_cs *engine)
> -{
> - int ret = 0;
> -
> - if (ctx->engine[engine->id].pin_count++ == 0) {
> - ret = intel_lr_context_do_pin(ctx, engine);
> - if (ret)
> - goto reset_pin_count;
> -
> - i915_gem_context_reference(ctx);
> - }
> - return ret;
> -
> -reset_pin_count:
> +err:
> ctx->engine[engine->id].pin_count = 0;
> return ret;
> }
> @@ -1119,17 +1092,21 @@ void intel_lr_context_unpin(struct intel_context *ctx,
> {
> struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
>
> - WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
> - if (--ctx->engine[engine->id].pin_count == 0) {
> - i915_gem_object_unpin_map(ctx_obj);
> - intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
> - i915_gem_object_ggtt_unpin(ctx_obj);
> - ctx->engine[engine->id].lrc_vma = NULL;
> - ctx->engine[engine->id].lrc_desc = 0;
> - ctx->engine[engine->id].lrc_reg_state = NULL;
> + lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> + GEM_BUG_ON(ctx->engine[engine->id].pin_count == 0);
I thought it is more readable to leave space between asserts and code.
And in pin as well if you decide to do it.
> + if (--ctx->engine[engine->id].pin_count)
> + return;
>
> - i915_gem_context_unreference(ctx);
> - }
Could move ctx_obj assignment just before use here.
> + i915_gem_object_unpin_map(ctx_obj);
> + intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
> +
> + i915_gem_object_ggtt_unpin(ctx_obj);
> +
> + ctx->engine[engine->id].lrc_vma = NULL;
> + ctx->engine[engine->id].lrc_desc = 0;
> + ctx->engine[engine->id].lrc_reg_state = NULL;
> +
> + i915_gem_context_unreference(ctx);
> }
>
> static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
> @@ -1982,6 +1959,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> i915_gem_object_unpin_map(engine->status_page.obj);
> engine->status_page.obj = NULL;
> }
> + intel_lr_context_unpin(engine->i915->kernel_context, engine);
I don't see engine->i915 upstream yet unless I am missing something.
dev_priv is already there on the other hand.
>
> engine->idle_lite_restore_wa = 0;
> engine->disable_lite_restore_wa = false;
> @@ -2104,11 +2082,10 @@ logical_ring_init(struct intel_engine_cs *engine)
> goto error;
>
> /* As this is the default context, always pin it */
> - ret = intel_lr_context_do_pin(dctx, engine);
> + ret = intel_lr_context_pin(dctx, engine);
> if (ret) {
> - DRM_ERROR(
> - "Failed to pin and map ringbuffer %s: %d\n",
> - engine->name, ret);
> + DRM_ERROR("Failed to pin context for %s: %d\n",
> + engine->name, ret);
> goto error;
> }
>
> @@ -2505,12 +2482,6 @@ void intel_lr_context_free(struct intel_context *ctx)
> if (!ctx_obj)
> continue;
>
> - if (ctx == ctx->i915->kernel_context) {
> - intel_unpin_ringbuffer_obj(ringbuf);
> - i915_gem_object_ggtt_unpin(ctx_obj);
> - i915_gem_object_unpin_map(ctx_obj);
> - }
> -
> WARN_ON(ctx->engine[i].pin_count);
> intel_ringbuffer_free(ringbuf);
> drm_gem_object_unreference(&ctx_obj->base);
>
Just some minors then and correct rebase. Otherwise I think it is correct.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list