From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Berry Subject: Re: [Mesa-dev] [PATCH 5/5] intel gen4-5: Make noperspective clipping work. Date: Tue, 17 Jul 2012 07:07:56 -0700 Message-ID: References: <1341082215-22933-1-git-send-email-galibert@pobox.com> <1341082215-22933-6-git-send-email-galibert@pobox.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0513365795==" Return-path: In-Reply-To: <1341082215-22933-6-git-send-email-galibert@pobox.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Olivier Galibert Cc: mesa-dev@lists.freedesktop.org, intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0513365795== Content-Type: multipart/alternative; boundary=0015175cf8d4c26d5e04c507120a --0015175cf8d4c26d5e04c507120a Content-Type: text/plain; charset=ISO-8859-1 On 30 June 2012 11:50, Olivier Galibert wrote: > At this point all interpolation tests with fixed clipping work. > > Signed-off-by: Olivier Galibert > --- > 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))); > + } > 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)); } > + > + /* > + * 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); > 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); } 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 */ > + > + /* 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); > 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); > + > + /* Build a register with coordinates from the second and new > vertices */ > 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) */ > + 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 */ > /* t_nopersp -= v0_ndc_copy.xyxy * Therefore t_nopersp = vec4(v1_ndc.xy - v0_ndc.xy, * dest_ndc.xy - v0_ndc.xy) */ > + 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. > + */ > /* 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|, * , ) */ > + 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)); > Shouldn't this be brw_imm_f(0)? > + 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); > That was a very clever computation. Well done. > + > + 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 > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > 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 --0015175cf8d4c26d5e04c507120a Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 30 June 2012 11:50, Olivier Galibert <galibert@pobox.com> wrote:
At this point all interpolation tests with fixed clipping work.

