[Intel-gfx] [PATCH 2/5] drm/i915: Only enqueue already completed requests

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 10:51:48)
> 
> On 21/09/2019 10:55, Chris Wilson wrote:
> > If we are asked to submit a completed request, just move it onto the
> > active-list without modifying it's payload. If we try to emit the
> > modified payload of a completed request, we risk racing with the
> > ring->head update during retirement which may advance the head past our
> > breadcrumb and so we generate a warning for the emission being behind
> > the RING_HEAD.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 45 +++++++++++++----------------
> >   drivers/gpu/drm/i915/i915_request.c | 28 ++++++++++--------
> >   drivers/gpu/drm/i915/i915_request.h |  2 +-
> >   3 files changed, 37 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 53e823d36b28..53bc4308793c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -806,6 +806,9 @@ static bool can_merge_rq(const struct i915_request *prev,
> >       GEM_BUG_ON(prev == next);
> >       GEM_BUG_ON(!assert_priority_queue(prev, next));
> >   
> > +     if (i915_request_completed(next))
> > +             return true;
> > +
> 
> This reads very un-intuitive. Why would it always be okay to merge 
> possibly unrelated requests? From which it follows it must be a hack of 
> some kind - from which it follows it needs a comment to explain it.

We don't submit a known completed request, hence there is no context
switch.

> > @@ -1256,11 +1244,16 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >                               GEM_BUG_ON(ve->siblings[0] != engine);
> >                       }
> >   
> > -                     __i915_request_submit(rq);
> > -                     if (!i915_request_completed(rq)) {
> > +                     if (__i915_request_submit(rq)) {
> >                               submit = true;
> >                               last = rq;
> >                       }
> > +
> > +                     if (!submit) {
> > +                             spin_unlock(&ve->base.active.lock);
> > +                             rb = rb_first_cached(&execlists->virtual);
> > +                             continue;
> > +                     }
> 
> This block also needs a comment I think. It's about skipping until first 
> incomplete request in the queue?

It's the same logic as before. It's just saying having detected that we
have a bunch of veng requests, keep searching that tree in decreasing
priority order until it is no longer relevant.

> > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> > index 9bd8538b1907..db1a0048a753 100644
> > --- a/drivers/gpu/drm/i915/i915_request.c
> > +++ b/drivers/gpu/drm/i915/i915_request.c
> > @@ -377,9 +377,10 @@ __i915_request_await_execution(struct i915_request *rq,
> >       return 0;
> >   }
> >   
> > -void __i915_request_submit(struct i915_request *request)
> > +bool __i915_request_submit(struct i915_request *request)
> >   {
> >       struct intel_engine_cs *engine = request->engine;
> > +     bool result = false;
> >   
> >       GEM_TRACE("%s fence %llx:%lld, current %d\n",
> >                 engine->name,
> > @@ -389,6 +390,9 @@ void __i915_request_submit(struct i915_request *request)
> >       GEM_BUG_ON(!irqs_disabled());
> >       lockdep_assert_held(&engine->active.lock);
> >   
> > +     if (i915_request_completed(request))
> > +             goto xfer;
> 
> A comment here as well I think to explain what happens next with 
> completed requests put on the active list. They just get removed during 
> retire? Do they need to even be put on that list?

They get removed during retire. They must be removed from the priority
queue; and if they had been retired already they would not be in the
priority queue, ergo at this point they are completed and not retired.
-Chris