On ma, 2016-08-29 at 16:45 +0100, Chris Wilson wrote:
> On Mon, Aug 29, 2016 at 04:43:04PM +0300, Joonas Lahtinen wrote:
> >
> > On su, 2016-08-28 at 21:46 +0100, Chris Wilson wrote:
> > > + * (and so added to the list to be woken).
> > > + */
> > > +
> > > + smp_mb__before_spinlock();
> > > + spin_lock_irqsave_nested(&x->lock, flags, 1 + !!continuation);
> > > + if (continuation) {
> > > + list_splice_tail_init(&x->task_list, continuation);
> > > + } else {
> > > + LIST_HEAD(extra);
> > > +
> > > + do {
> > > + __wake_up_locked_key(x, TASK_NORMAL, &extra);
> > It might be worth mentioning here that we've rigged our callback so
> > that it will be called synchronously here so that the code can be
> > understood with less waitqueue internal digging.
> We're not the only set of callback on this list, we also allow for
> regular wait_event entries.
>
Right, but we're inspecting the extra variable without delays, which
will seem strange compared to conventional wake_up. So I would still
place a comment.
> >
> > >
> > > + if (list_empty(&extra))
> > > + break;
> > > +
> > > + list_splice_tail_init(&extra, &x->task_list);
> > > + } while (1);
> > Why exactly do you loop here, shouldn't single invocation of
> > __wake_up_locked_key trigger all the callbacks and result in all the
> > entries being listed?
> We handle recursion of fence completion by extending the task_list in
> the top-level fence, and handle the extra fence to be woken (which will
> remove them from the task list again) by looping.
Right, the code is definitely not obvious.
> > > + atomic_set(&fence->pending, 1);
> > fence->pending = ATOMIC_INIT(1);
> Tried. ATOMIC_INIT is only valid in declarations.
Seems so, duh.
You can apply the R-b.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
