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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev