Add VK_EXT_external_memory_metal #2414

aitor-lunarg · 2024-08-20T07:03:53Z

No description provided.

oddhack · 2024-08-21T07:32:40Z

@aitor-lunarg process-wise, we require that extension number and flag bits be reserved by a commit to main prior to the extension development branch actually being able to use them. I don't know where this extension stands deployment / release-wise, but we need to separate out the reservations and accept those first, to avoid a race condition (see extension 597 for a guide to what that PR would look like).

aitor-lunarg · 2024-10-08T03:57:25Z

Before moving forward with this extension, there's one concern that we need to address. Metal does not provide all Vulkan formats, so there are times where an implementation will emulate some of them. Why is this an issue:

Implementations may decide to emulate those formats with an internal representation such that those formats are backed by multiple textures. Taking MoltenVK as an example implementation with VK_FORMAT_G8_B8_R8_3PLANE_420_UNORM. This format is backed by 3 textures (one for each plane), so it is not possible to export all of the backing textures through a single handle unfortunately as of now.

There are 3 potential solutions that come to mind to fix this issue as we stand:

Force implementation to have those multi-planar emulated implementations in a single texture if they want to export it. Unsure if there could be issues with precision when sampling that would prevent this approach.
Modify the API for this extension to allow exporting more than one single handle for these cases. This would lead to a query call first to understand how many handles the user will get, and then get the handles. However, this is quite counter intuitive since we imply that a VkDeviceMemory object may not be a single handle. When importing we would also need to provide all handles as the implementation expects them. I don't really like this approach.
Modify the handle to MTLHeap and force implementations to have all resources stored in those objects if they want to export them. Again, this also has issues since not all Apple hardware may support MTLHeap and there are a few known restrictions that make it ugly for non-Apple silicon GPUs.

@linyaa-kiwi would it be possible to bring this up for discussion this coming SI meeting? I would like to understand if other similar extensions have run into similar issues and how they've tackled them if so. Or just get general feedback/ideas about this issue.

cc/ @kocdemir

linyaa-kiwi · 2024-10-16T13:07:14Z

@linyaa-kiwi would it be possible to bring this up for discussion this coming SI meeting? I would like to understand if other similar extensions have run into similar issues and how they've tackled them if so. Or just get general feedback/ideas about this issue.

@aitor-lunarg Would you like to discuss in today's SI meeting? If not, what week works best for you?

proposals/VK_EXT_external_memory_metal.adoc

chapters/memory.adoc

chapters/VK_EXT_external_memory_metal/device_memory.adoc

chapters/capabilities.adoc

aitor-lunarg · 2024-12-04T19:03:44Z

~~Seems like GitHub is having some issues, would it be possible to retrigger the failed checks (failed due to a 500 error when trying to clone required repos)?~~ Nvm, I saw I can do that myself

linyaa-kiwi

I found some more points for improvement today.

linyaa-kiwi · 2024-12-09T22:28:19Z

appendices/VK_EXT_external_memory_metal.adoc

+=== Description
+
+An application may wish to reference device memory in multiple Vulkan device instances, in multiple processes, and/or in Metal API.
+This extension enables an application to export non-Vulkan handles from Vulkan memory objects such that the underlying resources can


s/non-Vulkan handles/Metal handles/

The text says "export", but the extension supports import too.

Let me know if the new wording works!

So close! "Import" is misspelled as "impor". Other than the typo, the text looks good.

chapters/capabilities.adoc

chapters/memory.adoc

linyaa-kiwi · 2024-12-09T22:56:50Z

proposals/VK_EXT_external_memory_metal.adoc

+implementations will be required to report `VK_EXTERNAL_MEMORY_FEATURE_DEDICATED_ONLY_BIT` for
+`VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT`. When importing a MTLTexture through the creation of a
+link:{refpage}VkDeviceMemory.html[VkDeviceMemory], the link:{refpage}VkImage.html[VkImage] it will be bound to
+must have the same creation parameters as the MTLTexture used at import.


"must have same creation parameters as the MTLTexture used at import"

This needs a VU. A good example is VUID-VkMemoryAllocateInfo-pNext-02388, which is about AHardwareBuffer. There are more VUs in that neighborhood that are worth considering too. The new VU (new VUs) should enumerate which fields of VkImageCreateInfo must match the MTLTexture's creation params.

