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

[Karolina Drobnik <karolina.drobnik@xxxxxxxxx>]

Hi Christian,

On 11.07.2022 14:25, Christian König wrote:
> Hi Karolina,
>
> Am 11.07.22 um 14:17 schrieb Karolina Drobnik:
> > 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@xxxxxxx>
> > > > > ---
> > > > >   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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C9a9587aefd2d4ac2d86208da63375cb6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637931386683611766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=4WsMutcFJ2HwBqld%2BTv9N1Tx6cbFMwJJZ6kjm5rbfoI%3D&amp;reserved=0 
> >
> >
> > 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.
>
> Thanks, I've already managed to reproduce it.
>
> Not sure what's going on here, but could be that the test case was never 
> correct in the first place. Need to double check.

That's also a possibility, but I couldn't verify it before writing to 
you, as it's not my area of expertise.

Thanks for taking a look at this.

All the best,
Karolina

> Thanks,
> Christian.
>
> > Many thanks,
> > Karolina
> >
> > ------------------
> > [1] - 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%23building&amp;data=05%7C01%7Cchristian.koenig%40amd.com%7C9a9587aefd2d4ac2d86208da63375cb6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637931386683611766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=FV0Ao6ra8EOyr4cOs4N7mCmpOEUUObTrgyOrd0tvEV8%3D&amp;reserved=0 
> >
> >
> > > 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