On 23/09/2019 14:39, Chris Wilson wrote:
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.
Ok.
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 :)
I disliked stale since it has negative connotations and this is actually
normal/expected situation (albeit rare), but maybe it's just me. Old is
perhaps better. But you can also keep stale I guess.
+ 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?
I thought it does, but then I did not know what I would write so it's
useful. So I think it's fine after all.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
