On Wed, Sep 3, 2014 at 11:33 PM, Iago Toral Quiroga <ito...@igalia.com> wrote: > On mié, 2014-09-03 at 18:51 -0700, Jordan Justen wrote: >> Rather than: >> i965/gen6/gs: refactor gen6_gs_state >> >> How about something like: >> i965/gen6/gs: Skeleton for user GS program support > > Sure, that sounds good to me. > >> (more below) >> >> On Thu, Aug 14, 2014 at 4:11 AM, Iago Toral Quiroga <ito...@igalia.com> >> wrote: >> > From: Samuel Iglesias Gonsalvez <sigles...@igalia.com> >> > >> > Currently, gen6 only uses geometry shaders for transform feedback so the >> > state >> > we emit is not suitable to accomodate general purpose, user-provided >> > geometry >> > shaders. This patch paves the way to add these support and the needed >> > 3DSTATE_GS packet modifications for it. >> > >> > Previous code that emitted state to implement transform feedback in gen6 >> > goes >> > to upload_gs_state_adhoc_tf(). >> > >> > Signed-off-by: Samuel Iglesias Gonsalvez <sigles...@igalia.com> >> > --- >> > src/mesa/drivers/dri/i965/gen6_gs_state.c | 105 >> > ++++++++++++++++++++++++++---- >> > 1 file changed, 94 insertions(+), 11 deletions(-) >> > >> > diff --git a/src/mesa/drivers/dri/i965/gen6_gs_state.c >> > b/src/mesa/drivers/dri/i965/gen6_gs_state.c >> > index 9648fb7..e132959 100644 >> > --- a/src/mesa/drivers/dri/i965/gen6_gs_state.c >> > +++ b/src/mesa/drivers/dri/i965/gen6_gs_state.c >> > @@ -31,7 +31,7 @@ >> > #include "intel_batchbuffer.h" >> > >> > static void >> > -upload_gs_state(struct brw_context *brw) >> > +upload_gs_state_for_tf(struct brw_context *brw) >> > { >> > /* Disable all the constant buffers. */ >> > BEGIN_BATCH(5); >> > @@ -49,11 +49,11 @@ upload_gs_state(struct brw_context *brw) >> > OUT_BATCH(GEN6_GS_SPF_MODE | GEN6_GS_VECTOR_MASK_ENABLE); >> > OUT_BATCH(0); /* no scratch space */ >> > OUT_BATCH((2 << GEN6_GS_DISPATCH_START_GRF_SHIFT) | >> > - (brw->ff_gs.prog_data->urb_read_length << >> > GEN6_GS_URB_READ_LENGTH_SHIFT)); >> > + (brw->ff_gs.prog_data->urb_read_length << >> > GEN6_GS_URB_READ_LENGTH_SHIFT)); >> > OUT_BATCH(((brw->max_gs_threads - 1) << GEN6_GS_MAX_THREADS_SHIFT) | >> > - GEN6_GS_STATISTICS_ENABLE | >> > - GEN6_GS_SO_STATISTICS_ENABLE | >> > - GEN6_GS_RENDERING_ENABLE); >> > + GEN6_GS_STATISTICS_ENABLE | >> > + GEN6_GS_SO_STATISTICS_ENABLE | >> > + GEN6_GS_RENDERING_ENABLE); >> >> This looks like tabs to spaces conversion. For lines that need to be >> changed, it is good to do that conversion. But, in this case, the only >> change is the name of the function. > > Okay. > > You granted the reviewed-by anyway though, so I understand that this > comment is only for reference. Or would you prefer that we remove these > changes from the patch?
Could you clean it up? It makes review easier, both now, and in looking back through the history. (also, git blame...) Thanks, -Jordan >> > OUT_BATCH(GEN6_GS_SVBI_PAYLOAD_ENABLE | >> > GEN6_GS_SVBI_POSTINCREMENT_ENABLE | >> > (brw->ff_gs.prog_data->svbi_postincrement_value << >> > @@ -65,24 +65,107 @@ upload_gs_state(struct brw_context *brw) >> > OUT_BATCH(_3DSTATE_GS << 16 | (7 - 2)); >> > OUT_BATCH(0); /* prog_bo */ >> > OUT_BATCH((0 << GEN6_GS_SAMPLER_COUNT_SHIFT) | >> > - (0 << GEN6_GS_BINDING_TABLE_ENTRY_COUNT_SHIFT)); >> > + (0 << GEN6_GS_BINDING_TABLE_ENTRY_COUNT_SHIFT)); >> > OUT_BATCH(0); /* scratch space base offset */ >> > OUT_BATCH((1 << GEN6_GS_DISPATCH_START_GRF_SHIFT) | >> > - (0 << GEN6_GS_URB_READ_LENGTH_SHIFT) | >> > - (0 << GEN6_GS_URB_ENTRY_READ_OFFSET_SHIFT)); >> > + (0 << GEN6_GS_URB_READ_LENGTH_SHIFT) | >> > + (0 << GEN6_GS_URB_ENTRY_READ_OFFSET_SHIFT)); >> > OUT_BATCH((0 << GEN6_GS_MAX_THREADS_SHIFT) | >> > - GEN6_GS_STATISTICS_ENABLE | >> > - GEN6_GS_RENDERING_ENABLE); >> > + GEN6_GS_STATISTICS_ENABLE | >> > + GEN6_GS_RENDERING_ENABLE); >> > + OUT_BATCH(0); >> > + ADVANCE_BATCH(); >> > + } >> > +} >> > + >> > +static void >> > +upload_gs_state(struct brw_context *brw) >> > +{ >> > + /* BRW_NEW_GEOMETRY_PROGRAM */ >> > + bool active = brw->geometry_program; >> > + /* CACHE_NEW_GS_PROG */ >> > + const struct brw_vec4_prog_data *prog_data = &brw->gs.prog_data->base; >> > + const struct brw_stage_state *stage_state = &brw->gs.base; >> > + >> > + if (active) { >> > + /* FIXME: enable constant buffers */ >> > + BEGIN_BATCH(5); >> > + OUT_BATCH(_3DSTATE_CONSTANT_GS << 16 | (5 - 2)); >> > + OUT_BATCH(0); >> > + OUT_BATCH(0); >> > OUT_BATCH(0); >> > + OUT_BATCH(0); >> > + ADVANCE_BATCH(); >> > + >> > + BEGIN_BATCH(7); >> > + OUT_BATCH(_3DSTATE_GS << 16 | (7 - 2)); >> > + OUT_BATCH(stage_state->prog_offset); >> > + >> > + /* GEN6_GS_SPF_MODE and GEN6_GS_VECTOR_MASK_ENABLE are enabled as it >> > + * was previously done for gen6. >> > + * >> > + * TODO: test with both disabled to see if the HW is behaving >> > + * as expected, like in gen7. >> > + */ >> > + OUT_BATCH(GEN6_GS_SPF_MODE | GEN6_GS_VECTOR_MASK_ENABLE | >> > + ((ALIGN(stage_state->sampler_count, 4)/4) << >> > + GEN6_GS_SAMPLER_COUNT_SHIFT) | >> > + ((prog_data->base.binding_table.size_bytes / 4) << >> > + GEN6_GS_BINDING_TABLE_ENTRY_COUNT_SHIFT)); >> > + >> > + if (prog_data->total_scratch) { >> > + OUT_RELOC(stage_state->scratch_bo, >> > + I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, >> > + ffs(prog_data->total_scratch) - 11); >> > + } else { >> > + OUT_BATCH(0); /* no scratch space */ >> > + } >> > + >> > + OUT_BATCH((prog_data->urb_read_length << >> > + GEN6_GS_URB_READ_LENGTH_SHIFT) | >> > + (0 << GEN6_GS_URB_ENTRY_READ_OFFSET_SHIFT) | >> > + (prog_data->base.dispatch_grf_start_reg << >> > + GEN6_GS_DISPATCH_START_GRF_SHIFT)); >> > + >> > + OUT_BATCH(((brw->max_gs_threads - 1) << GEN6_GS_MAX_THREADS_SHIFT) | >> > + GEN6_GS_STATISTICS_ENABLE | >> > + GEN6_GS_SO_STATISTICS_ENABLE | >> > + GEN6_GS_RENDERING_ENABLE); >> > + >> > + /* FIXME: Enable SVBI payload only when TF is enable in SNB for >> > + * user-provided GS. >> > + */ >> > + if (0) { >> > + /* GEN6_GS_REORDER is equivalent to GEN7_GS_REORDER_TRAILING >> > + * in gen7. SNB and IVB specs are the same regarding the >> > reordering of >> > + * TRISTRIP/TRISTRIP_REV vertices and triangle orientation, so >> > we do >> > + * the same thing in both generations. For more details, see the >> > + * comment in gen7_gs_state.c >> > + */ >> > + OUT_BATCH(GEN6_GS_REORDER | >> > + GEN6_GS_SVBI_PAYLOAD_ENABLE | >> > + GEN6_GS_SVBI_POSTINCREMENT_ENABLE | >> > + /* FIXME: prog_data->svbi_postincrement_value instead >> > of 0 */ >> > + (0 << GEN6_GS_SVBI_POSTINCREMENT_VALUE_SHIFT) | >> > + GEN6_GS_ENABLE); >> > + } else { >> > + OUT_BATCH(GEN6_GS_REORDER | GEN6_GS_ENABLE); >> > + } >> > ADVANCE_BATCH(); >> > + } else { >> > + /* In gen6, transform feedback support is done with an ad-hoc GS >> > + * program. This function provides the needed 3DSTATE_GS. >> > + */ >> > + upload_gs_state_for_tf(brw); >> >> Also handles the 'fully disable GS' case, which is a little weird. >> Seems fine though, but I think the comment could note that. >> >> Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com> >> >> > } >> > + brw->gs.enabled = active; >> > } >> > >> > const struct brw_tracked_state gen6_gs_state = { >> > .dirty = { >> > .mesa = _NEW_TRANSFORM, >> > .brw = BRW_NEW_CONTEXT | BRW_NEW_PUSH_CONSTANT_ALLOCATION, >> > - .cache = CACHE_NEW_FF_GS_PROG >> > + .cache = (CACHE_NEW_GS_PROG | CACHE_NEW_FF_GS_PROG) >> > }, >> > .emit = upload_gs_state, >> > }; >> > -- >> > 1.9.1 >> > >> > _______________________________________________ >> > mesa-dev mailing list >> > mesa-dev@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev