On 8/19/2021 4:53 PM, Matthew Brost wrote:
On Thu, Aug 19, 2021 at 04:54:00PM -0700, Daniele Ceraolo Spurio wrote:
On 8/18/2021 11:16 PM, Matthew Brost wrote:
When unwinding requests on a reset context, if other requests in the
context are in the priority list the requests could be resubmitted out
of seqno order. Traverse the list of active requests in reverse and
append to the head of the priority list to fix this.
Fixes: eb5e7da736f3 ("drm/i915/guc: Reset implementation for new GuC interface")
Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 32c414aa9009..9ca0ba4ea85a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -805,9 +805,9 @@ __unwind_incomplete_requests(struct intel_context *ce)
spin_lock_irqsave(&sched_engine->lock, flags);
spin_lock(&ce->guc_active.lock);
- list_for_each_entry_safe(rq, rn,
- &ce->guc_active.requests,
- sched.link) {
+ list_for_each_entry_safe_reverse(rq, rn,
+ &ce->guc_active.requests,
+ sched.link) {
if (i915_request_completed(rq))
The execlists unwind function has a list_del if the request is completed.
Any reason not to do that here?
Def isn't needed here as this is done in remove_from_context(), probably
not needed in execlists mode either.
continue;
@@ -824,7 +824,7 @@ __unwind_incomplete_requests(struct intel_context *ce)
}
GEM_BUG_ON(i915_sched_engine_is_empty(sched_engine));
- list_add_tail(&rq->sched.link, pl);
+ list_add(&rq->sched.link, pl);
Since you always do both list_del and list_add and it doesn't look like you
use the fact that the list is empty between the 2 calls, you can merge them
in a list_move.
Can't use a list move here because we drop
spin_lock(&ce->guc_active.lock), that gets fixed later in the series and
at that point we likely can use a list_move.
fair enough. I'll leave it to you to decide if it is worth moving this
patch after the next one and using a list_move. With or without that,
this is:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Daniele
Matt
Apart from these nits, the change to navigate the list in reverse and append
here at the top LGTM.
Daniele
set_bit(I915_FENCE_FLAG_PQUEUE, &rq->fence.flags);
spin_lock(&ce->guc_active.lock);
