<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 9, 2017 at 5:32 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Feb 01, 2017 at 05:43:55PM -0800, Jason Ekstrand wrote: > This seemed to help Dota 2 by a percent or two. I wasn't able to reproduce this on SKL, could you qualify this with "on BDW"? </blockquote><div> </div><div>I've changed it to: It's a bit hard to measure because it almost gets lost in the noise, but this seemed to help Dota 2 by a percent or two on my Broadwell GT3e desktop. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > --- > src/intel/vulkan/genX_pipeline.c | 133 +++++++++++++++++++++++++++++---------- > 1 file changed, 99 insertions(+), 34 deletions(-) > > diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c > index f6940d2..1961874 100644 > --- a/src/intel/vulkan/genX_pipeline.c > +++ b/src/intel/vulkan/genX_pipeline.c > @@ -634,6 +634,95 @@ static const uint32_t vk_to_gen_stencil_op[] = { > [VK_STENCIL_OP_DECREMENT_AND_WRAP] = STENCILOP_DECR, > }; > > +static bool > +may_write_stencil_face(const VkStencilOpState *face, > + VkCompareOp depthCompareOp) > +{ > + return (face->compareOp != VK_COMPARE_OP_ALWAYS && > + face->failOp != VK_STENCIL_OP_KEEP) || > + (face->compareOp != VK_COMPARE_OP_NEVER && > + ((depthCompareOp != VK_COMPARE_OP_NEVER && > + face->passOp != VK_STENCIL_OP_KEEP) || > + (depthCompareOp != VK_COMPARE_OP_ALWAYS && > + face->depthFailOp != VK_STENCIL_OP_KEEP))); This is quite confusing to me. Could you split out each part of this condition? Comments may also help. </blockquote><div> </div><div>Done. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +} > + > +/* Intel hardware is fairly sensitive to whether or not depth/stencil writes > + * are enabled. In the presence of discards, disabling writes means that the > + * depth/stencil testing can get moved earlier in the pipeline which allows it Could you cite the source for this statement? This doesn't match my understanding of what the following sections of the hardware docs say: * ILK PRM: 8.4.3.2 Early Depth Test Cases [Pre-DevSNB] * IVB PRM: 11.5.2 Early Depth Test/Stencil Test/Write According to these docs, on IVB+, depth/stencil testing is always performed early when the PS doesn't write out depth. In the presence of discards, disabling writes makes a depth pixel promoted instead of non-promoted, meaning that the writes aren't performed at the end of the PS. </blockquote><div> </div><div>Done. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + * to avoid executing the fragment shader of the depth or stencil test fails. > + * > + * Unfortunately, the way depth and stencil testing is specified, there are > + * many case where, regardless of depth/stencil writes being enabled, nothing > + * actually gets written due to some other bit of state being set. This > + * function attempts to "sanitize" the depth stencil state and disable writes > + * and sometimes even testing whenever possible. > + */ This is a great idea! <div><div class="gmail-h5"> > +static void > +sanitize_ds_state(VkPipelineDepthStencilStateCreateInfo *state, > + bool *stencilWriteEnable, > + VkImageAspectFlags ds_aspects) > +{ > + *stencilWriteEnable = state->stencilTestEnable; > + > + /* If the depth test is disabled, we won't be writing anything. */ > + if (!state->depthTestEnable) > + state->depthWriteEnable = false; > + > + /* The Vulkan spec requires that if either depth or stencil is not present, > + * the pipeline is to act as if the test silently passes. > + */ > + if (!(ds_aspects & VK_IMAGE_ASPECT_DEPTH_BIT)) { > + state->depthWriteEnable = false; > + state->depthCompareOp = VK_COMPARE_OP_ALWAYS; > + } > + > + if (!(ds_aspects & VK_IMAGE_ASPECT_STENCIL_BIT)) { > + *stencilWriteEnable = false; > + state->front.compareOp = VK_COMPARE_OP_ALWAYS; > + state->back.compareOp = VK_COMPARE_OP_ALWAYS; > + } > + > + /* If the stencil test is enabled and always fails, then we will never get > + * to the depth test so we can just disable the depth test entirely. > + */ > + if (state->stencilTestEnable && </div></div>This also needs: ds_aspects & VK_IMAGE_ASPECT_STENCIL_BIT && If a stencil aspect isn't there, the stencil test always passes. You could mention this nuance in the comment above. </blockquote><div> </div><div>If we don't have stencil, we set the compare ops to ALWAYS above so the two cases below will fail. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + state->front.compareOp == VK_COMPARE_OP_NEVER && > + state->back.compareOp == VK_COMPARE_OP_NEVER) { > + state->depthTestEnable = false; > + state->depthWriteEnable = false; > + } We may want to handle the following cases eventually: 1. state->maxDepthBounds < state->minDepthBounds 2. compareMask && compareOp combinations > + > + /* If depthCompareOp is EQUAL then the value we would be writing to the > + * depth buffer is the same as the value that's already there so there's no > + * point in writing it. > + */ > + if (state->depthCompareOp == VK_COMPARE_OP_EQUAL) > + state->depthWriteEnable = false; > + > + /* If the stencil ops are such that we don't actually ever modify the > + * stencil buffer, we should disable writes. > + */ > + if (!may_write_stencil_face(&state->front, state->depthCompareOp) && > + !may_write_stencil_face(&state->back, state->depthCompareOp)) This is not enough to disable writes. Even if the user sets VK_STENCIL_OP_KEEP and VK_COMPARE_OP_ALWAYS in the appropriate fields, a non-0xFF writemask would require us to write new values into the stencil buffer. </blockquote><div> </div><div>I believe you misunderstand stencil write masks. From the spec: The least significant s bits of <code>writeMask</code>, where s is the number of bits in the stencil framebuffer attachment, specify an integer mask. Where a 1 appears in this mask, the corresponding bit in the stencil value in the depth/stencil attachment is written; where a 0 appears, the bit is not written. The <code>writeMask</code> value uses either the front-facing or back-facing state based on the facing-ness of the fragment. </div><div>In other words, the mask simply controls which bits get updated. It does not automatically cause any bits not in the mask to get zeroed out. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> -Nanley <div><div class="gmail-h5"> > + *stencilWriteEnable = false; > + > + /* If the depth test always passes and we never write out depth, that's the > + * same as if the depth test is disabled entirely. > + */ > + if (state->depthCompareOp == VK_COMPARE_OP_ALWAYS && > + !state->depthWriteEnable) > + state->depthTestEnable = false; > + > + /* If the stencil test always passes and we never write out stencil, that's > + * the same as if the stencil test is disabled entirely. > + */ > + if (state->front.compareOp == VK_COMPARE_OP_ALWAYS && > + state->back.compareOp == VK_COMPARE_OP_ALWAYS && > + !*stencilWriteEnable) > + state->stencilTestEnable = false; > +} > + > static void > emit_ds_state(struct anv_pipeline *pipeline, > const VkPipelineDepthStencilStateCreateInfo *pCreateInfo, > @@ -656,12 +745,20 @@ emit_ds_state(struct anv_pipeline *pipeline, > return; > } > > + VkImageAspectFlags ds_aspects = 0; > + if (subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) { > + VkFormat depth_stencil_format = > + pass->attachments[subpass->depth_stencil_attachment].format; > + ds_aspects = vk_format_aspects(depth_stencil_format); > + } > + > VkPipelineDepthStencilStateCreateInfo info = *pCreateInfo; > + sanitize_ds_state(&info, &pipeline->writes_stencil, ds_aspects); > + pipeline->writes_depth = info.depthWriteEnable; > + pipeline->depth_test_enable = info.depthTestEnable; > > /* VkBool32 depthBoundsTestEnable; // optional (depth_bounds_test) */ > > - pipeline->writes_stencil = info.stencilTestEnable; > - > #if GEN_GEN <= 7 > struct GENX(DEPTH_STENCIL_STATE) depth_stencil = { > #else > @@ -683,38 +780,6 @@ emit_ds_state(struct anv_pipeline *pipeline, > .BackfaceStencilTestFunction = vk_to_gen_compare_op[info.back.compareOp], > }; > > - VkImageAspectFlags aspects = 0; > - if (subpass->depth_stencil_attachment != VK_ATTACHMENT_UNUSED) { > - VkFormat depth_stencil_format = > - pass->attachments[subpass->depth_stencil_attachment].format; > - aspects = vk_format_aspects(depth_stencil_format); > - } > - > - /* The Vulkan spec requires that if either depth or stencil is not present, > - * the pipeline is to act as if the test silently passes. > - */ > - if (!(aspects & VK_IMAGE_ASPECT_DEPTH_BIT)) { > - depth_stencil.DepthBufferWriteEnable = false; > - depth_stencil.DepthTestFunction = PREFILTEROPALWAYS; > - } > - > - if (!(aspects & VK_IMAGE_ASPECT_STENCIL_BIT)) { > - depth_stencil.StencilBufferWriteEnable = false; > - depth_stencil.StencilTestFunction = PREFILTEROPALWAYS; > - depth_stencil.BackfaceStencilTestFunction = PREFILTEROPALWAYS; > - } > - > - /* From the Broadwell PRM: > - * > - * "If Depth_Test_Enable = 1 AND Depth_Test_func = EQUAL, the > - * Depth_Write_Enable must be set to 0." > - */ > - if (info.depthTestEnable && info.depthCompareOp == VK_COMPARE_OP_EQUAL) > - depth_stencil.DepthBufferWriteEnable = false; > - > - pipeline->writes_depth = depth_stencil.DepthBufferWriteEnable; > - pipeline->depth_test_enable = depth_stencil.DepthTestEnable; > - > #if GEN_GEN <= 7 > GENX(DEPTH_STENCIL_STATE_pack)(NULL, depth_stencil_dw, &depth_stencil); > #else > -- > 2.5.0.400.gff86faf > </div></div>> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>