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

[Emil Velikov]

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 ?

Thanks
Emil