<div dir="ltr">On 10 September 2013 12:44, 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><div>On 09/09/2013 08:20 AM, Paul Berry wrote:<br>
> +void<br>
> +vec4_gs_visitor::emit_control_data_bits()<br>
> +{<br>
> + assert(c->control_data_bits_per_vertex != 0);<br>
> +<br>
> + /* Since the URB_WRITE_OWORD message operates with 128-bit (vec4 sized)<br>
> + * granularity, we need to use two tricks to ensure that the batch of 32<br>
> + * control data bits is written to the appropriate DWORD in the URB. To<br>
> + * select which vec4 we are writing to, we use the "slot {0,1} offset"<br>
> + * fields of the message header. To select which DWORD in the vec4 we are<br>
> + * writing to, we use the channel mask fields of the message header. To<br>
> + * avoid punalizing geometry shaders that emit a small number of vertices<br>
<br>
</div></div>PUNY GEOMETRY SHADERS! I think you meant "penalized" :)<br></blockquote><div><br></div><div>Fixed, thanks :)<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">
<div><br>
> + * with extra bookkeeping, we only do each of these tricks when<br>
> + * c->prog_data.control_data_header_size_bits is large enough to make it<br>
> + * necessary.<br>
> + *<br>
> + * Note: this means that if we're outputting just a single DWORD of control<br>
> + * data bits, we'll actually replicate it four times since we won't do any<br>
> + * channel masking. But that's not a problem since in this case the<br>
> + * hardware only pays attention to the first DWORD.<br>
> + */<br>
> + enum brw_urb_write_flags urb_write_flags = BRW_URB_WRITE_OWORD;<br>
> + if (c->control_data_header_size_bits > 32)<br>
> + urb_write_flags = urb_write_flags | BRW_URB_WRITE_USE_CHANNEL_MASKS;<br>
<br>
</div>Maybe shorten this to:<br>
urb_write_flags |= BRW_URB_WRITE_USE_CHANNEL_MASKS;<br>
<div><br>
> + if (c->control_data_header_size_bits > 128)<br>
> + urb_write_flags = urb_write_flags | BRW_URB_WRITE_PER_SLOT_OFFSET;<br>
<br>
</div>Ditto.<br></blockquote><div><br></div><div>Sadly, commit cfe39ea (i965: Allow C++ type safety in the use of enum brw_urb_write_flags.) prevents this. It was one of the disadvantages I listed when I sent the patch out for review (<a href="http://lists.freedesktop.org/archives/mesa-dev/2013-August/043775.html" target="_blank">http://lists.freedesktop.org/archives/mesa-dev/2013-August/043775.html</a>). Chad and Francisco had some ideas for ways to avoid those disadvantages, but their ideas didn't seem to garner much support.<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">
<div><div><br>
> void<br>
> vec4_gs_visitor::visit(ir_emit_vertex *)<br>
> {<br>
> @@ -238,6 +383,45 @@ vec4_gs_visitor::visit(ir_emit_vertex *)<br>
> src_reg(num_output_vertices), BRW_CONDITIONAL_L));<br>
> emit(IF(BRW_PREDICATE_NORMAL));<br>
> {<br>
> + /* If we're outputting 32 control data bits or less, then we can wait<br>
> + * until the shader is over to output them all. Otherwise we need to<br>
> + * output them as we go. Now is the time to do it, since we're about to<br>
> + * output the vertex_count'th vertex, so it's guaranteed that the<br>
> + * control data bits associated with the (vertex_count - 1)th vertex is<br>
<br>
</div></div>"are" correct? (control data bits is plural...)<br></blockquote><div><br></div><div>Fixed, thanks.<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">
<div><br>
> + * correct.<br>
> + */<br>
> + if (c->control_data_header_size_bits > 32) {<br>
> + this->current_annotation = "emit vertex: emit control data bits";<br>
> + /* Only emit control data bits if we've finished accumulating a batch<br>
> + * of 32 bits. This is the case when:<br>
> + *<br>
> + * (vertex_count * bits_per_vertex) % 32 == 0<br>
> + *<br>
> + * Since bits_per_vertex is a power of two, that's equivalent to:<br>
> + *<br>
> + * vertex_count & (32 / bits_per_vertex - 1) == 0<br>
> + */<br>
<br>
</div>I had a bit of trouble with this derivation, but Paul helped me out on IRC.<br>
<br>
Although it's not mentioned here, bits_per_vertex is either 1 or 2.<br>
<br>
The first case is easy:<br>
<div>(vertex_count * bits_per_vertex) % 32 == 0<br>
</div>vertex_count % 32 == 0<br>
vertex_count & 31 == 0 (common trick)<br>
<br>
For the second case, we're writing 2 bits per vertex, which means that<br>
every 16 vertices, we need to emit something. So we can actually treat<br>
this as (vertex_count % 16 == 0), which is equivalent to (vertex_count &<br>
15 == 0).<br></blockquote><div><br></div><div>Not that it really matters, since as you point out bits_per_vertex is always either 1 or 2, but technically the derivation works for any value of bits_per_vertex that's a power of 2. I've expanded the comment to include a fuller derivation:<br>
<br> /* Only emit control data bits if we've finished accumulating a batch<br> * of 32 bits. This is the case when:<br> *<br> * (vertex_count * bits_per_vertex) % 32 == 0<br> *<br>
* (in other words, when the last 5 bits of vertex_count *<br> * bits_per_vertex are 0). Assuming bits_per_vertex == 2^n for some<br> * integer n (which is always the case, since bits_per_vertex is<br>
* always 1 or 2), this is equivalent to requiring that the last 5-n<br> * bits of vertex_count are 0:<br> *<br> * vertex_count & (2^(5-n) - 1) == 0<br> *<br> * 2^(5-n) == 2^5 / 2^n == 32 / bits_per_vertex, so this is<br>
* equivalent to:<br> *<br> * vertex_count & (32 / bits_per_vertex - 1) == 0<br> */<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div><br>
> @@ -253,7 +437,47 @@ vec4_gs_visitor::visit(ir_emit_vertex *)<br>
> void<br>
> vec4_gs_visitor::visit(ir_end_primitive *)<br>
> {<br>
> - assert(!"Not implemented yet");<br>
> + /* We can only do EndPrimitive() functionality when the control data<br>
> + * consists of cut bits. Fortunately, the only time it isn't is when the<br>
> + * output type is points, in which case EndPrimitive() is a no-op.<br>
> + */<br>
> + if (c->prog_data.control_data_format !=<br>
> + GEN7_GS_CONTROL_DATA_FORMAT_GSCTL_CUT) {<br>
> + return;<br>
> + }<br>
> +<br>
> + /* Cut bits use one bit per vertex. */<br>
> + assert(c->control_data_bits_per_vertex == 1);<br>
> +<br>
> + /* Cut bit n should be set to 1 if EndPrimitive() was called after emitting<br>
> + * vertex n, 0 otherwise. So all we need to do here is mark bit<br>
> + * (vertex_count - 1) % 32 in the cut_bits register to indicate that<br>
> + * EndPrimitive() was called after emitting vertex (vertex_count - 1);<br>
> + * vec4_gs_visitor::emit_control_data_bits() will take care of the rest.<br>
> + *<br>
> + * Note that if EndPrimitve() is called before emitting any vertices, this<br>
> + * will cause us to set bit 31 of the control_data_bits register to 1.<br>
> + * That's fine because:<br>
> + *<br>
> + * - If max_vertices < 32, then vertex number 31 (zero-based) will never be<br>
> + * output, so the hardware will ignore cut bit 31.<br>
> + *<br>
> + * - If max_vertices == 32, then vertex number 31 is guaranteed to be the<br>
> + * last vertex, so setting cut bit 31 has no effect (since the primitive<br>
> + * is automatically ended when the GS terminates).<br>
> + *<br>
> + * - If max_vertices > 32, then the ir_emit_vertex visitor will reset the<br>
> + * control_data_bits register to 0 when the first vertex is emitted.<br>
> + */<br>
> +<br>
> + /* control_data_bits |= 1 << ((vertex_count - 1) % 32) */<br>
> + src_reg one(this, glsl_type::uint_type);<br>
> + emit(MOV(dst_reg(one), 1u));<br>
> + src_reg prev_count(this, glsl_type::uint_type);<br>
> + emit(ADD(dst_reg(prev_count), this->vertex_count, 0xffffffffu));<br>
<br>
</div></div>This threw me a bit...wasn't obviously equivalent to the formula in the<br>
comment above.<br>
<br>
But after some investigation,<br>
(vertex_count - 1) % 32 == vertex_count + 0xffffffffu<br>
for vertex_count in [1..31]. They're different for vertex_count == 0,<br>
but 1 << either-value is equivalent, so the final expression checks out.<br>
<br>
Clever.<br></blockquote><div><br></div><div>Actually, that alone wouldn't be good enough, because vertex_count can get larger than 31 (it counts all vertices output by the geometry shader, not just those that we are currently accumulating cut bits for). The real reason they are equivalent is in a detail that's buried in the documentation of the SHL instruction: it ignores all but the bottom 5 bits of its second source argument, so it's as if there's an implicit "% 32" on that argument. I've added a comment above the SHL instruction to make that clearer:<br>
<br> /* Note: we're relying on the fact that the GEN SHL instruction only pays<br> * attention to the lower 5 bits of its second source argument, so on this<br> * architecture, 1 << (vertex_count - 1) is equivalent to 1 <<<br>
* ((vertex_count - 1) % 32).<br> */<br> emit(SHL(dst_reg(mask), one, prev_count));<br><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">
<div><br>
> + src_reg mask(this, glsl_type::uint_type);<br>
> + emit(SHL(dst_reg(mask), one, prev_count));<br>
> + emit(OR(dst_reg(this->control_data_bits), this->control_data_bits, mask));<br>
> }<br>
><br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.h b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.h<br>
> index 1193e28..90dd1de 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.h<br>
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.h<br>
> @@ -96,8 +96,10 @@ protected:<br>
><br>
> private:<br>
> int setup_varying_inputs(int payload_reg, int *attribute_map);<br>
> + void emit_control_data_bits();<br>
><br>
> src_reg vertex_count;<br>
> + src_reg control_data_bits;<br>
> const struct brw_gs_compile * const c;<br>
> };<br>
><br>
><br>
<br>
</div>After looking at this a bunch, I think your approach makes sense, and I<br>
haven't been able to come up with anything simpler. Nice work.<br>
<br>
Other than my few questions and comments, this series is:<br>
Reviewed-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>><br>
</blockquote></div><br></div><div class="gmail_extra">Thanks so much for the detailed review, Ken. Assuming I don't hear anything to the contrary, I'll plan on pushing the series tomorrow.<br></div></div>