On Thu, Nov 3, 2016 at 5:41 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> 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.
hmm, lockdep will care more about lock classes than lock instances..
>> 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.
oh, right, extra get/put makes sense.. fwiw it should be easy enough
to trigger with some code that merges a sw-sync fence w/ other
fences..
> 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);
> }
hmm, I guess just to try to avoid the wq?
BR,
-R
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
