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

["Christian König" <christian.koenig@amd.com>]

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