Re: [Intel-gfx] [PATCH 4/7] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock

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

On 07/08/2020 12:54, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-08-07 12:26:41)
>>
>> On 07/08/2020 09:32, Chris Wilson wrote:
>>>    static void signal_irq_work(struct irq_work *work)
>>>    {
>>>        struct intel_breadcrumbs *b = container_of(work, typeof(*b), irq_work);
>>>        const ktime_t timestamp = ktime_get();
>>> +     struct llist_node *signal, *sn;
>>>        struct intel_context *ce, *cn;
>>>        struct list_head *pos, *next;
>>> -     LIST_HEAD(signal);
>>> +
>>> +     signal = NULL;
>>> +     if (unlikely(!llist_empty(&b->signaled_requests)))
>>> +             signal = llist_del_all(&b->signaled_requests);
>>> @@ -242,7 +248,9 @@ static void signal_irq_work(struct irq_work *work)
>>>                         * spinlock as the callback chain may end up adding
>>>                         * more signalers to the same context or engine.
>>>                         */
>>> -                     __signal_request(rq, &signal);
>>> +                     clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags);
>>> +                     if (__signal_request(rq))
>>> +                             signal = __llist_add(&rq->signal_node, signal);
>>
>> Presumably here you count on no possible races, allowing for a more
>> optimized, custom, __llist_add. It needs a comment at minimum, or even
>> better just use llist_add.
> 
> It's a purely local singly linked list here. We own the
> request->signal_node as that is locked by the b->irq_lock and the
> clear_bit, and signal is a local variable.

True.. just a comment then please and with that:

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