| Message ID | 20220802210158.4162525-1-jason.ekstrand@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: Use dma_fence_unwrap_for_each when importing fences | expand |
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@collabora.com> > Reported-by: Sarah Walker <Sarah.Walker@imgtec.com> > Cc: Christian König <christian.koenig@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. 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); >
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@collabora.com> > > Reported-by: Sarah Walker <Sarah.Walker@imgtec.com> > > Cc: Christian König <christian.koenig@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. --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); > > >
On Mon, Aug 8, 2022 at 11:39 AM Jason Ekstrand <jason.ekstrand@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@collabora.com> > > > Reported-by: Sarah Walker <Sarah.Walker@imgtec.com> > > > Cc: Christian König <christian.koenig@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); > > > > > > >
Am 24.08.22 um 16:47 schrieb Jason Ekstrand: > On Mon, Aug 8, 2022 at 11:39 AM Jason Ekstrand > <jason.ekstrand@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@collabora.com> > > > Reported-by: Sarah Walker <Sarah.Walker@imgtec.com> > > > Cc: Christian König <christian.koenig@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); > > > > > >
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); + 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);
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@collabora.com> Reported-by: Sarah Walker <Sarah.Walker@imgtec.com> Cc: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)