On 30 June 2012 11:50, Olivier Galibert <galibert at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20120716/50865587/attachment-0001.html>
