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@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
