drm/syncobj: add sync obj wait interface. (v6)

From: Dave Airlie <[email protected]>

This interface will allow sync object to be used to back Vulkan fences. This API is pretty much the vulkan fence waiting API, and I've ported the code from amdgpu.

v2: accept relative timeout, pass remaining time back to userspace.
v3: return to absolute timeouts.
v4: absolute zero = poll,
rewrite any/all code to have same operation for arrays return -EINVAL for 0 fences.
v4.1: fixup fences allocation check, use u64_to_user_ptr
v5: move to sec/nsec, and use timespec64 for calcs.
v6: use -ETIME and drop the out status flag. (-ETIME is suggested by ickle, I can feel a shed painting)

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/drm_internal.h |   2 +
 drivers/gpu/drm/drm_ioctl.c    |   2 +
 drivers/gpu/drm/drm_syncobj.c  | 142 ++++++++++++++++++++++++++++++
+++++++++++
 include/uapi/drm/drm.h         |  13 ++++
 4 files changed, 159 insertions(+)

drivers/gpu/drm/drm_ renamed from drivers/gpu/drm/drm_internal.h +0 -0

internal.h
index 5cecc97..d71b50d 100644

drivers/gpu/drm/drm_internal.h +2 -0

@@ -157,3 +157,5 @@ 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_wait_ioctl(struct drm_device *dev, void *data,
+                          struct drm_file *file_private);

drivers/gpu/drm/drm_ioctl.c +2 -0

index f1e5681..385ce74 100644
@@ -657,6 +657,8 @@ 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_WAIT, drm_syncobj_wait_ioctl,
+                     DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 

#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )

drivers/gpu/drm/drm_syncobj.c +2 -0

index 89441bc..2d5a7a1 100644
@@ -1,5 +1,7 @@
 /*
  * Copyright 2017 Red Hat
+ * Parts ported from amdgpu (fence wait code).
+ * Copyright 2016 Advanced Micro Devices, Inc.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the

"Software"),

drivers/gpu/drm/drm_syncobj.c +6 -0

@@ -31,6 +33,9 @@
  * that contain an optional fence. The fence can be updated with a new
  * fence, or be NULL.
  *
+ * syncobj's can be waited upon, where it will wait for the underlying
+ * fence.
+ *
  * syncobj's can be export to fd's and back, these fd's are opaque and
  * have no other use case, except passing the syncobj between processes.
  *

@@ -451,3 +456,140 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device
 *dev, void *data,
        return drm_syncobj_fd_to_handle(file_private, args->fd,
                                        &args->handle);
 }
+
+/**
+ * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute

value
+ *
+ * @timeout_sec: timeout sec component, 0 for poll
+ * @timeout_nsec: timeout nsec component in ns, 0 for poll
+ * both must be 0 for poll.
+ *
+ * Calculate the timeout in jiffies from an absolute time in sec/nsec.
+ */
+static unsigned long drm_timeout_abs_to_jiffies(int64_t timeout_sec, uint64_t timeout_nsec)

drivers/gpu/drm/drm_syncobj.c +16 -0

+{
+       struct timespec64 abs_timeout, timeout, max_jiffy_timespec;
+       unsigned long timeout_jiffies;
+
+       /* make 0 timeout means poll - absolute 0 doesn't seem valid */
+       if (timeout_sec == 0 && timeout_nsec == 0)
+               return 0;
+
+       abs_timeout.tv_sec = timeout_sec;
+       abs_timeout.tv_nsec = timeout_nsec;
+
+       /* clamp timeout if it's to large */
+       if (!timespec64_valid_strict(&abs_timeout))
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       timeout = timespec64_sub(abs_timeout,

ktime_to_timespec64(ktime_get()));

drivers/gpu/drm/drm_syncobj.c +21 -0

+       if (!timespec64_valid(&timeout))
+               return 0;
+
+       jiffies_to_timespec64(MAX_JIFFY_OFFSET, &max_jiffy_timespec);
+       if (timespec64_compare(&timeout, &max_jiffy_timespec) >= 0)
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       timeout_jiffies = timespec64_to_jiffies(&timeout);
+       /*  clamp timeout to avoid infinite timeout */
+       if (timeout_jiffies >= MAX_SCHEDULE_TIMEOUT)
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       return timeout_jiffies + 1;
+}
+
+static int drm_syncobj_wait_fences(struct drm_device *dev,
+                                  struct drm_file *file_private,
+                                  struct drm_syncobj_wait *wait,
+                                  struct dma_fence **fences)
+{
+       unsigned long timeout = drm_timeout_abs_to_jiffies(wait->timeout_sec,

wait->timeout_nsec);

drivers/gpu/drm/drm_syncobj.c +7 -0

+       int ret = 0;
+       uint32_t first = ~0;
+
+       if (wait->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) {
+               int i;
+               for (i = 0; i < wait->count_handles; i++) {
+                       ret = dma_fence_wait_timeout(fences[i], true,

timeout);

drivers/gpu/drm/drm_syncobj.c +38 -0

+
+                       if (ret < 0)
+                               return ret;
+                       if (ret == 0)
+                               break;
+                       timeout = ret;
+               }
+               first = 0;
+       } else {
+               ret = dma_fence_wait_any_timeout(fences,
+                                                wait->count_handles,
+                                                true, timeout,
+                                                &first);
+       }
+
+       if (ret < 0)
+               return ret;
+
+       wait->first_signaled = first;
+       if (ret == 0)
+               return -ETIME;
+       return 0;
+}
+
+int
+drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+                      struct drm_file *file_private)
+{
+       struct drm_syncobj_wait *args = data;
+       uint32_t *handles;
+       struct dma_fence **fences;
+       int ret = 0;
+       int i;
+
+       if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+               return -ENODEV;
+
+       if (args->flags != 0 && args->flags != DRM_SYNCOBJ_WAIT_FLAGS_WAIT_

ALL)

drivers/gpu/drm/drm_syncobj.c +24 -0

+               return -EINVAL;
+
+       if (args->count_handles == 0)
+               return -EINVAL;
+
+       /* Get the handles from userspace */
+       handles = kmalloc_array(args->count_handles, sizeof(uint32_t),
+                               GFP_KERNEL);
+       if (handles == NULL)
+               return -ENOMEM;
+
+       if (copy_from_user(handles,
+                          u64_to_user_ptr(args->handles),
+                          sizeof(uint32_t) * args->count_handles)) {
+               ret = -EFAULT;
+               goto err_free_handles;
+       }
+
+       fences = kcalloc(args->count_handles,
+                        sizeof(struct dma_fence *), GFP_KERNEL);
+       if (!fences) {
+               ret = -ENOMEM;
+               goto err_free_handles;
+       }

drivers/gpu/drm/drm_syncobj.c +10 -0

+
+err_free_fence_array:
+       for (i = 0; i < args->count_handles; i++)
+               dma_fence_put(fences[i]);
+       kfree(fences);
+err_free_handles:
+       kfree(handles);
+
+       return ret;
+}

include/uapi/drm/drm.h +13 -0

index 101593a..91746a7 100644
@@ -718,6 +718,18 @@ struct drm_syncobj_handle {
        __u32 pad;
 };
 
+#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
+struct drm_syncobj_wait {
+       __u64 handles;
+       /* absolute timeout */
+       __s64 timeout_sec;
+       __s64 timeout_nsec;
+       __u32 count_handles;
+       __u32 flags;
+       __u32 first_signaled; /* only valid when not waiting all */
+       __u32 pad;
+};
+
 #if defined(__cplusplus)
 }
 #endif

@@ -840,6 +852,7 @@ extern "C" {
 #define DRM_IOCTL_SYNCOBJ_DESTROY      DRM_IOWR(0xC0, struct
 drm_syncobj_destroy)
 #define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct
 drm_syncobj_handle)
 #define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct
 drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_WAIT         DRM_IOWR(0xC3, struct

drm_syncobj_wait)

