Re: [Intel-gfx] [PATCH] drm/i915/gt: Replace intel_engine_transfer_stale_breadcrumbs

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

On 17/07/2020 09:24, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-07-17 09:13:21)
>>
>> On 16/07/2020 18:28, Chris Wilson wrote:
>>> @@ -341,8 +325,10 @@ static void insert_breadcrumb(struct i915_request *rq,
>>>                        break;
>>>        }
>>>        list_add(&rq->signal_link, pos);
>>> -     if (pos == &ce->signals) /* catch transitions from empty list */
>>> +     if (pos == &ce->signals) { /* catch transitions from empty list */
>>>                list_move_tail(&ce->signal_link, &b->signalers);
>>> +             irq_work_queue(&b->irq_work); /* check after enabling irq */
>>> +     }
>>>        GEM_BUG_ON(!check_signal_order(ce, rq));
>>>    
>>>        set_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>>> @@ -401,7 +387,7 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>>>    
>>>        spin_unlock(&b->irq_lock);
>>>    
>>> -     return !__request_completed(rq);
>>> +     return true;
>>
>> Maybe my in head diff apply is failing me, but I think there isn't a
>> "return false" path left so could be made a return void function.
> 
> There is no return false path anymore (since we always queue the worker
> which should run immediately after dma_fence_enable_signaling if
> necessary, that seemed to be more sensible than conditionally using the
> worker, I also looked at splitting enable_breadcrumb and
> activate_breadcrumb, but the two paths are more similar than not), I
> kept it bool so that it matched i915_fence_enable_signaling.

It's a bit questionable, in this case it would probably be better to 
have explicit "return true" in i915_fence_enable_signaling. But it is a 
minor point anyway and bugfix trumps it.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx