<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Aug 10, 2019 at 4:55 PM Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> writes: > On Sat, Aug 10, 2019 at 2:22 PM Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> > wrote: > >> Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> writes: >> >> > On Fri, Aug 9, 2019 at 7:22 PM Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> >> > wrote: >> > >> >> See "i965/gen9: Optimize slice and subslice load balancing behavior." >> >> for the rationale. Marked optional because no performance evaluation >> >> has been done on this commit, it is provided to match the hashing >> >> settings of the Iris driver. Test reports welcome. >> >> >> > >> > Improves Aztec Ruins by 2.7% (tested 5 times in each configuration). I >> > also tested Batman: Arkham City but it only reports integer FPS numbers >> and >> > no change was noticable. >> > >> > >> >> --- >> >> src/intel/vulkan/anv_genX.h | 4 ++ >> >> src/intel/vulkan/anv_private.h | 6 ++ >> >> src/intel/vulkan/genX_blorp_exec.c | 6 ++ >> >> src/intel/vulkan/genX_cmd_buffer.c | 96 ++++++++++++++++++++++++++++++ >> >> 4 files changed, 112 insertions(+) >> >> >> >> diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h >> >> index a5435e566a3..06c6b467acf 100644 >> >> --- a/src/intel/vulkan/anv_genX.h >> >> +++ b/src/intel/vulkan/anv_genX.h >> >> @@ -44,6 +44,10 @@ void genX(cmd_buffer_apply_pipe_flushes)(struct >> >> anv_cmd_buffer *cmd_buffer); >> >> >> >> void genX(cmd_buffer_emit_gen7_depth_flush)(struct anv_cmd_buffer >> >> *cmd_buffer); >> >> >> >> +void genX(cmd_buffer_emit_hashing_mode)(struct anv_cmd_buffer >> *cmd_buffer, >> >> + unsigned width, unsigned >> height, >> >> + unsigned scale); >> >> + >> >> void genX(flush_pipeline_select_3d)(struct anv_cmd_buffer *cmd_buffer); >> >> void genX(flush_pipeline_select_gpgpu)(struct anv_cmd_buffer >> *cmd_buffer); >> >> >> >> diff --git a/src/intel/vulkan/anv_private.h >> >> b/src/intel/vulkan/anv_private.h >> >> index 2465f264354..b381386a716 100644 >> >> --- a/src/intel/vulkan/anv_private.h >> >> +++ b/src/intel/vulkan/anv_private.h >> >> @@ -2421,6 +2421,12 @@ struct anv_cmd_state { >> >> >> >> bool >> >> conditional_render_enabled; >> >> >> >> + /** >> >> + * Last rendering scale argument provided to >> >> + * genX(cmd_buffer_emit_hashing_mode)(). >> >> + */ >> >> + unsigned current_hash_scale; >> >> + >> >> /** >> >> * Array length is anv_cmd_state::pass::attachment_count. Array >> >> content is >> >> * valid only when recording a render pass instance. >> >> diff --git a/src/intel/vulkan/genX_blorp_exec.c >> >> b/src/intel/vulkan/genX_blorp_exec.c >> >> index 1592e7f7e3d..e9eedc06696 100644 >> >> --- a/src/intel/vulkan/genX_blorp_exec.c >> >> +++ b/src/intel/vulkan/genX_blorp_exec.c >> >> @@ -223,6 +223,12 @@ genX(blorp_exec)(struct blorp_batch *batch, >> >> genX(cmd_buffer_config_l3)(cmd_buffer, cfg); >> >> } >> >> >> >> + const unsigned scale = params->fast_clear_op ? UINT_MAX : 1; >> >> + if (cmd_buffer->state.current_hash_scale != scale) { >> >> + genX(cmd_buffer_emit_hashing_mode)(cmd_buffer, params->x1 - >> >> params->x0, >> >> + params->y1 - params->y0, >> scale); >> >> + } >> >> + >> >> #if GEN_GEN >= 11 >> >> /* The PIPE_CONTROL command description says: >> >> * >> >> diff --git a/src/intel/vulkan/genX_cmd_buffer.c >> >> b/src/intel/vulkan/genX_cmd_buffer.c >> >> index 86ef1663ac4..e9e5570d49f 100644 >> >> --- a/src/intel/vulkan/genX_cmd_buffer.c >> >> +++ b/src/intel/vulkan/genX_cmd_buffer.c >> >> @@ -1595,6 +1595,7 @@ genX(CmdExecuteCommands)( >> >> */ >> >> primary->state.current_pipeline = UINT32_MAX; >> >> primary->state.current_l3_config = NULL; >> >> + primary->state.current_hash_scale = 0; >> >> >> >> /* Each of the secondary command buffers will use its own state base >> >> * address. We need to re-emit state base address for the primary >> >> after >> >> @@ -2663,6 +2664,9 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer >> >> *cmd_buffer) >> >> >> >> genX(cmd_buffer_config_l3)(cmd_buffer, pipeline->urb.l3_config); >> >> >> >> + if (cmd_buffer->state.current_hash_scale != 1) >> >> + genX(cmd_buffer_emit_hashing_mode)(cmd_buffer, UINT_MAX, >> UINT_MAX, >> >> 1); >> >> >> > >> > Can we do the check against current_hash_scale as an early return inside >> > the emit function since that's already where the "current" value is >> > updated? Maybe call it cmd_buffer_config_hashing_mode if "emit" no >> longer >> > seems right with that change? I don't really care about the rename. >> > >> >> That's what my first revision did. I moved the current_hash_scale check >> one level up because the function call overhead was visible in >> brw_upload_pipeline_state(), which is quite a hot path in i965. > > > That's a bit surprising but believable if the two are in different files > and there's a real function call going on there. > > >> That >> said I don't have any data justifying the change in the Vulkan driver -- >> Because caller and callee are defined in the same compilation unit it >> *might* be that the function is inlined into >> genX(cmd_buffer_flush_state), though it's large enough that it may not >> > > I highly doubt it would be a problem. It's a lot of lines of code but once > you optimize things a bit, it's actually quite a small function. > > >> be, either way it seemed like a good idea to do things consistently >> across drivers. >> > > The functions right next to it for l3$ configuration and PIPELINE_SELECT > are both called unconditionally. IMO, consistency within the Vulkan driver > is more important than consistency between iris and ANV given that they > have completely different state tracking models. > I don't really care one way or another since I have no data justifying the micro-optimization in Vulkan (I do have data showing a statistically significant benefit in CPU-bound tests from the corresponding change on i965). Will gladly undo the change in exchange for an R-b. </blockquote><div> </div><div>Sounds fine with me. Just to be clear, were you expecting a conditional review or were you planning to send a v2? I was waiting for a v2. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > --Jason > > >> > >> >> + >> >> genX(flush_pipeline_select_3d)(cmd_buffer); >> >> >> >> if (vb_emit) { >> >> @@ -3925,6 +3929,98 @@ genX(cmd_buffer_emit_gen7_depth_flush)(struct >> >> anv_cmd_buffer *cmd_buffer) >> >> } >> >> } >> >> >> >> +/** >> >> + * Update the pixel hashing modes that determine the balancing of PS >> >> threads >> >> + * across subslices and slices. >> >> + * >> >> + * \param width Width bound of the rendering area (already scaled down >> if >> >> \p >> >> + * scale is greater than 1). >> >> + * \param height Height bound of the rendering area (already scaled >> down >> >> if \p >> >> + * scale is greater than 1). >> >> + * \param scale The number of framebuffer samples that could >> potentially >> >> be >> >> + * affected by an individual channel of the PS thread. >> This >> >> is >> >> + * typically one for single-sampled rendering, but for >> >> operations >> >> + * like CCS resolves and fast clears a single PS >> invocation >> >> may >> >> + * update a huge number of pixels, in which case a finer >> >> + * balancing is desirable in order to maximally utilize >> the >> >> + * bandwidth available. UINT_MAX can be used as shorthand >> >> for >> >> + * "finest hashing mode available". >> >> + */ >> >> +void >> >> +genX(cmd_buffer_emit_hashing_mode)(struct anv_cmd_buffer *cmd_buffer, >> >> + unsigned width, unsigned height, >> >> + unsigned scale) >> >> +{ >> >> +#if GEN_GEN == 9 >> >> + const struct gen_device_info *devinfo = &cmd_buffer->device->info; >> >> + const unsigned slice_hashing[] = { >> >> + /* Because all Gen9 platforms with more than one slice require >> >> + * three-way subslice hashing, a single "normal" 16x16 slice >> hashing >> >> + * block is guaranteed to suffer from substantial imbalance, with >> >> one >> >> + * subslice receiving twice as much work as the other two in the >> >> + * slice. >> >> + * >> >> + * The performance impact of that would be particularly severe >> when >> >> + * three-way hashing is also in use for slice balancing (which is >> >> the >> >> + * case for all Gen9 GT4 platforms), because one of the slices >> >> + * receives one every three 16x16 blocks in either direction, >> which >> >> + * is roughly the periodicity of the underlying subslice >> imbalance >> >> + * pattern ("roughly" because in reality the hardware's >> >> + * implementation of three-way hashing doesn't do exact modulo 3 >> >> + * arithmetic, which somewhat decreases the magnitude of this >> effect >> >> + * in practice). This leads to a systematic subslice imbalance >> >> + * within that slice regardless of the size of the primitive. >> The >> >> + * 32x32 hashing mode guarantees that the subslice imbalance >> within >> >> a >> >> + * single slice hashing block is minimal, largely eliminating >> this >> >> + * effect. >> >> + */ >> >> + _32x32, >> >> + /* Finest slice hashing mode available. */ >> >> + NORMAL >> >> + }; >> >> + const unsigned subslice_hashing[] = { >> >> + /* 16x16 would provide a slight cache locality benefit especially >> >> + * visible in the sampler L1 cache efficiency of low-bandwidth >> >> + * non-LLC platforms, but it comes at the cost of greater >> subslice >> >> + * imbalance for primitives of dimensions approximately >> intermediate >> >> + * between 16x4 and 16x16. >> >> + */ >> >> + _16x4, >> >> + /* Finest subslice hashing mode available. */ >> >> + _8x4 >> >> + }; >> >> + /* Dimensions of the smallest hashing block of a given hashing mode. >> >> If >> >> + * the rendering area is smaller than this there can't possibly be >> any >> >> + * benefit from switching to this mode, so we optimize out the >> >> + * transition. >> >> + */ >> >> + const unsigned min_size[][2] = { >> >> + { 16, 4 }, >> >> + { 8, 4 } >> >> + }; >> >> + const unsigned idx = scale > 1; >> >> + >> >> + if (width > min_size[idx][0] || height > min_size[idx][1]) { >> >> + uint32_t gt_mode; >> >> + >> >> + anv_pack_struct(&gt_mode, GENX(GT_MODE), >> >> + .SliceHashing = (devinfo->num_slices > 1 ? >> >> slice_hashing[idx] : 0), >> >> + .SliceHashingMask = (devinfo->num_slices > 1 ? >> -1 : >> >> 0), >> >> + .SubsliceHashing = subslice_hashing[idx], >> >> + .SubsliceHashingMask = -1); >> >> + >> >> + anv_batch_emit(&cmd_buffer->batch, GENX(PIPE_CONTROL), pc) { >> >> + pc.StallAtPixelScoreboard = true; >> >> + pc.CommandStreamerStallEnable = true; >> >> + } >> >> >> > >> > This would be better as >> > >> > cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_CS_STALL_BIT; >> > genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer); >> > >> > That way workarounds get applied more properly and we can avoid redundant >> > PIPE_CONTROLs. apply_pipe_flushes will insert the StallAtPixelScoreboard >> > for you if needed. That said, maybe it's best to also OR in >> > ANV_PIPE_STALL_AT_SCOREBOARD_BIT just to be sure. >> > >> >> OK, changed locally. >> >> > Other than that, the patch looks good to me. Happy to see the perf >> bump. :) >> > >> >> Thanks for testing it out! >> >> > I did consider maybe trying to move things around so that the change is >> > done more as a subpass thing but there are some subtle interactions to >> > think through there and I'm convinced what you have here is correct so >> > let's go with it for now. >> > >> > --Jason >> > >> > >> >> + >> >> + emit_lri(&cmd_buffer->batch, GENX(GT_MODE_num), gt_mode); >> >> + >> >> + cmd_buffer->state.current_hash_scale = scale; >> >> + } >> >> +#endif >> >> +} >> >> + >> >> static void >> >> cmd_buffer_emit_depth_stencil(struct anv_cmd_buffer *cmd_buffer) >> >> { >> >> -- >> >> 2.22.0 >> >> >> >> _______________________________________________ >> >> mesa-dev mailing list >> >> <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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> >> </blockquote></div></div>