Add flag DRM_SYNCOBJ_QUERY_FLAGS_ERROR to make the
DRM_IOCTL_SYNCOBJ_QUERY ioctl fill out the handles array with the
error code of the first fence found per syncobj and 0 if one is not
found and maintain the normal return value in points.
Suggested-by: Christian König <christian.koenig@amd.com>
Suggested-by: Michel Dänzer <michel.daenzer@mailbox.org>
Signed-off-by: Yicong Hui <yiconghui@gmail.com>
---
Changes in v3:
* Fixed inline comments by converting to multi-line comments in
accordance to kernel style guidelines.
* No longer using a separate superfluous function to walk the fence
chain, and instead queries the last signaled fence in in the chain for
its error code
* Fixed types for error and handles array.
drivers/gpu/drm/drm_syncobj.c | 22 ++++++++++++++++++++--
include/uapi/drm/drm.h | 5 +++++
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 2d4ab745fdad..b74e491f9d8b 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1654,14 +1654,17 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
{
struct drm_syncobj_timeline_array *args = data;
struct drm_syncobj **syncobjs;
+ unsigned int valid_flags = DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED |
+ DRM_SYNCOBJ_QUERY_FLAGS_ERROR;
uint64_t __user *points = u64_to_user_ptr(args->points);
+ uint32_t __user *handles = u64_to_user_ptr(args->handles);
uint32_t i;
- int ret;
+ int ret, error;
if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
return -EOPNOTSUPP;
- if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED)
+ if (args->flags & ~valid_flags)
return -EINVAL;
if (args->count_handles == 0)
@@ -1681,6 +1684,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
fence = drm_syncobj_fence_get(syncobjs[i]);
chain = to_dma_fence_chain(fence);
+
if (chain) {
struct dma_fence *iter, *last_signaled =
dma_fence_get(fence);
@@ -1688,6 +1692,8 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
if (args->flags &
DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) {
point = fence->seqno;
+ error = dma_fence_get_status(fence);
+
} else {
dma_fence_chain_for_each(iter, fence) {
if (iter->context != fence->context) {
@@ -1702,16 +1708,28 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
point = dma_fence_is_signaled(last_signaled) ?
last_signaled->seqno :
to_dma_fence_chain(last_signaled)->prev_seqno;
+
+ error = dma_fence_get_status(last_signaled);
}
dma_fence_put(last_signaled);
} else {
point = 0;
+ error = fence ? dma_fence_get_status(fence) : 0;
}
dma_fence_put(fence);
+
ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
ret = ret ? -EFAULT : 0;
if (ret)
break;
+
+ if (args->flags & DRM_SYNCOBJ_QUERY_FLAGS_ERROR) {
+ ret = copy_to_user(&handles[i], &error, sizeof(*handles));
+
+ ret = ret ? -EFAULT : 0;
+ if (ret)
+ break;
+ }
}
drm_syncobj_array_free(syncobjs, args->count_handles);
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 27cc159c1d27..213b4dc9b612 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -1044,6 +1044,11 @@ struct drm_syncobj_array {
};
#define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last available point on timeline syncobj */
+/*
+ * Copy the status of the fence as output into the handles array.
+ * The handles array is overwritten by that.
+ */
+#define DRM_SYNCOBJ_QUERY_FLAGS_ERROR (1 << 1)
struct drm_syncobj_timeline_array {
__u64 handles;
__u64 points;
--
2.53.0
On Wed, Feb 25, 2026 at 12:46:07PM +0000, Yicong Hui wrote:
> Add flag DRM_SYNCOBJ_QUERY_FLAGS_ERROR to make the
> DRM_IOCTL_SYNCOBJ_QUERY ioctl fill out the handles array with the
> error code of the first fence found per syncobj and 0 if one is not
> found and maintain the normal return value in points.
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Suggested-by: Michel Dänzer <michel.daenzer@mailbox.org>
> Signed-off-by: Yicong Hui <yiconghui@gmail.com>
> ---
> Changes in v3:
> * Fixed inline comments by converting to multi-line comments in
> accordance to kernel style guidelines.
> * No longer using a separate superfluous function to walk the fence
> chain, and instead queries the last signaled fence in in the chain for
> its error code
> * Fixed types for error and handles array.
>
>
> drivers/gpu/drm/drm_syncobj.c | 22 ++++++++++++++++++++--
> include/uapi/drm/drm.h | 5 +++++
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 2d4ab745fdad..b74e491f9d8b 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1654,14 +1654,17 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> {
> struct drm_syncobj_timeline_array *args = data;
> struct drm_syncobj **syncobjs;
> + unsigned int valid_flags = DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED |
> + DRM_SYNCOBJ_QUERY_FLAGS_ERROR;
> uint64_t __user *points = u64_to_user_ptr(args->points);
> + uint32_t __user *handles = u64_to_user_ptr(args->handles);
> uint32_t i;
> - int ret;
> + int ret, error;
>
> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> return -EOPNOTSUPP;
>
> - if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED)
> + if (args->flags & ~valid_flags)
> return -EINVAL;
>
> if (args->count_handles == 0)
> @@ -1681,6 +1684,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>
> fence = drm_syncobj_fence_get(syncobjs[i]);
> chain = to_dma_fence_chain(fence);
> +
> if (chain) {
> struct dma_fence *iter, *last_signaled =
> dma_fence_get(fence);
> @@ -1688,6 +1692,8 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> if (args->flags &
> DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) {
> point = fence->seqno;
> + error = dma_fence_get_status(fence);
> +
> } else {
> dma_fence_chain_for_each(iter, fence) {
> if (iter->context != fence->context) {
> @@ -1702,16 +1708,28 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> point = dma_fence_is_signaled(last_signaled) ?
> last_signaled->seqno :
> to_dma_fence_chain(last_signaled)->prev_seqno;
> +
> + error = dma_fence_get_status(last_signaled);
> }
> dma_fence_put(last_signaled);
> } else {
> point = 0;
> + error = fence ? dma_fence_get_status(fence) : 0;
dma_fence_get_status returns 0 (unsignaled), 1 (signaled with no error),
or fence->error (signaled with error != 0).
Is it intentional to return 1 to user space for a signaled fence? What
if a driver sets fence->error to 1?
Side note: the fence error kernel doc says fence->error is only valid if
< 0, but dma_fence_get_status doesn’t enforce that.
Also, returning fence->error directly to user space seems like a massive
problem. Right now, drivers can set fence->error to whatever they want,
but now this gets reported to user space and suddenly has meaning. Does
user space take certain actions based on the specific error code (e.g.,
-ECANCELED, -ETIME, etc.)? It certainly can’t, because we have no
internal kernel standards for what fence->error actually means. Two
different drivers could assign the same error code but mean entirely
different things—or the opposite could be true.
Thus, without some standardization plus fixing every single driver, I
really think the best we can report in a generic mechanism like a
syncobj is simply “error” or “no error."
Matt
> }
> dma_fence_put(fence);
> +
> ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
> ret = ret ? -EFAULT : 0;
> if (ret)
> break;
> +
> + if (args->flags & DRM_SYNCOBJ_QUERY_FLAGS_ERROR) {
> + ret = copy_to_user(&handles[i], &error, sizeof(*handles));
> +
> + ret = ret ? -EFAULT : 0;
> + if (ret)
> + break;
> + }
> }
> drm_syncobj_array_free(syncobjs, args->count_handles);
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 27cc159c1d27..213b4dc9b612 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1044,6 +1044,11 @@ struct drm_syncobj_array {
> };
>
> #define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last available point on timeline syncobj */
> +/*
> + * Copy the status of the fence as output into the handles array.
> + * The handles array is overwritten by that.
> + */
> +#define DRM_SYNCOBJ_QUERY_FLAGS_ERROR (1 << 1)
> struct drm_syncobj_timeline_array {
> __u64 handles;
> __u64 points;
> --
> 2.53.0
>
On 2/27/26 01:23, Matthew Brost wrote:
...
>> @@ -1702,16 +1708,28 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>> point = dma_fence_is_signaled(last_signaled) ?
>> last_signaled->seqno :
>> to_dma_fence_chain(last_signaled)->prev_seqno;
>> +
>> + error = dma_fence_get_status(last_signaled);
>> }
>> dma_fence_put(last_signaled);
>> } else {
>> point = 0;
>> + error = fence ? dma_fence_get_status(fence) : 0;
>
> dma_fence_get_status returns 0 (unsignaled), 1 (signaled with no error),
> or fence->error (signaled with error != 0).
>
> Is it intentional to return 1 to user space for a signaled fence? What
> if a driver sets fence->error to 1?
>
> Side note: the fence error kernel doc says fence->error is only valid if
> < 0, but dma_fence_get_status doesn’t enforce that.
dma_fence_get_status() enforces this with a WARN_ON().
> Also, returning fence->error directly to user space seems like a massive
> problem. Right now, drivers can set fence->error to whatever they want,
> but now this gets reported to user space and suddenly has meaning. Does
> user space take certain actions based on the specific error code (e.g.,
> -ECANCELED, -ETIME, etc.)? It certainly can’t, because we have no
> internal kernel standards for what fence->error actually means. Two
> different drivers could assign the same error code but mean entirely
> different things—or the opposite could be true.
That is not even remotely true. fence->error is already used in the UAPI for syncfiles for like 10years or so. Android is massively relying on that.
There is also documentation on what values drivers should use: https://elixir.bootlin.com/linux/v6.19.3/source/include/linux/dma-fence.h#L565
The error reporting was just missing from drm_syncobj and only implemented for syncfiles and that's what this patch set here is fixing.
Regards,
Christian.
>
> Thus, without some standardization plus fixing every single driver, I
> really think the best we can report in a generic mechanism like a
> syncobj is simply “error” or “no error."
>
> Matt
>
>> }
>> dma_fence_put(fence);
>> +
>> ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
>> ret = ret ? -EFAULT : 0;
>> if (ret)
>> break;
>> +
>> + if (args->flags & DRM_SYNCOBJ_QUERY_FLAGS_ERROR) {
>> + ret = copy_to_user(&handles[i], &error, sizeof(*handles));
>> +
>> + ret = ret ? -EFAULT : 0;
>> + if (ret)
>> + break;
>> + }
>> }
>> drm_syncobj_array_free(syncobjs, args->count_handles);
>>
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 27cc159c1d27..213b4dc9b612 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -1044,6 +1044,11 @@ struct drm_syncobj_array {
>> };
>>
>> #define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last available point on timeline syncobj */
>> +/*
>> + * Copy the status of the fence as output into the handles array.
>> + * The handles array is overwritten by that.
>> + */
>> +#define DRM_SYNCOBJ_QUERY_FLAGS_ERROR (1 << 1)
>> struct drm_syncobj_timeline_array {
>> __u64 handles;
>> __u64 points;
>> --
>> 2.53.0
>>
On 25/02/2026 12:46, Yicong Hui wrote:
> Add flag DRM_SYNCOBJ_QUERY_FLAGS_ERROR to make the
> DRM_IOCTL_SYNCOBJ_QUERY ioctl fill out the handles array with the
> error code of the first fence found per syncobj and 0 if one is not
> found and maintain the normal return value in points.
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Suggested-by: Michel Dänzer <michel.daenzer@mailbox.org>
> Signed-off-by: Yicong Hui <yiconghui@gmail.com>
> ---
> Changes in v3:
> * Fixed inline comments by converting to multi-line comments in
> accordance to kernel style guidelines.
> * No longer using a separate superfluous function to walk the fence
> chain, and instead queries the last signaled fence in in the chain for
> its error code
> * Fixed types for error and handles array.
>
>
> drivers/gpu/drm/drm_syncobj.c | 22 ++++++++++++++++++++--
> include/uapi/drm/drm.h | 5 +++++
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 2d4ab745fdad..b74e491f9d8b 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1654,14 +1654,17 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> {
> struct drm_syncobj_timeline_array *args = data;
> struct drm_syncobj **syncobjs;
> + unsigned int valid_flags = DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED |
> + DRM_SYNCOBJ_QUERY_FLAGS_ERROR;
> uint64_t __user *points = u64_to_user_ptr(args->points);
> + uint32_t __user *handles = u64_to_user_ptr(args->handles);
> uint32_t i;
> - int ret;
> + int ret, error;
>
> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> return -EOPNOTSUPP;
>
> - if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED)
> + if (args->flags & ~valid_flags)
> return -EINVAL;
>
> if (args->count_handles == 0)
> @@ -1681,6 +1684,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>
> fence = drm_syncobj_fence_get(syncobjs[i]);
> chain = to_dma_fence_chain(fence);
> +
Random whitespace changes should be avoided.
> if (chain) {
> struct dma_fence *iter, *last_signaled =
> dma_fence_get(fence);
> @@ -1688,6 +1692,8 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> if (args->flags &
> DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) {
> point = fence->seqno;
> + error = dma_fence_get_status(fence);
> +
Ditto.
> } else {
> dma_fence_chain_for_each(iter, fence) {
> if (iter->context != fence->context) {
> @@ -1702,16 +1708,28 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> point = dma_fence_is_signaled(last_signaled) ?
> last_signaled->seqno :
> to_dma_fence_chain(last_signaled)->prev_seqno;
> +
> + error = dma_fence_get_status(last_signaled);
> }
> dma_fence_put(last_signaled);
> } else {
> point = 0;
> + error = fence ? dma_fence_get_status(fence) : 0;
> }
> dma_fence_put(fence);
> +
More of the same. Although in this case I think it is an improvement so
you may keep it.
> ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
> ret = ret ? -EFAULT : 0;
> if (ret)
> break;
> +
> + if (args->flags & DRM_SYNCOBJ_QUERY_FLAGS_ERROR) {
> + ret = copy_to_user(&handles[i], &error, sizeof(*handles));
> +
This blank line is not inserted between the existing code but still
please remove it - it is not separating any logical blocks so it is not
improving readability.
Apart from nitpicks, the implementation looks correct to me. But
userspace folks need to bless it and use it, as other people have
already commented.
And uapi is fine since fence status is already UABI courtesy of
sync_file. So it is not promoting anything kernel internal to UABI.
> + ret = ret ? -EFAULT : 0;
> + if (ret)
> + break;
> + }
> }
> drm_syncobj_array_free(syncobjs, args->count_handles);
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 27cc159c1d27..213b4dc9b612 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1044,6 +1044,11 @@ struct drm_syncobj_array {
> };
>
> #define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last available point on timeline syncobj */
> +/*
> + * Copy the status of the fence as output into the handles array.
> + * The handles array is overwritten by that.
The documentation could be improved though. Make it clear that one
status per handle is returned (use more plural) and we need an
explanation of what is the status, or a link to something existing.
For example sync_file uapi header documents it like this:
* @status: status of the fence 0:active 1:signaled <0:error
See if you can come up with something clear and to the point for this
comment block?
Regards,
Tvrtko
> + */
> +#define DRM_SYNCOBJ_QUERY_FLAGS_ERROR (1 << 1)
> struct drm_syncobj_timeline_array {
> __u64 handles;
> __u64 points;
© 2016 - 2026 Red Hat, Inc.