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

Jason Ekstrand jason at jlekstrand.net
Fri Feb 10 23:21:38 UTC 2017


On Fri, Feb 10, 2017 at 2:55 PM, Nanley Chery <nanleychery at gmail.com> 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.
>
> 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;
>    }
>

This covers a lot fewer cases.  For instance, if all of the stencil ops are
VK_STENCIL_OP_KEEP but compareOp == VK_COMPARE_OP_GREATER, your function
will claim that it writes stencil even though it clearly doesn't.


>    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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170210/cd71aae6/attachment-0001.html>


More information about the mesa-dev mailing list