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

[Nicolai Hähnle]

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 :/

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...

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;
>  }
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.