[Mesa-dev] [PATCH v2 5/7] anv/pipeline: Be smarter about depth/stencil state

Nanley Chery nanleychery at gmail.com
Mon Feb 13 19:14:16 UTC 2017


On Fri, Feb 10, 2017 at 09:26:52PM -0800, Jason Ekstrand wrote:
> On Fri, Feb 10, 2017 at 5:29 PM, Nanley Chery <nanleychery at gmail.com> 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 <nanleychery at gmail.com>
> > 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 <lionel.g.landwerlin at intel.com>
> > > > > > ---
> > > > > >  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?
> >

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?

-Nanley

> > -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
> > > > > > mesa-dev at lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > >
> >


More information about the mesa-dev mailing list