On 24/11/2020 17:31, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2020-11-24 17:13:02)
On 24/11/2020 11:42, Chris Wilson wrote:
Since the introduction of preempt-to-busy, requests can complete in the
background, even while they are not on the engine->active.requests list.
As such, the engine->active.request list itself is not in strict
retirement order, and we have to scan the entire list while unwinding to
not miss any. However, if the request is completed we currently leave it
on the list [until retirement], but we could just as simply remove it
and stop treating it as active. We would only have to then traverse it
once while unwinding in quick succession.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/gt/intel_lrc.c | 6 ++++--
drivers/gpu/drm/i915/i915_request.c | 3 ++-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 30aa59fb7271..cf11cbac241b 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1116,8 +1116,10 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
list_for_each_entry_safe_reverse(rq, rn,
&engine->active.requests,
sched.link) {
- if (i915_request_completed(rq))
- continue; /* XXX */
+ if (i915_request_completed(rq)) {
+ list_del_init(&rq->sched.link);
+ continue;
+ }
__i915_request_unsubmit(rq);
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 8d7d29c9e375..a9db1376b996 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -321,7 +321,8 @@ bool i915_request_retire(struct i915_request *rq)
* after removing the breadcrumb and signaling it, so that we do not
* inadvertently attach the breadcrumb to a completed request.
*/
- remove_from_engine(rq);
+ if (!list_empty(&rq->sched.link))
+ remove_from_engine(rq);
The list_empty check is unlocked so is list_del_init in
remove_from_engine safe on potentially already unlinked request or it
needs to re-check under the lock?
It's safe. The unwind is under the lock, and remove_from_engine takes
the lock, and both do list_del_init() which is a no-op if already
removed. And the end state of the flag bits is the same on each path. We
can skip the __notify_execute_cb_imm() since we know in unwind it is
executing and there should be no cb.
The test before we take the lock is only allowed to skip the active.lock
if it sees the list is already decoupled, in which case we can leave it
to the unwind to remove it from the engine (and we know that the request
can only have been inflight prior to completion). Since the test is not
locked, we don't serialise with the removal, but the list_del_init is
the last action on the request so there is no window where the unwind is
accessing the request after it may have been retired.
list_move() will not confuse list_empty(), as although it does a
list_del_entry, it is not temporarily re-initialised to an empty list.
List_del_init is indeed safe. List_move.. which one you think can race
with retire? Preempt-to-busy unwinding an almost completed request yet
again? Or even preempt timeout racing with completion?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
