Re: [PATCH 3/5] dma-buf: return only unsignaled fences in dma_fence_unwrap_for_each v3

From: Karolina Drobnik <karolina.drobnik@intel.com>
To: "Christian König" <christian.koenig@amd.com>,
	"Christian König" <ckoenig.leichtzumerken@gmail.com>
Cc: <daniel@ffwll.ch>, <linaro-mm-sig@lists.linaro.org>,
	<dri-devel@lists.freedesktop.org>, <linux-media@vger.kernel.org>
Subject: Re: [PATCH 3/5] dma-buf: return only unsignaled fences in dma_fence_unwrap_for_each v3
Date: Mon, 11 Jul 2022 14:17:38 +0200	[thread overview]
Message-ID: <fe4cfc97-ca20-b28d-8369-5cd404f27e19@intel.com> (raw)
In-Reply-To: <4c9598f0-4ba7-c18e-2ccd-f508769a72e9@amd.com>

Hi Christian,

On 11.07.2022 11:57, Christian König wrote:
> Hi Karolina,
> 
> Am 11.07.22 um 11:44 schrieb Karolina Drobnik:
>> Hi Christian,
>>
>> I'm sorry for digging this one out so late.
>>
>> On 06.05.2022 16:10, Christian König wrote:
>>> dma_fence_chain containers cleanup signaled fences automatically, so
>>> filter those out from arrays as well.
>>>
>>> v2: fix missing walk over the array
>>> v3: massively simplify the patch and actually update the description.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   include/linux/dma-fence-unwrap.h | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/dma-fence-unwrap.h 
>>> b/include/linux/dma-fence-unwrap.h
>>> index e7c219da4ed7..a4d342fef8e0 100644
>>> --- a/include/linux/dma-fence-unwrap.h
>>> +++ b/include/linux/dma-fence-unwrap.h
>>> @@ -43,9 +43,13 @@ 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))
>>> +         fence = dma_fence_unwrap_next(cursor))            \
>>> +        if (!dma_fence_is_signaled(fence))
>>>     #endif
>>
>> It looks like this particular patch affects merging Sync Fences, which 
>> is reflected by failing IGT test (igt@sw_sync)[1]. The failing 
>> subtests are:
>>   - sync_merge - merging different fences on the same timeline, neither
>>          single nor merged fences are signaled
>>
>>   - sync_merge_same - merging the fence with itself on the same
>>          timeline, the fence didn't signal at all
>>
>>   - sync_multi_timeline_wait - merging different fences on different
>>          timelines; the subtest checks if counting fences of
>>          various states works. Currently, it can only see 2
>>          active fences, 0 signaling (should be 2 active,
>>          1 signaling)
>>
>> Reverting this commit on the top of drm-tip fixes the issue, but I'm 
>> not sure if it wouldn't impact other places in the code. Please let me 
>> know if I can be of any help.
> 
> 
> Thanks for letting me know. Not sure what's going on here, but I can 
> take a look today if time permits.

The reproduction with IGTs should be quite easy. You'll need to 
clone/download the IGT code and follow instructions for Building[1] the 
project (make sure you have meson and ninja installed):

   https://gitlab.freedesktop.org/drm/igt-gpu-tools

Once you have it up and running, go to <igt path>/build/tests, and run 
the subtests:

   ./sw_sync --run sync_merge
   ./sw_sync --run sync_merge_same
   ./sw_sync --run sync_multi_timeline_wait

You can run all the subtests with ./sw_sync, but I think these are the 
most relevant to you.

Many thanks,
Karolina

------------------
[1] - https://gitlab.freedesktop.org/drm/igt-gpu-tools#building

> Do you have a description how to easy reproduce this? E.g. how to run 
> just those specific igts?
> 
> Thanks,
> Christian.
> 
>>
>> All the best,
>> Karolina
>>