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

[Paul Berry]

On 29 August 2013 21:31, Kenneth Graunke <kenneth at whitecape.org> wrote:

> 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.
>

I definitely agree with you that the emit_two_things() function would be a
bad idea.  But I don't think that necessarily means that emit_sampler() and
emit_binding_table_pointers() functions would be a bad idea.  Some of the
benefit of splitting code into functions is that the names of the functions
help to improve readability of the caller.  Using emit_samplers() and
emit_binding_table_pointers() would have that benefit; using
emit_two_things() wouldn't.


>
>      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.
>

In an effort to help us make a more informed decision about this, I've
investigated the following questions:

1. Are there any historical commits that would have been simpler if we had
shared code between VS and FS in the way that we're currently discussing
sharing code between VS and GS?  Are there any that would have been more
complex?  Are there any historical commits where we made mistakes because
the code wasn't shared (e.g. changed one function but forgot to change the
other)?  I looked just at gen7, and I looked at patches since the beginning
of 2012.  I considered both the code sharing that I've proposed in this
patch as well as the counter-proposal to just share the code for constant
emission.

These two commits would have been helped by the code sharing I've proposed;
they would also have been helped if we just shared code for constant
emission:

e6893b9 (Set MOCS L3 cacheability for IVB/BYT (v2))
2273b65 (Change L3 MOCS of 3DSTATE_CONSTANT_VS/PS)

These two commits would have been helped by the code sharing I've proposed,
but not by sharing code for constant emission.

decc708 (Upload separate per-stage sampler state tables)
f5a690c (i965: Split sampler count variable to be per-stage)

In all of the above cases the savings would have been small--one diff hunk
instead of two.

I didn't find any commits that would have been more complex under either my
or Eric's proposal.  I didn't find any instances of past mistakes where we
modified one function but not the other.



2. If we start sharing the code to populate the first 4 DWORDs of
3DSTATE_{VS,GS} now, is it likely that in the future we'll be able to
extend the code sharing to 3DSTATE_{HS,DS,PS}?  To answer this question, I
looked at the documentation for all of these commands in Gen7 (IVB and
HSW).  I found these differences:

- 3DSTATE_HS uses a completely different layout from the others; for
example its kernel start pointer isn't until dw3, whereas the other stages
use dw1.

- dw2[26] is "denormal mode" for PS; it's MBZ for VS, DS, and GS.

- dw2[17] is "thread priority" for HSW only on VS, DS, and PS; it's "thread
priority" for both HSW and IVB on GS.  I suspect this is a documentation
bug, and it's really meant to be HSW only for all stages.

- dw2[15:14] are "rounding mode" on PS only.

- "Accesses UAV" (a HSW-only bit) is on dw2[12] for VS and GS, dw2[14] for
DS, and dw4[5] for PS.  I suspect this bit is related to
ARB_shader_image_load_store functionality, but I'm not certain.

- dw2[11] is "mask stack exception enable" on GS and PS; it's MBZ on VS and
DS.

At the moment we never set any of the bits that differ between VS, DS, GS,
and PS, so we could share the code to populate dw0-3 between those 4 stages
without having to make stage-specific exceptions.  It's possible that we
might have to make stage-specific exceptions in the future if we ever
decide to set some of those bits, but the only likely candidate seems to be
"Accesses UAV".  It's pretty clear that we couldn't share any code to
populate dw0-3 of 3DSTATE_HS.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130901/b7c38d93/attachment.html>