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 <ch...@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@linux.intel.com>
Below comments addressed,
Reviewed-by: Joonas Lahtinen <joonas.lahti...@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);
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
