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 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);
>