[Mesa-dev] [PATCH] glsl/gs: Fix transform feedback of gl_ClipDistance.

[Pohjolainen, Topi]

On Thu, Oct 24, 2013 at 03:38:49AM -0700, Paul Berry wrote:
>    On 24 October 2013 00:13, Pohjolainen, Topi <topi.pohjolainen at intel.com>
>    wrote:
> 
>      On Wed, Oct 23, 2013 at 01:08:42PM -0700, Paul Berry wrote:
>      > Since gl_ClipDistance is lowered from an array of floats to an array
>      > of vec4's during compilation, transform feedback has special logic to
>      > keep track of the pre-lowered array size so that attempting to perform
>      > transform feedback on gl_ClipDistance produces a result with the
>      > correct size.
>      >
>      > Previously, this special logic always consulted the vertex shader's
>      > size for gl_ClipDistance.  This patch fixes it so that it uses the
>      > geometry shader's size for gl_ClipDistance when a geometry shader is
>      > in use.
>      >
>      > Fixes piglit test spec/glsl-1.50/transform-feedback-type-and-size.
>      > ---
>      >  src/glsl/link_varyings.cpp | 2 +-
>      >  src/glsl/linker.cpp        | 2 ++
>      >  src/mesa/main/mtypes.h     | 6 ++++++
>      >  3 files changed, 9 insertions(+), 1 deletion(-)
>      >
>      > diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
>      > index 4ba6d8a..c503645 100644
>      > --- a/src/glsl/link_varyings.cpp
>      > +++ b/src/glsl/link_varyings.cpp
>      > @@ -328,7 +328,7 @@ tfeedback_decl::assign_location(struct gl_context
>      *ctx,
>      >        const unsigned vector_elements =
>      >          
>      this->matched_candidate->type->fields.array->vector_elements;
>      >        unsigned actual_array_size = this->is_clip_distance_mesa ?
>      > -         prog->Vert.ClipDistanceArraySize :
>      > +         prog->LastClipDistanceArraySize :
>      >           this->matched_candidate->type->array_size();
>      >
>      >        if (this->is_subscripted) {
>      > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>      > index b23c31a..d8f655c 100644
>      > --- a/src/glsl/linker.cpp
>      > +++ b/src/glsl/linker.cpp
>      > @@ -2100,6 +2100,7 @@ link_shaders(struct gl_context *ctx, struct
>      gl_shader_program *prog)
>      >        validate_vertex_shader_executable(prog, sh);
>      >        if (!prog->LinkStatus)
>      >        goto done;
>      > +      prog->LastClipDistanceArraySize =
>      prog->Vert.ClipDistanceArraySize;
>      >
>      >        _mesa_reference_shader(ctx,
>      &prog->_LinkedShaders[MESA_SHADER_VERTEX],
>      >                            sh);
>      > @@ -2132,6 +2133,7 @@ link_shaders(struct gl_context *ctx, struct
>      gl_shader_program *prog)
>      >        validate_geometry_shader_executable(prog, sh);
>      >        if (!prog->LinkStatus)
>      >        goto done;
>      > +      prog->LastClipDistanceArraySize =
>      prog->Geom.ClipDistanceArraySize;
>      >
>      >        _mesa_reference_shader(ctx,
>      &prog->_LinkedShaders[MESA_SHADER_GEOMETRY],
>      >                            sh);
>      > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>      > index 6374e8c..bc7dea4 100644
>      > --- a/src/mesa/main/mtypes.h
>      > +++ b/src/mesa/main/mtypes.h
>      > @@ -2476,6 +2476,12 @@ struct gl_shader_program
>      >     unsigned NumUserUniformStorage;
>      >     struct gl_uniform_storage *UniformStorage;
>      >
>      > +   /**
>      > +    * Size of the gl_ClipDistance array that is output from the last
>      pipeline
>      > +    * stage before the geometry shader.
> 
>      Can you explain the "before the geometry shader" part? This is used by
>      the
>      transform feedback mechanism and hence represents the size of the
>      varying output
>      by the geometry shader (if present of course), right? This comment in
>      turn
>      refers to the varying output by vertex shader and _read_ by geometry
>      shader.
> 
>    Oops.  I meant to say "...output from the last pipeline stage before the
>    fragment shader".  Is that clearer?
>    In other words, for now, it's the geometry shader (if present), and the
>    vertex shader otherwise.  But in the future, when we add tessellation
>    shaders, it'll be:
>    - the size of gl_ClipDistance output by the geometry shader, if present.
>    - Otherwise, the size of gl_ClipDistance output by the tessellation
>    evaluation shader, if present.
>    - Otherwise, the size of gl_ClipDistance output by the tessellation
>    control shader, if present.
>    - Otherwise, the size of gl_ClipDistance output by the vertex shader.

This makes perfect sense, thanks!

>     
> 
>      > +    */
>      > +   GLuint LastClipDistanceArraySize;
>      > +
>      >     struct gl_uniform_block *UniformBlocks;
>      >     unsigned NumUniformBlocks;
>      >
>      > --
>      > 1.8.4.1
>      >
>      > _______________________________________________
>      > mesa-dev mailing list
>      > mesa-dev at lists.freedesktop.org
>      > http://lists.freedesktop.org/mailman/listinfo/mesa-dev