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

[Chris Wilson]

Quoting Abodunrin, Akeem G (2020-06-16 22:31:45)
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Chris
> > Wilson
> > Sent: Tuesday, June 16, 2020 12:02 PM
> > To: intel-gfx at lists.freedesktop.org
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Subject: [Intel-gfx] [PATCH] drm/i915/gt: Decouple completed requests on
> > unwind
> > 
> > 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 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index e866b8d721ed..4eb397b0e14d 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1114,8 +1114,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);
> 
> Albeit this seems like a valid approach to resolve inconsistence in the list of requests that are active or retired, but we can't just delete completed requests from the list until full retirement is done - otherwise we stand the risk of out-of-the-order list, and could lead to inconsistence (which is the original problem you intend to resolve). Have you thought about locking mechanism?

The list is always in execution [context] order. Within a context it is
in retirement order. It is irrelevant whether it is removed here or in
remove_from_engine().
-Chris