[PATCH 3/5] drm: add timeline support for syncobj export/import
Koenig, Christian
Christian.Koenig at amd.com
Mon Nov 5 12:13:38 UTC 2018
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 at amd.com>
>>>>> Cc: Daniel Rakos <Daniel.Rakos at amd.com>
>>>>> Cc: Jason Ekstrand <jason at jlekstrand.net>
>>>>> Cc: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
>>>>> Cc: Dave Airlie <airlied at redhat.com>
>>>>> Cc: Christian König <christian.koenig at amd.com>
>>>>> Cc: Chris Wilson <chris at 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
>
More information about the dri-devel
mailing list