Re: [Intel-gfx] [PATCH 05/16] drm/i915/gt: Move the breadcrumb to the signaler if completed upon cancel

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

On 24/11/2020 11:42, Chris Wilson wrote:
> If while we are cancelling the breadcrumb signaling, we find that the
> request is already completed, move it to the irq signaler and let it be
> signaled.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index a24cc1ff08a0..f5f6feed0fa6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -363,6 +363,14 @@ void intel_breadcrumbs_free(struct intel_breadcrumbs *b)
>   	kfree(b);
>   }
>   
> +static void irq_signal_request(struct i915_request *rq,
> +			       struct intel_breadcrumbs *b)
> +{
> +	if (__signal_request(rq) &&
> +	    llist_add(&rq->signal_node, &b->signaled_requests))
> +		irq_work_queue(&b->irq_work);
> +}
> +
>   static void insert_breadcrumb(struct i915_request *rq)
>   {
>   	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
> @@ -380,9 +388,7 @@ static void insert_breadcrumb(struct i915_request *rq)
>   	 * its signal completion.
>   	 */
>   	if (__request_completed(rq)) {
> -		if (__signal_request(rq) &&
> -		    llist_add(&rq->signal_node, &b->signaled_requests))
> -			irq_work_queue(&b->irq_work);
> +		irq_signal_request(rq, b);
>   		return;
>   	}
>   
> @@ -453,6 +459,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>   
>   void i915_request_cancel_breadcrumb(struct i915_request *rq)
>   {
> +	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
>   	struct intel_context *ce = rq->context;
>   	bool release;
>   
> @@ -461,11 +468,16 @@ void i915_request_cancel_breadcrumb(struct i915_request *rq)
>   
>   	spin_lock(&ce->signal_lock);
>   	list_del_rcu(&rq->signal_link);
> -	release = remove_signaling_context(rq->engine->breadcrumbs, ce);
> +	release = remove_signaling_context(b, ce);
>   	spin_unlock(&ce->signal_lock);
>   	if (release)
>   		intel_context_put(ce);
>   
> +	if (__request_completed(rq)) {
> +		irq_signal_request(rq, b);
> +		return;

This is a bit unintuitive - irq_signal_request does things conditionally 
based on the signaled flag, but here the return value is ignored and 
reference kept regardless. Which makes me wonder how can the combo of 
the two always dtrt. Because __request_completed is seqno based, which 
can happen before setting the signaled flag. Like if retire races with 
breadcrumbs. Am I missing something?

Regards,

Tvrtko

> +	}
> +
>   	i915_request_put(rq);
>   }
>   
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx