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>
---
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);
+
/**
* 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;
fence = drm_syncobj_fence_get(syncobjs[i]);
+
+ if (args->flags & DRM_SYNCOBJ_QUERY_FLAGS_ERROR) {
+ int64_t error = 0;
+
+ if (fence)
+ error = dma_fence_chain_find_error(fence);
+
+ ret = copy_to_user(&handles[i], &error, sizeof(int64_t));
+ ret = ret ? -EFAULT : 0;
+ if (ret) {
+ dma_fence_put(fence);
+ break;
+ }
+
+ }
+
chain = to_dma_fence_chain(fence);
if (chain) {
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 */
struct drm_syncobj_timeline_array {
__u64 handles;
__u64 points;
--
2.53.0
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@amd.com>
> Suggested-by: Michel Dänzer <michel.daenzer@mailbox.org>
> Signed-off-by: Yicong Hui <yiconghui@gmail.com>
> ---
> 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;
>> +
>> /**
>> * 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.
>
> ...
>
>> + int64_t error = 0;
>
> The error code is an int and only 32bits.
Understood, will change that!
>
>> +
>> + 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!
Ah, that's my mistake. Was thrown off by the __u64 in the struct definition but it is obvious now that it is a u32. Fixed as well!
>
>> + 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.
>
Hi! Were you thinking something that looks a little bit like this?
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);
On 2/20/26 18:05, Yicong Hui wrote:
>>> +
>>> /**
>>> * 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.
>>
>> ...
>>
>>> + int64_t error = 0;
>>
>> The error code is an int and only 32bits.
> Understood, will change that!
>
>>
>>> +
>>> + 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!
> Ah, that's my mistake. Was thrown off by the __u64 in the struct definition but it is obvious now that it is a u32. Fixed as well!
>
>>
>>> + 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.
>>
>
> Hi! Were you thinking something that looks a little bit like this?
Yeah that looks like what I had in mind to.
Thanks,
Christian.
>
> 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);
>
On 2/20/26 5:07 PM, Christian König wrote:
> On 2/20/26 18:05, Yicong Hui wrote:
>>>> +
>>>> /**
>>>> * 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.
>>>
>>> ...
>>>
>>>> + int64_t error = 0;
>>>
>>> The error code is an int and only 32bits.
>> Understood, will change that!
>>
>>>
>>>> +
>>>> + 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!
>> Ah, that's my mistake. Was thrown off by the __u64 in the struct definition but it is obvious now that it is a u32. Fixed as well!
>>
>>>
>>>> + 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.
>>>
>>
>> Hi! Were you thinking something that looks a little bit like this?
>
> Yeah that looks like what I had in mind to.
>
> Thanks,
> Christian.
>
Hi! What should I do at this point? Send a v3, or..?
Thanks!
Yicong
On 2/20/26 20:40, Yicong Hui wrote:
> On 2/20/26 5:07 PM, Christian König wrote:
>> On 2/20/26 18:05, Yicong Hui wrote:
>>>>> +
>>>>> /**
>>>>> * 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.
>>>>
>>>> ...
>>>>
>>>>> + int64_t error = 0;
>>>>
>>>> The error code is an int and only 32bits.
>>> Understood, will change that!
>>>
>>>>
>>>>> +
>>>>> + 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!
>>> Ah, that's my mistake. Was thrown off by the __u64 in the struct definition but it is obvious now that it is a u32. Fixed as well!
>>>
>>>>
>>>>> + 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.
>>>>
>>>
>>> Hi! Were you thinking something that looks a little bit like this?
>>
>> Yeah that looks like what I had in mind to.
>>
>> Thanks,
>> Christian.
>>
>
> Hi! What should I do at this point? Send a v3, or..?
You can go ahead and send a v3. But after that I think we need some userspace code using that.
Could you also look into writing IGT tests for this?
Thanks,
Christian.
>
> Thanks!
> Yicong
© 2016 - 2026 Red Hat, Inc.