Re: [PATCH 3/5] drm: add timeline support for syncobj export/import

["Koenig, Christian" <Christian.Koenig@amd.com>]

Am 02.11.18 um 11:34 schrieb zhoucm1:
>
> On 2018年11月02日 17:54, Koenig, Christian wrote:
>> Am 02.11.18 um 10:42 schrieb zhoucm1:
>>>
>>> On 2018年11月02日 16:46, Christian König wrote:
>>>> Am 02.11.18 um 09:25 schrieb Chunming Zhou:
>>>>> user space can specify timeline point fence to export/import.
>>>>>
>>>>> 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>
>>>>  From the coding it looks good to me, but I can't judge if that really
>>>> makes sense to export/import individual points.
>>>>
>>>> I would have rather expected that always the whole timeline is
>>>> exported/imported. Otherwise the whole thing with waiting for sync
>>>> points to appear doesn't really make sense to me.
>>> We can Cpu wait on individual points, why can not we export/import 
>>> them?
>>> I confirmed with Daniel before, he said the extension need both them,
>>> export/import individual points and export/import the whole timeline
>>> semaphore.
>>> More discussion is
>>> https://gitlab.khronos.org/vulkan/vulkan/merge_requests/2696
>> Ah! It looked initially to me that this was the only way to
>> export/import timeline sync objects.
>>
>> Thinking more about it wouldn't it be better to provide a function to
>> signal a sync point from another sync point?
>>
>> That would provide the same functionality, would be cleaner to implement
>> and more flexible on top.
>>
>> E.g. if you want to export only a specific sync point you first create a
>> new syncobj and the move over this sync point into the syncobj.
> Sorry, I didn't completely get your means. Could you detail a bit?
> but I think we need to meet export/import goals:
> 1. export sepcific sync point or binary syncobj fence to sync file fd 
> or handle.

Well that is not an export nor import in the sense of the existing API 
because it creates new objects while the existing export function just 
return a different type of handle for the same object.

> 2. import fence from fd/handle to timeline sync point or banory syncobj.

My idea is to have a "transfer" function which says signal this syncobj 
from this other syncobj (both can either be timeline or binary).

This way you can handle quite a bunch of use cases with just a single 
function.

E.g. export a specific timeline point into an fd: Create a new syncobj, 
transfer your sync pointer from the existing object to that one, export 
it as fd.

Converting a timeline synobj into a binary: Create a new binary syncobj, 
transfer the last sync point from the existing to that new one.

etc etc...

Regards,
Christian.

