Reviewed-by: Marek Olšák <marek.ol...@amd.com> Marek
On Tue, Apr 23, 2019 at 2:04 AM Timothy Arceri <tarc...@itsqueeze.com> wrote: > 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 > 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