On 26 October 2011 12:17, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net">eric@anholt.net</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 class="im">On Tue, 25 Oct 2011 20:38:26 -0700, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote:<br>
> This required the following changes:<br>
><br>
> - WM setup now makes the appropriate set of barycentric coordinates<br>
> (perspective vs. noperspective) available to the fragment shader,<br>
> based on whether the shader requires perspective interpolation,<br>
> noperspective interpolation, both, or neither.<br>
><br>
> - The fragment shader backend now uses the appropriate set of<br>
> barycentric coordiantes when interpolating, based on the<br>
> interpolation mode returned by<br>
> ir_variable::determine_interpolation_mode().<br>
><br>
> - SF setup now uses _mesa_compute_flag_attributes_flat() to determine<br>
> which attributes are to be flat shaded (as opposed to the old logic,<br>
> which only flat shaded colors).<br>
<br>
</div>_mesa_compute_flag_attributes_flat() doesn'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'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 <<a href="mailto:eric@anholt.net">eric@anholt.net</a>><br>
<div class="im"><br>
> - CLIP setup now ensures that the clipper outputs non-perspective<br>
> barycentric coordinates when they are needed by the fragment shader.<br>
><br>
> Fixes the remaining piglit tests of interpolation qualifiers that were<br>
> failing:<br>
> - interpolation-flat-*-smooth-none<br>
> - interpolation-flat-other-flat-none<br>
> - interpolation-noperspective-*<br>
> - interpolation-smooth-gl_*Color-flat-*<br>
> ---<br>
> src/mesa/drivers/dri/i965/brw_context.h | 7 ++++-<br>
> src/mesa/drivers/dri/i965/brw_fs.cpp | 9 ++++--<br>
> src/mesa/drivers/dri/i965/brw_wm.c | 40 ++++++++++++++++++++++++---<br>
> src/mesa/drivers/dri/i965/gen6_clip_state.c | 32 +++++++++++++++++++++-<br>
> src/mesa/drivers/dri/i965/gen6_sf_state.c | 21 +++++++-------<br>
> src/mesa/drivers/dri/i965/gen6_wm_state.c | 9 +++++-<br>
> src/mesa/drivers/dri/i965/gen7_clip_state.c | 12 +++++++-<br>
> src/mesa/drivers/dri/i965/gen7_sf_state.c | 21 +++++++-------<br>
> src/mesa/drivers/dri/i965/gen7_wm_state.c | 10 +++++-<br>
> 9 files changed, 126 insertions(+), 35 deletions(-)<br>
><br>
<br>
</div><div><div></div><div class="h5">> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c<br>
> index 546d522..923beaf 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_wm.c<br>
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c<br>
> @@ -129,10 +129,41 @@ brw_wm_non_glsl_emit(struct brw_context *brw, struct brw_wm_compile *c)<br>
> * (see enum brw_wm_barycentric_interp_mode) is needed by the fragment shader.<br>
> */<br>
> unsigned<br>
> -brw_compute_barycentric_interp_modes(void)<br>
> +brw_compute_barycentric_interp_modes(bool shade_model_flat,<br>
> + const struct gl_fragment_program *fprog)<br>
> {<br>
> - /* At the moment the only interpolation mode we support is perspective. */<br>
> - return (1 << BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC);<br>
> + unsigned barycentric_interp_modes = 0;<br>
> + int attr;<br>
> +<br>
> + /* Loop through all fragment shader inputs to figure out what interpolation<br>
> + * modes are in use, and set the appropriate bits in<br>
> + * barycentric_interp_modes.<br>
> + */<br>
> + for (attr = 0; attr < FRAG_ATTRIB_MAX; ++attr) {<br>
> + enum glsl_interp_qualifier interp_qualifier =<br>
> + fprog->InterpQualifier[attr];<br>
> + bool is_gl_Color = attr == FRAG_ATTRIB_COL0 || attr == FRAG_ATTRIB_COL1;<br>
> +<br>
> + /* Ignore unused inputs. */<br>
> + if (!(fprog->Base.InputsRead & BITFIELD64_BIT(attr)))<br>
> + continue;<br>
> +<br>
> + /* Ignore WPOS and FACE, because they don't require interpolation. */<br>
> + if (attr == FRAG_ATTRIB_WPOS || attr == FRAG_ATTRIB_FACE)<br>
> + continue;<br>
> +<br>
> + if (interp_qualifier == INTERP_QUALIFIER_NOPERSPECTIVE) {<br>
> + barycentric_interp_modes |=<br>
> + 1 << BRW_WM_NONPERSPECTIVE_PIXEL_BARYCENTRIC;<br>
> + } else if (interp_qualifier == INTERP_QUALIFIER_SMOOTH ||<br>
> + (!(shade_model_flat && is_gl_Color) &&<br>
> + interp_qualifier == INTERP_QUALIFIER_NONE)) {<br>
> + barycentric_interp_modes |=<br>
> + 1 << BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC;<br>
> + }<br>
> + }<br>
> +<br>
> + return barycentric_interp_modes;<br>
> }<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't even use perspective barycentric coords for gl_FragCoord on Gen6+, since the hardware computes it for us (it'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 "does the program use noperspective?" flag in the program,<br>
and then we wouldn'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't used, by forcing the hardware to populate two registers that aren't going to be used. I don'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>
> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c<br>
> index e0004c5..2a9ebea 100644<br>
> --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c<br>
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c<br>
> @@ -40,6 +40,10 @@ upload_wm_state(struct brw_context *brw)<br>
> brw_fragment_program_const(brw->fragment_program);<br>
> bool writes_depth = false;<br>
> uint32_t dw1;<br>
> + unsigned barycentric_interp_modes;<br>
> +<br>
> + /* _NEW_LIGHT */<br>
> + bool flat_shade = (ctx->Light.ShadeModel == GL_FLAT);<br>
><br>
> dw1 = 0;<br>
> dw1 |= GEN7_WM_STATISTICS_ENABLE;<br>
> @@ -61,6 +65,8 @@ upload_wm_state(struct brw_context *brw)<br>
> writes_depth = true;<br>
> dw1 |= GEN7_WM_PSCDEPTH_ON;<br>
> }<br>
> + barycentric_interp_modes =<br>
> + brw_compute_barycentric_interp_modes(flat_shade, &fp->program);<br>
<br>
</div>I'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're right, separating this declaration out from its use is silly. I think I'd rather keep the code that depends on BRW_NEW_FRAGMENT_PROGRAM together under the "/* BRW_NEW_FRAGMENT_PROGRAM */" comment, though, so I'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">
> /* _NEW_COLOR */<br>
> if (fp->program.UsesKill || ctx->Color.AlphaEnabled)<br>
> @@ -72,7 +78,7 @@ upload_wm_state(struct brw_context *brw)<br>
> dw1 |= GEN7_WM_DISPATCH_ENABLE;<br>
> }<br>
><br>
> - dw1 |= brw_compute_barycentric_interp_modes() <<<br>
> + dw1 |= barycentric_interp_modes <<<br>
> GEN7_WM_BARYCENTRIC_INTERPOLATION_MODE_SHIFT;<br>
</div></div></blockquote></div><br>