[Mesa-dev] [PATCH v2 2/2] i965 Gen6: Implement gl_ClipVertex.
Chad Versace
chad at chad-versace.us
Tue Oct 4 10:32:25 PDT 2011
-----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
- --
Chad Versace
chad at 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.
> @@ -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-----
More information about the mesa-dev
mailing list