Quoting Chris Wilson (2020-07-31 16:12:32)
> 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.
How about a compromise and I sell the patch with a comment:
/*
* Keep the irq armed until the interrupt after all listeners are gone.
*
* Enabling/disabling the interrupt is rather costly, roughly a couple
* of hundred microseconds. If we are proactive and enable/disable
* the interrupt around every request that wants a breadcrumb, we
* quickly drown in the extra orders of magnitude of latency imposed
* on request submission.
*
* So we try to be lazy, and keep the interrupts enabled until no
* more listeners appear within a breadcrumb interrupt interval (that
* is until a request completes that no one cares about). The
* observation is that listeners come in batches, and will often
* listen to a bunch of requests in succession.
*
* We also try to avoid raising too many interrupts, as they may
* be generated by userspace batches and it is unfortunately rather
* too easy to drown the CPU under a flood of GPU interrupts. Thus
* whenever no one appears to be listening, we turn off the interrupts.
* Fewer interrupts should conserve power -- at the very least, fewer
* interrupt draw less ire from other users of the system and tools
* like powertop.
*/
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
