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.
--Jason
On Tue, Feb 7, 2017 at 2:28 PM, Nanley Chery <nanleych...@gmail.com> 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 <nanleych...@gmail.com>
> 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
> > > > 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
