On 30 June 2012 11:50, Olivier Galibert <<a href="mailto:galibert@pobox.com" target="_blank">galibert@pobox.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Signed-off-by: Olivier Galibert <<a href="mailto:galibert@pobox.com">galibert@pobox.com</a>> --- 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; </blockquote><div> 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 :) </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> /* _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)); </blockquote><div> This is much easier to follow as a loop. Thanks for cleaning this up. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> } 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)); + } </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> } } @@ -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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>