Re: [Mesa-dev] [PATCH] st/mesa: fix texture deletion context mix-up issues (v2)
On 03/23/2019 10:49 AM, Jose Fonseca wrote: Looks good to me. Reviewed-by: Jose Fonseca Though I wonder if this could happen also when not destroying the current context. (Ie, if we need zoombie textures too?) If we're not destroying the thread's current context, this patch temporarily binds the context as the current one. If the contexts textures are not shared, they'll be deleted. If they are shared, they won't be deleted. I think that part is fairly straight-forward. -Brian Jose *From:* Brian Paul *Sent:* Friday, March 22, 2019 19:51 *To:* mesa-dev@lists.freedesktop.org *Cc:* Jose Fonseca; Neha Bhende *Subject:* [PATCH] st/mesa: fix texture deletion context mix-up issues (v2) When we destroy a context, we need to temporarily make that context the current one for the thread. That's because during context tear-down we make many calls to _mesa_reference_texobj(, NULL). Note there's no context parameter. If the texture's refcount goes to zero and we need to delete it, we use the thread's current context. But if that context isn't the context we're tearing down, we get into trouble when deallocating sampler views. See patch 593e36f956 ("st/mesa: implement "zombie" sampler views (v2)") for background information. Also, we need to release any sampler views attached to the fallback textures. Fixes a crash on exit with a glretrace of the Nobel Clinician application. v2: at end of st_destroy_context(), check if save_ctx == ctx and unbind the context if so. --- src/mesa/state_tracker/st_context.c | 51 - 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c index f037384..09d467a 100644 --- a/src/mesa/state_tracker/st_context.c +++ b/src/mesa/state_tracker/st_context.c @@ -917,15 +917,39 @@ st_destroy_context(struct st_context *st) { struct gl_context *ctx = st->ctx; struct st_framebuffer *stfb, *next; + struct gl_framebuffer *save_drawbuffer; + struct gl_framebuffer *save_readbuffer; + + /* Save the current context and draw/read buffers*/ + GET_CURRENT_CONTEXT(save_ctx); + if (save_ctx) { + save_drawbuffer = save_ctx->WinSysDrawBuffer; + save_readbuffer = save_ctx->WinSysReadBuffer; + } else { + save_drawbuffer = save_readbuffer = NULL; + } - GET_CURRENT_CONTEXT(curctx); + /* + * We need to bind the context we're deleting so that + * _mesa_reference_texobj_() uses this context when deleting textures. + * Similarly for framebuffer objects, etc. + */ + _mesa_make_current(ctx, NULL, NULL); - if (curctx == NULL) { - /* No current context, but we need one to release - * renderbuffer surface when we release framebuffer. - * So temporarily bind the context. - */ - _mesa_make_current(ctx, NULL, NULL); + /* This must be called first so that glthread has a chance to finish */ + _mesa_glthread_destroy(ctx); + + _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); + + /* For the fallback textures, free any sampler views belonging to this + * context. + */ + for (unsigned i = 0; i < NUM_TEXTURE_TARGETS; i++) { + struct st_texture_object *stObj = + st_texture_object(ctx->Shared->FallbackTex[i]); + if (stObj) { + st_texture_release_context_sampler_view(st, stObj); + } } st_context_free_zombie_objects(st); @@ -933,11 +957,6 @@ st_destroy_context(struct st_context *st) mtx_destroy(>zombie_sampler_views.mutex); mtx_destroy(>zombie_shaders.mutex); - /* This must be called first so that glthread has a chance to finish */ - _mesa_glthread_destroy(ctx); - - _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); - st_reference_fragprog(st, >fp, NULL); st_reference_prog(st, >gp, NULL); st_reference_vertprog(st, >vp, NULL); @@ -965,4 +984,12 @@ st_destroy_context(struct st_context *st) st = NULL; free(ctx); + + if (save_ctx == ctx) { + /* unbind the context we just deleted */ + _mesa_make_current(NULL, NULL, NULL); + } else { + /* Restore the current context and draw/read buffers (may be NULL) */ + _mesa_make_current(save_ctx, save_drawbuffer, save_readbuffer); + } } -- 1.8.5.6 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: fix texture deletion context mix-up issues (v2)
On 25/03/2019 14:26, Brian Paul wrote: > On 03/23/2019 10:49 AM, Jose Fonseca wrote: >> Looks good to me. >> >> Reviewed-by: Jose Fonseca >> >> Though I wonder if this could happen also when not destroying the >> current context. (Ie, if we need zoombie textures too?) > > If we're not destroying the thread's current context, this patch > temporarily binds the context as the current one. If the contexts > textures are not shared, they'll be deleted. If they are shared, they > won't be deleted. I think that part is fairly straight-forward. OK. Thinking more about it, we only need zombies for the sample views, not the textures themselves. Jose ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: fix texture deletion context mix-up issues (v2)
Looks good to me. Reviewed-by: Neha Bhende Regards, Neha From: mesa-dev on behalf of Brian Paul Sent: Friday, March 22, 2019 12:51 PM To: mesa-dev@lists.freedesktop.org Cc: Neha Bhende Subject: [Mesa-dev] [PATCH] st/mesa: fix texture deletion context mix-up issues (v2) When we destroy a context, we need to temporarily make that context the current one for the thread. That's because during context tear-down we make many calls to _mesa_reference_texobj(, NULL). Note there's no context parameter. If the texture's refcount goes to zero and we need to delete it, we use the thread's current context. But if that context isn't the context we're tearing down, we get into trouble when deallocating sampler views. See patch 593e36f956 ("st/mesa: implement "zombie" sampler views (v2)") for background information. Also, we need to release any sampler views attached to the fallback textures. Fixes a crash on exit with a glretrace of the Nobel Clinician application. v2: at end of st_destroy_context(), check if save_ctx == ctx and unbind the context if so. --- src/mesa/state_tracker/st_context.c | 51 - 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c index f037384..09d467a 100644 --- a/src/mesa/state_tracker/st_context.c +++ b/src/mesa/state_tracker/st_context.c @@ -917,15 +917,39 @@ st_destroy_context(struct st_context *st) { struct gl_context *ctx = st->ctx; struct st_framebuffer *stfb, *next; + struct gl_framebuffer *save_drawbuffer; + struct gl_framebuffer *save_readbuffer; + + /* Save the current context and draw/read buffers*/ + GET_CURRENT_CONTEXT(save_ctx); + if (save_ctx) { + save_drawbuffer = save_ctx->WinSysDrawBuffer; + save_readbuffer = save_ctx->WinSysReadBuffer; + } else { + save_drawbuffer = save_readbuffer = NULL; + } - GET_CURRENT_CONTEXT(curctx); + /* +* We need to bind the context we're deleting so that +* _mesa_reference_texobj_() uses this context when deleting textures. +* Similarly for framebuffer objects, etc. +*/ + _mesa_make_current(ctx, NULL, NULL); - if (curctx == NULL) { - /* No current context, but we need one to release - * renderbuffer surface when we release framebuffer. - * So temporarily bind the context. - */ - _mesa_make_current(ctx, NULL, NULL); + /* This must be called first so that glthread has a chance to finish */ + _mesa_glthread_destroy(ctx); + + _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); + + /* For the fallback textures, free any sampler views belonging to this +* context. +*/ + for (unsigned i = 0; i < NUM_TEXTURE_TARGETS; i++) { + struct st_texture_object *stObj = + st_texture_object(ctx->Shared->FallbackTex[i]); + if (stObj) { + st_texture_release_context_sampler_view(st, stObj); + } } st_context_free_zombie_objects(st); @@ -933,11 +957,6 @@ st_destroy_context(struct st_context *st) mtx_destroy(>zombie_sampler_views.mutex); mtx_destroy(>zombie_shaders.mutex); - /* This must be called first so that glthread has a chance to finish */ - _mesa_glthread_destroy(ctx); - - _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); - st_reference_fragprog(st, >fp, NULL); st_reference_prog(st, >gp, NULL); st_reference_vertprog(st, >vp, NULL); @@ -965,4 +984,12 @@ st_destroy_context(struct st_context *st) st = NULL; free(ctx); + + if (save_ctx == ctx) { + /* unbind the context we just deleted */ + _mesa_make_current(NULL, NULL, NULL); + } else { + /* Restore the current context and draw/read buffers (may be NULL) */ + _mesa_make_current(save_ctx, save_drawbuffer, save_readbuffer); + } } -- 1.8.5.6 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fmesa-devdata=02%7C01%7Cbhenden%40vmware.com%7Ce45f81c0931c4632b08f08d6af001248%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C63612186127578sdata=tJMl2feeW7EsZQcvpFWKvifdAAF7GBY43%2FYTdUAXWL8%3Dreserved=0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: fix texture deletion context mix-up issues (v2)
Looks good to me. Reviewed-by: Jose Fonseca Though I wonder if this could happen also when not destroying the current context. (Ie, if we need zoombie textures too?) Jose From: Brian Paul Sent: Friday, March 22, 2019 19:51 To: mesa-dev@lists.freedesktop.org Cc: Jose Fonseca; Neha Bhende Subject: [PATCH] st/mesa: fix texture deletion context mix-up issues (v2) When we destroy a context, we need to temporarily make that context the current one for the thread. That's because during context tear-down we make many calls to _mesa_reference_texobj(, NULL). Note there's no context parameter. If the texture's refcount goes to zero and we need to delete it, we use the thread's current context. But if that context isn't the context we're tearing down, we get into trouble when deallocating sampler views. See patch 593e36f956 ("st/mesa: implement "zombie" sampler views (v2)") for background information. Also, we need to release any sampler views attached to the fallback textures. Fixes a crash on exit with a glretrace of the Nobel Clinician application. v2: at end of st_destroy_context(), check if save_ctx == ctx and unbind the context if so. --- src/mesa/state_tracker/st_context.c | 51 - 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c index f037384..09d467a 100644 --- a/src/mesa/state_tracker/st_context.c +++ b/src/mesa/state_tracker/st_context.c @@ -917,15 +917,39 @@ st_destroy_context(struct st_context *st) { struct gl_context *ctx = st->ctx; struct st_framebuffer *stfb, *next; + struct gl_framebuffer *save_drawbuffer; + struct gl_framebuffer *save_readbuffer; + + /* Save the current context and draw/read buffers*/ + GET_CURRENT_CONTEXT(save_ctx); + if (save_ctx) { + save_drawbuffer = save_ctx->WinSysDrawBuffer; + save_readbuffer = save_ctx->WinSysReadBuffer; + } else { + save_drawbuffer = save_readbuffer = NULL; + } - GET_CURRENT_CONTEXT(curctx); + /* +* We need to bind the context we're deleting so that +* _mesa_reference_texobj_() uses this context when deleting textures. +* Similarly for framebuffer objects, etc. +*/ + _mesa_make_current(ctx, NULL, NULL); - if (curctx == NULL) { - /* No current context, but we need one to release - * renderbuffer surface when we release framebuffer. - * So temporarily bind the context. - */ - _mesa_make_current(ctx, NULL, NULL); + /* This must be called first so that glthread has a chance to finish */ + _mesa_glthread_destroy(ctx); + + _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); + + /* For the fallback textures, free any sampler views belonging to this +* context. +*/ + for (unsigned i = 0; i < NUM_TEXTURE_TARGETS; i++) { + struct st_texture_object *stObj = + st_texture_object(ctx->Shared->FallbackTex[i]); + if (stObj) { + st_texture_release_context_sampler_view(st, stObj); + } } st_context_free_zombie_objects(st); @@ -933,11 +957,6 @@ st_destroy_context(struct st_context *st) mtx_destroy(>zombie_sampler_views.mutex); mtx_destroy(>zombie_shaders.mutex); - /* This must be called first so that glthread has a chance to finish */ - _mesa_glthread_destroy(ctx); - - _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); - st_reference_fragprog(st, >fp, NULL); st_reference_prog(st, >gp, NULL); st_reference_vertprog(st, >vp, NULL); @@ -965,4 +984,12 @@ st_destroy_context(struct st_context *st) st = NULL; free(ctx); + + if (save_ctx == ctx) { + /* unbind the context we just deleted */ + _mesa_make_current(NULL, NULL, NULL); + } else { + /* Restore the current context and draw/read buffers (may be NULL) */ + _mesa_make_current(save_ctx, save_drawbuffer, save_readbuffer); + } } -- 1.8.5.6 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/mesa: fix texture deletion context mix-up issues (v2)
Looks alright to me, it's all quite tricky stuff... Reviewed-by: Roland Scheidegger Am 22.03.19 um 20:51 schrieb Brian Paul: > When we destroy a context, we need to temporarily make that context > the current one for the thread. > > That's because during context tear-down we make many calls to > _mesa_reference_texobj(, NULL). Note there's no context > parameter. If the texture's refcount goes to zero and we need to > delete it, we use the thread's current context. But if that context > isn't the context we're tearing down, we get into trouble when > deallocating sampler views. See patch 593e36f956 ("st/mesa: > implement "zombie" sampler views (v2)") for background information. > > Also, we need to release any sampler views attached to the fallback > textures. > > Fixes a crash on exit with a glretrace of the Nobel Clinician > application. > > v2: at end of st_destroy_context(), check if save_ctx == ctx and > unbind the context if so. > --- > src/mesa/state_tracker/st_context.c | 51 > - > 1 file changed, 39 insertions(+), 12 deletions(-) > > diff --git a/src/mesa/state_tracker/st_context.c > b/src/mesa/state_tracker/st_context.c > index f037384..09d467a 100644 > --- a/src/mesa/state_tracker/st_context.c > +++ b/src/mesa/state_tracker/st_context.c > @@ -917,15 +917,39 @@ st_destroy_context(struct st_context *st) > { > struct gl_context *ctx = st->ctx; > struct st_framebuffer *stfb, *next; > + struct gl_framebuffer *save_drawbuffer; > + struct gl_framebuffer *save_readbuffer; > + > + /* Save the current context and draw/read buffers*/ > + GET_CURRENT_CONTEXT(save_ctx); > + if (save_ctx) { > + save_drawbuffer = save_ctx->WinSysDrawBuffer; > + save_readbuffer = save_ctx->WinSysReadBuffer; > + } else { > + save_drawbuffer = save_readbuffer = NULL; > + } > > - GET_CURRENT_CONTEXT(curctx); > + /* > +* We need to bind the context we're deleting so that > +* _mesa_reference_texobj_() uses this context when deleting textures. > +* Similarly for framebuffer objects, etc. > +*/ > + _mesa_make_current(ctx, NULL, NULL); > > - if (curctx == NULL) { > - /* No current context, but we need one to release > - * renderbuffer surface when we release framebuffer. > - * So temporarily bind the context. > - */ > - _mesa_make_current(ctx, NULL, NULL); > + /* This must be called first so that glthread has a chance to finish */ > + _mesa_glthread_destroy(ctx); > + > + _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); > + > + /* For the fallback textures, free any sampler views belonging to this > +* context. > +*/ > + for (unsigned i = 0; i < NUM_TEXTURE_TARGETS; i++) { > + struct st_texture_object *stObj = > + st_texture_object(ctx->Shared->FallbackTex[i]); > + if (stObj) { > + st_texture_release_context_sampler_view(st, stObj); > + } > } > > st_context_free_zombie_objects(st); > @@ -933,11 +957,6 @@ st_destroy_context(struct st_context *st) > mtx_destroy(>zombie_sampler_views.mutex); > mtx_destroy(>zombie_shaders.mutex); > > - /* This must be called first so that glthread has a chance to finish */ > - _mesa_glthread_destroy(ctx); > - > - _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); > - > st_reference_fragprog(st, >fp, NULL); > st_reference_prog(st, >gp, NULL); > st_reference_vertprog(st, >vp, NULL); > @@ -965,4 +984,12 @@ st_destroy_context(struct st_context *st) > st = NULL; > > free(ctx); > + > + if (save_ctx == ctx) { > + /* unbind the context we just deleted */ > + _mesa_make_current(NULL, NULL, NULL); > + } else { > + /* Restore the current context and draw/read buffers (may be NULL) */ > + _mesa_make_current(save_ctx, save_drawbuffer, save_readbuffer); > + } > } > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] st/mesa: fix texture deletion context mix-up issues (v2)
When we destroy a context, we need to temporarily make that context the current one for the thread. That's because during context tear-down we make many calls to _mesa_reference_texobj(, NULL). Note there's no context parameter. If the texture's refcount goes to zero and we need to delete it, we use the thread's current context. But if that context isn't the context we're tearing down, we get into trouble when deallocating sampler views. See patch 593e36f956 ("st/mesa: implement "zombie" sampler views (v2)") for background information. Also, we need to release any sampler views attached to the fallback textures. Fixes a crash on exit with a glretrace of the Nobel Clinician application. v2: at end of st_destroy_context(), check if save_ctx == ctx and unbind the context if so. --- src/mesa/state_tracker/st_context.c | 51 - 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c index f037384..09d467a 100644 --- a/src/mesa/state_tracker/st_context.c +++ b/src/mesa/state_tracker/st_context.c @@ -917,15 +917,39 @@ st_destroy_context(struct st_context *st) { struct gl_context *ctx = st->ctx; struct st_framebuffer *stfb, *next; + struct gl_framebuffer *save_drawbuffer; + struct gl_framebuffer *save_readbuffer; + + /* Save the current context and draw/read buffers*/ + GET_CURRENT_CONTEXT(save_ctx); + if (save_ctx) { + save_drawbuffer = save_ctx->WinSysDrawBuffer; + save_readbuffer = save_ctx->WinSysReadBuffer; + } else { + save_drawbuffer = save_readbuffer = NULL; + } - GET_CURRENT_CONTEXT(curctx); + /* +* We need to bind the context we're deleting so that +* _mesa_reference_texobj_() uses this context when deleting textures. +* Similarly for framebuffer objects, etc. +*/ + _mesa_make_current(ctx, NULL, NULL); - if (curctx == NULL) { - /* No current context, but we need one to release - * renderbuffer surface when we release framebuffer. - * So temporarily bind the context. - */ - _mesa_make_current(ctx, NULL, NULL); + /* This must be called first so that glthread has a chance to finish */ + _mesa_glthread_destroy(ctx); + + _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); + + /* For the fallback textures, free any sampler views belonging to this +* context. +*/ + for (unsigned i = 0; i < NUM_TEXTURE_TARGETS; i++) { + struct st_texture_object *stObj = + st_texture_object(ctx->Shared->FallbackTex[i]); + if (stObj) { + st_texture_release_context_sampler_view(st, stObj); + } } st_context_free_zombie_objects(st); @@ -933,11 +957,6 @@ st_destroy_context(struct st_context *st) mtx_destroy(>zombie_sampler_views.mutex); mtx_destroy(>zombie_shaders.mutex); - /* This must be called first so that glthread has a chance to finish */ - _mesa_glthread_destroy(ctx); - - _mesa_HashWalk(ctx->Shared->TexObjects, destroy_tex_sampler_cb, st); - st_reference_fragprog(st, >fp, NULL); st_reference_prog(st, >gp, NULL); st_reference_vertprog(st, >vp, NULL); @@ -965,4 +984,12 @@ st_destroy_context(struct st_context *st) st = NULL; free(ctx); + + if (save_ctx == ctx) { + /* unbind the context we just deleted */ + _mesa_make_current(NULL, NULL, NULL); + } else { + /* Restore the current context and draw/read buffers (may be NULL) */ + _mesa_make_current(save_ctx, save_drawbuffer, save_readbuffer); + } } -- 1.8.5.6 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev