Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add VK_EXT_external_memory_metal #2414

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aitor-lunarg
Copy link
Contributor

No description provided.

@oddhack
Copy link
Contributor

oddhack commented Aug 21, 2024

@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
Copy link
Contributor Author

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:

  1. 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.
  2. 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.
  3. 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
Copy link
Contributor

@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?

chapters/memory.adoc Outdated Show resolved Hide resolved
the device memory object.

The lifespan of the returned handle object is the same as the associated `VkDeviceMemory` object.
The app must: retain the handle object if it intends to use it beyond that lifespan.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that the lifetime language is incorrect, unless I misunderstand the meaning of "retain". The text says that the lifespan of the Metal handle is the same as the VkDeviceMemory; therefore, destruction of the VkDeviceMemory also invalidates the Metal handle. However, the text's sentence suggests that the lifetime can be extended by "retaining" the handle; this sentence contradicts the first sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metal objects are reference counted and only destroyed when the count reaches 0. This part of the spec is trying to state that unless the user calls retain (which increases the count) on the returned Metal object, the lifespan will be that of the VkDeviceMemory. That is why beyond that lifespan is used.

I will try to think of a better way to word this part since I see it can lead to confusion. If you have any suggestion on how to word it, do let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if it's clearer now!

| ename:VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLBUFFER_BIT_EXT | Must match | Must match
| ename:VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT | Must match | Must match
| ename:VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLHEAP_BIT_EXT | Must match | Must match
endif::VK_EXT_external_memory_metal[]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about the choice of "Must match" in this table. I suspect that the constraint is impossible to satisfy in the extension's intended use cases.

When sharing a Metal handle with a different VkDevice in a different VkInstance, the "must match" constraint is easy to satisfy. That's not my concern.

I assume that an intended use case for the extension is to share Metal handles outside of Vulkan. (Please correct me if I'm wrong). In that case, do the relevant sharing apis expose the equivalent of driverUUID, and does that UUID match the Vulkan driver's driverUUID?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that an intended use case for the extension is to share Metal handles outside of Vulkan.

For the Android Emulator, sharing outside of the Vulkan is not a priority, as we won't be sharing the handle with OpenGL on Mac, and we share the buffers and images only between different Vulkan instances and devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe changing driverUUID here to No restriction should be the way forward since Metal does not have an equivalent. The only requirement should be that the deviceUUID matches, and Metal does provide an equivalent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants