[Mesa-dev] [PATCH 1/2] main: Remove interface block array index for doing the name comparison

Samuel Iglesias Gonsálvez siglesias at igalia.com
Fri Oct 23 00:11:37 PDT 2015



On 22/10/15 13:06, Timothy Arceri wrote:
> On Thu, 2015-10-22 at 11:01 +0200, Samuel Iglesias Gonsalvez wrote:
>> From ARB_program_query_interface spec:
>>
>> "uint GetProgramResourceIndex(uint program, enum programInterface,
>>                                    const char *name);
>>  [...]
>>  If <name> exactly matches the name string of one of the active
>> resources
>>  for <programInterface>, the index of the matched resource is
>> returned.
>>  Additionally, if <name> would exactly match the name string of an
>> active
>>  resource if "[0]" were appended to <name>, the index of the matched
>>  resource is returned. [...]"
>>
>> "A string provided to GetProgramResourceLocation or
>>  GetProgramResourceLocationIndex is considered to match an active
>> variable
>>  if:
>> [...]
>>    * if the string identifies the base name of an active array, where
>> the
>>      string would exactly match the name of the variable if the
>> suffix
>>      "[0]" were appended to the string;
>> [...]
>> "
>>
>> But this is only happening on those ARB_program_query_interface's
>> queries.
>> For the rest of specs we need to keep old behavior. For that reason,
>> arb_program_interface_query boolean is added to the affected
>> functions.
>>
>> Fixes the following two dEQP-GLES31 tests:
>>
>> dEQP
>> -GLES31.functional.program_interface_query.shader_storage_block.resou
>> rce_list.block_array
>> dEQP
>> -GLES31.functional.program_interface_query.shader_storage_block.resou
>> rce_list.block_array_single_element
>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>> Cc: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>  src/mesa/main/program_resource.c |  6 ++---
>>  src/mesa/main/shader_query.cpp   | 58
>> +++++++++++++++++++++++++++++++---------
>>  src/mesa/main/shaderapi.c        |  4 +--
>>  src/mesa/main/shaderapi.h        |  9 ++++---
>>  src/mesa/main/uniforms.c         |  6 ++---
>>  5 files changed, 60 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/mesa/main/program_resource.c
>> b/src/mesa/main/program_resource.c
>> index eb71fdd..d029926 100644
>> --- a/src/mesa/main/program_resource.c
>> +++ b/src/mesa/main/program_resource.c
>> @@ -251,7 +251,7 @@ _mesa_GetProgramResourceIndex(GLuint program,
>> GLenum programInterface,
>>     case GL_UNIFORM_BLOCK:
>>     case GL_SHADER_STORAGE_BLOCK:
>>        res = _mesa_program_resource_find_name(shProg,
>> programInterface, name,
>> -                                             &array_index);
>> +                                             &array_index, true);
>>        if (!res || array_index > 0)
>>           return GL_INVALID_INDEX;
>>  
>> @@ -377,7 +377,7 @@ _mesa_GetProgramResourceLocation(GLuint program,
>> GLenum programInterface,
>>           goto fail;
>>     }
>>  
>> -   return _mesa_program_resource_location(shProg, programInterface,
>> name);
>> +   return _mesa_program_resource_location(shProg, programInterface,
>> name, true);
>>  fail:
>>     _mesa_error(ctx, GL_INVALID_ENUM,
>> "glGetProgramResourceLocation(%s %s)",
>>                 _mesa_enum_to_string(programInterface), name);
>> @@ -411,5 +411,5 @@ _mesa_GetProgramResourceLocationIndex(GLuint
>> program, GLenum programInterface,
>>     }
>>  
>>     return _mesa_program_resource_location_index(shProg,
>> GL_PROGRAM_OUTPUT,
>> -                                                name);
>> +                                                name, true);
>>  }
>> diff --git a/src/mesa/main/shader_query.cpp
>> b/src/mesa/main/shader_query.cpp
>> index 8182d3d..49766ca 100644
>> --- a/src/mesa/main/shader_query.cpp
>> +++ b/src/mesa/main/shader_query.cpp
>> @@ -218,7 +218,7 @@ _mesa_GetAttribLocation(GLhandleARB program,
>> const GLcharARB * name)
>>     unsigned array_index = 0;
>>     struct gl_program_resource *res =
>>        _mesa_program_resource_find_name(shProg, GL_PROGRAM_INPUT,
>> name,
>> -                                       &array_index);
>> +                                       &array_index, false);
>>  
>>     if (!res)
>>        return -1;
>> @@ -367,7 +367,7 @@ _mesa_GetFragDataIndex(GLuint program, const
>> GLchar *name)
>>        return -1;
>>  
>>     return _mesa_program_resource_location_index(shProg,
>> GL_PROGRAM_OUTPUT,
>> -                                                name);
>> +                                                name, false);
>>  }
>>  
>>  GLint GLAPIENTRY
>> @@ -404,7 +404,7 @@ _mesa_GetFragDataLocation(GLuint program, const
>> GLchar *name)
>>     unsigned array_index = 0;
>>     struct gl_program_resource *res =
>>        _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT,
>> name,
>> -                                       &array_index);
>> +                                       &array_index, false);
>>  
>>     if (!res)
>>        return -1;
>> @@ -533,9 +533,12 @@ valid_array_index(const GLchar *name, unsigned
>> *array_index)
>>  struct gl_program_resource *
>>  _mesa_program_resource_find_name(struct gl_shader_program *shProg,
>>                                   GLenum programInterface, const char
>> *name,
>> -                                 unsigned *array_index)
>> +                                 unsigned *array_index,
>> +                                 bool arb_program_interface_query)
>>  {
>>     struct gl_program_resource *res = shProg->ProgramResourceList;
>> +   const char *name_first_square_bracket = strchr(name, '[');
>> +
>>     for (unsigned i = 0; i < shProg->NumProgramResourceList; i++,
>> res++) {
>>        if (res->Type != programInterface)
>>           continue;
>> @@ -543,6 +546,33 @@ _mesa_program_resource_find_name(struct
>> gl_shader_program *shProg,
>>        /* Resource basename. */
>>        const char *rname = _mesa_program_resource_name(res);
>>        unsigned baselen = strlen(rname);
>> +      const char *rname_first_square_bracket = strchr(rname, '[');
>> +
>> +      /* From ARB_program_interface_query spec:
>> +       *
>> +       * "uint GetProgramResourceIndex(uint program, enum
>> programInterface,
>> +       *                               const char *name);
>> +       *  [...]
>> +       *  If <name> exactly matches the name string of one of the
>> active
>> +       *  resources for <programInterface>, the index of the matched
>> resource is
>> +       *  returned. Additionally, if <name> would exactly match the
>> name string
>> +       *  of an active resource if "[0]" were appended to <name>,
>> the index of
>> +       *  the matched resource is returned. [...]"
>> +       *
>> +       * "A string provided to GetProgramResourceLocation or
>> +       * GetProgramResourceLocationIndex is considered to match an
>> active variable
>> +       * if:
>> +       *
>> +       *  * the string exactly matches the name of the active
>> variable;
>> +       *
>> +       *  * if the string identifies the base name of an active
>> array, where the
>> +       *    string would exactly match the name of the variable if
>> the suffix
>> +       *    "[0]" were appended to the string; [...]"
>> +       */
>> +      /* Remove array's index from interface block name comparison
>> */
>> +      if (arb_program_interface_query &&
>> +          !name_first_square_bracket && rname_first_square_bracket)
>> +         baselen -= strlen(rname_first_square_bracket);
> 
> This patch doesn't look right to me for starters I don't think it will
> work for arrays of arrays as matching works like this
> 
> resource_name[2][3] or resource_name[2][3][0]
> 

Yes, fails for that case. I am working in a new version that works too
for that case. I forgot the AoA case :-)

> Also this looks like a problem only with shader storage blocks, for
> other types normaly _mesa_program_resource_name() returns the resource
> with the innermost array removed (it normally doesn't get appended in
> the first place, at least thats how uniforms work see the name building
> in link_uniforms.cpp).
> 

It is not shader storage block specific, that also happens for uniform
blocks (shader storage blocks use the same code). I have written an
example like:

uniform UBO {
  vec4 v;
} ubo[2];

"UBO[0]" and "UBO[1]" active variables are returned by
glGetProgramResourceName() because they are got from their respective
gl_program_resource's Name field.

Sam

> I don't have time to dip deeper right now but just a couple of things
> to consider.
> 
>>  
>>        if (strncmp(rname, name, baselen) == 0) {
>>           switch (programInterface) {
>> @@ -845,12 +875,14 @@ program_resource_location(struct
>> gl_shader_program *shProg,
>>   */
>>  GLint
>>  _mesa_program_resource_location(struct gl_shader_program *shProg,
>> -                                GLenum programInterface, const char
>> *name)
>> +                                GLenum programInterface, const char
>> *name,
>> +                                bool arb_program_interface_query)
>>  {
>>     unsigned array_index = 0;
>>     struct gl_program_resource *res =
>>        _mesa_program_resource_find_name(shProg, programInterface,
>> name,
>> -                                       &array_index);
>> +                                       &array_index,
>> +                                       arb_program_interface_query);
>>  
>>     /* Resource not found. */
>>     if (!res)
>> @@ -865,10 +897,12 @@ _mesa_program_resource_location(struct
>> gl_shader_program *shProg,
>>   */
>>  GLint
>>  _mesa_program_resource_location_index(struct gl_shader_program
>> *shProg,
>> -                                      GLenum programInterface, const
>> char *name)
>> +                                      GLenum programInterface, const
>> char *name,
>> +                                      bool
>> arb_program_interface_query)
>>  {
>>     struct gl_program_resource *res =
>> -      _mesa_program_resource_find_name(shProg, programInterface,
>> name, NULL);
>> +      _mesa_program_resource_find_name(shProg, programInterface,
>> name, NULL,
>> +                                       arb_program_interface_query);
>>  
>>     /* Non-existent variable or resource is not referenced by
>> fragment stage. */
>>     if (!res || !(res->StageReferences & (1 <<
>> MESA_SHADER_FRAGMENT)))
>> @@ -946,7 +980,7 @@ get_buffer_property(struct gl_shader_program
>> *shProg,
>>              const char *iname = RESOURCE_UBO(res)
>> ->Uniforms[i].IndexName;
>>              struct gl_program_resource *uni =
>>                 _mesa_program_resource_find_name(shProg, GL_UNIFORM,
>> iname,
>> -                                                NULL);
>> +                                                NULL, false);
>>              if (!uni)
>>                 continue;
>>              (*val)++;
>> @@ -958,7 +992,7 @@ get_buffer_property(struct gl_shader_program
>> *shProg,
>>              const char *iname = RESOURCE_UBO(res)
>> ->Uniforms[i].IndexName;
>>              struct gl_program_resource *uni =
>>                 _mesa_program_resource_find_name(shProg, GL_UNIFORM,
>> iname,
>> -                                                NULL);
>> +                                                NULL, false);
>>              if (!uni)
>>                 continue;
>>              *val++ =
>> @@ -982,7 +1016,7 @@ get_buffer_property(struct gl_shader_program
>> *shProg,
>>              const char *iname = RESOURCE_UBO(res)
>> ->Uniforms[i].IndexName;
>>              struct gl_program_resource *uni =
>>                 _mesa_program_resource_find_name(shProg,
>> GL_BUFFER_VARIABLE,
>> -                                                iname, NULL);
>> +                                                iname, NULL, false);
>>              if (!uni)
>>                 continue;
>>              (*val)++;
>> @@ -994,7 +1028,7 @@ get_buffer_property(struct gl_shader_program
>> *shProg,
>>              const char *iname = RESOURCE_UBO(res)
>> ->Uniforms[i].IndexName;
>>              struct gl_program_resource *uni =
>>                 _mesa_program_resource_find_name(shProg,
>> GL_BUFFER_VARIABLE,
>> -                                                iname, NULL);
>> +                                                iname, NULL, false);
>>              if (!uni)
>>                 continue;
>>              *val++ =
>> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>> index 765602e..00cefd0 100644
>> --- a/src/mesa/main/shaderapi.c
>> +++ b/src/mesa/main/shaderapi.c
>> @@ -2267,7 +2267,7 @@ _mesa_GetSubroutineUniformLocation(GLuint
>> program, GLenum shadertype,
>>     }
>>  
>>     resource_type = _mesa_shader_stage_to_subroutine_uniform(stage);
>> -   return _mesa_program_resource_location(shProg, resource_type,
>> name);
>> +   return _mesa_program_resource_location(shProg, resource_type,
>> name, false);
>>  }
>>  
>>  GLuint GLAPIENTRY
>> @@ -2302,7 +2302,7 @@ _mesa_GetSubroutineIndex(GLuint program, GLenum
>> shadertype,
>>     }
>>  
>>     resource_type = _mesa_shader_stage_to_subroutine(stage);
>> -   res = _mesa_program_resource_find_name(shProg, resource_type,
>> name, NULL);
>> +   res = _mesa_program_resource_find_name(shProg, resource_type,
>> name, NULL, false);
>>     if (!res) {
>>        _mesa_error(ctx, GL_INVALID_OPERATION, "%s", api_name);
>>       return -1;
>> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
>> index fba767b..2e6b9af 100644
>> --- a/src/mesa/main/shaderapi.h
>> +++ b/src/mesa/main/shaderapi.h
>> @@ -233,7 +233,8 @@ _mesa_program_resource_index(struct
>> gl_shader_program *shProg,
>>  extern struct gl_program_resource *
>>  _mesa_program_resource_find_name(struct gl_shader_program *shProg,
>>                                   GLenum programInterface, const char
>> *name,
>> -                                 unsigned *array_index);
>> +                                 unsigned *array_index,
>> +                                 bool arb_program_interface_query);
>>  
>>  extern struct gl_program_resource *
>>  _mesa_program_resource_find_index(struct gl_shader_program *shProg,
>> @@ -250,11 +251,13 @@ _mesa_program_resource_name_len(struct
>> gl_program_resource *res);
>>  
>>  extern GLint
>>  _mesa_program_resource_location(struct gl_shader_program *shProg,
>> -                                GLenum programInterface, const char
>> *name);
>> +                                GLenum programInterface, const char
>> *name,
>> +                                bool arb_program_interface_query);
>>  
>>  extern GLint
>>  _mesa_program_resource_location_index(struct gl_shader_program
>> *shProg,
>> -                                      GLenum programInterface, const
>> char *name);
>> +                                      GLenum programInterface, const
>> char *name,
>> +                                      bool
>> arb_program_interface_query);
>>  
>>  extern unsigned
>>  _mesa_program_resource_prop(struct gl_shader_program *shProg,
>> diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
>> index bc23538..d240183 100644
>> --- a/src/mesa/main/uniforms.c
>> +++ b/src/mesa/main/uniforms.c
>> @@ -921,7 +921,7 @@ _mesa_GetUniformLocation(GLuint programObj, const
>> GLcharARB *name)
>>        return -1;
>>     }
>>  
>> -   return _mesa_program_resource_location(shProg, GL_UNIFORM, name);
>> +   return _mesa_program_resource_location(shProg, GL_UNIFORM, name,
>> false);
>>  }
>>  
>>  GLuint GLAPIENTRY
>> @@ -943,7 +943,7 @@ _mesa_GetUniformBlockIndex(GLuint program,
>>  
>>     struct gl_program_resource *res =
>>        _mesa_program_resource_find_name(shProg, GL_UNIFORM_BLOCK,
>> -                                       uniformBlockName, NULL);
>> +                                       uniformBlockName, NULL,
>> false);
>>     if (!res)
>>        return GL_INVALID_INDEX;
>>  
>> @@ -979,7 +979,7 @@ _mesa_GetUniformIndices(GLuint program,
>>     for (i = 0; i < uniformCount; i++) {
>>        struct gl_program_resource *res =
>>           _mesa_program_resource_find_name(shProg, GL_UNIFORM,
>> uniformNames[i],
>> -                                          NULL);
>> +                                          NULL, false);
>>        uniformIndices[i] = _mesa_program_resource_index(shProg, res);
>>     }
>>  }
> 


More information about the mesa-dev mailing list