On Tue, Feb 16, 2016 at 12:21:02PM -0800, Jordan Justen wrote: > On 2016-02-16 12:03:10, Ben Widawsky wrote: > > On Tue, Feb 16, 2016 at 10:09:50AM -0800, Jordan Justen wrote: > > > On gen7 (Ivy Bridge, Haswell), we will get a GPU hang if an indirect > > > dispatch is used, but one of the dimensions is 0. > > > > > > Therefore we use predicated rendering on the GPGPU_WALKER command to > > > handle this case. > > > > > > Fixes piglit test: spec/arb_compute_shader/zero-dispatch-size > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94100 > > > Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com> > > > Cc: Kenneth Graunke <kenn...@whitecape.org> > > > Cc: Ben Widawsky <b...@bwidawsk.net> > > > Cc: Ilia Mirkin <imir...@alum.mit.edu> > > > --- > > > src/mesa/drivers/dri/i965/brw_compute.c | 104 > > > +++++++++++++++++++++++++++----- > > > src/mesa/drivers/dri/i965/brw_defines.h | 1 + > > > 2 files changed, 91 insertions(+), 14 deletions(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_compute.c > > > b/src/mesa/drivers/dri/i965/brw_compute.c > > > index d9f181a..bbb8ce3 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_compute.c > > > +++ b/src/mesa/drivers/dri/i965/brw_compute.c > > > @@ -35,6 +35,92 @@ > > > > > > > > > static void > > > +brw_prepare_indirect_gpgpu_walker(struct brw_context *brw) > > > +{ > > > > Just FYI: > > There is a blurb in the predicate text: > > To ensure the memory sources of the MI_LOAD_REGISTER_MEM commands are > > coherent > > with previous 3D_PIPECONTROL store-DWord operations, software can use the > > new > > Pipe Control Flush Enable bit in the PIPE_CONTROL command. > > > > I suppose it's never the case that we'll be writing these with > > PIPE_CONTROL, so > > it's safe to ignore this. > > > > On irc it sounded like you didn't think the flush was required. I'm > going to stick with that unless you tell me otherwise. > > The LRM is coming from a user BO, and they may have set the values by > mapping the buffer and writing it from the CPU, or by writing it from > a shader (for example SSBO). >
The note seems to imply that DW writes with a pipe control can be deferred and so you need to flush are previously pipe controls... again, yeah, I think we're safe. > > > + GLintptr indirect_offset = brw->compute.num_work_groups_offset; > > > + drm_intel_bo *bo = brw->compute.num_work_groups_bo; > > > + > > > + brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMX, bo, > > > + I915_GEM_DOMAIN_VERTEX, 0, > > > + indirect_offset + 0); > > > + brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMY, bo, > > > + I915_GEM_DOMAIN_VERTEX, 0, > > > + indirect_offset + 4); > > > + brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMZ, bo, > > > + I915_GEM_DOMAIN_VERTEX, 0, > > > + indirect_offset + 8); > > > + > > > + if (brw->gen > 7) > > > + return; > > > + > > > + /* Clear upper 32-bits of SRC0 and all 64-bits of SRC1 > > > + */ > > > + BEGIN_BATCH(7); > > > + OUT_BATCH(MI_LOAD_REGISTER_IMM | (7 - 2)); > > > + OUT_BATCH(MI_PREDICATE_SRC0 + 4); > > > + OUT_BATCH(0u); > > > + OUT_BATCH(MI_PREDICATE_SRC1 + 0); > > > + OUT_BATCH(0u); > > > + OUT_BATCH(MI_PREDICATE_SRC1 + 4); > > > + OUT_BATCH(0u); > > > + ADVANCE_BATCH(); > > > + > > > + /* Load compute_dispatch_indirect_x_size into SRC0 > > > + */ > > > + brw_load_register_mem(brw, MI_PREDICATE_SRC0, bo, > > > + I915_GEM_DOMAIN_INSTRUCTION, 0, > > > + indirect_offset + 0); > > > + > > > + /* predicate = (compute_dispatch_indirect_x_size == 0); > > > + */ > > > + BEGIN_BATCH(1); > > > + OUT_BATCH(GEN7_MI_PREDICATE | > > > + MI_PREDICATE_LOADOP_LOAD | > > > + MI_PREDICATE_COMBINEOP_SET | > > > + MI_PREDICATE_COMPAREOP_SRCS_EQUAL); > > > + ADVANCE_BATCH(); > > > + > > > + /* Load compute_dispatch_indirect_y_size into SRC0 > > > + */ > > > + brw_load_register_mem(brw, MI_PREDICATE_SRC0, bo, > > > + I915_GEM_DOMAIN_INSTRUCTION, 0, > > > + indirect_offset + 4); > > > + > > > + /* predicate |= (compute_dispatch_indirect_y_size == 0); > > > + */ > > > + BEGIN_BATCH(1); > > > + OUT_BATCH(GEN7_MI_PREDICATE | > > > + MI_PREDICATE_LOADOP_LOAD | > > > + MI_PREDICATE_COMBINEOP_OR | > > > + MI_PREDICATE_COMPAREOP_SRCS_EQUAL); > > > + ADVANCE_BATCH(); > > > + > > > + /* Load compute_dispatch_indirect_z_size into SRC0 > > > + */ > > > + brw_load_register_mem(brw, MI_PREDICATE_SRC0, bo, > > > + I915_GEM_DOMAIN_INSTRUCTION, 0, > > > + indirect_offset + 8); > > > + > > > + /* predicate |= (compute_dispatch_indirect_z_size == 0); > > > + */ > > > + BEGIN_BATCH(1); > > > + OUT_BATCH(GEN7_MI_PREDICATE | > > > + MI_PREDICATE_LOADOP_LOAD | > > > + MI_PREDICATE_COMBINEOP_OR | > > > + MI_PREDICATE_COMPAREOP_SRCS_EQUAL); > > > + ADVANCE_BATCH(); > > > + > > > + /* predicate = !predicate; > > > + */ > > > + BEGIN_BATCH(1); > > > + OUT_BATCH(GEN7_MI_PREDICATE | > > > + MI_PREDICATE_LOADOP_LOADINV | > > > + MI_PREDICATE_COMBINEOP_OR | > > > + MI_PREDICATE_COMPAREOP_FALSE); > > > + ADVANCE_BATCH(); > > > +} > > > > I think all of your comments would fit on one line... > > > > Just summing up our conversation, I believe you could have slightly simpler > > code > > using DELTAS_EQUAL, but it's not entirely clear. > > > > > + > > > +static void > > > brw_emit_gpgpu_walker(struct brw_context *brw) > > > { > > > const struct brw_cs_prog_data *prog_data = brw->cs.prog_data; > > > @@ -45,20 +131,10 @@ brw_emit_gpgpu_walker(struct brw_context *brw) > > > if (brw->compute.num_work_groups_bo == NULL) { > > > indirect_flag = 0; > > > } else { > > > - GLintptr indirect_offset = brw->compute.num_work_groups_offset; > > > - drm_intel_bo *bo = brw->compute.num_work_groups_bo; > > > - > > > - indirect_flag = GEN7_GPGPU_INDIRECT_PARAMETER_ENABLE; > > > - > > > - brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMX, bo, > > > - I915_GEM_DOMAIN_VERTEX, 0, > > > - indirect_offset + 0); > > > - brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMY, bo, > > > - I915_GEM_DOMAIN_VERTEX, 0, > > > - indirect_offset + 4); > > > - brw_load_register_mem(brw, GEN7_GPGPU_DISPATCHDIMZ, bo, > > > - I915_GEM_DOMAIN_VERTEX, 0, > > > - indirect_offset + 8); > > > + indirect_flag = > > > + GEN7_GPGPU_INDIRECT_PARAMETER_ENABLE | > > > + ((brw->gen == 7) ? GEN7_GPGPU_PREDICATE_ENABLE : 0); > > > + brw_prepare_indirect_gpgpu_walker(brw); > > > } > > > > > > const unsigned simd_size = prog_data->simd_size; > > > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h > > > b/src/mesa/drivers/dri/i965/brw_defines.h > > > index b1fa559..60b696c 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_defines.h > > > +++ b/src/mesa/drivers/dri/i965/brw_defines.h > > > @@ -2938,6 +2938,7 @@ enum brw_wm_barycentric_interp_mode { > > > #define GPGPU_WALKER 0x7105 > > > /* GEN7 DW0 */ > > > # define GEN7_GPGPU_INDIRECT_PARAMETER_ENABLE (1 << 10) > > > +# define GEN7_GPGPU_PREDICATE_ENABLE (1 << 8) > > > /* GEN8+ DW2 */ > > > # define GPGPU_WALKER_INDIRECT_LENGTH_SHIFT 0 > > > # define GPGPU_WALKER_INDIRECT_LENGTH_MASK INTEL_MASK(15, 0) > > > > Just making sure, but the API expects nothing happens if ANY of the > > workgroup > > sizes are 0? Because that's what's going to happen now, isn't it? > > Correct. For DispatchCompute the ext spec says: > > "If the work group count in any dimension is zero, no work groups are > dispatched." > > And for DispatchComputeIndirect: > > ... "is equivalent (assuming no errors are generated) to calling > DispatchCompute with <num_groups_x>, <num_groups_y> and > <num_groups_z>" ... > > -Jordan Thanks for answering: Reviewed-by: Ben Widawsky <benjamin.widaw...@intel.com> -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev