From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 06/12] drm/i915: Name the inner most per-engine intel_context struct
Date: Mon, 23 May 2016 10:26:39 +0100 [thread overview]
Message-ID: <5742CCCF.5010200@linux.intel.com> (raw)
In-Reply-To: <1463922152-2709-6-git-send-email-chris@chris-wilson.co.uk>
On 22/05/16 14:02, Chris Wilson wrote:
> We want to give a name to the currently anonymous per-engine struct
> inside the context, so that we can assign it to a local variable and
> save clumsy typing. The name we have chosen is intel_context as it
> reflects the HW facing portion of the context state (the logical context
> state, the registers, the ringbuffer etc).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_guc_submission.c | 15 ++---
> drivers/gpu/drm/i915/intel_lrc.c | 90 +++++++++++++++---------------
> 3 files changed, 51 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0a3442fcd93e..e6735dc9eeb2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -869,7 +869,7 @@ struct i915_gem_context {
> } legacy_hw_ctx;
>
> /* Execlists */
> - struct {
> + struct intel_context {
> struct drm_i915_gem_object *state;
> struct intel_ringbuffer *ringbuf;
> int pin_count;
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a292672f36d5..c29c1d19764f 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -363,7 +363,6 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
> struct i915_gem_context *ctx = client->owner;
> struct guc_context_desc desc;
> struct sg_table *sg;
> - enum intel_engine_id id;
> u32 gfx_addr;
>
> memset(&desc, 0, sizeof(desc));
> @@ -373,10 +372,10 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
> desc.priority = client->priority;
> desc.db_id = client->doorbell_id;
>
> - for_each_engine_id(engine, dev_priv, id) {
> + for_each_engine(engine, dev_priv) {
> + struct intel_context *ce = &ctx->engine[engine->id];
> struct guc_execlist_context *lrc = &desc.lrc[engine->guc_id];
> struct drm_i915_gem_object *obj;
> - uint64_t ctx_desc;
>
> /* TODO: We have a design issue to be solved here. Only when we
> * receive the first batch, we know which engine is used by the
> @@ -385,20 +384,18 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
> * for now who owns a GuC client. But for future owner of GuC
> * client, need to make sure lrc is pinned prior to enter here.
> */
> - obj = ctx->engine[id].state;
> - if (!obj)
> + if (!ce->state)
> break; /* XXX: continue? */
>
> - ctx_desc = intel_lr_context_descriptor(ctx, engine);
> - lrc->context_desc = (u32)ctx_desc;
> + lrc->context_desc = lower_32_bits(ce->lrc_desc);
Could have kept use of intel_lr_context_descriptor for better separation.
>
> /* The state page is after PPHWSP */
> - gfx_addr = i915_gem_obj_ggtt_offset(obj);
> + gfx_addr = i915_gem_obj_ggtt_offset(ce->state);
> lrc->ring_lcra = gfx_addr + LRC_STATE_PN * PAGE_SIZE;
> lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
> (engine->guc_id << GUC_ELC_ENGINE_OFFSET);
>
> - obj = ctx->engine[id].ringbuf->obj;
> + obj = ce->ringbuf->obj;
> gfx_addr = i915_gem_obj_ggtt_offset(obj);
>
> lrc->ring_begin = gfx_addr;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 9a55478e23ac..4b7c9680b097 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -300,7 +300,7 @@ logical_ring_init_platform_invariants(struct intel_engine_cs *engine)
> * descriptor for a pinned context
> *
> * @ctx: Context to work on
> - * @ring: Engine the descriptor will be used with
> + * @engine: Engine the descriptor will be used with
> *
> * The context descriptor encodes various attributes of a context,
> * including its GTT address and some flags. Because it's fairly
> @@ -318,16 +318,17 @@ static void
> intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine)
> {
> + struct intel_context *ce = &ctx->engine[engine->id];
> u64 desc;
>
> BUILD_BUG_ON(MAX_CONTEXT_HW_ID > (1<<GEN8_CTX_ID_WIDTH));
>
> desc = engine->ctx_desc_template; /* bits 0-11 */
> - desc |= ctx->engine[engine->id].lrc_vma->node.start + /* bits 12-31 */
> - LRC_PPHWSP_PN * PAGE_SIZE;
> + desc |= ce->lrc_vma->node.start + LRC_PPHWSP_PN * PAGE_SIZE;
> + /* bits 12-31 */
> desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits 32-52 */
>
> - ctx->engine[engine->id].lrc_desc = desc;
> + ce->lrc_desc = desc;
> }
>
> uint64_t intel_lr_context_descriptor(struct i915_gem_context *ctx,
> @@ -674,6 +675,7 @@ static int execlists_move_to_gpu(struct drm_i915_gem_request *req,
> int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request)
> {
> struct intel_engine_cs *engine = request->engine;
> + struct intel_context *ce = &request->ctx->engine[engine->id];
> int ret;
>
> /* Flush enough space to reduce the likelihood of waiting after
> @@ -682,13 +684,13 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> */
> request->reserved_space += EXECLISTS_REQUEST_SIZE;
>
> - if (request->ctx->engine[engine->id].state == NULL) {
> + if (!ce->state) {
> ret = execlists_context_deferred_alloc(request->ctx, engine);
> if (ret)
> return ret;
> }
>
> - request->ringbuf = request->ctx->engine[engine->id].ringbuf;
> + request->ringbuf = ce->ringbuf;
>
> if (i915.enable_guc_submission) {
> /*
> @@ -711,12 +713,12 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> if (ret)
> goto err_unpin;
>
> - if (!request->ctx->engine[engine->id].initialised) {
> + if (!ce->initialised) {
> ret = engine->init_context(request);
> if (ret)
> goto err_unpin;
>
> - request->ctx->engine[engine->id].initialised = true;
> + ce->initialised = true;
> }
>
> /* Note that after this point, we have committed to using
> @@ -936,24 +938,22 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine)
> {
> struct drm_i915_private *dev_priv = ctx->i915;
> - struct drm_i915_gem_object *ctx_obj;
> - struct intel_ringbuffer *ringbuf;
> + struct intel_context *ce = &ctx->engine[engine->id];
> void *vaddr;
> u32 *lrc_reg_state;
> int ret;
>
> lockdep_assert_held(&ctx->i915->dev->struct_mutex);
>
> - if (ctx->engine[engine->id].pin_count++)
> + if (ce->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);
> + ret = i915_gem_obj_ggtt_pin(ce->state, GEN8_LR_CONTEXT_ALIGN,
> + PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> if (ret)
> goto err;
>
> - vaddr = i915_gem_object_pin_map(ctx_obj);
> + vaddr = i915_gem_object_pin_map(ce->state);
> if (IS_ERR(vaddr)) {
> ret = PTR_ERR(vaddr);
> goto unpin_ctx_obj;
> @@ -961,16 +961,16 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
>
> lrc_reg_state = vaddr + LRC_STATE_PN * PAGE_SIZE;
>
> - ringbuf = ctx->engine[engine->id].ringbuf;
> - ret = intel_pin_and_map_ringbuffer_obj(dev_priv, ringbuf);
> + ret = intel_pin_and_map_ringbuffer_obj(dev_priv, ce->ringbuf);
> if (ret)
> goto unpin_map;
>
> - ctx->engine[engine->id].lrc_vma = i915_gem_obj_to_ggtt(ctx_obj);
> + ce->lrc_vma = i915_gem_obj_to_ggtt(ce->state);
> intel_lr_context_descriptor_update(ctx, engine);
> - lrc_reg_state[CTX_RING_BUFFER_START+1] = ringbuf->vma->node.start;
> - ctx->engine[engine->id].lrc_reg_state = lrc_reg_state;
> - ctx_obj->dirty = true;
> +
> + lrc_reg_state[CTX_RING_BUFFER_START+1] = ce->ringbuf->vma->node.start;
> + ce->lrc_reg_state = lrc_reg_state;
> + ce->state->dirty = true;
>
> /* Invalidate GuC TLB. */
> if (i915.enable_guc_submission)
> @@ -980,34 +980,33 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
> return 0;
>
> unpin_map:
> - i915_gem_object_unpin_map(ctx_obj);
> + i915_gem_object_unpin_map(ce->state);
> unpin_ctx_obj:
> - i915_gem_object_ggtt_unpin(ctx_obj);
> + i915_gem_object_ggtt_unpin(ce->state);
> err:
> - ctx->engine[engine->id].pin_count = 0;
> + ce->pin_count = 0;
> return ret;
> }
>
> void intel_lr_context_unpin(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine)
> {
> - struct drm_i915_gem_object *ctx_obj;
> + struct intel_context *ce = &ctx->engine[engine->id];
>
> lockdep_assert_held(&ctx->i915->dev->struct_mutex);
> - GEM_BUG_ON(ctx->engine[engine->id].pin_count == 0);
> + GEM_BUG_ON(ce->pin_count == 0);
>
> - if (--ctx->engine[engine->id].pin_count)
> + if (--ce->pin_count)
> return;
>
> - intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
> + intel_unpin_ringbuffer_obj(ce->ringbuf);
>
> - ctx_obj = ctx->engine[engine->id].state;
> - i915_gem_object_unpin_map(ctx_obj);
> - i915_gem_object_ggtt_unpin(ctx_obj);
> + i915_gem_object_unpin_map(ce->state);
> + i915_gem_object_ggtt_unpin(ce->state);
>
> - ctx->engine[engine->id].lrc_vma = NULL;
> - ctx->engine[engine->id].lrc_desc = 0;
> - ctx->engine[engine->id].lrc_reg_state = NULL;
> + ce->lrc_vma = NULL;
> + ce->lrc_desc = 0;
> + ce->lrc_reg_state = NULL;
>
> i915_gem_context_put(ctx);
> }
> @@ -2493,12 +2492,13 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine)
> {
> struct drm_i915_gem_object *ctx_obj;
> + struct intel_context *ce = &ctx->engine[engine->id];
> uint32_t context_size;
> struct intel_ringbuffer *ringbuf;
> int ret;
>
> WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL);
> - WARN_ON(ctx->engine[engine->id].state);
> + WARN_ON(ce->state);
>
> context_size = round_up(intel_lr_context_size(engine), 4096);
>
> @@ -2523,9 +2523,9 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
> goto error_ringbuf;
> }
>
> - ctx->engine[engine->id].ringbuf = ringbuf;
> - ctx->engine[engine->id].state = ctx_obj;
> - ctx->engine[engine->id].initialised = engine->init_context == NULL;
> + ce->ringbuf = ringbuf;
> + ce->state = ctx_obj;
> + ce->initialised = engine->init_context == NULL;
>
> return 0;
>
> @@ -2533,8 +2533,8 @@ error_ringbuf:
> intel_ringbuffer_free(ringbuf);
> error_deref_obj:
> drm_gem_object_unreference(&ctx_obj->base);
> - ctx->engine[engine->id].ringbuf = NULL;
> - ctx->engine[engine->id].state = NULL;
> + ce->ringbuf = NULL;
> + ce->state = NULL;
> return ret;
> }
>
> @@ -2544,10 +2544,8 @@ void intel_lr_context_reset(struct drm_i915_private *dev_priv,
> struct intel_engine_cs *engine;
>
> for_each_engine(engine, dev_priv) {
> - struct drm_i915_gem_object *ctx_obj =
> - ctx->engine[engine->id].state;
> - struct intel_ringbuffer *ringbuf =
> - ctx->engine[engine->id].ringbuf;
> + struct intel_context *ce = &ctx->engine[engine->id];
> + struct drm_i915_gem_object *ctx_obj = ce->state;
> void *vaddr;
> uint32_t *reg_state;
>
> @@ -2566,7 +2564,7 @@ void intel_lr_context_reset(struct drm_i915_private *dev_priv,
>
> i915_gem_object_unpin_map(ctx_obj);
>
> - ringbuf->head = 0;
> - ringbuf->tail = 0;
> + ce->ringbuf->head = 0;
> + ce->ringbuf->tail = 0;
> }
> }
>
Looks OK anyway.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-05-23 9:26 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-22 13:02 [PATCH 01/12] drm/i915/fbdev: Limit the global async-domain synchronization Chris Wilson
2016-05-22 13:02 ` [PATCH 02/12] drm/i915: Rename struct intel_context Chris Wilson
2016-05-23 9:09 ` Tvrtko Ursulin
2016-05-22 13:02 ` [PATCH 03/12] drm/i915: Apply lockdep annotations to i915_gem_context.c Chris Wilson
2016-05-23 9:11 ` Tvrtko Ursulin
2016-05-22 13:02 ` [PATCH 04/12] drm/i915: Rename and inline i915_gem_context_get() Chris Wilson
2016-05-23 9:15 ` Tvrtko Ursulin
2016-05-22 13:02 ` [PATCH 05/12] drm/i915: Rename i915_gem_context_reference/unreference() Chris Wilson
2016-05-23 9:17 ` Tvrtko Ursulin
2016-05-23 9:25 ` Chris Wilson
2016-05-24 8:10 ` Daniel Vetter
2016-05-22 13:02 ` [PATCH 06/12] drm/i915: Name the inner most per-engine intel_context struct Chris Wilson
2016-05-23 9:26 ` Tvrtko Ursulin [this message]
2016-05-23 10:17 ` Chris Wilson
2016-05-23 10:55 ` Tvrtko Ursulin
2016-05-23 11:08 ` Chris Wilson
2016-05-22 13:02 ` [PATCH 07/12] drm/i915: Move pinning of dev_priv->kernel_context into its creator Chris Wilson
2016-05-23 9:33 ` Tvrtko Ursulin
2016-05-23 9:45 ` Chris Wilson
2016-05-22 13:02 ` [PATCH 08/12] drm/i915: Show i915_gem_context owner in debugfs Chris Wilson
2016-05-23 9:42 ` Tvrtko Ursulin
2016-05-23 9:52 ` Chris Wilson
2016-05-22 13:02 ` [PATCH 09/12] drm/i915: Put the kernel_context in drm_i915_private next to its friends Chris Wilson
2016-05-23 9:45 ` Tvrtko Ursulin
2016-05-22 13:02 ` [PATCH 10/12] drm/i915: Merge legacy+execlists context structs Chris Wilson
2016-05-23 10:26 ` Tvrtko Ursulin
2016-05-23 10:40 ` Chris Wilson
2016-05-22 13:02 ` [PATCH 11/12] drm/i915: Rearrange i915_gem_context Chris Wilson
2016-05-23 10:27 ` Tvrtko Ursulin
2016-05-22 13:02 ` [PATCH 12/12] drm/i915: Show context objects in debugfs/i915_gem_objects Chris Wilson
2016-05-24 8:13 ` Daniel Vetter
2016-05-24 8:21 ` Chris Wilson
2016-05-22 13:33 ` ✗ Ro.CI.BAT: failure for series starting with [01/12] drm/i915/fbdev: Limit the global async-domain synchronization Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5742CCCF.5010200@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.