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"> At this point all interpolation tests with fixed clipping work. Signed-off-by: Olivier Galibert <<a href="mailto:galibert@pobox.com">galibert@pobox.com</a>> --- src/mesa/drivers/dri/i965/brw_clip.c | 9 ++ src/mesa/drivers/dri/i965/brw_clip.h | 1 + src/mesa/drivers/dri/i965/brw_clip_util.c | 133 ++++++++++++++++++++++++++--- 3 files changed, 132 insertions(+), 11 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_clip.c b/src/mesa/drivers/dri/i965/brw_clip.c index 952eb4a..6bfdf24 100644 --- a/src/mesa/drivers/dri/i965/brw_clip.c +++ b/src/mesa/drivers/dri/i965/brw_clip.c @@ -234,6 +234,15 @@ brw_upload_clip_prog(struct brw_context *brw) break; } } + key.has_noperspective_shading = 0; + for (i = 0; i < BRW_VERT_RESULT_MAX; i++) { + if (brw_get_interpolation_mode(brw, i) == INTERP_QUALIFIER_NOPERSPECTIVE && + brw->vs.prog_data->vue_map.slot_to_vert_result[i] != VERT_RESULT_HPOS) { + key.has_noperspective_shading = 1; + break; + } + } + key.pv_first = (ctx->Light.ProvokingVertex == GL_FIRST_VERTEX_CONVENTION); brw_copy_interpolation_modes(brw, key.interpolation_mode); /* _NEW_TRANSFORM (also part of VUE map)*/ diff --git a/src/mesa/drivers/dri/i965/brw_clip.h b/src/mesa/drivers/dri/i965/brw_clip.h index 0ea0394..2a7245a 100644 --- a/src/mesa/drivers/dri/i965/brw_clip.h +++ b/src/mesa/drivers/dri/i965/brw_clip.h @@ -47,6 +47,7 @@ struct brw_clip_prog_key { GLuint primitive:4; GLuint nr_userclip:4; GLuint has_flat_shading:1; + GLuint has_noperspective_shading:1; GLuint pv_first:1; GLuint do_unfilled:1; GLuint fill_cw:2; /* includes cull information */ diff --git a/src/mesa/drivers/dri/i965/brw_clip_util.c b/src/mesa/drivers/dri/i965/brw_clip_util.c index 7b0205a..5bdcef8 100644 --- a/src/mesa/drivers/dri/i965/brw_clip_util.c +++ b/src/mesa/drivers/dri/i965/brw_clip_util.c @@ -129,6 +129,8 @@ static void brw_clip_project_vertex( struct brw_clip_compile *c, /* Interpolate between two vertices and put the result into a0.0. * Increment a0.0 accordingly. + * + * Beware that dest_ptr can be equal to v0_ptr. */ void brw_clip_interp_vertex( struct brw_clip_compile *c, struct brw_indirect dest_ptr, @@ -138,8 +140,9 @@ void brw_clip_interp_vertex( struct brw_clip_compile *c, bool force_edgeflag) { struct brw_compile *p = &c->func; - struct brw_reg tmp = get_tmp(c); - GLuint slot; + struct brw_context *brw = p->brw; + struct brw_reg tmp, t_nopersp, v0_ndc_copy; + GLuint slot, delta; /* Just copy the vertex header: */ @@ -148,13 +151,119 @@ void brw_clip_interp_vertex( struct brw_clip_compile *c, * back on Ironlake, so needn't change it */ brw_copy_indirect_to_indirect(p, dest_ptr, v0_ptr, 1); - - /* Iterate over each attribute (could be done in pairs?) + + /* + * First handle the 3D and NDC positioning, in case we need + * noperspective interpolation. Doing it early has no performance + * impact in any case. + */ + + /* Start by picking up the v0 NDC coordinates, because that vertex + * may be shared with the destination. + */ + if (c->key.has_noperspective_shading) { + v0_ndc_copy = get_tmp(c); + brw_MOV(p, v0_ndc_copy, deref_4f(v0_ptr, + brw_vert_result_to_offset(&c->vue_map, + BRW_VERT_RESULT_NDC))); + } </blockquote><div> Style nit-pick: this is a lot of indentation. How about this instead: if (c->key.has_noperspective_shading) { unsigned offset = brw_vert_result_to_offset(&c->vue_map, BRW_VERT_RESULT_NDC); v0_ndc_copy = get_tmp(c); brw_MOV(p, v0_ndc_copy, deref_4f(v0_ptr, offset)); } </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* + * Compute the new 3D position + */ + + delta = brw_vert_result_to_offset(&c->vue_map, VERT_RESULT_HPOS); + tmp = get_tmp(c); + brw_MUL(p, + vec4(brw_null_reg()), + deref_4f(v1_ptr, delta), + t0); + + brw_MAC(p, + tmp, + negate(deref_4f(v0_ptr, delta)), + t0); + + brw_ADD(p, + deref_4f(dest_ptr, delta), + deref_4f(v0_ptr, delta), + tmp); + release_tmp(c, tmp); </blockquote><div> Since delta and tmp are used elsewhere in this function for other purposes, I would feel more comfortable if we created a local scope for them by enclosing this small chunk of code in curly braces, e.g.: { /* * Compute the new 3D position */ GLuint delta = brw_vert_result_to_offset(&c->vue_map, VERT_RESULT_HPOS); struct brw_reg tmp = get_tmp(c); brw_MUL(p, vec4(brw_null_reg()), deref_4f(v1_ptr, delta), t0); brw_MAC(p, tmp, negate(deref_4f(v0_ptr, delta)), t0); brw_ADD(p, deref_4f(dest_ptr, delta), deref_4f(v0_ptr, delta), tmp); release_tmp(c, tmp); } </div><div>Also, it would be nice to have a comment above each small group of instructions showing what they compute as a formula. For example, above these three instructions we could say: /* dest_hpos = v0_hpos * (1 - t0) + v1_hpos * t0 */ </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* Then recreate the projected (NDC) coordinate in the new vertex + * header */ + brw_clip_project_vertex(c, dest_ptr); + + /* + * If we have noperspective attributes, we now need to compute the + * screen-space t. + */ + if (c->key.has_noperspective_shading) { + delta = brw_vert_result_to_offset(&c->vue_map, BRW_VERT_RESULT_NDC); + t_nopersp = get_tmp(c); + tmp = get_tmp(c); </blockquote><div> Along the same lines as my previous comment, I'd prefer to make these three variables local to this scope, e.g.: if (c->key.has_noperspective_shading) { GLuint delta = brw_vert_result_to_offset(&c->vue_map, BRW_VERT_RESULT_NDC); struct brw_reg t_nopersp = get_tmp(c); struct brw_reg tmp = get_tmp(c); </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + /* Build a register with coordinates from the second and new vertices */ </blockquote><div> In the code below, I could really use some comments to clarify the computation you're doing. I'll insert some suggestions inline: /* t_nopersp = vec4(v1_ndc.xy, dest_ndc.xy) */ </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + brw_MOV(p, t_nopersp, deref_4f(v1_ptr, delta)); + brw_MOV(p, tmp, deref_4f(dest_ptr, delta)); + brw_set_access_mode(p, BRW_ALIGN_16); + brw_MOV(p, + brw_writemask(t_nopersp, WRITEMASK_ZW), + brw_swizzle(tmp, 0,1,0,1)); + + /* Subtract the coordinates of the first vertex */ </blockquote><div> /* t_nopersp -= v0_ndc_copy.xyxy * Therefore t_nopersp = vec4(v1_ndc.xy - v0_ndc.xy, * dest_ndc.xy - v0_ndc.xy) */ </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + brw_ADD(p, t_nopersp, t_nopersp, negate(brw_swizzle(v0_ndc_copy, 0,1,0,1))); + + /* Add the absolute value of the X and Y deltas so that if the + * points aren't in the same place on the screen we get non-zero + * values to divide. + * + * After that we have vert1-vert0 in t_nopersp.x and vertnew-vert0 in t_nopersp.y. + */ </blockquote><div> /* t_nopersp.xy = |t_nopersp.xz| + |t_nopersp.yw| * Therefore: * t_nopersp = vec4(|v1_ndc.x - v0_ndc.x| + |v1_ndc.y - v0_ndc.y|, * |dest_ndc.x - v0_ndc.x| + |dest_ndc.y - v0_ndc.y|, * <don't care>, <don't care>) */ </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + brw_ADD(p, + brw_writemask(t_nopersp, WRITEMASK_XY), + brw_abs(brw_swizzle(t_nopersp, 0,2,0,0)), + brw_abs(brw_swizzle(t_nopersp, 1,3,0,0))); + brw_set_access_mode(p, BRW_ALIGN_1); + + /* If the points are in the same place (vert1-vert0 == 0), just + * substitute a value that will ensure that we don't divide by + * 0. + */ + brw_CMP(p, vec1(brw_null_reg()), BRW_CONDITIONAL_EQ, + vec1(t_nopersp), + brw_imm_f(1)); </blockquote><div> Shouldn't this be brw_imm_f(0)? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + brw_IF(p, BRW_EXECUTE_1); + brw_MOV(p, t_nopersp, brw_imm_vf4(VF_ONE, VF_ZERO, VF_ZERO, VF_ZERO)); + brw_ENDIF(p); + + /* Now compute t_nopersp = t_nopersp.y/t_nopersp.x and broadcast it */ + brw_math_invert(p, get_element(t_nopersp, 0), get_element(t_nopersp, 0)); + brw_MUL(p, + vec1(t_nopersp), + vec1(t_nopersp), + vec1(suboffset(t_nopersp, 1))); + brw_set_access_mode(p, BRW_ALIGN_16); + brw_MOV(p, t_nopersp, brw_swizzle(t_nopersp, 0,0,0,0)); + brw_set_access_mode(p, BRW_ALIGN_1); </blockquote><div> That was a very clever computation. Well done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + release_tmp(c, tmp); + release_tmp(c, v0_ndc_copy); + } + + /* Now we can iterate over each attribute + * (could be done in pairs?) + */ + tmp = get_tmp(c); for (slot = 0; slot < c->vue_map.num_slots; slot++) { int vert_result = c->vue_map.slot_to_vert_result[slot]; GLuint delta = brw_vue_slot_to_offset(slot); + /* HPOS is already handled */ + if(vert_result == VERT_RESULT_HPOS) + continue; + if (vert_result == VERT_RESULT_EDGE) { if (force_edgeflag) brw_MOV(p, deref_4f(dest_ptr, delta), brw_imm_f(1)); @@ -174,15 +283,20 @@ void brw_clip_interp_vertex( struct brw_clip_compile *c, * * New = attr0 + t*attr1 - t*attr0 */ + + struct brw_reg t = + brw_get_interpolation_mode(brw, slot) == INTERP_QUALIFIER_NOPERSPECTIVE ? + t_nopersp : t0; + brw_MUL(p, vec4(brw_null_reg()), deref_4f(v1_ptr, delta), - t0); + t); brw_MAC(p, tmp, negate(deref_4f(v0_ptr, delta)), - t0); + t); brw_ADD(p, deref_4f(dest_ptr, delta), @@ -198,11 +312,8 @@ void brw_clip_interp_vertex( struct brw_clip_compile *c, } release_tmp(c, tmp); - - /* Recreate the projected (NDC) coordinate in the new vertex - * header: - */ - brw_clip_project_vertex(c, dest_ptr ); + if (c->key.has_noperspective_shading) + release_tmp(c, t_nopersp); } void brw_clip_emit_vue(struct brw_clip_compile *c, -- 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> With the exception of my question about brw_imm_f(0), all of my comments on this patch are stylistic suggestions. So assuming we get the brw_imm_f(0) thing figured out, this patch is: Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>