[PATCH RESEND v2] drm/syncobj: handle NULL fence in syncobj_eventfd_entry_func
During syncobj_eventfd_entry_func, dma_fence_chain_find_seqno may set
the fence to NULL if the given seqno is signaled and a later seqno has
already been submitted. In that case, the eventfd should be signaled
immediately which currently does not happen.
This is a similar issue to the one addressed by b19926d4f3a6
("drm/syncobj: Deal with signalled fences in drm_syncobj_find_fence")
As a fix, if the return value of dma_fence_chain_find_seqno indicates
success but it sets the fence to NULL, we will assign a stub fence to
ensure the following code still signals the eventfd.
v1 -> v2: assign a stub fence instead of signaling the eventfd
Signed-off-by: Erik Kurzinger
Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
---
drivers/gpu/drm/drm_syncobj.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 6c82138b2c70..cb5b5838ccf3 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1441,10 +1441,20 @@ syncobj_eventfd_entry_func(struct drm_syncobj *syncobj,
/* This happens inside the syncobj lock */
fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, 1));
+ if (!fence)
+ return;
+
ret = dma_fence_chain_find_seqno(, entry->point);
- if (ret != 0 || !fence) {
+ if (ret != 0) {
+ /* The given seqno has not been submitted yet. */
dma_fence_put(fence);
return;
+ } else if (!fence) {
+ /* If dma_fence_chain_find_seqno returns 0 but sets the fence
+* to NULL, it implies that the given seqno is signaled and a
+* later seqno has already been submitted. Assign a stub fence
+* so that the eventfd still gets signaled below. */
+ fence = dma_fence_get_stub();
}
list_del_init(>node);
--
2.43.2
Re: [PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
It looks these these patches have still not been merged after one month, is
there anything more that needs to be done for this to happen?
On 1/25/24 10:12, Daniel Vetter wrote:
> On Fri, Jan 19, 2024 at 08:32:06AM -0800, Erik Kurzinger wrote:
>> When waiting for a syncobj timeline point whose fence has not yet been
>> submitted with the WAIT_FOR_SUBMIT flag, a callback is registered using
>> drm_syncobj_fence_add_wait and the thread is put to sleep until the
>> timeout expires. If the fence is submitted before then,
>> drm_syncobj_add_point will wake up the sleeping thread immediately which
>> will proceed to wait for the fence to be signaled.
>>
>> However, if the WAIT_AVAILABLE flag is used instead,
>> drm_syncobj_fence_add_wait won't get called, meaning the waiting thread
>> will always sleep for the full timeout duration, even if the fence gets
>> submitted earlier. If it turns out that the fence *has* been submitted
>> by the time it eventually wakes up, it will still indicate to userspace
>> that the wait completed successfully (it won't return -ETIME), but it
>> will have taken much longer than it should have.
>>
>> To fix this, we must call drm_syncobj_fence_add_wait if *either* the
>> WAIT_FOR_SUBMIT flag or the WAIT_AVAILABLE flag is set. The only
>> difference being that with WAIT_FOR_SUBMIT we will also wait for the
>> fence to be signaled after it has been submitted while with
>> WAIT_AVAILABLE we will return immediately.
>>
>> IGT test patch:
>> https://lists.freedesktop.org/archives/igt-dev/2024-January/067537.html
>>
>> v1 -> v2: adjust lockdep_assert_none_held_once condition
>>
>> Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
>> Signed-off-by: Erik Kurzinger
>
> Yeah I think this series catches now all the corner cases I spotted in v1.
> On the series:
>
> Reviewed-by: Daniel Vetter
>> ---
>> drivers/gpu/drm/drm_syncobj.c | 6 --
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 94ebc71e5be5..97be8b140599 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -1058,7 +1058,8 @@ static signed long
>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>> uint64_t *points;
>> uint32_t signaled_count, i;
>>
>> -if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
>> +if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
>> lockdep_assert_none_held_once();
>>
>> points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
>> @@ -1127,7 +1128,8 @@ static signed long
>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>> * fallthough and try a 0 timeout wait!
>> */
>>
>> -if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>> +if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
>> for (i = 0; i < count; ++i)
>> drm_syncobj_fence_add_wait(syncobjs[i], [i]);
>> }
>> --
>> 2.43.0
>>
>
Re: [PATCH] drm/syncobj: handle NULL fence in syncobj_eventfd_entry_func
Sorry, I realized there is a mistake in this patch after sending it out. It
results in a use-after-free of "entry". I've sent out an updated version which
should avoid the issue.
On 1/25/24 10:03, Erik Kurzinger wrote:
> During syncobj_eventfd_entry_func, dma_fence_chain_find_seqno may set
> the fence to NULL if the given seqno is signaled and a later seqno has
> already been submitted. In that case, the eventfd should be signaled
> immediately which currently does not happen.
>
> This is a similar issue to the one addressed by b19926d4f3a6
> ("drm/syncobj: Deal with signalled fences in drm_syncobj_find_fence")
>
> As a fix, if the return value of dma_fence_chain_find_seqno indicates
> success but it sets the fence to NULL, we should simply signal the
> eventfd immediately.
>
> Signed-off-by: Erik Kurzinger
> Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
> ---
> drivers/gpu/drm/drm_syncobj.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index e04965878a08..cc3af1084950 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1441,10 +1441,21 @@ syncobj_eventfd_entry_func(struct drm_syncobj
> *syncobj,
>
> /* This happens inside the syncobj lock */
> fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, 1));
> + if (!fence)
> + return;
> +
> ret = dma_fence_chain_find_seqno(, entry->point);
> - if (ret != 0 || !fence) {
> + if (ret != 0) {
> + /* The given seqno has not been submitted yet. */
> dma_fence_put(fence);
> return;
> + } else if (!fence) {
> + /* If dma_fence_chain_find_seqno returns 0 but sets the fence
> + * to NULL, it implies that the given seqno is signaled and a
> + * later seqno has already been submitted. Signal the eventfd
> + * immediately in that case. */
> + eventfd_signal(entry->ev_fd_ctx, 1);
> + syncobj_eventfd_entry_free(entry);
> }
>
> list_del_init(>node);
[PATCH v2] drm/syncobj: handle NULL fence in syncobj_eventfd_entry_func
During syncobj_eventfd_entry_func, dma_fence_chain_find_seqno may set
the fence to NULL if the given seqno is signaled and a later seqno has
already been submitted. In that case, the eventfd should be signaled
immediately which currently does not happen.
This is a similar issue to the one addressed by b19926d4f3a6
("drm/syncobj: Deal with signalled fences in drm_syncobj_find_fence")
As a fix, if the return value of dma_fence_chain_find_seqno indicates
success but it sets the fence to NULL, we will assign a stub fence to
ensure the following code still signals the eventfd.
v1 -> v2: assign a stub fence instead of signaling the eventfd
Signed-off-by: Erik Kurzinger
Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
---
drivers/gpu/drm/drm_syncobj.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index e04965878a08..10476204f8b0 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1441,10 +1441,20 @@ syncobj_eventfd_entry_func(struct drm_syncobj *syncobj,
/* This happens inside the syncobj lock */
fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, 1));
+ if (!fence)
+ return;
+
ret = dma_fence_chain_find_seqno(, entry->point);
- if (ret != 0 || !fence) {
+ if (ret != 0) {
+ /* The given seqno has not been submitted yet. */
dma_fence_put(fence);
return;
+ } else if (!fence) {
+ /* If dma_fence_chain_find_seqno returns 0 but sets the fence
+* to NULL, it implies that the given seqno is signaled and a
+* later seqno has already been submitted. Assign a stub fence
+* so that the eventfd still gets signaled below. */
+ fence = dma_fence_get_stub();
}
list_del_init(>node);
--
2.43.0
[PATCH] drm/syncobj: handle NULL fence in syncobj_eventfd_entry_func
During syncobj_eventfd_entry_func, dma_fence_chain_find_seqno may set
the fence to NULL if the given seqno is signaled and a later seqno has
already been submitted. In that case, the eventfd should be signaled
immediately which currently does not happen.
This is a similar issue to the one addressed by b19926d4f3a6
("drm/syncobj: Deal with signalled fences in drm_syncobj_find_fence")
As a fix, if the return value of dma_fence_chain_find_seqno indicates
success but it sets the fence to NULL, we should simply signal the
eventfd immediately.
Signed-off-by: Erik Kurzinger
Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
---
drivers/gpu/drm/drm_syncobj.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index e04965878a08..cc3af1084950 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1441,10 +1441,21 @@ syncobj_eventfd_entry_func(struct drm_syncobj *syncobj,
/* This happens inside the syncobj lock */
fence = dma_fence_get(rcu_dereference_protected(syncobj->fence, 1));
+ if (!fence)
+ return;
+
ret = dma_fence_chain_find_seqno(, entry->point);
- if (ret != 0 || !fence) {
+ if (ret != 0) {
+ /* The given seqno has not been submitted yet. */
dma_fence_put(fence);
return;
+ } else if (!fence) {
+ /* If dma_fence_chain_find_seqno returns 0 but sets the fence
+* to NULL, it implies that the given seqno is signaled and a
+* later seqno has already been submitted. Signal the eventfd
+* immediately in that case. */
+ eventfd_signal(entry->ev_fd_ctx, 1);
+ syncobj_eventfd_entry_free(entry);
}
list_del_init(>node);
--
2.43.0
[PATCH v2 3/3] drm/syncobj: call might_sleep before waiting for fence submission
If either the DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT or
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE flags are passed to
drm_syncobj_array_wait_timeout, the function might sleep if the fence at
one of the given timeline points has not yet been submitted. Therefore,
we should call might_sleep in that case to catch potential bugs.
Signed-off-by: Erik Kurzinger
---
drivers/gpu/drm/drm_syncobj.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index c59bb02e2c07..e04965878a08 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1062,8 +1062,10 @@ static signed long drm_syncobj_array_wait_timeout(struct
drm_syncobj **syncobjs,
uint32_t signaled_count, i;
if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
-DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
+DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
+ might_sleep();
lockdep_assert_none_held_once();
+ }
points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
if (points == NULL)
--
2.43.0
[PATCH v2 2/3] drm/syncobj: reject invalid flags in drm_syncobj_find_fence
The only flag that is meaningful to drm_syncobj_find_fence is DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT. It should return -EINVAL for any other flag bits. Signed-off-by: Erik Kurzinger --- drivers/gpu/drm/drm_syncobj.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 97be8b140599..c59bb02e2c07 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -448,6 +448,9 @@ int drm_syncobj_find_fence(struct drm_file *file_private, u64 timeout = nsecs_to_jiffies64(DRM_SYNCOBJ_WAIT_FOR_SUBMIT_TIMEOUT); int ret; + if (flags & ~DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) + return -EINVAL; + if (!syncobj) return -ENOENT; -- 2.43.0
[PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
When waiting for a syncobj timeline point whose fence has not yet been
submitted with the WAIT_FOR_SUBMIT flag, a callback is registered using
drm_syncobj_fence_add_wait and the thread is put to sleep until the
timeout expires. If the fence is submitted before then,
drm_syncobj_add_point will wake up the sleeping thread immediately which
will proceed to wait for the fence to be signaled.
However, if the WAIT_AVAILABLE flag is used instead,
drm_syncobj_fence_add_wait won't get called, meaning the waiting thread
will always sleep for the full timeout duration, even if the fence gets
submitted earlier. If it turns out that the fence *has* been submitted
by the time it eventually wakes up, it will still indicate to userspace
that the wait completed successfully (it won't return -ETIME), but it
will have taken much longer than it should have.
To fix this, we must call drm_syncobj_fence_add_wait if *either* the
WAIT_FOR_SUBMIT flag or the WAIT_AVAILABLE flag is set. The only
difference being that with WAIT_FOR_SUBMIT we will also wait for the
fence to be signaled after it has been submitted while with
WAIT_AVAILABLE we will return immediately.
IGT test patch:
https://lists.freedesktop.org/archives/igt-dev/2024-January/067537.html
v1 -> v2: adjust lockdep_assert_none_held_once condition
Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
Signed-off-by: Erik Kurzinger
---
drivers/gpu/drm/drm_syncobj.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 94ebc71e5be5..97be8b140599 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1058,7 +1058,8 @@ static signed long drm_syncobj_array_wait_timeout(struct
drm_syncobj **syncobjs,
uint64_t *points;
uint32_t signaled_count, i;
- if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
+ if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
+DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
lockdep_assert_none_held_once();
points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
@@ -1127,7 +1128,8 @@ static signed long drm_syncobj_array_wait_timeout(struct
drm_syncobj **syncobjs,
* fallthough and try a 0 timeout wait!
*/
- if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
+ if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
+DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
for (i = 0; i < count; ++i)
drm_syncobj_fence_add_wait(syncobjs[i], [i]);
}
--
2.43.0
[PATCH] drm/syncobj: call drm_syncobj_fence_add_wait when, WAIT_AVAILABLE flag is set
When waiting for a syncobj timeline point whose fence has not yet been
submitted with the WAIT_FOR_SUBMIT flag, a callback is registered using
drm_syncobj_fence_add_wait and the thread is put to sleep until the
timeout expires. If the fence is submitted before then,
drm_syncobj_add_point will wake up the sleeping thread immediately which
will proceed to wait for the fence to be signaled.
However, if the WAIT_AVAILABLE flag is used instead,
drm_syncobj_fence_add_wait won't get called, meaning the waiting thread
will always sleep for the full timeout duration, even if the fence gets
submitted earlier. If it turns out that the fence *has* been submitted
by the time it eventually wakes up, it will still indicate to userspace
that the wait completed successfully (it won't return -ETIME), but it
will have taken much longer than it should have.
To fix this, we must call drm_syncobj_fence_add_wait if *either* the
WAIT_FOR_SUBMIT flag or the WAIT_AVAILABLE flag is set. The only
difference being that with WAIT_FOR_SUBMIT we will also wait for the
fence to be signaled after it has been submitted while with
WAIT_AVAILABLE we will return immediately.
IGT test patch:
https://lists.freedesktop.org/archives/igt-dev/2024-January/067282.html
Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
Signed-off-by: Erik Kurzinger
---
drivers/gpu/drm/drm_syncobj.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index a8e6b61a188c..a1443c673f30 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1121,7 +1121,8 @@ static signed long drm_syncobj_array_wait_timeout(struct
drm_syncobj **syncobjs,
* fallthough and try a 0 timeout wait!
*/
- if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
+ if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
+DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
for (i = 0; i < count; ++i)
drm_syncobj_fence_add_wait(syncobjs[i], [i]);
}
--
2.43.0
Re: [PATCH v2] drm/syncobj: fix DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE
Is there anything else needed for this fix to be merged? I have shared an
accompanying patch for the IGT test suite here
https://lists.freedesktop.org/archives/igt-dev/2023-August/060154.html
On 8/16/23 09:26, Erik Kurzinger wrote:
> If DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT is invoked with the
> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE flag set but no fence has yet been
> submitted for the given timeline point the call will fail immediately
> with EINVAL. This does not match the intended behavior where the call
> should wait until the fence has been submitted (or the timeout expires).
>
> The following small example program illustrates the issue. It should
> wait for 5 seconds and then print ETIME, but instead it terminates right
> away after printing EINVAL.
>
> #include
> #include
> #include
> #include
> #include
> int main(void)
> {
> int fd = open("/dev/dri/card0", O_RDWR);
> uint32_t syncobj;
> drmSyncobjCreate(fd, 0, );
> struct timespec ts;
> clock_gettime(CLOCK_MONOTONIC, );
> uint64_t point = 1;
> if (drmSyncobjTimelineWait(fd, , , 1,
> ts.tv_sec * 10 + ts.tv_nsec +
> 50, // 5s
> DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE,
> NULL)) {
> printf("drmSyncobjTimelineWait failed %d\n", errno);
> }
> }
>
> Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
> Signed-off-by: Erik Kurzinger
> Reviewed by: Simon Ser
> ---
> drivers/gpu/drm/drm_syncobj.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index add45001e939..a8e6b61a188c 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1087,7 +1087,8 @@ static signed long
> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> fence = drm_syncobj_fence_get(syncobjs[i]);
> if (!fence || dma_fence_chain_find_seqno(, points[i])) {
> dma_fence_put(fence);
> - if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> + if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
> continue;
> } else {
> timeout = -EINVAL;
Re: [PATCH v2] drm/syncobj: fix DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE
That's actually a bit of a tricky question. It seems that the existing IGT syncobj_timeline test asserts the incorrect behavior that waiting for an unsubmitted fence with WAIT_AVAILABLE set should fail with EINVAL. I've sent a patch to igt-dev correcting this https://lists.freedesktop.org/archives/igt-dev/2023-August/059858.html however that will cause the test to fail with current kernels. I don't know how big of a problem that would be. Personally I kinda feel like the test *should* fail with current kernels, since current kernels do indeed have a bug. On 8/16/23 09:40, Simon Ser wrote: > BTW, question: do you know if we could add an IGT test to make sure we > don't regress this?
[PATCH v2] drm/syncobj: fix DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE
If DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT is invoked with the
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE flag set but no fence has yet been
submitted for the given timeline point the call will fail immediately
with EINVAL. This does not match the intended behavior where the call
should wait until the fence has been submitted (or the timeout expires).
The following small example program illustrates the issue. It should
wait for 5 seconds and then print ETIME, but instead it terminates right
away after printing EINVAL.
#include
#include
#include
#include
#include
int main(void)
{
int fd = open("/dev/dri/card0", O_RDWR);
uint32_t syncobj;
drmSyncobjCreate(fd, 0, );
struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, );
uint64_t point = 1;
if (drmSyncobjTimelineWait(fd, , , 1,
ts.tv_sec * 10 + ts.tv_nsec +
50, // 5s
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE, NULL)) {
printf("drmSyncobjTimelineWait failed %d\n", errno);
}
}
Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
Signed-off-by: Erik Kurzinger
Reviewed by: Simon Ser
---
drivers/gpu/drm/drm_syncobj.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index add45001e939..a8e6b61a188c 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1087,7 +1087,8 @@ static signed long drm_syncobj_array_wait_timeout(struct
drm_syncobj **syncobjs,
fence = drm_syncobj_fence_get(syncobjs[i]);
if (!fence || dma_fence_chain_find_seqno(, points[i])) {
dma_fence_put(fence);
- if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
+ if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
+DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
continue;
} else {
timeout = -EINVAL;
--
2.41.0
[PATCH] drm/syncobj: fix DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE
If DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT is invoked with the
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE flag set but no fence has yet been
submitted for the given timeline point the call will fail immediately
with EINVAL. This does not match the intended behavior where the call
should wait until the fence has been submitted (or the timeout expires).
The following small example program illustrates the issue. It should
wait for 5 seconds and then print ETIME, but instead it terminates right
away after printing EINVAL.
#include
#include
#include
#include
#include
int main(void)
{
int fd = open("/dev/dri/card0", O_RDWR);
uint32_t syncobj;
drmSyncobjCreate(fd, 0, );
struct timespec ts;
clock_gettime(CLOCK_MONOTONIC, );
uint64_t point = 1;
if (drmSyncobjTimelineWait(fd, , , 1,
ts.tv_sec * 10 + ts.tv_nsec +
50, // 5s
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE, NULL)) {
printf("drmSyncobjTimelineWait failed %d\n", errno);
}
}
Signed-off-by: Erik Kurzinger
---
drivers/gpu/drm/drm_syncobj.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index add45001e939..a8e6b61a188c 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1087,7 +1087,8 @@ static signed long drm_syncobj_array_wait_timeout(struct
drm_syncobj **syncobjs,
fence = drm_syncobj_fence_get(syncobjs[i]);
if (!fence || dma_fence_chain_find_seqno(, points[i])) {
dma_fence_put(fence);
- if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
+ if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
+DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
continue;
} else {
timeout = -EINVAL;
--
2.41.0
Re: [PATCH v3] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE
The Xwayland merge request linked below has been updated to the new interface
(via pending libdrm wrappers).
I have also posted a revised IGT test here
https://lists.freedesktop.org/archives/igt-dev/2023-July/058449.html
On 7/21/23 11:50, Erik Kurzinger wrote:
> These new ioctls perform a task similar to
> DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the
> IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the
> timeline point to import or export the fence to or from on a timeline
> syncobj.
>
> This eliminates the need to use a temporary binary syncobj along with
> DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the
> technique userspace has had to employ up to this point. While that does
> work, it is rather awkward from the programmer's perspective. Since DRM
> syncobjs have been proposed as the basis for display server explicit
> synchronization protocols, e.g. [1] and [2], providing a more
> streamlined interface now seems worthwhile.
>
> [1]
> https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90
> [2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967
>
> Accompanying userspace patches...
> IGT:
> https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/8cbee0c1782d6232de129e78cece3b94113992a5
> libdrm:
> https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/8e1ac8d831e2f7b202314c849a61a8e623657c0b
>
> V1 -> V2:
> fixed conflict with DRM_IOCTL_MODE_GETFB2
> re-ordered arguments of drm_syncobj_import_sync_file_fence
>
> V2 -> V3:
> add missing comma (whoops)
>
> Signed-off-by: Erik Kurzinger
> Reviewed-by: Simon Ser
> ---
> drivers/gpu/drm/drm_internal.h | 4 +++
> drivers/gpu/drm/drm_ioctl.c| 4 +++
> drivers/gpu/drm/drm_syncobj.c | 62 ++
> include/uapi/drm/drm.h | 8 +
> 4 files changed, 71 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index ba12acd55139..903731937595 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -255,6 +255,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device
> *dev, void *data,
> struct drm_file *file_private);
> int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_private);
> +int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
> +struct drm_file *file_private);
> +int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
> +struct drm_file *file_private);
>
> /* drm_framebuffer.c */
> void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index f03ffbacfe9b..92d6da811afd 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -711,6 +711,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
> DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE,
> drm_syncobj_import_sync_file_ioctl,
> + DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE,
> drm_syncobj_export_sync_file_ioctl,
> + DRM_RENDER_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl,
> 0),
> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE,
> drm_crtc_queue_sequence_ioctl, 0),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl,
> DRM_MASTER),
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index be3e8787d207..ee87707e7587 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -185,6 +185,13 @@
> * Note that if you want to transfer a struct _fence_chain from a given
> * point on a timeline syncobj from/into a binary syncobj, you can use the
> * point 0 to mean take/replace the fence in the syncobj.
> + *
> + * _IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and
> _IOCTL_SYNCOBJ_EXPORT_SYNC_FILE
> + * let the client import or export the struct _fence_chain of a syncobj
> + * at a particular timeline point from or to a sync file.
> + * These behave similarly to _SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE
> + * and _SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE described above,
> except
> + * that they accommodate timeline syncobjs in addition to binary syncobjs.
> */
>
> #include
> @@ -736
[PATCH v3] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE
These new ioctls perform a task similar to
DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the
IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the
timeline point to import or export the fence to or from on a timeline
syncobj.
This eliminates the need to use a temporary binary syncobj along with
DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the
technique userspace has had to employ up to this point. While that does
work, it is rather awkward from the programmer's perspective. Since DRM
syncobjs have been proposed as the basis for display server explicit
synchronization protocols, e.g. [1] and [2], providing a more
streamlined interface now seems worthwhile.
[1] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90
[2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967
Accompanying userspace patches...
IGT:
https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/e6f5c81bf893010411f1b471d68bb493ed36af67
libdrm:
https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/22180768f85f1cce36ff34bbef34956b8803d6aa
V1 -> V2:
fixed conflict with DRM_IOCTL_MODE_GETFB2
re-ordered arguments of drm_syncobj_import_sync_file_fence
V2 -> V3:
add missing comma (whoops)
Signed-off-by: Erik Kurzinger
Reviewed-by: Simon Ser
---
drivers/gpu/drm/drm_internal.h | 4 +++
drivers/gpu/drm/drm_ioctl.c| 4 +++
drivers/gpu/drm/drm_syncobj.c | 62 ++
include/uapi/drm/drm.h | 8 +
4 files changed, 71 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index ba12acd55139..903731937595 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -255,6 +255,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device
*dev, void *data,
struct drm_file *file_private);
int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private);
+int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
+int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
/* drm_framebuffer.c */
void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f03ffbacfe9b..92d6da811afd 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -711,6 +711,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE,
drm_syncobj_import_sync_file_ioctl,
+ DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE,
drm_syncobj_export_sync_file_ioctl,
+ DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl,
0),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE,
drm_crtc_queue_sequence_ioctl, 0),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl,
DRM_MASTER),
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index be3e8787d207..ee87707e7587 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -185,6 +185,13 @@
* Note that if you want to transfer a struct _fence_chain from a given
* point on a timeline syncobj from/into a binary syncobj, you can use the
* point 0 to mean take/replace the fence in the syncobj.
+ *
+ * _IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and _IOCTL_SYNCOBJ_EXPORT_SYNC_FILE
+ * let the client import or export the struct _fence_chain of a syncobj
+ * at a particular timeline point from or to a sync file.
+ * These behave similarly to _SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE
+ * and _SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE described above, except
+ * that they accommodate timeline syncobjs in addition to binary syncobjs.
*/
#include
@@ -736,10 +743,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file
*file_private,
}
static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
- int fd, int handle)
+ int fd, int handle, u64 point)
{
struct dma_fence *fence = sync_file_get_fence(fd);
struct drm_syncobj *syncobj;
+ int ret = 0;
if (!fence)
return -EINVAL;
@@ -750,14 +758,23 @@ static int drm_syncobj_import_sync_file_fence(struct
drm_file *file_private,
return -ENOENT;
}
- drm_syncobj_replace_fence(syncobj, fence);
+ if (poin
Re: [PATCH] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE
That's a fair point. With my IGT patch I don't think we would have coverage of the old path any more. I'll try to fix that somehow, and I think your suggestion of including some "invalid" cases is also a good one. Anyway, apart from that I've posted a v2 of the kernel patch addressing your feedback from earlier. I've also rebased it on top of drm-misc-next. On 7/20/23 23:59, Simon Ser wrote: > I had a look at the IGT and I'm not sure about the approach. It seems > like the patch replaces occurrences of the old FLAGS_IMPORT_SYNC_FILE > and FLAGS_EXPORT_SYNC_FILE plus TRANSFER with the new IOCTLs. However > we still want to test the functionality of that old codepath: we need > to continue to test that the old IOCTLs work as expected. > > Are the old IOCTLs still sufficiently tested elsewhere? If not, we need > to either duplicate the tests, either add a flag to the test function > to select between old and new. > > Also, it would be good to have some basic tests for invalid cases, e.g. > for the invalid zero syncobj handle, for timeline points which haven't > materialized yet, etc. > > I wonder if we need to detect at runtime whether the IOCTL is available. > I'm not sure what the IGT requirements are, is it supposed to run on > any Linux version, or does it require drm-next? > > It would help to post the IGT patches on the mailing list so that we > can do a proper review there.
[PATCH v2] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE
These new ioctls perform a task similar to
DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the
IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the
timeline point to import or export the fence to or from on a timeline
syncobj.
This eliminates the need to use a temporary binary syncobj along with
DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the
technique userspace has had to employ up to this point. While that does
work, it is rather awkward from the programmer's perspective. Since DRM
syncobjs have been proposed as the basis for display server explicit
synchronization protocols, e.g. [1] and [2], providing a more
streamlined interface now seems worthwhile.
[1] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90
[2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967
Accompanying userspace patches...
IGT:
https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/e6f5c81bf893010411f1b471d68bb493ed36af67
libdrm:
https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/22180768f85f1cce36ff34bbef34956b8803d6aa
V1 -> V2:
fixed conflict with DRM_IOCTL_MODE_GETFB2
re-ordered arguments of drm_syncobj_import_sync_file_fence
Signed-off-by: Erik Kurzinger
---
drivers/gpu/drm/drm_internal.h | 4 +++
drivers/gpu/drm/drm_ioctl.c| 4 +++
drivers/gpu/drm/drm_syncobj.c | 62 ++
include/uapi/drm/drm.h | 8 +
4 files changed, 71 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index ba12acd55139..903731937595 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -255,6 +255,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device
*dev, void *data,
struct drm_file *file_private);
int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private);
+int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
+int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
/* drm_framebuffer.c */
void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f03ffbacfe9b..92d6da811afd 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -711,6 +711,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE,
drm_syncobj_import_sync_file_ioctl,
+ DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE,
drm_syncobj_export_sync_file_ioctl,
+ DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl,
0),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE,
drm_crtc_queue_sequence_ioctl, 0),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl,
DRM_MASTER),
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index be3e8787d207..ca77a265a1ff 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -185,6 +185,13 @@
* Note that if you want to transfer a struct _fence_chain from a given
* point on a timeline syncobj from/into a binary syncobj, you can use the
* point 0 to mean take/replace the fence in the syncobj.
+ *
+ * _IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and _IOCTL_SYNCOBJ_EXPORT_SYNC_FILE
+ * let the client import or export the struct _fence_chain of a syncobj
+ * at a particular timeline point from or to a sync file.
+ * These behave similarly to _SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE
+ * and _SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE described above, except
+ * that they accommodate timeline syncobjs in addition to binary syncobjs.
*/
#include
@@ -736,10 +743,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file
*file_private,
}
static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
- int fd, int handle)
+ int fd, int handle, u64 point)
{
struct dma_fence *fence = sync_file_get_fence(fd);
struct drm_syncobj *syncobj;
+ int ret = 0;
if (!fence)
return -EINVAL;
@@ -750,14 +758,23 @@ static int drm_syncobj_import_sync_file_fence(struct
drm_file *file_private,
return -ENOENT;
}
- drm_syncobj_replace_fence(syncobj, fence);
+ if (point == 0) {
+ drm_syncobj_replace_fence(syncobj, fence);
+ } e
Re: [PATCH] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE
On 7/20/23 02:43, Simon Ser wrote:
> On Wednesday, July 19th, 2023 at 19:05, Erik Kurzinger
> wrote:
>
>> These new ioctls perform a task similar to
>> DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the
>> IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the
>> timeline point to import or export the fence to or from on a timeline
>> syncobj.
>>
>> This eliminates the need to use a temporary binary syncobj along with
>> DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the
>> technique userspace has had to employ up to this point. While that does
>> work, it is rather awkward from the programmer's perspective. Since DRM
>> syncobjs have been proposed as the basis for display server explicit
>> synchronization protocols, e.g. [1] and [2], providing a more
>> streamlined interface now seems worthwhile.
>
> This looks like a good idea to me! The patch looks good as well, apart
> from one tricky issue, see below...
>
>> [1]
>> https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90
>> [2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967
>>
>> Accompanying userspace patches...
>> IGT:
>> https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/241e7f379aeaa9b22a32277e77ad4011c8717a57
>> libdrm:
>> https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/b3961a592fc6f8b05f7e3a12413fb58eca2dbfa2
>
> (Unfortunately this isn't enough when it comes to user-space patches: the
> kernel rules require a "real" user of the new IOCTL, not just a libdr IOCTL
> wrapper. I will post a patch to make use of this from wlroots if that helps.)
>
Thanks for taking a look, Simon. If that's the case I could also update my
Xwayland MR to use the new functions.
>> Signed-off-by: Erik Kurzinger
>> ---
>> drivers/gpu/drm/drm_internal.h | 4 +++
>> drivers/gpu/drm/drm_ioctl.c| 4 +++
>> drivers/gpu/drm/drm_syncobj.c | 60 ++
>> include/uapi/drm/drm.h | 9 +
>> 4 files changed, 71 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
>> index d7e023bbb0d5..64a28ed26a16 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -253,6 +253,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device
>> *dev, void *data,
>>struct drm_file *file_private);
>> int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>> struct drm_file *file_private);
>> +int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
>> + struct drm_file *file_private);
>> +int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
>> + struct drm_file *file_private);
>>
>> /* drm_framebuffer.c */
>> void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 7c9d66ee917d..0344e8e447bc 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -710,6 +710,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>>DRM_RENDER_ALLOW),
>> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
>>DRM_RENDER_ALLOW),
>> +DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE,
>> drm_syncobj_import_sync_file_ioctl,
>> + DRM_RENDER_ALLOW),
>> +DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE,
>> drm_syncobj_export_sync_file_ioctl,
>> + DRM_RENDER_ALLOW),
>> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl,
>> 0),
>> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE,
>> drm_crtc_queue_sequence_ioctl, 0),
>> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl,
>> DRM_MASTER),
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 0c2be8360525..bf0c1eae353a 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -181,6 +181,13 @@
>> * Note that if you want to transfer a struct _fence_chain from a given
>> * point on a timeline syncobj from/into a binary syncobj, you can use the
>> * point 0 to mean take/replace the fence in the syncobj.
>> + *
>> + * _IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and
>> _IOCTL_SYNCOBJ_EXPORT_SYNC_FILE
>> + * le
[PATCH] drm/syncobj: add DRM_IOCTL_SYNCOBJ_IMPORT/EXPORT_SYNC_FILE
These new ioctls perform a task similar to
DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD/FD_TO_HANDLE with the
IMPORT/EXPORT_SYNC_FILE flag set, except that they allow specifying the
timeline point to import or export the fence to or from on a timeline
syncobj.
This eliminates the need to use a temporary binary syncobj along with
DRM_IOCTL_SYNCOBJ_TRANSFER to achieve such a thing, which is the
technique userspace has had to employ up to this point. While that does
work, it is rather awkward from the programmer's perspective. Since DRM
syncobjs have been proposed as the basis for display server explicit
synchronization protocols, e.g. [1] and [2], providing a more
streamlined interface now seems worthwhile.
[1] https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/90
[2] https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/967
Accompanying userspace patches...
IGT:
https://gitlab.freedesktop.org/ekurzinger/igt-gpu-tools/-/commit/241e7f379aeaa9b22a32277e77ad4011c8717a57
libdrm:
https://gitlab.freedesktop.org/ekurzinger/drm/-/commit/b3961a592fc6f8b05f7e3a12413fb58eca2dbfa2
Signed-off-by: Erik Kurzinger
---
drivers/gpu/drm/drm_internal.h | 4 +++
drivers/gpu/drm/drm_ioctl.c| 4 +++
drivers/gpu/drm/drm_syncobj.c | 60 ++
include/uapi/drm/drm.h | 9 +
4 files changed, 71 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index d7e023bbb0d5..64a28ed26a16 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -253,6 +253,10 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device
*dev, void *data,
struct drm_file *file_private);
int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private);
+int drm_syncobj_import_sync_file_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
+int drm_syncobj_export_sync_file_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
/* drm_framebuffer.c */
void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 7c9d66ee917d..0344e8e447bc 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -710,6 +710,10 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_IMPORT_SYNC_FILE,
drm_syncobj_import_sync_file_ioctl,
+ DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_EXPORT_SYNC_FILE,
drm_syncobj_export_sync_file_ioctl,
+ DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl,
0),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE,
drm_crtc_queue_sequence_ioctl, 0),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl,
DRM_MASTER),
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 0c2be8360525..bf0c1eae353a 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -181,6 +181,13 @@
* Note that if you want to transfer a struct _fence_chain from a given
* point on a timeline syncobj from/into a binary syncobj, you can use the
* point 0 to mean take/replace the fence in the syncobj.
+ *
+ * _IOCTL_SYNCOBJ_IMPORT_SYNC_FILE and _IOCTL_SYNCOBJ_EXPORT_SYNC_FILE
+ * let the client import or export the struct _fence_chain of a syncobj
+ * at a particular timeline point from or to a sync file.
+ * These behave similarly to _SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE
+ * and _SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE described above, except
+ * that they accommodate timeline syncobjs in addition to binary syncobjs.
*/
#include
@@ -682,10 +689,11 @@ static int drm_syncobj_fd_to_handle(struct drm_file
*file_private,
}
static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
- int fd, int handle)
+ int fd, u64 point, int handle)
{
struct dma_fence *fence = sync_file_get_fence(fd);
struct drm_syncobj *syncobj;
+ int ret = 0;
if (!fence)
return -EINVAL;
@@ -696,14 +704,23 @@ static int drm_syncobj_import_sync_file_fence(struct
drm_file *file_private,
return -ENOENT;
}
- drm_syncobj_replace_fence(syncobj, fence);
+ if (point == 0) {
+ drm_syncobj_replace_fence(syncobj, fence);
+ } else {
+ struct dma_fence_chain *chain = dma_fence_chain_alloc();
+ if (chain
