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

[Matthew Brost <matthew.brost@intel.com>]

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
>