From be7a68153f9442ba400a005f5296fd4448c8a66c Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Mon, 18 Apr 2022 15:32:40 -0400 Subject: [PATCH] Fix error where previously bound push constants can override a descriptor buffer binding used by a subsequent pipeline that does not use push constants. This error was previously introduced in 2a17f75, where a push constants binding could override the Metal buffer binding 0 of a subsequent pipeline that does not use push constants. When pipeline binding is encoded, track which stages use push constants and only encode push constants if the pipeline and stage uses them. (unrelated) Make use of MVKResourcesCommandEncoderState::getPipeline() consistent. --- Docs/Whats_New.md | 2 ++ .../MoltenVK/Commands/MVKCommandEncoderState.h | 3 ++- .../MoltenVK/Commands/MVKCommandEncoderState.mm | 17 +++++++++-------- MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm | 5 +++-- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/Docs/Whats_New.md b/Docs/Whats_New.md index a1414982..1b7d3134 100644 --- a/Docs/Whats_New.md +++ b/Docs/Whats_New.md @@ -22,6 +22,8 @@ Released TBD - `VK_KHR_portability_enumeration` support added to `MoltenVK_icd.json`, and documentation updated to indicate the impact of the `VK_KHR_portability_enumeration` extension during runtime loading on *macOS* via the *Vulkan Loader*. +- Fix error where previously bound push constants can override a descriptor buffer binding + used by a subsequent pipeline that does not use push constants. diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.h b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.h index 38a044d7..d7d2c48c 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.h +++ b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.h @@ -198,7 +198,7 @@ public: void setPushConstants(uint32_t offset, MVKArrayRef pushConstants); /** Sets the index of the Metal buffer used to hold the push constants. */ - void setMTLBufferIndex(uint32_t mtlBufferIndex); + void setMTLBufferIndex(uint32_t mtlBufferIndex, bool pipelineStageUsesPushConstants); /** Constructs this instance for the specified command encoder. */ MVKPushConstantsCommandEncoderState(MVKCommandEncoder* cmdEncoder, @@ -212,6 +212,7 @@ protected: MVKSmallVector _pushConstants; VkShaderStageFlagBits _shaderStage; uint32_t _mtlBufferIndex = 0; + bool _pipelineStageUsesPushConstants = false; }; diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm index 3cfd9c7b..7d475987 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm +++ b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm @@ -166,18 +166,19 @@ void MVKPushConstantsCommandEncoderState:: setPushConstants(uint32_t offset, MVK if (pcBuffSize > 0) { markDirty(); } } -void MVKPushConstantsCommandEncoderState::setMTLBufferIndex(uint32_t mtlBufferIndex) { - if (mtlBufferIndex != _mtlBufferIndex) { - _mtlBufferIndex = mtlBufferIndex; - markDirty(); - } +void MVKPushConstantsCommandEncoderState::setMTLBufferIndex(uint32_t mtlBufferIndex, bool pipelineStageUsesPushConstants) { + if ((mtlBufferIndex != _mtlBufferIndex) || (pipelineStageUsesPushConstants != _pipelineStageUsesPushConstants)) { + _mtlBufferIndex = mtlBufferIndex; + _pipelineStageUsesPushConstants = pipelineStageUsesPushConstants; + markDirty(); + } } // At this point, I have been marked not-dirty, under the assumption that I will make changes to the encoder. // However, some of the paths below decide not to actually make any changes to the encoder. In that case, // I should remain dirty until I actually do make encoder changes. void MVKPushConstantsCommandEncoderState::encodeImpl(uint32_t stage) { - if (_pushConstants.empty() ) { return; } + if ( !_pipelineStageUsesPushConstants || _pushConstants.empty() ) { return; } _isDirty = true; // Stay dirty until I actually decide to make a change to the encoder @@ -739,7 +740,7 @@ void MVKGraphicsResourcesCommandEncoderState::markDirty() { void MVKGraphicsResourcesCommandEncoderState::encodeImpl(uint32_t stage) { - MVKGraphicsPipeline* pipeline = (MVKGraphicsPipeline*)_cmdEncoder->_graphicsPipelineState.getPipeline(); + MVKGraphicsPipeline* pipeline = (MVKGraphicsPipeline*)getPipeline(); bool fullImageViewSwizzle = pipeline->fullImageViewSwizzle() || getDevice()->_pMetalFeatures->nativeTextureSwizzle; bool forTessellation = pipeline->isTessellationPipeline(); @@ -975,7 +976,7 @@ void MVKComputeResourcesCommandEncoderState::encodeImpl(uint32_t) { encodeMetalArgumentBuffer(kMVKShaderStageCompute); - MVKPipeline* pipeline = _cmdEncoder->_computePipelineState.getPipeline(); + MVKPipeline* pipeline = getPipeline(); bool fullImageViewSwizzle = pipeline ? pipeline->fullImageViewSwizzle() : false; if (_resourceBindings.swizzleBufferBinding.isDirty) { diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm index 2751bde1..3ada4623 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm @@ -175,8 +175,9 @@ MVKPipelineLayout::~MVKPipelineLayout() { void MVKPipeline::bindPushConstants(MVKCommandEncoder* cmdEncoder) { for (uint32_t stage = kMVKShaderStageVertex; stage < kMVKShaderStageCount; stage++) { - if (cmdEncoder && _stageUsesPushConstants[stage]) { - cmdEncoder->getPushConstants(mvkVkShaderStageFlagBitsFromMVKShaderStage(MVKShaderStage(stage)))->setMTLBufferIndex(_pushConstantsBufferIndex.stages[stage]); + if (cmdEncoder) { + auto* pcState = cmdEncoder->getPushConstants(mvkVkShaderStageFlagBitsFromMVKShaderStage(MVKShaderStage(stage))); + pcState->setMTLBufferIndex(_pushConstantsBufferIndex.stages[stage], _stageUsesPushConstants[stage]); } } }