<div class="gmail_quote">On 29 September 2011 23:16, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div></div><div>On 09/27/2011 11:05 AM, Paul Berry wrote:<br>
> This patch implements proper support for gl_ClipVertex by causing the<br>
> new VS backend to populate the clip distance VUE slots using<br>
> VERT_RESULT_CLIP_VERTEX when appropriate, and by using the<br>
> untransformed clip planes in ctx->Transform.EyeUserPlane rather than<br>
> the transformed clip planes in ctx->Transform._ClipUserPlane.<br>
><br>
> When fixed functionality is in use the driver needs to do the old<br>
> behavior (clip based on VERT_RESULT_HPOS and<br>
> ctx->Transform._ClipUserPlane). This happens automatically because we<br>
> use the old VS backend for fixed functionality.<br>
><br>
> Fixes the following Piglit tests on i965 Gen6:<br>
> - vs-clip-vertex-const-accept<br>
> - vs-clip-vertex-const-reject<br>
> - vs-clip-vertex-different-from-position<br>
> - vs-clip-vertex-equal-to-position<br>
> - vs-clip-vertex-homogeneity<br>
> - vs-clip-based-on-position<br>
> - vs-clip-based-on-position-homogeneity<br>
> - clip-plane-transformation clipvert_pos<br>
> - clip-plane-transformation pos_clipvert<br>
> - clip-plane-transformation pos<br>
> ---<br>
> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 31 ++++++++++++++++++++++-<br>
> src/mesa/drivers/dri/i965/brw_vs.c | 8 +++++-<br>
> 2 files changed, 36 insertions(+), 3 deletions(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
> index e5eda22..f335a86 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp<br>
> @@ -553,7 +553,16 @@ vec4_visitor::setup_uniform_clipplane_values()<br>
> this->userplane[compacted_clipplane_index] = dst_reg(UNIFORM, this->uniforms);<br>
> this->userplane[compacted_clipplane_index].type = BRW_REGISTER_TYPE_F;<br>
> for (int j = 0; j < 4; ++j) {<br>
> - c->prog_data.param[this->uniforms * 4 + j] = &ctx->Transform._ClipUserPlane[i][j];<br>
> + /* For fixed functionality shaders, we need to clip based on<br>
> + * ctx->Transform._ClipUserPlane (which has been transformed by<br>
> + * Mesa core into clip coordinates). For user-supplied vertex<br>
> + * shaders, we need to use the untransformed clip planes in<br>
> + * ctx->Transform.EyeUserPlane. Since vec4_visitor is currently<br>
> + * only used for user-supplied vertex shaders, we can hardcode<br>
> + * this to EyeUserPlane for now.<br>
> + */<br>
> + c->prog_data.param[this->uniforms * 4 + j]<br>
> + = &ctx->Transform.EyeUserPlane[i][j];<br>
<br>
</div></div>So, we trade support for fixed function clipping for gl_ClipVertex<br>
clipping? That seems really unfortunate. I know we don't use the new<br>
VS backend for fixed function today, but we will.<br></blockquote><div><br></div><div>My intention was never to give up support for fixed function clipping. I just don't know how to tell, from within the vertex shader backend, whether the shader we're compiling is an application-defined GLSL shader or Mesa's built-in fixed function vertex shader. Since at the moment we use the old VS backend for fixed function, and the new VS backend for application-defined GLSL shaders, I figured I could dodge the question for now by putting the fixed-function logic in the old VS backend and the non-fixed-function logic in the new VS backend. Unfortunately your eyes were too sharp for me to get away with that dodge :)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Couldn't you just do:<br>
const bool clip_vertex = c->prog_data.outputs_written &<br>
BITFIELD64_BIT(VERT_RESULT_CLIP_VERTEX);<br>
<div><br>
c->prog_data.param[this->uniforms * 4 + j] =<br>
</div> clip_vertex ? ctx->Transform.EyeUserPlane[i][j]<br>
: ctx->Transform._ClipUserPlane[i][j];<br>
<br>
...or is outputs_written not available at this point in time?<br></blockquote><div><br></div><div>Yes, outputs_written is available at this point in time. But I'm not certain whether this code would be correct. The question hinges on how we interpret a subtle ambiguity in the GLSL 1.30 spec: what happens in the case where clipping is in use, but the application-supplied vertex shader doesn't write to either gl_ClipVertex or gl_ClipDistance? Accompany me, if you dare, into ambiguous spec land:</div>
<div><br></div><div>GL 2.1, GL 3.0, GLSL 1.10, and GLSL 1.20 all say that the behavior is undefined if the vertex shader writes to neither gl_ClipVertex nor gl_ClipDistance. But GLSL 1.30 says this: "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." The subtle ambiguity is: when using gl_Position for comparison against the user clip planes, should we transform it from clip coordinates to eye coordinates before comparing it with the user clip planes? Or equivalently, should we transform the user clip planes from eye coordinates to clip coordinates before comparing them with gl_Position? (The second, equivalent form of the question is the form that is relevant to Mesa; our answer determines whether we should upload ctx->Transform.EyeUserPlane or ctx->Transform._ClipUserPlane).</div>
<div><br></div><div>If the answer is "yes, the coordinates should be transformed", then we should use ctx->Transform.EyeUserPlane when the shader writes to gl_ClipVertex and ctx->Transform._ClipUserPlane when it doesn't. In that case your suggestion would work fine, and the code I submitted is wrong. But if the answer is "no, the coordinates should not be transformed", then we need to use ctx->Transform.EyeUserPlane for application-provided vertex shaders, and ctx->Transform._ClipUserPlane for Mesa's built-in fixed function vertex shader.</div>
<div><br></div><div>IMHO, the correct answer is "no, the coordinates should not be transformed". I'm basing this on discussions I had with Chad last Wednesday while I was writing the clip-plane-transformation Piglit test. But I'm by no means certain. Here are the arguments I can think of both for and against doing the coordinate transformation:</div>
<div><br></div><div>Argument against: If the spec writers intended for a coordinate transformation to be used when using gl_Position to substitute for gl_ClipVertex, surely they would have specifically said this in the spec. They didn't.</div>
<div><br></div><div>Argument against: GL 2.1 and GL 3.0 say "The user must ensure that the clip vertex and client-defined clip planes are defined in the same coordinate space." This seems to heavily imply that there is no preferred coordinate space for gl_ClipVertex; the application may use whatever coordinate space it desires, provided that it specifies user clip planes in the same coordinate space. So who is to say that we should transform from clip coordinates to eye coordinates when using gl_Position to substitute for gl_ClipVertex? Since the specs never explicitly say what coordinate space gl_ClipVertex is intended to be in, the only sensible interpretation is to do no coordinate transformation at all.</div>
<div><br></div><div>Argument for: In the description of ClipPlane() in GL 2.1 and GL 3.0, it is clear that the clip planes specified by the application are meant to be translated from object coordinates to eye coordinates at the time the clip planes are specified (this must be the case, because the spec says we use the model-view matrix to do this transformation). Since gl_ClipVertex's sole purpose is to be used for comparison against user clip planes, and the user clip planes are clearly meant to be stored in eye coordinates, gl_ClipVertex is clearly meant to be in eye coordinates. Therefore when using gl_Position to substitute for gl_ClipVertex, we should transform from clip coordinates to eye coordinates.</div>
<div><br></div><div>Argument for: The shader "void main() { gl_Position = gl_ModelViewProjectionMatrix * gl_Vertex; }" ought to behave as much like fixed functionality as possible. Since clip planes are interpreted in eye coordinates in fixed functionality, when using gl_Position to substitute for gl_ClipVertex, we really ought to transform from clip coordinates to eye coordinates.</div>
<div><br></div><div>Incidentally, I was hoping I could resolve this question by trying some experiments on my nVidia-based system at home, and seeing what it did. Unfortunately, what it does if you don't write to gl_ClipVertex is: fail to clip at all. So if it's any consolation, no matter what we do we'll be in better conformance with the specs than nVidia :)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Yeah, I know it's untested, and untested code = broken code, and all<br>
that, but...if you already know what you need to do...why not just do it?<br>
<div><div></div><div><br>
> }<br>
> ++compacted_clipplane_index;<br>
> ++this->uniforms;<br>
> @@ -1840,9 +1849,27 @@ vec4_visitor::emit_clip_distances(struct brw_reg reg, int offset)<br>
> return;<br>
> }<br>
><br>
> + /* From the GLSL 1.30 spec, section 7.1 (Vertex Shader Special Variables):<br>
> + *<br>
> + * "If a linked set of shaders forming the vertex stage contains no<br>
> + * static write to gl_ClipVertex or gl_ClipDistance, but the<br>
> + * application has requested clipping against user clip planes through<br>
> + * the API, then the coordinate written to gl_Position is used for<br>
> + * comparison against the user clip planes."<br>
> + *<br>
> + * This function is only called if the shader didn't write to<br>
> + * gl_ClipDistance. Accordingly, we use gl_ClipVertex to perform clipping<br>
> + * if the user wrote to it; otherwise we use gl_Position.<br>
> + */<br>
> + gl_vert_result clip_vertex = VERT_RESULT_CLIP_VERTEX;<br>
> + if (!(c->prog_data.outputs_written<br>
> + & BITFIELD64_BIT(VERT_RESULT_CLIP_VERTEX))) {<br>
> + clip_vertex = VERT_RESULT_HPOS;<br>
> + }<br>
> +<br>
> for (int i = 0; i + offset < c->key.nr_userclip && i < 4; ++i) {<br>
> emit(DP4(dst_reg(brw_writemask(reg, 1 << i)),<br>
> - src_reg(output_reg[VERT_RESULT_HPOS]),<br>
> + src_reg(output_reg[clip_vertex]),<br>
> src_reg(this->userplane[i + offset])));<br>
> }<br>
> }<br>
> diff --git a/src/mesa/drivers/dri/i965/brw_vs.c b/src/mesa/drivers/dri/i965/brw_vs.c<br>
> index 93c6838..4fd260f 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_vs.c<br>
> +++ b/src/mesa/drivers/dri/i965/brw_vs.c<br>
> @@ -137,11 +137,17 @@ brw_compute_vue_map(struct brw_vue_map *vue_map,<br>
> /* The hardware doesn't care about the rest of the vertex outputs, so just<br>
> * assign them contiguously. Don't reassign outputs that already have a<br>
> * slot.<br>
> + *<br>
> + * Also, don't assign a slot for VERT_RESULT_CLIP_VERTEX, since it is<br>
> + * unsupported in pre-GEN6, and in GEN6+ the vertex shader converts it into<br>
> + * clip distances.<br>
> */<br>
> for (int i = 0; i < VERT_RESULT_MAX; ++i) {<br>
> if ((outputs_written & BITFIELD64_BIT(i)) &&<br>
> vue_map->vert_result_to_slot[i] == -1) {<br>
<br>
</div></div>I'd probably just fold this into the surrounding if statement...<br>
if (...) {<br>
if (...) {<br>
}<br>
}<br>
looks a little odd, IMHO. Especially since the outer if statement's<br>
conditional already spans multiple lines.<br></blockquote><div><br></div><div>Ok.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div></div><div><br>
> - assign_vue_slot(vue_map, i);<br>
> + if (i != VERT_RESULT_CLIP_VERTEX) {<br>
> + assign_vue_slot(vue_map, i);<br>
> + }<br>
> }<br>
> }<br>
> }<br>
</div></div></blockquote></div><br>