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

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

[Christian König]

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@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))
 
 struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
 					   struct dma_fence **fences,
-- 
2.25.1

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

[Karolina Drobnik]

Hi Christian,

On 12.07.2022 12:28, Christian König 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@amd.com>

After applying the patch, fence merging works and all sw_sync subtests 
are passing. Thanks for taking care of this.

Tested-by: Karolina Drobnik <karolina.drobnik@intel.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))
>   
>   struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
>   					   struct dma_fence **fences,

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

[Christian König]

Hi Karolina,

Am 12.07.22 um 14:04 schrieb Karolina Drobnik:
> Hi Christian,
>
> On 12.07.2022 12:28, Christian König 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@amd.com>
>
> After applying the patch, fence merging works and all sw_sync subtests 
> are passing. Thanks for taking care of this.
>
> Tested-by: Karolina Drobnik <karolina.drobnik@intel.com>

can anybody give me an rb or at least an Acked-by as well so that I can 
push this upstream?

Thanks,
Christian.

>
>> ---
>>   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))
>>     struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
>>                          struct dma_fence **fences,

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

[Alex Deucher]

On Tue, Jul 12, 2022 at 8:06 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Hi Karolina,
>
> Am 12.07.22 um 14:04 schrieb Karolina Drobnik:
> > Hi Christian,
> >
> > On 12.07.2022 12:28, Christian König 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@amd.com>
> >
> > After applying the patch, fence merging works and all sw_sync subtests
> > are passing. Thanks for taking care of this.
> >
> > Tested-by: Karolina Drobnik <karolina.drobnik@intel.com>
>
> can anybody give me an rb or at least an Acked-by as well so that I can
> push this upstream?

Patch makes sense.

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Thanks,
> Christian.
>
> >
> >> ---
> >>   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))
> >>     struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
> >>                          struct dma_fence **fences,
>

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2514 bytes --]

Hi Christian

Am 12.07.22 um 12:28 schrieb Christian König:
> This reverts commit 8f61973718485f3e89bc4f408f929048b7b47c83.

I only found this commit in drm-misc-next. Should the revert be 
cherry-picked into drm-misc-next-fixes?

Best regards
Thomas

> 
> 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@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))
>   
>   struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
>   					   struct dma_fence **fences,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Christian König]

Hi Thomas,

Am 14.07.22 um 10:40 schrieb Thomas Zimmermann:
> Hi Christian
>
> Am 12.07.22 um 12:28 schrieb Christian König:
>> This reverts commit 8f61973718485f3e89bc4f408f929048b7b47c83.
>
> I only found this commit in drm-misc-next. Should the revert be 
> cherry-picked into drm-misc-next-fixes?

yes for all three patches you just pinged me.

I've already tried to push them to drm-misc-next-fixes, but the patches 
somehow wouldn't apply. I think the -next-fixes branch was somehow 
lagging behind.

Thanks,
Christian.

>
> Best regards
> Thomas
>
>>
>> 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@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))
>>     struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
>>                          struct dma_fence **fences,
>

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3446 bytes --]

Hi

Am 14.07.22 um 10:49 schrieb Christian König:
> Hi Thomas,
> 
> Am 14.07.22 um 10:40 schrieb Thomas Zimmermann:
>> Hi Christian
>>
>> Am 12.07.22 um 12:28 schrieb Christian König:
>>> This reverts commit 8f61973718485f3e89bc4f408f929048b7b47c83.
>>
>> I only found this commit in drm-misc-next. Should the revert be 
>> cherry-picked into drm-misc-next-fixes?
> 
> yes for all three patches you just pinged me.
> 
> I've already tried to push them to drm-misc-next-fixes, but the patches 
> somehow wouldn't apply. I think the -next-fixes branch was somehow 
> lagging behind.

I just forwarded drm-misc-next-fixes to the latest state of drm-next. 
Chances are, these patches will apply now.

Best regards
Thomas

> 
> Thanks,
> Christian.
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> 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@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))
>>>     struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
>>>                          struct dma_fence **fences,
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Christian König]

