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); > + } > } > } > } _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev