Re: [CI 22/25] drm/i915: Track the previous pinned context inside the request

[Dave Gordon <david.s.gordon@intel.com>]

On 28/04/16 09:56, 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>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---

Hmmm .. not sure this is going to play well with request reordering in 
the scheduler, or with preemption! I think it makes it a bit more 
difficult; perhaps we will just have to clear it for all preempted requests.

.Dave.

>   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 0bd9d17ff974..eb4a95b853dd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2305,6 +2305,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 e4c8a8b998d1..fc68af50ed1b 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 4be11f8d0e04..0682a79be02c 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -771,12 +771,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);
>

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