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