[Mesa-dev] [PATCH 2/2] i965 Gen6: Implement gl_ClipVertex.

[Kenneth Graunke]

On 09/27/2011 11:05 AM, Paul Berry wrote:
> This patch implements proper support for gl_ClipVertex by causing the
> new VS backend to populate the clip distance VUE slots using
> VERT_RESULT_CLIP_VERTEX when appropriate, and by using the
> untransformed clip planes in ctx->Transform.EyeUserPlane rather than
> the transformed clip planes in ctx->Transform._ClipUserPlane.
> 
> When fixed functionality is in use the driver needs to do the old
> behavior (clip based on VERT_RESULT_HPOS and
> ctx->Transform._ClipUserPlane).  This happens automatically because we
> use the old VS backend for fixed functionality.
> 
> Fixes the following Piglit tests on i965 Gen6:
> - vs-clip-vertex-const-accept
> - vs-clip-vertex-const-reject
> - vs-clip-vertex-different-from-position
> - vs-clip-vertex-equal-to-position
> - vs-clip-vertex-homogeneity
> - vs-clip-based-on-position
> - vs-clip-based-on-position-homogeneity
> - clip-plane-transformation clipvert_pos
> - clip-plane-transformation pos_clipvert
> - clip-plane-transformation pos
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp |   31 ++++++++++++++++++++++-
>  src/mesa/drivers/dri/i965/brw_vs.c             |    8 +++++-
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index e5eda22..f335a86 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -553,7 +553,16 @@ vec4_visitor::setup_uniform_clipplane_values()
>           this->userplane[compacted_clipplane_index] = dst_reg(UNIFORM, this->uniforms);
>           this->userplane[compacted_clipplane_index].type = BRW_REGISTER_TYPE_F;
>           for (int j = 0; j < 4; ++j) {
> -            c->prog_data.param[this->uniforms * 4 + j] = &ctx->Transform._ClipUserPlane[i][j];
> +            /* For fixed functionality shaders, we need to clip based on
> +             * ctx->Transform._ClipUserPlane (which has been transformed by
> +             * Mesa core into clip coordinates).  For user-supplied vertex
> +             * shaders, we need to use the untransformed clip planes in
> +             * ctx->Transform.EyeUserPlane.  Since vec4_visitor is currently
> +             * only used for user-supplied vertex shaders, we can hardcode
> +             * this to EyeUserPlane for now.
> +             */
> +            c->prog_data.param[this->uniforms * 4 + j]
> +               = &ctx->Transform.EyeUserPlane[i][j];

So, we trade support for fixed function clipping for gl_ClipVertex
clipping?  That seems really unfortunate.  I know we don't use the new
VS backend for fixed function today, but we will.

Couldn't you just do:
const bool clip_vertex = c->prog_data.outputs_written &
BITFIELD64_BIT(VERT_RESULT_CLIP_VERTEX);

c->prog_data.param[this->uniforms * 4 + j] =
   clip_vertex ? ctx->Transform.EyeUserPlane[i][j]
               : ctx->Transform._ClipUserPlane[i][j];

...or is outputs_written not available at this point in time?

Yeah, I know it's untested, and untested code = broken code, and all
that, but...if you already know what you need to do...why not just do it?

>           }
>           ++compacted_clipplane_index;
>           ++this->uniforms;
> @@ -1840,9 +1849,27 @@ vec4_visitor::emit_clip_distances(struct brw_reg reg, int offset)
>        return;
>     }
>  
> +   /* From the GLSL 1.30 spec, section 7.1 (Vertex Shader Special Variables):
> +    *
> +    *     "If a linked set of shaders forming the vertex stage contains no
> +    *     static write to gl_ClipVertex or gl_ClipDistance, but the
> +    *     application has requested clipping against user clip planes through
> +    *     the API, then the coordinate written to gl_Position is used for
> +    *     comparison against the user clip planes."
> +    *
> +    * This function is only called if the shader didn't write to
> +    * gl_ClipDistance.  Accordingly, we use gl_ClipVertex to perform clipping
> +    * if the user wrote to it; otherwise we use gl_Position.
> +    */
> +   gl_vert_result clip_vertex = VERT_RESULT_CLIP_VERTEX;
> +   if (!(c->prog_data.outputs_written
> +         & BITFIELD64_BIT(VERT_RESULT_CLIP_VERTEX))) {
> +      clip_vertex = VERT_RESULT_HPOS;
> +   }
> +
>     for (int i = 0; i + offset < c->key.nr_userclip && i < 4; ++i) {
>        emit(DP4(dst_reg(brw_writemask(reg, 1 << i)),
> -               src_reg(output_reg[VERT_RESULT_HPOS]),
> +               src_reg(output_reg[clip_vertex]),
>                 src_reg(this->userplane[i + offset])));
>     }
>  }
> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c
> index 93c6838..4fd260f 100644
> --- a/src/mesa/drivers/dri/i965/brw_vs.c
> +++ b/src/mesa/drivers/dri/i965/brw_vs.c
> @@ -137,11 +137,17 @@ brw_compute_vue_map(struct brw_vue_map *vue_map,
>     /* The hardware doesn't care about the rest of the vertex outputs, so just
>      * assign them contiguously.  Don't reassign outputs that already have a
>      * slot.
> +    *
> +    * Also, don't assign a slot for VERT_RESULT_CLIP_VERTEX, since it is
> +    * unsupported in pre-GEN6, and in GEN6+ the vertex shader converts it into
> +    * clip distances.
>      */
>     for (int i = 0; i < VERT_RESULT_MAX; ++i) {
>        if ((outputs_written & BITFIELD64_BIT(i)) &&
>            vue_map->vert_result_to_slot[i] == -1) {

I'd probably just fold this into the surrounding if statement...
if (...) {
   if (...) {
   }
}
looks a little odd, IMHO.  Especially since the outer if statement's
conditional already spans multiple lines.

> -         assign_vue_slot(vue_map, i);
> +         if (i != VERT_RESULT_CLIP_VERTEX) {
> +            assign_vue_slot(vue_map, i);
> +         }
>        }
>     }
>  }