Re: [Intel-gfx] [PATCH 06/12] drm/i915: Name the inner most per-engine intel_context struct

[Chris Wilson]

On Mon, May 23, 2016 at 11:55:16AM +0100, Tvrtko Ursulin wrote:
> 
> On 23/05/16 11:17, Chris Wilson wrote:
> >On Mon, May 23, 2016 at 10:26:39AM +0100, Tvrtko Ursulin wrote:
> >>>@@ -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.
> >
> >I was leaning the other way, since the code doesn't want
> >intel_lr_context_descriptor() just happens to want to reuse some of the
> >bits e.g. engine->ctx_desc_template | lrca
> 
> Thats true, but it was at least using an exported function with
> documented content, rather than directly fishing out stuff from
> essentially private data elsewhere.

It's still fishing out essentially private data though :)
If engine->ctx_desc_template considers GuC for its set of flags, it is
purely by happenstance.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 06/12] drm/i915: Name the inner most per-engine intel_context struct

[Tvrtko Ursulin]

On 23/05/16 11:17, Chris Wilson wrote:

On Mon, May 23, 2016 at 10:26:39AM +0100, Tvrtko Ursulin wrote:

@@ -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.


I was leaning the other way, since the code doesn't want
intel_lr_context_descriptor() just happens to want to reuse some of the
bits e.g. engine->ctx_desc_template | lrca


Thats true, but it was at least using an exported function with 
documented content, rather than directly fishing out stuff from 
essentially private data elsewhere.



i.e. lrc->context_desc != ce->lrc_desc and I would prefer it to be
clarified as to exactly what the GuC expects.


Yes that would be best. Cc-ed Dave for when he is back to comment what 
does GuC really wants in there.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 06/12] drm/i915: Name the inner most per-engine intel_context struct

[Chris Wilson]

On Mon, May 23, 2016 at 10:26:39AM +0100, Tvrtko Ursulin wrote:
> >@@ -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.

I was leaning the other way, since the code doesn't want
intel_lr_context_descriptor() just happens to want to reuse some of the
bits e.g. engine->ctx_desc_template | lrca

i.e. lrc->context_desc != ce->lrc_desc and I would prefer it to be
clarified as to exactly what the GuC expects.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 06/12] drm/i915: Name the inner most per-engine intel_context struct

[Tvrtko Ursulin]

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 
Cc: Dave Gordon 
Cc: Tvrtko Ursulin 
---
  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 > (1engine[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;
+  

[Intel-gfx] [PATCH 06/12] drm/i915: Name the inner most per-engine intel_context struct

[Chris Wilson]

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 
Cc: Dave Gordon 
Cc: Tvrtko Ursulin 
---
 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);
 
/* 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 > (1engine[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 @@ s