-
Notifications
You must be signed in to change notification settings - Fork 11
Fix macOS support #74
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
base: main
Are you sure you want to change the base?
Conversation
- Untangle GL dependenices - Add dummy files to prevent linker errors on empty archives - Add missing macOS framework dependencies
On macOS MoltenVK can be installed as part of Vulkan SDK or as standalone. If standalone, VK_KHR_PORTABILITY_ENUMERATION_EXTENSION_NAME will not be supported. Therefore, we decouple the detection of portability enumeration from detection of MoltenVK.
Just like QNX, macOS does not natively support DMA buffers. Also indicate that VK_EXT_QUEUE_FAMILY_FOREIGN_EXTENSION_NAME is emulated in `vkEnumerateDeviceExtensionProperties`.
MoltenVK enforces that a single VkImage can be dedicated to a memory binding. When this code runs it breaks that promise and results in an error return when `vkBindImageMemory` is called by the client.
|
Thanks for the change, looks like this removes a workaround we've used for some system images (in commit 'Remove replacement dedicated allocation for macOS') and I'll need to test it thoroughly on that side to ensure everything is ok (or suggest a conditional removal there). Feel free to provide different PRs for the other changes, so they can be reviewed and merged earlier. |
|
You may want to add a Github action for the configuration of the meson build on MacOS you're using, to ensure there are no future breakages. |
kocdemir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested the Remove replacement dedicated allocation for macOS change from this PR, and it only works fine with the newer versions of MoltenVK, so probably some bugs are now fixed and we can revert the related workarounds (we also have changes inside moltenvk, that's why vanilla versions won't work). Unfortunately, that change needs to go in at the same time with our moltenvk binary distribution with the emulator, so I'll do those changes in our internal branch separately and they will be merged here afterwards.
For the rest of the changes I've put some comments.
| #else | ||
| return mColorBufferVk != nullptr; | ||
| #endif | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo ';'
I think this function is not being used, please remove.
|
|
||
| WorkerProcessingResult FrameBuffer::Impl::sendReadbackWorkerCmd(const Readback& readback) { | ||
| ensureReadbackWorker(); | ||
| #if GFXSTREAM_ENABLE_HOST_GLES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code block is not directly using GL, so instead of disabling the functionality here I think we need to implement a vulkan version of the readback worker class, or a dummy one that throws errors for now, to make sure ensureReadbackWorker assigns m_readbackWorker to something and we get proper error messages for unimplemented features.
| #endif | ||
|
|
||
| // VK_EXT_QUEUE_FAMILY_FOREIGN_EXTENSION_NAME is emulated by gfxstream | ||
| if (!hasDeviceExtension(properties, VK_EXT_QUEUE_FAMILY_FOREIGN_EXTENSION_NAME)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ideally requires checking in shouldPassthrough, as otherwise it will directly return vkEnumerateDeviceExtensionProperties results for non-moltenvk cases.
On the other hand, VK_EXT_QUEUE_FAMILY_FOREIGN_EXTENSION_NAME is already in kEmulatedDeviceExtensions, so this requires a change on the guest side.
Looking at the guest side code, looks like currently it's only enabled for android guests:
if (hostHasExternalMemorySupport) {
#ifdef VK_USE_PLATFORM_ANDROID_KHR
filteredExts.push_back(
VkExtensionProperties{"VK_ANDROID_external_memory_android_hardware_buffer", 7});
filteredExts.push_back(VkExtensionProperties{"VK_EXT_queue_family_foreign", 1});
#endif
#ifdef VK_USE_PLATFORM_FUCHSIA
filteredExts.push_back(VkExtensionProperties{"VK_FUCHSIA_external_memory", 1});
filteredExts.push_back(VkExtensionProperties{"VK_FUCHSIA_buffer_collection", 1});
#endif
} else {
...
So, this change needs to be made on mesa3d side instead to include linux guests.
This fixes compile on macOS and gets basic rendering working through Mesa + QEMU + rutabaga. A couple of thoughts:
VK_EXT_QUEUE_FAMILY_FOREIGN_EXTENSION_NAMEemulation is done in Mesa but the guest fails to interceptvkEnumerateDeviceExtensionPropertiesto indicate the support. The change adds this to the host side because it's easier to change than the guest (which requires updating Mesa + downstream distros). However, this may introduce compatibility issues with older guests which does not emulate the extension. No matter which side gets updated, it introduces compatibility issues to the other side since there's no good way to indicate (from guest to host) that something is being emulated.