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

Vulkan examples regression: descriptor indexing and oit #2216

Closed
SRSaunders opened this issue Apr 25, 2024 · 11 comments
Closed

Vulkan examples regression: descriptor indexing and oit #2216

SRSaunders opened this issue Apr 25, 2024 · 11 comments
Labels
Bug Completed Issue has been fixed, or enhancement implemented.

Comments

@SRSaunders
Copy link
Contributor

SRSaunders commented Apr 25, 2024

I have been working on updating macOS and iOS support and fixing a few defects that I found in Sascha Willems' Vulkan examples repository. See SaschaWillems/Vulkan#1117 and SaschaWillems/Vulkan#1119. During this work I observed two examples that expose what appear to regressions across MoltenVK releases:

• The descriptorindexing example runs well on Vulkan SDK 1.3.250.1/MoltenVK 1.2.4, but fails on any later release with:

[mvk-error] SPIR-V to MSL conversion error: Argument buffer resource base type could not be determined. When padding argument buffer elements, all descriptor set resources must be supplied with a base type by the app.
[mvk-error] VK_ERROR_INVALID_SHADER_NV: Fragment shader function could not be compiled into pipeline. See previous logged error.
Fatal : VkResult is "ERROR_INVALID_SHADER_NV" in /Users/steve/XcodeProjects/Vulkan/examples/descriptorindexing/descriptorindexing.cpp at line 346
Assertion failed: (res == VK_SUCCESS), function preparePipelines, file descriptorindexing.cpp, line 346.

• The oit example runs well up to SDK 1.3.275.0/MoltenVK 1.2.7, but fails on the current release (1.3.280.0/1):

-[MTLTextureDescriptorInternal validateWithDevice:]:1344: failed assertion `Texture Descriptor Validation
MTLTextureUsage has unknown bits 0x20.

The first of these may be a duplicate of #2180, but I just wanted to report them here to make sure the MoltenVK team is aware.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented May 5, 2024

After a bit of debugging, I believe the first one is a SPIRVCross defect. I patched in the old SPIRVCross (from MoltenVK 1.2.4) at commit 12542fc to the most recent head of MoltenVK 1.2.9. I had to comment out a few newer msl options, but this did not seem to affect things for the descriptorindexing example. I now have a working example with this mashup. Note the newest SPRIVCross for SDK 1.3.283/MVK 1.2.9 does not fix it.

I have not done any research on the second issue above (oit example). Using the older SPIRVCross does not fix it.
Screenshot 2024-05-05 at 11 22 08 AM

@SRSaunders
Copy link
Contributor Author

Issue one (descriptorindexing) reported with additional data at the existing KhronosGroup/SPIRV-Cross#2198

Issue two (oit) still not understood.

@cdavis5e
Copy link
Collaborator

cdavis5e commented May 5, 2024

The oit example runs well up to SDK 1.3.275.0/MoltenVK 1.2.7, but fails on the current release (1.3.280.0/1):

-[MTLTextureDescriptorInternal validateWithDevice:]:1344: failed assertion `Texture Descriptor Validation
MTLTextureUsage has unknown bits 0x20.

What OS? More specifically, what version?

@SRSaunders
Copy link
Contributor Author

OS is Ventura 13.6.6 (on x86) with Xcode 15.2. GPU is AMD Radeon 6600 (Metal Tier 2).

@cdavis5e
Copy link
Collaborator

cdavis5e commented May 6, 2024

OS is Ventura 13.6.6 (on x86) with Xcode 15.2. GPU is AMD Radeon 6600 (Metal Tier 2).

The bit 0x20 is MTLTextureUsageShaderAtomic, which wasn't introduced until Sonoma. We shouldn't be using that bit prior to Sonoma.

@billhollings
Copy link
Contributor

The descriptorindexing example runs well on Vulkan SDK 1.3.250.1/MoltenVK 1.2.4, but fails on any later release... /clip/ ...The first of these may be a duplicate of #2180, but I just wanted to report them here to make sure the MoltenVK team is aware.

Thanks for submitting this as an issue affecting a known sample app! It made it much easier to track down!

PR #2231 fixes this part of this issue.

@billhollings billhollings added the Completed Issue has been fixed, or enhancement implemented. label May 6, 2024
@billhollings
Copy link
Contributor

OS is Ventura 13.6.6 (on x86) with Xcode 15.2. GPU is AMD Radeon 6600 (Metal Tier 2).

The bit 0x20 is MTLTextureUsageShaderAtomic, which wasn't introduced until Sonoma. We shouldn't be using that bit prior to Sonoma.

@SRSaunders Can you try changing:

MTLTextureUsage mtlUsage = pixFmts->getMTLTextureUsage(getCombinedUsage(), mtlPixFmt, _samples, _isLinear || _isLinearForAtomics, needsReinterpretation, _hasExtendedUsage, _shouldSupportAtomics);

to include , _shouldSupportAtomics && getPhysicalDevice()->useNativeTextureAtomics()); (and maybe put a couple of line-returns as needed to make the line not so far off to the right), and then retest, please?

@SRSaunders
Copy link
Contributor Author

Fantastic! Both issues are solved for me now.

For issue one (arg buffer padding), I checked out KhronosGroup/SPIRV-Cross@e41d79f and rebuilt the External Dependencies project, followed by the MoltenVK project. The descriptorindexing example is now working fine.

For issue two (MTLTextureUsageShaderAtomic), I added your additional code and rebuilt MoltenVK. The oit example is now working again.

Thanks for jumping on these and solving so quickly.

@billhollings
Copy link
Contributor

billhollings commented May 6, 2024

For issue two (MTLTextureUsageShaderAtomic), I added your additional code and rebuilt MoltenVK. The oit example is now working again.

Wonderful! Thanks for testing.

I'll make that simple change here and run CTS to see if it breaks anything, then I'll issue a PR.

@billhollings
Copy link
Contributor

billhollings commented May 7, 2024

For issue two (MTLTextureUsageShaderAtomic), I added your additional code and rebuilt MoltenVK. The oit example is now working again.

Wonderful! Thanks for testing.

I'll make that simple change here and run CTS to see if it breaks anything, then I'll issue a PR.

The updated PR #2231 now also fixes this part of this issue.

@SRSaunders
Copy link
Contributor Author

I have now tested against the new merged PR and the Vulkan examples work well. Thanks again.

Closing issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Completed Issue has been fixed, or enhancement implemented.
Projects
None yet
Development

No branches or pull requests

3 participants