[Mesa-dev] [PATCH v2 5/7] anv/pipeline: Be smarter about depth/stencil state
Jason Ekstrand
jason at jlekstrand.net
Sat Feb 11 05:26:52 UTC 2017
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?
>
> -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
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170210/d16293da/attachment-0001.html>
More information about the mesa-dev
mailing list