<div dir="ltr"><div><div><div><div>On 29 August 2013 21:31, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 08/29/2013 04:06 PM, Eric Anholt wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
The struct argument is what makes me cringe. If upload_vs_state() was:<br>
static void<br>
upload_vs_state(struct brw_context *brw)<br>
{<br>
struct gl_context *ctx = &brw->ctx;<br>
uint32_t floating_point_mode = 0;<br>
const int max_threads_shift = brw->is_haswell ?<br>
HSW_VS_MAX_THREADS_SHIFT : GEN6_VS_MAX_THREADS_SHIFT;<br>
<br>
/* the VS has a special requirement for synchronization due to a bug<br>
* in its constant handling.<br>
*/<br>
gen7_emit_vs_workaround_flush(<u></u>brw);<br>
<br>
emit_binding_table_pointers(_<u></u>3DSTATE_BINDING_TABLE_<u></u>POINTERS_VS, brw->vs.bind_bo_offset);<br>
emit_sampler(_3DSTATE_SAMPLER_<u></u>STATE_POINTERS_VS, brw->vs.sampler_offset);<br>
</blockquote>
<br></div>
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.<br>
<br>
Plus, both functions are exactly the same, and don't even have any functionality built in. We may as well just do:<br>
<br>
void<br>
emit_two_things(unsigned x, unsigned y)<br>
{<br>
BEGIN_BATCH(2);<br>
OUT_BATCH(x);<br>
OUT_BATCH(y);<br>
ADVANCE_BATCH();<br>
}<br>
<br>
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.<br></blockquote><div><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
emit_constants(3DSTATE_<u></u>CONSTANT_VS, &brw->vs.stage_state);<br>
</blockquote>
<br>
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.<br>
<br>
Plus, that function would have a single, obvious purpose - emit 3DSTATE_CONSTANT_XS for some X.<div class="im"><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
/* inline emit of 3DSTATE_VS just like we have today, since there's a<br>
* whole bunch of stage-specific stuff as is evident from the<br>
* alt_floating_point_mode, state_cmd, state_cmd_size,<br>
* stage_specific_cmd_data arguments.<br>
*/<br>
}<br>
<br>
that would share the definitely-common code without the contortions to<br>
make 3DSTATE_VS be "shared". Plus you get to fairly trivially reuse<br>
those 3 shared packets from blorp, which this patch didn't do today.<br>
(only "fairly" trivially because you'd want to have individual<br>
enable/disable packets for the constants to really trivially reuse from<br>
blorp).<br>
</blockquote>
<br></div>
I definitely don't want to share portions of 3DSTATE_VS.<br>
</blockquote></div><br></div><div class="gmail_extra">In an effort to help us make a more informed decision about this, I've investigated the following questions:<br><br></div><div class="gmail_extra">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.<br>
<br></div><div class="gmail_extra">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:<br></div><div class="gmail_extra">
<br>e6893b9 (Set MOCS L3 cacheability for IVB/BYT (v2))<br>2273b65 (Change L3 MOCS of 3DSTATE_CONSTANT_VS/PS)<br><br></div><div class="gmail_extra">These two commits would have been helped by the code sharing I've proposed, but not by sharing code for constant emission.<br>
</div><div class="gmail_extra"><br>decc708 (Upload separate per-stage sampler state tables)<br>f5a690c (i965: Split sampler count variable to be per-stage)<br><br></div><div class="gmail_extra"><div class="gmail_extra">In all of the above cases the savings would have been small--one diff hunk instead of two.<br>
</div><br>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.<br><br>
</div><br><div class="gmail_extra"><br></div><div class="gmail_extra">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:<br>
<br></div><div class="gmail_extra">- 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.<br><br></div><div class="gmail_extra">
- dw2[26] is "denormal mode" for PS; it's MBZ for VS, DS, and GS.<br><br></div><div class="gmail_extra">- 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.<br>
</div><br></div>- dw2[15:14] are "rounding mode" on PS only.<br><br></div>- "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.<br>
<br></div>- dw2[11] is "mask stack exception enable" on GS and PS; it's MBZ on VS and DS.<br><br></div>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.<br>
</div>