[Mesa-dev] [PATCH 5/5] anv/pipeline: Be smarter about depth/stencil state
Nanley Chery
nanleychery at gmail.com
Fri Feb 10 21:07:14 UTC 2017
On Thu, Feb 09, 2017 at 05:32:55PM -0800, Nanley Chery 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"?
>
> > ---
> > 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.
>
> > +}
> > +
> > +/* 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.
>
Looking over the ILK doc again, it actually looks like that
depth/stencil testing can be moved earlier when stencil writes are
disabled.
If stencil writes are enabled, the pixels must be treated as Computed
depth (described above).
-Nanley
> > + * 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.
>
> > + 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.
>
> -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