Add the VK_EXT_external_semaphore_drm_syncobj extension#2692
Add the VK_EXT_external_semaphore_drm_syncobj extension#2692mahkoh wants to merge 1 commit intoKhronosGroup:mainfrom
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. |
This extension enables interop between timeline semaphores and Linux DRM synchronization objects (syncobj).
This extension enables interop between timeline semaphores and Linux DRM synchronization objects (syncobj).
Closes #2473