[Mesa-dev] [PATCH 1/5] anv: Disable stencil writes when both write masks are zero
Nanley Chery
nanleychery at gmail.com
Wed Feb 8 20:30:01 UTC 2017
On Tue, Feb 07, 2017 at 04:53:51PM -0800, Jason Ekstrand wrote:
> On Tue, Feb 7, 2017 at 3:14 PM, Jason Ekstrand <jason at jlekstrand.net> 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.
>
Thanks for re-benchmarking the series! Could you update the last
sentence to reflect the new performance data? You could alternatively
omit the current data if you'd prefer.
With that change, this patch is
Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>
> --Jason
>
>
> > --Jason
> >
> >
> > On Tue, Feb 7, 2017 at 2:28 PM, Nanley Chery <nanleychery at 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 <nanleychery at 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 at lists.freedesktop.org
> >> > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >> > >
> >>
> >
> >
More information about the mesa-dev
mailing list