[Mesa-dev] [PATCH] mesa: fix GetProgramiv/GetActiveAttrib regression

[Tapani Pälli]

On 10/02/2015 07:46 PM, Ilia Mirkin wrote:
> On Mon, Sep 28, 2015 at 5:12 AM, Tapani Pälli <tapani.palli at intel.com> wrote:
>> Commit 4639cea2921669527eb43dcb49724c05afb27e8e added packed varyings
>> as part of program resource list. We need to take this to account with
>> all functions dealing with active input attributes.
>>
>> Patch fixes the issue by adding additional check to is_active_attrib
>> and iterating active attribs explicitly in GetActiveAttrib function.
>>
>> I've tested that fix works for Assault Android Cactus (demo version)
>> and does not cause Piglit or CTS regressions in glGetProgramiv tests.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92122
>> ---
>>   src/mesa/main/shader_query.cpp | 44 +++++++++++++++++++++++++++++++++---------
>>   1 file changed, 35 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
>> index 99d9e10..d023504 100644
>> --- a/src/mesa/main/shader_query.cpp
>> +++ b/src/mesa/main/shader_query.cpp
>> @@ -101,15 +101,34 @@ _mesa_BindAttribLocation(GLhandleARB program, GLuint index,
>>       */
>>   }
>>
>> +/* Function checks if given input variable is packed varying by checking
>> + * if there also exists output variable with the same name.
>> + */
>> +static bool
>> +is_packed_varying(struct gl_shader_program *shProg, const ir_variable *input)
>> +{
>> +   assert(input->data.mode == ir_var_shader_in);
> No statements before variable decls in core code.
ok
>> +   unsigned array_index = 0;
>> +   struct gl_program_resource *out =
>> +      _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT, input->name,
>> +                                       &array_index);
> This is slow, right? I.e. it's a loop?

It is agreeably slow, alternatively we could add new flag to ir_variable 
to make things quick.

>> +   if (out)
>> +      return true;
>> +   return false;
>> +}
>> +
>>   static bool
>> -is_active_attrib(const ir_variable *var)
>> +is_active_attrib(struct gl_shader_program *shProg,
>> +                 const ir_variable *var)
>>   {
>>      if (!var)
>>         return false;
>>
>>      switch (var->data.mode) {
>>      case ir_var_shader_in:
>> -      return var->data.location != -1;
>> +      return (var->data.location != -1 &&
>> +              var->data.location_frac == 0 &&
>> +              !is_packed_varying(shProg, var));
>>
>>      case ir_var_system_value:
>>         /* From GL 4.3 core spec, section 11.1.1 (Vertex Attributes):
>> @@ -154,19 +173,25 @@ _mesa_GetActiveAttrib(GLhandleARB program, GLuint desired_index,
>>         return;
>>      }
>>
>> -   struct gl_program_resource *res =
>> -      _mesa_program_resource_find_index(shProg, GL_PROGRAM_INPUT,
>> -                                        desired_index);
> You appear to replace this function, which is O(n), with a loop around
> what is essentially this same function, which is O(n^2). Could you
> write a more targeted helper which achieves what you want with a
> single scan of the program resource list?

OK, I can simplify this. At this point I think it would be fine to 
revert the patches adding packed varyings to resource list. I can then 
send these changes together when adding them back.

>> +   struct gl_program_resource *res = shProg->ProgramResourceList;
>> +   unsigned index = -1;
>> +   for (unsigned i = 0; i < shProg->NumProgramResourceList; i++, res++) {
>> +      if (res->Type != GL_PROGRAM_INPUT ||
>> +          !is_active_attrib(shProg, RESOURCE_VAR(res)))
>> +         continue;
>> +      if (++index == desired_index)
>> +         break;
>> +   }
>>
>>      /* User asked for index that does not exist. */
>> -   if (!res) {
>> +   if (!res || index != desired_index) {
>>         _mesa_error(ctx, GL_INVALID_VALUE, "glGetActiveAttrib(index)");
>>         return;
>>      }
>>
>>      const ir_variable *const var = RESOURCE_VAR(res);
>>
>> -   if (!is_active_attrib(var))
>> +   if (!is_active_attrib(shProg, var))
>>         return;
>>
>>      const char *var_name = var->name;
>> @@ -252,7 +277,7 @@ _mesa_count_active_attribs(struct gl_shader_program *shProg)
>>      for (unsigned j = 0; j < shProg->NumProgramResourceList; j++, res++) {
>>         if (res->Type == GL_PROGRAM_INPUT &&
>>             res->StageReferences & (1 << MESA_SHADER_VERTEX) &&
>> -          is_active_attrib(RESOURCE_VAR(res)))
>> +          is_active_attrib(shProg, RESOURCE_VAR(res)))
>>            count++;
>>      }
>>      return count;
>> @@ -271,7 +296,8 @@ _mesa_longest_attribute_name_length(struct gl_shader_program *shProg)
>>      size_t longest = 0;
>>      for (unsigned j = 0; j < shProg->NumProgramResourceList; j++, res++) {
>>         if (res->Type == GL_PROGRAM_INPUT &&
>> -          res->StageReferences & (1 << MESA_SHADER_VERTEX)) {
>> +          res->StageReferences & (1 << MESA_SHADER_VERTEX) &&
>> +          is_active_attrib(shProg, RESOURCE_VAR(res))) {
>>
>>             const size_t length = strlen(RESOURCE_VAR(res)->name);
>>             if (length >= longest)
>> --
>> 2.4.3
>>