On Fri, Feb 10, 2017 at 10:59:38AM -0800, Jason Ekstrand wrote: > On Thu, Feb 9, 2017 at 5:32 PM, Nanley Chery <nanleych...@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. > >
Sounds good. > > > --- > > > 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. > > > > > + 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. > Thank you for clearing up my confusion. -Nanley > --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@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