[Mesa-dev] [PATCH] st/mesa: add destroy_drawable interface
Marek Olšák
maraeo at gmail.com
Mon Jul 24 22:16:07 UTC 2017
Hi Charmaine,
Which commits do I need to revert in 17.2 to unbreak Steam on Mesa? I
guess it's more than one.
Thanks,
Marek
On Sat, Jul 22, 2017 at 3:05 AM, Charmaine Lee <charmainel at vmware.com> wrote:
>
> Hi Christoph,
>
> Can you provide an apitrace of the test that crashes?
>
> Thanks.
> -Charmaine
>
> ________________________________________
> From: mesa-dev <mesa-dev-bounces at lists.freedesktop.org> on behalf of Christoph Haag <haagch+mesadev at frickel.club>
> Sent: Friday, July 21, 2017 4:52:41 PM
> To: mesa-dev at lists.freedesktop.org
> Subject: Re: [Mesa-dev] [PATCH] st/mesa: add destroy_drawable interface
>
> This patch breaks steam for me. Segfault backtrace:
>
> #0 0x00000023 in ?? ()
> No symbol table info available.
> #1 0xf6b1a417 in _mesa_hash_table_search (ht=0x585bb7e8, key=0x5820ee88) at hash_table.c:246
> __PRETTY_FUNCTION__ = "_mesa_hash_table_search"
> #2 0xf6a4488e in st_framebuffer_iface_remove (stfbi=0x5820ee88) at state_tracker/st_manager.c:545
> entry = 0xf6bb3038 <swap_fences_unref+86>
> #3 0xf6a448e5 in st_api_destroy_drawable (stapi=0xf7212bc0 <st_gl_api>, stfbi=0x5820ee88) at state_tracker/st_manager.c:564
> No locals.
> #4 0xf6bb2b64 in dri_destroy_buffer (dPriv=0x57d62560) at dri_drawable.c:186
> drawable = 0x5820ee88
> screen = 0x57e1a0a8
> stapi = 0xf7212bc0 <st_gl_api>
> i = 7
> #5 0xf6bb132c in dri_put_drawable (pdp=0x57d62560) at dri_util.c:642
> No locals.
> #6 0xf6bb145c in driDestroyDrawable (pdp=0x57d62560) at dri_util.c:695
> No locals.
> #7 0xf759249a in loader_dri3_drawable_fini (draw=0x581fb0f0) at loader_dri3_helper.c:109
> i = 1478471608
> #8 0xf758ca73 in dri3_destroy_drawable (base=0x581fb0d0) at dri3_glx.c:366
> pdraw = 0x581fb0d0
> #9 0xf7583b5a in driReleaseDrawables (gc=0x57e19ef8) at dri_common.c:452
> priv = 0x57e082d0
> pdraw = 0x581fb0d0
> #10 0xf758c679 in dri3_bind_context (context=0x57e19ef8, old=0x57e19ef8, draw=165675047, read=165675047) at dri3_glx.c:223
> pcp = 0x57e19ef8
> psc = 0x57e07608
> pdraw = 0x5866eb28
> pread = 0x5866eb28
> dri_draw = 0x0
> dri_read = 0x0
> #11 0xf754cd12 in MakeContextCurrent (dpy=0x57c16f30, draw=165675047, read=165675047, gc_user=0x57e19ef8) at glxcurrent.c:228
> gc = 0x57e19ef8
> oldGC = 0x57e19ef8
> #12 0xf754ce75 in glXMakeCurrent (dpy=0x57c16f30, draw=165675047, gc=0x57e19ef8) at glxcurrent.c:274
> No locals.
> #13 0xeb48addb in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/vgui2_s.so
> No symbol table info available.
> #14 0xeb48dd0e in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/vgui2_s.so
> No symbol table info available.
> #15 0xeb49d81d in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/vgui2_s.so
> No symbol table info available.
> #16 0xefd74193 in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/steamui.so
> No symbol table info available.
> #17 0xefd76946 in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/steamui.so
> No symbol table info available.
> #18 0xefd77f16 in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/steamui.so
> No symbol table info available.
> #19 0x5663d660 in RunSteam(int, char**, bool) ()
> No symbol table info available.
> #20 0x5663e5f3 in ?? ()
> No symbol table info available.
> #21 0x56629aac in ?? ()
> No symbol table info available.
> #22 0xf799a1d3 in __libc_start_main () from /usr/lib32/libc.so.6
> No symbol table info available.
> #23 0x5662d159 in _start ()
> No symbol table info available.
>
>
> On 20.07.2017 20:26, Charmaine Lee wrote:
>> With this patch, the st manager will maintain a hash table for
>> the active framebuffer interface objects. A destroy_drawable interface
>> is added to allow the state tracker to notify the st manager to remove
>> the associated framebuffer interface object from the hash table,
>> so the associated framebuffer and its resources can be deleted
>> at framebuffers purge time.
>>
>> Fixes bug 101829 "read-after-free in st_framebuffer_validate"
>>
>> Tested-by: Brad King <brad.king at kitware.com>
>> Tested-by: Gert Wollny <gw.fossdev at gmail.com>
>> ---
>> src/gallium/include/state_tracker/st_api.h | 7 ++
>> src/gallium/state_trackers/dri/dri_drawable.c | 6 +-
>> src/gallium/state_trackers/glx/xlib/xm_api.c | 5 ++
>> src/gallium/state_trackers/glx/xlib/xm_st.c | 2 +
>> src/gallium/state_trackers/wgl/stw_st.c | 6 +-
>> src/mesa/state_tracker/st_manager.c | 95 ++++++++++++++++++++++++++-
>> src/mesa/state_tracker/st_manager.h | 5 ++
>> 7 files changed, 123 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/gallium/include/state_tracker/st_api.h b/src/gallium/include/state_tracker/st_api.h
>> index 30a4866..9b660f7 100644
>> --- a/src/gallium/include/state_tracker/st_api.h
>> +++ b/src/gallium/include/state_tracker/st_api.h
>> @@ -552,6 +552,13 @@ struct st_api
>> * Get the currently bound context in the calling thread.
>> */
>> struct st_context_iface *(*get_current)(struct st_api *stapi);
>> +
>> + /**
>> + * Notify the st manager the framebuffer interface object
>> + * is no longer valid.
>> + */
>> + void (*destroy_drawable)(struct st_api *stapi,
>> + struct st_framebuffer_iface *stfbi);
>> };
>>
>> /**
>> diff --git a/src/gallium/state_trackers/dri/dri_drawable.c b/src/gallium/state_trackers/dri/dri_drawable.c
>> index 0cfdc30..c7df0f6 100644
>> --- a/src/gallium/state_trackers/dri/dri_drawable.c
>> +++ b/src/gallium/state_trackers/dri/dri_drawable.c
>> @@ -169,6 +169,8 @@ void
>> dri_destroy_buffer(__DRIdrawable * dPriv)
>> {
>> struct dri_drawable *drawable = dri_drawable(dPriv);
>> + struct dri_screen *screen = drawable->screen;
>> + struct st_api *stapi = screen->st_api;
>> int i;
>>
>> pipe_surface_reference(&drawable->drisw_surface, NULL);
>> @@ -180,7 +182,9 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
>>
>> swap_fences_unref(drawable);
>>
>> - drawable->base.ID = 0;
>> + /* Notify the st manager that this drawable is no longer valid */
>> + stapi->destroy_drawable(stapi, &drawable->base);
>> +
>> FREE(drawable);
>> }
>>
>> diff --git a/src/gallium/state_trackers/glx/xlib/xm_api.c b/src/gallium/state_trackers/glx/xlib/xm_api.c
>> index 881dd44..e4b1e9d 100644
>> --- a/src/gallium/state_trackers/glx/xlib/xm_api.c
>> +++ b/src/gallium/state_trackers/glx/xlib/xm_api.c
>> @@ -595,6 +595,11 @@ xmesa_free_buffer(XMesaBuffer buffer)
>> */
>> b->ws.drawable = 0;
>>
>> + /* Notify the st manager that the associated framebuffer interface
>> + * object is no longer valid.
>> + */
>> + stapi->destroy_drawable(stapi, buffer->stfb);
>> +
>> /* XXX we should move the buffer to a delete-pending list and destroy
>> * the buffer until it is no longer current.
>> */
>> diff --git a/src/gallium/state_trackers/glx/xlib/xm_st.c b/src/gallium/state_trackers/glx/xlib/xm_st.c
>> index 9e30efa..6a0f4aa 100644
>> --- a/src/gallium/state_trackers/glx/xlib/xm_st.c
>> +++ b/src/gallium/state_trackers/glx/xlib/xm_st.c
>> @@ -273,6 +273,7 @@ xmesa_st_framebuffer_flush_front(struct st_context_iface *stctx,
>> return ret;
>> }
>>
>> +static uint32_t xmesa_stfbi_ID = 0;
>>
>> struct st_framebuffer_iface *
>> xmesa_create_st_framebuffer(XMesaDisplay xmdpy, XMesaBuffer b)
>> @@ -302,6 +303,7 @@ xmesa_create_st_framebuffer(XMesaDisplay xmdpy, XMesaBuffer b)
>> stfbi->visual = &xstfb->stvis;
>> stfbi->flush_front = xmesa_st_framebuffer_flush_front;
>> stfbi->validate = xmesa_st_framebuffer_validate;
>> + stfbi->ID = p_atomic_inc_return(&xmesa_stfbi_ID);
>> p_atomic_set(&stfbi->stamp, 1);
>> stfbi->st_manager_private = (void *) xstfb;
>>
>> diff --git a/src/gallium/state_trackers/wgl/stw_st.c b/src/gallium/state_trackers/wgl/stw_st.c
>> index c2844b0..85a8b17 100644
>> --- a/src/gallium/state_trackers/wgl/stw_st.c
>> +++ b/src/gallium/state_trackers/wgl/stw_st.c
>> @@ -256,7 +256,11 @@ stw_st_destroy_framebuffer_locked(struct st_framebuffer_iface *stfb)
>> for (i = 0; i < ST_ATTACHMENT_COUNT; i++)
>> pipe_resource_reference(&stwfb->textures[i], NULL);
>>
>> - stwfb->base.ID = 0;
>> + /* Notify the st manager that the framebuffer interface is no
>> + * longer valid.
>> + */
>> + stw_dev->stapi->destroy_drawable(stw_dev->stapi, &stwfb->base);
>> +
>> FREE(stwfb);
>> }
>>
>> diff --git a/src/mesa/state_tracker/st_manager.c b/src/mesa/state_tracker/st_manager.c
>> index cb816de..ebc7ca8 100644
>> --- a/src/mesa/state_tracker/st_manager.c
>> +++ b/src/mesa/state_tracker/st_manager.c
>> @@ -38,6 +38,7 @@
>> #include "main/fbobject.h"
>> #include "main/renderbuffer.h"
>> #include "main/version.h"
>> +#include "util/hash_table.h"
>> #include "st_texture.h"
>>
>> #include "st_context.h"
>> @@ -59,6 +60,10 @@
>> #include "util/u_surface.h"
>> #include "util/list.h"
>>
>> +struct hash_table;
>> +static struct hash_table *st_fbi_ht; /* framebuffer iface objects hash table */
>> +
>> +static mtx_t st_mutex = _MTX_INITIALIZER_NP;
>>
>> /**
>> * Map an attachment to a buffer index.
>> @@ -490,6 +495,76 @@ st_framebuffer_reference(struct st_framebuffer **ptr,
>> _mesa_reference_framebuffer((struct gl_framebuffer **) ptr, fb);
>> }
>>
>> +
>> +static uint32_t
>> +st_framebuffer_iface_hash(const void *key)
>> +{
>> + return (uintptr_t)key;
>> +}
>> +
>> +
>> +static bool
>> +st_framebuffer_iface_equal(const void *a, const void *b)
>> +{
>> + return (struct st_framebuffer_iface *)a == (struct st_framebuffer_iface *)b;
>> +}
>> +
>> +
>> +static boolean
>> +st_framebuffer_iface_lookup(const struct st_framebuffer_iface *stfbi)
>> +{
>> + struct hash_entry *entry;
>> +
>> + mtx_lock(&st_mutex);
>> + entry = _mesa_hash_table_search(st_fbi_ht, stfbi);
>> + mtx_unlock(&st_mutex);
>> +
>> + return entry != NULL;
>> +}
>> +
>> +
>> +static boolean
>> +st_framebuffer_iface_insert(struct st_framebuffer_iface *stfbi)
>> +{
>> + struct hash_entry *entry;
>> +
>> + mtx_lock(&st_mutex);
>> + entry = _mesa_hash_table_insert(st_fbi_ht, stfbi, stfbi);
>> + mtx_unlock(&st_mutex);
>> +
>> + return entry != NULL;
>> +}
>> +
>> +
>> +static void
>> +st_framebuffer_iface_remove(struct st_framebuffer_iface *stfbi)
>> +{
>> + struct hash_entry *entry;
>> +
>> + mtx_lock(&st_mutex);
>> + entry = _mesa_hash_table_search(st_fbi_ht, stfbi);
>> + if (!entry)
>> + goto unlock;
>> +
>> + _mesa_hash_table_remove(st_fbi_ht, entry);
>> +
>> +unlock:
>> + mtx_unlock(&st_mutex);
>> +}
>> +
>> +
>> +/**
>> + * The framebuffer interface object is no longer valid.
>> + * Remove the object from the framebuffer interface hash table.
>> + */
>> +static void
>> +st_api_destroy_drawable(struct st_api *stapi,
>> + struct st_framebuffer_iface *stfbi)
>> +{
>> + st_framebuffer_iface_remove(stfbi);
>> +}
>> +
>> +
>> /**
>> * Purge the winsys buffers list to remove any references to
>> * non-existing framebuffer interface objects.
>> @@ -506,7 +581,7 @@ st_framebuffers_purge(struct st_context *st)
>> * and unreference the framebuffer object, so its resources can be
>> * deleted.
>> */
>> - if (stfb->iface->ID != stfb->iface_ID) {
>> + if (!st_framebuffer_iface_lookup(stfb->iface)) {
>> LIST_DEL(&stfb->head);
>> st_framebuffer_reference(&stfb, NULL);
>> }
>> @@ -810,6 +885,14 @@ st_framebuffer_reuse_or_create(struct st_context *st,
>> cur = st_framebuffer_create(st, stfbi);
>>
>> if (cur) {
>> + /* add the referenced framebuffer interface object to
>> + * the framebuffer interface object hash table.
>> + */
>> + if (!st_framebuffer_iface_insert(stfbi)) {
>> + st_framebuffer_reference(&cur, NULL);
>> + return NULL;
>> + }
>> +
>> /* add to the context's winsys buffers list */
>> LIST_ADD(&cur->head, &st->winsys_buffers);
>>
>> @@ -881,6 +964,8 @@ 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);
>> }
>>
>> /**
>> @@ -1015,10 +1100,18 @@ static const struct st_api st_gl_api = {
>> .create_context = st_api_create_context,
>> .make_current = st_api_make_current,
>> .get_current = st_api_get_current,
>> + .destroy_drawable = st_api_destroy_drawable,
>> };
>>
>> 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;
>> }
>> diff --git a/src/mesa/state_tracker/st_manager.h b/src/mesa/state_tracker/st_manager.h
>> index 68adf2f..c54f29e 100644
>> --- a/src/mesa/state_tracker/st_manager.h
>> +++ b/src/mesa/state_tracker/st_manager.h
>> @@ -34,6 +34,7 @@
>>
>> struct st_context;
>> struct st_framebuffer;
>> +struct st_framebuffer_interface;
>>
>> void
>> st_manager_flush_frontbuffer(struct st_context *st);
>> @@ -48,4 +49,8 @@ st_manager_add_color_renderbuffer(struct st_context *st, struct gl_framebuffer *
>> void
>> st_framebuffer_reference(struct st_framebuffer **ptr,
>> struct st_framebuffer *stfb);
>> +
>> +void
>> +st_framebuffer_interface_destroy(struct st_framebuffer_interface *stfbi);
>> +
>> #endif /* ST_MANAGER_H */
>>
>
> _______________________________________________
> 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