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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On su, 2016-08-28 at 21:46 +0100, Chris Wilson wrote:
> +static void i915_sw_fence_free(struct kref *kref)
> +{
> +	struct i915_sw_fence *fence = container_of(kref, typeof(*fence), kref);
> +
> +	WARN_ON(atomic_read(&fence->pending) > 0);
> +
> +	if (fence->flags & I915_SW_FENCE_MASK)
> +		WARN_ON(__i915_sw_fence_notify(fence) != NOTIFY_DONE);

Suspicious to call _notify from free without any notification type
parameter. Better add the notification type parameter.

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

> +	unsigned long flags;
> +
> +	atomic_set(&fence->pending, -1); /* 0 -> -1 [done] */
> +
> +	/*
> +	 * To prevent unbounded recursion as we traverse the graph of
> +	 * i915_sw_fences, we move the task_list from this the next ready
> +	 * fence to the tail of the original fence's task_list

".. from this the next ready fence to ..." Strange expression.

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

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

> +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);

> +static bool i915_sw_fence_check_if_after(struct i915_sw_fence *fence,
> +				  const struct i915_sw_fence * const signaler)

Naming is still bad, but _err_if_after is not much better.

With above addressed;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

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