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

Alejandro Piñeiro apinheiro at igalia.com
Tue Nov 29 11:35:11 UTC 2016


On 29/11/16 02:38, Timothy Arceri wrote:
> 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 :)

Ah yes, that was true, that patch was really long.

>
>>> 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.

Ok.

>
>>>
>>> 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.

To you for simplifying the code.

>
>> 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-stable mailing list