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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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 <chris@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