On 26 October 2011 12:17, Eric Anholt <span dir="ltr">&lt;<a href="mailto:eric@anholt.net">eric@anholt.net</a>&gt;</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 class="im">On Tue, 25 Oct 2011 20:38:26 -0700, Paul Berry &lt;<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>&gt; wrote:<br>
&gt; This required the following changes:<br>
&gt;<br>
&gt; - WM setup now makes the appropriate set of barycentric coordinates<br>
&gt;   (perspective vs. noperspective) available to the fragment shader,<br>
&gt;   based on whether the shader requires perspective interpolation,<br>
&gt;   noperspective interpolation, both, or neither.<br>
&gt;<br>
&gt; - The fragment shader backend now uses the appropriate set of<br>
&gt;   barycentric coordiantes when interpolating, based on the<br>
&gt;   interpolation mode returned by<br>
&gt;   ir_variable::determine_interpolation_mode().<br>
&gt;<br>
&gt; - SF setup now uses _mesa_compute_flag_attributes_flat() to determine<br>
&gt;   which attributes are to be flat shaded (as opposed to the old logic,<br>
&gt;   which only flat shaded colors).<br>
<br>
</div>_mesa_compute_flag_attributes_flat() doesn&#39;t appear to be a real thing.<br></blockquote><div><br>Oops, it was a real thing in v1 of the series, and I forgot to change the commit message.  I&#39;ll fix that.<br>
 </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
One code change comment below, then the whole v2 series is:<br>
<br>
Reviewed-by: Eric Anholt &lt;<a href="mailto:eric@anholt.net">eric@anholt.net</a>&gt;<br>
<div class="im"><br>
&gt; - CLIP setup now ensures that the clipper outputs non-perspective<br>
&gt;   barycentric coordinates when they are needed by the fragment shader.<br>
&gt;<br>
&gt; Fixes the remaining piglit tests of interpolation qualifiers that were<br>
&gt; failing:<br>
&gt; - interpolation-flat-*-smooth-none<br>
&gt; - interpolation-flat-other-flat-none<br>
&gt; - interpolation-noperspective-*<br>
&gt; - interpolation-smooth-gl_*Color-flat-*<br>
&gt; ---<br>
&gt;  src/mesa/drivers/dri/i965/brw_context.h     |    7 ++++-<br>
&gt;  src/mesa/drivers/dri/i965/brw_fs.cpp        |    9 ++++--<br>
&gt;  src/mesa/drivers/dri/i965/brw_wm.c          |   40 ++++++++++++++++++++++++---<br>
&gt;  src/mesa/drivers/dri/i965/gen6_clip_state.c |   32 +++++++++++++++++++++-<br>
&gt;  src/mesa/drivers/dri/i965/gen6_sf_state.c   |   21 +++++++-------<br>
&gt;  src/mesa/drivers/dri/i965/gen6_wm_state.c   |    9 +++++-<br>
&gt;  src/mesa/drivers/dri/i965/gen7_clip_state.c |   12 +++++++-<br>
&gt;  src/mesa/drivers/dri/i965/gen7_sf_state.c   |   21 +++++++-------<br>
&gt;  src/mesa/drivers/dri/i965/gen7_wm_state.c   |   10 +++++-<br>
&gt;  9 files changed, 126 insertions(+), 35 deletions(-)<br>
&gt;<br>
<br>
</div><div><div></div><div class="h5">&gt; diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c<br>
&gt; index 546d522..923beaf 100644<br>
&gt; --- a/src/mesa/drivers/dri/i965/brw_wm.c<br>
&gt; +++ b/src/mesa/drivers/dri/i965/brw_wm.c<br>
&gt; @@ -129,10 +129,41 @@ brw_wm_non_glsl_emit(struct brw_context *brw, struct brw_wm_compile *c)<br>
&gt;   * (see enum brw_wm_barycentric_interp_mode) is needed by the fragment shader.<br>
&gt;   */<br>
&gt;  unsigned<br>
&gt; -brw_compute_barycentric_interp_modes(void)<br>
&gt; +brw_compute_barycentric_interp_modes(bool shade_model_flat,<br>
&gt; +                                     const struct gl_fragment_program *fprog)<br>
&gt;  {<br>
&gt; -   /* At the moment the only interpolation mode we support is perspective. */<br>
&gt; -   return (1 &lt;&lt; BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC);<br>
&gt; +   unsigned barycentric_interp_modes = 0;<br>
&gt; +   int attr;<br>
&gt; +<br>
&gt; +   /* Loop through all fragment shader inputs to figure out what interpolation<br>
&gt; +    * modes are in use, and set the appropriate bits in<br>
&gt; +    * barycentric_interp_modes.<br>
&gt; +    */<br>
&gt; +   for (attr = 0; attr &lt; FRAG_ATTRIB_MAX; ++attr) {<br>
&gt; +      enum glsl_interp_qualifier interp_qualifier =<br>
&gt; +         fprog-&gt;InterpQualifier[attr];<br>
&gt; +      bool is_gl_Color = attr == FRAG_ATTRIB_COL0 || attr == FRAG_ATTRIB_COL1;<br>
&gt; +<br>
&gt; +      /* Ignore unused inputs. */<br>
&gt; +      if (!(fprog-&gt;Base.InputsRead &amp; BITFIELD64_BIT(attr)))<br>
&gt; +         continue;<br>
&gt; +<br>
&gt; +      /* Ignore WPOS and FACE, because they don&#39;t require interpolation. */<br>
&gt; +      if (attr == FRAG_ATTRIB_WPOS || attr == FRAG_ATTRIB_FACE)<br>
&gt; +         continue;<br>
&gt; +<br>
&gt; +      if (interp_qualifier == INTERP_QUALIFIER_NOPERSPECTIVE) {<br>
&gt; +         barycentric_interp_modes |=<br>
&gt; +            1 &lt;&lt; BRW_WM_NONPERSPECTIVE_PIXEL_BARYCENTRIC;<br>
&gt; +      } else if (interp_qualifier == INTERP_QUALIFIER_SMOOTH ||<br>
&gt; +                 (!(shade_model_flat &amp;&amp; is_gl_Color) &amp;&amp;<br>
&gt; +                  interp_qualifier == INTERP_QUALIFIER_NONE)) {<br>
&gt; +         barycentric_interp_modes |=<br>
&gt; +            1 &lt;&lt; BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC;<br>
&gt; +      }<br>
&gt; +   }<br>
&gt; +<br>
&gt; +   return barycentric_interp_modes;<br>
&gt;  }<br>
<br>
</div></div>One interesting behvaior change here is that we might not flag<br>
BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC.  This looks safe, because we only<br>
use it for LINTERP for smooth, and for gl_FragCoord which is also an<br>
input.<br></blockquote><div><br>In point of fact, we don&#39;t even use perspective barycentric coords for gl_FragCoord on Gen6+, since the hardware computes it for us (it&#39;s in R27-28 of the fragment shader payload).<br>
 </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
