From c0103fd008ab19781fe8830e3b41fb7a85b0211c Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Wed, 22 Jul 2020 14:52:06 -0400 Subject: [PATCH 1/2] Remove redundant validation check for 2D image views on 3D images. --- MoltenVK/MoltenVK/GPUObjects/MVKImage.mm | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm index 6efd58fa..e8ea8b5e 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKImage.mm @@ -1417,12 +1417,12 @@ MVKImageView::MVKImageView(MVKDevice* device, VkImageType imgType = _image->getImageType(); VkImageViewType viewType = pCreateInfo->viewType; - // VK_KHR_maintenance1 supports taking 2D image views of 3D slices. No dice in Metal. + // VK_KHR_maintenance1 supports taking 2D image views of 3D slices for sampling. + // No dice in Metal. But we are able to fake out a 3D render attachment by making the Metal view + // itself a 3D texture (when we create it), and setting the rendering depthPlane appropriately. if ((viewType == VK_IMAGE_VIEW_TYPE_2D || viewType == VK_IMAGE_VIEW_TYPE_2D_ARRAY) && (imgType == VK_IMAGE_TYPE_3D)) { - if ( !mvkIsAnyFlagEnabled(_usage, VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT) ) { + if (!mvkIsOnlyAnyFlagEnabled(_usage, VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT)) { setConfigurationResult(reportError(VK_ERROR_FEATURE_NOT_PRESENT, "vkCreateImageView(): 2D views on 3D images can only be used as color attachments.")); - } else if (!mvkIsOnlyAnyFlagEnabled(_usage, VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT)) { - reportError(VK_ERROR_FEATURE_NOT_PRESENT, "vkCreateImageView(): 2D views on 3D images can only be used as color attachments."); } } } From 1f68d5fc2aecbd23dc8b7dead572c2ff7ccec204 Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Wed, 22 Jul 2020 17:48:19 -0400 Subject: [PATCH 2/2] Fix intermittent concurrent shader specialization race condition. MVKShaderLibrary::getMTLFunction() synchronize and refactor release of Metal objects. Make use of existing autorelease pool instead of discrete retain/release. Wrap entire specialization operation in @synchronized() to guard against Metal internals not coping with multiple simultaneous specializations. --- Docs/Whats_New.md | 1 + .../MoltenVK/GPUObjects/MVKShaderModule.mm | 107 ++++++++---------- 2 files changed, 49 insertions(+), 59 deletions(-) diff --git a/Docs/Whats_New.md b/Docs/Whats_New.md index c965b388..ffd86fc4 100644 --- a/Docs/Whats_New.md +++ b/Docs/Whats_New.md @@ -32,6 +32,7 @@ Released TBD - Fix issue where mapped host-coherent device memory not updated from image contents on *macOS*. - Fix small memory leak when setting swapchain color space. - Fix new and unexpected App Store failure on newly deprecated color space values. +- Fix intermittent concurrent shader specialization race condition. - Include vertex attribute size when testing whether attribute offset exceeds stride. - Add `MVKPhysicalDeviceMetalFeatures::vertexStrideAlignment` to track Metal vertex binding stride alignment. - Add `MVKPhysicalDeviceMetalFeatures::indirectTessellationDrawing` to track if indirect tessellation drawing is supported. diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm b/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm index 4d02a67c..91e5b8c0 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKShaderModule.mm @@ -62,74 +62,63 @@ static uint32_t getWorkgroupDimensionSize(const SPIRVWorkgroupSizeDimension& wgD MVKMTLFunction MVKShaderLibrary::getMTLFunction(const VkSpecializationInfo* pSpecializationInfo, MVKShaderModule* shaderModule) { - if ( !_mtlLibrary ) { return MVKMTLFunctionNull; } + if ( !_mtlLibrary ) { return MVKMTLFunctionNull; } - id mtlFunc = nil; - @autoreleasepool { - NSString* mtlFuncName = @(_shaderConversionResults.entryPoint.mtlFunctionName.c_str()); - MVKDevice* mvkDev = _owner->getDevice(); - uint64_t startTime = mvkDev->getPerformanceTimestamp(); - mtlFunc = [_mtlLibrary newFunctionWithName: mtlFuncName]; // temp retain - mvkDev->addActivityPerformance(mvkDev->_performanceStatistics.shaderCompilation.functionRetrieval, startTime); + @synchronized (_mtlLibrary) { + @autoreleasepool { + NSString* mtlFuncName = @(_shaderConversionResults.entryPoint.mtlFunctionName.c_str()); + MVKDevice* mvkDev = _owner->getDevice(); - if (mtlFunc) { - // If the Metal device supports shader specialization, and the Metal function expects to be - // specialized, populate Metal function constant values from the Vulkan specialization info, - // and compiled a specialized Metal function, otherwise simply use the unspecialized Metal function. - if (mvkDev->_pMetalFeatures->shaderSpecialization) { - NSArray* mtlFCs = mtlFunc.functionConstantsDictionary.allValues; - if (mtlFCs.count) { - // The Metal shader contains function constants and expects to be specialized - // Populate the Metal function constant values from the Vulkan specialization info. - MTLFunctionConstantValues* mtlFCVals = [MTLFunctionConstantValues new]; // temp retain - if (pSpecializationInfo) { - // Iterate through the provided Vulkan specialization entries, and populate the - // Metal function constant value that matches the Vulkan specialization constantID. - for (uint32_t specIdx = 0; specIdx < pSpecializationInfo->mapEntryCount; specIdx++) { - const VkSpecializationMapEntry* pMapEntry = &pSpecializationInfo->pMapEntries[specIdx]; - NSUInteger mtlFCIndex = pMapEntry->constantID; - MTLFunctionConstant* mtlFC = getFunctionConstant(mtlFCs, mtlFCIndex); - if (mtlFC) { - [mtlFCVals setConstantValue: &(((char*)pSpecializationInfo->pData)[pMapEntry->offset]) - type: mtlFC.type - atIndex: mtlFCIndex]; + uint64_t startTime = mvkDev->getPerformanceTimestamp(); + id mtlFunc = [[_mtlLibrary newFunctionWithName: mtlFuncName] autorelease]; + mvkDev->addActivityPerformance(mvkDev->_performanceStatistics.shaderCompilation.functionRetrieval, startTime); + + if (mtlFunc) { + // If the Metal device supports shader specialization, and the Metal function expects to be specialized, + // populate Metal function constant values from the Vulkan specialization info, and compile a specialized + // Metal function, otherwise simply use the unspecialized Metal function. + if (mvkDev->_pMetalFeatures->shaderSpecialization) { + NSArray* mtlFCs = mtlFunc.functionConstantsDictionary.allValues; + if (mtlFCs.count > 0) { + // The Metal shader contains function constants and expects to be specialized. + // Populate the Metal function constant values from the Vulkan specialization info. + MTLFunctionConstantValues* mtlFCVals = [[MTLFunctionConstantValues new] autorelease]; + if (pSpecializationInfo) { + // Iterate through the provided Vulkan specialization entries, and populate the + // Metal function constant value that matches the Vulkan specialization constantID. + for (uint32_t specIdx = 0; specIdx < pSpecializationInfo->mapEntryCount; specIdx++) { + const VkSpecializationMapEntry* pMapEntry = &pSpecializationInfo->pMapEntries[specIdx]; + for (MTLFunctionConstant* mfc in mtlFCs) { + if (mfc.index == pMapEntry->constantID) { + [mtlFCVals setConstantValue: ((char*)pSpecializationInfo->pData + pMapEntry->offset) + type: mfc.type + atIndex: mfc.index]; + break; + } + } } } + + // Compile the specialized Metal function, and use it instead of the unspecialized Metal function. + MVKFunctionSpecializer fs(_owner); + mtlFunc = [fs.newMTLFunction(_mtlLibrary, mtlFuncName, mtlFCVals) autorelease]; } - - // Compile the specialized Metal function, and use it instead of the unspecialized Metal function. - MVKFunctionSpecializer* fs = new MVKFunctionSpecializer(_owner); - [mtlFunc release]; // temp release - mtlFunc = fs->newMTLFunction(_mtlLibrary, mtlFuncName, mtlFCVals); // temp retain - fs->destroy(); - [mtlFCVals release]; // temp release } + } else { + reportError(VK_ERROR_INVALID_SHADER_NV, "Shader module does not contain an entry point named '%s'.", mtlFuncName.UTF8String); } - } else { - reportError(VK_ERROR_INVALID_SHADER_NV, "Shader module does not contain an entry point named '%s'.", mtlFuncName.UTF8String); + + // Set the debug name. First try name of shader module, otherwise try name of owner. + NSString* dbName = shaderModule-> getDebugName(); + if ( !dbName ) { dbName = _owner-> getDebugName(); } + setLabelIfNotNil(mtlFunc, dbName); + + auto& wgSize = _shaderConversionResults.entryPoint.workgroupSize; + return MVKMTLFunction(mtlFunc, _shaderConversionResults, MTLSizeMake(getWorkgroupDimensionSize(wgSize.width, pSpecializationInfo), + getWorkgroupDimensionSize(wgSize.height, pSpecializationInfo), + getWorkgroupDimensionSize(wgSize.depth, pSpecializationInfo))); } - - // Set the debug name. First try name of shader module, otherwise try name of owner. - NSString* dbName = shaderModule-> getDebugName(); - if ( !dbName ) { dbName = _owner-> getDebugName(); } - setLabelIfNotNil(mtlFunc, dbName); } - - auto& wgSize = _shaderConversionResults.entryPoint.workgroupSize; - - MVKMTLFunction mvkMTLFunc(mtlFunc, _shaderConversionResults, MTLSizeMake(getWorkgroupDimensionSize(wgSize.width, pSpecializationInfo), - getWorkgroupDimensionSize(wgSize.height, pSpecializationInfo), - getWorkgroupDimensionSize(wgSize.depth, pSpecializationInfo))); - [mtlFunc release]; // temp release - - return mvkMTLFunc; -} - -// Returns the MTLFunctionConstant with the specified ID from the specified array of function constants. -// The specified ID is the index value contained within the function constant. -MTLFunctionConstant* MVKShaderLibrary::getFunctionConstant(NSArray* mtlFCs, NSUInteger mtlFCID) { - for (MTLFunctionConstant* mfc in mtlFCs) { if (mfc.index == mtlFCID) { return mfc; } } - return nil; } void MVKShaderLibrary::setEntryPointName(string& funcName) {