[Intel-gfx] [PATCH] drm/i915: Mark the removal of the i915_request from the sched.link

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-01-20 19:47:08)
> 
> On 20/01/2020 17:57, Chris Wilson wrote:
> > Keep the rq->fence.flags consistent with the status of the
> > rq->sched.link, and clear the associated bits when decoupling the link
> > on retirement (as we may wish to inspect those flags independent of
> > other state).
> > 
> > Fixes: 32ff621fd744 ("drm/i915/gt: Allow temporary suspension of inflight requests")
> > References: https://gitlab.freedesktop.org/drm/intel/issues/997
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_request.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 9ed0d3bc7249..78a5f5d3c070 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -221,6 +221,8 @@ static void remove_from_engine(struct i915_request *rq)
> >               locked = engine;
> >       }
> >       list_del_init(&rq->sched.link);
> > +     clear_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
> 
> This one I think can not be set in retirement. Or there is a path?

No, I don't think there's one for pqueue, it was just being consistent.
> 
> [comes back after writing the comment below]
> 
> Race between completion to hold puts the request on hold, then request 
> completes just as it is un-held? It needs retire to happen at the right 
> time, driven by ...? Is this it?

Yeah, but the clear one I was thinking about is 

static bool hold_request(const struct i915_request *rq)
{
        struct i915_dependency *p;

        /*
         * If one of our ancestors is on hold, we must also be on hold,
         * otherwise we will bypass it and execute before it.
         */
        list_for_each_entry(p, &rq->sched.signalers_list, signal_link) {
                const struct i915_request *s =
                        container_of(p->signaler, typeof(*s), sched);

                if (s->engine != rq->engine)
                        continue;

                if (i915_request_on_hold(s))
                        return true;
        }

        return false;
}

where we check the rq->fence.flags which holds stale information.

> 
> > +     clear_bit(I915_FENCE_FLAG_HOLD, &rq->fence.flags);
> 
> This one I think indeed can race with completion.

Clear both for consistency, caught out once, may be caught out again on
the other.
-Chris