[Mesa-dev] [PATCH 02/37] i965/gen6/gs: refactor gen6_gs_state
Jordan Justen
jljusten at gmail.com
Thu Sep 4 00:44:20 PDT 2014
On Wed, Sep 3, 2014 at 11:33 PM, Iago Toral Quiroga <itoral at 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 <itoral at igalia.com> wrote:
>> > From: Samuel Iglesias Gonsalvez <siglesias at 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 <siglesias at 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.justen at 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 at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
More information about the mesa-dev
mailing list