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@xxxxxxxxxxxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