Am 14.07.22 um 11:06 schrieb Thomas Zimmermann:
> Hi
>
> Am 14.07.22 um 10:49 schrieb Christian König:
>> Hi Thomas,
>>
>> Am 14.07.22 um 10:40 schrieb Thomas Zimmermann:
>>> Hi Christian
>>>
>>> Am 12.07.22 um 12:28 schrieb Christian König:
>>>> This reverts commit 8f61973718485f3e89bc4f408f929048b7b47c83.
>>>
>>> I only found this commit in drm-misc-next. Should the revert be 
>>> cherry-picked into drm-misc-next-fixes?
>>
>> yes for all three patches you just pinged me.
>>
>> I've already tried to push them to drm-misc-next-fixes, but the 
>> patches somehow wouldn't apply. I think the -next-fixes branch was 
>> somehow lagging behind.
>
> I just forwarded drm-misc-next-fixes to the latest state of drm-next. 
> Chances are, these patches will apply now.

Thanks, should I cherry pick them or are you going to do it?

And can we somehow make sure that when the drm-misc-next is merged into 
drm-next for upstreaming that drm-misc-next-fixes is up to date as well? 
That would make things much easier.

Thanks,
Christian.

>
> Best regards
> Thomas
>
>>
>> Thanks,
>> Christian.
>>
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> 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@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))
>>>>     struct dma_fence *__dma_fence_unwrap_merge(unsigned int 
>>>> num_fences,
>>>>                          struct dma_fence **fences,
>>>
>>
>

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4429 bytes --]

Hi

Am 14.07.22 um 11:13 schrieb Christian König:
> Am 14.07.22 um 11:06 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 14.07.22 um 10:49 schrieb Christian König:
>>> Hi Thomas,
>>>
>>> Am 14.07.22 um 10:40 schrieb Thomas Zimmermann:
>>>> Hi Christian
>>>>
>>>> Am 12.07.22 um 12:28 schrieb Christian König:
>>>>> This reverts commit 8f61973718485f3e89bc4f408f929048b7b47c83.
>>>>
>>>> I only found this commit in drm-misc-next. Should the revert be 
>>>> cherry-picked into drm-misc-next-fixes?
>>>
>>> yes for all three patches you just pinged me.
>>>
>>> I've already tried to push them to drm-misc-next-fixes, but the 
>>> patches somehow wouldn't apply. I think the -next-fixes branch was 
>>> somehow lagging behind.
>>
>> I just forwarded drm-misc-next-fixes to the latest state of drm-next. 
>> Chances are, these patches will apply now.
> 
> Thanks, should I cherry pick them or are you going to do it?

Please go ahead. You know best what needs to be reverted.

> 
> And can we somehow make sure that when the drm-misc-next is merged into 
> drm-next for upstreaming that drm-misc-next-fixes is up to date as well? 
> That would make things much easier.

It's the drm-misc maintainer's job; me in this case. I simply was late. 
I don't know if this update can be automated. For example, such that it 
automatically merges drm-next back into drm-misc-next-fixes afer -rc6. 
But agree that the transition period around -rc6 is a bit of a problem 
each time.

Best regards
Thomas

> 
> Thanks,
> Christian.
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> Thanks,
>>> Christian.
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>
>>>>> 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@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))
>>>>>     struct dma_fence *__dma_fence_unwrap_merge(unsigned int 
>>>>> num_fences,
>>>>>                          struct dma_fence **fences,
>>>>
>>>
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Daniel Vetter]

On Tue, 12 Jul 2022 at 12:28, Christian König
<ckoenig.leichtzumerken@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@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.
-Daniel

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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [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@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@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
>>
>

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

[Daniel Vetter]

On Wed, Aug 10, 2022 at 07:01:55PM +0200, Christian König wrote:
> Am 10.08.22 um 18:54 schrieb Daniel Vetter:
> > On Tue, 12 Jul 2022 at 12:28, Christian König
> > <ckoenig.leichtzumerken@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@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.

Ah ok. I guess we add that to the list of reasons why sync_file is a bit a
funny interface, and people should just use drm_syncobj instead :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch