| Message ID | 1461177750-20187-18-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote: > As the contexts are accessed by the hardware until the switch is completed > to a new context, the hardware may still be writing to the context object > after the breadcrumb is visible. We must not unpin/unbind/prune that > object whilst still active and so we keep the previous context pinned until > the following request. We can generalise the tracking we already do via > the engine->last_context and move it to the request so that it works > equally for execlists and GuC. > > v2: Drop the execlists double pin as that exposes a race inside the lrc > irq handler as it tries to access the context after it may be retired. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Below comments addressed, Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++ > drivers/gpu/drm/i915/i915_gem.c | 8 ++++---- > drivers/gpu/drm/i915/intel_lrc.c | 14 ++++++++------ > 3 files changed, 23 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a26a026ef8e2..515b8badce61 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2302,6 +2302,17 @@ struct drm_i915_gem_request { > struct intel_context *ctx; > struct intel_ringbuffer *ringbuf; > > + /** > + * Context related to the previous request. > + * As the contexts are accessed by the hardware until the switch is > + * completed to a new context, the hardware may still be writing > + * to the context object after the breadcrumb is visible. We must > + * not unpin/unbind/prune that object whilst still active and so > + * we keep the previous context pinned until the following (this) > + * request is retired. > + */ > + struct intel_context *previous_context; > + > /** Batch buffer related to this request if any (used for > error state dump only) */ > struct drm_i915_gem_object *batch_obj; > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index eaf1d13c943f..dbfc38f91f7d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1413,13 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) > list_del_init(&request->list); > i915_gem_request_remove_from_client(request); > > - if (request->ctx) { > + if (request->previous_context) { Could be if (i915.enable_execlists && request->previous_context) now. > if (i915.enable_execlists) > - intel_lr_context_unpin(request->ctx, request->engine); > - > - i915_gem_context_unreference(request->ctx); > + intel_lr_context_unpin(request->previous_context, > + request->engine); > } > > + i915_gem_context_unreference(request->ctx); Obviously some previous patch changed it so that request->ctx is never NULL at this point, as no more testing is done. > i915_gem_request_unreference(request); > } > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 8a544e2286f9..67c369ae649b 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -788,12 +788,14 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) > if (intel_engine_stopped(engine)) > return 0; > > - if (engine->last_context != request->ctx) { > - if (engine->last_context) > - intel_lr_context_unpin(engine->last_context, engine); > - intel_lr_context_pin(request->ctx, engine); > - engine->last_context = request->ctx; > - } > + /* We keep the previous context alive until we retire the following > + * request. This ensures that any the context object is still pinned > + * for any residual writes the HW makes into it on the context switch > + * into the next object following the breadcrumb. Otherwise, we may > + * retire the context too early. > + */ > + request->previous_context = engine->last_context; > + engine->last_context = request->ctx; > > if (dev_priv->guc.execbuf_client) > i915_guc_submit(dev_priv->guc.execbuf_client, request);
On Thu, Apr 21, 2016 at 10:07:50AM +0300, Joonas Lahtinen wrote: > On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote: > > As the contexts are accessed by the hardware until the switch is completed > > to a new context, the hardware may still be writing to the context object > > after the breadcrumb is visible. We must not unpin/unbind/prune that > > object whilst still active and so we keep the previous context pinned until > > the following request. We can generalise the tracking we already do via > > the engine->last_context and move it to the request so that it works > > equally for execlists and GuC. > > > > v2: Drop the execlists double pin as that exposes a race inside the lrc > > irq handler as it tries to access the context after it may be retired. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Below comments addressed, > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > --- > > drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++ > > drivers/gpu/drm/i915/i915_gem.c | 8 ++++---- > > drivers/gpu/drm/i915/intel_lrc.c | 14 ++++++++------ > > 3 files changed, 23 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index a26a026ef8e2..515b8badce61 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2302,6 +2302,17 @@ struct drm_i915_gem_request { > > struct intel_context *ctx; > > struct intel_ringbuffer *ringbuf; > > > > + /** > > + * Context related to the previous request. > > + * As the contexts are accessed by the hardware until the switch is > > + * completed to a new context, the hardware may still be writing > > + * to the context object after the breadcrumb is visible. We must > > + * not unpin/unbind/prune that object whilst still active and so > > + * we keep the previous context pinned until the following (this) > > + * request is retired. > > + */ > > + struct intel_context *previous_context; > > + > > /** Batch buffer related to this request if any (used for > > error state dump only) */ > > struct drm_i915_gem_object *batch_obj; > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index eaf1d13c943f..dbfc38f91f7d 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -1413,13 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) > > list_del_init(&request->list); > > i915_gem_request_remove_from_client(request); > > > > - if (request->ctx) { > > + if (request->previous_context) { > > Could be if (i915.enable_execlists && request->previous_context) now. I didn't do this because I intend (been trying!) to remove the execlists special case. So this will be if (request->previous_context) request->engine->unpin_context(request->engine, request->previous_context); Though the plan I actually have is to move the context over to activity tracking so that it finally behaves like legacy. And one step closer to removing the explicit ordering requirement between contexts. > > if (i915.enable_execlists) > > - intel_lr_context_unpin(request->ctx, request->engine); > > - > > - i915_gem_context_unreference(request->ctx); > > + intel_lr_context_unpin(request->previous_context, > > + request->engine); > > } > > > > + i915_gem_context_unreference(request->ctx); > > Obviously some previous patch changed it so that request->ctx is never > NULL at this point, as no more testing is done. Since requests. -Chris
On to, 2016-04-21 at 08:22 +0100, Chris Wilson wrote: > On Thu, Apr 21, 2016 at 10:07:50AM +0300, Joonas Lahtinen wrote: > > > > On ke, 2016-04-20 at 19:42 +0100, Chris Wilson wrote: > > > > > > As the contexts are accessed by the hardware until the switch is completed > > > to a new context, the hardware may still be writing to the context object > > > after the breadcrumb is visible. We must not unpin/unbind/prune that > > > object whilst still active and so we keep the previous context pinned until > > > the following request. We can generalise the tracking we already do via > > > the engine->last_context and move it to the request so that it works > > > equally for execlists and GuC. > > > > > > v2: Drop the execlists double pin as that exposes a race inside the lrc > > > irq handler as it tries to access the context after it may be retired. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Below comments addressed, > > > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > <SNIP> > > > index eaf1d13c943f..dbfc38f91f7d 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -1413,13 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) > > > list_del_init(&request->list); > > > i915_gem_request_remove_from_client(request); > > > > > > - if (request->ctx) { > > > + if (request->previous_context) { > > Could be if (i915.enable_execlists && request->previous_context) now. > I didn't do this because I intend (been trying!) to remove the execlists > special case. So this will be > > if (request->previous_context) > request->engine->unpin_context(request->engine, > request->previous_context); > > Though the plan I actually have is to move the context over to activity > tracking so that it finally behaves like legacy. And one step closer to > removing the explicit ordering requirement between contexts. > > > > > > > > > if (i915.enable_execlists) > > > - intel_lr_context_unpin(request->ctx, request->engine); > > > - > > > - i915_gem_context_unreference(request->ctx); > > > + intel_lr_context_unpin(request->previous_context, > > > + request->engine); > > > } > > > > > > + i915_gem_context_unreference(request->ctx); > > Obviously some previous patch changed it so that request->ctx is never > > NULL at this point, as no more testing is done. > Since requests. Might be worthy a note in the commit message. "Remove redundand check for request->ctx being NULL." Regards, Joonas > -Chris >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a26a026ef8e2..515b8badce61 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2302,6 +2302,17 @@ struct drm_i915_gem_request { struct intel_context *ctx; struct intel_ringbuffer *ringbuf; + /** + * Context related to the previous request. + * As the contexts are accessed by the hardware until the switch is + * completed to a new context, the hardware may still be writing + * to the context object after the breadcrumb is visible. We must + * not unpin/unbind/prune that object whilst still active and so + * we keep the previous context pinned until the following (this) + * request is retired. + */ + struct intel_context *previous_context; + /** Batch buffer related to this request if any (used for error state dump only) */ struct drm_i915_gem_object *batch_obj; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index eaf1d13c943f..dbfc38f91f7d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1413,13 +1413,13 @@ static void i915_gem_request_retire(struct drm_i915_gem_request *request) list_del_init(&request->list); i915_gem_request_remove_from_client(request); - if (request->ctx) { + if (request->previous_context) { if (i915.enable_execlists) - intel_lr_context_unpin(request->ctx, request->engine); - - i915_gem_context_unreference(request->ctx); + intel_lr_context_unpin(request->previous_context, + request->engine); } + i915_gem_context_unreference(request->ctx); i915_gem_request_unreference(request); } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 8a544e2286f9..67c369ae649b 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -788,12 +788,14 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request) if (intel_engine_stopped(engine)) return 0; - if (engine->last_context != request->ctx) { - if (engine->last_context) - intel_lr_context_unpin(engine->last_context, engine); - intel_lr_context_pin(request->ctx, engine); - engine->last_context = request->ctx; - } + /* We keep the previous context alive until we retire the following + * request. This ensures that any the context object is still pinned + * for any residual writes the HW makes into it on the context switch + * into the next object following the breadcrumb. Otherwise, we may + * retire the context too early. + */ + request->previous_context = engine->last_context; + engine->last_context = request->ctx; if (dev_priv->guc.execbuf_client) i915_guc_submit(dev_priv->guc.execbuf_client, request);
As the contexts are accessed by the hardware until the switch is completed to a new context, the hardware may still be writing to the context object after the breadcrumb is visible. We must not unpin/unbind/prune that object whilst still active and so we keep the previous context pinned until the following request. We can generalise the tracking we already do via the engine->last_context and move it to the request so that it works equally for execlists and GuC. v2: Drop the execlists double pin as that exposes a race inside the lrc irq handler as it tries to access the context after it may be retired. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 11 +++++++++++ drivers/gpu/drm/i915/i915_gem.c | 8 ++++---- drivers/gpu/drm/i915/intel_lrc.c | 14 ++++++++------ 3 files changed, 23 insertions(+), 10 deletions(-)