[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:24:31 PDT 2015
On 22/10/15 13:19, Samuel Iglesias Gonsálvez wrote:
>
>
> On 22/10/15 13:08, Tapani Pälli wrote:
>> On 10/22/2015 12:01 PM, 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.
>>
>> This seems strange as the extension spec (and GL core spec) has set of
>> different examples that state the new queries to match behavior of old
>> functions (using words "are equivalent to calling"), this is why our
>> tests also cross-check that new and old functions return same values.
>> I'll need to have a look at tests below.
>>
>> As example extension spec says GetFragDataIndex to be equivalent to
>> calling GetProgramResourceLocationIndex(program, PROGRAM_OUTPUT, name).
>>
>
> OK, I am going to review that. Thanks for sharing this information.
OK, you are right, ARB_program_interface_query says that
GetFragDataIndex, GetUniformLocation,
GetActiveUniformName and others are equivalent to calling the respective
functions from ARB_program_interface_query API.
That was my original patch did, but I modified it to have this version
because a regression in piglit:
bin/arb_shader_objects-getuniformlocation-array-of-struct-of-array
It complains because it defines a shader like:
struct S { mat4 m; vec4 v[10]; };
uniform S s[10];
[...]
And the application does the following call:
/* From page 80 of the OpenGL 2.1 spec:
*
* "A valid name cannot be a structure, an array of structures, or
* any portion of a single vector or a matrix."
*/
loc = glGetUniformLocation(prog, "s")
if (loc != -1) {
printf("s location = %d (should be -1)\n", loc);
pass = false;
}
If GetUniformLocation is equivalent to call to
GetProgramResourceLocation(program, UNIFORM, name), we return a location
value different than -1 for "s" case (as we will add "[0]" and compare
it to s[0] internally).
I ran the same test on NVIDIA proprietary OpenGL 4.40 driver and there
is no such regression: it returns -1 which is the expected value.
If we agree that Mesa should apply the name + "[0]" rule for these cases
too, I will update the patch with that change. Maybe we can do it only
if ARB_program_interface_query extension is supported.
What do you think Mesa should behave in this case?
Sam
>
>>> Fixes the following two dEQP-GLES31 tests:
>>>
>>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_list.block_array
>>>
>>> dEQP-GLES31.functional.program_interface_query.shader_storage_block.resource_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);
>>> 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