| Message ID | 1461521419-18086-13-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/04/16 19:10, Chris Wilson wrote: > Rather than reuse the current location of the context in the global GTT > for its hardware identifier, use the context's unique ID assigned to it > for its whole lifetime. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Hmmm I wonder whether this will work with the GuC? IIRC the GuC firmware requires the LRCA to be used as the CTX ID :( Or at least it used to, maybe that has changed now. .Dave. > --- > drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++------- > drivers/gpu/drm/i915/intel_lrc.c | 36 ++++++------------------------------ > drivers/gpu/drm/i915/intel_lrc.h | 3 --- > 3 files changed, 11 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 3bd2f89933ff..be2a4a0fae13 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2043,15 +2043,13 @@ static void i915_dump_lrc_obj(struct seq_file *m, > 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); > + > if (ctx_obj == NULL) { > - seq_printf(m, "Context on %s with no gem object\n", > - engine->name); > + seq_puts(m, "\tNot allocated\n"); > return; > } > > - seq_printf(m, "CONTEXT: %s %u\n", engine->name, > - intel_execlists_ctx_id(ctx, engine)); > - > if (!i915_gem_obj_ggtt_bound(ctx_obj)) > seq_puts(m, "\tNot bound in GGTT\n"); > else > @@ -2170,8 +2168,8 @@ static int i915_execlists(struct seq_file *m, void *data) > > seq_printf(m, "\t%d requests in queue\n", count); > if (head_req) { > - seq_printf(m, "\tHead request id: %u\n", > - intel_execlists_ctx_id(head_req->ctx, engine)); > + seq_printf(m, "\tHead request context: %u\n", > + head_req->ctx->hw_id); > seq_printf(m, "\tHead request tail: %u\n", > head_req->tail); > } > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 6179b591ee84..2ed7363f76ea 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -314,14 +314,12 @@ static void > intel_lr_context_descriptor_update(struct intel_context *ctx, > struct intel_engine_cs *engine) > { > - uint64_t lrca, desc; > + u64 desc; > > - lrca = ctx->engine[engine->id].lrc_vma->node.start + > - LRC_PPHWSP_PN * PAGE_SIZE; > - > - desc = engine->ctx_desc_template; /* bits 0-11 */ > - desc |= lrca; /* bits 12-31 */ > - desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */ > + desc = engine->ctx_desc_template; /* bits 0-11 */ > + desc |= ctx->engine[engine->id].lrc_vma->node.start + > + LRC_PPHWSP_PN * PAGE_SIZE; /* bits 12-31 */ > + desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits 32-51 */ > > ctx->engine[engine->id].lrc_desc = desc; > } > @@ -332,28 +330,6 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx, > return ctx->engine[engine->id].lrc_desc; > } > > -/** > - * intel_execlists_ctx_id() - get the Execlists Context ID > - * @ctx: Context to get the ID for > - * @ring: Engine to get the ID for > - * > - * Do not confuse with ctx->id! Unfortunately we have a name overload > - * here: the old context ID we pass to userspace as a handler so that > - * they can refer to a context, and the new context ID we pass to the > - * ELSP so that the GPU can inform us of the context status via > - * interrupts. > - * > - * The context ID is a portion of the context descriptor, so we can > - * just extract the required part from the cached descriptor. > - * > - * Return: 20-bits globally unique context ID. > - */ > -u32 intel_execlists_ctx_id(struct intel_context *ctx, > - struct intel_engine_cs *engine) > -{ > - return intel_lr_context_descriptor(ctx, engine) >> GEN8_CTX_ID_SHIFT; > -} > - > static void execlists_elsp_write(struct drm_i915_gem_request *rq0, > struct drm_i915_gem_request *rq1) > { > @@ -499,7 +475,7 @@ execlists_check_remove_request(struct intel_engine_cs *engine, u32 request_id) > if (!head_req) > return 0; > > - if (unlikely(intel_execlists_ctx_id(head_req->ctx, engine) != request_id)) > + if (unlikely(head_req->ctx->hw_id != request_id)) > return 0; > > WARN(head_req->elsp_submitted == 0, "Never submitted head request\n"); > diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h > index 461f1ef9b5c1..b17ab79333aa 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.h > +++ b/drivers/gpu/drm/i915/intel_lrc.h > @@ -114,9 +114,6 @@ void intel_lr_context_reset(struct drm_i915_private *dev_priv, > uint64_t intel_lr_context_descriptor(struct intel_context *ctx, > struct intel_engine_cs *engine); > > -u32 intel_execlists_ctx_id(struct intel_context *ctx, > - struct intel_engine_cs *engine); > - > /* Execlists */ > int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists); > struct i915_execbuffer_params; >
On Mon, Apr 25, 2016 at 04:21:58PM +0100, Dave Gordon wrote: > On 24/04/16 19:10, Chris Wilson wrote: > >Rather than reuse the current location of the context in the global GTT > >for its hardware identifier, use the context's unique ID assigned to it > >for its whole lifetime. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Hmmm I wonder whether this will work with the GuC? > IIRC the GuC firmware requires the LRCA to be used as the CTX ID :( > Or at least it used to, maybe that has changed now. I know you raised it last time, as far as I can tell the two are distinct in i915_guc_submission and you never pass the high 32 bits of the LRC context descriptor (the context id) to the GuC. i.e. guc_init_ctx_desc: ctx_desc = intel_lr_context_descriptor(ctx, engine); lrc->context_desc = (u32)ctx_desc; lrc->ring_lrca = i915_gem_object_gtt_offset(ctx_obj) + PAGE; lrc->context_id = client->ctx_index | engine->guc_id; The guc hasn't fared any worse then it did before applying the change... -Chris
On 25/04/16 16:48, Chris Wilson wrote: > On Mon, Apr 25, 2016 at 04:21:58PM +0100, Dave Gordon wrote: >> On 24/04/16 19:10, Chris Wilson wrote: >>> Rather than reuse the current location of the context in the global GTT >>> for its hardware identifier, use the context's unique ID assigned to it >>> for its whole lifetime. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Hmmm I wonder whether this will work with the GuC? >> IIRC the GuC firmware requires the LRCA to be used as the CTX ID :( >> Or at least it used to, maybe that has changed now. > > I know you raised it last time, as far as I can tell the two are > distinct in i915_guc_submission and you never pass the high 32 bits of > the LRC context descriptor (the context id) to the GuC. i.e. > > guc_init_ctx_desc: > ctx_desc = intel_lr_context_descriptor(ctx, engine); > lrc->context_desc = (u32)ctx_desc; > > lrc->ring_lrca = i915_gem_object_gtt_offset(ctx_obj) + PAGE; > lrc->context_id = client->ctx_index | engine->guc_id; > > The guc hasn't fared any worse then it did before applying the change... > -Chris I found a comment in the GuC firmware, // SubmissionByProxy: if KMD or other context submitted this // context. This means, ContextID is LRCA[31:20] But it turns out that's referring to the field that we always set to the LRCA anyway (since we DO use submission by proxy), but which would be used for other purposes in some other mode. So all OK :) Also, I've tested this patchset on SKL and we're not getting any complaints from the GuC :) One other thing I've noticed in the BSpec: it says the "s/w context id" is 21 bits rather than 20 (bits 52-32 inclusive). Then bits 53-54 are MBZ, and then the remaining 9 bits (55-63) are "GroupID" which seems to be a per-process identifier which is *also* required to be unique among active contexts. But maybe that's only when taken in combination with the LRCA, which is automatically unique among active contexts anyway? .Dave.
On Tue, Apr 26, 2016 at 09:06:00AM +0100, Dave Gordon wrote: > On 25/04/16 16:48, Chris Wilson wrote: > >On Mon, Apr 25, 2016 at 04:21:58PM +0100, Dave Gordon wrote: > >>On 24/04/16 19:10, Chris Wilson wrote: > >>>Rather than reuse the current location of the context in the global GTT > >>>for its hardware identifier, use the context's unique ID assigned to it > >>>for its whole lifetime. > >>> > >>>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>>Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Hmmm I wonder whether this will work with the GuC? > >>IIRC the GuC firmware requires the LRCA to be used as the CTX ID :( > >>Or at least it used to, maybe that has changed now. > > > >I know you raised it last time, as far as I can tell the two are > >distinct in i915_guc_submission and you never pass the high 32 bits of > >the LRC context descriptor (the context id) to the GuC. i.e. > > > >guc_init_ctx_desc: > > ctx_desc = intel_lr_context_descriptor(ctx, engine); > > lrc->context_desc = (u32)ctx_desc; > > > > lrc->ring_lrca = i915_gem_object_gtt_offset(ctx_obj) + PAGE; > > lrc->context_id = client->ctx_index | engine->guc_id; > > > >The guc hasn't fared any worse then it did before applying the change... > >-Chris > > I found a comment in the GuC firmware, > > // SubmissionByProxy: if KMD or other context submitted this > // context. This means, ContextID is LRCA[31:20] > > But it turns out that's referring to the field that we always set to > the LRCA anyway (since we DO use submission by proxy), but which > would be used for other purposes in some other mode. So all OK :) > > Also, I've tested this patchset on SKL and we're not getting any > complaints from the GuC :) > > One other thing I've noticed in the BSpec: it says the "s/w context > id" is 21 bits rather than 20 (bits 52-32 inclusive). Yup. Double checked, we can have 2x as many active contexts, yay! > Then bits > 53-54 are MBZ, and then the remaining 9 bits (55-63) are "GroupID" > which seems to be a per-process identifier which is *also* required > to be unique among active contexts. But maybe that's only when taken > in combination with the LRCA, which is automatically unique among > active contexts anyway? In theory, it holds that LRCA pinned whilst we think the context is active from the hardware point of view (certainly we never want to change the pages behind the GGTT whilst the hw is reading/writing from them). But keeping the context id alive for longer papers over some random GPU hangs on Braswell (when continually creating, using and destroinyg contexts). Afaict, the group id need to be only alive for as long as all contexts belonging to the group are active. It would be easy to just give a unique id to drm_i915_file, but as it is only a u8, we probably do have to track live groups. Not a big challenge, but it will add some runtime cost. A bridge to be crossed in future. -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 3bd2f89933ff..be2a4a0fae13 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2043,15 +2043,13 @@ static void i915_dump_lrc_obj(struct seq_file *m, 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); + if (ctx_obj == NULL) { - seq_printf(m, "Context on %s with no gem object\n", - engine->name); + seq_puts(m, "\tNot allocated\n"); return; } - seq_printf(m, "CONTEXT: %s %u\n", engine->name, - intel_execlists_ctx_id(ctx, engine)); - if (!i915_gem_obj_ggtt_bound(ctx_obj)) seq_puts(m, "\tNot bound in GGTT\n"); else @@ -2170,8 +2168,8 @@ static int i915_execlists(struct seq_file *m, void *data) seq_printf(m, "\t%d requests in queue\n", count); if (head_req) { - seq_printf(m, "\tHead request id: %u\n", - intel_execlists_ctx_id(head_req->ctx, engine)); + seq_printf(m, "\tHead request context: %u\n", + head_req->ctx->hw_id); seq_printf(m, "\tHead request tail: %u\n", head_req->tail); } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 6179b591ee84..2ed7363f76ea 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -314,14 +314,12 @@ static void intel_lr_context_descriptor_update(struct intel_context *ctx, struct intel_engine_cs *engine) { - uint64_t lrca, desc; + u64 desc; - lrca = ctx->engine[engine->id].lrc_vma->node.start + - LRC_PPHWSP_PN * PAGE_SIZE; - - desc = engine->ctx_desc_template; /* bits 0-11 */ - desc |= lrca; /* bits 12-31 */ - desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */ + desc = engine->ctx_desc_template; /* bits 0-11 */ + desc |= ctx->engine[engine->id].lrc_vma->node.start + + LRC_PPHWSP_PN * PAGE_SIZE; /* bits 12-31 */ + desc |= (u64)ctx->hw_id << GEN8_CTX_ID_SHIFT; /* bits 32-51 */ ctx->engine[engine->id].lrc_desc = desc; } @@ -332,28 +330,6 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx, return ctx->engine[engine->id].lrc_desc; } -/** - * intel_execlists_ctx_id() - get the Execlists Context ID - * @ctx: Context to get the ID for - * @ring: Engine to get the ID for - * - * Do not confuse with ctx->id! Unfortunately we have a name overload - * here: the old context ID we pass to userspace as a handler so that - * they can refer to a context, and the new context ID we pass to the - * ELSP so that the GPU can inform us of the context status via - * interrupts. - * - * The context ID is a portion of the context descriptor, so we can - * just extract the required part from the cached descriptor. - * - * Return: 20-bits globally unique context ID. - */ -u32 intel_execlists_ctx_id(struct intel_context *ctx, - struct intel_engine_cs *engine) -{ - return intel_lr_context_descriptor(ctx, engine) >> GEN8_CTX_ID_SHIFT; -} - static void execlists_elsp_write(struct drm_i915_gem_request *rq0, struct drm_i915_gem_request *rq1) { @@ -499,7 +475,7 @@ execlists_check_remove_request(struct intel_engine_cs *engine, u32 request_id) if (!head_req) return 0; - if (unlikely(intel_execlists_ctx_id(head_req->ctx, engine) != request_id)) + if (unlikely(head_req->ctx->hw_id != request_id)) return 0; WARN(head_req->elsp_submitted == 0, "Never submitted head request\n"); diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h index 461f1ef9b5c1..b17ab79333aa 100644 --- a/drivers/gpu/drm/i915/intel_lrc.h +++ b/drivers/gpu/drm/i915/intel_lrc.h @@ -114,9 +114,6 @@ void intel_lr_context_reset(struct drm_i915_private *dev_priv, uint64_t intel_lr_context_descriptor(struct intel_context *ctx, struct intel_engine_cs *engine); -u32 intel_execlists_ctx_id(struct intel_context *ctx, - struct intel_engine_cs *engine); - /* Execlists */ int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists); struct i915_execbuffer_params;