Re: [PATCH 06/11] drm/syncobj: add timeline payload query ioctl v4

[zhoucm1 <zhoucm1@xxxxxxx>]

On 2019年02月17日 03:22, Christian König wrote:
> Am 15.02.19 um 20:31 schrieb Lionel Landwerlin via amd-gfx:
> > On 07/12/2018 09:55, Chunming Zhou wrote:
> > > user mode can query timeline payload.
> > > v2: check return value of copy_to_user
> > > v3: handle querying entry by entry
> > > v4: rebase on new chain container, simplify interface
> > >
> > > Signed-off-by: Chunming Zhou <david1.zhou@xxxxxxx>
> > > Cc: Daniel Rakos <Daniel.Rakos@xxxxxxx>
> > > Cc: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> > > Cc: Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>
> > > Cc: Dave Airlie <airlied@xxxxxxxxxx>
> > > Cc: Christian König <christian.koenig@xxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/drm_internal.h |  2 ++
> > >   drivers/gpu/drm/drm_ioctl.c    |  2 ++
> > >   drivers/gpu/drm/drm_syncobj.c  | 43 
> > > ++++++++++++++++++++++++++++++++++
> > >   include/uapi/drm/drm.h         | 10 ++++++++
> > >   4 files changed, 57 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_internal.h 
> > > b/drivers/gpu/drm/drm_internal.h
> > > index 18b41e10195c..dab4d5936441 100644
> > > --- a/drivers/gpu/drm/drm_internal.h
> > > +++ b/drivers/gpu/drm/drm_internal.h
> > > @@ -184,6 +184,8 @@ int drm_syncobj_reset_ioctl(struct drm_device 
> > > *dev, void *data,
> > >                   struct drm_file *file_private);
> > >   int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
> > >                    struct drm_file *file_private);
> > > +int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> > > +                struct drm_file *file_private);
> > >     /* drm_framebuffer.c */
> > >   void drm_framebuffer_print_info(struct drm_printer *p, unsigned 
> > > int indent,
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index a9a17ed35cc4..7578ef6dc1d1 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -681,6 +681,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> > >                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > >       DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
> > >                 DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > > +    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
> > > +              DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > >       DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, 
> > > drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
> > >       DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, 
> > > drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
> > >       DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, 
> > > drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED),
> > > diff --git a/drivers/gpu/drm/drm_syncobj.c 
> > > b/drivers/gpu/drm/drm_syncobj.c
> > > index 348079bb0965..f97fa00ca1d0 100644
> > > --- a/drivers/gpu/drm/drm_syncobj.c
> > > +++ b/drivers/gpu/drm/drm_syncobj.c
> > > @@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device 
> > > *dev, void *data,
> > >         return ret;
> > >   }
> > > +
> > > +int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> > > +                struct drm_file *file_private)
> > > +{
> > > +    struct drm_syncobj_timeline_array *args = data;
> > > +    struct drm_syncobj **syncobjs;
> > > +    uint64_t __user *points = u64_to_user_ptr(args->points);
> > > +    uint32_t i;
> > > +    int ret;
> > > +
> > > +    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
> > > +        return -ENODEV;
> > > +
> > > +    if (args->pad != 0)
> > > +        return -EINVAL;
> > > +
> > > +    if (args->count_handles == 0)
> > > +        return -EINVAL;
> > > +
> > > +    ret = drm_syncobj_array_find(file_private,
> > > +                     u64_to_user_ptr(args->handles),
> > > +                     args->count_handles,
> > > +                     &syncobjs);
> > > +    if (ret < 0)
> > > +        return ret;
> > > +
> > > +    for (i = 0; i < args->count_handles; i++) {
> > > +        struct dma_fence_chain *chain;
> > > +        struct dma_fence *fence;
> > > +        uint64_t point;
> > > +
> > > +        fence = drm_syncobj_fence_get(syncobjs[i]);
> > > +        chain = to_dma_fence_chain(fence);
> > > +        point = chain ? fence->seqno : 0;
> >
> >
> > Sorry, I don' t want to sound annoying, but this looks like this 
> > could report values going backward.
>
> Well please be annoying as much as you can :) But yeah all that stuff 
> has been discussed before as well.
>
> > Anything add a point X to a timeline that has reached value Y with X 
> > < Y would trigger that.
>
> Yes, that can indeed happen.
trigger what? when adding x (x < y), then return 0 when query?
Why would this happen?
No, syncobj->fence should always be there and the last chain node, if it 
is ever added.

-David
> But adding a timeline point X which is before the already added point 
> Y is illegal in the first place :)
>
> So when the application does something stupid and breaks it can just 
> keep the pieces.
>
> In the kernel we still do the most defensive thing and sync to 
> everything in this case.
>
> I'm just not sure if we should print an error into syslog or just 
> continue silently.
>
> Regards,
> Christian.
>
> > Either through the submission or userspace signaling or importing 
> > another syncpoint's fence.
> >
> >
> > -Lionel
> >
> >
> > > +        ret = copy_to_user(&points[i], &point, sizeof(uint64_t));
> > > +        ret = ret ? -EFAULT : 0;
> > > +        if (ret)
> > > +            break;
> > > +    }
> > > +    drm_syncobj_array_free(syncobjs, args->count_handles);
> > > +
> > > +    return ret;
> > > +}
> > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > index 0092111d002c..b2c36f2b2599 100644
> > > --- a/include/uapi/drm/drm.h
> > > +++ b/include/uapi/drm/drm.h
> > > @@ -767,6 +767,14 @@ struct drm_syncobj_array {
> > >       __u32 pad;
> > >   };
> > >   +struct drm_syncobj_timeline_array {
> > > +    __u64 handles;
> > > +    __u64 points;
> > > +    __u32 count_handles;
> > > +    __u32 pad;
> > > +};
> > > +
> > > +
> > >   /* Query current scanout sequence number */
> > >   struct drm_crtc_get_sequence {
> > >       __u32 crtc_id;        /* requested crtc_id */
> > > @@ -924,6 +932,8 @@ extern "C" {
> > >   #define DRM_IOCTL_MODE_REVOKE_LEASE    DRM_IOWR(0xC9, struct 
> > > drm_mode_revoke_lease)
> > >     #define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT    DRM_IOWR(0xCA, struct 
> > > drm_syncobj_timeline_wait)
> > > +#define DRM_IOCTL_SYNCOBJ_QUERY        DRM_IOWR(0xCB, struct 
> > > drm_syncobj_timeline_array)
> > > +
> > >   /**
> > >    * Device specific ioctls should only be in their respective headers
> > >    * The device specific ioctl range is from 0x40 to 0x9f.
> >
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx