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 <[email protected]>
> Suggested-by: Michel Dänzer <[email protected]>
> Signed-off-by: Yicong Hui <[email protected]>
> ---
> 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;
