<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Aug 9, 2019 at 8:43 AM Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com">lionel.g.landwerlin@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The Vulkan timeline semaphores allow signaling to happen on the point<br>
of the timeline without all of the its dependencies to be created.<br>
<br>
The current 2 implementations (AMD/Intel) of the Vulkan spec on top of<br>
the Linux kernel are using a thread to wait on the dependencies of a<br>
given point to materialize and delay actual submission to the kernel<br>
driver until the wait completes.<br>
<br>
If a binary semaphore is submitted for signaling along the side of a<br>
timeline semaphore waiting for completion that means that the drm<br>
syncobj associated with that binary semaphore will not have a DMA<br>
fence associated with it by the time vkQueueSubmit() returns. This and<br>
the fact that a binary semaphore can be signaled and unsignaled as<br>
before its DMA fences materialize mean that we cannot just rely on the<br>
fence within the syncobj but we also need a sideband payload verifying<br>
that the fence in the syncobj matches the last submission from the<br>
Vulkan API point of view.<br>
<br>
This change adds a sideband payload that is incremented with signaled<br>
syncobj when vkQueueSubmit() is called. The next vkQueueSubmit()<br>
waiting on a the syncobj will read the sideband payload and wait for a<br>
fence chain element with a seqno superior or equal to the sideband<br>
payload value to be added into the fence chain and use that fence to<br>
trigger the submission on the kernel driver.<br>
<br>
v2: Use a separate ioctl to get/set the sideband value (Christian)<br>
<br>
v3: Use 2 ioctls for get/set (Christian)<br>
<br>
v4: Use a single new ioctl<br>
<br>
v5: a bunch of blattant mistakes<br>
    Store payload atomically (Chris)<br>
<br>
Signed-off-by: Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com" target="_blank">lionel.g.landwerlin@intel.com</a>><br>
Cc: Christian Koenig <<a href="mailto:Christian.Koenig@amd.com" target="_blank">Christian.Koenig@amd.com</a>><br>
Cc: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>><br>
Cc: David(ChunMing) Zhou <<a href="mailto:David1.Zhou@amd.com" target="_blank">David1.Zhou@amd.com</a>><br>
---<br>
 drivers/gpu/drm/drm_internal.h |  2 ++<br>
 drivers/gpu/drm/drm_ioctl.c    |  3 ++<br>
 drivers/gpu/drm/drm_syncobj.c  | 58 +++++++++++++++++++++++++++++++++-<br>
 include/drm/drm_syncobj.h      |  9 ++++++<br>
 include/uapi/drm/drm.h         | 17 ++++++++++<br>
 5 files changed, 88 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h<br>
index 51a2055c8f18..e297dfd85019 100644<br>
--- a/drivers/gpu/drm/drm_internal.h<br>
+++ b/drivers/gpu/drm/drm_internal.h<br>
@@ -208,6 +208,8 @@ int drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,<br>
                                      struct drm_file *file_private);<br>
 int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,<br>
                            struct drm_file *file_private);<br>
+int drm_syncobj_binary_ioctl(struct drm_device *dev, void *data,<br>
+                            struct drm_file *file_private);<br>
<br>
 /* drm_framebuffer.c */<br>
 void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,<br>
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c<br>
index f675a3bb2c88..644d0bc800a4 100644<br>
--- a/drivers/gpu/drm/drm_ioctl.c<br>
+++ b/drivers/gpu/drm/drm_ioctl.c<br>
@@ -703,6 +703,9 @@ static const struct drm_ioctl_desc drm_ioctls[] = {<br>
                      DRM_RENDER_ALLOW),<br>
        DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,<br>
                      DRM_RENDER_ALLOW),<br>
+       DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_BINARY, drm_syncobj_binary_ioctl,<br>
+                     DRM_RENDER_ALLOW),<br>
+<br>
        DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 0),<br>
        DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, 0),<br>
        DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER),<br>
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c<br>
index b927e482e554..d2d3a8d1374d 100644<br>
--- a/drivers/gpu/drm/drm_syncobj.c<br>
+++ b/drivers/gpu/drm/drm_syncobj.c<br>
@@ -1150,8 +1150,10 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data,<br>
        if (ret < 0)<br>
                return ret;<br>
<br>
-       for (i = 0; i < args->count_handles; i++)<br>
+       for (i = 0; i < args->count_handles; i++) {<br>
                drm_syncobj_replace_fence(syncobjs[i], NULL);<br>
+               atomic64_set(&syncobjs[i]->binary_payload, 0);<br>
+       }<br>
<br>
        drm_syncobj_array_free(syncobjs, args->count_handles);<br>
<br>
@@ -1321,6 +1323,60 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,<br>
                if (ret)<br>
                        break;<br>
        }<br>
+<br>
+       drm_syncobj_array_free(syncobjs, args->count_handles);<br>
+<br>
+       return ret;<br>
+}<br>
+<br>
+int drm_syncobj_binary_ioctl(struct drm_device *dev, void *data,<br>
+                            struct drm_file *file_private)<br>
+{<br>
+       struct drm_syncobj_binary_array *args = data;<br>
+       struct drm_syncobj **syncobjs;<br>
+       u32 __user *access_flags = u64_to_user_ptr(args->access_flags);<br>
+       u64 __user *values = u64_to_user_ptr(args->values);<br>
+       u32 i;<br>
+       int ret;<br>
+<br>
+       if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))<br>
+               return -EOPNOTSUPP;<br>
+<br>
+       if (args->pad != 0)<br>
+               return -EINVAL;<br>
+<br>
+       if (args->count_handles == 0)<br>
+               return -EINVAL;<br>
+<br>
+       ret = drm_syncobj_array_find(file_private,<br>
+                                    u64_to_user_ptr(args->handles),<br>
+                                    args->count_handles,<br>
+                                    &syncobjs);<br>
+       if (ret < 0)<br>
+               return ret;<br>
+<br>
+       for (i = 0; i < args->count_handles; i++) {<br>
+               u32 flags;<br>
+<br>
+               if (get_user(flags, &access_flags[i])) {<br>
+                       ret = -EFAULT;<br>
+                       break;<br>
+               }<br>
+<br>
+<br>
+               if (flags & DRM_SYNCOBJ_BINARY_VALUE_READ) {<br>
+                       u64 value = atomic64_read(&syncobjs[i]->binary_payload);<br>
+<br>
+                       if (put_user(value, &values[i])) {<br>
+                               ret = -EFAULT;<br>
+                               break;<br>
+                       }<br>
+               }<br>
+<br>
+               if (flags & DRM_SYNCOBJ_BINARY_VALUE_INC)<br>
+                       atomic64_inc(&syncobjs[i]->binary_payload);<br></blockquote><div><br></div><div>You go out of your way to use atomics but then don't bother to do the read and increment atomically.  Maybe something like</div><div><br></div><div>u64 value = 0;<br></div><div>if (flags & DRM_SYNCOBJ_BINARY_VALUE_INC)</div><div>   value = atomic64_inc(...);</div><div>else if (flags & DRM_SYNCOBJ_BINARY_VALUE_READ)</div><div>   value = atomic64_read(...);</div><div><br></div><div>if (flags & DRM_SYNCOBJ_BINARY_VALUE_READ)</div><div>   put_user(...)<br></div><div><br></div><div>--Jason</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+       }<br>
+<br>
        drm_syncobj_array_free(syncobjs, args->count_handles);<br>
<br>
        return ret;<br>
diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h<br>
index 6cf7243a1dc5..aa76cb3f9107 100644<br>
--- a/include/drm/drm_syncobj.h<br>
+++ b/include/drm/drm_syncobj.h<br>
@@ -61,6 +61,15 @@ struct drm_syncobj {<br>
         * @file: A file backing for this syncobj.<br>
         */<br>
        struct file *file;<br>
+       /**<br>
+        * @binary_payload: A 64bit payload for binary syncobjs.<br>
+        *<br>
+        * We use the payload value to wait on binary syncobj fences to<br>
+        * materialize. It is a reservation mechanism for the signaler to<br>
+        * express that at some point in the future a dma fence with the same<br>
+        * seqno will be put into the syncobj.<br>
+        */<br>
+       atomic64_t binary_payload;<br>
 };<br>
<br>
 void drm_syncobj_free(struct kref *kref);<br>
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h<br>
index 8a5b2f8f8eb9..78a0a413b788 100644<br>
--- a/include/uapi/drm/drm.h<br>
+++ b/include/uapi/drm/drm.h<br>
@@ -785,6 +785,22 @@ struct drm_syncobj_timeline_array {<br>
        __u32 pad;<br>
 };<br>
<br>
+struct drm_syncobj_binary_array {<br>
+       /* A pointer to an array of u32 syncobj handles. */<br>
+       __u64 handles;<br>
+       /* A pointer to an array of u32 access flags for each handle. */<br>
+       __u64 access_flags;<br>
+       /* The binary value of a syncobj is read before it is incremented. */<br>
+#define DRM_SYNCOBJ_BINARY_VALUE_READ (1u << 0)<br>
+#define DRM_SYNCOBJ_BINARY_VALUE_INC  (1u << 1)<br>
+       /* A pointer to an array of u64 values written to by the kernel if the<br>
+        * handle is flagged for reading.<br>
+        */<br>
+       __u64 values;<br>
+       /* The length of the 3 arrays above. */<br>
+       __u32 count_handles;<br>
+       __u32 pad;<br>
+};<br>
<br>
 /* Query current scanout sequence number */<br>
 struct drm_crtc_get_sequence {<br>
@@ -946,6 +962,7 @@ extern "C" {<br>
 #define DRM_IOCTL_SYNCOBJ_QUERY                DRM_IOWR(0xCB, struct drm_syncobj_timeline_array)<br>
 #define DRM_IOCTL_SYNCOBJ_TRANSFER     DRM_IOWR(0xCC, struct drm_syncobj_transfer)<br>
 #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL      DRM_IOWR(0xCD, struct drm_syncobj_timeline_array)<br>
+#define DRM_IOCTL_SYNCOBJ_BINARY       DRM_IOWR(0xCE, struct drm_syncobj_binary_array)<br>
<br>
 /**<br>
  * Device specific ioctls should only be in their respective headers<br>
-- <br>
2.23.0.rc1<br>
<br>
</blockquote></div></div>