[Intel-gfx] [PATCH] drm/i915: Only skip requests once a context is banned

[Tvrtko Ursulin]

On 03/01/2017 11:59, Chris Wilson wrote:
> If we skip before banning, we have an inconsistent interface between
> execbuf still queueing valid request but those requests already queued
> being cancelled. If we only cancel the pending requests once we stop
> accepting new requests, the interface is more consistent.
>
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0bfc99fcac8c..3d0eb9391d3d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2739,7 +2739,7 @@ static void reset_request(struct drm_i915_gem_request *request)
>  static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_gem_request *request;
> -	struct i915_gem_context *incomplete_ctx;
> +	struct i915_gem_context *hung_ctx;
>  	struct intel_timeline *timeline;
>  	unsigned long flags;
>  	bool ring_hung;
> @@ -2751,6 +2751,8 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	if (!request)
>  		return;
>
> +	hung_ctx = request->ctx;
> +
>  	ring_hung = engine->hangcheck.stalled;
>  	if (engine->hangcheck.seqno != intel_engine_get_seqno(engine)) {
>  		DRM_DEBUG_DRIVER("%s pardoned, was guilty? %s\n",
> @@ -2760,10 +2762,10 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	}
>
>  	if (ring_hung) {
> -		i915_gem_context_mark_guilty(request->ctx);
> +		i915_gem_context_mark_guilty(hung_ctx);
>  		request->fence.status = -EIO;
>  	} else {
> -		i915_gem_context_mark_innocent(request->ctx);
> +		i915_gem_context_mark_innocent(hung_ctx);
>  	}
>
>  	if (!ring_hung)
> @@ -2775,6 +2777,10 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	/* Setup the CS to resume from the breadcrumb of the hung request */
>  	engine->reset_hw(engine, request);
>
> +	/* If this context is now banned, skip all of its pending requests. */
> +	if (!i915_gem_context_is_banned(hung_ctx))
> +		return;
> +
>  	/* Users of the default context do not rely on logical state
>  	 * preserved between batches. They have to emit full state on
>  	 * every batch and so it is safe to execute queued requests following
> @@ -2783,17 +2789,16 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	 * Other contexts preserve state, now corrupt. We want to skip all
>  	 * queued requests that reference the corrupt context.
>  	 */
> -	incomplete_ctx = request->ctx;
> -	if (i915_gem_context_is_default(incomplete_ctx))
> +	if (i915_gem_context_is_default(hung_ctx))
>  		return;
>
> -	timeline = i915_gem_context_lookup_timeline(incomplete_ctx, engine);
> +	timeline = i915_gem_context_lookup_timeline(hung_ctx, engine);
>
>  	spin_lock_irqsave(&engine->timeline->lock, flags);
>  	spin_lock(&timeline->lock);
>
>  	list_for_each_entry_continue(request, &engine->timeline->requests, link)
> -		if (request->ctx == incomplete_ctx)
> +		if (request->ctx == hung_ctx)
>  			reset_request(request);
>
>  	list_for_each_entry(request, &timeline->requests, link)
>

LGTM, but hopefully Mika can also double-check.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko