Add the VK_EXT_external_semaphore_drm_syncobj extension#2692
Conversation
|
Mesa implementation: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/40171 |
There was a problem hiding this comment.
Pull request overview
Adds the VK_EXT_external_semaphore_drm_syncobj Vulkan extension to enable import/export interop between Vulkan timeline semaphores and Linux DRM syncobj file descriptors, addressing issue #2473.
Changes:
- Registers the new extension and external semaphore handle type bit in
vk.xml. - Adds spec language/valid usage for the new handle type in the synchronization and capabilities chapters.
- Introduces the extension appendix page.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
xml/vk.xml |
Adds the new extension definition and extends VkExternalSemaphoreHandleTypeFlagBits with VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_DRM_SYNCOBJ_BIT_EXT. |
chapters/synchronization.adoc |
Adds handle type table entry and new Valid Usage statements tying the handle type to timeline semaphores. |
chapters/capabilities.adoc |
Documents the handle type as a Linux DRM syncobj FD and updates the handle-type compatibility table. |
appendices/VK_EXT_external_semaphore_drm_syncobj.adoc |
New appendix page for the extension metadata and interfaces. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cubanismo
left a comment
There was a problem hiding this comment.
Thanks for taking the time to move this extension forward @mahkoh! I've made a quick pass through and left some feedback, but this is looking pretty good. I'll ask the SI TSG to review as well. Please keep us updated on implementation progress.
Note this will also need CTS test coverage (https://github.com/KhronosGroup/VK-GL-CTS/tree/main/external/vulkancts) and validation layer coverage (https://github.com/KhronosGroup/Vulkan-ValidationLayers) to be merged. Let us know if you plan on working on those as well. Extending the existing timeline semaphore opaque FD coverage to run on DRM syncobj handle types would likely be sufficient for CTS.
| </require> | ||
| </extension> | ||
| <extension name="VK_EXT_extension_678" number="678" author="EXT" contact="Julian Orth @mahkoh" supported="disabled"> | ||
| <extension name="VK_EXT_external_semaphore_drm_syncobj" number="678" type="device" depends="VK_VERSION_1_2" author="EXT" contact="Julian Orth @mahkoh" supported="vulkan" nofeatures="true"> |
There was a problem hiding this comment.
This extension will have to have a feature bit. This is a newer requirement that most of the other external object specs predate. You can look at the VK_NV_external_memory_rdma spec for an example of a single-feature XML and spec update.
There was a problem hiding this comment.
This should be done now.
| endif::VK_NV_external_sci_sync[] | ||
| ifdef::VK_EXT_external_semaphore_drm_syncobj[] | ||
| * ename:VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_DRM_SYNCOBJ_BIT_EXT specifies a | ||
| file descriptor handle to a Linux DRM synchronization object (syncobj). |
There was a problem hiding this comment.
I think this extension specifically assumes DRM timeline synchronization objects, but binary syncobjs are a thing too, so the spec should probably be specific here and elsewhere. You may want to consider including that in the name as well, but it might also be considered overly verbose. I could go either way personally.
There was a problem hiding this comment.
I don't believe the kernel distinguishes between binary and timeline syncobjs. The ioctl to create a syncobj does not have a way to allocate one or the other:
struct drm_syncobj_create {
__u32 handle;
#define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
__u32 flags;
};The kernel warns against using both binary and timeline ioctls on the same syncobj but I believe the following sentence from this PR already covers this:
Vulkan timeline
semaphore values correspond to syncobj points.There was a problem hiding this comment.
Indeed, I can confirm that the kernel makes no difference between binary and timeline syncobjs. A binary syncobj is a timeline syncobj with only point 0 being used.
There was a problem hiding this comment.
I do think it's reasonable to have a Vulkan VU or at least a note that says that it should only be imported as the same timeline type it was exported as. That won't cover every case since the client can go call DRM ioctls itself, but if someone is, for instance, using this to share semaphores between two different Vulkan devices, they're going to run into trouble if they mix and match.
There was a problem hiding this comment.
This extension only supports import to/export from timeline semaphores.
fe71466 to
f5054cd
Compare
|
@gfxstrand cc to provide feedback |
They are linked above this comment now. |
|
@mahkoh While going through the release process in Khronos, it was pointed out this change is missing a proposal document, which is required for all new extensions. It doesn't have to be very long for a small extension like this, but please document the motivation behind the extension, any design alternatives considered, and any issues that came up during review (E.g., good to document why it makes sense to limit this to Vulkan timeline semaphores rather than adding it for binary semaphores as well). As an example of the format/layout, here's a link to the proposal document for VK_EXT_swapchain_maintenance1: https://docs.vulkan.org/features/latest/features/proposals/VK_EXT_swapchain_maintenance1.html |
|
I've added a proposal document. I haven't considered any alternatives so I've left out the solution space section. |
|
Heads-up: we have made a formatting change to vk.xml to crunch out whitespace in XML attribute lists, which will probably affect you the next time you sync this branch with current main branch. When you do this, try following the process in https://github.com/KhronosGroup/Vulkan-Docs/wiki/Merge-XML-Whitespace, which should prevent the need for any tedious conflict resolution consequent to the whitespace change. |
This extension enables interop between timeline semaphores and Linux DRM synchronization objects (syncobj).
|
@wusha01 The comments you've posted don't seem to be appearing anywhere. |
| :sectnums: | ||
|
|
||
| This extension aims to add interoperability between timeline semaphores and | ||
| syncobjs. |
There was a problem hiding this comment.
nitpicking:
maybe use the full name 'Linux DRM synchronization objects (syncobjs) " instead of the short form?
| This extension aims to add interoperability between timeline semaphores and | ||
| syncobjs. | ||
|
|
||
| == Problem Statement |
There was a problem hiding this comment.
Nitpicking:IMHO, the information in this session seems to fit better as a proposal instead of a problem statement. :) Particaluarly, the information about the syncobjs provides the justification why it is the chosen solution, while the information about 'OPAQUE_FD' seems like an alternative proposal, and the limitation comes along with it.
For the problem statement, it may be sufficient to explain what is the problem the extension is intended to solve. For example:
Vulkan does not define an external sync handle type that is suited for sharing timeline semaphore outside of Vulkan.
The existing external semaphore handle types for cross-API sharing are limited to binary semaphore usage. This limits Vulkan's ability to use timeline semaphore as sync primitive for cross API and process sharing.
There was a problem hiding this comment.
Vulkan does not define an external sync handle type that is suited for sharing timeline semaphore outside of Vulkan.
I don't think that is true. Vulkan does have such types on windows. And I think opaque FDs can be shared with cuda on the nvidia driver.
I wrote this section to show specifically why there is value in supporting syncobj interop. If it is already assumed that all interop with any external synchronization primitive is valuable, then I'd rather just cut this section down to the following without changing the proposal section.
Vulkan currently does not provide semaphore interoperability with syncobjs which are a synchronization primitive used on linux.
There was a problem hiding this comment.
Fair enough.
I like the trim down version much better :)
There was a problem hiding this comment.
I can do that if it is the consensus. If I were to read the proposal document, I would prefer to know more about why the extension was added and what things I could do with it.
There was a problem hiding this comment.
The reason I suggested an alternative problem statement is that the original problem statement reads like “Vulkan lacks a sync handle type for syncobj” and I slightly disagree with it.
I think the actual problem is that we lack a mechanism to share timeline semaphore payloads between Vulkan and other APIs on Linux. Opaque_FD could be used in a fairly limited set of use cases, while syncobj fits better to that problem, because it is a Linux kernel primitive with the same semantics as a Vulkan timeline semaphore, and is widely supported by the Linux ecosys. Therefore exporting/importing timeline semaphore payloads through drm syncobj handle becomes the choice on Linux.
There was a problem hiding this comment.
Thanks very much for writing the proposal document, by the way.
If we don’t receive others input on this, I’m fine with using the original problem statement.
There was a problem hiding this comment.
The reason I suggested an alternative problem statement is that the original problem statement reads like “Vulkan lacks a sync handle type for syncobj” and I slightly disagree with it.
That was how I intended it to be read. The two usecases from the problem statement are the usecases I wanted to solve with this extension.
It being the first well-defined handle type for timeline semaphores on linux is only a side effect and having any other such type would not solve the usecases I had in mind, since they specifically use syncobjs.
This extension enables interop between timeline semaphores and Linux DRM synchronization objects (syncobj).
Closes #2473