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.
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);
>
