From bbc29393d3beaf6344c7188547b4ff61b63946ae Mon Sep 17 00:00:00 2001 From: Charmaine Lee Date: Fri, 21 Jul 2017 21:41:06 -0700 Subject: [PATCH] st/mesa: create framebuffer iface hash table per st manager With commit 5124bf98239, a framebuffer interface hash table is created in st_gl_api_create(), which is called in dri_init_screen_helper() for each screen. When the hash table is overwritten with multiple calls to st_gl_api_create(), it can cause race condition. This patch fixes the problem by creating a framebuffer interface hash table per state tracker manager. Fixes crash with steam. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101876 Fixes: 5124bf98239 ("st/mesa: add destroy_drawable interface") Tested-by: Christoph Haag Reviewed-by: Brian Paul --- src/gallium/include/state_tracker/st_api.h | 21 ++++ src/gallium/state_trackers/dri/dri_drawable.c | 1 + src/gallium/state_trackers/dri/dri_screen.c | 3 + src/gallium/state_trackers/glx/xlib/xm_api.c | 3 + src/gallium/state_trackers/glx/xlib/xm_st.c | 1 + src/gallium/state_trackers/wgl/stw_device.c | 3 + src/gallium/state_trackers/wgl/stw_st.c | 1 + src/mesa/state_tracker/st_manager.c | 107 +++++++++++++----- 8 files changed, 113 insertions(+), 27 deletions(-) diff --git a/src/gallium/include/state_tracker/st_api.h b/src/gallium/include/state_tracker/st_api.h index 9b660f74c93..bc62a69da37 100644 --- a/src/gallium/include/state_tracker/st_api.h +++ b/src/gallium/include/state_tracker/st_api.h @@ -284,6 +284,7 @@ struct st_context_attribs }; struct st_context_iface; +struct st_manager; /** * Represent a windowing system drawable. @@ -316,6 +317,11 @@ struct st_framebuffer_iface */ uint32_t ID; + /** + * The state tracker manager that manages this object. + */ + struct st_manager *state_manager; + /** * Available for the state tracker manager to use. */ @@ -375,6 +381,11 @@ struct st_context_iface void *st_context_private; void *st_manager_private; + /** + * The state tracker manager that manages this object. + */ + struct st_manager *state_manager; + /** * The CSO context associated with this context in case we need to draw * something before swap buffers. @@ -483,6 +494,16 @@ struct st_manager */ void (*set_background_context)(struct st_context_iface *stctxi, struct util_queue_monitoring *queue_info); + + /** + * Destroy any private data used by the state tracker manager. + */ + void (*destroy)(struct st_manager *smapi); + + /** + * Available for the state tracker manager to use. + */ + void *st_manager_private; }; /** diff --git a/src/gallium/state_trackers/dri/dri_drawable.c b/src/gallium/state_trackers/dri/dri_drawable.c index c7df0f63327..9e0dd6bcfb3 100644 --- a/src/gallium/state_trackers/dri/dri_drawable.c +++ b/src/gallium/state_trackers/dri/dri_drawable.c @@ -158,6 +158,7 @@ dri_create_buffer(__DRIscreen * sPriv, dPriv->driverPrivate = (void *)drawable; p_atomic_set(&drawable->base.stamp, 1); drawable->base.ID = p_atomic_inc_return(&drifb_ID); + drawable->base.state_manager = &screen->base; return GL_TRUE; fail: diff --git a/src/gallium/state_trackers/dri/dri_screen.c b/src/gallium/state_trackers/dri/dri_screen.c index 1dd7bd3ec12..59a850b1423 100644 --- a/src/gallium/state_trackers/dri/dri_screen.c +++ b/src/gallium/state_trackers/dri/dri_screen.c @@ -457,6 +457,9 @@ dri_destroy_option_cache(struct dri_screen * screen) void dri_destroy_screen_helper(struct dri_screen * screen) { + if (screen->base.destroy) + screen->base.destroy(&screen->base); + if (screen->st_api && screen->st_api->destroy) screen->st_api->destroy(screen->st_api); diff --git a/src/gallium/state_trackers/glx/xlib/xm_api.c b/src/gallium/state_trackers/glx/xlib/xm_api.c index e4b1e9dd4de..828253bdb19 100644 --- a/src/gallium/state_trackers/glx/xlib/xm_api.c +++ b/src/gallium/state_trackers/glx/xlib/xm_api.c @@ -181,6 +181,9 @@ xmesa_close_display(Display *display) * xmdpy->screen->destroy(xmdpy->screen); * } */ + + if (xmdpy->smapi->destroy) + xmdpy->smapi->destroy(xmdpy->smapi); free(xmdpy->smapi); XFree((char *) info); diff --git a/src/gallium/state_trackers/glx/xlib/xm_st.c b/src/gallium/state_trackers/glx/xlib/xm_st.c index 6a0f4aad7d8..0c42e653c76 100644 --- a/src/gallium/state_trackers/glx/xlib/xm_st.c +++ b/src/gallium/state_trackers/glx/xlib/xm_st.c @@ -304,6 +304,7 @@ xmesa_create_st_framebuffer(XMesaDisplay xmdpy, XMesaBuffer b) stfbi->flush_front = xmesa_st_framebuffer_flush_front; stfbi->validate = xmesa_st_framebuffer_validate; stfbi->ID = p_atomic_inc_return(&xmesa_stfbi_ID); + stfbi->state_manager = xmdpy->smapi; p_atomic_set(&stfbi->stamp, 1); stfbi->st_manager_private = (void *) xstfb; diff --git a/src/gallium/state_trackers/wgl/stw_device.c b/src/gallium/state_trackers/wgl/stw_device.c index 6c0f14d4f36..b88e1100ebb 100644 --- a/src/gallium/state_trackers/wgl/stw_device.c +++ b/src/gallium/state_trackers/wgl/stw_device.c @@ -199,6 +199,9 @@ stw_cleanup(void) DeleteCriticalSection(&stw_dev->fb_mutex); DeleteCriticalSection(&stw_dev->ctx_mutex); + if (stw_dev->smapi->destroy) + stw_dev->smapi->destroy(stw_dev->smapi); + FREE(stw_dev->smapi); stw_dev->stapi->destroy(stw_dev->stapi); diff --git a/src/gallium/state_trackers/wgl/stw_st.c b/src/gallium/state_trackers/wgl/stw_st.c index 85a8b1743ea..5e165c89f56 100644 --- a/src/gallium/state_trackers/wgl/stw_st.c +++ b/src/gallium/state_trackers/wgl/stw_st.c @@ -235,6 +235,7 @@ stw_st_create_framebuffer(struct stw_framebuffer *fb) stwfb->fb = fb; stwfb->stvis = fb->pfi->stvis; stwfb->base.ID = p_atomic_inc_return(&stwfb_ID); + stwfb->base.state_manager = stw_dev->smapi; stwfb->base.visual = &stwfb->stvis; p_atomic_set(&stwfb->base.stamp, 1); diff --git a/src/mesa/state_tracker/st_manager.c b/src/mesa/state_tracker/st_manager.c index ebc7ca8b133..834bcc9f8c2 100644 --- a/src/mesa/state_tracker/st_manager.c +++ b/src/mesa/state_tracker/st_manager.c @@ -61,9 +61,13 @@ #include "util/list.h" struct hash_table; -static struct hash_table *st_fbi_ht; /* framebuffer iface objects hash table */ +struct st_manager_private +{ + struct hash_table *stfbi_ht; /* framebuffer iface objects hash table */ + mtx_t st_mutex; +}; -static mtx_t st_mutex = _MTX_INITIALIZER_NP; +static void st_manager_destroy(struct st_manager *); /** * Map an attachment to a buffer index. @@ -511,45 +515,63 @@ st_framebuffer_iface_equal(const void *a, const void *b) static boolean -st_framebuffer_iface_lookup(const struct st_framebuffer_iface *stfbi) +st_framebuffer_iface_lookup(struct st_manager *smapi, + const struct st_framebuffer_iface *stfbi) { + struct st_manager_private *smPriv = + (struct st_manager_private *)smapi->st_manager_private; struct hash_entry *entry; - mtx_lock(&st_mutex); - entry = _mesa_hash_table_search(st_fbi_ht, stfbi); - mtx_unlock(&st_mutex); + assert(smPriv); + assert(smPriv->stfbi_ht); + + mtx_lock(&smPriv->st_mutex); + entry = _mesa_hash_table_search(smPriv->stfbi_ht, stfbi); + mtx_unlock(&smPriv->st_mutex); return entry != NULL; } static boolean -st_framebuffer_iface_insert(struct st_framebuffer_iface *stfbi) +st_framebuffer_iface_insert(struct st_manager *smapi, + struct st_framebuffer_iface *stfbi) { + struct st_manager_private *smPriv = + (struct st_manager_private *)smapi->st_manager_private; struct hash_entry *entry; - mtx_lock(&st_mutex); - entry = _mesa_hash_table_insert(st_fbi_ht, stfbi, stfbi); - mtx_unlock(&st_mutex); + assert(smPriv); + assert(smPriv->stfbi_ht); + + mtx_lock(&smPriv->st_mutex); + entry = _mesa_hash_table_insert(smPriv->stfbi_ht, stfbi, stfbi); + mtx_unlock(&smPriv->st_mutex); return entry != NULL; } static void -st_framebuffer_iface_remove(struct st_framebuffer_iface *stfbi) +st_framebuffer_iface_remove(struct st_manager *smapi, + struct st_framebuffer_iface *stfbi) { + struct st_manager_private *smPriv = + (struct st_manager_private *)smapi->st_manager_private; struct hash_entry *entry; - mtx_lock(&st_mutex); - entry = _mesa_hash_table_search(st_fbi_ht, stfbi); + if (!smPriv || !smPriv->stfbi_ht); + return; + + mtx_lock(&smPriv->st_mutex); + entry = _mesa_hash_table_search(smPriv->stfbi_ht, stfbi); if (!entry) goto unlock; - _mesa_hash_table_remove(st_fbi_ht, entry); + _mesa_hash_table_remove(smPriv->stfbi_ht, entry); unlock: - mtx_unlock(&st_mutex); + mtx_unlock(&smPriv->st_mutex); } @@ -561,7 +583,10 @@ static void st_api_destroy_drawable(struct st_api *stapi, struct st_framebuffer_iface *stfbi) { - st_framebuffer_iface_remove(stfbi); + if (stfbi) + return; + + st_framebuffer_iface_remove(stfbi->state_manager, stfbi); } @@ -572,16 +597,24 @@ st_api_destroy_drawable(struct st_api *stapi, static void st_framebuffers_purge(struct st_context *st) { + struct st_context_iface *st_iface = &st->iface; + struct st_manager *smapi = st_iface->state_manager; struct st_framebuffer *stfb, *next; + assert(smapi); + LIST_FOR_EACH_ENTRY_SAFE_REV(stfb, next, &st->winsys_buffers, head) { + struct st_framebuffer_iface *stfbi = stfb->iface; + + assert(stfbi); + /** * If the corresponding framebuffer interface object no longer exists, * remove the framebuffer object from the context's winsys buffers list, * and unreference the framebuffer object, so its resources can be * deleted. */ - if (!st_framebuffer_iface_lookup(stfb->iface)) { + if (!st_framebuffer_iface_lookup(smapi, stfbi)) { LIST_DEL(&stfb->head); st_framebuffer_reference(&stfb, NULL); } @@ -778,6 +811,21 @@ st_api_create_context(struct st_api *stapi, struct st_manager *smapi, return NULL; } + /* Create a hash table for the framebuffer interface objects + * if it has not been created for this st manager. + */ + if (smapi->st_manager_private == NULL) { + struct st_manager_private *smPriv; + + smPriv = CALLOC_STRUCT(st_manager_private); + mtx_init(&smPriv->st_mutex, mtx_plain); + smPriv->stfbi_ht = _mesa_hash_table_create(NULL, + st_framebuffer_iface_hash, + st_framebuffer_iface_equal); + smapi->st_manager_private = smPriv; + smapi->destroy = st_manager_destroy; + } + if (attribs->flags & ST_CONTEXT_FLAG_ROBUST_ACCESS) ctx_flags |= PIPE_CONTEXT_ROBUST_BUFFER_ACCESS; @@ -846,6 +894,7 @@ st_api_create_context(struct st_api *stapi, struct st_manager *smapi, st->iface.st_context_private = (void *) smapi; st->iface.cso_context = st->cso_context; st->iface.pipe = st->pipe; + st->iface.state_manager = smapi; *error = ST_CONTEXT_SUCCESS; return &st->iface; @@ -888,7 +937,7 @@ st_framebuffer_reuse_or_create(struct st_context *st, /* add the referenced framebuffer interface object to * the framebuffer interface object hash table. */ - if (!st_framebuffer_iface_insert(stfbi)) { + if (!st_framebuffer_iface_insert(stfbi->state_manager, stfbi)) { st_framebuffer_reference(&cur, NULL); return NULL; } @@ -964,8 +1013,6 @@ st_api_make_current(struct st_api *stapi, struct st_context_iface *stctxi, static void st_api_destroy(struct st_api *stapi) { - _mesa_hash_table_destroy(st_fbi_ht, NULL); - mtx_destroy(&st_mutex); } /** @@ -1051,6 +1098,19 @@ st_manager_add_color_renderbuffer(struct st_context *st, return TRUE; } +static void +st_manager_destroy(struct st_manager *smapi) +{ + struct st_manager_private *smPriv = smapi->st_manager_private; + + if (smPriv && smPriv->stfbi_ht) { + _mesa_hash_table_destroy(smPriv->stfbi_ht, NULL); + mtx_destroy(&smPriv->st_mutex); + free(smPriv); + smapi->st_manager_private = NULL; + } +} + static unsigned get_version(struct pipe_screen *screen, struct st_config_options *options, gl_api api) @@ -1106,12 +1166,5 @@ static const struct st_api st_gl_api = { struct st_api * st_gl_api_create(void) { - /* Create a hash table for all the framebuffer interface objects */ - - mtx_init(&st_mutex, mtx_plain); - st_fbi_ht = _mesa_hash_table_create(NULL, - st_framebuffer_iface_hash, - st_framebuffer_iface_equal); - return (struct st_api *) &st_gl_api; } -- 2.30.2