[Intel-gfx] [PATCH] dma-buf: revert "return only unsignaled fences in dma_fence_unwrap_for_each v3"

[Christian König]

Am 10.08.22 um 18:54 schrieb Daniel Vetter:
> On Tue, 12 Jul 2022 at 12:28, Christian König
> <ckoenig.leichtzumerken at gmail.com> wrote:
>> This reverts commit 8f61973718485f3e89bc4f408f929048b7b47c83.
>>
>> It turned out that this is not correct. Especially the sync_file info
>> IOCTL needs to see even signaled fences to correctly report back their
>> status to userspace.
>>
>> Instead add the filter in the merge function again where it makes sense.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/dma-buf/dma-fence-unwrap.c | 3 ++-
>>   include/linux/dma-fence-unwrap.h   | 6 +-----
>>   2 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c
>> index 502a65ea6d44..7002bca792ff 100644
>> --- a/drivers/dma-buf/dma-fence-unwrap.c
>> +++ b/drivers/dma-buf/dma-fence-unwrap.c
>> @@ -72,7 +72,8 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
>>          count = 0;
>>          for (i = 0; i < num_fences; ++i) {
>>                  dma_fence_unwrap_for_each(tmp, &iter[i], fences[i])
>> -                       ++count;
>> +                       if (!dma_fence_is_signaled(tmp))
>> +                               ++count;
>>          }
>>
>>          if (count == 0)
>> diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h
>> index 390de1ee9d35..66b1e56fbb81 100644
>> --- a/include/linux/dma-fence-unwrap.h
>> +++ b/include/linux/dma-fence-unwrap.h
>> @@ -43,14 +43,10 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor);
>>    * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all
>>    * potential fences in them. If @head is just a normal fence only that one is
>>    * returned.
>> - *
>> - * Note that signalled fences are opportunistically filtered out, which
>> - * means the iteration is potentially over no fence at all.
>>    */
>>   #define dma_fence_unwrap_for_each(fence, cursor, head)                 \
>>          for (fence = dma_fence_unwrap_first(head, cursor); fence;       \
>> -            fence = dma_fence_unwrap_next(cursor))                     \
>> -               if (!dma_fence_is_signaled(fence))
>> +            fence = dma_fence_unwrap_next(cursor))
> Not sure it's worth it, but could we still filter but keep the fence
> if there's an error?
>
> I'm honestly also not entirely sure whether error propagation is a
> terrific idea, since every single use-case I've seen in i915 was a
> mis-design and not good at all. So burning it all down and declaring
> the testcases invalid might be the right thing to do here.

This is not about error propagation.

The sync_file has an IOCTL which asks how many of the merged fences are 
already signaled. When we filter signaled fences here the result of this 
is always 0.

We have an igt test exercising this which reported that the IOCTL 
doesn't work any more.

Christian.

> -Daniel
>
>>   struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
>>                                             struct dma_fence **fences,
>> --
>> 2.25.1
>>
>