Re: [Intel-gfx] [PATCH] drm/i915/execlists: Reclaim hanging virtual request

Tvrtko Ursulin
2020-01-21T11:20:43Z

On 21/01/2020 10:09, Chris Wilson wrote:
> If we encounter a hang on a virtual engine, as we process the hang the
> request may already have been moved back to the virtual engine (we are
> processing the hang on the physical engine). We need to reclaim the
> request from the virtual engine so that the locking is consistent and
> local to the real engine on which we will hold the request for error
> state capturing.
>
> Fixes: 748317386afb ("drm/i915/execlists: Offline error capture")
> Testcase: igt/gem_exec_balancer/hang
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_lrc.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 3a30767ff0c4..a0acf1898c1e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2582,6 +2582,26 @@ static void execlists_capture(struct intel_engine_cs *engine)
> cap->rq = active_request(cap->rq->context->timeline, cap->rq);
> GEM_BUG_ON(!cap->rq);
>
> + if (cap->rq->engine != engine) { /* preempted virtual engine */
> + struct virtual_engine *ve = to_virtual_engine(cap->rq->engine);
> + unsigned long flags;
> +
> + /*
> + * An unsubmitted request along a virtual engine will
> + * remain on the active (this) engine until we are able
> + * to process the context switch away (and so mark the
> + * context as no longer in flight). That cannot have happened
> + * yet, otherwise we would not be hanging!
> + */
> + spin_lock_irqsave(&ve->base.active.lock, flags);
> + GEM_BUG_ON(intel_context_inflight(cap->rq->context) != engine);
> + GEM_BUG_ON(ve->request != cap->rq);
> + ve->request = NULL;
> + spin_unlock_irqrestore(&ve->base.active.lock, flags);
> +
> + cap->rq->engine = engine;
> + }

Conceptually this I think belongs in execlists_hold, not capture. Since
hold has the responsibility to hold correctly.

Then also, this is all about a race with __unwind_incomplete_requests,
yes? If so would need to be done under the engine->active.lock or this
can still happen between now and execlists_hold.

Regards,

Tvrtko

> +
> /*
> * Remove the request from the execlists queue, and take ownership
> * of the request. We pass it to our worker who will _slowly_ compress