[Mesa-dev] [PATCH 5/5] anv/pipeline: Be smarter about depth/stencil state
Nanley Chery
nanleychery at gmail.com
Fri Feb 10 01:32:55 UTC 2017
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.
> + * 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