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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
