[Mesa-dev] [PATCH v2 5/7] anv/pipeline: Be smarter about depth/stencil state
Jason Ekstrand
jason at jlekstrand.net
Sat Feb 11 01:04:48 UTC 2017
On Fri, Feb 10, 2017 at 4:47 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> On Fri, Feb 10, 2017 at 03:21:38PM -0800, Jason Ekstrand wrote:
> > 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.
> >
>
> You're right, there are more than 3/10 cases in which we know we won't
> be writing to the stencil face. Here's another attempt at an
> easier-to-understand function:
>
This function gets it wrong in the following case:
compareOp == VK_COMPARE_OP_GREATER
failOp == VK_STENCIL_OP_KEEP
passOp == VK_STENCIL_OP_REPLACE
depthFailOp == VK_STENCIL_OP_REPLACE
In this case, st_may_fail == true but sfo_not_keep == false so it returns
false. However, it can clearly still write stencil if the stencil test
passes.
--Jason
> static bool
> may_write_stencil_face(const VkStencilOpState *face,
> VkCompareOp depthCompareOp)
> {
> const bool spo_not_keep = face->passOp != VK_STENCIL_OP_KEEP;
> const bool sfo_not_keep = face->failOp != VK_STENCIL_OP_KEEP;
> const bool dfo_not_keep = face->depthFailOp != VK_STENCIL_OP_KEEP;
>
> const bool dt_may_fail = depthCompareOp != VK_COMPARE_OP_ALWAYS;
> const bool st_may_fail = face->compareOp != VK_COMPARE_OP_ALWAYS;
>
> if (st_may_fail) {
> /* If the stencil test may fail, but the fail op is not keep, we
> might be
> * writing to the stencil face.
> */
> if (sfo_not_keep)
> return true;
> } else {
> /* If the stencil test will pass, but the pass op is not keep, we
> might
> * be writing to the stencil face.
> */
> if (spo_not_keep)
> return true;
>
> /* If the stencil test will pass, and the depth test may fail, but
> the
> * depth fail op is not keep, we might be writing to the stencil
> face.
> */
> if (dt_may_fail && dfo_not_keep)
> return true;
> }
>
> return false;
> }
>
> -Nanley
>
> >
> > > 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/104037c2/attachment-0001.html>
More information about the mesa-dev
mailing list