From 37e4fefe5fb326c0dd3221930edecb9fa72e5bee Mon Sep 17 00:00:00 2001 From: Bill Hollings Date: Wed, 18 Oct 2023 09:58:11 -0400 Subject: [PATCH] Emit primitiveRestartEnable disabled warning only for strip topology. - Move check and warning to MVKRenderingCommandEncoderState. - Pass primitiveRestartEnable to MVKRenderingCommandEncoderState. - Warn only if primitiveRestartEnable disabled and strip topology is used. --- MoltenVK/MoltenVK/Commands/MVKCmdRendering.h | 2 ++ MoltenVK/MoltenVK/Commands/MVKCmdRendering.mm | 14 ++++---------- .../MoltenVK/Commands/MVKCommandEncoderState.h | 3 +++ .../MoltenVK/Commands/MVKCommandEncoderState.mm | 16 ++++++++++++++++ MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h | 1 + MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm | 9 ++------- 6 files changed, 28 insertions(+), 17 deletions(-) diff --git a/MoltenVK/MoltenVK/Commands/MVKCmdRendering.h b/MoltenVK/MoltenVK/Commands/MVKCmdRendering.h index fc67422e0..7f1df4b43 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCmdRendering.h +++ b/MoltenVK/MoltenVK/Commands/MVKCmdRendering.h @@ -584,6 +584,8 @@ class MVKCmdSetPrimitiveRestartEnable : public MVKCommand { protected: MVKCommandTypePool* getTypePool(MVKCommandPool* cmdPool) override; + + VkBool32 _primitiveRestartEnable; }; diff --git a/MoltenVK/MoltenVK/Commands/MVKCmdRendering.mm b/MoltenVK/MoltenVK/Commands/MVKCmdRendering.mm index 1aa4aa5c1..c4bb75484 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCmdRendering.mm +++ b/MoltenVK/MoltenVK/Commands/MVKCmdRendering.mm @@ -524,19 +524,13 @@ VkResult MVKCmdSetPrimitiveRestartEnable::setContent(MVKCommandBuffer* cmdBuff, VkBool32 primitiveRestartEnable) { - // Validate - // In Metal, primitive restart cannot be disabled. - // Just issue warning here, as it is very likely the app is not actually expecting - // to use primitive restart at all, and is just setting this as a "just-in-case", - // and forcing an error here would be unexpected to the app (including CTS). - if ( !primitiveRestartEnable ) { - reportWarning(VK_ERROR_FEATURE_NOT_PRESENT, "Metal does not support disabling primitive restart."); - } - + _primitiveRestartEnable = primitiveRestartEnable; return VK_SUCCESS; } -void MVKCmdSetPrimitiveRestartEnable::encode(MVKCommandEncoder* cmdEncoder) {} +void MVKCmdSetPrimitiveRestartEnable::encode(MVKCommandEncoder* cmdEncoder) { + cmdEncoder->_renderingState.setPrimitiveRestartEnable(_primitiveRestartEnable, true); +} #pragma mark - diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.h b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.h index c518c54a0..82ea4eabb 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.h +++ b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.h @@ -312,6 +312,8 @@ class MVKRenderingCommandEncoderState : public MVKCommandEncoderState { void setViewports(const MVKArrayRef viewports, uint32_t firstViewport, bool isDynamic); void setScissors(const MVKArrayRef scissors, uint32_t firstScissor, bool isDynamic); + void setPrimitiveRestartEnable(VkBool32 primitiveRestartEnable, bool isDynamic); + void setRasterizerDiscardEnable(VkBool32 rasterizerDiscardEnable, bool isDynamic); void beginMetalRenderPass() override; @@ -345,6 +347,7 @@ class MVKRenderingCommandEncoderState : public MVKCommandEncoderState { MVKRenderStateFlags _dirtyStates; MVKRenderStateFlags _modifiedStates; bool _mtlDepthBiasEnable[StateScope::Count] = {}; + bool _mtlPrimitiveRestartEnable[StateScope::Count] = {}; bool _mtlRasterizerDiscardEnable[StateScope::Count] = {}; bool _cullBothFaces[StateScope::Count] = {}; }; diff --git a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm index 35f0b6e8e..9e17aa99a 100644 --- a/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm +++ b/MoltenVK/MoltenVK/Commands/MVKCommandEncoderState.mm @@ -436,6 +436,11 @@ setContent(Scissors); } +void MVKRenderingCommandEncoderState::setPrimitiveRestartEnable(VkBool32 primitiveRestartEnable, bool isDynamic) { + bool mtlPrimitiveRestartEnable = static_cast(primitiveRestartEnable); + setContent(PrimitiveRestartEnable); +} + void MVKRenderingCommandEncoderState::setRasterizerDiscardEnable(VkBool32 rasterizerDiscardEnable, bool isDynamic) { bool mtlRasterizerDiscardEnable = static_cast(rasterizerDiscardEnable); setContent(RasterizerDiscardEnable); @@ -473,6 +478,17 @@ [rendEnc setStencilFrontReferenceValue: sr.frontFaceValue backReferenceValue: sr.backFaceValue]; } + // Validate + // In Metal, primitive restart cannot be disabled. + // Just issue warning here, as it is very likely the app is not actually expecting + // to use primitive restart at all, and is just setting this as a "just-in-case", + // and forcing an error here would be unexpected to the app (including CTS). + auto mtlPrimType = getPrimitiveType(); + if (isDirty(PrimitiveRestartEnable) && !getContent(PrimitiveRestartEnable) && + (mtlPrimType == MTLPrimitiveTypeTriangleStrip || mtlPrimType == MTLPrimitiveTypeLineStrip)) { + reportWarning(VK_ERROR_FEATURE_NOT_PRESENT, "Metal does not support disabling primitive restart."); + } + if (isDirty(Viewports)) { auto& mtlViewports = getContent(Viewports); if (_cmdEncoder->_pDeviceFeatures->multiViewport) { diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h index 3bb087358..062b646b5 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h +++ b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.h @@ -427,6 +427,7 @@ class MVKGraphicsPipeline : public MVKPipeline { uint32_t _tessCtlPatchOutputBufferIndex = 0; uint32_t _tessCtlLevelBufferIndex = 0; + bool _primitiveRestartEnable = true; bool _hasRasterInfo = false; bool _needsVertexSwizzleBuffer = false; bool _needsVertexBufferSizeBuffer = false; diff --git a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm index 49399b063..e6e05fa5b 100644 --- a/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm +++ b/MoltenVK/MoltenVK/GPUObjects/MVKPipeline.mm @@ -295,6 +295,7 @@ // Rasterization cmdEncoder->_renderingState.setPrimitiveTopology(_vkPrimitiveTopology, false); + cmdEncoder->_renderingState.setPrimitiveRestartEnable(_primitiveRestartEnable, false); cmdEncoder->_renderingState.setBlendConstants(_blendConstants, false); cmdEncoder->_renderingState.setStencilReferenceValues(_depthStencilInfo); cmdEncoder->_renderingState.setViewports(_viewports.contents(), 0, false); @@ -507,13 +508,7 @@ ? pCreateInfo->pInputAssemblyState->topology : VK_PRIMITIVE_TOPOLOGY_POINT_LIST); - // In Metal, primitive restart cannot be disabled. - // Just issue warning here, as it is very likely the app is not actually expecting - // to use primitive restart at all, and is just setting this as a "just-in-case", - // and forcing an error here would be unexpected to the app (including CTS). - if (pCreateInfo->pInputAssemblyState && !pCreateInfo->pInputAssemblyState->primitiveRestartEnable) { - reportWarning(VK_ERROR_FEATURE_NOT_PRESENT, "vkCreateGraphicsPipeline(): Metal does not support disabling primitive restart."); - } + _primitiveRestartEnable = pCreateInfo->pInputAssemblyState ? pCreateInfo->pInputAssemblyState->primitiveRestartEnable : true; // Rasterization _hasRasterInfo = mvkSetOrClear(&_rasterInfo, pCreateInfo->pRasterizationState);