[PATCH] dma-buf: Use dma_fence_unwrap_for_each when importing fences

[Jason Ekstrand]

On Mon, Aug 8, 2022 at 11:39 AM Jason Ekstrand <jason.ekstrand at collabora.com>
wrote:

> On Sun, 2022-08-07 at 18:35 +0200, Christian König wrote:
> > Am 02.08.22 um 23:01 schrieb Jason Ekstrand:
> > > Ever since 68129f431faa ("dma-buf: warn about containers in
> > > dma_resv object"),
> > > dma_resv_add_shared_fence will warn if you attempt to add a
> > > container fence.
> > > While most drivers were fine, fences can also be added to a
> > > dma_resv via the
> > > recently added DMA_BUF_IOCTL_IMPORT_SYNC_FILE.  Use
> > > dma_fence_unwrap_for_each
> > > to add each fence one at a time.
> > >
> > > Fixes: 594740497e99 ("dma-buf: Add an API for importing sync files
> > > (v10)")
> > > Signed-off-by: Jason Ekstrand <jason.ekstrand at collabora.com>
> > > Reported-by: Sarah Walker <Sarah.Walker at imgtec.com>
> > > Cc: Christian König <christian.koenig at amd.com>
> > > ---
> > >   drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++------
> > >   1 file changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > > index 630133284e2b..8d5d45112f52 100644
> > > --- a/drivers/dma-buf/dma-buf.c
> > > +++ b/drivers/dma-buf/dma-buf.c
> > > @@ -15,6 +15,7 @@
> > >   #include <linux/slab.h>
> > >   #include <linux/dma-buf.h>
> > >   #include <linux/dma-fence.h>
> > > +#include <linux/dma-fence-unwrap.h>
> > >   #include <linux/anon_inodes.h>
> > >   #include <linux/export.h>
> > >   #include <linux/debugfs.h>
> > > @@ -391,8 +392,10 @@ static long dma_buf_import_sync_file(struct
> > > dma_buf *dmabuf,
> > >                                      const void __user *user_data)
> > >   {
> > >         struct dma_buf_import_sync_file arg;
> > > -       struct dma_fence *fence;
> > > +       struct dma_fence *fence, *f;
> > >         enum dma_resv_usage usage;
> > > +       struct dma_fence_unwrap iter;
> > > +       unsigned int num_fences;
> > >         int ret = 0;
> > >
> > >         if (copy_from_user(&arg, user_data, sizeof(arg)))
> > > @@ -411,13 +414,21 @@ static long dma_buf_import_sync_file(struct
> > > dma_buf *dmabuf,
> > >         usage = (arg.flags & DMA_BUF_SYNC_WRITE) ?
> > > DMA_RESV_USAGE_WRITE :
> > >
> > > DMA_RESV_USAGE_READ;
> > >
> > > -       dma_resv_lock(dmabuf->resv, NULL);
> > > +       num_fences = 0;
> > > +       dma_fence_unwrap_for_each(f, &iter, fence)
> > > +               ++num_fences;
> > >
> > > -       ret = dma_resv_reserve_fences(dmabuf->resv, 1);
> > > -       if (!ret)
> > > -               dma_resv_add_fence(dmabuf->resv, fence, usage);
> > > +       if (num_fences > 0) {
> > > +               dma_resv_lock(dmabuf->resv, NULL);
> > >
> > > -       dma_resv_unlock(dmabuf->resv);
> > > +               ret = dma_resv_reserve_fences(dmabuf->resv,
> > > num_fences);
> >
> > That looks like it is misplaced.
> >
> > You *must* only lock the reservation object once and then add all
> > fences
> > in one go.
>
> That's what I'm doing.  Lock, reserve, add a bunch, unlock.  I am
> assuming that the iterator won't suddenly want to iterate more fences
> between my initial count and when I go to add them but I think that
> assumption is ok.
>

Bump.  This has been sitting here for a couple of weeks.  I still don't see
the problem.

--Jason


> --Jason
>
>
> > Thinking now about it we probably had a bug around that before as
> > well.
> > Going to double check tomorrow.
> >
> > Regards,
> > Christian.
> >
> > > +               if (!ret) {
> > > +                       dma_fence_unwrap_for_each(f, &iter, fence)
> > > +                               dma_resv_add_fence(dmabuf->resv, f,
> > > usage);
> > > +               }
> > > +
> > > +               dma_resv_unlock(dmabuf->resv);
> > > +       }
> > >
> > >         dma_fence_put(fence);
> > >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220824/9dacdfef/attachment.htm>