[Mesa-dev] [PATCH 21/22] i965/gen7: Generalize gen7_vs_state in preparation for GS.

[Kenneth Graunke]

On 08/29/2013 04:06 PM, Eric Anholt wrote:
> The struct argument is what makes me cringe.  If upload_vs_state() was:
> static void
> upload_vs_state(struct brw_context *brw)
> {
>     struct gl_context *ctx = &brw->ctx;
>     uint32_t floating_point_mode = 0;
>     const int max_threads_shift = brw->is_haswell ?
>        HSW_VS_MAX_THREADS_SHIFT : GEN6_VS_MAX_THREADS_SHIFT;
>
>     /* the VS has a special requirement for synchronization due to a bug
>      * in its constant handling.
>      */
>     gen7_emit_vs_workaround_flush(brw);
>
>     emit_binding_table_pointers(_3DSTATE_BINDING_TABLE_POINTERS_VS, brw->vs.bind_bo_offset);
>     emit_sampler(_3DSTATE_SAMPLER_STATE_POINTERS_VS, brw->vs.sampler_offset);

I don't really like the above.  The existing four lines of code are 
concise enough, and it's obvious exactly what they do.  Here, I have to 
go read these two functions to see what they do.

Plus, both functions are exactly the same, and don't even have any 
functionality built in.  We may as well just do:

void
emit_two_things(unsigned x, unsigned y)
{
    BEGIN_BATCH(2);
    OUT_BATCH(x);
    OUT_BATCH(y);
    ADVANCE_BATCH();
}

If it took additional effort to do one of these things - say, emitting 
the binding table also required some kind of flush - then I wouldn't 
mind putting it in a helper function.

>     emit_constants(3DSTATE_CONSTANT_VS, &brw->vs.stage_state);

I'm fine with putting the constant emission into a helper function. 
Although it's simple code, it's quite a few lines, so factoring it out 
would make things more concise.  All stages could easily reuse it.

Plus, that function would have a single, obvious purpose - emit 
3DSTATE_CONSTANT_XS for some X.

>     /* inline emit of 3DSTATE_VS just like we have today, since there's a
>      * whole bunch of stage-specific stuff as is evident from the
>      * alt_floating_point_mode, state_cmd, state_cmd_size,
>      * stage_specific_cmd_data arguments.
>      */
> }
>
> that would share the definitely-common code without the contortions to
> make 3DSTATE_VS be "shared".  Plus you get to fairly trivially reuse
> those 3 shared packets from blorp, which this patch didn't do today.
> (only "fairly" trivially because you'd want to have individual
> enable/disable packets for the constants to really trivially reuse from
> blorp).

I definitely don't want to share portions of 3DSTATE_VS.