Added a new VU to handle this case. I am thinking that maybe we could add a few more to force the users to provide the right size at allocation when importing buffers or heaps, but it kind of seems slightly pointless. Right now, we just say we don't really care about the size for buffers and heaps since we will use Metal to know the size of said resources. Any way, let me know your thoughts!

I don't have a strong opinion on whether the spec should (a) require size to be correct or (b) ignore size. That's a decision is yours. In my opinion, the only practical difference is that choice (b) inhibits some VVL checks.

Regardless of the choice of (a) or (b) though for MTLBuffer and MTLHeap, more spec text is needed. If (a), then a VU is needed (similar to VUID-VkMemoryAllocateInfo-allocationSize-00647 or VUID-VkMemoryAllocateInfo-allocationSize-02383). If (b), then text similar to the MTLTexture "ignore" text is needed.

However, if you do choose (b), I prefer that the value not be actually ignored. In the past two years, several members of the Vulkan WG have become frustrated with the unintended consequences caused by ignored values in the spec. Superficially, it seems that an ignored allocationSize would cause no problems. But sometimes it can cause surprising difficulties for tools and layers. Instead of saying that allocationSize is ignored, I think it's better to add a VU that requires it to be 0.

I went (b), added a VUID to enforce allocationSize to be 0 when importing MTLTextures so there's no issue in the future. Let me know what you think!

linyaa-kiwi · 2024-12-19T18:18:36Z

chapters/VK_EXT_external_memory_metal/device_memory.adoc

+  * pname:handleType must: have been included in
+    slink:VkExportMemoryAllocateInfo::pname:handleTypes when pname:memory
+    was created
+  * pname:handleType it must: be


s/pname:handleType it must: be/pname:handleType must: be/

linyaa-kiwi · 2024-12-19T18:19:12Z

chapters/VK_EXT_external_memory_metal/device_memory.adoc

+****
+  * pname:handle must: point to a valid id<MTLBuffer>, id<MTLTexture> or
+    id<MTLDevice>
+  * pname:handleType it must: be 


s/pname:handleType it must: be/pname:handleType must: be/

linyaa-kiwi · 2024-12-19T18:27:11Z

chapters/memory.adoc

+ifdef::VK_EXT_external_memory_metal[]
+  * If the parameters define an import operation and the external handle is
+    a ename:VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT, then
+    pname:pNext must: include a slink:VkMemoryDedicatedAllocateInfo.


This VU needs a little more text.

[...] must: include a slink:VkMemoryDedicatedAllocateInfo structure with pname:image not equal to dlink:VK_NULL_HANDLE.

Added some more context. Let me know if that is clear enough or if a different wording is needed!

linyaa-kiwi · 2024-12-19T18:39:46Z

proposals/VK_EXT_external_memory_metal.adoc

+implementations will be required to report `VK_EXTERNAL_MEMORY_FEATURE_DEDICATED_ONLY_BIT` for
+`VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT`. When importing a MTLTexture through the creation of a
+link:{refpage}VkDeviceMemory.html[VkDeviceMemory], the link:{refpage}VkImage.html[VkImage] it will be bound to
+must have the same creation parameters as the MTLTexture used at import.


I don't have a strong opinion on whether the spec should (a) require size to be correct or (b) ignore size. That's a decision is yours. In my opinion, the only practical difference is that choice (b) inhibits some VVL checks.

Regardless of the choice of (a) or (b) though for MTLBuffer and MTLHeap, more spec text is needed. If (a), then a VU is needed (similar to VUID-VkMemoryAllocateInfo-allocationSize-00647 or VUID-VkMemoryAllocateInfo-allocationSize-02383). If (b), then text similar to the MTLTexture "ignore" text is needed.

However, if you do choose (b), I prefer that the value not be actually ignored. In the past two years, several members of the Vulkan WG have become frustrated with the unintended consequences caused by ignored values in the spec. Superficially, it seems that an ignored allocationSize would cause no problems. But sometimes it can cause surprising difficulties for tools and layers. Instead of saying that allocationSize is ignored, I think it's better to add a VU that requires it to be 0.

linyaa-kiwi · 2025-01-08T16:37:54Z

