<div dir="ltr">Whops, accidentally sent this reply just to Chris.<br><div><br><div class="gmail_quote">---------- Forwarded message ----------<br>From: <b class="gmail_sendername">Paul Berry</b> <span dir="ltr"><<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>></span><br>
Date: 5 June 2013 08:29<br>Subject: Re: [Mesa-dev] [PATCH 1/2] i965/clip: Add support for gl_ClipVertex<br>To: Chris Forbes <<a href="mailto:chrisf@ijw.co.nz">chrisf@ijw.co.nz</a>><br><br><br><div dir="ltr"><div class="im">
On 5 June 2013 15:50, Chris Forbes <span dir="ltr"><<a href="mailto:chrisf@ijw.co.nz" target="_blank">chrisf@ijw.co.nz</a>></span> wrote:<br></div><div class="gmail_extra"><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
When clipping against a user clip plane, and gl_ClipVertex is provided<br>
in the vertex, use it instead of hpos.<br>
<br>
Signed-off-by: Chris Forbes <<a href="mailto:chrisf@ijw.co.nz" target="_blank">chrisf@ijw.co.nz</a>><br>
---<br>
src/mesa/drivers/dri/i965/brw_clip.h | 1 +<br>
src/mesa/drivers/dri/i965/brw_clip_tri.c | 44 +++++++++++++++++++++++++++++---<br>
2 files changed, 41 insertions(+), 4 deletions(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_clip.h b/src/mesa/drivers/dri/i965/brw_clip.h<br>
index c6581ad..a0d5d93 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_clip.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_clip.h<br>
@@ -100,6 +100,7 @@ struct brw_clip_compile {<br>
struct brw_reg plane_equation;<br>
<br>
struct brw_reg ff_sync;<br>
+ struct brw_reg vertex_src_mask;<br></blockquote><div><br></div></div><div>It would have saved me a lot of review effort to have a comment above this new variable, telling me (a) the meaning of 1's and 0's in this register, and (b) what the bitmask is indexed by. Consider saying something like "Bitmask indicating which coordinate attribute should be used for comparison to each clipping plane. 0 indicates that the plane should be clipped against VARYING_SLOT_POS, because it's one of the fixed +/- x, y, or z planes that constitute the bounds of the view volume. 1 indicates that it should be clipped against VARYING_SLOT_CLIP_VERTEX (if available), because it's a user-defined clipping plane." <br>
</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
} reg;<br>
<br>
/* Number of registers storing VUE data */<br>
diff --git a/src/mesa/drivers/dri/i965/brw_clip_tri.c b/src/mesa/drivers/dri/i965/brw_clip_tri.c<br>
index 05959f6..5491ca8 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_clip_tri.c<br>
+++ b/src/mesa/drivers/dri/i965/brw_clip_tri.c<br>
@@ -119,6 +119,9 @@ void brw_clip_tri_alloc_regs( struct brw_clip_compile *c,<br>
i++;<br>
}<br>
<br>
+ c->reg.vertex_src_mask = retype(brw_vec1_grf(i, 0), BRW_REGISTER_TYPE_UD);<br>
+ i++;<br>
+<br>
if (intel->needs_ff_sync) {<br>
c->reg.ff_sync = retype(brw_vec1_grf(i, 0), BRW_REGISTER_TYPE_UD);<br>
i++;<br>
@@ -219,6 +222,31 @@ void brw_clip_tri_flat_shade( struct brw_clip_compile *c )<br>
}<br>
<br>
<br>
+static inline void<br>
+load_vertex_pos(struct brw_clip_compile *c, struct brw_indirect vtx,<br>
+ struct brw_reg dst,<br>
+ GLuint hpos_offset, GLuint clip_offset)<br>
+{<br>
+ struct brw_compile *p = &c->func;<br>
+<br>
+ /*<br>
+ * Roughly:<br>
+ * dst = (vertex_src_mask & 1) ? src.hpos : src.clipvertex;<br>
+ */<br>
+<br>
+ brw_set_conditionalmod(p, BRW_CONDITIONAL_NZ);<br>
+ brw_AND(p, vec1(brw_null_reg()), c->reg.vertex_src_mask, brw_imm_ud(1));<br>
+ brw_IF(p, BRW_EXECUTE_1);<br>
+ {<br>
+ brw_MOV(p, dst, deref_4f(vtx, clip_offset));<br>
+ }<br>
+ brw_ELSE(p);<br>
+ {<br>
+ brw_MOV(p, dst, deref_4f(vtx, hpos_offset));<br>
+ }<br>
+ brw_ENDIF(p);<br>
+}<br>
+<br>
<br>
/* Use mesa's clipping algorithms, translated to GEN4 assembly.<br>
*/<br>
@@ -233,13 +261,17 @@ void brw_clip_tri( struct brw_clip_compile *c )<br>
struct brw_indirect outlist_ptr = brw_indirect(5, 0);<br>
struct brw_indirect freelist_ptr = brw_indirect(6, 0);<br>
GLuint hpos_offset = brw_varying_to_offset(&c->vue_map, VARYING_SLOT_POS);<br>
-<br>
+ GLuint clipvert_offset = brw_clip_have_varying(c, VARYING_SLOT_CLIP_VERTEX)<br>
+ ? brw_varying_to_offset(&c->vue_map, VARYING_SLOT_CLIP_VERTEX)<br>
+ : hpos_offset;<br>
+<br>
brw_MOV(p, get_addr_reg(vtxPrev), brw_address(c->reg.vertex[2]) );<br>
brw_MOV(p, get_addr_reg(plane_ptr), brw_clip_plane0_address(c));<br>
brw_MOV(p, get_addr_reg(inlist_ptr), brw_address(c->reg.inlist));<br>
brw_MOV(p, get_addr_reg(outlist_ptr), brw_address(c->reg.outlist));<br>
<br>
brw_MOV(p, get_addr_reg(freelist_ptr), brw_address(c->reg.vertex[3]) );<br>
+ brw_MOV(p, c->reg.vertex_src_mask, brw_imm_ud(0xfc0));<br></blockquote><div><br></div></div></div><div>It would also be nice to have a quick comment here explaining the source of the magic value 0xfc0 (it's initializing the bitfield based on the fact that the first 6 planes constitute the bounds of the view volume, and the next 6 planes are user clip planes). <br>
</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
brw_DO(p, BRW_EXECUTE_1);<br>
{<br>
@@ -269,15 +301,17 @@ void brw_clip_tri( struct brw_clip_compile *c )<br>
*/<br>
brw_MOV(p, get_addr_reg(vtx), deref_1uw(inlist_ptr, 0));<br>
<br>
+ load_vertex_pos(c, vtxPrev, vec4(c->reg.dpPrev), hpos_offset, clipvert_offset);<br>
/* IS_NEGATIVE(prev) */<br>
brw_set_conditionalmod(p, BRW_CONDITIONAL_L);<br>
- brw_DP4(p, vec4(c->reg.dpPrev), deref_4f(vtxPrev, hpos_offset), c->reg.plane_equation);<br>
+ brw_DP4(p, vec4(c->reg.dpPrev), vec4(c->reg.dpPrev), c->reg.plane_equation);<br>
brw_IF(p, BRW_EXECUTE_1);<br>
{<br>
+ load_vertex_pos(c, vtx, vec4(c->reg.dp), hpos_offset, clipvert_offset);<br>
/* IS_POSITIVE(next)<br>
*/<br>
brw_set_conditionalmod(p, BRW_CONDITIONAL_GE);<br>
- brw_DP4(p, vec4(c->reg.dp), deref_4f(vtx, hpos_offset), c->reg.plane_equation);<br>
+ brw_DP4(p, vec4(c->reg.dp), vec4(c->reg.dp), c->reg.plane_equation);<br>
brw_IF(p, BRW_EXECUTE_1);<br>
{<br>
<br>
@@ -316,10 +350,11 @@ void brw_clip_tri( struct brw_clip_compile *c )<br>
brw_ADD(p, get_addr_reg(outlist_ptr), get_addr_reg(outlist_ptr), brw_imm_uw(sizeof(short)));<br>
brw_ADD(p, c->reg.nr_verts, c->reg.nr_verts, brw_imm_ud(1));<br>
<br>
+ load_vertex_pos(c, vtx, vec4(c->reg.dp), hpos_offset, clipvert_offset);<br>
/* IS_NEGATIVE(next)<br>
*/<br>
brw_set_conditionalmod(p, BRW_CONDITIONAL_L);<br>
- brw_DP4(p, vec4(c->reg.dp), deref_4f(vtx, hpos_offset), c->reg.plane_equation);<br>
+ brw_DP4(p, vec4(c->reg.dp), vec4(c->reg.dp), c->reg.plane_equation);<br>
brw_IF(p, BRW_EXECUTE_1);<br>
{<br>
/* Going out of bounds. Avoid division by zero as we<br>
@@ -392,6 +427,7 @@ void brw_clip_tri( struct brw_clip_compile *c )<br>
*/<br>
brw_set_conditionalmod(p, BRW_CONDITIONAL_NZ);<br>
brw_SHR(p, c->reg.planemask, c->reg.planemask, brw_imm_ud(1));<br>
+ brw_SHR(p, c->reg.vertex_src_mask, c->reg.vertex_src_mask, brw_imm_ud(1));<br></blockquote><div><br></div></div></div><div>This covers the case of clipping triangles. For lines, I believe you need to make similar changes to clip_and_emit_line() (brw_clip_line.c). You might want to add a couple of piglit tests too.<br>
</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
}<br>
brw_WHILE(p);<br>
}<br>
<span><font color="#888888">--<br>
1.8.3<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div></div><br></div></div>
</div><br></div></div>