[Intel-gfx] [PATCH 16/21] drm/i915/gt: Track signaled breadcrumbs outside of the breadcrumb spinlock

[Tvrtko Ursulin]

On 31/07/2020 16:12, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-07-31 16:06:55)
>>
>> On 30/07/2020 10:37, Chris Wilson wrote:
>>> @@ -191,17 +188,19 @@ 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);
>>>    
>>>        spin_lock(&b->irq_lock);
>>>    
>>> -     if (list_empty(&b->signalers))
>>> +     if (!signal && list_empty(&b->signalers))
>>
>> The only open from previous round was on this change. If I understood
>> your previous reply correctly, checking this or not simply controls the
>> disarm point and is not related to this patch. With the check added now
>> we would disarm later, because even already signaled requests would keep
>> it armed. I would prefer this was a separate patch if you could possibly
>> be convinced.
> 
> I considered that since we add to the lockless list and then queue the
> irq work, that is a path that is not driven by the interrupt and so
> causing an issue with the idea of the interrupt shadow. Having a simple
> test I thought was a positive side-effect to filter out the early
> irq_work.

I don't really follow. I look at it like this: No active signalers so we 
can disarm. What is the purpose of keeping the interrupt enabled if all 
that is on list are already completed requests? They will get signaled 
in the very same run of signal_irq_work so I don't see a connection with 
lockless list and keeping the interrupts on for longer.

Regards,

Tvrtko