[Mesa-dev] [PATCH 1/5] anv: Disable stencil writes when both write masks are zero
Jason Ekstrand
jason at jlekstrand.net
Wed Feb 8 00:53:51 UTC 2017
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.
--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
>> > >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170207/d763797c/attachment-0001.html>
More information about the mesa-dev
mailing list