Re: [Intel-gfx] [PATCH 06/16] drm/i915/gt: Decouple completed requests on unwind

[Tvrtko Ursulin]

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 <ch...@chris-wilson.co.uk>
---
  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?

Regards,

Tvrtko

        GEM_BUG_ON(!llist_empty(&rq->execute_cb));
  
  	__list_del_entry(&rq->link); /* poison neither prev/next (RCU walks) */

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx