Re: [Intel-gfx] [PATCH 03/27] drm/i915/guc: Unwind context requests in reverse order

[Matthew Brost <matthew.brost@intel.com>]

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@intel.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> >   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);
>