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

["Christian König via amd-gfx" <amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>]

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@amd.com>
>> Cc: Daniel Rakos <Daniel.Rakos@amd.com>
>> Cc: Jason Ekstrand <jason@jlekstrand.net>
>> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   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. 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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