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

[Christian König]

On 2/20/26 03:26, 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@xxxxxxx>
> Suggested-by: Michel Dänzer <michel.daenzer@xxxxxxxxxxx>
> Signed-off-by: Yicong Hui <yiconghui@xxxxxxxxx>
> ---
>  drivers/dma-buf/dma-fence-chain.c | 52 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_syncobj.c     | 21 ++++++++++++-
>  include/linux/dma-fence-chain.h   |  1 +
>  include/uapi/drm/drm.h            |  1 +
>  4 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c
> index a8a90acf4f34..076d78b73379 100644
> --- a/drivers/dma-buf/dma-fence-chain.c
> +++ b/drivers/dma-buf/dma-fence-chain.c
> @@ -76,6 +76,58 @@ struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
>  }
>  EXPORT_SYMBOL(dma_fence_chain_walk);
>  
> +/**
> + * dma_fence_chain_find_error - find the latest error
> + * @fence: current chain node
> + *
> + * Walk the chain repeatedly until reaches a fence with error, or the
> + * end of the fence chain. Does not garbage collect.
> + *
> + * Returns the first error it finds in the chain.
> + */
> +int64_t dma_fence_chain_find_error(struct dma_fence *fence)
> +{
> +	struct dma_fence_chain *chain, *prev_chain;
> +	struct dma_fence *prev;
> +	int64_t error = 0;
> +
> +	chain = to_dma_fence_chain(fence);
> +	if (!chain)
> +		return fence->error;
> +
> +	if (chain->fence->error)
> +		return chain->fence->error;
> +
> +	while ((prev = dma_fence_chain_get_prev(chain))) {
> +		prev_chain = to_dma_fence_chain(prev);
> +
> +		if (prev_chain) {
> +
> +			if (prev_chain->fence->error) {
> +				error = prev_chain->fence->error;
> +				dma_fence_put(prev);
> +				break;
> +			}
> +
> +			chain = prev_chain;
> +		} else {
> +
> +			if (prev->error)
> +				error = prev->error;
> +			dma_fence_put(prev);
> +			break;
> +		}
> +
> +
> +		dma_fence_put(prev);
> +
> +	}
> +
> +
> +	return error;
> +}
> +EXPORT_SYMBOL(dma_fence_chain_find_error);

That is superfluous, only signaled fences can have an error.

> +
>  /**
>   * dma_fence_chain_find_seqno - find fence chain node by seqno
>   * @pfence: pointer to the chain node where to start
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 2d4ab745fdad..322f64b72775 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);
> +	uint64_t __user *handles = u64_to_user_ptr(args->handles);
>  	uint32_t i;
>  	int ret;
>  
>  	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)
> @@ -1680,6 +1683,22 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>  		uint64_t point;

Make a local variable "int error" here.

>  
>  		fence = drm_syncobj_fence_get(syncobjs[i]);
> +
> +		if (args->flags & DRM_SYNCOBJ_QUERY_FLAGS_ERROR) {

That should probably be just another functionality before the existing copy, but see below after the "if (chain)".

> +			int64_t error = 0;

The error code is an int and only 32bits.

> +
> +			if (fence)
> +				error = dma_fence_chain_find_error(fence);
> +
> +			ret = copy_to_user(&handles[i], &error, sizeof(int64_t));

The handles are also only 32bits!

> +			ret = ret ? -EFAULT : 0;
> +			if (ret) {
> +				dma_fence_put(fence);
> +				break;
> +			}
> +
> +		}
> +
>  		chain = to_dma_fence_chain(fence);
>  		if (chain) {

In this code path whenever point is assigned something do a "error = dma_fence_get_status(fence);" and then eventually copy the error to userspace after copying the point.

>  			struct dma_fence *iter, *last_signaled =
> diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h
> index 68c3c1e41014..b4ada124d7b6 100644
> --- a/include/linux/dma-fence-chain.h
> +++ b/include/linux/dma-fence-chain.h
> @@ -122,6 +122,7 @@ static inline void dma_fence_chain_free(struct dma_fence_chain *chain)
>  	     iter = dma_fence_chain_walk(iter))
>  
>  struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence);
> +int64_t dma_fence_chain_find_error(struct dma_fence *fence);
>  int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t seqno);
>  void dma_fence_chain_init(struct dma_fence_chain *chain,
>  			  struct dma_fence *prev,
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 27cc159c1d27..2640cc0a09fe 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -1044,6 +1044,7 @@ struct drm_syncobj_array {
>  };
>  
>  #define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last available point on timeline syncobj */
> +#define DRM_SYNCOBJ_QUERY_FLAGS_ERROR (1 << 1) /* fill out error codes if they are found */

First of all comments please never after a define! The existing code has that already messed up.

Then the new define needs more documentation. Something like:

/*
 * Copy the status of the fence as output into the handles array.
 * The handles array is overwritten by that.
 */

Regards,
Christian.

>  struct drm_syncobj_timeline_array {
>  	__u64 handles;
>  	__u64 points;