[Mesa-dev] [PATCH 1/2] i965/clip: Add support for gl_ClipVertex
Paul Berry
stereotype441 at gmail.com
Wed Jun 5 13:56:17 PDT 2013
On 5 June 2013 13:49, Chris Forbes <chrisf at ijw.co.nz> wrote:
> Hi Paul
>
> You're right, it needs some more comments :)
>
> Would you be happy with me landing the triangle case first, and then
> following up with a fix for lines at some point in the near future? As
> is, it fixes some pretty awful misrendering.
>
> -- Chris
>
Yeah, I think I'm ok with that. Would you mind adding a note to the commit
message saying that line clipping isn't fixed yet (also maybe a FIXME
comment in clip_and_emit_line(), describing in a sentence or two what still
needs to be done)?
With those changes (and the comments I suggested previously), this patch is:
Reviewed-by: Paul Berry <stereotype441>
>
> On Thu, Jun 6, 2013 at 3:29 AM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > On 5 June 2013 15:50, Chris Forbes <chrisf at ijw.co.nz> wrote:
> >>
> >> When clipping against a user clip plane, and gl_ClipVertex is provided
> >> in the vertex, use it instead of hpos.
> >>
> >> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
> >> ---
> >> src/mesa/drivers/dri/i965/brw_clip.h | 1 +
> >> src/mesa/drivers/dri/i965/brw_clip_tri.c | 44
> >> +++++++++++++++++++++++++++++---
> >> 2 files changed, 41 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_clip.h
> >> b/src/mesa/drivers/dri/i965/brw_clip.h
> >> index c6581ad..a0d5d93 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_clip.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_clip.h
> >> @@ -100,6 +100,7 @@ struct brw_clip_compile {
> >> struct brw_reg plane_equation;
> >>
> >> struct brw_reg ff_sync;
> >> + struct brw_reg vertex_src_mask;
> >
> >
> > 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."
> >
> >>
> >> } reg;
> >>
> >> /* Number of registers storing VUE data */
> >> diff --git a/src/mesa/drivers/dri/i965/brw_clip_tri.c
> >> b/src/mesa/drivers/dri/i965/brw_clip_tri.c
> >> index 05959f6..5491ca8 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_clip_tri.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_clip_tri.c
> >> @@ -119,6 +119,9 @@ void brw_clip_tri_alloc_regs( struct
> brw_clip_compile
> >> *c,
> >> i++;
> >> }
> >>
> >> + c->reg.vertex_src_mask = retype(brw_vec1_grf(i, 0),
> >> BRW_REGISTER_TYPE_UD);
> >> + i++;
> >> +
> >> if (intel->needs_ff_sync) {
> >> c->reg.ff_sync = retype(brw_vec1_grf(i, 0),
> BRW_REGISTER_TYPE_UD);
> >> i++;
> >> @@ -219,6 +222,31 @@ void brw_clip_tri_flat_shade( struct
> brw_clip_compile
> >> *c )
> >> }
> >>
> >>
> >> +static inline void
> >> +load_vertex_pos(struct brw_clip_compile *c, struct brw_indirect vtx,
> >> + struct brw_reg dst,
> >> + GLuint hpos_offset, GLuint clip_offset)
> >> +{
> >> + struct brw_compile *p = &c->func;
> >> +
> >> + /*
> >> + * Roughly:
> >> + * dst = (vertex_src_mask & 1) ? src.hpos : src.clipvertex;
> >> + */
> >> +
> >> + brw_set_conditionalmod(p, BRW_CONDITIONAL_NZ);
> >> + brw_AND(p, vec1(brw_null_reg()), c->reg.vertex_src_mask,
> >> brw_imm_ud(1));
> >> + brw_IF(p, BRW_EXECUTE_1);
> >> + {
> >> + brw_MOV(p, dst, deref_4f(vtx, clip_offset));
> >> + }
> >> + brw_ELSE(p);
> >> + {
> >> + brw_MOV(p, dst, deref_4f(vtx, hpos_offset));
> >> + }
> >> + brw_ENDIF(p);
> >> +}
> >> +
> >>
> >> /* Use mesa's clipping algorithms, translated to GEN4 assembly.
> >> */
> >> @@ -233,13 +261,17 @@ void brw_clip_tri( struct brw_clip_compile *c )
> >> struct brw_indirect outlist_ptr = brw_indirect(5, 0);
> >> struct brw_indirect freelist_ptr = brw_indirect(6, 0);
> >> GLuint hpos_offset = brw_varying_to_offset(&c->vue_map,
> >> VARYING_SLOT_POS);
> >> -
> >> + GLuint clipvert_offset = brw_clip_have_varying(c,
> >> VARYING_SLOT_CLIP_VERTEX)
> >> + ? brw_varying_to_offset(&c->vue_map, VARYING_SLOT_CLIP_VERTEX)
> >> + : hpos_offset;
> >> +
> >> brw_MOV(p, get_addr_reg(vtxPrev), brw_address(c->reg.vertex[2])
> );
> >> brw_MOV(p, get_addr_reg(plane_ptr), brw_clip_plane0_address(c));
> >> brw_MOV(p, get_addr_reg(inlist_ptr), brw_address(c->reg.inlist));
> >> brw_MOV(p, get_addr_reg(outlist_ptr), brw_address(c->reg.outlist));
> >>
> >> brw_MOV(p, get_addr_reg(freelist_ptr), brw_address(c->reg.vertex[3])
> >> );
> >> + brw_MOV(p, c->reg.vertex_src_mask, brw_imm_ud(0xfc0));
> >
> >
> > 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).
> >
> >>
> >>
> >> brw_DO(p, BRW_EXECUTE_1);
> >> {
> >> @@ -269,15 +301,17 @@ void brw_clip_tri( struct brw_clip_compile *c )
> >> */
> >> brw_MOV(p, get_addr_reg(vtx), deref_1uw(inlist_ptr, 0));
> >>
> >> + load_vertex_pos(c, vtxPrev, vec4(c->reg.dpPrev),
> hpos_offset,
> >> clipvert_offset);
> >> /* IS_NEGATIVE(prev) */
> >> brw_set_conditionalmod(p, BRW_CONDITIONAL_L);
> >> - brw_DP4(p, vec4(c->reg.dpPrev), deref_4f(vtxPrev,
> >> hpos_offset), c->reg.plane_equation);
> >> + brw_DP4(p, vec4(c->reg.dpPrev), vec4(c->reg.dpPrev),
> >> c->reg.plane_equation);
> >> brw_IF(p, BRW_EXECUTE_1);
> >> {
> >> + load_vertex_pos(c, vtx, vec4(c->reg.dp), hpos_offset,
> >> clipvert_offset);
> >> /* IS_POSITIVE(next)
> >> */
> >> brw_set_conditionalmod(p, BRW_CONDITIONAL_GE);
> >> - brw_DP4(p, vec4(c->reg.dp), deref_4f(vtx, hpos_offset),
> >> c->reg.plane_equation);
> >> + brw_DP4(p, vec4(c->reg.dp), vec4(c->reg.dp),
> >> c->reg.plane_equation);
> >> brw_IF(p, BRW_EXECUTE_1);
> >> {
> >>
> >> @@ -316,10 +350,11 @@ void brw_clip_tri( struct brw_clip_compile *c )
> >> brw_ADD(p, get_addr_reg(outlist_ptr),
> >> get_addr_reg(outlist_ptr), brw_imm_uw(sizeof(short)));
> >> brw_ADD(p, c->reg.nr_verts, c->reg.nr_verts,
> >> brw_imm_ud(1));
> >>
> >> + load_vertex_pos(c, vtx, vec4(c->reg.dp), hpos_offset,
> >> clipvert_offset);
> >> /* IS_NEGATIVE(next)
> >> */
> >> brw_set_conditionalmod(p, BRW_CONDITIONAL_L);
> >> - brw_DP4(p, vec4(c->reg.dp), deref_4f(vtx, hpos_offset),
> >> c->reg.plane_equation);
> >> + brw_DP4(p, vec4(c->reg.dp), vec4(c->reg.dp),
> >> c->reg.plane_equation);
> >> brw_IF(p, BRW_EXECUTE_1);
> >> {
> >> /* Going out of bounds. Avoid division by zero as we
> >> @@ -392,6 +427,7 @@ void brw_clip_tri( struct brw_clip_compile *c )
> >> */
> >> brw_set_conditionalmod(p, BRW_CONDITIONAL_NZ);
> >> brw_SHR(p, c->reg.planemask, c->reg.planemask, brw_imm_ud(1));
> >> + brw_SHR(p, c->reg.vertex_src_mask, c->reg.vertex_src_mask,
> >> brw_imm_ud(1));
> >
> >
> > 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.
> >
> >>
> >> }
> >> brw_WHILE(p);
> >> }
> >> --
> >> 1.8.3
> >>
> >> _______________________________________________
> >> 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/mesa-dev/attachments/20130605/aac22e8c/attachment.html>
More information about the mesa-dev
mailing list