[Mesa-dev] [PATCH] mesa: fix active subroutine uniforms properly

Timothy Arceri timothy.arceri at collabora.com
Tue Nov 29 04:38:26 UTC 2016


On Mon, 2016-11-28 at 08:38 -0200, Alejandro Piñeiro wrote:
> On 26/11/16 21:31, Timothy Arceri wrote:
> > 
> > 07fe2d565b introduced a big hack in order to return
> > NumSubroutineUniforms when querying ACTIVE_RESOURCES for
> > <shader>_SUBROUTINE_UNIFORM interfaces. 
> 
> I wouldn't call that a big hack, but obviously I'm biased.

Big was intended to describe the amount of code required as opposed to
being an emphasized criticism :)

> 
> > 
> > However this is the
> > wrong fix we are meant to be returning the number of active
> > resources i.e. the count of subroutine uniforms in the
> > resource list which is what the code was previously doing,
> > anything else will cause trouble when trying to retrieve
> > the resource properties based on the ACTIVE_RESOURCES count.
> 
> Just out of curiosity, does this patch fix any piglit/cts/<whatever>
> test?

No I just noticed the code while pass by. A test could certainly be
written to detect the problem but I decided just to fix it and move on
on this occasion. Feel free to come up with one if you like.

> 
> > 
> > 
> > The real problem is that NumSubroutineUniforms was counting
> > array elements as separate uniforms but the innermost array
> > is always considered a single uniform so we fix that count
> > instead which was counted incorrectly in 7fa0250f9.
> 
> Ok, makes sense. And the patch gets everything simpler and with less
> lines of code. Just in case, I have just checked and the piglit test
> I
> created then
> (arb_program_interface_query-compare-with-shader-subroutine) and the
> cts
> test that was failing
> (GL45-CTS.program_interface_query.subroutines-compute).
> 
> So:
> Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>

Thanks.