Signed-off-by: Olivier Galibert <g= alibert@pobox.com>
---
=A0src/mesa/drivers/dri/i965/brw_clip.c =A0 =A0 =A0| =A0 =A09 ++
=A0src/mesa/drivers/dri/i965/brw_clip.h =A0 =A0 =A0| =A0 =A01 +
=A0src/mesa/drivers/dri/i965/brw_clip_util.c | =A0133 +++++++++++++++++++++= +++++---
=A03 files changed, 132 insertions(+), 11 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_clip.c b/src/mesa/drivers/dri/i9= 65/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)
=A0 =A0 =A0 =A0 =A0 break;
=A0 =A0 =A0 =A0}
=A0 =A0 }
+ =A0 key.has_noperspective_shading =3D 0;
+ =A0 for (i =3D 0; i < BRW_VERT_RESULT_MAX; i++) {
+ =A0 =A0 =A0if (brw_get_interpolation_mode(brw, i) =3D=3D INTERP_QUALIFIER= _NOPERSPECTIVE &&
+ =A0 =A0 =A0 =A0 =A0brw->vs.prog_data->vue_map.slot_to_vert_result[i= ] !=3D VERT_RESULT_HPOS) {
+ =A0 =A0 =A0 =A0 key.has_noperspective_shading =3D 1;
+ =A0 =A0 =A0 =A0 break;
+ =A0 =A0 =A0}
+ =A0 }
+
=A0 =A0 key.pv_first =3D (ctx->Light.ProvokingVertex =3D=3D GL_FIRST_VER= TEX_CONVENTION);
=A0 =A0 brw_copy_interpolation_modes(brw, key.interpolation_mode);
=A0 =A0 /* _NEW_TRANSFORM (also part of VUE map)*/
diff --git a/src/mesa/drivers/dri/i965/brw_clip.h b/src/mesa/drivers/dri/i9= 65/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 {
=A0 =A0 GLuint primitive:4;
=A0 =A0 GLuint nr_userclip:4;
=A0 =A0 GLuint has_flat_shading:1;
+ =A0 GLuint has_noperspective_shading:1;
=A0 =A0 GLuint pv_first:1;
=A0 =A0 GLuint do_unfilled:1;
=A0 =A0 GLuint fill_cw:2; =A0 =A0 =A0 =A0 =A0 /* includes cull information = */
diff --git a/src/mesa/drivers/dri/i965/brw_clip_util.c b/src/mesa/drivers/d= ri/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_co= mpile *c,

=A0/* Interpolate between two vertices and put the result into a0.0.
=A0 * Increment a0.0 accordingly.
+ *
+ * Beware that dest_ptr can be equal to v0_ptr.
=A0 */
=A0void brw_clip_interp_vertex( struct brw_clip_compile *c,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct brw_indir= ect dest_ptr,
@@ -138,8 +140,9 @@ void brw_clip_interp_vertex( struct brw_clip_compile *c= ,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bool force_edgef= lag)
=A0{
=A0 =A0 struct brw_compile *p =3D &c->func;
- =A0 struct brw_reg tmp =3D get_tmp(c);
- =A0 GLuint slot;
+ =A0 struct brw_context *brw =3D p->brw;
+ =A0 struct brw_reg tmp, t_nopersp, v0_ndc_copy;
+ =A0 GLuint slot, delta;

=A0 =A0 /* Just copy the vertex header:
=A0 =A0 =A0*/
@@ -148,13 +151,119 @@ void brw_clip_interp_vertex( struct brw_clip_compile= *c,
=A0 =A0 =A0* back on Ironlake, so needn't change it
=A0 =A0 =A0*/
=A0 =A0 brw_copy_indirect_to_indirect(p, dest_ptr, v0_ptr, 1);
-
- =A0 /* Iterate over each attribute (could be done in pairs?)
+
+ =A0 /*
+ =A0 =A0* First handle the 3D and NDC positioning, in case we need
+ =A0 =A0* noperspective interpolation. =A0Doing it early has no performanc= e
+ =A0 =A0* impact in any case.
+ =A0 =A0*/
+
+ =A0 /* Start by picking up the v0 NDC coordinates, because that vertex + =A0 =A0* may be shared with the destination.
+ =A0 =A0*/
+ =A0 if (c->key.has_noperspective_shading) {
+ =A0 =A0 =A0v0_ndc_copy =3D get_tmp(c);
+ =A0 =A0 =A0brw_MOV(p, v0_ndc_copy, deref_4f(v0_ptr,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 brw_vert_result_to_offset(&c->vue_map,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 BRW_VERT_RESULT_NDC= )));
+ =A0 }

Style nit-pick: this is a lot of indentati= on.=A0 How about this instead:

=A0=A0 if (c->key.has_noperspectiv= e_shading) {
=A0=A0=A0=A0=A0 unsigned offset =3D
=A0=A0=A0=A0=A0=A0= =A0=A0 brw_vert_result_to_offset(&c->vue_map, BRW_VERT_RESULT_NDC);<= br> =A0=A0=A0=A0=A0 v0_ndc_copy =3D get_tmp(c);
=A0=A0=A0=A0=A0 brw_MOV(p, v= 0_ndc_copy, deref_4f(v0_ptr, offset));
=A0=A0 }

=A0
+
+ =A0 /*
+ =A0 =A0* Compute the new 3D position
+ =A0 =A0*/
+
+ =A0 delta =3D brw_vert_result_to_offset(&c->vue_map, VERT_RESULT_H= POS);
+ =A0 tmp =3D get_tmp(c);
+ =A0 brw_MUL(p,
+ =A0 =A0 =A0 =A0 =A0 vec4(brw_null_reg()),
+ =A0 =A0 =A0 =A0 =A0 deref_4f(v1_ptr, delta),
+ =A0 =A0 =A0 =A0 =A0 t0);
+
+ =A0 brw_MAC(p,
+ =A0 =A0 =A0 =A0 =A0 tmp,
+ =A0 =A0 =A0 =A0 =A0 negate(deref_4f(v0_ptr, delta)),
+ =A0 =A0 =A0 =A0 =A0 t0);
+
+ =A0 brw_ADD(p,
+ =A0 =A0 =A0 =A0 =A0 deref_4f(dest_ptr, delta),
+ =A0 =A0 =A0 =A0 =A0 deref_4f(v0_ptr, delta),
+ =A0 =A0 =A0 =A0 =A0 tmp);
+ =A0 release_tmp(c, tmp);

Since delta and tmp are= used elsewhere in this function for other purposes, I would feel more comf= ortable if we created a local scope for them by enclosing this small chunk = of code in curly braces, e.g.:

=A0=A0 {
=A0=A0=A0=A0=A0 /*
=A0=A0=A0=A0=A0=A0 * Compute the new = 3D position
=A0=A0=A0=A0=A0=A0 */

=A0=A0=A0=A0=A0 GLuint delta = =3D brw_vert_result_to_offset(&c->vue_map, VERT_RESULT_HPOS);
=A0= =A0=A0=A0=A0 struct brw_reg tmp =3D get_tmp(c);
=A0=A0=A0=A0=A0 brw_MUL(= p,
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 vec4(brw_null_reg()),
=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 deref_4f(v1_ptr, delta),
=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0 t0);

=A0=A0=A0=A0=A0 brw_MAC(p,
=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 tmp,
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 negate(deref_4f(v0_ptr, delta)),
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0 t0);

=A0=A0=A0=A0=A0 brw_ADD(p,
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 d= eref_4f(dest_ptr, delta),
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 deref_= 4f(v0_ptr, delta),
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 tmp);
=A0= =A0=A0=A0=A0 release_tmp(c, tmp);
=A0=A0 }

Also, it wo= uld be nice to have a comment above each small group of instructions showin= g what they compute as a formula.=A0 For example, above these three instruc= tions we could say:

/* dest_hpos =3D v0_hpos * (1 - t0) + v1_hpos * t0 */
=A0
+
+ =A0 /* Then recreate the projected (NDC) coordinate in the new vertex
+ =A0 =A0* header
=A0 =A0 =A0*/
+ =A0 brw_clip_project_vertex(c, dest_ptr);
+
+ =A0 /*
+ =A0 =A0* If we have noperspective attributes, we now need to compute the<= br> + =A0 =A0* screen-space t.
+ =A0 =A0*/
+ =A0 if (c->key.has_noperspective_shading) {
+ =A0 =A0 =A0delta =3D brw_vert_result_to_offset(&c->vue_map, BRW_VE= RT_RESULT_NDC);
+ =A0 =A0 =A0t_nopersp =3D get_tmp(c);
+ =A0 =A0 =A0tmp =3D get_tmp(c);

Along the same li= nes as my previous comment, I'd prefer to make these three variables lo= cal to this scope, e.g.:

=A0=A0 if (c->key.has_noperspective_shad= ing) {
=A0=A0=A0=A0=A0 GLuint delta =3D brw_vert_result_to_offset(&c->vue_m= ap, BRW_VERT_RESULT_NDC);
=A0=A0=A0=A0=A0 struct brw_reg t_nopersp =3D g= et_tmp(c);
=A0=A0=A0=A0=A0 struct brw_reg tmp =3D get_tmp(c);

=A0=
+
+ =A0 =A0 =A0/* Build a register with coordinates from the second and new v= ertices */

In the code below, I could really use s= ome comments to clarify the computation you're doing.=A0 I'll inser= t some suggestions inline:
=A0
=A0=A0=A0=A0=A0 /* t_nopersp =3D vec4(v1_ndc.xy, dest_ndc.xy) */
=
+ =A0 =A0 =A0brw_MOV(p, t_nopersp, deref_4f(v1_ptr, delta));
+ =A0 =A0 =A0brw_MOV(p, tmp, deref_4f(dest_ptr, delta));
+ =A0 =A0 =A0brw_set_access_mode(p, BRW_ALIGN_16);
+ =A0 =A0 =A0brw_MOV(p,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0brw_writemask(t_nopersp, WRITEMASK_ZW),
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0brw_swizzle(tmp, 0,1,0,1));
+
+ =A0 =A0 =A0/* Subtract the coordinates of the first vertex */
=A0=A0=A0=A0=A0 /* t_nopersp -=3D v0_ndc_copy.xyxy
=A0=A0=A0= =A0=A0=A0 * Therefore t_nopersp =3D vec4(v1_ndc.xy - v0_ndc.xy,
=A0=A0= =A0=A0=A0=A0 *=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0 dest_ndc.xy - v0_ndc.xy)
=A0=A0=A0=A0=A0=A0 */
+ =A0 =A0 =A0brw_ADD(p, t_nopersp, t_nopersp, negate(brw_swizzle(v0_ndc_cop= y, 0,1,0,1)));
+
+ =A0 =A0 =A0/* Add the absolute value of the X and Y deltas so that if the=
+ =A0 =A0 =A0 * points aren't in the same place on the screen we get no= n-zero
+ =A0 =A0 =A0 * values to divide.
+ =A0 =A0 =A0 *
+ =A0 =A0 =A0 * After that we have vert1-vert0 in t_nopersp.x and vertnew-v= ert0 in t_nopersp.y.
+ =A0 =A0 =A0 */
=A0=A0=A0=A0=A0 /* t_nopersp.xy =3D |= t_nopersp.xz| + |t_nopersp.yw|
=A0=A0=A0=A0=A0=A0 * Therefore:
=A0=A0= =A0=A0=A0=A0 * t_nopersp =3D vec4(|v1_ndc.x - v0_ndc.x| + |v1_ndc.y - v0_nd= c.y|,
=A0=A0=A0=A0=A0=A0 *=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 |dest_ndc.x - v0_ndc.x| + |dest_ndc.y - v0_ndc.y|,
=A0=A0=A0=A0=A0=A0 *=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 <= ;don't care>, <don't care>)
=A0=A0=A0=A0=A0=A0 */
<= /div>
+ =A0 =A0 =A0brw_ADD(p,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0brw_writemask(t_nopersp, WRITEMASK_XY),
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0brw_abs(brw_swizzle(t_nopersp, 0,2,0,0)),
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0brw_abs(brw_swizzle(t_nopersp, 1,3,0,0)));
+ =A0 =A0 =A0brw_set_access_mode(p, BRW_ALIGN_1);
+
+ =A0 =A0 =A0/* If the points are in the same place (vert1-vert0 =3D=3D 0),= just
+ =A0 =A0 =A0 * substitute a value that will ensure that we don't divid= e by
+ =A0 =A0 =A0 * 0.
+ =A0 =A0 =A0 */
+ =A0 =A0 =A0brw_CMP(p, vec1(brw_null_reg()), BRW_CONDITIONAL_EQ,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0vec1(t_nopersp),
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0brw_imm_f(1));

Should= n't this be brw_imm_f(0)?
=A0
+ =A0 =A0 =A0brw_IF(p, BRW_EXECUTE_1);
+ =A0 =A0 =A0brw_MOV(p, t_nopersp, brw_imm_vf4(VF_ONE, VF_ZERO, VF_ZERO, VF= _ZERO));
+ =A0 =A0 =A0brw_ENDIF(p);
+
+ =A0 =A0 =A0/* Now compute t_nopersp =3D t_nopersp.y/t_nopersp.x and broad= cast it */
+ =A0 =A0 =A0brw_math_invert(p, get_element(t_nopersp, 0), get_element(t_no= persp, 0));
+ =A0 =A0 =A0brw_MUL(p,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0vec1(t_nopersp),
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0vec1(t_nopersp),
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0vec1(suboffset(t_nopersp, 1)));
+ =A0 =A0 =A0brw_set_access_mode(p, BRW_ALIGN_16);
+ =A0 =A0 =A0brw_MOV(p, t_nopersp, brw_swizzle(t_nopersp, 0,0,0,0));
+ =A0 =A0 =A0brw_set_access_mode(p, BRW_ALIGN_1);

= That was a very clever computation.=A0 Well done.
=A0
+
+ =A0 =A0 =A0release_tmp(c, tmp);
+ =A0 =A0 =A0release_tmp(c, v0_ndc_copy);
+ =A0 }
+
+ =A0 /* Now we can iterate over each attribute
+ =A0 =A0* (could be done in pairs?)
+ =A0 =A0*/
+ =A0 tmp =3D get_tmp(c);
=A0 =A0 for (slot =3D 0; slot < c->vue_map.num_slots; slot++) {
=A0 =A0 =A0 =A0int vert_result =3D c->vue_map.slot_to_vert_result[slot];=
=A0 =A0 =A0 =A0GLuint delta =3D brw_vue_slot_to_offset(slot);

+ =A0 =A0 =A0/* HPOS is already handled */
+ =A0 =A0 =A0if(vert_result =3D=3D VERT_RESULT_HPOS)
+ =A0 =A0 =A0 =A0 continue;
+
=A0 =A0 =A0 =A0if (vert_result =3D=3D VERT_RESULT_EDGE) {
=A0 =A0 =A0 =A0 =A0if (force_edgeflag)
=A0 =A0 =A0 =A0 =A0 =A0 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,
=A0 =A0 =A0 =A0 =A0 *
=A0 =A0 =A0 =A0 =A0 * =A0 =A0 =A0 =A0New =3D attr0 + t*attr1 - t*attr0
=A0 =A0 =A0 =A0 =A0 */
+
+ =A0 =A0 =A0 =A0 struct brw_reg t =3D
+ =A0 =A0 =A0 =A0 =A0 =A0brw_get_interpolation_mode(brw, slot) =3D=3D INTER= P_QUALIFIER_NOPERSPECTIVE ?
+ =A0 =A0 =A0 =A0 =A0 =A0t_nopersp : t0;
+
=A0 =A0 =A0 =A0 =A0brw_MUL(p,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vec4(brw_null_reg()),
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0deref_4f(v1_ptr, delta),
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0t0);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0t);

=A0 =A0 =A0 =A0 =A0brw_MAC(p,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tmp,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0negate(deref_4f(v0_ptr, delta)),
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0t0);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0t);

=A0 =A0 =A0 =A0 =A0brw_ADD(p,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0deref_4f(dest_ptr, delta),
@@ -198,11 +312,8 @@ void brw_clip_interp_vertex( struct brw_clip_compile *= c,
=A0 =A0 }

=A0 =A0 release_tmp(c, tmp);
-
- =A0 /* Recreate the projected (NDC) coordinate in the new vertex
- =A0 =A0* header:
- =A0 =A0*/
- =A0 brw_clip_project_vertex(c, dest_ptr );
+ =A0 if (c->key.has_noperspective_shading)
+ =A0 =A0 =A0release_tmp(c, t_nopersp);
=A0}

=A0void brw_clip_emit_vue(struct brw_clip_compile *c,
--
1.7.10.rc3.1.gb306

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedeskto= p.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

With the exception of my question abou= t brw_imm_f(0), all of my comments on this patch are stylistic suggestions.= =A0 So assuming we get the brw_imm_f(0) thing figured out, this patch is:
Reviewed-by: Paul Berry <= stereotype441@gmail.com>
--0015175cf8d4c26d5e04c507120a-- --===============0513365795== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0513365795==--