[Intel-gfx] [PATCH 06/16] drm/i915/gt: Decouple completed requests on unwind

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-11-24 17:13:02)
> 
> On 24/11/2020 11:42, Chris Wilson wrote:
> > Since the introduction of preempt-to-busy, requests can complete in the
> > background, even while they are not on the engine->active.requests list.
> > As such, the engine->active.request list itself is not in strict
> > retirement order, and we have to scan the entire list while unwinding to
> > not miss any. However, if the request is completed we currently leave it
> > on the list [until retirement], but we could just as simply remove it
> > and stop treating it as active. We would only have to then traverse it
> > once while unwinding in quick succession.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 6 ++++--
> >   drivers/gpu/drm/i915/i915_request.c | 3 ++-
> >   2 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 30aa59fb7271..cf11cbac241b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1116,8 +1116,10 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
> >       list_for_each_entry_safe_reverse(rq, rn,
> >                                        &engine->active.requests,
> >                                        sched.link) {
> > -             if (i915_request_completed(rq))
> > -                     continue; /* XXX */
> > +             if (i915_request_completed(rq)) {
> > +                     list_del_init(&rq->sched.link);
> > +                     continue;
> > +             }
> >   
> >               __i915_request_unsubmit(rq);
> >   
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 8d7d29c9e375..a9db1376b996 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -321,7 +321,8 @@ bool i915_request_retire(struct i915_request *rq)
> >        * after removing the breadcrumb and signaling it, so that we do not
> >        * inadvertently attach the breadcrumb to a completed request.
> >        */
> > -     remove_from_engine(rq);
> > +     if (!list_empty(&rq->sched.link))
> > +             remove_from_engine(rq);
> 
> The list_empty check is unlocked so is list_del_init in 
> remove_from_engine safe on potentially already unlinked request or it 
> needs to re-check under the lock?

It's safe. The unwind is under the lock, and remove_from_engine takes
the lock, and both do list_del_init() which is a no-op if already
removed. And the end state of the flag bits is the same on each path. We
can skip the __notify_execute_cb_imm() since we know in unwind it is
executing and there should be no cb.

The test before we take the lock is only allowed to skip the active.lock
if it sees the list is already decoupled, in which case we can leave it
to the unwind to remove it from the engine (and we know that the request
can only have been inflight prior to completion). Since the test is not
locked, we don't serialise with the removal, but the list_del_init is
the last action on the request so there is no window where the unwind is
accessing the request after it may have been retired.

list_move() will not confuse list_empty(), as although it does a
list_del_entry, it is not temporarily re-initialised to an empty list.
-Chris