<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 14, 2017 at 11:01 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 Tue, Feb 14, 2017 at 10:53:37AM -0800, Jason Ekstrand wrote: > On Tue, Feb 14, 2017 at 10:35 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> > wrote: > > > On Mon, Feb 13, 2017 at 08:45:47PM -0800, Jason Ekstrand wrote: > > > On Mon, Feb 13, 2017 at 6:13 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > > wrote: > > > > > > > On Mon, Feb 13, 2017 at 5:00 PM, Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> > > > > wrote: > > > > > > > >> On Fri, Feb 10, 2017 at 11:02:21AM -0800, Jason Ekstrand wrote: > > > >> > This improves the performance of Dota 2 on my Sky Lake Skull Canyon > > > >> > machine by about 2-3%. > > > >> > > > > >> > > > >> This patch looks good. Could we compare our setup and testing in the > > > >> office? I can't reproduce these numbers. > > > >> > > > > > > > > I'll have to do more runs. At the moment, I'm not seeing a large boost > > > > but I'll get a larger sample size and see if it's visible. > > > > > > > > > > I did a bunch more runs. It looks like it helps somewhere between 0 and > > > 1%. Not as much as I saw earlier, but it's definitely non-negative. > > > > > > > Although I've confirmed that the fix is enabled in Dota 2, I'm also not > > seeing any major changes on my SKL GT4. I get positive and negative > > changes in FPS, but it's all < 1% and seemingly noise. > > > > With the commit message updated with your new findings, this patch is > > Reviewed-by: Nanley Chery <<a href="mailto:nanley.g.chery@intel.com">nanley.g.chery@intel.com</a>> > > > > Here's what I wrote: > > Unfortunately, this doesn't substantially improve the performance of any > known apps. With Dota 2 on my Sky Lake gt4, it seems help by somewhere > between 0% and 1% but there's enough noise that it's hard to get a clear > picture. > </div></div>Looks good to me. </blockquote><div> </div><div>Cool! Thanks for your careful review! </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -Nanley <div class="HOEnZb"><div class="h5"> > > > > > --Jason > > > > > > > > > > --Jason > > > > > > > > > > > >> -Nanley > > > >> > > > >> > Reviewed-by: Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com">lionel.g.landwerlin@intel.com</a>> > > > >> > --- > > > >> > src/intel/vulkan/anv_private.h | 1 + > > > >> > src/intel/vulkan/gen8_cmd_buffer.c | 157 > > > >> ++++++++++++++++++++++++++++++++++++- > > > >> > src/intel/vulkan/genX_pipeline.c | 6 +- > > > >> > 3 files changed, 158 insertions(+), 6 deletions(-) > > > >> > > > > >> > diff --git a/src/intel/vulkan/anv_private.h > > > >> b/src/intel/vulkan/anv_private.h > > > >> > index fa6032e..25c9c93 100644 > > > >> > --- a/src/intel/vulkan/anv_private.h > > > >> > +++ b/src/intel/vulkan/anv_private.h > > > >> > @@ -1482,6 +1482,7 @@ struct anv_pipeline { > > > >> > bool writes_depth; > > > >> > bool depth_test_enable; > > > >> > bool writes_stencil; > > > >> > + bool > > stencil_test_enable; > > > >> > bool depth_clamp_enable; > > > >> > bool kill_pixel; > > > >> > > > > >> > diff --git a/src/intel/vulkan/gen8_cmd_buffer.c > > > >> b/src/intel/vulkan/gen8_cmd_buffer.c > > > >> > index 0628f3a..59fd537 100644 > > > >> > --- a/src/intel/vulkan/gen8_cmd_buffer.c > > > >> > +++ b/src/intel/vulkan/gen8_cmd_buffer.c > > > >> > @@ -157,16 +157,39 @@ __emit_sf_state(struct anv_cmd_buffer > > *cmd_buffer) > > > >> > 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; > > > >> > > > > >> > + cmd_buffer->state.pma_fix_enabled = enable; > > > >> > + > > > >> > + /* According to the Broadwell PIPE_CONTROL documentation, > > software > > > >> should > > > >> > + * emit a PIPE_CONTROL with the CS Stall and Depth Cache Flush > > bits > > > >> set > > > >> > + * prior to the LRI. If stencil buffer writes are enabled, > > then a > > > >> Render > > > >> > + * Cache Flush is also necessary. > > > >> > + * > > > >> > + * The Skylake docs say to use a depth stall rather than a > > command > > > >> > + * streamer stall. However, the hardware seems to violently > > > >> disagree. > > > >> > + * A full command streamer stall seems to be needed in both > > cases. > > > >> > + */ > > > >> > anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) { > > > >> > pc.DepthCacheFlushEnable = true; > > > >> > pc.CommandStreamerStallEnable = true; > > > >> > pc.RenderTargetCacheFlushEnable = true; > > > >> > } > > > >> > > > > >> > +#if GEN_GEN == 9 > > > >> > + > > > >> > + uint32_t cache_mode; > > > >> > + anv_pack_struct(&cache_mode, GENX(CACHE_MODE_0), > > > >> > + .STCPMAOptimizationEnable = enable, > > > >> > + .STCPMAOptimizationEnableMask = true); > > > >> > + anv_batch_emit(&cmd_buffer->batch, GENX(MI_LOAD_REGISTER_IMM), > > > >> lri) { > > > >> > + lri.RegisterOffset = GENX(CACHE_MODE_0_num); > > > >> > + lri.DataDWord = cache_mode; > > > >> > + } > > > >> > + > > > >> > +#elif GEN_GEN == 8 > > > >> > + > > > >> > uint32_t cache_mode; > > > >> > anv_pack_struct(&cache_mode, GENX(CACHE_MODE_1), > > > >> > .NPPMAFixEnable = enable, > > > >> > @@ -178,18 +201,20 @@ genX(cmd_buffer_enable_pma_fix)(struct > > > >> anv_cmd_buffer *cmd_buffer, bool enable) > > > >> > lri.DataDWord = cache_mode; > > > >> > } > > > >> > > > > >> > +#endif /* GEN_GEN == 8 */ > > > >> > + > > > >> > /* 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. > > > >> > + * > > > >> > + * Again, the Skylake docs give a different set of flushes but > > the > > > >> BDW > > > >> > + * flushes seem to work just as well. > > > >> > */ > > > >> > 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 > > > >> > @@ -281,6 +306,126 @@ want_depth_pma_fix(struct anv_cmd_buffer > > > >> *cmd_buffer) > > > >> > wm_prog_data->computed_depth_mode != PSCDEPTH_OFF; > > > >> > } > > > >> > > > > >> > +static inline bool > > > >> > +want_stencil_pma_fix(struct anv_cmd_buffer *cmd_buffer) > > > >> > +{ > > > >> > + assert(GEN_GEN == 9); > > > >> > + > > > >> > + /* From the Skylake PRM Vol. 2c CACHE_MODE_1::STC PMA > > Optimization > > > >> Enable: > > > >> > + * > > > >> > + * Clearing this bit will force the STC cache to wait for > > pending > > > >> > + * retirement of pixels at the HZ-read stage and do the > > STC-test > > > >> for > > > >> > + * Non-promoted, R-computed and Computed depth modes instead > > of > > > >> > + * postponing the STC-test to RCPFE. > > > >> > + * > > > >> > + * STC_TEST_EN = 3DSTATE_STENCIL_BUFFER:: > > STENCIL_BUFFER_ENABLE > > > >> && > > > >> > + * 3DSTATE_WM_DEPTH_STENCIL::StencilTestEnable > > > >> > + * > > > >> > + * STC_WRITE_EN = 3DSTATE_STENCIL_BUFFER:: > > STENCIL_BUFFER_ENABLE > > > >> && > > > >> > + * (3DSTATE_WM_DEPTH_STENCIL::Stencil Buffer > > > >> Write Enable && > > > >> > + * 3DSTATE_DEPTH_BUFFER::STENCIL_ > > WRITE_ENABLE) > > > >> > + * > > > >> > + * COMP_STC_EN = STC_TEST_EN && > > > >> > + * 3DSTATE_PS_EXTRA:: > > PixelShaderComputesStencil > > > >> > + * > > > >> > + * SW parses the pipeline states to generate the following > > > >> logical > > > >> > + * signal indicating if PMA FIX can be enabled. > > > >> > + * > > > >> > + * STC_PMA_OPT = > > > >> > + * 3DSTATE_WM::ForceThreadDispatch != 1 && > > > >> > + * !(3DSTATE_RASTER::ForceSampleCount != > > NUMRASTSAMPLES_0) && > > > >> > + * 3DSTATE_DEPTH_BUFFER::SURFACE_TYPE != NULL && > > > >> > + * 3DSTATE_DEPTH_BUFFER::HIZ Enable && > > > >> > + * !(3DSTATE_WM::EDSC_Mode == 2) && > > > >> > + * 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) && > > > >> > + * (COMP_STC_EN || STC_WRITE_EN) && > > > >> > + * ((3DSTATE_PS_EXTRA::PixelShaderKillsPixels || > > > >> > + * 3DSTATE_WM::ForceKillPix == ON || > > > >> > + * 3DSTATE_PS_EXTRA::oMask Present to RenderTarget || > > > >> > + * 3DSTATE_PS_BLEND::AlphaToCoverageEnable || > > > >> > + * 3DSTATE_PS_BLEND::AlphaTestEnable || > > > >> > + * 3DSTATE_WM_CHROMAKEY::ChromaKeyKillEnable) || > > > >> > + * (3DSTATE_PS_EXTRA::Pixel Shader Computed Depth mode != > > > >> PSCDEPTH_OFF)) > > > >> > + */ > > > >> > + > > > >> > + /* These are always true: > > > >> > + * 3DSTATE_WM::ForceThreadDispatch != 1 && > > > >> > + * !(3DSTATE_RASTER::ForceSampleCount != NUMRASTSAMPLES_0) > > > >> > + */ > > > >> > + > > > >> > + /* 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; > > > >> > + > > > >> > + /* We can't possibly know if HiZ is enabled without the > > framebuffer > > > >> */ > > > >> > + assert(cmd_buffer->state.framebuffer); > > > >> > + > > > >> > + /* HiZ is enabled so we had better have a depth buffer with HiZ > > */ > > > >> > + const struct anv_image_view *ds_iview = > > > >> > + anv_cmd_buffer_get_depth_stencil_view(cmd_buffer); > > > >> > + assert(ds_iview && ds_iview->image->aux_usage == > > ISL_AUX_USAGE_HIZ); > > > >> > + > > > >> > + /* 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 == 2) */ > > > >> > + const struct brw_wm_prog_data *wm_prog_data = > > > >> get_wm_prog_data(pipeline); > > > >> > + if (wm_prog_data->early_fragment_tests) > > > >> > + return false; > > > >> > + > > > >> > + /* We never use anv_pipeline for HiZ ops so this is trivially > > true: > > > >> > + * !(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_STENCIL_BUFFER::STENCIL_BUFFER_ENABLE && > > > >> > + * 3DSTATE_WM_DEPTH_STENCIL::StencilTestEnable > > > >> > + */ > > > >> > + const bool stc_test_en = > > > >> > + (ds_iview->image->aspects & VK_IMAGE_ASPECT_STENCIL_BIT) && > > > >> > + pipeline->stencil_test_enable; > > > >> > + > > > >> > + /* 3DSTATE_STENCIL_BUFFER::STENCIL_BUFFER_ENABLE && > > > >> > + * (3DSTATE_WM_DEPTH_STENCIL::Stencil Buffer Write Enable && > > > >> > + * 3DSTATE_DEPTH_BUFFER::STENCIL_WRITE_ENABLE) > > > >> > + */ > > > >> > + const bool stc_write_en = > > > >> > + (ds_iview->image->aspects & VK_IMAGE_ASPECT_STENCIL_BIT) && > > > >> > + pipeline->writes_stencil; > > > >> > + > > > >> > + /* STC_TEST_EN && 3DSTATE_PS_EXTRA::PixelShaderComputesStencil > > */ > > > >> > + const bool comp_stc_en = stc_test_en && > > > >> wm_prog_data->computed_stencil; > > > >> > + > > > >> > + /* COMP_STC_EN || STC_WRITE_EN */ > > > >> > + if (!(comp_stc_en || stc_write_en)) > > > >> > + return false; > > > >> > + > > > >> > + /* (3DSTATE_PS_EXTRA::PixelShaderKillsPixels || > > > >> > + * 3DSTATE_WM::ForceKillPix == ON || > > > >> > + * 3DSTATE_PS_EXTRA::oMask Present to RenderTarget || > > > >> > + * 3DSTATE_PS_BLEND::AlphaToCoverageEnable || > > > >> > + * 3DSTATE_PS_BLEND::AlphaTestEnable || > > > >> > + * 3DSTATE_WM_CHROMAKEY::ChromaKeyKillEnable) || > > > >> > + * (3DSTATE_PS_EXTRA::Pixel Shader Computed Depth mode != > > > >> PSCDEPTH_OFF) > > > >> > + */ > > > >> > + return pipeline->kill_pixel || > > > >> > + wm_prog_data->computed_depth_mode != PSCDEPTH_OFF; > > > >> > +} > > > >> > + > > > >> > void > > > >> > genX(cmd_buffer_flush_dynamic_state)(struct anv_cmd_buffer > > > >> *cmd_buffer) > > > >> > { > > > >> > @@ -390,6 +535,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 | > > > >> > ANV_CMD_DIRTY_DYNAMIC_STENCIL_ > > COMPARE_MASK > > > >> | > > > >> > ANV_CMD_DIRTY_DYNAMIC_STENCIL_ > > WRITE_MASK > > > >> | > > > >> > ANV_CMD_DIRTY_DYNAMIC_STENCIL_ > > REFERENCE)) > > > >> { > > > >> > @@ -415,6 +561,9 @@ genX(cmd_buffer_flush_dynamic_state)(struct > > > >> anv_cmd_buffer *cmd_buffer) > > > >> > > > > >> > anv_batch_emit_merge(&cmd_buffer->batch, dwords, > > > >> > pipeline->gen9.wm_depth_stencil); > > > >> > + > > > >> > + genX(cmd_buffer_enable_pma_fix)(cmd_buffer, > > > >> > + > > want_stencil_pma_fix(cmd_buffe > > > >> r)); > > > >> > } > > > >> > #endif > > > >> > > > > >> > diff --git a/src/intel/vulkan/genX_pipeline.c > > > >> b/src/intel/vulkan/genX_pipeline.c > > > >> > index d2af8b9..3b0ae08 100644 > > > >> > --- a/src/intel/vulkan/genX_pipeline.c > > > >> > +++ b/src/intel/vulkan/genX_pipeline.c > > > >> > @@ -784,6 +784,7 @@ emit_ds_state(struct anv_pipeline *pipeline, > > > >> > * to make sure it's initialized to something useful. > > > >> > */ > > > >> > pipeline->writes_stencil = false; > > > >> > + pipeline->stencil_test_enable = false; > > > >> > pipeline->writes_depth = false; > > > >> > pipeline->depth_test_enable = false; > > > >> > memset(depth_stencil_dw, 0, sizeof(depth_stencil_dw)); > > > >> > @@ -799,6 +800,7 @@ emit_ds_state(struct anv_pipeline *pipeline, > > > >> > > > > >> > VkPipelineDepthStencilStateCreateInfo info = *pCreateInfo; > > > >> > sanitize_ds_state(&info, &pipeline->writes_stencil, ds_aspects); > > > >> > + pipeline->stencil_test_enable = info.stencilTestEnable; > > > >> > pipeline->writes_depth = info.depthWriteEnable; > > > >> > pipeline->depth_test_enable = info.depthTestEnable; > > > >> > > > > >> > @@ -1556,8 +1558,8 @@ compute_kill_pixel(struct anv_pipeline > > *pipeline, > > > >> > 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: > > > >> > + * not we want to enable the PMA fix on gen8 or gen9. It's > > given > > > >> by this > > > >> > + * chunk of the giant formula: > > > >> > * > > > >> > * (3DSTATE_PS_EXTRA::PixelShaderKillsPixels || > > > >> > * 3DSTATE_PS_EXTRA::oMask Present to RenderTarget || > > > >> > -- > > > >> > 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>