[Mesa-dev] [PATCH 28/40] glsl/mesa: make a copy of attribute bindings in case of cache fallback

[Timothy Arceri]

On 10/02/17 22:25, Nicolai Hähnle wrote:
> On 07.02.2017 04:42, Timothy Arceri wrote:
>> From: Timothy Arceri <timothy.arceri at collabora.com>
>>
>> If the shader cache falls back to doing a compile and link we need the
>> original attribute bindings as they could have changed after the program
>> was first linked.
>
> So, same question as for patch #22, I don't really understand why the 
> fallback attributes should be necessary and what the data flow is, 
> since the cache_fallback happens during glLinkProgram().

See reply to 22.

>
> On top of that:
>
>> ---
>>  src/compiler/glsl/shader_cache.cpp | 15 +++++++++++++++
>>  src/mesa/main/mtypes.h             |  7 +++++++
>>  src/mesa/main/shaderobj.c          |  6 ++++++
>>  3 files changed, 28 insertions(+)
>>
>> diff --git a/src/compiler/glsl/shader_cache.cpp 
>> b/src/compiler/glsl/shader_cache.cpp
>> index 729dd09..ddcd530 100644
>> --- a/src/compiler/glsl/shader_cache.cpp
>> +++ b/src/compiler/glsl/shader_cache.cpp
>> @@ -808,9 +808,22 @@ read_hash_table(struct blob_reader *metadata, 
>> struct string_to_uint_map *hash)
>>  }
>>
>>  static void
>> +copy_hash_entry(const void *key, void *data, void *closure)
>> +{
>> +   struct string_to_uint_map *ht = (struct string_to_uint_map *) 
>> closure;
>> +
>> +   /* string_to_uint_map increases everything by 1 so we need to 
>> allow for
>> +    * this when copying the data directly.
>> +    */
>> +   ht->put(((intptr_t) data) - 1, (const char *) key);
>> +}
>> +
>> +static void
>>  write_hash_tables(struct blob *metadata, struct gl_shader_program 
>> *prog)
>>  {
>>     write_hash_table(metadata, prog->AttributeBindings);
>> +   hash_table_call_foreach(prog->AttributeBindings->ht, 
>> copy_hash_entry,
>> + prog->data->FallbackAttributeBindings);
>>     write_hash_table(metadata, prog->FragDataBindings);
>>     write_hash_table(metadata, prog->FragDataIndexBindings);
>>  }
>> @@ -819,6 +832,8 @@ static void
>>  read_hash_tables(struct blob_reader *metadata, struct 
>> gl_shader_program *prog)
>>  {
>>     read_hash_table(metadata, prog->AttributeBindings);
>> +   hash_table_call_foreach(prog->AttributeBindings->ht, 
>> copy_hash_entry,
>> + prog->data->FallbackAttributeBindings);
>
> It seems rather suspicious that both write_hash_tables and 
> read_hash_tables perform the copy in the same direction, and I don't 
> see FallbackAttributeBindings being used anywhere else.

I'm not sure what you mean about the direction (need to take a better 
look tomorrow its late here). As you point out tgsi fallback is done 
during linking so it doesn't need it. We use is for the fallback in i965 
as we have no NIR level cache [1].

[1] 
https://github.com/tarceri/Mesa/commit/31089f44c9a74ab303d36616f4e9520bd5a62d05

>
> Cheers,
> Nicolai
>
>>     read_hash_table(metadata, prog->FragDataBindings);
>>     read_hash_table(metadata, prog->FragDataIndexBindings);
>>  }
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index f65cd76..d1dde0c 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -2688,6 +2688,13 @@ struct gl_shader_program_data
>>     GLuint NumFallbackShaders;
>>     struct gl_shader **FallbackShaders; /**< Shaders used for cache 
>> fallback */
>>
>> +   /**
>> +    * If the shader cache falls back to doing a compile and link we 
>> need the
>> +    * original attribute bindings as they could have changed after 
>> the program
>> +    * was first linked.
>> +    */
>> +   struct string_to_uint_map *FallbackAttributeBindings;
>> +
>>     /** List of all active resources after linking. */
>>     struct gl_program_resource *ProgramResourceList;
>>     unsigned NumProgramResourceList;
>> diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
>> index ed19a72..245a0b9 100644
>> --- a/src/mesa/main/shaderobj.c
>> +++ b/src/mesa/main/shaderobj.c
>> @@ -276,6 +276,7 @@ init_shader_program(struct gl_shader_program *prog)
>>     prog->RefCount = 1;
>>
>>     prog->AttributeBindings = string_to_uint_map_ctor();
>> +   prog->data->FallbackAttributeBindings = string_to_uint_map_ctor();
>>     prog->FragDataBindings = string_to_uint_map_ctor();
>>     prog->FragDataIndexBindings = string_to_uint_map_ctor();
>>
>> @@ -393,6 +394,11 @@ _mesa_free_shader_program_data(struct gl_context 
>> *ctx,
>>        shProg->AttributeBindings = NULL;
>>     }
>>
>> +   if (shProg->data->FallbackAttributeBindings) {
>> + string_to_uint_map_dtor(shProg->data->FallbackAttributeBindings);
>> +      shProg->data->FallbackAttributeBindings = NULL;
>> +   }
>> +
>>     if (shProg->FragDataBindings) {
>>        string_to_uint_map_dtor(shProg->FragDataBindings);
>>        shProg->FragDataBindings = NULL;
>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev