On Tuesday, February 16, 2016 10:09:50 AM PST 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)
static functions don't need the "brw_" prefix. (Similarly, in core Mesa, we don't use the "_mesa_" prefix for static functions...this helps identify them.) > +{ > + 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 > + */ As Ben mentioned, I do prefer one line comments, but it's up to you... > + 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(); > +} > + > +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); Bonus parens here...I'd prefer: (brw->gen == 7 ? GEN7_GPGPU_PREDICATE_ENABLE : 0); This looks good to me, thanks for doing this! Regardless of whether we need a flush, this is an improvement. I'm semi-inclined to add one just out of paranoia, but frankly I doubt this case is going to come up in the real world anyway. I'd be happy to see this land as is; if Curro or Kristian think we need a flush, we can always add it later. With or without any of my style suggestions, Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> > + 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) >
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev