<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Feb 13, 2017 at 11:14 AM, 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 09:26:52PM -0800, Jason Ekstrand wrote: > On Fri, Feb 10, 2017 at 5:29 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > 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 || > > > > Why does the depth compare op independently impact the stencil op? > > Did you mean && instead of ||? > > > > Perhaps it would help if I explained the way that I'm reasoning about > things a bit better. > > 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. > > 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. > > 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. > > 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. > > --Jason > > > > > 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? > > </div></div>It looks like you missed this question. The second function I sent isn't 100% correct, but I think the general structure of the code and comments is easier to grasp. I do find your explanation helpful to understanding how your function works. If you disagree with using my proposed function as a starting point, could you massage your explanation into code comments? </blockquote><div> </div><div>Yeah, I would probably say that your function is the wrong mental model. I can definitely add more code comments and send another version. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -Nanley <div class="HOEnZb"><div class="h5"> > > -Nanley > > > > > >-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>