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 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx