[Mesa-dev] [PATCH 01/20] mesa: fix and simplify resource query for arrays
Tapani Pälli
tapani.palli at intel.com
Wed Jul 29 22:56:18 PDT 2015
On 07/30/2015 08:50 AM, Timothy Arceri wrote:
>
> On 30 July 2015 3:27:41 pm AEST, "Tapani Pälli" <tapani.palli at intel.com> wrote:
>> On 07/30/2015 08:25 AM, Tapani Pälli wrote:
>>> Hi Timothy;
>>>
>>> Would it be OK for you to wait a bit with these 3 first patches? I'm
>>> currently going through failing PIQ tests and I have a 1 line fix to
>>> valid_program_resource_index_name() that together with your patch
>>> "glsl: set stage flag for structs and arrays in resource list" will
>>> fix all of the failing "ES31-CTS.program_interface_query.uniform"
>>> tests. Frankly I don't see the value of such a big refactor necessary
>>> for this.
> The value is in removing almost 200 lines of complicated code trying to solve the same problem in different ways. How its this not a good thing?
>
> Also once this is done we can also work towards removing UniformHash from Mesa.
Sure, I agree that removing lots of code is a good thing. I just don't
quite yet understand how some of this will work, many of the removed
checks were there for a good reason (faling tests!). Just to make sure,
did you run Piglit and conformance without regressions? Anyway, I will
read through your changes and think about this more.
>
>
>
>> Woops, make it "ES31-CTS.program_interface_query.uniform-types", not
>> "ES31-CTS.program_interface_query.uniform". It looks to me that most
>> tests fail just because of not figuring out the correct index for the
>> resource and that can be fixed by a valid_program_resource_index_name
>> change.
>>
>>
>>> On 07/29/2015 04:56 PM, Timothy Arceri wrote:
>>>> This removes the need for multiple functions designed to validate an
>>>> array
>>>> subscript and replaces them with a call to a single function.
>>>>
>>>> The change also means that validation is now only done once and the
>>>> index
>>>> is retrived at the same time, as a result the getUniformLocation
>> code
>>>> can
>>>> be simplified saving an extra hash table lookup (and yet another
>>>> validation call).
>>>>
>>>> This chage also fixes some tests in:
>>>> ES31-CTS.program_interface_query.uniform
>>>>
>>>> V3: rebase on subroutines, and move the resource index array == 0
>>>> check into _mesa_GetProgramResourceIndex() to simplify things
>> further
>>>> V2: Fix bounds checks for program input/output, split unrelated
>>>> comment fix
>>>> and _mesa_get_uniform_location() removal into their own patch.
>>>>
>>>> Cc: Tapani Pälli <tapani.palli at intel.com>
>>>> ---
>>>> src/mesa/main/program_resource.c | 14 ++--
>>>> src/mesa/main/shader_query.cpp | 174
>>>> +++++++++++++++++++++------------------
>>>> src/mesa/main/shaderapi.c | 2 +-
>>>> src/mesa/main/shaderapi.h | 3 +-
>>>> src/mesa/main/uniforms.c | 5 +-
>>>> 5 files changed, 106 insertions(+), 92 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/program_resource.c
>>>> b/src/mesa/main/program_resource.c
>>>> index 641ef22..fdbd5b3 100644
>>>> --- a/src/mesa/main/program_resource.c
>>>> +++ b/src/mesa/main/program_resource.c
>>>> @@ -229,6 +229,7 @@ _mesa_GetProgramResourceIndex(GLuint program,
>>>> GLenum programInterface,
>>>> const GLchar *name)
>>>> {
>>>> GET_CURRENT_CONTEXT(ctx);
>>>> + unsigned array_index = 0;
>>>> struct gl_program_resource *res;
>>>> struct gl_shader_program *shProg =
>>>> _mesa_lookup_shader_program_err(ctx, program,
>>>> @@ -268,13 +269,10 @@ _mesa_GetProgramResourceIndex(GLuint program,
>>>> GLenum programInterface,
>>>> case GL_PROGRAM_OUTPUT:
>>>> case GL_UNIFORM:
>>>> case GL_TRANSFORM_FEEDBACK_VARYING:
>>>> - /* Validate name syntax for array variables */
>>>> - if (!valid_program_resource_index_name(name))
>>>> - return GL_INVALID_INDEX;
>>>> - /* fall-through */
>>>> case GL_UNIFORM_BLOCK:
>>>> - res = _mesa_program_resource_find_name(shProg,
>>>> programInterface, name);
>>>> - if (!res)
>>>> + res = _mesa_program_resource_find_name(shProg,
>>>> programInterface, name,
>>>> + &array_index);
>>>> + if (!res || array_index > 0)
>>>> return GL_INVALID_INDEX;
>>>> return _mesa_program_resource_index(shProg, res);
>>>> @@ -403,7 +401,7 @@ _mesa_GetProgramResourceLocation(GLuint program,
>>>> GLenum programInterface,
>>>> struct gl_shader_program *shProg =
>>>> lookup_linked_program(program,
>> "glGetProgramResourceLocation");
>>>> - if (!shProg || !name || invalid_array_element_syntax(name))
>>>> + if (!shProg || !name)
>>>> return -1;
>>>> /* Validate programInterface. */
>>>> @@ -453,7 +451,7 @@ _mesa_GetProgramResourceLocationIndex(GLuint
>>>> program, GLenum programInterface,
>>>> struct gl_shader_program *shProg =
>>>> lookup_linked_program(program,
>>>> "glGetProgramResourceLocationIndex");
>>>> - if (!shProg || !name || invalid_array_element_syntax(name))
>>>> + if (!shProg || !name)
>>>> return -1;
>>>> /* From the GL_ARB_program_interface_query spec:
>>>> diff --git a/src/mesa/main/shader_query.cpp
>>>> b/src/mesa/main/shader_query.cpp
>>>> index 3e52a09..dd11b81 100644
>>>> --- a/src/mesa/main/shader_query.cpp
>>>> +++ b/src/mesa/main/shader_query.cpp
>>>> @@ -44,7 +44,8 @@ extern "C" {
>>>> static GLint
>>>> program_resource_location(struct gl_shader_program *shProg,
>>>> - struct gl_program_resource *res, const
>>>> char *name);
>>>> + struct gl_program_resource *res, const
>>>> char *name,
>>>> + unsigned array_index);
>>>> /**
>>>> * Declare convenience functions to return resource data in a
>> given
>>>> type.
>>>> @@ -272,13 +273,15 @@ _mesa_GetAttribLocation(GLhandleARB program,
>>>> const GLcharARB * name)
>>>> if (shProg->_LinkedShaders[MESA_SHADER_VERTEX] == NULL)
>>>> return -1;
>>>> + unsigned array_index = 0;
>>>> struct gl_program_resource *res =
>>>> - _mesa_program_resource_find_name(shProg, GL_PROGRAM_INPUT,
>> name);
>>>> + _mesa_program_resource_find_name(shProg, GL_PROGRAM_INPUT,
>> name,
>>>> + &array_index);
>>>> if (!res)
>>>> return -1;
>>>> - GLint loc = program_resource_location(shProg, res, name);
>>>> + GLint loc = program_resource_location(shProg, res, name,
>>>> array_index);
>>>> /* The extra check against against 0 is made because of
>>>> builtin-attribute
>>>> * locations that have offset applied. Function
>>>> program_resource_location
>>>> @@ -456,13 +459,15 @@ _mesa_GetFragDataLocation(GLuint program,
>> const
>>>> GLchar *name)
>>>> if (shProg->_LinkedShaders[MESA_SHADER_FRAGMENT] == NULL)
>>>> return -1;
>>>> + unsigned array_index = 0;
>>>> struct gl_program_resource *res =
>>>> - _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT,
>>>> name);
>>>> + _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT,
>> name,
>>>> + &array_index);
>>>> if (!res)
>>>> return -1;
>>>> - GLint loc = program_resource_location(shProg, res, name);
>>>> + GLint loc = program_resource_location(shProg, res, name,
>>>> array_index);
>>>> /* The extra check against against 0 is made because of
>>>> builtin-attribute
>>>> * locations that have offset applied. Function
>>>> program_resource_location
>>>> @@ -552,39 +557,32 @@ _mesa_program_resource_array_size(struct
>>>> gl_program_resource *res)
>>>> return 0;
>>>> }
>>>> -static int
>>>> -array_index_of_resource(struct gl_program_resource *res,
>>>> - const char *name)
>>>> +/**
>>>> + * Checks if array subscript is valid and if so sets array_index.
>>>> + */
>>>> +static bool
>>>> +valid_array_index(const GLchar *name, unsigned *array_index)
>>>> {
>>>> - assert(res->Data);
>>>> + unsigned idx = 0;
>>>> + const GLchar *out_base_name_end;
>>>> - switch (res->Type) {
>>>> - case GL_PROGRAM_INPUT:
>>>> - case GL_PROGRAM_OUTPUT:
>>>> - return get_matching_index(RESOURCE_VAR(res), name);
>>>> - default:
>>>> - assert(!"support for resource type not implemented");
>>>> - return -1;
>>>> - }
>>>> + idx = parse_program_resource_name(name, &out_base_name_end);
>>>> + if (idx < 0)
>>>> + return false;
>>>> +
>>>> + if (array_index)
>>>> + *array_index = idx;
>>>> +
>>>> + return true;
>>>> }
>>>> /* Find a program resource with specific name in given
>> interface.
>>>> */
>>>> struct gl_program_resource *
>>>> _mesa_program_resource_find_name(struct gl_shader_program *shProg,
>>>> - GLenum programInterface, const
>> char
>>>> *name)
>>>> + GLenum programInterface, const
>> char
>>>> *name,
>>>> + unsigned *array_index)
>>>> {
>>>> - GET_CURRENT_CONTEXT(ctx);
>>>> - const char *full_name = name;
>>>> -
>>>> - /* When context has 'VertexID_is_zero_based' set, gl_VertexID
>> has
>>>> been
>>>> - * lowered to gl_VertexIDMESA.
>>>> - */
>>>> - if (name && ctx->Const.VertexID_is_zero_based) {
>>>> - if (strcmp(name, "gl_VertexID") == 0)
>>>> - full_name = "gl_VertexIDMESA";
>>>> - }
>>>> -
>>>> struct gl_program_resource *res = shProg->ProgramResourceList;
>>>> for (unsigned i = 0; i < shProg->NumProgramResourceList; i++,
>>>> res++) {
>>>> if (res->Type != programInterface)
>>>> @@ -594,38 +592,46 @@ _mesa_program_resource_find_name(struct
>>>> gl_shader_program *shProg,
>>>> const char *rname = _mesa_program_resource_name(res);
>>>> unsigned baselen = strlen(rname);
>>>> - switch (programInterface) {
>>>> - case GL_TRANSFORM_FEEDBACK_VARYING:
>>>> - case GL_UNIFORM_BLOCK:
>>>> - case GL_UNIFORM:
>>>> - case GL_VERTEX_SUBROUTINE_UNIFORM:
>>>> - case GL_GEOMETRY_SUBROUTINE_UNIFORM:
>>>> - case GL_FRAGMENT_SUBROUTINE_UNIFORM:
>>>> - case GL_COMPUTE_SUBROUTINE_UNIFORM:
>>>> - case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
>>>> - case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
>>>> - case GL_VERTEX_SUBROUTINE:
>>>> - case GL_GEOMETRY_SUBROUTINE:
>>>> - case GL_FRAGMENT_SUBROUTINE:
>>>> - case GL_COMPUTE_SUBROUTINE:
>>>> - case GL_TESS_CONTROL_SUBROUTINE:
>>>> - case GL_TESS_EVALUATION_SUBROUTINE:
>>>> - if (strncmp(rname, name, baselen) == 0) {
>>>> + if (strncmp(rname, name, baselen) == 0) {
>>>> + switch (programInterface) {
>>>> + case GL_UNIFORM_BLOCK:
>>>> /* Basename match, check if array or struct. */
>>>> if (name[baselen] == '\0' ||
>>>> name[baselen] == '[' ||
>>>> name[baselen] == '.') {
>>>> return res;
>>>> }
>>>> + break;
>>>> + case GL_TRANSFORM_FEEDBACK_VARYING:
>>>> + case GL_UNIFORM:
>>>> + case GL_VERTEX_SUBROUTINE_UNIFORM:
>>>> + case GL_GEOMETRY_SUBROUTINE_UNIFORM:
>>>> + case GL_FRAGMENT_SUBROUTINE_UNIFORM:
>>>> + case GL_COMPUTE_SUBROUTINE_UNIFORM:
>>>> + case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
>>>> + case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
>>>> + case GL_VERTEX_SUBROUTINE:
>>>> + case GL_GEOMETRY_SUBROUTINE:
>>>> + case GL_FRAGMENT_SUBROUTINE:
>>>> + case GL_COMPUTE_SUBROUTINE:
>>>> + case GL_TESS_CONTROL_SUBROUTINE:
>>>> + case GL_TESS_EVALUATION_SUBROUTINE:
>>>> + if (name[baselen] == '.') {
>>>> + return res;
>>>> + }
>>>> + /* fall-through */
>>>> + case GL_PROGRAM_INPUT:
>>>> + case GL_PROGRAM_OUTPUT:
>>>> + if (name[baselen] == '\0') {
>>>> + return res;
>>>> + } else if (name[baselen] == '[' &&
>>>> + valid_array_index(name, array_index)) {
>>>> + return res;
>>>> + }
>>>> + break;
>>>> + default:
>>>> + assert(!"not implemented for given interface");
>>>> }
>>>> - break;
>>>> - case GL_PROGRAM_INPUT:
>>>> - case GL_PROGRAM_OUTPUT:
>>>> - if (array_index_of_resource(res, full_name) >= 0)
>>>> - return res;
>>>> - break;
>>>> - default:
>>>> - assert(!"not implemented for given interface");
>>>> }
>>>> }
>>>> return NULL;
>>>> @@ -787,19 +793,9 @@ _mesa_get_program_resource_name(struct
>>>> gl_shader_program *shProg,
>>>> static GLint
>>>> program_resource_location(struct gl_shader_program *shProg,
>>>> - struct gl_program_resource *res, const
>>>> char *name)
>>>> + struct gl_program_resource *res, const
>>>> char *name,
>>>> + unsigned array_index)
>>>> {
>>>> - unsigned index, offset;
>>>> - int array_index = -1;
>>>> - long offset_ret;
>>>> - const GLchar *base_name_end;
>>>> -
>>>> - if (res->Type == GL_PROGRAM_INPUT || res->Type ==
>>>> GL_PROGRAM_OUTPUT) {
>>>> - array_index = array_index_of_resource(res, name);
>>>> - if (array_index < 0)
>>>> - return -1;
>>>> - }
>>>> -
>>>> /* Built-in locations should report GL_INVALID_INDEX. */
>>>> if (is_gl_identifier(name))
>>>> return GL_INVALID_INDEX;
>>>> @@ -810,13 +806,22 @@ program_resource_location(struct
>>>> gl_shader_program *shProg,
>>>> */
>>>> switch (res->Type) {
>>>> case GL_PROGRAM_INPUT:
>>>> + /* If the input is an array, fail if the index is out of
>>>> bounds. */
>>>> + if (array_index > 0
>>>> + && array_index >= RESOURCE_VAR(res)->type->length) {
>>>> + return -1;
>>>> + }
>>>> return RESOURCE_VAR(res)->data.location + array_index -
>>>> VERT_ATTRIB_GENERIC0;
>>>> case GL_PROGRAM_OUTPUT:
>>>> + /* If the output is an array, fail if the index is out of
>>>> bounds. */
>>>> + if (array_index > 0
>>>> + && array_index >= RESOURCE_VAR(res)->type->length) {
>>>> + return -1;
>>>> + }
>>>> return RESOURCE_VAR(res)->data.location + array_index -
>>>> FRAG_RESULT_DATA0;
>>>> case GL_UNIFORM:
>>>> - index = _mesa_get_uniform_location(shProg, name, &offset);
>>>> -
>>>> - if (index == GL_INVALID_INDEX)
>>>> + /* If the uniform is built-in, fail. */
>>>> + if (RESOURCE_UNI(res)->builtin)
>>>> return -1;
>>>> /* From the GL_ARB_uniform_buffer_object spec:
>>>> @@ -830,17 +835,21 @@ program_resource_location(struct
>>>> gl_shader_program *shProg,
>>>> RESOURCE_UNI(res)->atomic_buffer_index != -1)
>>>> return -1;
>>>> - /* location in remap table + array element offset */
>>>> - return RESOURCE_UNI(res)->remap_location + offset;
>>>> -
>>>> + /* fallthrough */
>>>> case GL_VERTEX_SUBROUTINE_UNIFORM:
>>>> case GL_GEOMETRY_SUBROUTINE_UNIFORM:
>>>> case GL_FRAGMENT_SUBROUTINE_UNIFORM:
>>>> case GL_COMPUTE_SUBROUTINE_UNIFORM:
>>>> case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
>>>> case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
>>>> - offset_ret = parse_program_resource_name(name,
>> &base_name_end);
>>>> - return RESOURCE_UNI(res)->remap_location + ((offset_ret !=
>> -1)
>>>> ? offset_ret : 0);
>>>> + /* If the uniform is an array, fail if the index is out of
>>>> bounds. */
>>>> + if (array_index > 0
>>>> + && array_index >= RESOURCE_UNI(res)->array_elements) {
>>>> + return -1;
>>>> + }
>>>> +
>>>> + /* location in remap table + array element offset */
>>>> + return RESOURCE_UNI(res)->remap_location + array_index;
>>>> default:
>>>> return -1;
>>>> }
>>>> @@ -854,14 +863,16 @@ GLint
>>>> _mesa_program_resource_location(struct gl_shader_program *shProg,
>>>> GLenum programInterface, const
>> char
>>>> *name)
>>>> {
>>>> + unsigned array_index = 0;
>>>> struct gl_program_resource *res =
>>>> - _mesa_program_resource_find_name(shProg, programInterface,
>> name);
>>>> + _mesa_program_resource_find_name(shProg, programInterface,
>> name,
>>>> + &array_index);
>>>> /* Resource not found. */
>>>> if (!res)
>>>> return -1;
>>>> - return program_resource_location(shProg, res, name);
>>>> + return program_resource_location(shProg, res, name,
>> array_index);
>>>> }
>>>> /**
>>>> @@ -873,7 +884,7 @@ _mesa_program_resource_location_index(struct
>>>> gl_shader_program *shProg,
>>>> GLenum programInterface,
>>>> const char *name)
>>>> {
>>>> struct gl_program_resource *res =
>>>> - _mesa_program_resource_find_name(shProg, programInterface,
>> name);
>>>> + _mesa_program_resource_find_name(shProg, programInterface,
>>>> name, NULL);
>>>> /* Non-existent variable or resource is not referenced by
>>>> fragment stage. */
>>>> if (!res || !(res->StageReferences & (1 <<
>> MESA_SHADER_FRAGMENT)))
>>>> @@ -949,7 +960,8 @@ get_buffer_property(struct gl_shader_program
>>>> *shProg,
>>>> for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms;
>>>> i++) {
>>>> const char *iname =
>>>> RESOURCE_UBO(res)->Uniforms[i].IndexName;
>>>> struct gl_program_resource *uni =
>>>> - _mesa_program_resource_find_name(shProg, GL_UNIFORM,
>>>> iname);
>>>> + _mesa_program_resource_find_name(shProg, GL_UNIFORM,
>>>> iname,
>>>> + NULL);
>>>> if (!uni)
>>>> continue;
>>>> (*val)++;
>>>> @@ -959,7 +971,8 @@ get_buffer_property(struct gl_shader_program
>>>> *shProg,
>>>> for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms;
>>>> i++) {
>>>> const char *iname =
>>>> RESOURCE_UBO(res)->Uniforms[i].IndexName;
>>>> struct gl_program_resource *uni =
>>>> - _mesa_program_resource_find_name(shProg, GL_UNIFORM,
>>>> iname);
>>>> + _mesa_program_resource_find_name(shProg, GL_UNIFORM,
>>>> iname,
>>>> + NULL);
>>>> if (!uni)
>>>> continue;
>>>> *val++ =
>>>> @@ -1099,7 +1112,8 @@ _mesa_program_resource_prop(struct
>>>> gl_shader_program *shProg,
>>>> case GL_PROGRAM_INPUT:
>>>> case GL_PROGRAM_OUTPUT:
>>>> *val = program_resource_location(shProg, res,
>>>> - _mesa_program_resource_name(res));
>>>> + _mesa_program_resource_name(res),
>>>> + 0);
>>>> return 1;
>>>> default:
>>>> goto invalid_operation;
>>>> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>>>> index 0887c68..b702dcd 100644
>>>> --- a/src/mesa/main/shaderapi.c
>>>> +++ b/src/mesa/main/shaderapi.c
>>>> @@ -2229,7 +2229,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);
>>>> + res = _mesa_program_resource_find_name(shProg, resource_type,
>>>> name, NULL);
>>>> 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 c081f74..0a10191 100644
>>>> --- a/src/mesa/main/shaderapi.h
>>>> +++ b/src/mesa/main/shaderapi.h
>>>> @@ -232,7 +232,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);
>>>> + GLenum programInterface, const
>> char
>>>> *name,
>>>> + unsigned *array_index);
>>>> extern struct gl_program_resource *
>>>> _mesa_program_resource_find_index(struct gl_shader_program
>> *shProg,
>>>> diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
>>>> index 6ba746e..ff1df72 100644
>>>> --- a/src/mesa/main/uniforms.c
>>>> +++ b/src/mesa/main/uniforms.c
>>>> @@ -952,7 +952,7 @@ _mesa_GetUniformBlockIndex(GLuint program,
>>>> struct gl_program_resource *res =
>>>> _mesa_program_resource_find_name(shProg, GL_UNIFORM_BLOCK,
>>>> - uniformBlockName);
>>>> + uniformBlockName, NULL);
>>>> if (!res)
>>>> return GL_INVALID_INDEX;
>>>> @@ -987,7 +987,8 @@ _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]);
>>>> + _mesa_program_resource_find_name(shProg, GL_UNIFORM,
>>>> uniformNames[i],
>>>> + NULL);
>>>> uniformIndices[i] = _mesa_program_resource_index(shProg,
>> res);
>>>> }
>>>> }
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list