I do wonder about the value of minimally flagging the perspective<br>
barycentric coord.  It seems like a lot of this would end up simpler if<br>
we just had a &quot;does the program use noperspective?&quot; flag in the program,<br>
and then we wouldn&#39;t have to do this work at state emit time, by<br>
assuming that we always use perspective.<br></blockquote><div><br>Yeah, that might make some of this code faster.  The tradeoff would be that it might slow down rendering slightly for these cases where perspective barycentric coords aren&#39;t used, by forcing the hardware to populate two registers that aren&#39;t going to be used.  I don&#39;t really have good intuition about how to weigh these two possibilities against each other.<br>
 </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
On the other hand, the rule is make it work, then make it work fast.<br>
These patches look great as is.<br>
<div class="im"><br>
&gt; diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c<br>
&gt; index e0004c5..2a9ebea 100644<br>
&gt; --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c<br>
&gt; +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c<br>
&gt; @@ -40,6 +40,10 @@ upload_wm_state(struct brw_context *brw)<br>
&gt;        brw_fragment_program_const(brw-&gt;fragment_program);<br>
&gt;     bool writes_depth = false;<br>
&gt;     uint32_t dw1;<br>
&gt; +   unsigned barycentric_interp_modes;<br>
&gt; +<br>
&gt; +   /* _NEW_LIGHT */<br>
&gt; +   bool flat_shade = (ctx-&gt;Light.ShadeModel == GL_FLAT);<br>
&gt;<br>
&gt;     dw1 = 0;<br>
&gt;     dw1 |= GEN7_WM_STATISTICS_ENABLE;<br>
&gt; @@ -61,6 +65,8 @@ upload_wm_state(struct brw_context *brw)<br>
&gt;        writes_depth = true;<br>
&gt;        dw1 |= GEN7_WM_PSCDEPTH_ON;<br>
&gt;     }<br>
&gt; +   barycentric_interp_modes =<br>
&gt; +      brw_compute_barycentric_interp_modes(flat_shade, &amp;fp-&gt;program);<br>
<br>
</div>I&#39;d move this declaration down to the only use, or not make it a<br>
separate declaration at all.<br>
<div><div></div><div class="h5"><br></div></div></blockquote><div><br>You&#39;re right, separating this declaration out from its use is silly.  I think I&#39;d rather keep the code that depends on BRW_NEW_FRAGMENT_PROGRAM together under the &quot;/* BRW_NEW_FRAGMENT_PROGRAM */&quot; comment, though, so I&#39;ll move the use up and merge it with the declaration.<br>
 </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div><div class="h5">
&gt;     /* _NEW_COLOR */<br>
&gt;     if (fp-&gt;program.UsesKill || ctx-&gt;Color.AlphaEnabled)<br>
&gt; @@ -72,7 +78,7 @@ upload_wm_state(struct brw_context *brw)<br>
&gt;        dw1 |= GEN7_WM_DISPATCH_ENABLE;<br>
&gt;     }<br>
&gt;<br>
&gt; -   dw1 |= brw_compute_barycentric_interp_modes() &lt;&lt;<br>
&gt; +   dw1 |= barycentric_interp_modes &lt;&lt;<br>
&gt;        GEN7_WM_BARYCENTRIC_INTERPOLATION_MODE_SHIFT;<br>
</div></div></blockquote></div><br>