On 4 October 2011 10:32, Chad Versace <c...@chad-versace.us> wrote:
> -----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
>
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?
>
> - --
> Chad Versace
> c...@chad-versace.us
>
> 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 {
>
>
> The above hunk is spurious.
>
Oops. I'll remove this.
>
>
> > @@ -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
> */
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> 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-----
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
