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

zhoucm1 <zhoucm1@xxxxxxx> · Fri, 2 Nov 2018 18:34:54 +0800

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@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>
 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.
2. import fence from fd/handle to timeline sync point or banory syncobj.
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel







