[Intel-gfx] [PATCH 3/5] drm/i915: Fixup preempt-to-busy vs resubmission of a virtual request

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-09-23 14:32:23)
> 
> On 21/09/2019 10:55, Chris Wilson wrote:
> > As preempt-to-busy leaves the request on the HW as the resubmission is
> > processed, that request may complete in the background and even cause a
> > second virtual request to enter queue. This second virtual request
> > breaks our "single request in the virtual pipeline" assumptions.
> > Furthermore, as the virtual request may be completed and retired, we
> > lose the reference the virtual engine assumes is held. Normally, just
> > removing the request from the scheduler queue removes it from the
> > engine, but the virtual engine keeps track of its singleton request via
> > its ve->request. This pointer needs protecting with a reference.
> > 
> > Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_lrc.c | 34 ++++++++++++++++++++++-------
> >   1 file changed, 26 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 53bc4308793c..1b2bacc60300 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -529,7 +529,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
> >                               i915_request_cancel_breadcrumb(rq);
> >                               spin_unlock(&rq->lock);
> >                       }
> > -                     rq->engine = owner;
> >                       owner->submit_request(rq);
> >                       active = NULL;
> >               }
> > @@ -1248,6 +1247,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >                               submit = true;
> >                               last = rq;
> >                       }
> > +                     i915_request_put(rq);
> >   
> >                       if (!submit) {
> >                               spin_unlock(&ve->base.active.lock);
> > @@ -2535,6 +2535,7 @@ static void execlists_cancel_requests(struct intel_engine_cs *engine)
> >   
> >                       rq->engine = engine;
> >                       __i915_request_submit(rq);
> > +                     i915_request_put(rq);
> >   
> >                       ve->base.execlists.queue_priority_hint = INT_MIN;
> >               }
> > @@ -3787,7 +3788,9 @@ static void virtual_submission_tasklet(unsigned long data)
> >   
> >   static void virtual_submit_request(struct i915_request *rq)
> >   {
> > -     struct virtual_engine *ve = to_virtual_engine(rq->engine);
> > +     struct virtual_engine *ve = to_virtual_engine(rq->hw_context->engine);
> > +     struct i915_request *stale;
> > +     unsigned long flags;
> >   
> >       GEM_TRACE("%s: rq=%llx:%lld\n",
> >                 ve->base.name,
> > @@ -3796,15 +3799,30 @@ static void virtual_submit_request(struct i915_request *rq)
> >   
> >       GEM_BUG_ON(ve->base.submit_request != virtual_submit_request);
> >   
> > -     GEM_BUG_ON(ve->request);
> > -     GEM_BUG_ON(!list_empty(virtual_queue(ve)));
> > +     spin_lock_irqsave(&ve->base.active.lock, flags);
> > +
> > +     stale = ve->request;
> 
> fetch_and_zero so you don't have to set it to NULL a bit lower?

I iterated through xchg then fetch_and_zero, before settling on this
variant. My feeling was setting ve->request in both sides of the if()
was more balanced.

> s/stale/completed/, plus a comment describing preempt-to-busy is to blame?

completed is a little long, old? Already added a comment to remind about
the preempt-to-busy link :)

> > +     if (stale) {
> > +             GEM_BUG_ON(!i915_request_completed(stale));
> > +             __i915_request_submit(stale);
> > +             i915_request_put(stale);
> > +     }
> > +
> > +     if (i915_request_completed(rq)) {
> > +             __i915_request_submit(rq);
> > +             ve->request = NULL;
> > +     } else {
> > +             ve->base.execlists.queue_priority_hint = rq_prio(rq);
> > +             ve->request = i915_request_get(rq);
> > +             rq->engine = &ve->base; /* fixup from unwind */
> 
> The last line has me confused. Isn't this the normal veng rq submission 
> path?

It is also on the normal submission path.

> In which case rq->engine will already be set to veng.

Yes

> But on the 
> unwind path you have removed reset-back of rq->engine to owner. Ah.. the 
> unwind calls veng->submit_request on it, and then we end up in here.. 
> Okay, this is outside the normal path, I mean the else block has two 
> functions/paths, and this should be explained in a comment.

That was the intent of "fixup from unwind?"

I can squeeze in /* fixup __unwind_incomplete_requests */ is that more
clueful? Or do you think it needs more?
-Chris