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

Fix shader conversion failure when using storage images #2335

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Docs/Whats_New.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,16 @@ Released 2024-09-24
- Update `VkFormat` capabilities based on latest Metal docs.
- Ensure all MoltenVK config info set by `VK_EXT_layer_settings` is used.
- Support storage images in Metal argument buffers on _iOS_.
- `vkUpdateDescriptorSets()`: Support writing beyond descriptor binding size if subsequent bindings are of same type.
- Fix rendering issue with render pass that immediately follows a kernel dispatch.
- Fix race condition when `VkImage` destroyed while used by descriptor.
- Fix crash in `vkCmdPushDescriptorSetWithTemplateKHR()` when entries in
`VkDescriptorUpdateTemplateCreateInfo` are not sorted by offset.
- Fix issue where `vkQueueWaitIdle()` and `vkDeviceWaitIdle()` were not
waiting for all commands to be enqueued before enqueuing wait operation.
- Fix shader conversion failure when using storage images on _iOS_ & _tvOS_ with Tier 1 argument buffer support.
- Fix memory leak in debug utils messenger.
- Fix build failure on _VisionOS 2.0_ platform.
- `vkUpdateDescriptorSets()`: Support writing beyond descriptor binding size if subsequent bindings are of same type.
- Support `VK_FORMAT_A2B10G10R10_UNORM_PACK32` and `VK_FORMAT_A2R10G10B10_UNORM_PACK32` formats as surface formats on all platforms.
- Add `MTLStoreAction` mapping for `VK_ATTACHMENT_STORE_OP_NONE`.
- Add estimate of `presentMargin` in returned data from `vkGetPastPresentationTimingGOOGLE()`.
Expand Down
7 changes: 0 additions & 7 deletions MoltenVK/MoltenVK/GPUObjects/MVKDescriptor.mm
Original file line number Diff line number Diff line change
Expand Up @@ -724,13 +724,6 @@ void mvkPopulateShaderConversionConfig(mvk::SPIRVToMSLConversionConfiguration& s
setResourceIndexOffset(textureIndex, 1);
if (!getMetalFeatures().nativeTextureAtomics) { setResourceIndexOffset(bufferIndex, 1); }

#if MVK_IOS_OR_TVOS
// iOS Tier 1 argument buffers do not support writable images.
if (getMetalFeatures().argumentBuffersTier < MTLArgumentBuffersTier2) {
_layout->_canUseMetalArgumentBuffer = false;
}
#endif

if (pBinding->descriptorCount > 1 && !mtlFeats.arrayOfTextures) {
_layout->setConfigurationResult(reportError(VK_ERROR_FEATURE_NOT_PRESENT, "Device %s does not support arrays of textures.", _device->getName()));
}
Expand Down
1 change: 1 addition & 0 deletions MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ class MVKDescriptorSetLayout : public MVKVulkanAPIDeviceObject {
uint32_t getBufferSizeBufferArgBuferIndex() { return 0; }
id <MTLArgumentEncoder> getMTLArgumentEncoder(uint32_t variableDescriptorCount);
uint64_t getMetal3ArgumentBufferEncodedLength(uint32_t variableDescriptorCount);
bool checkCanUseArgumentBuffers(const VkDescriptorSetLayoutCreateInfo* pCreateInfo);
std::string getLogDescription();

MVKSmallVector<MVKDescriptorSetLayoutBinding> _bindings;
Expand Down
30 changes: 26 additions & 4 deletions MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.mm
Original file line number Diff line number Diff line change
Expand Up @@ -363,10 +363,6 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf

MVKDescriptorSetLayout::MVKDescriptorSetLayout(MVKDevice* device,
const VkDescriptorSetLayoutCreateInfo* pCreateInfo) : MVKVulkanAPIDeviceObject(device) {

_isPushDescriptorLayout = mvkIsAnyFlagEnabled(pCreateInfo->flags, VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR);
_canUseMetalArgumentBuffer = !_isPushDescriptorLayout; // Push descriptors don't use argument buffers

const VkDescriptorBindingFlags* pBindingFlags = nullptr;
for (const auto* next = (VkBaseInStructure*)pCreateInfo->pNext; next; next = next->pNext) {
switch (next->sType) {
Expand All @@ -382,6 +378,9 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
}
}

_isPushDescriptorLayout = mvkIsAnyFlagEnabled(pCreateInfo->flags, VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR);
_canUseMetalArgumentBuffer = checkCanUseArgumentBuffers(pCreateInfo); // After _isPushDescriptorLayout

// The bindings in VkDescriptorSetLayoutCreateInfo do not need to be provided in order of binding number.
// However, several subsequent operations, such as the dynamic offsets in vkCmdBindDescriptorSets()
// are ordered by binding number. To prepare for this, sort the bindings by binding number.
Expand All @@ -403,6 +402,7 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
return bindInfo1.pBinding->binding < bindInfo2.pBinding->binding;
});

// Create bindings. Must be done after _isPushDescriptorLayout & _canUseMetalArgumentBuffer are set.
uint32_t dslDescCnt = 0;
uint32_t dslMTLRezCnt = needsBuffSizeAuxBuff ? 1 : 0; // If needed, leave a slot for the buffer sizes buffer at front.
_bindings.reserve(bindCnt);
Expand All @@ -424,6 +424,28 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
return descStr.str();
}

// Check if argument buffers can be used, and return findings.
// Must be called after setting _isPushDescriptorLayout.
bool MVKDescriptorSetLayout::checkCanUseArgumentBuffers(const VkDescriptorSetLayoutCreateInfo* pCreateInfo) {

// iOS Tier 1 argument buffers do not support writable images.
#if MVK_IOS_OR_TVOS
if (getMetalFeatures().argumentBuffersTier < MTLArgumentBuffersTier2) {
for (uint32_t bindIdx = 0; bindIdx < pCreateInfo->bindingCount; bindIdx++) {
switch (pCreateInfo->pBindings[bindIdx].descriptorType) {
case VK_DESCRIPTOR_TYPE_STORAGE_IMAGE:
case VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER:
return false;
default:
break;
}
}
}
#endif

return !_isPushDescriptorLayout; // Push descriptors don't use argument buffers
}


#pragma mark -
#pragma mark MVKDescriptorSet
Expand Down
Loading