On Thu, Nov 03, 2016 at 04:31:14PM -0400, Rob Clark wrote:
> Currently with fence-array, we have a potential deadlock situation. If we
> fence_add_callback() on an array-fence, the array-fence's lock is aquired
> first, and in it's ->enable_signaling() callback, it will install cb's on
> it's array-member fences, so the array-member's lock is acquired second.
>
> But in the signal path, the array-member's lock is acquired first, and the
> array-fence's lock acquired second.
Could mention that this is only an issue if the same fence->lock
appeared more than once in the array. Which is possible.
> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
> index 67eb7c8..274bbb5 100644
> --- a/drivers/dma-buf/dma-fence-array.c
> +++ b/drivers/dma-buf/dma-fence-array.c
> @@ -43,9 +43,10 @@ static void dma_fence_array_cb_func(struct dma_fence *f,
> dma_fence_put(&array->base);
> }
>
> -static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
> +static void enable_signaling_worker(struct work_struct *w)
> {
> - struct dma_fence_array *array = to_dma_fence_array(fence);
> + struct dma_fence_array *array =
> + container_of(w, struct dma_fence_array, enable_signaling_worker);
> struct dma_fence_array_cb *cb = (void *)(&array[1]);
> unsigned i;
>
> @@ -63,11 +64,18 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
> if (dma_fence_add_callback(array->fences[i], &cb[i].cb,
> dma_fence_array_cb_func)) {
> dma_fence_put(&array->base);
> - if (atomic_dec_and_test(&array->num_pending))
> - return false;
> + if (atomic_dec_and_test(&array->num_pending)) {
> + dma_fence_signal(&array->base);
> + return;
> + }
> }
> }
> +}
>
> +static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
> +{
> + struct dma_fence_array *array = to_dma_fence_array(fence);
> + queue_work(system_unbound_wq, &array->enable_signaling_worker);
I think you need a dma_fence_get(fence) here with a corresponding put in
the worker. Sadly I still can't poke a hole in the lockdep warning.
What I might suggest is that we try
static bool is_signaled(struct dma_fence *fence)
{
if (test_bit(SIGNALED_BIT, &fence->flags))
return true;
return fence->ops->signaled && fence->ops->signaled(fence);
}
static bool dma_fence_array_enable_signaling(struct dma_fence *fence)
{
struct dma_fence_array *array = to_dma_fence_array(fence);
int num_pending = atomic_read(&array->num_pending);
int i;
for (i = 0; i < array->num_fences; i++)
if (is_signaled(array->fences[i]) && !--num_pending) {
atomic_set(&array->num_pending, 0);
return false;
}
dma_fence_get(&array->base);
queue_work(system_unbound_wq, &array->enable_signaling_worker);
}
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
