| Message ID | 1341082215-22933-2-git-send-email-galibert@pobox.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 30 June 2012 11:50, Olivier Galibert <galibert@pobox.com> wrote: > There was... confusion about which register goes where. With that > patch urb_setup is in line with the vue setup, even when these > annoying backcolor slots are used. And in addition the stray mov into > lalaland is avoided when only one of the front/back slots is used and > the backface is looking at you. The code instead picks whatever slot > was written to by the vertex shader. That makes most of the generated > piglit tests useless to test the backface selection though. > It looks like this patch does three separate things: 1. modify brw_fs.cpp and brw_wm_pass2.c to properly account for the fact that prior to Gen6, the SF stage of the pipeline doesn't remove backfacing colors from the URB entry, so the fragment shader compiler needs to skip over them when interpreting the contents of the thread payload. 2. rework the do_twoside_color() function so that it's easier to follow, and doesn't emit unnecessary code when there's nothing to do. 3. ensure that if the vertex program writes to gl_BackColor but not gl_FrontColor, then the gl_BackColor value shows up in the fragment shader, regardless of whether the polygon is front-facing or back-facing. Can you split this into three separate patches? That will make it easier to troubleshoot in case we find bugs with these patches in the future. Also, I'm not convinced that #3 is necessary. Is there something in the spec that dictates this behaviour? My reading of the spec is that if the vertex shader writes to gl_BackColor but not glFrontColor, then the contents of gl_Color in the fragment shader is undefined. If we *do* decide that #3 is necessary, then I think a better way to accomplish it is to handle it in the GLSL vertex shader front-end, by replacing gl_BackColor with gl_FrontColor in cases where gl_FrontColor is not written to. That way our special case code to handle this situation would be in just one place, rather than in three places (both fragment shader back-ends, and the SF program). Also then the fix would apply to all hardware, not just Intel Gen4-5. Finally, I couldn't figure out what you meant by "the stray mov into lalaland". Can you elaborate on which piece of code used to generate that stray mov, and why it doesn't anymore? Thanks. I have a few additional comments below. > > Signed-off-by: Olivier Galibert <galibert@pobox.com> > --- > src/mesa/drivers/dri/i965/brw_fs.cpp | 18 +++++- > src/mesa/drivers/dri/i965/brw_sf.c | 3 +- > src/mesa/drivers/dri/i965/brw_sf_emit.c | 93 > +++++++++++++++++------------- > src/mesa/drivers/dri/i965/brw_wm_pass2.c | 19 +++++- > 4 files changed, 89 insertions(+), 44 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp > b/src/mesa/drivers/dri/i965/brw_fs.cpp > index 6cef08a..710f2ff 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp > @@ -721,8 +721,24 @@ 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 > + * fragment shader. So the register number must always be > + * incremented, mapped or not. > + */ > if (fp_index >= 0) > - urb_setup[fp_index] = urb_next++; > + urb_setup[fp_index] = urb_next; > + urb_next++; > } > } > > diff --git a/src/mesa/drivers/dri/i965/brw_sf.c > b/src/mesa/drivers/dri/i965/brw_sf.c > index 23a874a..7867ab5 100644 > --- a/src/mesa/drivers/dri/i965/brw_sf.c > +++ b/src/mesa/drivers/dri/i965/brw_sf.c > @@ -192,7 +192,8 @@ brw_upload_sf_prog(struct brw_context *brw) > > /* _NEW_LIGHT */ > key.do_flat_shading = (ctx->Light.ShadeModel == GL_FLAT); > - key.do_twoside_color = (ctx->Light.Enabled && > ctx->Light.Model.TwoSide); > + key.do_twoside_color = (ctx->Light.Enabled && > ctx->Light.Model.TwoSide) || > + ctx->VertexProgram._TwoSideEnabled; > You're right that this needs fixing, but I think it should just be key.do_twoside_color = ctx->VertexProgram._TwoSideEnabled; My reasoning is: _TwoSideEnabled is derived state. It is set by src/mesa/main/state.c's update_twoside() function to either (a) ctx->VertexProgram.TwoSideEnabled, if there is a vertex program, or (b) ctx->Light.Enabled && ctx->Light.Model.TwoSide, if there isn't. Since it already accounts correctly for both ways the user can enable two-sided lighting, there's no reason for us to consult ctx->Light.Model.TwoSide in the driver. Note: brw_clip.c has some references to Light.Model.TwoSide as well. I suspect they are also wrong, but I'm not exactly sure what the correct code is. I suppose we should address that in a later patch :) > > /* _NEW_POLYGON */ > if (key.do_twoside_color) { > diff --git a/src/mesa/drivers/dri/i965/brw_sf_emit.c > b/src/mesa/drivers/dri/i965/brw_sf_emit.c > index ff6383b..9d8aa38 100644 > --- a/src/mesa/drivers/dri/i965/brw_sf_emit.c > +++ b/src/mesa/drivers/dri/i965/brw_sf_emit.c > @@ -79,24 +79,9 @@ have_attr(struct brw_sf_compile *c, GLuint attr) > /*********************************************************************** > * Twoside lighting > */ > -static void copy_bfc( struct brw_sf_compile *c, > - struct brw_reg vert ) > -{ > - struct brw_compile *p = &c->func; > - GLuint i; > - > - for (i = 0; i < 2; i++) { > - if (have_attr(c, VERT_RESULT_COL0+i) && > - have_attr(c, VERT_RESULT_BFC0+i)) > - brw_MOV(p, > - get_vert_result(c, vert, VERT_RESULT_COL0+i), > - get_vert_result(c, vert, VERT_RESULT_BFC0+i)); > - } > -} > - > - > static void do_twoside_color( struct brw_sf_compile *c ) > { > + GLuint i, need_0, need_1; > struct brw_compile *p = &c->func; > GLuint backface_conditional = c->key.frontface_ccw ? BRW_CONDITIONAL_G > : BRW_CONDITIONAL_L; > > @@ -105,12 +90,14 @@ static void do_twoside_color( struct brw_sf_compile > *c ) > if (c->key.primitive == SF_UNFILLED_TRIS) > return; > > - /* XXX: What happens if BFC isn't present? This could only happen > - * for user-supplied vertex programs, as t_vp_build.c always does > - * the right thing. > + /* If the vertex shader provides both front and backface color, do > + * the selection. Otherwise the generated code will pick up > + * whichever there is. > */ > - if (!(have_attr(c, VERT_RESULT_COL0) && have_attr(c, > VERT_RESULT_BFC0)) && > - !(have_attr(c, VERT_RESULT_COL1) && have_attr(c, > VERT_RESULT_BFC1))) > + need_0 = have_attr(c, VERT_RESULT_COL0) && have_attr(c, > VERT_RESULT_BFC0); > + need_1 = have_attr(c, VERT_RESULT_COL1) && have_attr(c, > VERT_RESULT_BFC1); > + > + if (!need_0 && !need_1) > return; > > /* Need to use BRW_EXECUTE_4 and also do an 4-wide compare in order > @@ -121,12 +108,15 @@ static void do_twoside_color( struct brw_sf_compile > *c ) > brw_push_insn_state(p); > brw_CMP(p, vec4(brw_null_reg()), backface_conditional, c->det, > brw_imm_f(0)); > brw_IF(p, BRW_EXECUTE_4); > - { > - switch (c->nr_verts) { > - case 3: copy_bfc(c, c->vert[2]); > - case 2: copy_bfc(c, c->vert[1]); > - case 1: copy_bfc(c, c->vert[0]); > - } > + for (i=0; i<c->nr_verts; i++) { > + if (need_0) > + brw_MOV(p, > + get_vert_result(c, c->vert[i], VERT_RESULT_COL0), > + get_vert_result(c, c->vert[i], VERT_RESULT_BFC0)); > + if (need_1) > + brw_MOV(p, > + get_vert_result(c, c->vert[i], VERT_RESULT_COL1), > + get_vert_result(c, c->vert[i], VERT_RESULT_BFC1)); > This is much easier to follow as a loop. Thanks for cleaning this up. > } > brw_ENDIF(p); > brw_pop_insn_state(p); > @@ -139,20 +129,27 @@ static void do_twoside_color( struct brw_sf_compile > *c ) > */ > > #define VERT_RESULT_COLOR_BITS (BITFIELD64_BIT(VERT_RESULT_COL0) | \ > - BITFIELD64_BIT(VERT_RESULT_COL1)) > + BITFIELD64_BIT(VERT_RESULT_COL1)) > > static void copy_colors( struct brw_sf_compile *c, > struct brw_reg dst, > - struct brw_reg src) > + struct brw_reg src, > + int allow_twoside) > { > struct brw_compile *p = &c->func; > GLuint i; > > for (i = VERT_RESULT_COL0; i <= VERT_RESULT_COL1; i++) { > - if (have_attr(c,i)) > + if (have_attr(c,i)) { > brw_MOV(p, > get_vert_result(c, dst, i), > get_vert_result(c, src, i)); > + > + } else if(allow_twoside && have_attr(c, i - VERT_RESULT_COL0 + > VERT_RESULT_BFC0)) { > + brw_MOV(p, > + get_vert_result(c, dst, i - VERT_RESULT_COL0 + > VERT_RESULT_BFC0), > + get_vert_result(c, src, i - VERT_RESULT_COL0 + > VERT_RESULT_BFC0)); > + } > } > } > > @@ -167,9 +164,19 @@ static void do_flatshade_triangle( struct > brw_sf_compile *c ) > struct brw_compile *p = &c->func; > struct intel_context *intel = &p->brw->intel; > struct brw_reg ip = brw_ip_reg(); > - GLuint nr = _mesa_bitcount_64(c->key.attrs & VERT_RESULT_COLOR_BITS); > GLuint jmpi = 1; > > + GLuint nr; > + > + if (c->key.do_twoside_color) { > + nr = ((c->key.attrs & (BITFIELD64_BIT(VERT_RESULT_COL0) | > BITFIELD64_BIT(VERT_RESULT_BFC0))) != 0) + > + ((c->key.attrs & (BITFIELD64_BIT(VERT_RESULT_COL1) | > BITFIELD64_BIT(VERT_RESULT_BFC1))) != 0); > + > + } else { > + nr = ((c->key.attrs & BITFIELD64_BIT(VERT_RESULT_COL0)) != 0) + > + ((c->key.attrs & BITFIELD64_BIT(VERT_RESULT_COL1)) != 0); > + } > + > if (!nr) > return; > > @@ -186,16 +193,16 @@ static void do_flatshade_triangle( struct > brw_sf_compile *c ) > brw_MUL(p, c->pv, c->pv, brw_imm_d(jmpi*(nr*2+1))); > brw_JMPI(p, ip, ip, c->pv); > > - copy_colors(c, c->vert[1], c->vert[0]); > - copy_colors(c, c->vert[2], c->vert[0]); > + copy_colors(c, c->vert[1], c->vert[0], c->key.do_twoside_color); > + copy_colors(c, c->vert[2], c->vert[0], c->key.do_twoside_color); > brw_JMPI(p, ip, ip, brw_imm_d(jmpi*(nr*4+1))); > > - copy_colors(c, c->vert[0], c->vert[1]); > - copy_colors(c, c->vert[2], c->vert[1]); > + copy_colors(c, c->vert[0], c->vert[1], c->key.do_twoside_color); > + copy_colors(c, c->vert[2], c->vert[1], c->key.do_twoside_color); > brw_JMPI(p, ip, ip, brw_imm_d(jmpi*nr*2)); > > - copy_colors(c, c->vert[0], c->vert[2]); > - copy_colors(c, c->vert[1], c->vert[2]); > + copy_colors(c, c->vert[0], c->vert[2], c->key.do_twoside_color); > + copy_colors(c, c->vert[1], c->vert[2], c->key.do_twoside_color); > > brw_pop_insn_state(p); > } > @@ -224,10 +231,10 @@ static void do_flatshade_line( struct brw_sf_compile > *c ) > > brw_MUL(p, c->pv, c->pv, brw_imm_d(jmpi*(nr+1))); > brw_JMPI(p, ip, ip, c->pv); > - copy_colors(c, c->vert[1], c->vert[0]); > + copy_colors(c, c->vert[1], c->vert[0], 0); > > brw_JMPI(p, ip, ip, brw_imm_ud(jmpi*nr)); > - copy_colors(c, c->vert[0], c->vert[1]); > + copy_colors(c, c->vert[0], c->vert[1], 0); > > brw_pop_insn_state(p); > } > @@ -337,13 +344,17 @@ calculate_masks(struct brw_sf_compile *c, > if (c->key.do_flat_shading) > persp_mask = c->key.attrs & ~(BITFIELD64_BIT(VERT_RESULT_HPOS) | > BITFIELD64_BIT(VERT_RESULT_COL0) | > - BITFIELD64_BIT(VERT_RESULT_COL1)); > + BITFIELD64_BIT(VERT_RESULT_COL1) | > + BITFIELD64_BIT(VERT_RESULT_BFC0) | > + BITFIELD64_BIT(VERT_RESULT_BFC1)); > else > persp_mask = c->key.attrs & ~(BITFIELD64_BIT(VERT_RESULT_HPOS)); > > if (c->key.do_flat_shading) > linear_mask = c->key.attrs & ~(BITFIELD64_BIT(VERT_RESULT_COL0) | > - BITFIELD64_BIT(VERT_RESULT_COL1)); > + BITFIELD64_BIT(VERT_RESULT_COL1) | > + BITFIELD64_BIT(VERT_RESULT_BFC0) | > + BITFIELD64_BIT(VERT_RESULT_BFC1)); > else > linear_mask = c->key.attrs; > > diff --git a/src/mesa/drivers/dri/i965/brw_wm_pass2.c > b/src/mesa/drivers/dri/i965/brw_wm_pass2.c > index 27c0a94..48143f3 100644 > --- a/src/mesa/drivers/dri/i965/brw_wm_pass2.c > +++ b/src/mesa/drivers/dri/i965/brw_wm_pass2.c > @@ -96,9 +96,26 @@ 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 > + * also written to. In addition, some slots can be > + * written in the vertex shader and not read in the > + * fragment shader. So the register number must always be > + * incremented, mapped or not. > + */ > if (fp_index >= 0) > - prealloc_reg(c, &c->payload.input_interp[fp_index], i++); > + prealloc_reg(c, &c->payload.input_interp[fp_index], i); > + i++; > } > } > assert(nr_interp_regs >= 1); > -- > 1.7.10.rc3.1.gb306 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev >
On Mon, Jul 16, 2012 at 08:43:17PM -0700, Paul Berry wrote: > Can you split this into three separate patches? That will make it easier > to troubleshoot in case we find bugs with these patches in the future. I'm going to try. > Also, I'm not convinced that #3 is necessary. Is there something in the > spec that dictates this behaviour? My reading of the spec is that if the > vertex shader writes to gl_BackColor but not glFrontColor, then the > contents of gl_Color in the fragment shader is undefined. Given the number of security issues/information leaks that happen due to reads out of place, I'm always extremely wary of reads from nowhere. So one pretty much has a choice between forcing a specific value (like 0) or reading from someplace else that makes sense. In that particular case I considered reading from the other color slot the easy way out. > If we *do* decide that #3 is necessary, then I think a better way to > accomplish it is to handle it in the GLSL vertex shader front-end, by > replacing gl_BackColor with gl_FrontColor in cases where gl_FrontColor is > not written to. That way our special case code to handle this situation > would be in just one place, rather than in three places (both fragment > shader back-ends, and the SF program). Also then the fix would apply to > all hardware, not just Intel Gen4-5. You'd have to switch off two-sided lighting too, but why not. > Finally, I couldn't figure out what you meant by "the stray mov into > lalaland". Can you elaborate on which piece of code used to generate that > stray mov, and why it doesn't anymore? Thanks. Looking at it again, I was wrong, it was protected. OG.
On Mon, Jul 16, 2012 at 08:43:17PM -0700, Paul Berry wrote: > Also, I'm not convinced that #3 is necessary. Is there something in the > spec that dictates this behaviour? My reading of the spec is that if the > vertex shader writes to gl_BackColor but not glFrontColor, then the > contents of gl_Color in the fragment shader is undefined. Oh, I remember why I did that in the first place. All the front/back piglit tests only write the appropriate color slot and not the other one. The language is annoying: The following built-in vertex output variables are available, but deprecated. A particular one should be written to if any functionality in a corresponding fragment shader or fixed pipeline uses it or state derived from it. Otherwise, behavior is undefined. out vec4 gl_FrontColor; out vec4 gl_BackColor; out vec4 gl_FrontSecondaryColor; out vec4 gl_BackSecondaryColor; [...] One could argue that you don't "use" gl_FrontColor if all your polygons are back-facing. Dunno. Do you consider all of the twoside piglit tests buggy? We can fix *that*. OG.
On 17 July 2012 01:57, Olivier Galibert <galibert@pobox.com> wrote: > On Mon, Jul 16, 2012 at 08:43:17PM -0700, Paul Berry wrote: > > Also, I'm not convinced that #3 is necessary. Is there something in the > > spec that dictates this behaviour? My reading of the spec is that if the > > vertex shader writes to gl_BackColor but not glFrontColor, then the > > contents of gl_Color in the fragment shader is undefined. > > Oh, I remember why I did that in the first place. All the front/back > piglit tests only write the appropriate color slot and not the other > one. > > The language is annoying: > The following built-in vertex output variables are available, but > deprecated. A particular one should be > written to if any functionality in a corresponding fragment shader or > fixed pipeline uses it or state derived > from it. Otherwise, behavior is undefined. > out vec4 gl_FrontColor; > out vec4 gl_BackColor; > out vec4 gl_FrontSecondaryColor; > out vec4 gl_BackSecondaryColor; > [...] > > One could argue that you don't "use" gl_FrontColor if all your > polygons are back-facing. Dunno. Do you consider all of the twoside > piglit tests buggy? We can fix *that*. > > OG. > > You have a good point about the piglit tests. It's weird that they don't set gl_FrontColor, but I can't bring myself to consider it a bug. Sigh, I was really hoping we could consider it undefined to write to gl_BackColor and not gl_FrontColor, but I guess it's reasonable for someone to expect that if they only write to gl_BackColor, and they only render backfacing polygons, they should get a sensible value in the fragment shader. If possible, I would still like to think of a way to address this situation that (a) doesn't require modifying both fragment shader back-ends and the SF program, and (b) helps all Mesa drivers, not just Intel Gen4-5. Especially because I suspect we may have bugs in Gen6-7 related to this situation. Would you be happy with one of the following two alternatives? 1. In the GLSL front-end, if we detect that a vertex shader writes to gl_BackColor but not gl_FrontColor, then automatically insert "gl_FrontColor = 0;" into the shader. This will guarantee that whenever gl_BackColor is written, gl_FrontColor is too. 2. In the function brw_compute_vue_map(), assign a VUE slot for VERT_RESULT_COL0 whenever *either* VERT_RESULT_COL0 or VERT_RESULT_BFC0 is used. This will guarantee that we always have a VUE slot available for front color, so we don't have to be as tricky in the FS and SF code. Note that alternative 2 leaves the contents of gl_FrontColor undefined if the vertex shader doesn't write to it. Although I appreciate what you're saying about security leaks due to reads out of place, I think that in this case the security risk is negligible, since the garbage value that winds up in gl_FrontColor will just be a value out of some random register in the vertex shader, not a random value out of memory and not a value belonging to some other process. This morning I'll try to ask some other Intel folks for their opinion on the subject.
On Tue, Jul 17, 2012 at 07:37:43AM -0700, Paul Berry wrote: > If possible, I would still like to think of a way to address this situation > that (a) doesn't require modifying both fragment shader back-ends and the > SF program, and (b) helps all Mesa drivers, not just Intel Gen4-5. > Especially because I suspect we may have bugs in Gen6-7 related to this > situation. You don't :-) It's correctly handled in gen6_sf_state.c::get_attr_override with similar semantics too. > Would you be happy with one of the following two alternatives? > > 1. In the GLSL front-end, if we detect that a vertex shader writes to > gl_BackColor but not gl_FrontColor, then automatically insert > "gl_FrontColor = 0;" into the shader. This will guarantee that whenever > gl_BackColor is written, gl_FrontColor is too. > > 2. In the function brw_compute_vue_map(), assign a VUE slot for > VERT_RESULT_COL0 whenever *either* VERT_RESULT_COL0 or VERT_RESULT_BFC0 is > used. This will guarantee that we always have a VUE slot available for > front color, so we don't have to be as tricky in the FS and SF code. With both methods the SF code is not really simplified. Doing the mov without testing would require writing to/reserving a slot for gl_BackColor if gl_FrontColor is written to, which wouldn't be acceptable. And to write to/reserve a slot for the two of them if gl_Color is read in any case. Probably unacceptable. So the need_* stuff is going to stay in any case :/ So the only simplification would be in the fs/wm and I'm somewhat afraid of having a vue slot that's not in outputs_written of the previous stage. They seem to be expected equivalent. > This morning I'll try to ask some other Intel folks for their opinion on > the subject. Did they have an opinion? Best, OG.
Olivier Galibert <galibert@pobox.com> writes: > On Tue, Jul 17, 2012 at 07:37:43AM -0700, Paul Berry wrote: >> If possible, I would still like to think of a way to address this situation >> that (a) doesn't require modifying both fragment shader back-ends and the >> SF program, and (b) helps all Mesa drivers, not just Intel Gen4-5. >> Especially because I suspect we may have bugs in Gen6-7 related to this >> situation. > > You don't :-) It's correctly handled in > gen6_sf_state.c::get_attr_override with similar semantics too. > >> Would you be happy with one of the following two alternatives? >> >> 1. In the GLSL front-end, if we detect that a vertex shader writes to >> gl_BackColor but not gl_FrontColor, then automatically insert >> "gl_FrontColor = 0;" into the shader. This will guarantee that whenever >> gl_BackColor is written, gl_FrontColor is too. >> >> 2. In the function brw_compute_vue_map(), assign a VUE slot for >> VERT_RESULT_COL0 whenever *either* VERT_RESULT_COL0 or VERT_RESULT_BFC0 is >> used. This will guarantee that we always have a VUE slot available for >> front color, so we don't have to be as tricky in the FS and SF code. > > With both methods the SF code is not really simplified. Doing the mov > without testing would require writing to/reserving a slot for > gl_BackColor if gl_FrontColor is written to, which wouldn't be > acceptable. And to write to/reserve a slot for the two of them if > gl_Color is read in any case. Probably unacceptable. So the need_* > stuff is going to stay in any case :/ I'm perfectly fine with the VUE containing slots for both when the app has gone out of its way to ask for deprecated two-sided color rendering.
On Mon, Jul 30, 2012 at 10:30:57AM -0700, Eric Anholt wrote: > I'm perfectly fine with the VUE containing slots for both when the app > has gone out of its way to ask for deprecated two-sided color > rendering. Are you also ok with recompiler the shaders when that enable is switched? OG.
Olivier Galibert <galibert@pobox.com> writes: > On Mon, Jul 30, 2012 at 10:30:57AM -0700, Eric Anholt wrote: >> I'm perfectly fine with the VUE containing slots for both when the app >> has gone out of its way to ask for deprecated two-sided color >> rendering. > > Are you also ok with recompiler the shaders when that enable is > switched? Yes, you have to include it in the program key and recompile. But people will consistently use the same values for things that land in program key contents, generally.
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 6cef08a..710f2ff 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -721,8 +721,24 @@ 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 + * fragment shader. So the register number must always be + * incremented, mapped or not. + */ if (fp_index >= 0) - urb_setup[fp_index] = urb_next++; + urb_setup[fp_index] = urb_next; + urb_next++; } } diff --git a/src/mesa/drivers/dri/i965/brw_sf.c b/src/mesa/drivers/dri/i965/brw_sf.c index 23a874a..7867ab5 100644 --- a/src/mesa/drivers/dri/i965/brw_sf.c +++ b/src/mesa/drivers/dri/i965/brw_sf.c @@ -192,7 +192,8 @@ brw_upload_sf_prog(struct brw_context *brw) /* _NEW_LIGHT */ key.do_flat_shading = (ctx->Light.ShadeModel == GL_FLAT); - key.do_twoside_color = (ctx->Light.Enabled && ctx->Light.Model.TwoSide); + key.do_twoside_color = (ctx->Light.Enabled && ctx->Light.Model.TwoSide) || + ctx->VertexProgram._TwoSideEnabled; /* _NEW_POLYGON */ if (key.do_twoside_color) { diff --git a/src/mesa/drivers/dri/i965/brw_sf_emit.c b/src/mesa/drivers/dri/i965/brw_sf_emit.c index ff6383b..9d8aa38 100644 --- a/src/mesa/drivers/dri/i965/brw_sf_emit.c +++ b/src/mesa/drivers/dri/i965/brw_sf_emit.c @@ -79,24 +79,9 @@ have_attr(struct brw_sf_compile *c, GLuint attr) /*********************************************************************** * Twoside lighting */ -static void copy_bfc( struct brw_sf_compile *c, - struct brw_reg vert ) -{ - struct brw_compile *p = &c->func; - GLuint i; - - for (i = 0; i < 2; i++) { - if (have_attr(c, VERT_RESULT_COL0+i) && - have_attr(c, VERT_RESULT_BFC0+i)) - brw_MOV(p, - get_vert_result(c, vert, VERT_RESULT_COL0+i), - get_vert_result(c, vert, VERT_RESULT_BFC0+i)); - } -} - - static void do_twoside_color( struct brw_sf_compile *c ) { + GLuint i, need_0, need_1; struct brw_compile *p = &c->func; GLuint backface_conditional = c->key.frontface_ccw ? BRW_CONDITIONAL_G : BRW_CONDITIONAL_L; @@ -105,12 +90,14 @@ static void do_twoside_color( struct brw_sf_compile *c ) if (c->key.primitive == SF_UNFILLED_TRIS) return; - /* XXX: What happens if BFC isn't present? This could only happen - * for user-supplied vertex programs, as t_vp_build.c always does - * the right thing. + /* If the vertex shader provides both front and backface color, do + * the selection. Otherwise the generated code will pick up + * whichever there is. */ - if (!(have_attr(c, VERT_RESULT_COL0) && have_attr(c, VERT_RESULT_BFC0)) && - !(have_attr(c, VERT_RESULT_COL1) && have_attr(c, VERT_RESULT_BFC1))) + need_0 = have_attr(c, VERT_RESULT_COL0) && have_attr(c, VERT_RESULT_BFC0); + need_1 = have_attr(c, VERT_RESULT_COL1) && have_attr(c, VERT_RESULT_BFC1); + + if (!need_0 && !need_1) return; /* Need to use BRW_EXECUTE_4 and also do an 4-wide compare in order @@ -121,12 +108,15 @@ static void do_twoside_color( struct brw_sf_compile *c ) brw_push_insn_state(p); brw_CMP(p, vec4(brw_null_reg()), backface_conditional, c->det, brw_imm_f(0)); brw_IF(p, BRW_EXECUTE_4); - { - switch (c->nr_verts) { - case 3: copy_bfc(c, c->vert[2]); - case 2: copy_bfc(c, c->vert[1]); - case 1: copy_bfc(c, c->vert[0]); - } + for (i=0; i<c->nr_verts; i++) { + if (need_0) + brw_MOV(p, + get_vert_result(c, c->vert[i], VERT_RESULT_COL0), + get_vert_result(c, c->vert[i], VERT_RESULT_BFC0)); + if (need_1) + brw_MOV(p, + get_vert_result(c, c->vert[i], VERT_RESULT_COL1), + get_vert_result(c, c->vert[i], VERT_RESULT_BFC1)); } brw_ENDIF(p); brw_pop_insn_state(p); @@ -139,20 +129,27 @@ static void do_twoside_color( struct brw_sf_compile *c ) */ #define VERT_RESULT_COLOR_BITS (BITFIELD64_BIT(VERT_RESULT_COL0) | \ - BITFIELD64_BIT(VERT_RESULT_COL1)) + BITFIELD64_BIT(VERT_RESULT_COL1)) static void copy_colors( struct brw_sf_compile *c, struct brw_reg dst, - struct brw_reg src) + struct brw_reg src, + int allow_twoside) { struct brw_compile *p = &c->func; GLuint i; for (i = VERT_RESULT_COL0; i <= VERT_RESULT_COL1; i++) { - if (have_attr(c,i)) + if (have_attr(c,i)) { brw_MOV(p, get_vert_result(c, dst, i), get_vert_result(c, src, i)); + + } else if(allow_twoside && have_attr(c, i - VERT_RESULT_COL0 + VERT_RESULT_BFC0)) { + brw_MOV(p, + get_vert_result(c, dst, i - VERT_RESULT_COL0 + VERT_RESULT_BFC0), + get_vert_result(c, src, i - VERT_RESULT_COL0 + VERT_RESULT_BFC0)); + } } } @@ -167,9 +164,19 @@ static void do_flatshade_triangle( struct brw_sf_compile *c ) struct brw_compile *p = &c->func; struct intel_context *intel = &p->brw->intel; struct brw_reg ip = brw_ip_reg(); - GLuint nr = _mesa_bitcount_64(c->key.attrs & VERT_RESULT_COLOR_BITS); GLuint jmpi = 1; + GLuint nr; + + if (c->key.do_twoside_color) { + nr = ((c->key.attrs & (BITFIELD64_BIT(VERT_RESULT_COL0) | BITFIELD64_BIT(VERT_RESULT_BFC0))) != 0) + + ((c->key.attrs & (BITFIELD64_BIT(VERT_RESULT_COL1) | BITFIELD64_BIT(VERT_RESULT_BFC1))) != 0); + + } else { + nr = ((c->key.attrs & BITFIELD64_BIT(VERT_RESULT_COL0)) != 0) + + ((c->key.attrs & BITFIELD64_BIT(VERT_RESULT_COL1)) != 0); + } + if (!nr) return; @@ -186,16 +193,16 @@ static void do_flatshade_triangle( struct brw_sf_compile *c ) brw_MUL(p, c->pv, c->pv, brw_imm_d(jmpi*(nr*2+1))); brw_JMPI(p, ip, ip, c->pv); - copy_colors(c, c->vert[1], c->vert[0]); - copy_colors(c, c->vert[2], c->vert[0]); + copy_colors(c, c->vert[1], c->vert[0], c->key.do_twoside_color); + copy_colors(c, c->vert[2], c->vert[0], c->key.do_twoside_color); brw_JMPI(p, ip, ip, brw_imm_d(jmpi*(nr*4+1))); - copy_colors(c, c->vert[0], c->vert[1]); - copy_colors(c, c->vert[2], c->vert[1]); + copy_colors(c, c->vert[0], c->vert[1], c->key.do_twoside_color); + copy_colors(c, c->vert[2], c->vert[1], c->key.do_twoside_color); brw_JMPI(p, ip, ip, brw_imm_d(jmpi*nr*2)); - copy_colors(c, c->vert[0], c->vert[2]); - copy_colors(c, c->vert[1], c->vert[2]); + copy_colors(c, c->vert[0], c->vert[2], c->key.do_twoside_color); + copy_colors(c, c->vert[1], c->vert[2], c->key.do_twoside_color); brw_pop_insn_state(p); } @@ -224,10 +231,10 @@ static void do_flatshade_line( struct brw_sf_compile *c ) brw_MUL(p, c->pv, c->pv, brw_imm_d(jmpi*(nr+1))); brw_JMPI(p, ip, ip, c->pv); - copy_colors(c, c->vert[1], c->vert[0]); + copy_colors(c, c->vert[1], c->vert[0], 0); brw_JMPI(p, ip, ip, brw_imm_ud(jmpi*nr)); - copy_colors(c, c->vert[0], c->vert[1]); + copy_colors(c, c->vert[0], c->vert[1], 0); brw_pop_insn_state(p); } @@ -337,13 +344,17 @@ calculate_masks(struct brw_sf_compile *c, if (c->key.do_flat_shading) persp_mask = c->key.attrs & ~(BITFIELD64_BIT(VERT_RESULT_HPOS) | BITFIELD64_BIT(VERT_RESULT_COL0) | - BITFIELD64_BIT(VERT_RESULT_COL1)); + BITFIELD64_BIT(VERT_RESULT_COL1) | + BITFIELD64_BIT(VERT_RESULT_BFC0) | + BITFIELD64_BIT(VERT_RESULT_BFC1)); else persp_mask = c->key.attrs & ~(BITFIELD64_BIT(VERT_RESULT_HPOS)); if (c->key.do_flat_shading) linear_mask = c->key.attrs & ~(BITFIELD64_BIT(VERT_RESULT_COL0) | - BITFIELD64_BIT(VERT_RESULT_COL1)); + BITFIELD64_BIT(VERT_RESULT_COL1) | + BITFIELD64_BIT(VERT_RESULT_BFC0) | + BITFIELD64_BIT(VERT_RESULT_BFC1)); else linear_mask = c->key.attrs; diff --git a/src/mesa/drivers/dri/i965/brw_wm_pass2.c b/src/mesa/drivers/dri/i965/brw_wm_pass2.c index 27c0a94..48143f3 100644 --- a/src/mesa/drivers/dri/i965/brw_wm_pass2.c +++ b/src/mesa/drivers/dri/i965/brw_wm_pass2.c @@ -96,9 +96,26 @@ 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 + * also written to. In addition, some slots can be + * written in the vertex shader and not read in the + * fragment shader. So the register number must always be + * incremented, mapped or not. + */ if (fp_index >= 0) - prealloc_reg(c, &c->payload.input_interp[fp_index], i++); + prealloc_reg(c, &c->payload.input_interp[fp_index], i); + i++; } } assert(nr_interp_regs >= 1);
There was... confusion about which register goes where. With that patch urb_setup is in line with the vue setup, even when these annoying backcolor slots are used. And in addition the stray mov into lalaland is avoided when only one of the front/back slots is used and the backface is looking at you. The code instead picks whatever slot was written to by the vertex shader. That makes most of the generated piglit tests useless to test the backface selection though. Signed-off-by: Olivier Galibert <galibert@pobox.com> --- src/mesa/drivers/dri/i965/brw_fs.cpp | 18 +++++- src/mesa/drivers/dri/i965/brw_sf.c | 3 +- src/mesa/drivers/dri/i965/brw_sf_emit.c | 93 +++++++++++++++++------------- src/mesa/drivers/dri/i965/brw_wm_pass2.c | 19 +++++- 4 files changed, 89 insertions(+), 44 deletions(-)