/**
* Device specific ioctls should only be in their respective headers
--
2.9.4

_______________________________________________
dri-devel mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Dave Airlie <[email protected] <mailto:[email protected]>>

This interface will allow sync object to be used to back Vulkan fences. This API is pretty much the vulkan fence waiting API, and I've ported the code from amdgpu.

v2: accept relative timeout, pass remaining time back to userspace.
v3: return to absolute timeouts.
v4: absolute zero = poll,
rewrite any/all code to have same operation for arrays return -EINVAL for 0 fences.
v4.1: fixup fences allocation check, use u64_to_user_ptr
v5: move to sec/nsec, and use timespec64 for calcs.
v6: use -ETIME and drop the out status flag. (-ETIME is suggested by ickle, I can feel a shed painting)

Signed-off-by Dave Airlie <[email protected]

<mailto:[email protected]>>
---
drivers/gpu/drm/drm_internal.h | 2 +
drivers/gpu/drm/drm_ioctl.c | 2 +
drivers/gpu/drm/drm_syncobj.c | 142
+++++++++++++++++++++++++++++++++++++++++
include/uapi/drm/drm.h | 13 ++++
4 files changed, 159 insertions(+)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 5cecc97..d71b50d 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -157,3 +157,5 @@ 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_wait_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index f1e5681..385ce74 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -657,6 +657,8 @@ 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_WAIT, drm_syncobj_wait_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
};

#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 89441bc..2d5a7a1 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1,5 +1,7 @@
/*
* Copyright 2017 Red Hat
+ * Parts ported from amdgpu (fence wait code).
+ * Copyright 2016 Advanced Micro Devices, Inc.
*
* Permission is hereby granted, free of charge, to any person
obtaining a
* copy of this software and associated documentation files (the
"Software"),
@@ -31,6 +33,9 @@
* that contain an optional fence. The fence can be updated with
a new
* fence, or be NULL.
*
+ * syncobj's can be waited upon, where it will wait for the underlying
+ * fence.
+ *
* syncobj's can be export to fd's and back, these fd's are
opaque and
* have no other use case, except passing the syncobj between
processes.
*
@@ -451,3 +456,140 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
return drm_syncobj_fd_to_handle(file_private, args->fd,
&args->handle);
}
+
+/**
+ * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute value
+ *
+ * @timeout_sec: timeout sec component, 0 for poll
+ * @timeout_nsec: timeout nsec component in ns, 0 for poll
+ * both must be 0 for poll.
+ *
+ * Calculate the timeout in jiffies from an absolute time in sec/nsec.
+ */
+static unsigned long drm_timeout_abs_to_jiffies(int64_t timeout_sec, uint64_t timeout_nsec)
+{
+ struct timespec64 abs_timeout, timeout, max_jiffy_timespec;
+ unsigned long timeout_jiffies;
+
+ /* make 0 timeout means poll - absolute 0 doesn't seem
valid */
+ if (timeout_sec == 0 && timeout_nsec == 0)
+ return 0;
+
+ abs_timeout.tv_sec = timeout_sec;
+ abs_timeout.tv_nsec = timeout_nsec;
+
+ /* clamp timeout if it's to large */
+ if (!timespec64_valid_strict(&abs_timeout))
+ return MAX_SCHEDULE_TIMEOUT - 1;
+
+ timeout = timespec64_sub(abs_timeout,
ktime_to_timespec64(ktime_get()));
+ if (!timespec64_valid(&timeout))
+ return 0;
+
+ jiffies_to_timespec64(MAX_JIFFY_OFFSET, &max_jiffy_timespec);
+ if (timespec64_compare(&timeout, &max_jiffy_timespec) >= 0)
+ return MAX_SCHEDULE_TIMEOUT - 1;
+
+ timeout_jiffies = timespec64_to_jiffies(&timeout);
+ /* clamp timeout to avoid infinite timeout */
+ if (timeout_jiffies >= MAX_SCHEDULE_TIMEOUT)
+ return MAX_SCHEDULE_TIMEOUT - 1;
+
+ return timeout_jiffies + 1;
+}
+
+static int drm_syncobj_wait_fences(struct drm_device *dev,
+ struct drm_file *file_private,
+ struct drm_syncobj_wait *wait,
+ struct dma_fence **fences)
+{
+ unsigned long timeout =
drm_timeout_abs_to_jiffies(wait->timeout_sec, wait->timeout_nsec);
+ int ret = 0;
+ uint32_t first = ~0;
+
+ if (wait->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) {
+ int i;
+ for (i = 0; i < wait->count_handles; i++) {
+ ret = dma_fence_wait_timeout(fences[i],
true, timeout);
+
+ if (ret < 0)
+ return ret;
+ if (ret == 0)
+ break;
+ timeout = ret;
+ }
+ first = 0;
+ } else {
+ ret = dma_fence_wait_any_timeout(fences,
+ wait->count_handles,
+ true, timeout,
+ &first);
+ }
+
+ if (ret < 0)
+ return ret;
+
+ wait->first_signaled = first;
+ if (ret == 0)
+ return -ETIME;
+ return 0;
+}
+
+int
+drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_wait *args = data;
+ uint32_t *handles;
+ struct dma_fence **fences;
+ int ret = 0;
+ int i;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ if (args->flags != 0 && args->flags !=
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
+ return -EINVAL;
+
+ if (args->count_handles == 0)
+ return -EINVAL;
+
+ /* Get the handles from userspace */
+ handles = kmalloc_array(args->count_handles, sizeof(uint32_t),
+ GFP_KERNEL);
+ if (handles == NULL)
+ return -ENOMEM;
+
+ if (copy_from_user(handles,
+ u64_to_user_ptr(args->handles),
+ sizeof(uint32_t) * args->count_handles)) {
+ ret = -EFAULT;
+ goto err_free_handles;
+ }
+
+ fences = kcalloc(args->count_handles,
+ sizeof(struct dma_fence *), GFP_KERNEL);
+ if (!fences) {
+ ret = -ENOMEM;
+ goto err_free_handles;
+ }
+
+ for (i = 0; i < args->count_handles; i++) {
+ ret = drm_syncobj_fence_get(file_private, handles[i],
+ &fences[i]);
+ if (ret)
+ goto err_free_fence_array;
+ }
+
+ ret = drm_syncobj_wait_fences(dev, file_private,
+ args, fences);

So, reading some CTS tests again, and I think we have a problem here. The Vulkan spec allows you to wait on a fence that is in the unsignaled state.

In theory, you could have thread A start waiting on a fence before
thread B submits the work which triggers that fence. This means that the dma_fence may not exist yet when vkWaitForFences gets called. If we really want to support the full Vulkan usage, we need to somehow support missing dma_fences by waiting for the dma_fence to show up. Unfortunately, I don't know enough about the internal kernel APIs to know what that would look like.

    +
    +err_free_fence_array:
    +       for (i = 0; i < args->count_handles; i++)
    +               dma_fence_put(fences[i]);
    +       kfree(fences);
    +err_free_handles:
    +       kfree(handles);
    +
    +       return ret;
    +}
    diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
    index 101593a..91746a7 100644
    --- a/include/uapi/drm/drm.h
    +++ b/include/uapi/drm/drm.h
    @@ -718,6 +718,18 @@ struct drm_syncobj_handle {
            __u32 pad;
     };

    +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
    +struct drm_syncobj_wait {
    +       __u64 handles;
    +       /* absolute timeout */
    +       __s64 timeout_sec;
    +       __s64 timeout_nsec;
    +       __u32 count_handles;
    +       __u32 flags;
    +       __u32 first_signaled; /* only valid when not waiting all */
    +       __u32 pad;
    +};
    +
     #if defined(__cplusplus)
     }
     #endif
    @@ -840,6 +852,7 @@ extern "C" {
     #define DRM_IOCTL_SYNCOBJ_DESTROY      DRM_IOWR(0xC0, struct
    drm_syncobj_destroy)
     #define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct
    drm_syncobj_handle)
     #define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct
    drm_syncobj_handle)
    +#define DRM_IOCTL_SYNCOBJ_WAIT         DRM_IOWR(0xC3, struct
    drm_syncobj_wait)

     /**
      * Device specific ioctls should only be in their respective headers
    --
    2.9.4

    _______________________________________________
    dri-devel mailing list
    [email protected]
    <mailto:[email protected]>
    https://lists.freedesktop.org/mailman/listinfo/dri-devel
    <https://lists.freedesktop.org/mailman/listinfo/dri-devel>

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

Quoted 206 lines · Dave Airlie

From: Dave Airlie <[email protected]>

This interface will allow sync object to be used to back Vulkan fences. This API is pretty much the vulkan fence waiting API, and I've ported the code from amdgpu.

v2: accept relative timeout, pass remaining time back to userspace.
v3: return to absolute timeouts.
v4: absolute zero = poll,
rewrite any/all code to have same operation for arrays return -EINVAL for 0 fences.
v4.1: fixup fences allocation check, use u64_to_user_ptr
v5: move to sec/nsec, and use timespec64 for calcs.
v6: use -ETIME and drop the out status flag. (-ETIME is suggested by ickle, I can feel a shed painting)

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/drm_internal.h |   2 +
 drivers/gpu/drm/drm_ioctl.c    |   2 +
 drivers/gpu/drm/drm_syncobj.c  | 142 ++++++++++++++++++++++++++++++
+++++++++++
 include/uapi/drm/drm.h         |  13 ++++
 4 files changed, 159 insertions(+)

drivers/gpu/drm/drm_internal.h +2 -0

index 5cecc97..d71b50d 100644
@@ -157,3 +157,5 @@ 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_wait_ioctl(struct drm_device *dev, void *data,
+                          struct drm_file *file_private);

drivers/gpu/drm/drm_ioctl.c +2 -0

index f1e5681..385ce74 100644
@@ -657,6 +657,8 @@ 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_WAIT, drm_syncobj_wait_ioctl,
+                     DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 

#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )

drivers/gpu/drm/drm_syncobj. renamed from drivers/gpu/drm/drm_syncobj.c +0 -0

c
index 89441bc..2d5a7a1 100644

drivers/gpu/drm/drm_syncobj.c +2 -0

@@ -1,5 +1,7 @@
 /*
  * Copyright 2017 Red Hat
+ * Parts ported from amdgpu (fence wait code).
+ * Copyright 2016 Advanced Micro Devices, Inc.
  *
  * Permission is hereby granted, free of charge, to any person obtaining

a
* copy of this software and associated documentation files (the
"Software"),

drivers/gpu/drm/drm_syncobj.c +6 -0

@@ -31,6 +33,9 @@
  * that contain an optional fence. The fence can be updated with a new
  * fence, or be NULL.
  *
+ * syncobj's can be waited upon, where it will wait for the underlying
+ * fence.
+ *
  * syncobj's can be export to fd's and back, these fd's are opaque and
  * have no other use case, except passing the syncobj between processes.
  *

@@ -451,3 +456,140 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device
 *dev, void *data,
        return drm_syncobj_fd_to_handle(file_private, args->fd,
                                        &args->handle);
 }
+
+/**
+ * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute

value
+ *
+ * @timeout_sec: timeout sec component, 0 for poll
+ * @timeout_nsec: timeout nsec component in ns, 0 for poll
+ * both must be 0 for poll.
+ *
+ * Calculate the timeout in jiffies from an absolute time in sec/nsec.
+ */
+static unsigned long drm_timeout_abs_to_jiffies(int64_t timeout_sec, uint64_t timeout_nsec)

