[Mesa-dev] [PATCH] Revert "glsl: don't reference shader prog data during cache fallback"
Timothy Arceri
tarceri at itsqueeze.com
Thu May 18 05:36:21 UTC 2017
On 18/05/17 13:11, Timothy Arceri wrote:
> On 17/05/17 22:50, Nicolai Hähnle wrote:
>> 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.
>
> I see what has happened, this patch made it in but some other fallback
> patches that should really go along with it were left in the i965 branch.
>
> https://cgit.freedesktop.org/~jljusten/mesa/commit/?h=i965-shader-cache&id=5d6c2450fe48e777f6c57d9b70653832afa9bccc
>
> https://cgit.freedesktop.org/~jljusten/mesa/commit/?h=i965-shader-cache&id=b832afe2c52c946a77b213b94850a765bbaf36af
>
> https://cgit.freedesktop.org/~jljusten/mesa/commit/?h=i965-shader-cache&id=d8e8e39cc8b83393eac1bc32c017d312fd9af082
>
> https://cgit.freedesktop.org/~jljusten/mesa/commit/?h=i965-shader-cache&id=919ca263e258142aa8649b2b1c640368c1c4bab9
>
>
> I can see two solutions:
>
> 1. Make use of the mesa_build_fallback_shader_program() helper Jordan
> has created (patch 2 above). This would probably allow us to skip some
> things in the glsl to tgsi pass on fallback.
>
> 2. Stop setting cache_fallback in st_load_tgsi_from_disk_cache(). This
> should be safe because unlike i965 we are always falling back at link
> time. We will unnecessarily free and reallocate a bunch of things here
> but it's probably not a huge deal.
>
> Either way we should probably turn the env var in patch 4 above into a
> MESA_GLSL param so that we can force the fallback path for easy testing
> with piglit. I think you will find with this current patch that uniforms
> (among other things) will never be allocated storage on fallback because
> we skip that also.
>
> For stable I think we might just want to land option 2, once we do some
> testing. I think option 1 would need a bit more time sitting in master
> before I was comfortable with it.
>
> Thanks for spotting this, it was an oversight in testing on my behalf.
>
> I've send out a patch for the env var and wait for your thoughts before
> doing anything further.
I changed my mind, I send out the env var patch and a fix that does
option 2 for now.
https://patchwork.freedesktop.org/series/24601/
>
> Thanks,
> Tim
>
>>
>> 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
>>>>
>>>>
>>
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list