[PATCH] dma-buf: fix dma_fence_array_signaled

[Christian König]

Am 08.11.24 um 14:01 schrieb Tvrtko Ursulin:
>
> On 08/11/2024 09:42, Christian König wrote:
>> The function silently assumed that signaling was already enabled for the
>> dma_fence_array. This meant that without enabling signaling first we 
>> would
>> never see forward progress.
>>
>> Fix that by falling back to testing each individual fence when signaling
>> isn't enabled yet.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/dma-buf/dma-fence-array.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence-array.c 
>> b/drivers/dma-buf/dma-fence-array.c
>> index 46ac42bcfac0..01203796827a 100644
>> --- a/drivers/dma-buf/dma-fence-array.c
>> +++ b/drivers/dma-buf/dma-fence-array.c
>> @@ -103,10 +103,22 @@ static bool 
>> dma_fence_array_enable_signaling(struct dma_fence *fence)
>>   static bool dma_fence_array_signaled(struct dma_fence *fence)
>>   {
>>       struct dma_fence_array *array = to_dma_fence_array(fence);
>> +    unsigned int i, num_pending;
>>   -    if (atomic_read(&array->num_pending) > 0)
>> +    num_pending = atomic_read(&array->num_pending);
>> +    if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, 
>> &array->base.flags)) {
>> +        if (!num_pending)
>> +            goto signal;
>>           return false;
>> +    }
>> +
>> +    for (i = 0; i < array->num_fences; ++i) {
>> +        if (dma_fence_is_signaled(array->fences[i]) && !--num_pending)
>> +            goto signal;
>> +    }
>> +    return false;
>
> Sampling num_pending (and decrementing) and test_bit from an unlocked 
> path makes one need to think if there are consequences, false 
> negatives, positives or something. Would it be fine to simplify like 
> the below?

Yeah I've played around with those ideas as well but came to the 
conclusion that neither of them are correct.

>
> static bool dma_fence_array_signaled(struct dma_fence *fence)
> {
>     struct dma_fence_array *array = to_dma_fence_array(fence);
>     unsigned int i;
>
>     if (atomic_read(&array->num_pending)) {
>         for (i = 0; i < array->num_fences; i++) {
>             if (!dma_fence_is_signaled(array->fences[i]))
>                 return false;

That's not correct. num_pending is not necessary equal to the number of 
fences in the array.

E.g. we have cases where num_pending is just 1 so that the 
dma_fence_array signals when *any* fence in it signals.

> }
>     }
>
>     dma_fence_array_clear_pending_error(array);
>     return true;
> }
>
> Or if the optimisation to not walk the array when signalling is 
> already enabled is deemed important, perhaps a less thinking inducing 
> way would be this:
...
> Decrementing locally cached num_pending in the loop I think does not 
> bring anything since when signalling is not enabled it will be stuck 
> at num_fences. So the loop walks the whole array versus bail on first 
> unsignalled, so latter even more efficient.

That is not for optimization but for correctness.

What the patch basically does is the following:
1. Grab the current value of num_pending.

2. Test if num_pending was potentially already modified because 
signaling is already enabled, if yes just test it and return the result.

3. If it wasn't modified go over the fences and see if we already have 
at least num_pending signaled.

I should probably add a code comment explaining that.

> In which case, should dma-fence-chain also be aligned to have the fast 
> path bail out?

Good point need to double check that code as well.

Thanks,
Christian.

>
> Regards,
>
> Tvrtko
>
>>   +signal:
>>       dma_fence_array_clear_pending_error(array);
>>       return true;
>>   }