On 4 October 2011 10:32, Chad Versace <<a href="mailto:chad@chad-versace.us">chad@chad-versace.us</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Overall, the patch looks good, but I'm going to take the bikeshed bait. Symbols of form GLxxx and constants of form GL_XXX should be reserved for those defined in the GL headers. If I encountered the symbol GLclipplane in Mesa, I would confidently, yet incorrectly, assume the symbol was defined in some GL extension of which I was unaware. Typically, when we wish a symbol name to be closely related to it GL analogue, we use one of the following naming schemes: fuctions: _mesa_CamelCaps, intel_no_caps struct: gl_no_caps enums: gl_no_caps constants: MESA_XXX For examples: _mesa_BindFramebufferEXT(), intel_bind_framebuffer() struct gl_framebuffer enum gl_format MESA_FORMAT_RGBA8888 </blockquote><div> Ok, I'm fine with this. Unless someone has a better suggestion, I propose renaming "GLclipplane" to "gl_clip_plane". Would that address your concerns, Chad? </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> - -- Chad Versace <a href="mailto:chad@chad-versace.us">chad@chad-versace.us</a> <div><div></div><div class="h5"> On 10/03/2011 02:17 PM, 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 a > GLSL-based vertex shader is in use. > > When not using a GLSL-based vertex shader, we use > ctx->Transform._ClipUserPlane (which is what we used prior to this > patch). This ensures that clipping is still performed correctly for > fixed function and ARB vertex programs. A new function, > brw_select_clip_planes() is used to determine whether to use > _ClipUserPlane or EyeUserPlane, so that the logic for making this > decision is shared between the new and old vertex shaders. > > 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_context.h | 3 ++ > src/mesa/drivers/dri/i965/brw_curbe.c | 10 ++++--- > src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 24 +++++++++++++++- > src/mesa/drivers/dri/i965/brw_vs.c | 34 +++++++++++++++++++++++- > src/mesa/drivers/dri/i965/gen6_vs_state.c | 3 +- > src/mesa/main/mtypes.h | 11 ++++++- > 6 files changed, 75 insertions(+), 10 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h > index d32eded..7c5bfd0 100644 > --- a/src/mesa/drivers/dri/i965/brw_context.h > +++ b/src/mesa/drivers/dri/i965/brw_context.h > @@ -899,6 +899,7 @@ struct brw_context > }; > > > + > #define BRW_PACKCOLOR8888(r,g,b,a) ((r<<24) | (g<<16) | (b<<8) | a) > > struct brw_instruction_info { </div></div>The above hunk is spurious. </blockquote><div> Oops. I'll remove this. </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"> <div><div></div><div class="h5"> > @@ -966,6 +967,8 @@ int brw_disasm (FILE *file, struct brw_instruction *inst, int gen); > void brw_compute_vue_map(struct brw_vue_map *vue_map, > const struct intel_context *intel, int nr_userclip, > GLbitfield64 outputs_written); > +GLclipplane *brw_select_clip_planes(struct gl_context *ctx); > + > > /*====================================================================== > * Inline conversion functions. These are better-typed than the > diff --git a/src/mesa/drivers/dri/i965/brw_curbe.c b/src/mesa/drivers/dri/i965/brw_curbe.c > index e1676de..5d2a024 100644 > --- a/src/mesa/drivers/dri/i965/brw_curbe.c > +++ b/src/mesa/drivers/dri/i965/brw_curbe.c > @@ -188,6 +188,7 @@ static void prepare_constant_buffer(struct brw_context *brw) > const GLuint bufsz = sz * 16 * sizeof(GLfloat); > GLfloat *buf; > GLuint i; > + GLclipplane *clip_planes; > > if (sz == 0) { > brw->curbe.last_bufsz = 0; > @@ -232,12 +233,13 @@ static void prepare_constant_buffer(struct brw_context *brw) > /* Clip planes: _NEW_TRANSFORM plus _NEW_PROJECTION to get to > * clip-space: > */ > + clip_planes = brw_select_clip_planes(ctx); > for (j = 0; j < MAX_CLIP_PLANES; j++) { > if (ctx->Transform.ClipPlanesEnabled & (1<<j)) { > - buf[offset + i * 4 + 0] = ctx->Transform._ClipUserPlane[j][0]; > - buf[offset + i * 4 + 1] = ctx->Transform._ClipUserPlane[j][1]; > - buf[offset + i * 4 + 2] = ctx->Transform._ClipUserPlane[j][2]; > - buf[offset + i * 4 + 3] = ctx->Transform._ClipUserPlane[j][3]; > + buf[offset + i * 4 + 0] = clip_planes[j][0]; > + buf[offset + i * 4 + 1] = clip_planes[j][1]; > + buf[offset + i * 4 + 2] = clip_planes[j][2]; > + buf[offset + i * 4 + 3] = clip_planes[j][3]; > i++; > } > } > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > index ad8b433..0f16342 100644 > --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > @@ -557,6 +557,8 @@ vec4_visitor::setup_uniform_values(int loc, const glsl_type *type) > void > vec4_visitor::setup_uniform_clipplane_values() > { > + GLclipplane *clip_planes = brw_select_clip_planes(ctx); > + > int compacted_clipplane_index = 0; > for (int i = 0; i < MAX_CLIP_PLANES; ++i) { > if (ctx->Transform.ClipPlanesEnabled & (1 << i)) { > @@ -564,7 +566,7 @@ 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]; > + c->prog_data.param[this->uniforms * 4 + j] = &clip_planes[i][j]; > } > ++compacted_clipplane_index; > ++this->uniforms; > @@ -1863,9 +1865,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..360deed 100644 > --- a/src/mesa/drivers/dri/i965/brw_vs.c > +++ b/src/mesa/drivers/dri/i965/brw_vs.c > @@ -137,15 +137,47 @@ 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) { > + vue_map->vert_result_to_slot[i] == -1 && > + i != VERT_RESULT_CLIP_VERTEX) { > assign_vue_slot(vue_map, i); > } > } > } > > + > +/** > + * Decide which set of clip planes should be used when clipping via > + * gl_Position or gl_ClipVertex. > + */ > +GLclipplane *brw_select_clip_planes(struct gl_context *ctx) > +{ > + if (ctx->Shader.CurrentVertexProgram) { > + /* There is currently a GLSL vertex shader, so clip according to GLSL > + * rules, which means compare gl_ClipVertex (or gl_Position, if > + * gl_ClipVertex wasn't assigned) against the eye-coordinate clip planes > + * that were stored in EyeUserPlane at the time the clip planes were > + * specified. > + */ > + return ctx->Transform.EyeUserPlane; > + } else { > + /* Either we are using fixed function or an ARB vertex program. In > + * either case the clip planes are going to be compared against > + * gl_Position (which is in clip coordinates) so we have to clip using > + * _ClipUserPlane, which was transformed into clip coordinates by Mesa > + * core. > + */ > + return ctx->Transform._ClipUserPlane; > + } > +} > + > + > static bool > do_vs_prog(struct brw_context *brw, > struct gl_shader_program *prog, > diff --git a/src/mesa/drivers/dri/i965/gen6_vs_state.c b/src/mesa/drivers/dri/i965/gen6_vs_state.c > index 0f6f6a7..d827ee0 100644 > --- a/src/mesa/drivers/dri/i965/gen6_vs_state.c > +++ b/src/mesa/drivers/dri/i965/gen6_vs_state.c > @@ -77,9 +77,10 @@ gen6_prepare_vs_push_constants(struct brw_context *brw) > * until we redo the VS backend. > */ > if (!uses_clip_distance) { > + GLclipplane *clip_planes = brw_select_clip_planes(ctx); > for (i = 0; i < MAX_CLIP_PLANES; i++) { > if (ctx->Transform.ClipPlanesEnabled & (1 << i)) { > - memcpy(param, ctx->Transform._ClipUserPlane[i], 4 * sizeof(float)); > + memcpy(param, clip_planes[i], 4 * sizeof(float)); > param += 4; > params_uploaded++; > } > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h > index ab03d9a..265559b 100644 > --- a/src/mesa/main/mtypes.h > +++ b/src/mesa/main/mtypes.h > @@ -1493,13 +1493,20 @@ struct gl_texture_attrib > > > /** > + * Data structure representing a single clip plane (e.g. one of the elements > + * of the ctx->Transform.EyeUserPlane or ctx->Transform._ClipUserPlane array). > + */ > +typedef GLfloat GLclipplane[4]; > + > + > +/** > * Transformation attribute group (GL_TRANSFORM_BIT). > */ > struct gl_transform_attrib > { > GLenum MatrixMode; /**< Matrix mode */ > - GLfloat EyeUserPlane[MAX_CLIP_PLANES][4]; /**< User clip planes */ > - GLfloat _ClipUserPlane[MAX_CLIP_PLANES][4]; /**< derived */ > + GLclipplane EyeUserPlane[MAX_CLIP_PLANES]; /**< User clip planes */ > + GLclipplane _ClipUserPlane[MAX_CLIP_PLANES]; /**< derived */ > GLbitfield ClipPlanesEnabled; /**< on/off bitmask */ > GLboolean Normalize; /**< Normalize all normals? */ > GLboolean RescaleNormals; /**< GL_EXT_rescale_normal */ </div></div>-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - <a href="http://enigmail.mozdev.org/" target="_blank">http://enigmail.mozdev.org/</a> iQIcBAEBAgAGBQJOi0MpAAoJEAIvNt057x8iiWIP/1WrKCQhfrU6u3fc5fNiZYlU MULaamE5N8PxdrXXEoBA4Yrmi5RyNo+ilPzDolLzVyaihykJ+pFUuRzb3pV48NSb RDnPvuq1AHb1gCSmr1VuNX5WbZJI9ElwzwmK5V/YiIr5hp9vbYsTfiIoxKYNIhdn aGkjhQ8zEhxD84r0PODg0nSEmS/KDYSm8kusDAhDIEmGyH2gYPbqUkBkSdMAMJqp XvZNtO/Odxf20/81KKsTGrLorhvxbyKblpaNG/WQ0vrlJ2PvK8gUuxNjQV/lW5u/ DQ+PEp3l1t4jPylE8li5oMDBRfSwGv0fIH9JNH1yxKLT7dbEzGxw7PfW17kSEdf5 fFVe0iyfXNFyZG+CYQjfQDOCH/w7rRpX7gev4gOVV+jXluaazG+V/4AMCYgg8JpG cSDZ+zK0UxQb1z6WWDwfIrMemcdYBV60ZqQBNJyoIa1GuanPGp66mrf1pmaWbkwY ADF/4NYtuabR3Ta5YMehHWt7eBiiEcfMGyJHqUnNcoYlvk56GxUB021DQaAWEYMn 7rLEPp1tPXgwTDr1Z3XdwGyYyFWg06MuYMhFhv/vUdEieJMbYYx6HmaMAPsmgnkO 9f1uwcEJ5ZMO59hDL/8y4/s3tXKIwdjmAko/pfsg+Cn0XNhaXSTPcmD15sCoNT0Y cXOcwHrdS9DfsWl+TH1r =tUpM -----END PGP SIGNATURE----- </blockquote></div>