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

[Chris Wilson]

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:
> > +static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
> > +					struct list_head *continuation)
> > +{
> > +	wait_queue_head_t *x = &fence->wait;
> 
> Rather mystique variable naming to me.

>From trying to keep familarity with struct completion.

> > +	 * (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.

> > +			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.

> > +void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn)
> > +{
> > +	BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK);
> > +
> > +	init_waitqueue_head(&fence->wait);
> > +	kref_init(&fence->kref);
> > +	atomic_set(&fence->pending, 1);
> 
> fence->pending = ATOMIC_INIT(1);

Tried. ATOMIC_INIT is only valid in declarations.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre