Whoops, forgot to copy the list on this reply.<br><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">Paul Berry</b> <span dir="ltr"><<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>></span><br>
Date: 2 September 2011 15:46<br>Subject: Re: [PATCH 09/36] i965: new VS: move clip distance computation (GEN5+) to a separate function.<br>To: Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>><br><br>
<br><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>
</div><br>