> 
> Thanks.
> 
> > 
> > 
> > Idealy we could probably completely remove
> > NumSubroutineUniforms and just compute its value when needed
> > from the resource list but this works for now.
> > 
> > Cc: Alejandro Piñeiro <apinheiro at igalia.com>
> > Cc: Tapani Pälli <tapani.palli at intel.com>
> > Cc: Dave Airlie <airlied at gmail.com>
> > Cc: 13.0 <mesa-stable at lists.freedesktop.org>
> > ---
> >  src/compiler/glsl/link_uniforms.cpp |   2 +
> >  src/compiler/glsl/linker.cpp        |   1 -
> >  src/mesa/main/program_resource.c    | 111 +++---------------------
> > ------------
> >  3 files changed, 10 insertions(+), 104 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_uniforms.cpp
> > b/src/compiler/glsl/link_uniforms.cpp
> > index f271093..66bcbda 100644
> > --- a/src/compiler/glsl/link_uniforms.cpp
> > +++ b/src/compiler/glsl/link_uniforms.cpp
> > @@ -623,6 +623,8 @@ private:
> >           uniform->opaque[shader_type].index = this-
> > >next_subroutine;
> >           uniform->opaque[shader_type].active = true;
> >  
> > +         prog->_LinkedShaders[shader_type]-
> > >NumSubroutineUniforms++;
> > +
> >           /* Increment the subroutine index by 1 for non-arrays and
> > by the
> >            * number of array elements for arrays.
> >            */
> > diff --git a/src/compiler/glsl/linker.cpp
> > b/src/compiler/glsl/linker.cpp
> > index 1a00a90..6f54f75 100644
> > --- a/src/compiler/glsl/linker.cpp
> > +++ b/src/compiler/glsl/linker.cpp
> > @@ -3159,7 +3159,6 @@ link_calculate_subroutine_compat(struct
> > gl_shader_program *prog)
> >           if (!uni)
> >              continue;
> >  
> > -         sh->NumSubroutineUniforms++;
> >           count = 0;
> >           if (sh->NumSubroutineFunctions == 0) {
> >              linker_error(prog, "subroutine uniform %s defined but
> > no valid functions found\n", uni->type->name);
> > diff --git a/src/mesa/main/program_resource.c
> > b/src/mesa/main/program_resource.c
> > index 859bda2..5461c4e 100644
> > --- a/src/mesa/main/program_resource.c
> > +++ b/src/mesa/main/program_resource.c
> > @@ -67,9 +67,7 @@ supported_interface_enum(struct gl_context *ctx,
> > GLenum iface)
> >  }
> >  
> >  static struct gl_shader_program *
> > -lookup_linked_program(GLuint program,
> > -                      const char *caller,
> > -                      bool raise_link_error)
> > +lookup_linked_program(GLuint program, const char *caller)
> >  {
> >     GET_CURRENT_CONTEXT(ctx);
> >     struct gl_shader_program *prog =
> > @@ -79,66 +77,13 @@ lookup_linked_program(GLuint program,
> >        return NULL;
> >  
> >     if (prog->data->LinkStatus == GL_FALSE) {
> > -      if (raise_link_error)
> > -         _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not
> > linked)",
> > -                     caller);
> > +      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not
> > linked)",
> > +                  caller);
> >        return NULL;
> >     }
> >     return prog;
> >  }
> >  
> > -static GLenum
> > -stage_from_program_interface(GLenum programInterface)
> > -{
> > -   switch(programInterface) {
> > -   case GL_VERTEX_SUBROUTINE_UNIFORM:
> > -      return MESA_SHADER_VERTEX;
> > -   case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
> > -      return MESA_SHADER_TESS_CTRL;
> > -   case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
> > -      return MESA_SHADER_TESS_EVAL;
> > -   case GL_GEOMETRY_SUBROUTINE_UNIFORM:
> > -      return MESA_SHADER_GEOMETRY;
> > -   case GL_FRAGMENT_SUBROUTINE_UNIFORM:
> > -      return MESA_SHADER_FRAGMENT;
> > -   case GL_COMPUTE_SUBROUTINE_UNIFORM:
> > -      return MESA_SHADER_COMPUTE;
> > -   default:
> > -      unreachable("unexpected programInterface value");
> > -   }
> > -}
> > -
> > -static struct gl_linked_shader *
> > -lookup_linked_shader(GLuint program,
> > -                     GLenum programInterface,
> > -                     const char *caller)
> > -{
> > -   struct gl_shader_program *shLinkedProg =
> > -      lookup_linked_program(program, caller, false);
> > -   gl_shader_stage stage =
> > stage_from_program_interface(programInterface);
> > -
> > -   if (!shLinkedProg)
> > -      return NULL;
> > -
> > -   return shLinkedProg->_LinkedShaders[stage];
> > -}
> > -
> > -static bool
> > -is_subroutine_uniform_program_interface(GLenum programInterface)
> > -{
> > -   switch(programInterface) {
> > -   case GL_VERTEX_SUBROUTINE_UNIFORM:
> > -   case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
> > -   case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
> > -   case GL_GEOMETRY_SUBROUTINE_UNIFORM:
> > -   case GL_FRAGMENT_SUBROUTINE_UNIFORM:
> > -   case GL_COMPUTE_SUBROUTINE_UNIFORM:
> > -      return true;
> > -   default:
> > -      return false;
> > -   }
> > -}
> > -
> >  void GLAPIENTRY
> >  _mesa_GetProgramInterfaceiv(GLuint program, GLenum
> > programInterface,
> >                              GLenum pname, GLint *params)
> > @@ -174,49 +119,9 @@ _mesa_GetProgramInterfaceiv(GLuint program,
> > GLenum programInterface,
> >     /* Validate pname against interface. */
> >     switch(pname) {
> >     case GL_ACTIVE_RESOURCES:
> > -      if
> > (is_subroutine_uniform_program_interface(programInterface)) {
> > -         /* ARB_program_interface_query doesn't explicitly says
> > that those
> > -          * uniforms would need a linked shader, or that should
> > fail if it is
> > -          * not the case, but Section 7.6 (Uniform Variables) of
> > the OpenGL
> > -          * 4.4 Core Profile says:
> > -          *
> > -          *    "A uniform is considered an active uniform if the
> > compiler and
> > -          *     linker determine that the uniform will actually be
> > accessed
> > -          *     when the executable code is executed. In cases
> > where the
> > -          *     compiler and linker cannot make a conclusive
> > determination,
> > -          *     the uniform will be considered active."
> > -          *
> > -          * So in order to know the real number of active
> > subroutine uniforms
> > -          * we would need a linked shader .
> > -          *
> > -          * At the same time, Section 7.3 (Program Objects) of the
> > OpenGL 4.4
> > -          * Core Profile says:
> > -          *
> > -          *    "The GL provides various commands allowing
> > applications to
> > -          *     enumerate and query properties of active variables
> > and in-
> > -          *     terface blocks for a specified program. If one of
> > these
> > -          *     commands is called with a program for which
> > LinkProgram
> > -          *     succeeded, the information recorded when the
> > program was
> > -          *     linked is returned. If one of these commands is
> > called with a
> > -          *     program for which LinkProgram failed, no error is
> > generated
> > -          *     unless otherwise noted."
> > -          *     <skip>
> > -          *    "If one of these commands is called with a program
> > for which
> > -          *     LinkProgram had never been called, no error is
> > generated
> > -          *     unless otherwise noted, and the program object is
> > considered
> > -          *     to have no active variables or interface blocks."
> > -          *
> > -          * So if the program is not linked we will return 0.
> > -          */
> > -         struct gl_linked_shader *sh =
> > -            lookup_linked_shader(program, programInterface,
> > "glGetProgramInterfaceiv");
> > -
> > -         *params = sh ? sh->NumSubroutineUniforms : 0;
> > -      } else {
> > -         for (i = 0, *params = 0; i < shProg-
> > >NumProgramResourceList; i++)
> > -            if (shProg->ProgramResourceList[i].Type ==
> > programInterface)
> > -               (*params)++;
> > -      }
> > +      for (i = 0, *params = 0; i < shProg->NumProgramResourceList; 
> > i++)
> > +         if (shProg->ProgramResourceList[i].Type ==
> > programInterface)
> > +            (*params)++;
> >        break;
> >     case GL_MAX_NAME_LENGTH:
> >        if (programInterface == GL_ATOMIC_COUNTER_BUFFER ||
> > @@ -500,7 +405,7 @@ _mesa_GetProgramResourceLocation(GLuint
> > program, GLenum programInterface,
> >     }
> >  
> >     struct gl_shader_program *shProg =
> > -      lookup_linked_program(program,
> > "glGetProgramResourceLocation", true);
> > +      lookup_linked_program(program,
> > "glGetProgramResourceLocation");
> >  
> >     if (!shProg || !name)
> >        return -1;
> > @@ -556,7 +461,7 @@ _mesa_GetProgramResourceLocationIndex(GLuint
> > program, GLenum programInterface,
> >     }
> >  
> >     struct gl_shader_program *shProg =
> > -      lookup_linked_program(program,
> > "glGetProgramResourceLocationIndex", true);
> > +      lookup_linked_program(program,
> > "glGetProgramResourceLocationIndex");
> >  
> >     if (!shProg || !name)
> >        return -1;
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list