<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Feb 10, 2017 at 5:29 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Feb 10, 2017 at 03:31:32PM -0800, Jason Ekstrand wrote: > On Fri, Feb 10, 2017 at 3:18 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > On Fri, Feb 10, 2017 at 02:55:42PM -0800, Nanley Chery wrote: > > > On Fri, Feb 10, 2017 at 11:02:19AM -0800, Jason Ekstrand wrote: > > > > 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. > > > > > > > > Reviewed-by: Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com">lionel.g.landwerlin@intel.com</a>> > > > > --- > > > > src/intel/vulkan/genX_pipeline.c | 171 ++++++++++++++++++++++++++++++ > > +-------- > > > > 1 file changed, 137 insertions(+), 34 deletions(-) > > > > > > > > diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_ > > pipeline.c > > > > index 71b9e30..d2af8b9 100644 > > > > --- a/src/intel/vulkan/genX_pipeline.c > > > > +++ b/src/intel/vulkan/genX_pipeline.c > > > > @@ -638,6 +638,133 @@ 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) > > > > +{ > > > > > > This function triggers some false negatives. One example is > > > > > > face->compareOp == VK_COMPARE_OP_GREATER && > > > face->depthFailOp == VK_STENCIL_OP_KEEP && > > > face->passOp == VK_STENCIL_OP_KEEP && > > > face->failOp == VK_STENCIL_OP_INVERT && > > > depthCompareOp == don't care; > > > > > > This function returns false even though a stencil write occurs if the > > > stencil test results in a fail. > > > > Thanks for pointing out in the office that this actually isn't a false > > negative. This function is correct, but I still think it could be > > simplified. I really needed a visual to evaluate it as it currently is > > written. > > > > There is a way I could write it that may be clearer. How about: > > may_write_stencil_face(const VkStencilOpState *face, > VkCompareOp depthCompareOp) > { > /* If compareOp is ALWAYS, then failOp doesn't matter */ > if (face->compareOp == VK_COMPARE_OP_ALWAYS) > face->failOp = VK_STENCIL_OP_KEEP; > > if (face->compareOp == VK_COMPARE_OP_NEVER || </div></div>Why does the depth compare op independently impact the stencil op? Did you mean && instead of ||? </blockquote><div> </div><div>Perhaps it would help if I explained the way that I'm reasoning about things a bit better. </div><div>In the first case (above), if compareOp is ALWAYS then the stencil test will never fail so the failOp doesn't matter. We can set it to KEEP or whatever else we like but KEEP is the most convenient. </div><div>The second case is a bit more complicated. Here we're looking at the stencil+depth pass op. In order for this to trigger, both the depth and the stencil test have to pass. If either compare function is NEVER then at least one of them will always fail and we'll never trigger the passOp. </div><div>In the third case, depthFailOp gets taken if stencil passes but depth fails. Again, if compareOp == NEVER or depthCompareOp == ALWAYS, then this case cannot happen and depthFailOp doesn't matter. </div><div>Does that make sense? It's not so much about figuring out which of the stencil ops get taken as it is about figuring out which ones don't matter because the corresponding depth and stencil test results cannot happen. Then, you take the ones that do matter and look at the stencil ops. If any of the stencil ops that *can* happen aren't KEEP, then we write stencil. </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > face->depthCompareOp == VK_COMPARE_OP_NEVER) > face->passOp = VK_STENCIL_OP_KEEP; > > if (face->compareOp == VK_COMPARE_OP_NEVER || > face->depthCompareOp == VK_COMPARE_OP_ALWAYS) > face->depthFailOp = VK_STENCIL_OP_KEEP; > > return face->failOp != VK_STENCIL_OP_KEEP || > face->depthFailOp != VK_STENCIL_OP_KEEP || > face->passOp != VK_STENCIL_OP_KEEP; Why does this work? > } > > I thought about writing it that way at one point but I decided sanitizing > the stencil ops wasn't needed. I can switch if you'd rather. > I sent another function to the list before I saw this, which I think is more straight-forward. What do you think? -Nanley <div class="HOEnZb"><div class="h5"> > >-Nanley > > > > > > > > The possible input combinations that affect writing to stencil can be > > > divided up into 10 sets. > > > > > > s = stencil compare op > > > d = depth compare op > > > df = depth fail op > > > sf = stencil fail op > > > sp = stencil pass op > > > > > > s.Always | !s.Always > > > d.Never | !d.Never | s.Never | > > > | d.Always | | | > > > !keep | | | | > > > | | | | > > > ------------|----------|------|--------------|-------------- > > > | | | | > > > keep df.Keep | sp.Keep | | sf.Keep | > > > | | | | > > > | | | | > > > > > > We know that stencil won't be written if the function inputs land us in > > > 3 of these sets (denoted by the *.Keep's above). I think this function > > > would be easier to understand if it determined if we fell in one of the > > > 3 instead of the 7. How about something like this: > > > > > > static bool > > > wont_write_stencil_face(const VkStencilOpState *face, > > > VkCompareOp depthCompareOp) > > > { > > > if (face->compareOp == VK_COMPARE_OP_NEVER && > > > face->failOp == VK_STENCIL_OP_KEEP) > > > return true; > > > > > > if (face->compareOp == VK_COMPARE_OP_ALWAYS) { > > > if (depthCompareOp == VK_COMPARE_OP_NEVER && > > > face->depthFailOp == VK_STENCIL_OP_KEEP) > > > return true; > > > if (depthCompareOp == VK_COMPARE_OP_ALWAYS && > > > face->passOp == VK_STENCIL_OP_KEEP) > > > return true; > > > } > > > return false; > > > } > > > > > > -Nanley > > > > > > > + /* If compareOp is ALWAYS then the stencil test will never fail > > and we can > > > > + * ignore failOp. If it's not ALWAYS and failOp is not KEEP, we > > may write > > > > + * stencil. > > > > + */ > > > > + if (face->compareOp != VK_COMPARE_OP_ALWAYS && > > > > + face->failOp != VK_STENCIL_OP_KEEP) > > > > + return true; > > > > + > > > > + > > > > + /* If the compareOp is NEVER then the stencil test will never pass > > and the > > > > + * passOp and depthFailOp don't matter. If compareOp isn't NEVER, > > then we > > > > + * need to take them into account. > > > > + */ > > > > + if (face->compareOp != VK_COMPARE_OP_NEVER) { > > > > + /* If depthOp is NEVER then passOp doesn't matter. */ > > > > + if (depthCompareOp != VK_COMPARE_OP_NEVER && > > > > + face->passOp != VK_STENCIL_OP_KEEP) > > > > + return true; > > > > + > > > > + /* If depthOp is ALWAYS then depthFailOp doesn't matter. */ > > > > + if (depthCompareOp != VK_COMPARE_OP_ALWAYS && > > > > + face->depthFailOp != VK_STENCIL_OP_KEEP) > > > > + return true; > > > > + } > > > > + > > > > + return false; > > > > +} > > > > + > > > > +/* Intel hardware is fairly sensitive to whether or not depth/stencil > > writes > > > > + * are enabled. In the presence of discards, it's fairly easy to get > > into the > > > > + * non-promoted case which means a fairly big performance hit. From > > the Iron > > > > + * Lake PRM, Vol 2, pt. 1, section 8.4.3.2, "Early Depth Test Cases": > > > > + * > > > > + * "Non-promoted depth (N) is active whenever the depth test can > > be done > > > > + * early but it cannot determine whether or not to write source > > depth to > > > > + * the depth buffer, therefore the depth write must be performed > > post pixel > > > > + * shader. This includes cases where the pixel shader can kill > > pixels, > > > > + * including via sampler chroma key, as well as cases where the > > alpha test > > > > + * function is enabled, which kills pixels based on a programmable > > alpha > > > > + * test. In this case, even if the depth test fails, the pixel > > cannot be > > > > + * killed if a stencil write is indicated. Whether or not the > > stencil write > > > > + * happens depends on whether or not the pixel is killed later. In > > these > > > > + * cases if stencil test fails and stencil writes are off, the > > pixels can > > > > + * also be killed early. If stencil writes are enabled, the pixels > > must be > > > > + * treated as Computed depth (described above)." > > > > + * > > > > + * The same thing as mentioned in the stencil case can happen in the > > depth > > > > + * case as well if it thinks it writes depth but, thanks to the depth > > test > > > > + * being GL_EQUAL, the write doesn't actually matter. A little extra > > work > > > > + * up-front to try and disable depth and stencil writes can make a big > > > > + * difference. > > > > + * > > > > + * 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. > > > > + */ > > > > +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 && > > > > + state->front.compareOp == VK_COMPARE_OP_NEVER && > > > > + state->back.compareOp == VK_COMPARE_OP_NEVER) { > > > > + state->depthTestEnable = false; > > > > + state->depthWriteEnable = false; > > > > + } > > > > + > > > > + /* 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)) > > > > + *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, > > > > @@ -663,12 +790,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 > > > > @@ -690,38 +825,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)) { > > > > - pipeline->writes_stencil = 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 > > > > > > > > _______________________________________________ > > > > 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> > > </div></div></blockquote></div> </div></div>