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. > + 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? _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev