<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 9, 2017 at 10:33 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Feb 08, 2017 at 06:27:52PM -0800, Jason Ekstrand wrote: > On Wed, Feb 8, 2017 at 5:34 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote: > > > On Thu, Feb 02, 2017 at 01:26:05PM -0800, Jason Ekstrand wrote: > > > In order to get good performance numbers for this, I had to hack up the > > > driver to whack wm_prog_data::uses_kill to true to emulate a discard and > > > used the Sascha "shadowmapping" demo. Setting uses_kill to true dropped > > > the framerate on the demo by 25-30%. Enabling the PMA fix brought it > > > back up to around 90% of the original framerate. This doesn't seem to > > > really impact Dota 2; probably because it doesn't use 16-bit depth. > > > > > > Reviewed-by: Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com">lionel.g.landwerlin@intel.com</a>> > > > --- > > > src/intel/vulkan/TODO | 1 - > > > src/intel/vulkan/anv_cmd_buffer.c | 2 + > > > src/intel/vulkan/anv_genX.h | 3 + > > > src/intel/vulkan/anv_private.h | 17 +++++ > > > src/intel/vulkan/gen7_cmd_buffer.c | 7 ++ > > > src/intel/vulkan/gen8_cmd_buffer.c | 133 ++++++++++++++++++++++++++++++ > > +++++++ > > > src/intel/vulkan/genX_blorp_exec.c | 5 ++ > > > src/intel/vulkan/genX_cmd_buffer.c | 15 ++++- > > > src/intel/vulkan/genX_pipeline.c | 38 +++++++++++ > > > 9 files changed, 219 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/intel/vulkan/TODO b/src/intel/vulkan/TODO > > > index 38acc0d..f8b73a1 100644 > > > --- a/src/intel/vulkan/TODO > > > +++ b/src/intel/vulkan/TODO > > > @@ -12,5 +12,4 @@ Performance: > > > - Compressed multisample support > > > - Pushing pieces of UBOs? > > > - Enable guardband clipping > > > - - pma stall workaround > > > - Use soft-pin to avoid relocations > > > diff --git a/src/intel/vulkan/anv_cmd_buffer.c > > b/src/intel/vulkan/anv_cmd_buffer.c > > > index 5886fa6..8c08f8d 100644 > > > --- a/src/intel/vulkan/anv_cmd_buffer.c > > > +++ b/src/intel/vulkan/anv_cmd_buffer.c > > > @@ -135,6 +135,8 @@ anv_cmd_state_reset(struct anv_cmd_buffer > > *cmd_buffer) > > > state->restart_index = UINT32_MAX; > > > state->dynamic = default_dynamic_state; > > > state->need_query_wa = true; > > > + state->pma_fix_enabled = false; > > > + state->hiz_enabled = false; > > > > > > if (state->attachments != NULL) { > > > vk_free(&cmd_buffer->pool->alloc, state->attachments); > > > diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h > > > index d04fe38..67147b0 100644 > > > --- a/src/intel/vulkan/anv_genX.h > > > +++ b/src/intel/vulkan/anv_genX.h > > > @@ -55,6 +55,9 @@ void genX(cmd_buffer_flush_dynamic_state)(struct > > anv_cmd_buffer *cmd_buffer); > > > > > > void genX(cmd_buffer_flush_compute_state)(struct anv_cmd_buffer > > *cmd_buffer); > > > > > > +void genX(cmd_buffer_enable_pma_fix)(struct anv_cmd_buffer *cmd_buffer, > > > + bool enable); > > > + > > > void > > > genX(emit_urb_setup)(struct anv_device *device, struct anv_batch *batch, > > > const struct gen_l3_config *l3_config, > > > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_ > > private.h > > > index 4fe3ebc..6efe4ea 100644 > > > --- a/src/intel/vulkan/anv_private.h > > > +++ b/src/intel/vulkan/anv_private.h > > > @@ -1163,6 +1163,20 @@ struct anv_cmd_state { > > > bool need_query_wa; > > > > > > /** > > > + * Whether or not the gen8 PMA fix is enabled. We ensure that, at > > the top > > > + * of any command buffer it disabled by disabling it in > > EndCommandBuffer > > ^ > > is? > > </div></div>Fixed? </blockquote><div> </div><div>done </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > > + * and before invoking the secondary in ExecuteCommands. > > > + */ > > > + bool pma_fix_enabled; > > > + > > > + /** > > > + * Whether or not we now for certain that HiZ is enabled for the > > current > > ^ > > know > > Fixed? </blockquote><div> </div><div>done </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > > > + * subpass. If, for whatever reason, we are unsure as to whether > > HiZ is > > > + * enabled or not, this will be false. > > > + */ > > > + bool hiz_enabled; > > > + > > > + /** > > > * Array length is anv_cmd_state::pass::attachment_count. Array > > content is > > > * valid only when recording a render pass instance. > > > */ > > > @@ -1465,8 +1479,11 @@ struct anv_pipeline { > > > > > > uint32_t cs_right_mask; > > > > > > + bool writes_depth; > > > + bool depth_test_enable; > > > bool writes_stencil; > > > bool depth_clamp_enable; > > > + bool kill_pixel; > > > > > > struct { > > > uint32_t sf[7]; > > > diff --git a/src/intel/vulkan/gen7_cmd_buffer.c > > b/src/intel/vulkan/gen7_cmd_buffer.c > > > index 013ed87..c1a25e8 100644 > > > --- a/src/intel/vulkan/gen7_cmd_buffer.c > > > +++ b/src/intel/vulkan/gen7_cmd_buffer.c > > > @@ -260,6 +260,13 @@ genX(cmd_buffer_flush_dynamic_state)(struct > > anv_cmd_buffer *cmd_buffer) > > > cmd_buffer->state.dirty = 0; > > > } > > > > > > +void > > > +genX(cmd_buffer_enable_pma_fix)(struct anv_cmd_buffer *cmd_buffer, > > > + bool enable) > > > +{ > > > + /* The NP PMA fix doesn't exist on gen7 */ > > > +} > > > + > > > void genX(CmdSetEvent)( > > > VkCommandBuffer commandBuffer, > > > VkEvent event, > > > diff --git a/src/intel/vulkan/gen8_cmd_buffer.c > > b/src/intel/vulkan/gen8_cmd_buffer.c > > > index 8c8de62..271ab3f 100644 > > > --- a/src/intel/vulkan/gen8_cmd_buffer.c > > > +++ b/src/intel/vulkan/gen8_cmd_buffer.c > > > @@ -155,6 +155,135 @@ __emit_sf_state(struct anv_cmd_buffer *cmd_buffer) > > > #endif > > > > > > void > > > +genX(cmd_buffer_enable_pma_fix)(struct anv_cmd_buffer *cmd_buffer, > > bool enable) > > > +{ > > > +#if GEN_GEN == 8 > > > + if (cmd_buffer->state.pma_fix_enabled == enable) > > > + return; > > > + > > > + anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) { > > > + pc.DepthCacheFlushEnable = true; > > > + pc.CommandStreamerStallEnable = true; > > > + pc.RenderTargetCacheFlushEnable = true; > > > > Instead of flushing the depth and RT caches, what do you think about > > implementing a stall that writes 0 to the workaround bo? That should > > consume less bandwidth. > > > > To make the pipeline idle, flushing the caches shouldn't be necessary, > > we should be able to do it with a valid pipecontrol packet that has the > > CS stall bit set. > > > > Without having an extremely good idea of how these bits interact with > caching, I'm very reluctant to try too hard to over-optimize the pipe > control. > > </div></div>Okay. I don't have any data on the impact of cache flushes, so I don't know if this is a micro-optimization or not. In any case, we are seeing a performance benefit, so I won't focus on the pipe control. > > The BDW PRM states: > > > > If the stall bit is set, the command streamer waits until the pipe is > > completely flushed. > > > > It waits until whatever flush/stall operation you gave it is finished. You > also need to specify a stall/flush operation. If you just do a depth stall You could alternatively specify a Post-Sync operation [BDW]: One of the following must also be set: • Render Target Cache Flush Enable ([12] of DW1) • Depth Cache Flush Enable ([0] of DW1) • Stall at Pixel Scoreboard ([1] of DW1) • Depth Stall ([13] of DW1) ---> • Post-Sync Operation ([13] of DW1) • DC Flush Enable ([5] of DW1) <div><div class="h5"> > for instance and no depth reads/writes are in-flight, I don't think it > actually stalls at all. We may be able to use StallAtPixelScoreboard > instead but, again, I don't know for sure how this interacts with caches. > > > > > + } > > > + > > > + uint32_t cache_mode; > > > + anv_pack_struct(&cache_mode, GENX(CACHE_MODE_1), > > > + .NPPMAFixEnable = enable, > > > + .NPEarlyZFailsDisableMask = enable, > > ^ > > I don't think you intended to set the mask here. It should be: > > > > .NPEarlyZFailsDisable = enable, > > > > Does fixing this impact your FPS measurements significantly? > > > > Thanks for catching that! I'm a bit surprised that it worked at all. I'll > definitely re-measure. > > > > > + .NPPMAFixEnableMask = true, > > > + .NPEarlyZFailsDisableMask = true); > > > + anv_batch_emit(&cmd_buffer->batch, GENX(MI_LOAD_REGISTER_IMM), lri) > > { > > > + lri.RegisterOffset = GENX(CACHE_MODE_1_num); > > > + lri.DataDWord = cache_mode; > > > + } > > > + > > > + /* After the LRI, a PIPE_CONTROL with both the Depth Stall and Depth > > Cache > > > + * Flush bits is often necessary. We do it regardless because it's > > easier. > > > + * The render cache flush is also necessary if stencil writes are > > enabled. > > > + */ > > > > I couldn't find a requirement to flush these specific caches in the > > docs, but I did find: > > > > Both of these flushes are documented in the bspec (not PRM) docs for > PIPE_CONTROL. > > > > To ensure this command gets executed before upcoming commands in the > > ring, either a stalling pipecontrol should be sent after this > > command, or MMIO 0x20C0 bit 7 should be set to 1. > > > > Perhaps we could use the flush I suggested earlier? > > > > > + anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) { > > > + pc.DepthStallEnable = true; > > > + pc.DepthCacheFlushEnable = true; > > > + pc.RenderTargetCacheFlushEnable = true; > > > + } > > > + > > > + cmd_buffer->state.pma_fix_enabled = enable; > > > +#endif /* GEN_GEN == 8 */ > > > +} > > > + > > > +static inline bool > > > +want_depth_pma_fix(struct anv_cmd_buffer *cmd_buffer) > > > +{ > > > + assert(GEN_GEN == 8); > > > + > > > + /* From the Broadwell PRM Vol. 2c CACHE_MODE_1::NP_PMA_FIX_ENABLE: > > > + * > > > + * SW must set this bit in order to enable this fix when following > > > + * expression is TRUE. > > > + * > > > + * 3DSTATE_WM::ForceThreadDispatch != 1 && > > > + * !(3DSTATE_RASTER::ForceSampleCount != NUMRASTSAMPLES_0) && > > > + * (3DSTATE_DEPTH_BUFFER::SURFACE_TYPE != NULL) && > > > + * (3DSTATE_DEPTH_BUFFER::HIZ Enable) && > > > + * !(3DSTATE_WM::EDSC_Mode == EDSC_PREPS) && > > > + * (3DSTATE_PS_EXTRA::PixelShaderValid) && > > > + * !(3DSTATE_WM_HZ_OP::DepthBufferClear || > > > + * 3DSTATE_WM_HZ_OP::DepthBufferResolve || > > > + * 3DSTATE_WM_HZ_OP::Hierarchical Depth Buffer Resolve Enable || > > > + * 3DSTATE_WM_HZ_OP::StencilBufferClear) && > > > + * (3DSTATE_WM_DEPTH_STENCIL::DepthTestEnable) && > > > + * (((3DSTATE_PS_EXTRA::PixelShaderKillsPixels || > > > + * 3DSTATE_PS_EXTRA::oMask Present to RenderTarget || > > > + * 3DSTATE_PS_BLEND::AlphaToCoverageEnable || > > > + * 3DSTATE_PS_BLEND::AlphaTestEnable || > > > + * 3DSTATE_WM_CHROMAKEY::ChromaKeyKillEnable) && > > > + * 3DSTATE_WM::ForceKillPix != ForceOff && > > > + * ((3DSTATE_WM_DEPTH_STENCIL::DepthWriteEnable && > > > + * 3DSTATE_DEPTH_BUFFER::DEPTH_WRITE_ENABLE) || > > > + * (3DSTATE_WM_DEPTH_STENCIL::Stencil Buffer Write Enable && > > > + * 3DSTATE_DEPTH_BUFFER::STENCIL_WRITE_ENABLE && > > > + * 3DSTATE_STENCIL_BUFFER::STENCIL_BUFFER_ENABLE))) || > > > + * (3DSTATE_PS_EXTRA:: Pixel Shader Computed Depth mode != > > PSCDEPTH_OFF)) > > > + * > > > + * This function only takes care of the pipeline parts of the > > equation. > > > > Which parts are the pipeline parts? It looks like you take care of the > > entire equation in this function. > > > > Yes, I do. > > </div></div>Since you do, can we omit the sentence? </blockquote><div> </div><div>Sure. Done. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > > + */ > > > + > > > + /* These are always true: > > > + * 3DSTATE_WM::ForceThreadDispatch != 1 && > > > + * !(3DSTATE_RASTER::ForceSampleCount != NUMRASTSAMPLES_0) > > > + */ > > > > I couldn't quite understand what ForcedSampleCount does from looking at > > the HW docs. If you know, could you tell me what this does and why it > > wouldn't change under any circumstance in the future? Otherwise, I > > think we should put an assertion where we assign ForcedSampleCount and > > reference this PMA optimization. > > > > Honestly, I'm not 100% sure what it's for. I think it lets you make the > rasterizer run at a different number of samples than the underlying > surface. We've never used this bit in Vulkan or GL. Also, there's no flag > for us to assert on. > > We can put an assertion below and/or a comment above this line: genX_pipeline.c: raster.ForcedSampleCount = FSC_NUMRASTSAMPLES_0; <div><div class="h5"></div></div></blockquote><div> </div><div>Sure, I'll add a comment so that we know to update the PMA fix if we ever change it. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > > > + > > > + /* We only enable the PMA fix if we know for certain that HiZ is > > enabled. > > > + * If we don't know whether HiZ is enabled or not, we disable the > > PMA fix > > > + * and there is no harm. > > > + * > > > + * (3DSTATE_DEPTH_BUFFER::SURFACE_TYPE != NULL) && > > > + * 3DSTATE_DEPTH_BUFFER::HIZ Enable > > > + */ > > > + if (!cmd_buffer->state.hiz_enabled) > > > + return false; > > > + > > > + /* 3DSTATE_PS_EXTRA::PixelShaderValid */ > > > + struct anv_pipeline *pipeline = cmd_buffer->state.pipeline; > > > + if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_FRAGMENT)) > > > + return false; > > > + > > > + /* !(3DSTATE_WM::EDSC_Mode == EDSC_PREPS) */ > > > + const struct brw_wm_prog_data *wm_prog_data = > > get_wm_prog_data(pipeline); > > > + if (wm_prog_data->early_fragment_tests) > > > + return false; > > > > In keeping with your top-to-bottom evaluation of the logical statement, > > could you move this part to be above the PixelShaderValid part? > > > > Not really. If we don't' have a fragment shader, we wm_prog_data is NULL > > </div></div>In that case couldn't we do the following? if (wm_prog_data && wm_prog_data->early_fragment_tests) return false; </blockquote><div> </div><div>I suppose... Seems easier just to check them in the other order. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > > + > > > + /* We never use anv_pipeline for HiZ ops so this is trivially true: <div><div class="h5">> > > + * !(3DSTATE_WM_HZ_OP::DepthBufferClear || > > ^ > > misaligned asterisk > > > > Fixed. > > > > > + * 3DSTATE_WM_HZ_OP::DepthBufferResolve || > > > + * 3DSTATE_WM_HZ_OP::Hierarchical Depth Buffer Resolve Enable || > > > + * 3DSTATE_WM_HZ_OP::StencilBufferClear) > > > + */ > > > + > > > + /* 3DSTATE_WM_DEPTH_STENCIL::DepthTestEnable */ > > > + if (!pipeline->depth_test_enable) > > > + return false; > > > + > > > + /* (((3DSTATE_PS_EXTRA::PixelShaderKillsPixels || > > > + * 3DSTATE_PS_EXTRA::oMask Present to RenderTarget || > > > + * 3DSTATE_PS_BLEND::AlphaToCoverageEnable || > > > + * 3DSTATE_PS_BLEND::AlphaTestEnable || > > > + * 3DSTATE_WM_CHROMAKEY::ChromaKeyKillEnable) && > > > + * 3DSTATE_WM::ForceKillPix != ForceOff && > > > + * ((3DSTATE_WM_DEPTH_STENCIL::DepthWriteEnable && > > > + * 3DSTATE_DEPTH_BUFFER::DEPTH_WRITE_ENABLE) || > > > + * (3DSTATE_WM_DEPTH_STENCIL::Stencil Buffer Write Enable && > > > + * 3DSTATE_DEPTH_BUFFER::STENCIL_WRITE_ENABLE && > > > + * 3DSTATE_STENCIL_BUFFER::STENCIL_BUFFER_ENABLE))) || > > > + * (3DSTATE_PS_EXTRA:: Pixel Shader Computed Depth mode != > > PSCDEPTH_OFF)) > > > + */ > > > + return (pipeline->kill_pixel && (pipeline->writes_depth || > > > + pipeline->writes_stencil)) || > > > > anv_pipeline::writes_stencil != > > (3DSTATE_WM_DEPTH_STENCIL::Stencil Buffer Write Enable && > > 3DSTATE_DEPTH_BUFFER::STENCIL_WRITE_ENABLE && > > 3DSTATE_STENCIL_BUFFER::STENCIL_BUFFER_ENABLE); > > > > The last two lines are not accounted for - we don't detect if the > > currently bound depth_stencil attachment lacks or possesses a stencil > > buffer. > > > > We should be checking for the presence of a stencil aspect in the > pipeline. I think that got missed when I added the writes_stencil bit. We > do by the end of the series, but I'll patch up the disable stencil writes > patch to make it check. > > </div></div>Sounds good. > > This is a valid scenario according to section 25.9. Stencil Test of the > > Vulkan spec, which states: > > > > If there is no stencil framebuffer attachment, stencil modification > > cannot occur, and it is as if the stencil tests always pass. > > > > anv_pipeline::writes_depth should be correct because hiz_enabled is true > > by the time we reach it. For hiz_enabled to be true, there must be a > > depth buffer present. > > > > Yup > > > > > + wm_prog_data->computed_depth_mode != PSCDEPTH_OFF; > > > +} > > > + > > > +void > > > genX(cmd_buffer_flush_dynamic_state)(struct anv_cmd_buffer *cmd_buffer) > > > { > > > struct anv_pipeline *pipeline = cmd_buffer->state.pipeline; > > > @@ -211,6 +340,7 @@ genX(cmd_buffer_flush_dynamic_state)(struct > > anv_cmd_buffer *cmd_buffer) > > > } > > > > > > if (cmd_buffer->state.dirty & (ANV_CMD_DIRTY_PIPELINE | > > > + ANV_CMD_DIRTY_RENDER_TARGETS | > > > > Why is this necessary? > > > > We set RENDER_TARGETS dirty when the depth buffer changes due to > NextSubpass or BeginCommandBuffer > > I'm familiar with when this flag gets set, but I don't see what case this catches over the already present bits. </blockquote><div> </div><div>We look at the framebuffer and subpass to determine whether or not HiZ is enabled. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > > ANV_CMD_DIRTY_DYNAMIC_STENCIL_COMPARE_MASK > > | > > > ANV_CMD_DIRTY_DYNAMIC_STENCIL_WRITE_MASK)) > > { > > > uint32_t wm_depth_stencil_dw[GENX(3DSTATE_WM_DEPTH_STENCIL_ <div><div class="h5">> > length)]; > > > @@ -234,6 +364,9 @@ genX(cmd_buffer_flush_dynamic_state)(struct > > anv_cmd_buffer *cmd_buffer) > > > > > > anv_batch_emit_merge(&cmd_buffer->batch, wm_depth_stencil_dw, > > > pipeline->gen8.wm_depth_stencil); > > > + > > > + genX(cmd_buffer_enable_pma_fix)(cmd_buffer, > > > + want_depth_pma_fix(cmd_buffer)); > > > } > > > #else > > > if (cmd_buffer->state.dirty & ANV_CMD_DIRTY_DYNAMIC_BLEND_CONSTANTS) > > { > > > diff --git a/src/intel/vulkan/genX_blorp_exec.c > > b/src/intel/vulkan/genX_blorp_exec.c > > > index 663e6c9..6f0b063 100644 > > > --- a/src/intel/vulkan/genX_blorp_exec.c > > > +++ b/src/intel/vulkan/genX_blorp_exec.c > > > @@ -154,6 +154,11 @@ genX(blorp_exec)(struct blorp_batch *batch, > > > > > > genX(cmd_buffer_emit_gen7_depth_flush)(cmd_buffer); > > > > > > + /* BLORP doesn't do anything fancy with depth such as discards, so > > we want > > > + * the PMA fix off. Also, off is always the safe option. > > > + */ > > > + genX(cmd_buffer_enable_pma_fix)(cmd_buffer, false); > > > + > > > blorp_exec(batch, params); > > > > > > cmd_buffer->state.vb_dirty = ~0; > > > diff --git a/src/intel/vulkan/genX_cmd_buffer.c > > b/src/intel/vulkan/genX_cmd_buffer.c > > > index b6b7f74..66de93a 100644 > > > --- a/src/intel/vulkan/genX_cmd_buffer.c > > > +++ b/src/intel/vulkan/genX_cmd_buffer.c > > > @@ -627,6 +627,11 @@ genX(EndCommandBuffer)( > > > { > > > ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer); > > > > > > + /* We want every command buffer to start with the PMA fix in a known > > state, > > > + * so we disable it at the end of the command buffer. > > > + */ > > > + genX(cmd_buffer_enable_pma_fix)(cmd_buffer, false); > > > + > > > genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer); > > > > > > anv_cmd_buffer_end_batch_buffer(cmd_buffer); > > > @@ -644,6 +649,11 @@ genX(CmdExecuteCommands)( > > > > > > assert(primary->level == VK_COMMAND_BUFFER_LEVEL_PRIMARY); > > > > > > + /* The secondary command buffers will assume that the PMA fix is > > disabled > > > + * when they begin executing. Make sure this is true. > > > + */ > > > + genX(cmd_buffer_enable_pma_fix)(primary, false); > > > + > > > for (uint32_t i = 0; i < commandBufferCount; i++) { > > > ANV_FROM_HANDLE(anv_cmd_buffer, secondary, pCmdBuffers[i]); > > > > > > @@ -2181,7 +2191,8 @@ cmd_buffer_emit_depth_stencil(struct > > anv_cmd_buffer *cmd_buffer) > > > const bool has_stencil = > > > image && (image->aspects & VK_IMAGE_ASPECT_STENCIL_BIT); > > > > > > - /* FIXME: Implement the PMA stall W/A */ > > > + cmd_buffer->state.hiz_enabled = has_hiz; > > > + > > > /* FIXME: Width and Height are wrong */ > > > > > > genX(cmd_buffer_emit_gen7_depth_flush)(cmd_buffer); > > > @@ -2419,6 +2430,8 @@ void genX(CmdEndRenderPass)( > > > > > > anv_cmd_buffer_resolve_subpass(cmd_buffer); > > > > > > + cmd_buffer->state.hiz_enabled = false; > > > + > > > #ifndef NDEBUG > > > anv_dump_add_framebuffer(cmd_buffer, cmd_buffer->state.framebuffer); > > > #endif > > > diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_ > > pipeline.c > > > index 18fe48c..0588d01 100644 > > > --- a/src/intel/vulkan/genX_pipeline.c > > > +++ b/src/intel/vulkan/genX_pipeline.c > > > @@ -653,6 +653,8 @@ emit_ds_state(struct anv_pipeline *pipeline, > > > * to make sure it's initialized to something useful. > > > */ > > > pipeline->writes_stencil = false; > > > + pipeline->writes_depth = false; > > > + pipeline->depth_test_enable = false; > > > memset(depth_stencil_dw, 0, sizeof(depth_stencil_dw)); > > > return; > > > } > > > @@ -711,6 +713,9 @@ emit_ds_state(struct anv_pipeline *pipeline, > > > 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 > > > @@ -1429,6 +1434,38 @@ emit_3dstate_vf_topology(struct anv_pipeline > > *pipeline) > > > } > > > #endif > > > > > > +static void > > > +compute_kill_pixel(struct anv_pipeline *pipeline, > > > + const VkPipelineMultisampleStateCreateInfo *ms_info, > > > + const struct anv_subpass *subpass) > > > +{ > > > + if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_FRAGMENT)) { > > > + pipeline->kill_pixel = false; > > > + return; > > > + } > > > + > > > + const struct brw_wm_prog_data *wm_prog_data = > > get_wm_prog_data(pipeline); > > > + > > > + /* This computes the KillPixel portion of the computation for > > whether or > > > + * not we want to enable the PMA fix on gen8. It's given by this > > chunk of > > > + * the giant formula: > > > + * > > > + * (3DSTATE_PS_EXTRA::PixelShaderKillsPixels || > > > + * 3DSTATE_PS_EXTRA::oMask Present to RenderTarget || > > > + * 3DSTATE_PS_BLEND::AlphaToCoverageEnable || > > > + * 3DSTATE_PS_BLEND::AlphaTestEnable || > > > + * 3DSTATE_WM_CHROMAKEY::ChromaKeyKillEnable) > > > + * > > > + * 3DSTATE_WM_CHROMAKEY::ChromaKeyKillEnable is always false and so > > is > > > + * 3DSTATE_PS_BLEND::AlphaTestEnable since Vulkan doesn't have a > > concept > > > + * of an alpha test. > > > + */ > > > + pipeline->kill_pixel = > > > + subpass->has_ds_self_dep || wm_prog_data->uses_kill || > > > > Why do we set kill_pixel in the presence of has_ds_self_dep? > > > > Section 7.1 of the Vulkan spec states: > > > > If a subpass uses the same attachment as both an input attachment and > > either a color attachment or a depth/stencil attachment, writes via > > the color or depth/stencil attachment are not automatically made > > visible to reads via the input attachment, causing a feedback loop, > > except in any of the following conditions: > > > > [...] > > > > I don't see how any of the conditions are satisfied through the use > > of kill_pixel. > > > > There's a really good comment where we set > 3DSTATE_PS::PixelShaderKillsPixel and > 3DSTATE_PS_EXTRA::PixelShaderKillsPixel > > </div></div>Sorry, I saw the comment earlier, but I didn't think about this long enough before commenting. Looks good. <div class="HOEnZb"><div class="h5"> > > > + wm_prog_data->uses_omask || > > > + (ms_info && ms_info->alphaToCoverageEnable); > > > +} > > > + > > > static VkResult > > > genX(graphics_pipeline_create)( > > > VkDevice _device, > > > @@ -1466,6 +1503,7 @@ genX(graphics_pipeline_create)( > > > emit_ds_state(pipeline, pCreateInfo->pDepthStencilState, pass, > > subpass); > > > emit_cb_state(pipeline, pCreateInfo->pColorBlendState, > > > pCreateInfo->pMultisampleState); > > > + compute_kill_pixel(pipeline, pCreateInfo->pMultisampleState, > > subpass); > > > > > > emit_urb_setup(pipeline); > > > > > > -- > > > 2.5.0.400.gff86faf > > > > > > _______________________________________________ > > > mesa-dev mailing list > > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > </div></div></blockquote></div> </div></div>