| Message ID | 1342728024-15055-5-git-send-email-galibert@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Olivier Galibert <galibert@pobox.com> writes: > Shaders, piglit test ones in particular, may write only to one of > gl_FrontColor/gl_BackColor. The standard is unclear on whether the > behaviour is defined in that case, but it seems reasonable to support > it. > > The choice done there to pick up whichever color was actually written > to. That makes most of the generated piglit tests useless to test the > backface selection, but it's simple and it works. > > Signed-off-by: Olivier Galibert <galibert@pobox.com> > --- > src/mesa/drivers/dri/i965/brw_fs.cpp | 9 +++++++++ > src/mesa/drivers/dri/i965/brw_wm_pass2.c | 9 +++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp > index 3f98137..3b62952 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp > @@ -972,6 +972,15 @@ fs_visitor::calculate_urb_setup() > if (c->key.vp_outputs_written & BITFIELD64_BIT(i)) { > int fp_index = _mesa_vert_result_to_frag_attrib((gl_vert_result) i); > > + /* Special case: two-sided vertex option, vertex program > + * only writes to the back color. Map it to the > + * associated front color location. > + */ > + if (i >= VERT_RESULT_BFC0 && i <= VERT_RESULT_BFC1 && > + ctx->VertexProgram._TwoSideEnabled && > + urb_setup[i - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0] == -1) > + fp_index = i - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0; In the fs_visitor (and brw_wm_pass*), you don't get to look at ctx-> state like that -- you're getting called once with some set of ctx state, but the program will get reused even if the ctx state changes. You'd have to get that state into the wm prog key, and use that, which would guarantee that you have the appropriate program code.
On Fri, Jul 20, 2012 at 10:01:03AM -0700, Eric Anholt wrote: > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp > > index 3f98137..3b62952 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp > > @@ -972,6 +972,15 @@ fs_visitor::calculate_urb_setup() > > if (c->key.vp_outputs_written & BITFIELD64_BIT(i)) { > > int fp_index = _mesa_vert_result_to_frag_attrib((gl_vert_result) i); > > > > + /* Special case: two-sided vertex option, vertex program > > + * only writes to the back color. Map it to the > > + * associated front color location. > > + */ > > + if (i >= VERT_RESULT_BFC0 && i <= VERT_RESULT_BFC1 && > > + ctx->VertexProgram._TwoSideEnabled && > > + urb_setup[i - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0] == -1) > > + fp_index = i - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0; > > In the fs_visitor (and brw_wm_pass*), you don't get to look at ctx-> > state like that -- you're getting called once with some set of ctx > state, but the program will get reused even if the ctx state changes. > You'd have to get that state into the wm prog key, and use that, which > would guarantee that you have the appropriate program code. Ok. OTOH, we don't actually *need* to look at TwoSideEnabled. If the rest of the condition triggers it's either correct or undefined behaviour. So we can do it systematically. OG.
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 3f98137..3b62952 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -972,6 +972,15 @@ fs_visitor::calculate_urb_setup() if (c->key.vp_outputs_written & BITFIELD64_BIT(i)) { int fp_index = _mesa_vert_result_to_frag_attrib((gl_vert_result) i); + /* Special case: two-sided vertex option, vertex program + * only writes to the back color. Map it to the + * associated front color location. + */ + if (i >= VERT_RESULT_BFC0 && i <= VERT_RESULT_BFC1 && + ctx->VertexProgram._TwoSideEnabled && + urb_setup[i - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0] == -1) + fp_index = i - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0; + /* The back color slot is skipped when the front color is * also written to. In addition, some slots can be * written in the vertex shader and not read in the diff --git a/src/mesa/drivers/dri/i965/brw_wm_pass2.c b/src/mesa/drivers/dri/i965/brw_wm_pass2.c index eacf7c0..48143f3 100644 --- a/src/mesa/drivers/dri/i965/brw_wm_pass2.c +++ b/src/mesa/drivers/dri/i965/brw_wm_pass2.c @@ -96,6 +96,15 @@ static void init_registers( struct brw_wm_compile *c ) if (c->key.vp_outputs_written & BITFIELD64_BIT(j)) { int fp_index = _mesa_vert_result_to_frag_attrib(j); + /* Special case: two-sided vertex option, vertex program + * only writes to the back color. Map it to the + * associated front color location. + */ + if (j >= VERT_RESULT_BFC0 && j <= VERT_RESULT_BFC1 && + intel->ctx.VertexProgram._TwoSideEnabled && + !(c->key.vp_outputs_written & BITFIELD64_BIT(j - VERT_RESULT_BFC0 + VERT_RESULT_COL0))) + fp_index = j - VERT_RESULT_BFC0 + FRAG_ATTRIB_COL0; + nr_interp_regs++; /* The back color slot is skipped when the front color is
Shaders, piglit test ones in particular, may write only to one of gl_FrontColor/gl_BackColor. The standard is unclear on whether the behaviour is defined in that case, but it seems reasonable to support it. The choice done there to pick up whichever color was actually written to. That makes most of the generated piglit tests useless to test the backface selection, but it's simple and it works. Signed-off-by: Olivier Galibert <galibert@pobox.com> --- src/mesa/drivers/dri/i965/brw_fs.cpp | 9 +++++++++ src/mesa/drivers/dri/i965/brw_wm_pass2.c | 9 +++++++++ 2 files changed, 18 insertions(+)