On 09/10/2013 09:56 PM, Paul Berry wrote: > On 10 September 2013 12:44, Kenneth Graunke wrote: [snip] > Maybe shorten this to: > urb_write_flags |= BRW_URB_WRITE_USE_CHANNEL_MASKS; > > > + if (c->control_data_header_size_bits > 128) > > + urb_write_flags = urb_write_flags | > BRW_URB_WRITE_PER_SLOT_OFFSET; > > Ditto. > > > 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 > (http://lists.freedesktop.org/archives/mesa-dev/2013-August/043775.html). > Chad and Francisco had some ideas for ways to avoid those disadvantages, > but their ideas didn't seem to garner much support.
Oh, right...not a big deal either way. [snip] > I had a bit of trouble with this derivation, but Paul helped me out > on IRC. > > Although it's not mentioned here, bits_per_vertex is either 1 or 2. > > The first case is easy: > (vertex_count * bits_per_vertex) % 32 == 0 > vertex_count % 32 == 0 > vertex_count & 31 == 0 (common trick) > > For the second case, we're writing 2 bits per vertex, which means that > every 16 vertices, we need to emit something. So we can actually treat > this as (vertex_count % 16 == 0), which is equivalent to (vertex_count & > 15 == 0). > > > 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: > > /* Only emit control data bits if we've finished accumulating a > batch > * of 32 bits. This is the case when: > * > * (vertex_count * bits_per_vertex) % 32 == 0 > * > * (in other words, when the last 5 bits of vertex_count * > * bits_per_vertex are 0). Assuming bits_per_vertex == 2^n for > some > * integer n (which is always the case, since bits_per_vertex is > * always 1 or 2), this is equivalent to requiring that the > last 5-n > * bits of vertex_count are 0: > * > * vertex_count & (2^(5-n) - 1) == 0 > * > * 2^(5-n) == 2^5 / 2^n == 32 / bits_per_vertex, so this is > * equivalent to: > * > * vertex_count & (32 / bits_per_vertex - 1) == 0 > */ Ah, that makes a lot of sense. Thanks for the extra comments. [snip] > > + /* control_data_bits |= 1 << ((vertex_count - 1) % 32) */ > > + src_reg one(this, glsl_type::uint_type); > > + emit(MOV(dst_reg(one), 1u)); > > + src_reg prev_count(this, glsl_type::uint_type); > > + emit(ADD(dst_reg(prev_count), this->vertex_count, 0xffffffffu)); > > This threw me a bit...wasn't obviously equivalent to the formula in the > comment above. > > But after some investigation, > (vertex_count - 1) % 32 == vertex_count + 0xffffffffu > for vertex_count in [1..31]. They're different for vertex_count == 0, > but 1 << either-value is equivalent, so the final expression checks out. > > Clever. > > 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: > > /* Note: we're relying on the fact that the GEN SHL instruction only pays > * attention to the lower 5 bits of its second source argument, so on > this > * architecture, 1 << (vertex_count - 1) is equivalent to 1 << > * ((vertex_count - 1) % 32). > */ > emit(SHL(dst_reg(mask), one, prev_count)); Ohh. Ew. Yeah, it works out nicely in this case, though, so as long as it's commented it's probably fine. [snip] > 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. Sounds good to me! --Ken _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
