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

[Nicolai Hähnle]

On 17.05.2017 14:26, Timothy Arceri wrote:
> 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.

Oh I see, thanks for the explanation.


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

As far as I understand, the gl_program object doesn't come from the 
cache in the fallback path. So the reference should happen in the place 
modified by this patch.

Cheers,
Nicolai

>
>>
>> Cheers,
>> Nicolai
>>
>>
>>>
>>> On 17/05/17 04:40, Nicolai Hähnle wrote:
>>>> From: Nicolai Hähnle <nicolai.haehnle at 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-stable at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>


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