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

Tapani Pälli tapani.palli at intel.com
Mon Nov 28 12:51:07 UTC 2016



On 11/28/2016 12:38 PM, 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.
>
>> 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?
>
>>
>> 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).

FYI I run CI for this, no regressions. Change looks good to me;

Reviewed-by: Tapani Pälli <tapani.palli at intel.com>


> So:
> Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>
>
> 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;
>
>


More information about the mesa-stable mailing list