drivers/gpu/drm/drm_syncobj.c +16 -0

+{
+       struct timespec64 abs_timeout, timeout, max_jiffy_timespec;
+       unsigned long timeout_jiffies;
+
+       /* make 0 timeout means poll - absolute 0 doesn't seem valid */
+       if (timeout_sec == 0 && timeout_nsec == 0)
+               return 0;
+
+       abs_timeout.tv_sec = timeout_sec;
+       abs_timeout.tv_nsec = timeout_nsec;
+
+       /* clamp timeout if it's to large */
+       if (!timespec64_valid_strict(&abs_timeout))
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       timeout = timespec64_sub(abs_timeout,

ktime_to_timespec64(ktime_get()));

drivers/gpu/drm/drm_syncobj.c +21 -0

+       if (!timespec64_valid(&timeout))
+               return 0;
+
+       jiffies_to_timespec64(MAX_JIFFY_OFFSET, &max_jiffy_timespec);
+       if (timespec64_compare(&timeout, &max_jiffy_timespec) >= 0)
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       timeout_jiffies = timespec64_to_jiffies(&timeout);
+       /*  clamp timeout to avoid infinite timeout */
+       if (timeout_jiffies >= MAX_SCHEDULE_TIMEOUT)
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       return timeout_jiffies + 1;
+}
+
+static int drm_syncobj_wait_fences(struct drm_device *dev,
+                                  struct drm_file *file_private,
+                                  struct drm_syncobj_wait *wait,
+                                  struct dma_fence **fences)
+{
+       unsigned long timeout = drm_timeout_abs_to_jiffies(wait->timeout_sec,

wait->timeout_nsec);

drivers/gpu/drm/drm_syncobj.c +7 -0

+       int ret = 0;
+       uint32_t first = ~0;
+
+       if (wait->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) {
+               int i;
+               for (i = 0; i < wait->count_handles; i++) {
+                       ret = dma_fence_wait_timeout(fences[i], true,

timeout);

drivers/gpu/drm/drm_syncobj.c +38 -0

+
+                       if (ret < 0)
+                               return ret;
+                       if (ret == 0)
+                               break;
+                       timeout = ret;
+               }
+               first = 0;
+       } else {
+               ret = dma_fence_wait_any_timeout(fences,
+                                                wait->count_handles,
+                                                true, timeout,
+                                                &first);
+       }
+
+       if (ret < 0)
+               return ret;
+
+       wait->first_signaled = first;
+       if (ret == 0)
+               return -ETIME;
+       return 0;
+}
+
+int
+drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+                      struct drm_file *file_private)
+{
+       struct drm_syncobj_wait *args = data;
+       uint32_t *handles;
+       struct dma_fence **fences;
+       int ret = 0;
+       int i;
+
+       if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+               return -ENODEV;
+
+       if (args->flags != 0 && args->flags !=

DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)

drivers/gpu/drm/drm_syncobj.c +24 -0

+               return -EINVAL;
+
+       if (args->count_handles == 0)
+               return -EINVAL;
+
+       /* Get the handles from userspace */
+       handles = kmalloc_array(args->count_handles, sizeof(uint32_t),
+                               GFP_KERNEL);
+       if (handles == NULL)
+               return -ENOMEM;
+
+       if (copy_from_user(handles,
+                          u64_to_user_ptr(args->handles),
+                          sizeof(uint32_t) * args->count_handles)) {
+               ret = -EFAULT;
+               goto err_free_handles;
+       }
+
+       fences = kcalloc(args->count_handles,
+                        sizeof(struct dma_fence *), GFP_KERNEL);
+       if (!fences) {
+               ret = -ENOMEM;
+               goto err_free_handles;
+       }

drivers/gpu/drm/drm_syncobj.c drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, · line 585

Quoted 10 lines

drivers/gpu/drm/drm_syncobj.c +10 -0

+
+       for (i = 0; i < args->count_handles; i++) {
+               ret = drm_syncobj_fence_get(file_private, handles[i],
+                                           &fences[i]);
+               if (ret)
+                       goto err_free_fence_array;
+       }
+
+       ret = drm_syncobj_wait_fences(dev, file_private,
+                                     args, fences);

So, reading some CTS tests again, and I think we have a problem here. The Vulkan spec allows you to wait on a fence that is in the unsignaled state.

At least on the closed source driver that would be illegal as far as I know.

You can't wait on a semaphore before the signal operation is send down to the kerel.

Regards,
Christian.

In theory, you could have thread A start waiting on a fence before
thread B submits the work which triggers that fence. This means that the dma_fence may not exist yet when vkWaitForFences gets called. If we really want to support the full Vulkan usage, we need to somehow support missing dma_fences by waiting for the dma_fence to show up. Unfortunately, I don't know enough about the internal kernel APIs to know what that would look like.

Quoted 48 lines

drivers/gpu/drm/drm_syncobj.c +10 -0

+
+err_free_fence_array:
+       for (i = 0; i < args->count_handles; i++)
+               dma_fence_put(fences[i]);
+       kfree(fences);
+err_free_handles:
+       kfree(handles);
+
+       return ret;
+}

