* [PATCH] dma-buf: Use dma_fence_unwrap_for_each when importing fences
@ 2022-08-02 21:01 Jason Ekstrand
2022-08-07 16:35 ` Christian König
0 siblings, 1 reply; 5+ messages in thread
From: Jason Ekstrand @ 2022-08-02 21:01 UTC (permalink / raw)
To: dri-devel; +Cc: Jason Ekstrand, Sarah Walker, Christian König
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);
+ 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);
--
2.36.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-buf: Use dma_fence_unwrap_for_each when importing fences
2022-08-02 21:01 [PATCH] dma-buf: Use dma_fence_unwrap_for_each when importing fences Jason Ekstrand
@ 2022-08-07 16:35 ` Christian König
2022-08-08 16:39 ` Jason Ekstrand
0 siblings, 1 reply; 5+ messages in thread
From: Christian König @ 2022-08-07 16:35 UTC (permalink / raw)
To: Jason Ekstrand, dri-devel; +Cc: Jason Ekstrand, Sarah Walker
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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-buf: Use dma_fence_unwrap_for_each when importing fences
2022-08-07 16:35 ` Christian König
@ 2022-08-08 16:39 ` Jason Ekstrand
2022-08-24 14:47 ` Jason Ekstrand
0 siblings, 1 reply; 5+ messages in thread
From: Jason Ekstrand @ 2022-08-08 16:39 UTC (permalink / raw)
To: Christian König, Jason Ekstrand, dri-devel; +Cc: Sarah Walker
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);
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-buf: Use dma_fence_unwrap_for_each when importing fences
2022-08-08 16:39 ` Jason Ekstrand
@ 2022-08-24 14:47 ` Jason Ekstrand
2022-08-24 15:05 ` Christian König
0 siblings, 1 reply; 5+ messages in thread
From: Jason Ekstrand @ 2022-08-24 14:47 UTC (permalink / raw)
To: Jason Ekstrand; +Cc: Sarah Walker, Christian König, dri-devel
[-- Attachment #1: Type: text/plain, Size: 3897 bytes --]
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);
> > >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 5874 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] dma-buf: Use dma_fence_unwrap_for_each when importing fences
2022-08-24 14:47 ` Jason Ekstrand
@ 2022-08-24 15:05 ` Christian König
0 siblings, 0 replies; 5+ messages in thread
From: Christian König @ 2022-08-24 15:05 UTC (permalink / raw)
To: Jason Ekstrand, Jason Ekstrand; +Cc: Sarah Walker, dri-devel
[-- Attachment #1: Type: text/plain, Size: 4812 bytes --]
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);
> > >
> >
>
[-- Attachment #2: Type: text/html, Size: 10201 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-24 20:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-02 21:01 [PATCH] dma-buf: Use dma_fence_unwrap_for_each when importing fences Jason Ekstrand
2022-08-07 16:35 ` Christian König
2022-08-08 16:39 ` Jason Ekstrand
2022-08-24 14:47 ` Jason Ekstrand
2022-08-24 15:05 ` Christian König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).