On 2 September 2011 15:46, Paul Berry <span dir="ltr"><<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5">On 2 September 2011 13:13, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br></div></div><div class="gmail_quote"><div>
<div></div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div></div><div>On Fri, 2 Sep 2011 09:06:48 -0700, Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> wrote:<br>
> ---<br>
> src/mesa/drivers/dri/i965/brw_vec4.h | 1 +<br>
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 32 ++++++++++++++---------<br>
> 2 files changed, 20 insertions(+), 13 deletions(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
> index 8c613bd..01313ec 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h<br>
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h<br>
> @@ -465,6 +465,7 @@ public:<br>
><br>
> void emit_ndc_computation();<br>
> void emit_psiz_and_flags(struct brw_reg reg);<br>
> + void emit_clip_distances(struct brw_reg reg, int offset);<br>
> int emit_vue_header_gen6(int header_mrf);<br>
> int emit_vue_header_gen4(int header_mrf);<br>
> void emit_urb_writes(void);<br>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
> index bd8878a..374cf8a 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
> @@ -1789,6 +1789,21 @@ vec4_visitor::emit_psiz_and_flags(struct brw_reg reg)<br>
> }<br>
> }<br>
><br>
> +void<br>
> +vec4_visitor::emit_clip_distances(struct brw_reg reg, int offset)<br>
> +{<br>
> + if (intel->gen < 6)<br>
> + /* Clip distance slots are set aside in gen5 because the hardware<br>
> + * requires them to be, but they are not used. */<br>
> + return;<br>
<br>
</div></div>Style consistency nits for the day: I like to see braces for if<br>
statements with a multi-line then case. Also, before your patch series<br>
there were only 15 instances of cuddling the "*/" onto the last line of<br>
a multi-line comment in our driver.<br></blockquote></div></div><div><br>Ah, yes. I figured I wouldn't be able to write 36 patches without failing to follow style conventions somewhere. Fortunately for me, these are conventions that I (a) didn't know about, and (b) don't mind following. I'll fix this and update docs/devinfo.html accordingly.<br>
</div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204, 204, 204);padding-left:1ex">
<br>
And now for some actual review: The hardware doesn't actually require<br>
the clip distance slots on gen5, we just fail to set up the hardware to<br>
not use them. I made a patch series at one point to do that, but given<br>
that I couldn't measure a performance difference and I was already<br>
living in fear of our VUE setup, I never pushed it.<br>
</blockquote></div></div><br>Yeah, I kind of suspected something like that was the case. I'll rewrite the comment to clarify this.<br>
</blockquote></div><br>Hmm, after some further thought about this, I'm kind of bewildered. The hardware docs definitely specify that clip distances belong in those slots, but I can't figure out what hardware settings would cause those clip distances to be used. (After all, clipping occurs in a driver-supplied GPU program on Gen5, and the GPU program we supply definitely doesn't read from those slots). But I think you made the right decision not to push any changes--since there's no measurable performance improvement, and we're not anticipating any future changes to Gen5 that would require reworking the VUE layout, there's no reason to move things around and risk a regression.<br>
<br>Considering my level of uncertainty about what actually is going on in the hardware, I think I'm not going to try to explain why we are setting aside space for clip distance with this somewhat weasely comment:<br>
<br> /* Clip distance slots are set aside in gen5, but they are not used. It<br> * is not clear whether we actually need to set aside space for them,<br> * but the performance cost is negligible.<br> */<br>
<br>...Unless you can suggest a better phrasing :)<br><br>