[Mesa-dev] [PATCH] mesa: fix ARRAY_SIZE query for GetProgramResourceiv

[Tapani Pälli]

On 09/29/2015 01:42 PM, Emil Velikov wrote:
> Hi guys,
>
> On 29 September 2015 at 10:30, Martin Peres
> <martin.peres at linux.intel.com> wrote:
>> On 28/09/15 13:51, Tapani Pälli wrote:
>>>
>>> Patch also refactors name length queries which were using array size
>>> in computation, this has to be done in same time to avoid regression in
>>> arb_program_interface_query-resource-query Piglit test.
>>>
>>> Fixes rest of the failures with
>>>      ES31-CTS.program_interface_query.no-locations
>>>
>>> v2: make additional check only for GS inputs
>>> v3: create helper function for resource name length
>>>       so that it gets calculated only in one place
>>>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> Reviewed-by: Martin Peres <martin.peres at linux.intel.com> [v2]
>>> ---
>>>    src/mesa/main/program_resource.c |  8 ++--
>>>    src/mesa/main/shader_query.cpp   | 94
>>> ++++++++++++++++++++++++----------------
>>>    src/mesa/main/shaderapi.h        |  3 ++
>>>    3 files changed, 62 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/src/mesa/main/program_resource.c
>>> b/src/mesa/main/program_resource.c
>>> index c609abe..eb71fdd 100644
>>> --- a/src/mesa/main/program_resource.c
>>> +++ b/src/mesa/main/program_resource.c
>>> @@ -111,11 +111,9 @@ _mesa_GetProgramInterfaceiv(GLuint program, GLenum
>>> programInterface,
>>>          for (i = 0, *params = 0; i < shProg->NumProgramResourceList; i++)
>>> {
>>>             if (shProg->ProgramResourceList[i].Type != programInterface)
>>>                continue;
>>> -         const char *name =
>>> -            _mesa_program_resource_name(&shProg->ProgramResourceList[i]);
>>> -         unsigned array_size =
>>> -
>>> _mesa_program_resource_array_size(&shProg->ProgramResourceList[i]);
>>> -         *params = MAX2(*params, strlen(name) + (array_size ? 3 : 0) +
>>> 1);
>>> +         unsigned len =
>>> +
>>> _mesa_program_resource_name_len(&shProg->ProgramResourceList[i]);
>>> +         *params = MAX2(*params, len + 1);
>>>          }
>>>          break;
>>>       case GL_MAX_NUM_ACTIVE_VARIABLES:
>>> diff --git a/src/mesa/main/shader_query.cpp
>>> b/src/mesa/main/shader_query.cpp
>>> index 99d9e10..a5572ff 100644
>>> --- a/src/mesa/main/shader_query.cpp
>>> +++ b/src/mesa/main/shader_query.cpp
>>> @@ -478,7 +478,7 @@ _mesa_program_resource_array_size(struct
>>> gl_program_resource *res)
>>>                 RESOURCE_XFB(res)->Size : 0;
>>>       case GL_PROGRAM_INPUT:
>>>       case GL_PROGRAM_OUTPUT:
>>> -      return RESOURCE_VAR(res)->data.max_array_access;
>>> +      return RESOURCE_VAR(res)->type->length;
>>>       case GL_UNIFORM:
>>>       case GL_VERTEX_SUBROUTINE_UNIFORM:
>>>       case GL_GEOMETRY_SUBROUTINE_UNIFORM:
>>> @@ -670,6 +670,57 @@ _mesa_program_resource_find_index(struct
>>> gl_shader_program *shProg,
>>>       return NULL;
>>>    }
>>>    +/* Function returns if resource name is expected to have index
>>> + * appended into it.
>>> + *
>>> + *
>>> + * Page 61 (page 73 of the PDF) in section 2.11 of the OpenGL ES 3.0
>>> + * spec says:
>>> + *
>>> + *     "If the active uniform is an array, the uniform name returned in
>>> + *     name will always be the name of the uniform array appended with
>>> + *     "[0]"."
>>> + *
>>> + * The same text also appears in the OpenGL 4.2 spec.  It does not,
>>> + * however, appear in any previous spec.  Previous specifications are
>>> + * ambiguous in this regard.  However, either name can later be passed
>>> + * to glGetUniformLocation (and related APIs), so there shouldn't be any
>>> + * harm in always appending "[0]" to uniform array names.
>>> + *
>>> + * Geometry shader stage has different naming convention where the
>>> 'normal'
>>> + * condition is an array, therefore for variables referenced in geometry
>>> + * stage we do not add '[0]'.
>>> + *
>>> + * Note, that TCS outputs and TES inputs should not have index appended
>>> + * either.
>>> + */
>>> +bool
>>> +add_index_to_name(struct gl_program_resource *res)
>>
>> static please!
>>
>> With this, you get:
>> Reviewed-by: Martin Peres <martin.peres at linux.intel.com>
>>
>> Thanks for the cleanup!
>>
> Would this be mesa-stable worthy material ?

We haven't had bugs filed for this but I guess it's just matter of time. 
Yes please!

// Tapani