On Tue, Feb 7, 2017 at 3:14 PM, Jason Ekstrand <[email protected]> wrote:
> Here's the new commit message: > > Vulkan doesn't have a stencilWriteEnable bit like it does for depth. > Instead, you have a stencil mask. Since the stencil mask is handled as > dynamic state, we have to handle it later during command buffer > construction. This helps Dota2 by a couple percent because it allows > the hardware to move the depth and stencil writes to early in more > cases. > > I'm double-checking benchmark results for the whole series. > I re-benchmarked the whole series. I can't see any difference except in the end where we play with depth and stencil operations to attempt to disable whenever possible. (The results are a lot noisier this time so it's hard to tell.) I could have sworn this patch made a difference though.... I think both are needed to actually get the perf bump. --Jason > --Jason > > > On Tue, Feb 7, 2017 at 2:28 PM, Nanley Chery <[email protected]> > wrote: > >> On Tue, Feb 07, 2017 at 12:25:18PM -0800, Jason Ekstrand wrote: >> > On Tue, Feb 7, 2017 at 12:13 PM, Nanley Chery <[email protected]> >> wrote: >> > >> > > On Wed, Feb 01, 2017 at 08:07:22PM -0800, Jason Ekstrand wrote: >> > > > The only mechanism Vulkan provides for disabling stencil writes is >> to set >> > > >> > > This isn't the only mechanism for explicitly disabling stencil writes. >> > > Stencil writes can also be disabled by disabling stencil testing (as >> can >> > > be seen in this patch). >> > > >> > >> > I guess that is technically true. I meant "it doesn't have a >> > disableStencilWrites like it does for depth". Maybe something like: >> > >> > Vulkan doesn't have a stencilWriteEnable bit like it does for depth. >> > Instead, you have a stencil mask. Since the stencil mask is... >> > >> > >> >> This is better. >> >> > > > the stencil write mask to 0. Since that is dynamic state, we have >> to >> > > move >> > > >> ^ >> > > extra >> word? >> > > >> >> Ping? >> >> > > Users can actually set the write masks at pipeline creation time. For >> > > that reason, I think it's clearer to say that it "may be" dynamic >> state >> > > instead of saying that it "is" dynamic state. >> > > >> > >> > Maybe? I guess it depends on whether you take "dynamic state" to mean >> > "state the client set at runtime vs. set in the pipeline" or if you just >> > consider it to mean the bucket of states that *may* be set at runtime. >> > >> >> You're right. anv takes the position of "may," whereas the spec takes >> the position of "is." It isn't necessarily clearer to default to the >> spec's definitions when writing commit messages for driver code. >> >> > >> > > > handle it late during command buffer builder. This helps Dota2 by a >> > > couple >> > > > percent because it allows the hardware to move the depth and stencil >> > > writes >> > > > to early in more cases. >> > > >> > > I was only able to reproduce a 0.1% performance improvement with this >> > > patch. Could show me how you're measuring the performance changes (on >> or >> > > offline)? >> > > >> > >> > For one thing, it was on my BDW gt3 desktop. Maybe SKL is impacted >> less? >> > >> >> Ah, okay. >> >> > >> > > -Nanley >> > > >> > > > >> > > > v2 (Jason Ekstrand): Always initialize the new pipeline variable >> > > > --- >> > > > src/intel/vulkan/anv_private.h | 1 + >> > > > src/intel/vulkan/gen7_cmd_buffer.c | 4 ++++ >> > > > src/intel/vulkan/gen8_cmd_buffer.c | 8 ++++++++ >> > > > src/intel/vulkan/genX_pipeline.c | 4 +++- >> > > > 4 files changed, 16 insertions(+), 1 deletion(-) >> > > > >> > > > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_ >> > > private.h >> > > > index a0cb35c..4fe3ebc 100644 >> > > > --- a/src/intel/vulkan/anv_private.h >> > > > +++ b/src/intel/vulkan/anv_private.h >> > > > @@ -1465,6 +1465,7 @@ struct anv_pipeline { >> > > > >> > > > uint32_t cs_right_mask; >> > > > >> > > > + bool writes_stencil; >> > > > bool depth_clamp_enable; >> > > > >> > > > struct { >> > > > diff --git a/src/intel/vulkan/gen7_cmd_buffer.c >> > > b/src/intel/vulkan/gen7_cmd_buffer.c >> > > > index 8d68aba..013ed87 100644 >> > > > --- a/src/intel/vulkan/gen7_cmd_buffer.c >> > > > +++ b/src/intel/vulkan/gen7_cmd_buffer.c >> > > > @@ -212,6 +212,10 @@ genX(cmd_buffer_flush_dynamic_state)(struct >> > > anv_cmd_buffer *cmd_buffer) >> > > > >> > > > .BackfaceStencilTestMask = d->stencil_compare_mask.back & >> 0xff, >> > > > .BackfaceStencilWriteMask = d->stencil_write_mask.back & >> 0xff, >> > > > + >> > > > + .StencilBufferWriteEnable = >> > > > + (d->stencil_write_mask.front || >> d->stencil_write_mask.back) >> > > && >> > > > + pipeline->writes_stencil, >> > > > }; >> > > > GENX(DEPTH_STENCIL_STATE_pack)(NULL, depth_stencil_dw, >> > > &depth_stencil); >> > > > >> > > > diff --git a/src/intel/vulkan/gen8_cmd_buffer.c >> > > b/src/intel/vulkan/gen8_cmd_buffer.c >> > > > index ab68872..8c8de62 100644 >> > > > --- a/src/intel/vulkan/gen8_cmd_buffer.c >> > > > +++ b/src/intel/vulkan/gen8_cmd_buffer.c >> > > > @@ -224,6 +224,10 @@ genX(cmd_buffer_flush_dynamic_state)(struct >> > > anv_cmd_buffer *cmd_buffer) >> > > > >> > > > .BackfaceStencilTestMask = d->stencil_compare_mask.back & >> 0xff, >> > > > .BackfaceStencilWriteMask = d->stencil_write_mask.back & >> 0xff, >> > > > + >> > > > + .StencilBufferWriteEnable = >> > > > + (d->stencil_write_mask.front || >> d->stencil_write_mask.back) >> > > && >> > > > + pipeline->writes_stencil, >> > > > }; >> > > > GENX(3DSTATE_WM_DEPTH_STENCIL_pack)(NULL, >> wm_depth_stencil_dw, >> > > > &wm_depth_stencil); >> > > > @@ -271,6 +275,10 @@ genX(cmd_buffer_flush_dynamic_state)(struct >> > > anv_cmd_buffer *cmd_buffer) >> > > > >> > > > .StencilReferenceValue = d->stencil_reference.front & >> 0xff, >> > > > .BackfaceStencilReferenceValue = >> d->stencil_reference.back & >> > > 0xff, >> > > > + >> > > > + .StencilBufferWriteEnable = >> > > > + (d->stencil_write_mask.front || >> d->stencil_write_mask.back) >> > > && >> > > > + pipeline->writes_stencil, >> > > > }; >> > > > GEN9_3DSTATE_WM_DEPTH_STENCIL_pack(NULL, dwords, >> > > &wm_depth_stencil); >> > > > >> > > > diff --git a/src/intel/vulkan/genX_pipeline.c >> b/src/intel/vulkan/genX_ >> > > pipeline.c >> > > > index 9d28466..18fe48c 100644 >> > > > --- a/src/intel/vulkan/genX_pipeline.c >> > > > +++ b/src/intel/vulkan/genX_pipeline.c >> > > > @@ -652,12 +652,15 @@ emit_ds_state(struct anv_pipeline *pipeline, >> > > > /* We're going to OR this together with the dynamic state. >> We >> > > need >> > > > * to make sure it's initialized to something useful. >> > > > */ >> > > > + pipeline->writes_stencil = false; >> > > > memset(depth_stencil_dw, 0, sizeof(depth_stencil_dw)); >> > > > return; >> > > > } >> > > > >> > > > /* VkBool32 depthBoundsTestEnable; // optional >> (depth_bounds_test) */ >> > > > >> > > > + pipeline->writes_stencil = info->stencilTestEnable; >> > > > + >> > > > #if GEN_GEN <= 7 >> > > > struct GENX(DEPTH_STENCIL_STATE) depth_stencil = { >> > > > #else >> > > > @@ -669,7 +672,6 @@ emit_ds_state(struct anv_pipeline *pipeline, >> > > > .DoubleSidedStencilEnable = true, >> > > > >> > > > .StencilTestEnable = info->stencilTestEnable, >> > > > - .StencilBufferWriteEnable = info->stencilTestEnable, >> > > > .StencilFailOp = vk_to_gen_stencil_op[info->front.failOp], >> > > > .StencilPassDepthPassOp = vk_to_gen_stencil_op[info-> >> > > front.passOp], >> > > > .StencilPassDepthFailOp = vk_to_gen_stencil_op[info-> >> > > front.depthFailOp], >> > > > -- >> > > > 2.5.0.400.gff86faf >> > > > >> > > > _______________________________________________ >> > > > mesa-dev mailing list >> > > > [email protected] >> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > > >> > >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
