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@xxxxxxxxx> 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@xxxxxxx>
> > > ---
> > > 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
