<div dir="ltr"><div>Reviewed-by: Marek Olšák <<a href="mailto:marek.olsak@amd.com">marek.olsak@amd.com</a>></div><div> </div><div>Marek </div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr 23, 2019 at 2:04 AM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Commit 624789e3708c moved the destruction of types out of atexit() and made use of a ref count instead. This is useful for avoiding a crash where drivers such as radeonsi are still compiling in a thread when the app exits and has not called MakeCurrent to change from the current context. While the above scenario is technically an app bug we shouldn't crash. However that change caused another race condition between the shader compilation tread in radeonsi and context teardown functions. This patch makes two changes to fix this new problem: First we explicitly call _mesa_destroy_shader_compiler_types() when destroying the st context rather than calling it indirectly via _mesa_free_context_data(). We do this as we must call it after st_destroy_context_priv() so that we don't destory the glsl types before the compilation threads finish. Next wait for the shader threads to finish in si_destroy_context() this also means we need to call context destroy before destroying the queues in si_destroy_screen(). Fixes: 624789e3708c ("compiler/glsl: handle case where we have multiple users for types") --- src/compiler/glsl/glsl_parser_extras.h | 1 + src/gallium/drivers/radeonsi/si_pipe.c | 8 ++++++-- src/mesa/drivers/dri/i915/intel_context.c | 2 +- src/mesa/drivers/dri/i965/brw_context.c | 2 +- src/mesa/drivers/dri/nouveau/nouveau_context.c | 2 +- src/mesa/drivers/dri/radeon/radeon_common_context.c | 2 +- src/mesa/drivers/osmesa/osmesa.c | 8 ++++---- src/mesa/drivers/x11/xm_api.c | 4 ++-- src/mesa/main/context.c | 7 ++++--- src/mesa/main/context.h | 2 +- src/mesa/state_tracker/st_context.c | 8 +++++++- 11 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/compiler/glsl/glsl_parser_extras.h b/src/compiler/glsl/glsl_parser_extras.h index f92d2160aac..edc6fc06c77 100644 --- a/src/compiler/glsl/glsl_parser_extras.h +++ b/src/compiler/glsl/glsl_parser_extras.h @@ -997,6 +997,7 @@ extern "C" { #endif struct glcpp_parser; +struct _mesa_glsl_parse_state; typedef void (*glcpp_extension_iterator)( struct _mesa_glsl_parse_state *state, diff --git a/src/gallium/drivers/radeonsi/si_pipe.c b/src/gallium/drivers/radeonsi/si_pipe.c index fa96ce34224..e9e1bd0aa38 100644 --- a/src/gallium/drivers/radeonsi/si_pipe.c +++ b/src/gallium/drivers/radeonsi/si_pipe.c @@ -150,6 +150,9 @@ static void si_destroy_context(struct pipe_context *context) struct si_context *sctx = (struct si_context *)context; int i; + util_queue_finish(&sctx->screen->shader_compiler_queue); + util_queue_finish(&sctx->screen->shader_compiler_queue_low_priority); + /* Unreference the framebuffer normally to disable related logic * properly. */ @@ -702,6 +705,9 @@ static void si_destroy_screen(struct pipe_screen* pscreen) if (!sscreen->ws->unref(sscreen->ws)) return; + mtx_destroy(&sscreen->aux_context_lock); + sscreen->aux_context->destroy(sscreen->aux_context); + util_queue_destroy(&sscreen->shader_compiler_queue); util_queue_destroy(&sscreen->shader_compiler_queue_low_priority); @@ -728,8 +734,6 @@ static void si_destroy_screen(struct pipe_screen* pscreen) si_gpu_load_kill_thread(sscreen); mtx_destroy(&sscreen->gpu_load_mutex); - mtx_destroy(&sscreen->aux_context_lock); - sscreen->aux_context->destroy(sscreen->aux_context); slab_destroy_parent(&sscreen->pool_transfers); diff --git a/src/mesa/drivers/dri/i915/intel_context.c b/src/mesa/drivers/dri/i915/intel_context.c index c23e5ffb26e..aa3175816cf 100644 --- a/src/mesa/drivers/dri/i915/intel_context.c +++ b/src/mesa/drivers/dri/i915/intel_context.c @@ -599,7 +599,7 @@ intelDestroyContext(__DRIcontext * driContextPriv) driDestroyOptionCache(&intel->optionCache); /* free the Mesa context */ - _mesa_free_context_data(&intel->ctx); + _mesa_free_context_data(&intel->ctx, true); _math_matrix_dtr(&intel->ViewportMatrix); diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c index ab637ddf721..df12f373f22 100644 --- a/src/mesa/drivers/dri/i965/brw_context.c +++ b/src/mesa/drivers/dri/i965/brw_context.c @@ -1226,7 +1226,7 @@ intelDestroyContext(__DRIcontext * driContextPriv) driDestroyOptionCache(&brw->optionCache); /* free the Mesa context */ - _mesa_free_context_data(&brw->ctx); + _mesa_free_context_data(&brw->ctx, true); ralloc_free(brw); driContextPriv->driverPrivate = NULL; diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c b/src/mesa/drivers/dri/nouveau/nouveau_context.c index 93f6ce473a1..8fec35237c0 100644 --- a/src/mesa/drivers/dri/nouveau/nouveau_context.c +++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c @@ -217,7 +217,7 @@ nouveau_context_deinit(struct gl_context *ctx) nouveau_object_del(&nctx->hw.chan); nouveau_scratch_destroy(ctx); - _mesa_free_context_data(ctx); + _mesa_free_context_data(ctx, true); } void diff --git a/src/mesa/drivers/dri/radeon/radeon_common_context.c b/src/mesa/drivers/dri/radeon/radeon_common_context.c index 47719baa575..689304aa4fc 100644 --- a/src/mesa/drivers/dri/radeon/radeon_common_context.c +++ b/src/mesa/drivers/dri/radeon/radeon_common_context.c @@ -270,7 +270,7 @@ void radeonDestroyContext(__DRIcontext *driContextPriv ) /* free atom list */ /* free the Mesa context data */ - _mesa_free_context_data(&radeon->glCtx); + _mesa_free_context_data(&radeon->glCtx, true); /* free the option cache */ driDestroyOptionCache(&radeon->optionCache); diff --git a/src/mesa/drivers/osmesa/osmesa.c b/src/mesa/drivers/osmesa/osmesa.c index 44374a2e917..a065161b89e 100644 --- a/src/mesa/drivers/osmesa/osmesa.c +++ b/src/mesa/drivers/osmesa/osmesa.c @@ -854,7 +854,7 @@ OSMesaCreateContextAttribs(const int *attribList, OSMesaContext sharelist) osmesa->gl_buffer = _mesa_create_framebuffer(osmesa->gl_visual); if (!osmesa->gl_buffer) { _mesa_destroy_visual( osmesa->gl_visual ); - _mesa_free_context_data( &osmesa->mesa ); + _mesa_free_context_data(&osmesa->mesa, true); free(osmesa); return NULL; } @@ -891,7 +891,7 @@ OSMesaCreateContextAttribs(const int *attribList, OSMesaContext sharelist) !_tnl_CreateContext( ctx ) || !_swsetup_CreateContext( ctx )) { _mesa_destroy_visual(osmesa->gl_visual); - _mesa_free_context_data(ctx); + _mesa_free_context_data(ctx, true); free(osmesa); return NULL; } @@ -919,7 +919,7 @@ OSMesaCreateContextAttribs(const int *attribList, OSMesaContext sharelist) if (ctx->Version < version_major * 10 + version_minor) { _mesa_destroy_visual(osmesa->gl_visual); - _mesa_free_context_data(ctx); + _mesa_free_context_data(ctx, true); free(osmesa); return NULL; } @@ -955,7 +955,7 @@ OSMesaDestroyContext( OSMesaContext osmesa ) _mesa_destroy_visual( osmesa->gl_visual ); _mesa_reference_framebuffer( &osmesa->gl_buffer, NULL ); - _mesa_free_context_data( &osmesa->mesa ); + _mesa_free_context_data(&osmesa->mesa, true); free( osmesa ); } } diff --git a/src/mesa/drivers/x11/xm_api.c b/src/mesa/drivers/x11/xm_api.c index e8405950656..90f89a36221 100644 --- a/src/mesa/drivers/x11/xm_api.c +++ b/src/mesa/drivers/x11/xm_api.c @@ -945,7 +945,7 @@ XMesaContext XMesaCreateContext( XMesaVisual v, XMesaContext share_list ) !_vbo_CreateContext( mesaCtx ) || !_tnl_CreateContext( mesaCtx ) || !_swsetup_CreateContext( mesaCtx )) { - _mesa_free_context_data(&c->mesa); + _mesa_free_context_data(&c->mesa, true); free(c); return NULL; } @@ -982,7 +982,7 @@ void XMesaDestroyContext( XMesaContext c ) _swrast_DestroyContext( mesaCtx ); _tnl_DestroyContext( mesaCtx ); _vbo_DestroyContext( mesaCtx ); - _mesa_free_context_data( mesaCtx ); + _mesa_free_context_data(mesaCtx, true); free( c ); } diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c index 7300d2f3c46..2c3d9a11ce3 100644 --- a/src/mesa/main/context.c +++ b/src/mesa/main/context.c @@ -1310,7 +1310,7 @@ fail: * \sa _mesa_initialize_context() and init_attrib_groups(). */ void -_mesa_free_context_data( struct gl_context *ctx ) +_mesa_free_context_data(struct gl_context *ctx, bool destroy_compiler_types) { if (!_mesa_get_current_context()){ /* No current context, but we may need one in order to delete @@ -1385,7 +1385,8 @@ _mesa_free_context_data( struct gl_context *ctx ) free(ctx->VersionString); - _mesa_destroy_shader_compiler_types(); + if (destroy_compiler_types) + _mesa_destroy_shader_compiler_types(); /* unbind the context if it's currently bound */ if (ctx == _mesa_get_current_context()) { @@ -1405,7 +1406,7 @@ void _mesa_destroy_context( struct gl_context *ctx ) { if (ctx) { - _mesa_free_context_data(ctx); + _mesa_free_context_data(ctx, true); free( (void *) ctx ); } } diff --git a/src/mesa/main/context.h b/src/mesa/main/context.h index 7de10e9924b..e9b270df14c 100644 --- a/src/mesa/main/context.h +++ b/src/mesa/main/context.h @@ -115,7 +115,7 @@ _mesa_initialize_context( struct gl_context *ctx, const struct dd_function_table *driverFunctions); extern void -_mesa_free_context_data( struct gl_context *ctx ); +_mesa_free_context_data(struct gl_context *ctx, bool destroy_compiler_types); extern void _mesa_destroy_context( struct gl_context *ctx ); diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c index 09d467aa360..067d47ef5e9 100644 --- a/src/mesa/state_tracker/st_context.c +++ b/src/mesa/state_tracker/st_context.c @@ -85,6 +85,7 @@ #include "util/u_upload_mgr.h" #include "util/u_vbuf.h" #include "cso_cache/cso_context.h" +#include "compiler/glsl/glsl_parser_extras.h" DEBUG_GET_ONCE_BOOL_OPTION(mesa_mvp_dp4, "MESA_MVP_DP4", FALSE) @@ -976,13 +977,18 @@ st_destroy_context(struct st_context *st) st_destroy_program_variants(st); - _mesa_free_context_data(ctx); + _mesa_free_context_data(ctx, false); /* This will free the st_context too, so 'st' must not be accessed * afterwards. */ st_destroy_context_priv(st, true); st = NULL; + /* This must be called after st_destroy_context_priv() to avoid a race + * condition between any shader compiler threads and context destruction. + */ + _mesa_destroy_shader_compiler_types(); + free(ctx); if (save_ctx == ctx) { -- 2.20.1 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a></blockquote></div>