Re: [RFC PATCH v3 1/3] drm/syncobj: Add flag DRM_SYNCOBJ_QUERY_FLAGS_ERROR to query errors

[Tvrtko Ursulin <tursulin@ursulin.net>]

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;