> 3. export/import the whole timeline seamphore.
>
> I'm still not sure how you mentioned can achieve these.
>
> Thanks,
> David
>>
>> Otherwise you mix multiple operations into one IOCTL and that is usually
>> not a good idea.
>>
>> Regards,
>> Christian.
>>
>>> Thanks,
>>> David Zhou
>>>> Christian.
>>>>
>>>>> ---
>>>>>    drivers/gpu/drm/drm_internal.h |  4 ++
>>>>>    drivers/gpu/drm/drm_ioctl.c    |  4 ++
>>>>>    drivers/gpu/drm/drm_syncobj.c  | 76
>>>>> ++++++++++++++++++++++++++++++----
>>>>>    include/uapi/drm/drm.h         | 11 +++++
>>>>>    4 files changed, 88 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_internal.h
>>>>> b/drivers/gpu/drm/drm_internal.h
>>>>> index 9c4826411a3c..5ad6cbdb68ab 100644
>>>>> --- a/drivers/gpu/drm/drm_internal.h
>>>>> +++ b/drivers/gpu/drm/drm_internal.h
>>>>> @@ -181,6 +181,10 @@ int drm_syncobj_handle_to_fd_ioctl(struct
>>>>> drm_device *dev, void *data,
>>>>>                       struct drm_file *file_private);
>>>>>    int drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void
>>>>> *data,
>>>>>                       struct drm_file *file_private);
>>>>> +int drm_syncobj_handle_to_fd_ioctl2(struct drm_device *dev, void
>>>>> *data,
>>>>> +                    struct drm_file *file_private);
>>>>> +int drm_syncobj_fd_to_handle_ioctl2(struct drm_device *dev, void
>>>>> *data,
>>>>> +                    struct drm_file *file_private);
>>>>>    int drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
>>>>>                   struct drm_file *file_private);
>>>>>    int drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void
>>>>> *data,
>>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c 
>>>>> b/drivers/gpu/drm/drm_ioctl.c
>>>>> index 7578ef6dc1d1..364d26e949cf 100644
>>>>> --- a/drivers/gpu/drm/drm_ioctl.c
>>>>> +++ b/drivers/gpu/drm/drm_ioctl.c
>>>>> @@ -673,6 +673,10 @@ static const struct drm_ioctl_desc drm_ioctls[]
>>>>> = {
>>>>>                  DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>>>        DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE,
>>>>> drm_syncobj_fd_to_handle_ioctl,
>>>>>                  DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>>> +    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD2,
>>>>> drm_syncobj_handle_to_fd_ioctl2,
>>>>> +              DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>>> +    DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE2,
>>>>> drm_syncobj_fd_to_handle_ioctl2,
>>>>> +              DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>>>        DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_WAIT, drm_syncobj_wait_ioctl,
>>>>>                  DRM_UNLOCKED|DRM_RENDER_ALLOW),
>>>>>        DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT,
>>>>> drm_syncobj_timeline_wait_ioctl,
>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>>>> b/drivers/gpu/drm/drm_syncobj.c
>>>>> index 9ad58d0d21cd..dffc42ba2f91 100644
>>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>>> @@ -677,7 +677,7 @@ static int drm_syncobj_fd_to_handle(struct
>>>>> drm_file *file_private,
>>>>>    }
>>>>>      static int drm_syncobj_import_sync_file_fence(struct drm_file
>>>>> *file_private,
>>>>> -                          int fd, int handle)
>>>>> +                          int fd, int handle, uint64_t point)
>>>>>    {
>>>>>        struct dma_fence *fence = sync_file_get_fence(fd);
>>>>>        struct drm_syncobj *syncobj;
>>>>> @@ -691,14 +691,14 @@ static int
>>>>> drm_syncobj_import_sync_file_fence(struct drm_file *file_private,
>>>>>            return -ENOENT;
>>>>>        }
>>>>>    -    drm_syncobj_replace_fence(syncobj, 0, fence);
>>>>> +    drm_syncobj_replace_fence(syncobj, point, fence);
>>>>>        dma_fence_put(fence);
>>>>>        drm_syncobj_put(syncobj);
>>>>>        return 0;
>>>>>    }
>>>>>      static int drm_syncobj_export_sync_file(struct drm_file
>>>>> *file_private,
>>>>> -                    int handle, int *p_fd)
>>>>> +                    int handle, uint64_t point, int *p_fd)
>>>>>    {
>>>>>        int ret;
>>>>>        struct dma_fence *fence;
>>>>> @@ -708,7 +708,7 @@ static int drm_syncobj_export_sync_file(struct
>>>>> drm_file *file_private,
>>>>>        if (fd < 0)
>>>>>            return fd;
>>>>>    -    ret = drm_syncobj_find_fence(file_private, handle, 0, 0,
>>>>> &fence);
>>>>> +    ret = drm_syncobj_find_fence(file_private, handle, point, 0,
>>>>> &fence);
>>>>>        if (ret)
>>>>>            goto err_put_fd;
>>>>>    @@ -817,9 +817,14 @@ drm_syncobj_handle_to_fd_ioctl(struct
>>>>> drm_device *dev, void *data,
>>>>>            args->flags !=
>>>>> DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
>>>>>            return -EINVAL;
>>>>>    -    if (args->flags &
>>>>> DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
>>>>> +    if (args->flags &
>>>>> DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE) {
>>>>> +        struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
>>>>> +                                   args->handle);
>>>>> +        if (!syncobj || syncobj->type != DRM_SYNCOBJ_TYPE_BINARY)
>>>>> +            return -EINVAL;
>>>>>            return drm_syncobj_export_sync_file(file_private,
>>>>> args->handle,
>>>>> -                            &args->fd);
>>>>> +                            0, &args->fd);
>>>>> +    }
>>>>>          return drm_syncobj_handle_to_fd(file_private, args->handle,
>>>>>                        &args->fd);
>>>>> @@ -841,15 +846,72 @@ drm_syncobj_fd_to_handle_ioctl(struct
>>>>> drm_device *dev, void *data,
>>>>>            args->flags !=
>>>>> DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
>>>>>            return -EINVAL;
>>>>>    +    if (args->flags &
>>>>> DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE) {
>>>>> +        struct drm_syncobj *syncobj = drm_syncobj_find(file_private,
>>>>> +                                   args->handle);
>>>>> +        if (!syncobj || syncobj->type != DRM_SYNCOBJ_TYPE_BINARY)
>>>>> +            return -EINVAL;
>>>>> +        return drm_syncobj_import_sync_file_fence(file_private,
>>>>> +                              args->fd,
>>>>> +                              args->handle,
>>>>> +                              0);
>>>>> +    }
>>>>> +
>>>>> +    return drm_syncobj_fd_to_handle(file_private, args->fd,
>>>>> +                    &args->handle);
>>>>> +}
>>>>> +
>>>>> +int
>>>>> +drm_syncobj_handle_to_fd_ioctl2(struct drm_device *dev, void *data,
>>>>> +                   struct drm_file *file_private)
>>>>> +{
>>>>> +    struct drm_syncobj_handle2 *args = data;
>>>>> +
>>>>> +    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    if (args->pad)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    if (args->flags != 0 &&
>>>>> +        args->flags !=
>>>>> DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    if (args->flags & 
>>>>> DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE)
>>>>> +        return drm_syncobj_export_sync_file(file_private,
>>>>> args->handle,
>>>>> +                            args->point, &args->fd);
>>>>> +
>>>>> +    return drm_syncobj_handle_to_fd(file_private, args->handle,
>>>>> +                    &args->fd);
>>>>> +}
>>>>> +
>>>>> +int
>>>>> +drm_syncobj_fd_to_handle_ioctl2(struct drm_device *dev, void *data,
>>>>> +                   struct drm_file *file_private)
>>>>> +{
>>>>> +    struct drm_syncobj_handle2 *args = data;
>>>>> +
>>>>> +    if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    if (args->pad)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    if (args->flags != 0 &&
>>>>> +        args->flags !=
>>>>> DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
>>>>> +        return -EINVAL;
>>>>> +
>>>>>        if (args->flags &
>>>>> DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE)
>>>>>            return drm_syncobj_import_sync_file_fence(file_private,
>>>>>                                  args->fd,
>>>>> -                              args->handle);
>>>>> +                              args->handle,
>>>>> +                              args->point);
>>>>>          return drm_syncobj_fd_to_handle(file_private, args->fd,
>>>>>                        &args->handle);
>>>>>    }
>>>>>    +
>>>>>    struct syncobj_wait_entry {
>>>>>        struct task_struct *task;
>>>>>        struct dma_fence *fence;
>>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>>>>> index 23c4979d8a1c..da5df2a42fc3 100644
>>>>> --- a/include/uapi/drm/drm.h
>>>>> +++ b/include/uapi/drm/drm.h
>>>>> @@ -735,6 +735,15 @@ struct drm_syncobj_handle {
>>>>>        __s32 fd;
>>>>>        __u32 pad;
>>>>>    };
>>>>> +struct drm_syncobj_handle2 {
>>>>> +    __u32 handle;
>>>>> +    __u32 flags;
>>>>> +    __u64 point;
>>>>> +
>>>>> +    __s32 fd;
>>>>> +    __u32 pad;
>>>>> +};
>>>>> +
>>>>>      #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
>>>>>    #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
>>>>> @@ -938,6 +947,8 @@ extern "C" {
>>>>>      #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_query)
>>>>> +#define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD2    DRM_IOWR(0xCC, struct
>>>>> drm_syncobj_handle2)
>>>>> +#define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE2    DRM_IOWR(0xCD, struct
>>>>> drm_syncobj_handle2)
>>>>>      /**
>>>>>     * Device specific ioctls should only be in their respective 
>>>>> headers
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel