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

[Christian König]

Am 24.08.22 um 16:47 schrieb 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.

I've send you a patch for a bug fix in the dma_resv object regarding this.

Apart from that I've just seen that I miss read the code a bit, didn't 
realized that there where two loops with dma_fence_unwrap_for_each().

Regards,
Christian.

>
> --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/0fe1d4dc/attachment-0001.htm>