On 30 July 2015 3:27:41 pm AEST, "Tapani Pälli" <tapani.pa...@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.
>>
>
>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.pa...@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
