Re: [Mesa-dev] [PATCH] Revert "glsl: don't reference shader prog data during cache fallback"

[Timothy Arceri]

On 17/05/17 22:03, Nicolai Hähnle wrote:
On 17.05.2017 01:49, Timothy Arceri wrote:
This might work for gallium based drivers but I'm pretty sure this will
cause problems for the i965 fallback path.

What do those problems look like?

Sorry, I meant to add more details. The problem is that when the i965 
driver has a cache miss for a variant this could be happening long after 
link time. By this point we already have a reference and we also can't 
just reallocate the shader data because uniforms etc have most likely 
been set by this point.

And any ideas on how to address the 
issue then? There are a number of places that assume prog->sh.data is 
non-NULL.

I'll need to take a closer look at the problem tomorrow. I'm not sure I 
understand why it is NULL, I thought the reference happens in the GLSL 
IR cache. Is it getting cleared somewhere by the tgsi pass?


Cheers,
Nicolai



On 17/05/17 04:40, Nicolai Hähnle wrote:
From: Nicolai Hähnle <nicolai.haeh...@amd.com>

This reverts commit 0e9991f957e296f46cfff40a94ffba0adf2a58e1.

At least when we fallback because of TGSI, the gl_program objects are
re-allocated, and we don't in fact already have a reference to the
gl_shader_program_data.

This fixes a crash that can be reproduced as follows:

1. Run any GL program with MESA_GLSL=cache_info.
2. Note the SHA of some tgsi_tokens.
3. Remove the corresponding file from ~/.cache/mesa and re-run.

Cc: 17.1 <mesa-sta...@lists.freedesktop.org>
---
  src/compiler/glsl/linker.cpp | 3 +--
  src/mesa/main/shaderobj.c    | 3 +--
  2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index 2e7dd2b..4776d09 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -2243,22 +2243,21 @@ link_intrastage_shaders(void *mem_ctx,
     struct gl_program *gl_prog =
        ctx->Driver.NewProgram(ctx,

_mesa_shader_stage_to_program(shader_list[0]->Stage),
                               prog->Name, false);
     if (!gl_prog) {
        prog->data->LinkStatus = linking_failure;
        _mesa_delete_linked_shader(ctx, linked);
        return NULL;
     }
  -   if (!prog->data->cache_fallback)
-      _mesa_reference_shader_program_data(ctx, &gl_prog->sh.data,
prog->data);
+   _mesa_reference_shader_program_data(ctx, &gl_prog->sh.data,
prog->data);
       /* Don't use _mesa_reference_program() just take ownership */
     linked->Program = gl_prog;
       linked->ir = new(linked) exec_list;
     clone_ir_list(mem_ctx, linked->ir, main->ir);
       link_fs_inout_layout_qualifiers(prog, linked, shader_list,
num_shaders);
     link_tcs_out_layout_qualifiers(prog, gl_prog, shader_list,
num_shaders);
     link_tes_in_layout_qualifiers(prog, gl_prog, shader_list,
num_shaders);
diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
index 8a5fa5e..f6f7bf5 100644
--- a/src/mesa/main/shaderobj.c
+++ b/src/mesa/main/shaderobj.c
@@ -427,22 +427,21 @@ _mesa_free_shader_program_data(struct gl_context
*ctx,
      /**
   * Free/delete a shader program object.
   */
  void
  _mesa_delete_shader_program(struct gl_context *ctx,
                              struct gl_shader_program *shProg)
  {
     _mesa_free_shader_program_data(ctx, shProg);
-   if (!shProg->data->cache_fallback)
-      _mesa_reference_shader_program_data(ctx, &shProg->data, NULL);
+   _mesa_reference_shader_program_data(ctx, &shProg->data, NULL);
     ralloc_free(shProg);
  }
      /**
   * Lookup a GLSL program object.
   */
  struct gl_shader_program *
  _mesa_lookup_shader_program(struct gl_context *ctx, GLuint name)
  {

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev