[Mesa-dev] [PATCH 5/5] anv/pipeline: Be smarter about depth/stencil state
Nanley Chery
nanleychery at gmail.com
Fri Feb 10 19:40:50 UTC 2017
On Fri, Feb 10, 2017 at 10:59:38AM -0800, Jason Ekstrand wrote:
> On Thu, Feb 9, 2017 at 5:32 PM, Nanley Chery <nanleychery at gmail.com> wrote:
>
> > On Wed, Feb 01, 2017 at 05:43:55PM -0800, Jason Ekstrand wrote:
> > > This seemed to help Dota 2 by a percent or two.
> >
> > I wasn't able to reproduce this on SKL, could you qualify this with "on
> > BDW"?
> >
>
> I've changed it to:
>
> 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.
>
>
> > > ---
> > > src/intel/vulkan/genX_pipeline.c | 133 +++++++++++++++++++++++++++++-
> > ---------
> > > 1 file changed, 99 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_
> > pipeline.c
> > > index f6940d2..1961874 100644
> > > --- a/src/intel/vulkan/genX_pipeline.c
> > > +++ b/src/intel/vulkan/genX_pipeline.c
> > > @@ -634,6 +634,95 @@ 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)
> > > +{
> > > + return (face->compareOp != VK_COMPARE_OP_ALWAYS &&
> > > + face->failOp != VK_STENCIL_OP_KEEP) ||
> > > + (face->compareOp != VK_COMPARE_OP_NEVER &&
> > > + ((depthCompareOp != VK_COMPARE_OP_NEVER &&
> > > + face->passOp != VK_STENCIL_OP_KEEP) ||
> > > + (depthCompareOp != VK_COMPARE_OP_ALWAYS &&
> > > + face->depthFailOp != VK_STENCIL_OP_KEEP)));
> >
> > This is quite confusing to me. Could you split out each
> > part of this condition? Comments may also help.
> >
>
> Done.
>
>
> > > +}
> > > +
> > > +/* Intel hardware is fairly sensitive to whether or not depth/stencil
> > writes
> > > + * are enabled. In the presence of discards, disabling writes means
> > that the
> > > + * depth/stencil testing can get moved earlier in the pipeline which
> > allows it
> >
> > Could you cite the source for this statement? This doesn't match my
> > understanding of what the following sections of the hardware docs say:
> > * ILK PRM: 8.4.3.2 Early Depth Test Cases [Pre-DevSNB]
> > * IVB PRM: 11.5.2 Early Depth Test/Stencil Test/Write
> >
> > According to these docs, on IVB+, depth/stencil testing is always
> > performed early when the PS doesn't write out depth. In the presence of
> > discards, disabling writes makes a depth pixel promoted instead of
> > non-promoted, meaning that the writes aren't performed at the end of the
> > PS.
> >
>
> Done.
>
>
> > > + * to avoid executing the fragment shader of the depth or stencil test
> > fails.
> > > + *
> > > + * 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.
> > > + */
> >
> > This is a great idea!
> >
> > > +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 &&
> >
> > This also needs:
> >
> > ds_aspects & VK_IMAGE_ASPECT_STENCIL_BIT &&
> >
> > If a stencil aspect isn't there, the stencil test always passes. You
> > could mention this nuance in the comment above.
> >
>
> If we don't have stencil, we set the compare ops to ALWAYS above so the two
> cases below will fail.
>
I see, thanks for pointing that out.
-Nanley
>
> > > + state->front.compareOp == VK_COMPARE_OP_NEVER &&
> > > + state->back.compareOp == VK_COMPARE_OP_NEVER) {
> > > + state->depthTestEnable = false;
> > > + state->depthWriteEnable = false;
> > > + }
> >
> > We may want to handle the following cases eventually:
> >
> > 1. state->maxDepthBounds < state->minDepthBounds
> > 2. compareMask && compareOp combinations
> >
> > > +
> > > + /* 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))
> >
> > This is not enough to disable writes. Even if the user sets
> > VK_STENCIL_OP_KEEP and VK_COMPARE_OP_ALWAYS in the appropriate fields, a
> > non-0xFF writemask would require us to write new values into the
> > stencil buffer.
> >
>
> I believe you misunderstand stencil write masks. From the spec:
>
> The least significant s bits of writeMask, where s is the number of bits in
> the stencil framebuffer attachment, specify an integer mask. Where a 1
> appears in this mask, the corresponding bit in the stencil value in the
> depth/stencil attachment is written; where a 0 appears, the bit is not
> written. The writeMask value uses either the front-facing or back-facing
> state based on the facing-ness of the fragment.
>
> In other words, the mask simply controls which bits get updated. It does
> not automatically cause any bits not in the mask to get zeroed out.
>
> --Jason
>
>
> > -Nanley
> >
> > > + *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,
> > > @@ -656,12 +745,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
> > > @@ -683,38 +780,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)) {
> > > - depth_stencil.StencilBufferWriteEnable = 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