On Friday, May 29, 2015 03:58:03 PM Ben Widawsky wrote: > On Fri, May 29, 2015 at 12:26:39PM -0700, Kenneth Graunke wrote: > > We used to store the GS dispatch mode in brw_gs_prog_data while > > separately storing the VS dispatch mode in brw_vue_prog_data::simd8. > > > > This patch introduces an enum to represent all possible dispatch modes, > > and stores it in brw_vue_prog_data::dispatch_mode, unifying the two. > > > > Based on a suggestion by Matt Turner. > > > > Signed-off-by: Kenneth Graunke <[email protected]> > > --- > > src/mesa/drivers/dri/i965/brw_context.h | 16 +++++++--------- > > src/mesa/drivers/dri/i965/brw_defines.h | 5 ++--- > > src/mesa/drivers/dri/i965/brw_vec4.cpp | 5 ++++- > > src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp | 8 ++++---- > > src/mesa/drivers/dri/i965/brw_vs_surface_state.c | 4 ++-- > > src/mesa/drivers/dri/i965/gen7_gs_state.c | 2 +- > > src/mesa/drivers/dri/i965/gen8_gs_state.c | 3 ++- > > src/mesa/drivers/dri/i965/gen8_vs_state.c | 3 ++- > > 8 files changed, 24 insertions(+), 22 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_context.h > > b/src/mesa/drivers/dri/i965/brw_context.h > > index abc11f6..01c4283 100644 > > --- a/src/mesa/drivers/dri/i965/brw_context.h > > +++ b/src/mesa/drivers/dri/i965/brw_context.h > > @@ -605,6 +605,12 @@ struct brw_ff_gs_prog_data { > > unsigned svbi_postincrement_value; > > }; > > > > +enum shader_dispatch_mode { > > + DISPATCH_MODE_4X1_SINGLE = 0, > > + DISPATCH_MODE_4X2_DUAL_INSTANCE = 1, > > + DISPATCH_MODE_4X2_DUAL_OBJECT = 2, > > + DISPATCH_MODE_SIMD8 = 3, > > +}; > > > > /* Note: brw_vue_prog_data_compare() must be updated when adding fields to > > * this struct! > > @@ -622,7 +628,7 @@ struct brw_vue_prog_data { > > */ > > GLuint urb_entry_size; > > > > - bool simd8; > > + enum shader_dispatch_mode dispatch_mode; > > }; > > > > > > @@ -720,14 +726,6 @@ struct brw_gs_prog_data > > int invocations; > > > > /** > > - * Dispatch mode, can be any of: > > - * GEN7_GS_DISPATCH_MODE_DUAL_OBJECT > > - * GEN7_GS_DISPATCH_MODE_DUAL_INSTANCE > > - * GEN7_GS_DISPATCH_MODE_SINGLE > > - */ > > - int dispatch_mode; > > - > > - /** > > * Gen6 transform feedback enabled flag. > > */ > > bool gen6_xfb_enabled; > > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h > > b/src/mesa/drivers/dri/i965/brw_defines.h > > index dedc381..f6da305 100644 > > --- a/src/mesa/drivers/dri/i965/brw_defines.h > > +++ b/src/mesa/drivers/dri/i965/brw_defines.h > > @@ -1773,9 +1773,8 @@ enum brw_message_target { > > # define GEN7_GS_CONTROL_DATA_FORMAT_GSCTL_SID 1 > > # define GEN7_GS_CONTROL_DATA_HEADER_SIZE_SHIFT 20 > > # define GEN7_GS_INSTANCE_CONTROL_SHIFT 15 > > -# define GEN7_GS_DISPATCH_MODE_SINGLE (0 << 11) > > -# define GEN7_GS_DISPATCH_MODE_DUAL_INSTANCE (1 << 11) > > -# define GEN7_GS_DISPATCH_MODE_DUAL_OBJECT (2 << 11) > > +# define GEN7_GS_DISPATCH_MODE_SHIFT 11 > > +# define GEN7_GS_DISPATCH_MODE_MASK INTEL_MASK(12, 11) > > # define GEN6_GS_STATISTICS_ENABLE (1 << 10) > > # define GEN6_GS_SO_STATISTICS_ENABLE (1 << 9) > > # define GEN6_GS_RENDERING_ENABLE (1 << 8) > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp > > b/src/mesa/drivers/dri/i965/brw_vec4.cpp > > index a324798..5a9c3f5 100644 > > --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp > > @@ -1894,6 +1894,8 @@ brw_vs_emit(struct brw_context *brw, > > brw_create_nir(brw, NULL, &c->vp->program.Base, > > MESA_SHADER_VERTEX); > > } > > > > + prog_data->base.dispatch_mode = DISPATCH_MODE_SIMD8; > > + > > fs_visitor v(brw, mem_ctx, MESA_SHADER_VERTEX, &c->key, > > &prog_data->base.base, prog, &c->vp->program.Base, 8); > > if (!v.run_vs()) { > > @@ -1926,11 +1928,12 @@ brw_vs_emit(struct brw_context *brw, > > g.generate_code(v.cfg, 8); > > assembly = g.get_assembly(final_assembly_size); > > > > - prog_data->base.simd8 = true; > > c->base.last_scratch = v.last_scratch; > > for whatever it's worth, it would have made more sense to put the > dispatch_mode > change here.
Right...it is a bit odd. I believe I was trying to follow the pattern
used for GS:
brw_gs_emit sets dispatch_mode to 4X2_DUAL_OBJECT, 4X2_DUAL_INSTANCE, or
4X1_SINGLE, then creates a vec4_gs_visitor and runs it. The visitor
inspects dispatch_mode to decide how to do things. This makes sense
because vec4_gs_visitor handles all three cases.
For VS, we use vec4_vs_visitor (for 4X2 dual vertex mode) or fs_visitor
(for SIMD8 mode); each handles only that one mode. So they never
inspect the dispatch_mode variable, and therefore it doesn't matter
whether we set it before or after.
But I figured it's a bit more consistent this way. *shrug*
> > }
> >
> > if (!assembly) {
> > + prog_data->base.dispatch_mode = DISPATCH_MODE_4X2_DUAL_OBJECT;
> > +
> > vec4_vs_visitor v(brw, c, prog_data, prog, mem_ctx);
> > if (!v.run()) {
> > if (prog) {
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> > b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> > index 363e30e..eacb2f5 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> > @@ -106,7 +106,7 @@ vec4_gs_visitor::setup_payload()
> > * to be interleaved, so one register contains two attribute slots.
> > */
> > int attributes_per_reg =
> > - c->prog_data.dispatch_mode == GEN7_GS_DISPATCH_MODE_DUAL_OBJECT ? 1
> > : 2;
> > + c->prog_data.base.dispatch_mode == DISPATCH_MODE_4X2_DUAL_OBJECT ? 1
> > : 2;
> >
> > /* If a geometry shader tries to read from an input that wasn't written
> > by
> > * the vertex shader, that produces undefined results, but it shouldn't
> > @@ -655,7 +655,7 @@ brw_gs_emit(struct brw_context *brw,
> > */
> > if (c->prog_data.invocations <= 1 &&
> > likely(!(INTEL_DEBUG & DEBUG_NO_DUAL_OBJECT_GS))) {
> > - c->prog_data.dispatch_mode = GEN7_GS_DISPATCH_MODE_DUAL_OBJECT;
> > + c->prog_data.base.dispatch_mode = DISPATCH_MODE_4X2_DUAL_OBJECT;
> >
> > vec4_gs_visitor v(brw, c, prog, mem_ctx, true /* no_spills */);
> > if (v.run()) {
> > @@ -690,9 +690,9 @@ brw_gs_emit(struct brw_context *brw,
> > * SINGLE mode.
> > */
> > if (c->prog_data.invocations <= 1 || brw->gen < 7)
> > - c->prog_data.dispatch_mode = GEN7_GS_DISPATCH_MODE_SINGLE;
> > + c->prog_data.base.dispatch_mode = DISPATCH_MODE_4X1_SINGLE;
> > else
> > - c->prog_data.dispatch_mode = GEN7_GS_DISPATCH_MODE_DUAL_INSTANCE;
> > + c->prog_data.base.dispatch_mode = DISPATCH_MODE_4X2_DUAL_INSTANCE;
> >
> > vec4_gs_visitor *gs = NULL;
> > const unsigned *ret = NULL;
> > diff --git a/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> > b/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> > index f82a62b..b2f91bd 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> > @@ -121,7 +121,7 @@ brw_upload_vs_pull_constants(struct brw_context *brw)
> > /* BRW_NEW_VS_PROG_DATA */
> > const struct brw_stage_prog_data *prog_data =
> > &brw->vs.prog_data->base.base;
> >
> > - dword_pitch = brw->vs.prog_data->base.simd8;
> > + dword_pitch = brw->vs.prog_data->base.dispatch_mode ==
> > DISPATCH_MODE_SIMD8;
> >
> > /* _NEW_PROGRAM_CONSTANTS */
> > brw_upload_pull_constants(brw, BRW_NEW_VS_CONSTBUF, &vp->program.Base,
> > @@ -151,7 +151,7 @@ brw_upload_vs_ubo_surfaces(struct brw_context *brw)
> > return;
> >
> > /* BRW_NEW_VS_PROG_DATA */
> > - dword_pitch = brw->vs.prog_data->base.simd8;
> > + dword_pitch = brw->vs.prog_data->base.dispatch_mode ==
> > DISPATCH_MODE_SIMD8;
> > brw_upload_ubo_surfaces(brw, prog->_LinkedShaders[MESA_SHADER_VERTEX],
> > &brw->vs.base, &brw->vs.prog_data->base.base,
> > dword_pitch);
>
> Seems like it might be worthwhile to remove dword_pitch altogether now and
> pass
> down the vue by itself, but I'm fine with either.
The problem is that brw_vue_prog_data only exists for VS/GS (stages that
deal with geometry), while brw_upload_pull_constants and
brw_upload_ubo_surfaces are also used for fragment programs.
> > diff --git a/src/mesa/drivers/dri/i965/gen7_gs_state.c
> > b/src/mesa/drivers/dri/i965/gen7_gs_state.c
> > index e1c4f8b..8d6d3fe 100644
> > --- a/src/mesa/drivers/dri/i965/gen7_gs_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen7_gs_state.c
> > @@ -112,7 +112,7 @@ upload_gs_state(struct brw_context *brw)
> > GEN7_GS_CONTROL_DATA_HEADER_SIZE_SHIFT) |
> > ((brw->gs.prog_data->invocations - 1) <<
> > GEN7_GS_INSTANCE_CONTROL_SHIFT) |
> > - brw->gs.prog_data->dispatch_mode |
> > + SET_FIELD(prog_data->dispatch_mode, GEN7_GS_DISPATCH_MODE) |
> > GEN6_GS_STATISTICS_ENABLE |
> > (brw->gs.prog_data->include_primitive_id ?
> > GEN7_GS_INCLUDE_PRIMITIVE_ID : 0) |
> > diff --git a/src/mesa/drivers/dri/i965/gen8_gs_state.c
> > b/src/mesa/drivers/dri/i965/gen8_gs_state.c
> > index 0763e91..26a02d3 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_gs_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_gs_state.c
> > @@ -76,7 +76,8 @@ gen8_upload_gs_state(struct brw_context *brw)
> >
> > uint32_t dw7 = (brw->gs.prog_data->control_data_header_size_hwords <<
> > GEN7_GS_CONTROL_DATA_HEADER_SIZE_SHIFT) |
> > - brw->gs.prog_data->dispatch_mode |
> > + SET_FIELD(prog_data->dispatch_mode,
> > + GEN7_GS_DISPATCH_MODE) |
> > ((brw->gs.prog_data->invocations - 1) <<
> > GEN7_GS_INSTANCE_CONTROL_SHIFT) |
> > GEN6_GS_STATISTICS_ENABLE |
> > diff --git a/src/mesa/drivers/dri/i965/gen8_vs_state.c
> > b/src/mesa/drivers/dri/i965/gen8_vs_state.c
> > index f92af55..9bfde38 100644
> > --- a/src/mesa/drivers/dri/i965/gen8_vs_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen8_vs_state.c
> > @@ -66,7 +66,8 @@ upload_vs_state(struct brw_context *brw)
> > (prog_data->urb_read_length << GEN6_VS_URB_READ_LENGTH_SHIFT)
> > |
> > (0 << GEN6_VS_URB_ENTRY_READ_OFFSET_SHIFT));
> >
> > - uint32_t simd8_enable = prog_data->simd8 ? GEN8_VS_SIMD8_ENABLE : 0;
> > + uint32_t simd8_enable = prog_data->dispatch_mode == DISPATCH_MODE_SIMD8
> > ?
> > + GEN8_VS_SIMD8_ENABLE : 0;
>
> I wouldn't mind an assert here that it's not one of those modes which
> shouldn't
> be used by VS, but I am assert happy...
That's not a bad idea. I'll write a patch for that.
>
> > OUT_BATCH(((brw->max_vs_threads - 1) << HSW_VS_MAX_THREADS_SHIFT) |
> > GEN6_VS_STATISTICS_ENABLE |
> > simd8_enable |
>
> Reviewed-by: Ben Widawsky <[email protected]>
Thanks, Ben!
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
