Merge pull request #2046 from billhollings/fix-prim-rsrt-warn

Emit primitiveRestartEnable disabled warning only for strip topology.
KhronosGroup · Oct 18, 2023 · 9e4ee9e · 9e4ee9e
2 parents 37268ca + 37e4fef
commit 9e4ee9e
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 17 deletions.
diff --git a/MoltenVK/MoltenVK/Commands/MVKCmdRendering.h b/MoltenVK/MoltenVK/Commands/MVKCmdRendering.h
@@ -584,6 +584,8 @@ class MVKCmdSetPrimitiveRestartEnable : public MVKCommand {
 
 protected:
 	MVKCommandTypePool<MVKCommand>* getTypePool(MVKCommandPool* cmdPool) override;
+
+	VkBool32 _primitiveRestartEnable;
 };
 
 

diff --git 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
@@ -312,6 +312,8 @@ class MVKRenderingCommandEncoderState : public MVKCommandEncoderState {
 	void setViewports(const MVKArrayRef<VkViewport> viewports, uint32_t firstViewport, bool isDynamic);
 	void setScissors(const MVKArrayRef<VkRect2D> 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
@@ -436,6 +436,11 @@
 	setContent(Scissors);
 }
 
+void MVKRenderingCommandEncoderState::setPrimitiveRestartEnable(VkBool32 primitiveRestartEnable, bool isDynamic) {
+	bool mtlPrimitiveRestartEnable = static_cast<bool>(primitiveRestartEnable);
+	setContent(PrimitiveRestartEnable);
+}
+
 void MVKRenderingCommandEncoderState::setRasterizerDiscardEnable(VkBool32 rasterizerDiscardEnable, bool isDynamic) {
 	bool mtlRasterizerDiscardEnable = static_cast<bool>(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
@@ -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
@@ -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);