[Mesa-dev] [PATCH] i965/gen7: Use predicated rendering for indirect compute
Jordan Justen
jordan.l.justen at intel.com
Tue Feb 16 20:21:02 UTC 2016
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.justen at intel.com>
> > Cc: Kenneth Graunke <kenneth at whitecape.org>
> > Cc: Ben Widawsky <ben at bwidawsk.net>
> > Cc: Ilia Mirkin <imirkin at 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).
> > + 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
More information about the mesa-dev
mailing list