include/uapi/drm/drm.h +13 -0

index 101593a..91746a7 100644
@@ -718,6 +718,18 @@ struct drm_syncobj_handle {
        __u32 pad;
 };
 
+#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
+struct drm_syncobj_wait {
+       __u64 handles;
+       /* absolute timeout */
+       __s64 timeout_sec;
+       __s64 timeout_nsec;
+       __u32 count_handles;
+       __u32 flags;
+       __u32 first_signaled; /* only valid when not waiting all */
+       __u32 pad;
+};
+
 #if defined(__cplusplus)
 }
 #endif

@@ -840,6 +852,7 @@ extern "C" {
 #define DRM_IOCTL_SYNCOBJ_DESTROY      DRM_IOWR(0xC0, struct
 drm_syncobj_destroy)
 #define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct
 drm_syncobj_handle)
 #define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct
 drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_WAIT         DRM_IOWR(0xC3, struct

drm_syncobj_wait)

/**
* Device specific ioctls should only be in their respective headers
--
2.9.4

Quoted 5 lines

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
amd-gfx mailing [email protected]://lists.freedesktop.org/mailman/listinfo/amd-gfx

I understand this discussion from closes source driver terminology.

If a process is killed before it sends out the signaling command, will some part of the GPU be in a waiting situation forever?

Alex Bin Xie

*From:*amd-gfx [mailto:amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org] *On Behalf Of *Jason Ekstrand
*Sent:* Monday, July 10, 2017 11:53 AM
*To:* Christian König <[email protected]>
*Cc:* Dave Airlie <[email protected]>; Maling list - DRI developers <[email protected]>; amd-gfx mailing list <[email protected]> *Subject:* Re: [PATCH] drm/syncobj: add sync obj wait interface. (v6)

On Mon, Jul 10, 2017 at 8:45 AM, Christian König <[email protected] <mailto:[email protected]>> wrote:

Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <[email protected] <mailto:[email protected]>> wrote:

From: Dave Airlie <[email protected] <mailto:[email protected]>>

This interface will allow sync object to be used to back Vulkan fences. This API is pretty much the vulkan fence waiting
API, and I've ported the code from amdgpu.

v2: accept relative timeout, pass remaining time back to userspace.
v3: return to absolute timeouts.
v4: absolute zero = poll,
rewrite any/all code to have same operation for arrays return -EINVAL for 0 fences.
v4.1: fixup fences allocation check, use u64_to_user_ptr
v5: move to sec/nsec, and use timespec64 for calcs.
v6: use -ETIME and drop the out status flag. (-ETIME is suggested by ickle, I can feel a shed painting)

Signed-off-by Dave Airlie <[email protected]

<mailto:[email protected]>>
---
drivers/gpu/drm/drm_internal.h | 2 +
drivers/gpu/drm/drm_ioctl.c | 2 +
drivers/gpu/drm/drm_syncobj.c | 142
+++++++++++++++++++++++++++++++++++++++++
include/uapi/drm/drm.h | 13 ++++
4 files changed, 159 insertions(+)

diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 5cecc97..d71b50d 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -157,3 +157,5 @@ 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_wait_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index f1e5681..385ce74 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -657,6 +657,8 @@ 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_WAIT, drm_syncobj_wait_ioctl,
+ DRM_UNLOCKED|DRM_RENDER_ALLOW),
};

#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 89441bc..2d5a7a1 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1,5 +1,7 @@
/*
* Copyright 2017 Red Hat
+ * Parts ported from amdgpu (fence wait code).
+ * Copyright 2016 Advanced Micro Devices, Inc. *
* Permission is hereby granted, free of charge, to any
person obtaining a
* copy of this software and associated documentation
files (the "Software"),
@@ -31,6 +33,9 @@
* that contain an optional fence. The fence can be
updated with a new
* fence, or be NULL.
*
+ * syncobj's can be waited upon, where it will wait for the underlying
+ * fence.
+ *
* syncobj's can be export to fd's and back, these fd's
are opaque and
* have no other use case, except passing the syncobj
between processes.
*
@@ -451,3 +456,140 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data,
return drm_syncobj_fd_to_handle(file_private,
args->fd,
&args->handle);
}
+
+/**
+ * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute value
+ *
+ * @timeout_sec: timeout sec component, 0 for poll
+ * @timeout_nsec: timeout nsec component in ns, 0 for poll
+ * both must be 0 for poll.
+ *
+ * Calculate the timeout in jiffies from an absolute time in sec/nsec.
+ */
+static unsigned long drm_timeout_abs_to_jiffies(int64_t timeout_sec, uint64_t timeout_nsec)
+{
+ struct timespec64 abs_timeout, timeout,
max_jiffy_timespec;
+ unsigned long timeout_jiffies;
+
+ /* make 0 timeout means poll - absolute 0 doesn't
seem valid */
+ if (timeout_sec == 0 && timeout_nsec == 0)
+ return 0;
+
+ abs_timeout.tv_sec = timeout_sec;
+ abs_timeout.tv_nsec = timeout_nsec;
+
+ /* clamp timeout if it's to large */
+ if (!timespec64_valid_strict(&abs_timeout))
+ return MAX_SCHEDULE_TIMEOUT - 1;
+
+ timeout = timespec64_sub(abs_timeout,
ktime_to_timespec64(ktime_get()));
+ if (!timespec64_valid(&timeout))
+ return 0;
+
+ jiffies_to_timespec64(MAX_JIFFY_OFFSET, &max_jiffy_timespec);
+ if (timespec64_compare(&timeout,
&max_jiffy_timespec) >= 0)
+ return MAX_SCHEDULE_TIMEOUT - 1;
+
+ timeout_jiffies = timespec64_to_jiffies(&timeout);
+ /* clamp timeout to avoid infinite timeout */
+ if (timeout_jiffies >= MAX_SCHEDULE_TIMEOUT)
+ return MAX_SCHEDULE_TIMEOUT - 1;
+
+ return timeout_jiffies + 1;
+}
+
+static int drm_syncobj_wait_fences(struct drm_device *dev,
+ struct drm_file *file_private,
+ struct drm_syncobj_wait *wait,
+ struct dma_fence **fences)
+{
+ unsigned long timeout =
drm_timeout_abs_to_jiffies(wait->timeout_sec, wait->timeout_nsec);
+ int ret = 0;
+ uint32_t first = ~0;
+
+ if (wait->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) {
+ int i;
+ for (i = 0; i < wait->count_handles; i++) {
+ ret =
dma_fence_wait_timeout(fences[i], true, timeout); +
+ if (ret < 0)
+ return ret;
+ if (ret == 0)
+ break;
+ timeout = ret;
+ }
+ first = 0;
+ } else {
+ ret = dma_fence_wait_any_timeout(fences,
+ wait->count_handles,
+ true, timeout,
+ &first);
+ }
+
+ if (ret < 0)
+ return ret;
+
+ wait->first_signaled = first;
+ if (ret == 0)
+ return -ETIME;
+ return 0;
+}
+
+int
+drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_private)
+{
+ struct drm_syncobj_wait *args = data;
+ uint32_t *handles;
+ struct dma_fence **fences;
+ int ret = 0;
+ int i;
+
+ if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+ return -ENODEV;
+
+ if (args->flags != 0 && args->flags !=
DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)
+ return -EINVAL;
+
+ if (args->count_handles == 0)
+ return -EINVAL;
+
+ /* Get the handles from userspace */
+ handles = kmalloc_array(args->count_handles,
sizeof(uint32_t),
+ GFP_KERNEL);
+ if (handles == NULL)
+ return -ENOMEM;
+
+ if (copy_from_user(handles,
+ u64_to_user_ptr(args->handles),
+ sizeof(uint32_t) * args->count_handles)) {
+ ret = -EFAULT;
+ goto err_free_handles;
+ }
+
+ fences = kcalloc(args->count_handles,
+ sizeof(struct dma_fence *),
GFP_KERNEL);
+ if (!fences) {
+ ret = -ENOMEM;
+ goto err_free_handles;
+ }
+
+ for (i = 0; i < args->count_handles; i++) {
+ ret = drm_syncobj_fence_get(file_private,
handles[i],
+ &fences[i]);
+ if (ret)
+ goto err_free_fence_array;
+ }
+
+ ret = drm_syncobj_wait_fences(dev, file_private,
+ args, fences);

So, reading some CTS tests again, and I think we have a problem here. The Vulkan spec allows you to wait on a fence that is in the unsignaled state.

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74

You can't wait on a semaphore before the signal operation is send down to the kerel.

We (Intel) deal with this today by tracking whether or not the fence has been submitted and using a condition variable in userspace to sort it all out. If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

--Jason

    Regards,
    Christian.




          In theory, you could have thread A start waiting on a fence
        before thread B submits the work which triggers that fence. 
        This means that the dma_fence may not exist yet when
        vkWaitForFences gets called.  If we really want to support the
        full Vulkan usage, we need to somehow support missing
        dma_fences by waiting for the dma_fence to show up.
        Unfortunately, I don't know enough about the internal kernel
        APIs to know what that would look like.

            +
            +err_free_fence_array:
            +       for (i = 0; i < args->count_handles; i++)
            +  dma_fence_put(fences[i]);
            +       kfree(fences);
            +err_free_handles:
            +       kfree(handles);
            +
            +       return ret;
            +}
            diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
            index 101593a..91746a7 100644
            --- a/include/uapi/drm/drm.h
            +++ b/include/uapi/drm/drm.h
            @@ -718,6 +718,18 @@ struct drm_syncobj_handle {
                    __u32 pad;
             };

            +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
            +struct drm_syncobj_wait {
            +       __u64 handles;
            +       /* absolute timeout */
            +       __s64 timeout_sec;
            +       __s64 timeout_nsec;
            +       __u32 count_handles;
            +       __u32 flags;
            +       __u32 first_signaled; /* only valid when not
            waiting all */
            +       __u32 pad;
            +};
            +
             #if defined(__cplusplus)
             }
             #endif
            @@ -840,6 +852,7 @@ extern "C" {
             #define DRM_IOCTL_SYNCOBJ_DESTROY   DRM_IOWR(0xC0, struct
            drm_syncobj_destroy)
             #define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1,
            struct drm_syncobj_handle)
             #define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2,
            struct drm_syncobj_handle)
            +#define DRM_IOCTL_SYNCOBJ_WAIT  DRM_IOWR(0xC3, struct
            drm_syncobj_wait)

             /**
              * Device specific ioctls should only be in their
            respective headers
            --
            2.9.4

            _______________________________________________
            dri-devel mailing list
            [email protected]
            <mailto:[email protected]>
            https://lists.freedesktop.org/mailman/listinfo/dri-devel

        _______________________________________________

        amd-gfx mailing list

        [email protected]
        <mailto:[email protected]>

        https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Am 10.07.2017 um 17:58 schrieb Xie, AlexBin:

I understand this discussion from closes source driver terminology.

If a process is killed before it sends out the signaling command, will some part of the GPU be in a waiting situation forever?

Yes, exactly that's the problem here and the reason why that even Microsoft forbids that under windows.

Christian.

Alex Bin Xie

*From:* amd-gfx [mailto:amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>] *On Behalf Of *Jason Ekstrand
*Sent:* Monday, July 10, 2017 11:53 AM
*To:* Christian König <[email protected]> <[email protected]>
*Cc:* Dave Airlie <[email protected]> <[email protected]>; Maling list - DRI developers <[email protected]> <[email protected]>; amd-gfx mailing list <[email protected]> <[email protected]> *Subject:* Re: [PATCH] drm/syncobj: add sync obj wait interface. (v6)

On Mon, Jul 10, 2017 at 8:45 AM, Christian König <[email protected]> wrote:

Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <[email protected]> wrote:

From: Dave Airlie <[email protected]>

This interface will allow sync object to be used to back Vulkan fences. This API is pretty much the vulkan fence waiting API, and I've ported the code from amdgpu.

v2: accept relative timeout, pass remaining time back to userspace.
v3: return to absolute timeouts.
v4: absolute zero = poll,
rewrite any/all code to have same operation for arrays return -EINVAL for 0 fences.
v4.1: fixup fences allocation check, use u64_to_user_ptr
v5: move to sec/nsec, and use timespec64 for calcs.
v6: use -ETIME and drop the out status flag. (-ETIME is suggested by ickle, I can feel a shed painting)

Signed-off-by Dave Airlie <[email protected]>

---
 drivers/gpu/drm/drm_internal.h |   2 +
 drivers/gpu/drm/drm_ioctl.c    |   2 +
 drivers/gpu/drm/drm_syncobj.c  | 142 ++++++++++++++++++++++++++++++
+++++++++++
 include/uapi/drm/drm.h         |  13 ++++
 4 files changed, 159 insertions(+)

drivers/gpu/drm/drm_ renamed from drivers/gpu/drm/drm_internal.h +0 -0

internal.h
index 5cecc97..d71b50d 100644

drivers/gpu/drm/drm_internal.h +2 -0

@@ -157,3 +157,5 @@ 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_wait_ioctl(struct drm_device *dev, void *data,
+                          struct drm_file *file_private);

drivers/gpu/drm/drm_ioctl.c +2 -0

index f1e5681..385ce74 100644
@@ -657,6 +657,8 @@ 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_WAIT, drm_syncobj_wait_ioctl,
+                     DRM_UNLOCKED|DRM_RENDER_ALLOW),
 };
 

#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )

drivers/gpu/drm/drm_syncobj.c +2 -0

index 89441bc..2d5a7a1 100644
@@ -1,5 +1,7 @@
 /*
  * Copyright 2017 Red Hat
+ * Parts ported from amdgpu (fence wait code).
+ * Copyright 2016 Advanced Micro Devices, Inc.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the

"Software"),

drivers/gpu/drm/drm_syncobj.c +6 -0

@@ -31,6 +33,9 @@
  * that contain an optional fence. The fence can be updated with a new
  * fence, or be NULL.
  *
+ * syncobj's can be waited upon, where it will wait for the underlying
+ * fence.
+ *
  * syncobj's can be export to fd's and back, these fd's are opaque and
  * have no other use case, except passing the syncobj between processes.
  *

@@ -451,3 +456,140 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device
 *dev, void *data,
        return drm_syncobj_fd_to_handle(file_private, args->fd,
                                        &args->handle);
 }
+
+/**
+ * drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute

value
+ *
+ * @timeout_sec: timeout sec component, 0 for poll
+ * @timeout_nsec: timeout nsec component in ns, 0 for poll
+ * both must be 0 for poll.
+ *
+ * Calculate the timeout in jiffies from an absolute time in sec/nsec.
+ */
+static unsigned long drm_timeout_abs_to_jiffies(int64_t timeout_sec, uint64_t timeout_nsec)

drivers/gpu/drm/drm_syncobj.c +16 -0

+{
+       struct timespec64 abs_timeout, timeout, max_jiffy_timespec;
+       unsigned long timeout_jiffies;
+
+       /* make 0 timeout means poll - absolute 0 doesn't seem valid */
+       if (timeout_sec == 0 && timeout_nsec == 0)
+               return 0;
+
+       abs_timeout.tv_sec = timeout_sec;
+       abs_timeout.tv_nsec = timeout_nsec;
+
+       /* clamp timeout if it's to large */
+       if (!timespec64_valid_strict(&abs_timeout))
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       timeout = timespec64_sub(abs_timeout,

ktime_to_timespec64(ktime_get()));

drivers/gpu/drm/drm_syncobj.c +21 -0

+       if (!timespec64_valid(&timeout))
+               return 0;
+
+       jiffies_to_timespec64(MAX_JIFFY_OFFSET, &max_jiffy_timespec);
+       if (timespec64_compare(&timeout, &max_jiffy_timespec) >= 0)
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       timeout_jiffies = timespec64_to_jiffies(&timeout);
+       /*  clamp timeout to avoid infinite timeout */
+       if (timeout_jiffies >= MAX_SCHEDULE_TIMEOUT)
+               return MAX_SCHEDULE_TIMEOUT - 1;
+
+       return timeout_jiffies + 1;
+}
+
+static int drm_syncobj_wait_fences(struct drm_device *dev,
+                                  struct drm_file *file_private,
+                                  struct drm_syncobj_wait *wait,
+                                  struct dma_fence **fences)
+{
+       unsigned long timeout = drm_timeout_abs_to_jiffies(wait->timeout_sec,

wait->timeout_nsec);

drivers/gpu/drm/drm_syncobj.c +7 -0

+       int ret = 0;
+       uint32_t first = ~0;
+
+       if (wait->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) {
+               int i;
+               for (i = 0; i < wait->count_handles; i++) {
+                       ret = dma_fence_wait_timeout(fences[i], true,

timeout);

drivers/gpu/drm/drm_syncobj.c +38 -0

+
+                       if (ret < 0)
+                               return ret;
+                       if (ret == 0)
+                               break;
+                       timeout = ret;
+               }
+               first = 0;
+       } else {
+               ret = dma_fence_wait_any_timeout(fences,
+                                                wait->count_handles,
+                                                true, timeout,
+                                                &first);
+       }
+
+       if (ret < 0)
+               return ret;
+
+       wait->first_signaled = first;
+       if (ret == 0)
+               return -ETIME;
+       return 0;
+}
+
+int
+drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
+                      struct drm_file *file_private)
+{
+       struct drm_syncobj_wait *args = data;
+       uint32_t *handles;
+       struct dma_fence **fences;
+       int ret = 0;
+       int i;
+
+       if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
+               return -ENODEV;
+
+       if (args->flags != 0 && args->flags != DRM_SYNCOBJ_WAIT_FLAGS_WAIT_

ALL)

drivers/gpu/drm/drm_syncobj.c +34 -0

+               return -EINVAL;
+
+       if (args->count_handles == 0)
+               return -EINVAL;
+
+       /* Get the handles from userspace */
+       handles = kmalloc_array(args->count_handles, sizeof(uint32_t),
+                               GFP_KERNEL);
+       if (handles == NULL)
+               return -ENOMEM;
+
+       if (copy_from_user(handles,
+                          u64_to_user_ptr(args->handles),
+                          sizeof(uint32_t) * args->count_handles)) {
+               ret = -EFAULT;
+               goto err_free_handles;
+       }
+
+       fences = kcalloc(args->count_handles,
+                        sizeof(struct dma_fence *), GFP_KERNEL);
+       if (!fences) {
+               ret = -ENOMEM;
+               goto err_free_handles;
+       }
+
+       for (i = 0; i < args->count_handles; i++) {
+               ret = drm_syncobj_fence_get(file_private, handles[i],
+                                           &fences[i]);
+               if (ret)
+                       goto err_free_fence_array;
+       }
+
+       ret = drm_syncobj_wait_fences(dev, file_private,
+                                     args, fences);

So, reading some CTS tests again, and I think we have a problem here. The Vulkan spec allows you to wait on a fence that is in the unsignaled state.

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/
modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74

You can't wait on a semaphore before the signal operation is send down to the kerel.

We (Intel) deal with this today by tracking whether or not the fence has been submitted and using a condition variable in userspace to sort it all out. If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

--Jason

Regards,
Christian.

In theory, you could have thread A start waiting on a fence before
thread B submits the work which triggers that fence. This means that the dma_fence may not exist yet when vkWaitForFences gets called. If we really want to support the full Vulkan usage, we need to somehow support missing dma_fences by waiting for the dma_fence to show up. Unfortunately, I don't know enough about the internal kernel APIs to know what that would look like.

drivers/gpu/drm/drm_syncobj.c +10 -0

+
+err_free_fence_array:
+       for (i = 0; i < args->count_handles; i++)
+               dma_fence_put(fences[i]);
+       kfree(fences);
+err_free_handles:
+       kfree(handles);
+
+       return ret;
+}

include/uapi/drm/drm.h +13 -0

index 101593a..91746a7 100644
@@ -718,6 +718,18 @@ struct drm_syncobj_handle {
        __u32 pad;
 };
 
+#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
+struct drm_syncobj_wait {
+       __u64 handles;
+       /* absolute timeout */
+       __s64 timeout_sec;
+       __s64 timeout_nsec;
+       __u32 count_handles;
+       __u32 flags;
+       __u32 first_signaled; /* only valid when not waiting all */
+       __u32 pad;
+};
+
 #if defined(__cplusplus)
 }
 #endif

@@ -840,6 +852,7 @@ extern "C" {
 #define DRM_IOCTL_SYNCOBJ_DESTROY      DRM_IOWR(0xC0, struct
 drm_syncobj_destroy)
 #define DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD DRM_IOWR(0xC1, struct
 drm_syncobj_handle)
 #define DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE DRM_IOWR(0xC2, struct
 drm_syncobj_handle)
+#define DRM_IOCTL_SYNCOBJ_WAIT         DRM_IOWR(0xC3, struct

drm_syncobj_wait)

/**
* Device specific ioctls should only be in their respective headers
--
2.9.4

_______________________________________________
dri-devel mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________

amd-gfx mailing list

[email protected]

https://lists.freedesktop.org/mailman/listinfo/amd-gfx

    Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

Quoted 6 lines

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <[email protected] <mailto:[email protected]>> wrote:
[SNIP]
So, reading some CTS tests again, and I think we have a problem
here. The Vulkan spec allows you to wait on a fence that is in
the unsignaled state.

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74

You can't wait on a semaphore before the signal operation is send down to the kerel.

We (Intel) deal with this today by tracking whether or not the fence has been submitted and using a condition variable in userspace to sort it all out.

If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

Am 10.07.2017 um 17:52 schrieb Jason Ekstrand:

Quoted 9 lines · Christian König

Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <[email protected]> wrote:
[SNIP]
So, reading some CTS tests again, and I think we have a problem here.
The Vulkan spec allows you to wait on a fence that is in the unsignaled
state.

Quoted 3 lines

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/
modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74

Quoted 3 lines

You can't wait on a semaphore before the signal operation is send down to the kerel.

We (Intel) deal with this today by tracking whether or not the fence has been submitted and using a condition variable in userspace to sort it all out.

Which sounds exactly like what AMD is doing in it's drivers as well.

If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

That would clearly get a NAK from my side, even Microsoft forbids wait before signal because you can easily end up in deadlock situations.

On Mon, Jul 10, 2017 at 9:15 AM, Christian König <[email protected] <mailto:[email protected]>> wrote:

    Am 10.07.2017 um 17:52 schrieb Jason Ekstrand:

Quoted 26 lines

    On Mon, Jul 10, 2017 at 8:45 AM, Christian König
    <[email protected] <mailto:[email protected]>> wrote:

        Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

Quoted 6 lines

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <[email protected] <mailto:[email protected]>> wrote:
[SNIP]
So, reading some CTS tests again, and I think we have a problem here. The Vulkan spec allows you to wait on a fence that is in the unsignaled state.

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74
<https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74>

You can't wait on a semaphore before the signal operation is send down to the kerel.

Quoted 3 lines

We (Intel) deal with this today by tracking whether or not the fence has been submitted and using a condition variable in userspace to sort it all out.

Which sounds exactly like what AMD is doing in it's drivers as well.

Which doesn't work cross-process so...

Quoted 2 lines

If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

That would clearly get a NAK from my side, even Microsoft forbids wait before signal because you can easily end up in deadlock situations.

Please don't NAK things that are required by the API specification and CTS tests.

That makes it very hard for people like me to get their jobs done. :-)

Quoted 32 lines

On Mon, Jul 10, 2017 at 9:15 AM, Christian König <[email protected] <mailto:[email protected]>> wrote:

     Am 10.07.2017 um 17:52 schrieb Jason Ekstrand:

Quoted 28 lines

     On Mon, Jul 10, 2017 at 8:45 AM, Christian König
     <[email protected] <mailto:[email protected]>> wrote:

         Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

Quoted 6 lines

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <[email protected] <mailto:[email protected]>> wrote:
[SNIP]
So, reading some CTS tests again, and I think we have a problem here. The Vulkan spec allows you to wait on a fence that is in the unsignaled state.

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74
<https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74>

You can't wait on a semaphore before the signal operation is send down to the kerel.

We (Intel) deal with this today by tracking whether or not the fence has been submitted and using a condition variable in userspace to sort it all out.

Quoted 4 lines

Which sounds exactly like what AMD is doing in it's drivers as well.

Which doesn't work cross-process so...

Surely it can be made to work by providing suitable kernel APIs to userspace?

Quoted 7 lines

Quoted 2 lines

If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

That would clearly get a NAK from my side, even Microsoft forbids wait before signal because you can easily end up in deadlock situations.

Please don't NAK things that are required by the API specification and CTS tests.

There is no requirement for every aspect of the Vulkan API specification to be mirrored 1:1 in the kernel <-> userspace API. We have to work out what makes sense at each level.

Am 11.07.2017 um 04:36 schrieb Michel Dänzer:

Quoted 41 lines · Jason Ekstrand

Quoted 40 lines

On Mon, Jul 10, 2017 at 9:15 AM, Christian König
<[email protected] <mailto:[email protected]>> wrote:

     Am 10.07.2017 um 17:52 schrieb Jason Ekstrand:

Quoted 34 lines

     On Mon, Jul 10, 2017 at 8:45 AM, Christian König
     <[email protected] <mailto:[email protected]>> wrote:

         Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

Quoted 7 lines

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie
<[email protected] <mailto:[email protected]>> wrote:
[SNIP]
So, reading some CTS tests again, and I think we have a
problem here. The Vulkan spec allows you to wait on a fence
that is in the unsignaled state.

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/master/
external/vulkancts/modules/vulkan/synchronization/vktSync hronizationBasicFenceTests.cpp#L74
<https://github.com/KhronosGroup/VK-GL-CTS/blob/master/
external/vulkancts/modules/vulkan/synchronization/vktSync hronizationBasicFenceTests.cpp#L74>

You can't wait on a semaphore before the signal operation is send down to the kerel.

We (Intel) deal with this today by tracking whether or not the fence has been submitted and using a condition variable in userspace to sort it all out.

Quoted 8 lines

Quoted 5 lines

Which sounds exactly like what AMD is doing in it's drivers as well.

Which doesn't work cross-process so...

Surely it can be made to work by providing suitable kernel APIs to userspace?

Well, that's exactly what Jason proposed to do, but I'm not very keen of that.

If we ever want to share fences across processes (which we do),

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

then this needs to be sorted in the kernel.

That would clearly get a NAK from my side, even Microsoft forbids wait before signal because you can easily end up in deadlock
situations.

Quoted 7 lines

Quoted 3 lines

Please don't NAK things that are required by the API specification and CTS tests.

There is no requirement for every aspect of the Vulkan API specification to be mirrored 1:1 in the kernel <-> userspace API. We have to work out what makes sense at each level.

Exactly, if we have a synchronization problem between two processes that should be solved in userspace.

E.g. if process A hasn't submitted it's work to the kernel it should flush it's commands before sending a flip event to the compositor.

[SNIP]

If we ever want to share fences across processes
(which we do),
then this needs to be sorted in the kernel.

That would clearly get a NAK from my side, even
Microsoft forbids
wait before signal because you can easily end up in
deadlock situations.

Please don't NAK things that are required by the API specification and
CTS tests.

There is no requirement for every aspect of the Vulkan API specification
to be mirrored 1:1 in the kernel <-> userspace API. We have to work out
what makes sense at each level.

Exactly, if we have a synchronization problem between two processes that should be solved in userspace.

E.g. if process A hasn't submitted it's work to the kernel it should flush it's commands before sending a flip event to the compositor.

Ok, I think there is some confusion here on what is being proposed. Here are some things that are *not* being proposed:

1. This does *not* allow a client to block another client's GPU work
indefinitely. This is entirely for a CPU wait API to allow for a "wait for submit" as well as a "wait for finish".

2. This is *not* for system compositors that need to be robust
against malicious clients.

The expected use for the OPAQUE_FD is two very tightly integrated processes which trust each other but need to be able to share synchronization primitives.

Could we do this "in userspace"? Yes, with added kernel API. we would need some way of strapping a second FD onto a syncobj or combining two FDs into one to send across the wire or something like that, then add a shared memory segment, and then pile on a bunch of code to do cross-process condition variables and state tracking. I really don't see how that's a better solution than adding a flag to the kernel API to just do what we want.

Am 11.07.2017 um 17:43 schrieb Jason Ekstrand:

Quoted 18 lines · Christian König

[SNIP]

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

That would clearly get a NAK from my side, even Microsoft forbids wait before signal because you can easily end up in deadlock
situations.

Quoted 6 lines

Quoted 2 lines

Please don't NAK things that are required by the API specification and CTS tests.

There is no requirement for every aspect of the Vulkan API specification to be mirrored 1:1 in the kernel <-> userspace API. We have to work out what makes sense at each level.

Quoted 5 lines

Exactly, if we have a synchronization problem between two processes that should be solved in userspace.

E.g. if process A hasn't submitted it's work to the kernel it should flush it's commands before sending a flip event to the compositor.

Ok, I think there is some confusion here on what is being proposed. Here are some things that are *not* being proposed:

1. This does *not* allow a client to block another client's GPU work
indefinitely. This is entirely for a CPU wait API to allow for a "wait for submit" as well as a "wait for finish".

Yeah, that is a rather good point.

2. This is *not* for system compositors that need to be robust against
malicious clients.

I can see the point, but I think the kernel interface should still be idiot prove even in the unlikely case the universe suddenly stops to produce idiots.

The expected use for the OPAQUE_FD is two very tightly integrated processes which trust each other but need to be able to share synchronization primitives.

Well, that raises a really important question: What is the actual use case for this if not communication between client and compositor?

Could we do this "in userspace"? Yes, with added kernel API. we would need some way of strapping a second FD onto a syncobj or combining two FDs into one to send across the wire or something like that, then add a shared memory segment, and then pile on a bunch of code to do cross-process condition variables and state tracking. I really don't see how that's a better solution than adding a flag to the kernel API to just do what we want.

Way to complicated.

My thinking was rather to optionally allow a single page to be mmap()ed into the process address space from the fd and then put a futex, pthread_cond or X shared memory fence or anything like that into it.

Quoted 3 lines · Christian König

Am 11.07.2017 um 17:43 schrieb Jason Ekstrand:

On Tue, Jul 11, 2017 at 12:17 AM, Christian König <

[email protected]>

Quoted 7 lines

Quoted 6 lines · wrote:

[SNIP]

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

That would clearly get a NAK from my side, even Microsoft forbids wait before signal because you can easily end up in deadlock
situations.

Please don't NAK things that are required by the API specification and CTS tests.

There is no requirement for every aspect of the Vulkan API

specification

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

to be mirrored 1:1 in the kernel <-> userspace API. We have to work out what makes sense at each level.

Exactly, if we have a synchronization problem between two processes that should be solved in userspace.

E.g. if process A hasn't submitted it's work to the kernel it should

flush

Quoted 8 lines

Quoted 1 line

it's commands before sending a flip event to the compositor.

Ok, I think there is some confusion here on what is being proposed. Here are some things that are *not* being proposed:

1. This does *not* allow a client to block another client's GPU work
indefinitely. This is entirely for a CPU wait API to allow for a "wait

for

Quoted 2 lines

submit" as well as a "wait for finish".

Quoted 6 lines

Yeah, that is a rather good point.

2. This is *not* for system compositors that need to be robust against
malicious clients.

I can see the point, but I think the kernel interface should still be

idiot

Quoted 2 lines

prove even in the unlikely case the universe suddenly stops to produce idiots.

Quoted 1 line

The expected use for the OPAQUE_FD is two very tightly integrated

processes

Quoted 4 lines

which trust each other but need to be able to share synchronization primitives.

Well, that raises a really important question: What is the actual use

case

Quoted 1 line

for this if not communication between client and compositor?

VR clients and compositors.

Quoted 1 line

Could we do this "in userspace"? Yes, with added kernel API. we would

need

Quoted 1 line

some way of strapping a second FD onto a syncobj or combining two FDs

into

Quoted 1 line

one to send across the wire or something like that, then add a shared

memory

Quoted 4 lines

segment, and then pile on a bunch of code to do cross-process condition variables and state tracking. I really don't see how that's a better solution than adding a flag to the kernel API to just do what we want.

Quoted 3 lines

Way to complicated.

My thinking was rather to optionally allow a single page to be mmap()ed

into

Quoted 1 line

the process address space from the fd and then put a futex, pthread_cond

or

Quoted 1 line

X shared memory fence or anything like that into it.

Is that easier than just waiting in the kernel, I'm not sure how optimised we need this path to be.

[SNIP]

Is that easier than just waiting in the kernel, I'm not sure how optimised we need this path to be.

I don't think so. I think it's more-or-less the same code regardless of how it's done. The advantage of doing it in the kernel is that it's standardized (we don't have to both go write that userspace code) and it doesn't have the problems stated above.

Am 12.07.2017 um 17:53 schrieb Jason Ekstrand:

[SNIP]

Quoted 3 lines

Is that easier than just waiting in the kernel, I'm not sure how optimised we need this path to be.

I don't think so. I think it's more-or-less the same code regardless of how it's done. The advantage of doing it in the kernel is that it's standardized (we don't have to both go write that userspace code) and it doesn't have the problems stated above.

Ok, I'm convinced. The next price question is then how do we do it?

I mean writing an IOCTL to wait for a fence to appear is simple, but do we also need to wait for a fence to change?

Quoted 29 lines · Christian König

Am 10.07.2017 um 17:52 schrieb Jason Ekstrand:

Quoted 9 lines · Christian König

Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <[email protected]> wrote:
[SNIP]
So, reading some CTS tests again, and I think we have a problem here. The Vulkan spec allows you to wait on a fence that is in the unsignaled state.

Quoted 3 lines

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/vulkancts/
modules/vulkan/synchronization/vktSynchronizationBasicFenceTests.cpp#L74

Quoted 3 lines

You can't wait on a semaphore before the signal operation is send down to the kerel.

Quoted 7 lines

We (Intel) deal with this today by tracking whether or not the fence has been submitted and using a condition variable in userspace to sort it all out.

Which sounds exactly like what AMD is doing in it's drivers as well.

Which doesn't work cross-process so...

Quoted 7 lines

If we ever want to share fences across processes (which we do), then this needs to be sorted in the kernel.

That would clearly get a NAK from my side, even Microsoft forbids wait before signal because you can easily end up in deadlock situations.

Please don't NAK things that are required by the API specification and CTS tests. That makes it very hard for people like me to get their jobs done. :-)

Now, as for whether or not it's a good idea. First off, we do have timeouts an a status querying mechanism so an application can just set a timeout of 1s and do something if it times out. Second, if the application is a compositor or something else that doesn't trust its client, it shouldn't be using the OPAQUE_FD mechanism of Vulkan semaphore/fence sharing anyway. For those scenarios, they can require the untrusted client to use FENCE_FD (sync file) and they have all of the usual guarantees about when the work got submitted, etc.

Also, I'm more than happy to put this all behind a flag so it's not the default behavior.

Quoted 1 line · Jason Ekstrand

On Mon, Jul 10, 2017 at 9:15 AM, Christian König <

[email protected]>

Quoted 5 lines

Quoted 3 lines · wrote:

Am 10.07.2017 um 17:52 schrieb Jason Ekstrand:

On Mon, Jul 10, 2017 at 8:45 AM, Christian König <

[email protected]>

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines · wrote:

Am 10.07.2017 um 17:28 schrieb Jason Ekstrand:

On Wed, Jul 5, 2017 at 6:04 PM, Dave Airlie <[email protected]>

Quoted 3 lines · wrote:

Quoted 3 lines

Quoted 3 lines

[SNIP]
So, reading some CTS tests again, and I think we have a problem here. The Vulkan spec allows you to wait on a fence that is in the

unsignaled

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

state.

At least on the closed source driver that would be illegal as far as I know.

Then they are doing workarounds in userspace. There are definitely CTS tests for this:

https://github.com/KhronosGroup/VK-GL-CTS/blob/

master/external/vulkancts/

Quoted 1 line

Quoted 1 line

modules/vulkan/synchronization/vktSynchronizationBasicFenceTe

sts.cpp#L74

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

You can't wait on a semaphore before the signal operation is send

down to

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

the kerel.

We (Intel) deal with this today by tracking whether or not the fence

has

Quoted 1 line

Quoted 1 line

been submitted and using a condition variable in userspace to sort it

all

Quoted 9 lines

Quoted 5 lines

out.

Which sounds exactly like what AMD is doing in it's drivers as well.

Which doesn't work cross-process so...

Quoted 1 line

If we ever want to share fences across processes (which we do), then

this

Quoted 8 lines

Quoted 6 lines

needs to be sorted in the kernel.

That would clearly get a NAK from my side, even Microsoft forbids wait before signal because you can easily end up in deadlock situations.

Please don't NAK things that are required by the API specification and

CTS

Quoted 1 line

tests. That makes it very hard for people like me to get their jobs

done.

Quoted 5 lines

:-)

Now, as for whether or not it's a good idea. First off, we do have timeouts an a status querying mechanism so an application can just set a timeout of 1s and do something if it times out. Second, if the

application

Quoted 3 lines

is a compositor or something else that doesn't trust its client, it shouldn't be using the OPAQUE_FD mechanism of Vulkan semaphore/fence sharing anyway. For those scenarios, they can require the untrusted

client

Quoted 1 line

to use FENCE_FD (sync file) and they have all of the usual guarantees

about

Quoted 4 lines

when the work got submitted, etc.

Also, I'm more than happy to put this all behind a flag so it's not the default behavior.

Android had a similar requirement to have a fence fd before the fence existed in hwc1, before they fixed that in hwc2. But it's probably still useful for deeply pipelined renderes with littel memory, aka tiled renderers on phones.

The idea we've tossed around is to create a so-called future fence. In the kernel if you try to deref a future fence, the usual thing that happens is you'll block (interruptibly, which we can because fence lookup might fail), _until_ a real fence shows up and can be returned. That implements the uapi expectations without risking deadlocks in the kernel, albeit with a bit much blocking. Still better than doing the same in userspace (since in userspace you probably need to do that when importing the fence, not at execbuf time).