diff mbox series

[05/16] drm/i915/gt: Move the breadcrumb to the signaler if completed upon cancel

Message ID 20201124114219.29020-5-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [01/16] drm/i915/gem: Drop free_work for GEM contexts | expand

Commit Message

Chris Wilson Nov. 24, 2020, 11:42 a.m. UTC 
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(-)

Comments

Tvrtko Ursulin Nov. 24, 2020, 4:19 p.m. UTC | #1 
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);
>   }
>   
>
Chris Wilson Nov. 24, 2020, 4:30 p.m. UTC | #2 
Quoting Tvrtko Ursulin (2020-11-24 16:19:15)
> 
> 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?

static void irq_signal_request()

Yes, the completion must happen before the signal bit is set, and there
is race on who sets the signal bit.

So if, and only if, __signal_request() is the first to set the signal
bit do we keep the reference to the request and enqueue it to execute the
callbacks in the irq-worker.

If the request is completed, but someone else has already signaled the
request, the reference is dropped.

static bool __signal_request(struct i915_request *rq)
{
        GEM_BUG_ON(test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));

        if (!__dma_fence_signal(&rq->fence)) {
                i915_request_put(rq);
                return false;
        }

        return true;
}

I see your point that the reference handling is not obvious. Worth
taking another pass over it to split the different paths into their
different ways so the ownership is not hidden away.
-Chris
Tvrtko Ursulin Nov. 24, 2020, 5:02 p.m. UTC | #3 
On 24/11/2020 16:30, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-11-24 16:19:15)
>>
>> 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?
> 
> static void irq_signal_request()
> 
> Yes, the completion must happen before the signal bit is set, and there
> is race on who sets the signal bit.
> 
> So if, and only if, __signal_request() is the first to set the signal
> bit do we keep the reference to the request and enqueue it to execute the
> callbacks in the irq-worker.
> 
> If the request is completed, but someone else has already signaled the
> request, the reference is dropped.
> 
> static bool __signal_request(struct i915_request *rq)
> {
>          GEM_BUG_ON(test_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags));
> 
>          if (!__dma_fence_signal(&rq->fence)) {
>                  i915_request_put(rq);
>                  return false;
>          }
> 
>          return true;
> }
> 
> I see your point that the reference handling is not obvious. Worth
> taking another pass over it to split the different paths into their
> different ways so the ownership is not hidden away.

It looks like I confused irq_signal_request and __signal_request. Former 
has no return value so what I wrote makes no sense. And then it is 
indeed fine what the patch does but I do think at least a good comment 
under the if (__request_completed) branch in cancel breadcrumb is needed.

If irq_signal_request could be made return the result from 
__dma_fence_signal and if other callers would be able to easily handle 
the change even better. (I mean moving the i915_request_put out of 
__signal_request.)

Regards,

Tvrtko
diff mbox series

Patch

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;
+	}
+
 	i915_request_put(rq);
 }