[Mesa-dev] [PATCH V2 3/3] st/mesa: don't mark the program as in cache_fallback when there is cache miss

[Timothy Arceri]

On 19/05/17 19:37, Nicolai Hähnle wrote:
> On 19.05.2017 03:02, Timothy Arceri wrote:
>> When we fallback currently the gl_program objects are re-allocated.
>>
>> This is likely to change when the i965 cache lands, but for now
>> this fixes a crash when using MESA_GLSL=cache_fb. This env var
>> simulates the fallback path taken when a tgsi cache item doesn't
>> exist due to being evicted previously or some kind of error.
>>
>> Unlike i965 we are always falling back at link time so it's safe to
>> just re-allocate everything. We will be unnecessarily freeing and
>> re-allocate a bunch of things here but it's probably not a huge deal,
>> and can be changed when the i965 code lands.
>>
>> Fixes: 0e9991f957e2 ("glsl: don't reference shader prog data during 
>> cache fallback")
>> ---
>>  src/compiler/glsl/shader_cache.cpp       | 2 +-
>>  src/mesa/main/mtypes.h                   | 2 ++
>>  src/mesa/state_tracker/st_shader_cache.c | 2 +-
>>  3 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/shader_cache.cpp 
>> b/src/compiler/glsl/shader_cache.cpp
>> index 800d3a2..dd56501 100644
>> --- a/src/compiler/glsl/shader_cache.cpp
>> +++ b/src/compiler/glsl/shader_cache.cpp
>> @@ -1285,21 +1285,21 @@ bool
>>  shader_cache_read_program_metadata(struct gl_context *ctx,
>>                                     struct gl_shader_program *prog)
>>  {
>>     /* Fixed function programs generated by Mesa are not cached. So don't
>>      * try to read metadata for them from the cache.
>>      */
>>     if (prog->Name == 0)
>>        return false;
>>
>>     struct disk_cache *cache = ctx->Cache;
>> -   if (!cache || prog->data->cache_fallback)
>> +   if (!cache || prog->data->cache_fallback || prog->data->skip_cache)
>>        return false;
>>
>>     /* Include bindings when creating sha1. These bindings change the 
>> resulting
>>      * binary so they are just as important as the shader source.
>>      */
>>     char *buf = ralloc_strdup(NULL, "vb: ");
>>     prog->AttributeBindings->iterate(create_binding_str, &buf);
>>     ralloc_strcat(&buf, "fb: ");
>>     prog->FragDataBindings->iterate(create_binding_str, &buf);
>>     ralloc_strcat(&buf, "fbi: ");
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index 941311b..9ca4bb8 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -2807,20 +2807,22 @@ struct gl_shader_program_data
>>
>>     struct gl_active_atomic_buffer *AtomicBuffers;
>>     unsigned NumAtomicBuffers;
>>
>>     /* Shader cache variables used during restore */
>>     unsigned NumUniformDataSlots;
>>     union gl_constant_value *UniformDataSlots;
>>
>>     bool cache_fallback;
>>
>> +   bool skip_cache;
> 
> Can you give a simple explanation of the semantics of cache_fallback and 
> skip_cache? I have the impression that we're adding to an ever more 
> complicated state machine that doesn't have a very clean definition :/

Sure.

> 
> You mentioned in the commit message that there's a chance that this 
> could become simpler when the i965 part lands, so I guess we can live 
> with it for now...

Yeah, I thought about using it now but it seemed too invasive for 
something we want to backport to stable.

> 
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> 
>> +
>>     /** List of all active resources after linking. */
>>     struct gl_program_resource *ProgramResourceList;
>>     unsigned NumProgramResourceList;
>>
>>     enum gl_link_status LinkStatus;   /**< GL_LINK_STATUS */
>>     GLboolean Validated;
>>     GLchar *InfoLog;
>>
>>     unsigned Version;       /**< GLSL version used for linking */
>>
>> diff --git a/src/mesa/state_tracker/st_shader_cache.c 
>> b/src/mesa/state_tracker/st_shader_cache.c
>> index 45438e5..31c3430 100644
>> --- a/src/mesa/state_tracker/st_shader_cache.c
>> +++ b/src/mesa/state_tracker/st_shader_cache.c
>> @@ -384,15 +384,15 @@ st_load_tgsi_from_disk_cache(struct gl_context 
>> *ctx,
>>  fallback_recompile:
>>     free(buffer);
>>
>>     if (ctx->_Shader->Flags & GLSL_CACHE_INFO)
>>        fprintf(stderr, "TGSI cache falling back to recompile.\n");
>>
>>     for (unsigned i = 0; i < prog->NumShaders; i++) {
>>        _mesa_glsl_compile_shader(ctx, prog->Shaders[i], false, false, 
>> true);
>>     }
>>
>> -   prog->data->cache_fallback = true;
>> +   prog->data->skip_cache = true;
>>     _mesa_glsl_link_shader(ctx, prog);
>>
>>     return true;
>>  }
>>
> 
>