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

[Yicong Hui]

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