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

[Daniele Ceraolo Spurio]

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 at intel.com>
>>> Cc: <stable at 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.

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 at intel.com>

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