[Intel-gfx] [PATCH 01/17] drm/i915: Add a sw fence for collecting up dma fences

[Joonas Lahtinen]

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