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 willcause 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=919ca263e258142aa8649b2b1c640368c1c4bab9I 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, TimCheers, NicolaiCheers, NicolaiOn 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
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