Thanks. I'll give it another review today.

aitor-lunarg force-pushed the VK_EXT_external_memory_metal branch from a1ac4fa to 9d16d7e Compare August 20, 2024 08:32

oddhack mentioned this pull request Aug 21, 2024

Add VK_EXT_disable_wayland_color_management #2410

Open

aitor-lunarg mentioned this pull request Aug 21, 2024

Reserve extension 603 #2416

Merged

linyaa-kiwi added the System Integration label Aug 21, 2024

oddhack assigned linyaa-kiwi Aug 21, 2024

kocdemir reviewed Oct 17, 2024

View reviewed changes

proposals/VK_EXT_external_memory_metal.adoc Outdated Show resolved Hide resolved

kocdemir reviewed Oct 17, 2024

View reviewed changes

chapters/memory.adoc Outdated Show resolved Hide resolved

linyaa-kiwi reviewed Nov 12, 2024

View reviewed changes

chapters/VK_EXT_external_memory_metal/device_memory.adoc Outdated Show resolved Hide resolved

chapters/capabilities.adoc Show resolved Hide resolved

aitor-lunarg added 4 commits December 5, 2024 03:29

Add VK_EXT_external_memory_metal

b759c5c

Add missing VUIDs for VK_EXT_external_memory_metal

513876c

Add MTLHeap as a possible handle to export/import

a56da35

Reword lifespan segment, merge VUID and fix typos

44958d2

aitor-lunarg force-pushed the VK_EXT_external_memory_metal branch from c3095fa to 44958d2 Compare December 4, 2024 18:31

aitor-lunarg added 2 commits December 5, 2024 03:34

VK_EXT_external_memory_metal fix build due to rebase issue

8f65937

VK_EXT_external_memory_metal has no features

bfd9ee2

VK_EXT_external_memory_metal add missing const to param

5eabdbb

aitor-lunarg force-pushed the VK_EXT_external_memory_metal branch from bf0a84a to 5eabdbb Compare December 4, 2024 21:41

VK_EXT_external_memory_metal fix parameter name

e120acf

linyaa-kiwi reviewed Dec 9, 2024

View reviewed changes

VK_EXT_external_memory_metal address MTLHeap and VUID feedback

db6fa18

linyaa-kiwi reviewed Dec 19, 2024

View reviewed changes

VK_EXT_external_memory_metal typo fixes and allocationSize VUID

7deddcd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VK_EXT_external_memory_metal #2414

Add VK_EXT_external_memory_metal #2414

aitor-lunarg commented Aug 20, 2024

oddhack commented Aug 21, 2024 •

edited

Loading

aitor-lunarg commented Oct 8, 2024

linyaa-kiwi commented Oct 16, 2024

aitor-lunarg commented Dec 4, 2024 •

edited

Loading

linyaa-kiwi left a comment

linyaa-kiwi Dec 9, 2024

linyaa-kiwi Dec 9, 2024

aitor-lunarg Dec 13, 2024

linyaa-kiwi Dec 17, 2024

linyaa-kiwi Dec 9, 2024

aitor-lunarg Dec 13, 2024

linyaa-kiwi Dec 19, 2024

aitor-lunarg Jan 7, 2025

linyaa-kiwi Dec 19, 2024

aitor-lunarg Jan 7, 2025

linyaa-kiwi Dec 19, 2024

aitor-lunarg Jan 7, 2025

linyaa-kiwi Dec 19, 2024

aitor-lunarg Jan 7, 2025

linyaa-kiwi Dec 19, 2024

linyaa-kiwi commented Jan 8, 2025

Add VK_EXT_external_memory_metal #2414

Are you sure you want to change the base?

Add VK_EXT_external_memory_metal #2414

Conversation

aitor-lunarg commented Aug 20, 2024

oddhack commented Aug 21, 2024 • edited Loading

aitor-lunarg commented Oct 8, 2024

linyaa-kiwi commented Oct 16, 2024

aitor-lunarg commented Dec 4, 2024 • edited Loading

linyaa-kiwi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linyaa-kiwi commented Jan 8, 2025

oddhack commented Aug 21, 2024 •

edited

Loading

aitor-lunarg commented Dec 4, 2024 •

edited

Loading