<div dir="ltr">"The GL state tracker, which is the only one that runs into the multi-context subtleties (due to share groups), already guarantees that sampler views are destroyed before their context of creation is destroyed."<div> </div><div>How does the GL state tracker guarantee this? Does this guarantee also apply to pipe_surfaces? Context: ChromiumOS previously ran into an issue where sampler_views and pipe_surfaces outlived the context that created them [1], and we have technical debt [2][3][4] emanating from this. It would be great to remove this, if things have changed in the Gallium code. </div><div> </div><div>[1] <a href="https://lists.freedesktop.org/archives/mesa-dev/2011-September/011578.html">https://lists.freedesktop.org/archives/mesa-dev/2011-September/011578.html</a></div><div>[2] <a href="https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/files/10.3-state_tracker-gallium-fix-crash-with-st_renderbuffer.patch">https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/files/10.3-state_tracker-gallium-fix-crash-with-st_renderbuffer.patch</a></div><div>[3] <a href="https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/files/10.3-state_tracker-gallium-fix-crash-with-st_renderbuffer-freedreno.patch">https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/files/10.3-state_tracker-gallium-fix-crash-with-st_renderbuffer-freedreno.patch</a></div><div>[4] <a href="https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/files/12.1-radeonsi-sampler_view_destroy.patch">https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/files/12.1-radeonsi-sampler_view_destroy.patch</a></div></div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Oct 10, 2017 at 2:42 PM, Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com" target="_blank">nhaehnle@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Same as on IRC: On 10.10.2017 04:06, Marek Olšák wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Is there any difference with piglit/drawoverhead? </blockquote> Yes, there is. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> If yes, would this be useful? <a href="https://patchwork.freedesktop.org/patch/41241/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/patch/41241/</a> </blockquote> Surprisingly, not that much. I'm going to think though a couple of other options, but want to already push patch #3; I sent a version that is rebased on master and disentangled from the locking stuff. Cheers, Nicolai<div class="HOEnZb"><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Marek On Fri, Oct 6, 2017 at 10:38 PM, Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com" target="_blank">nhaehnle@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> From: Nicolai HÃ¤hnle <<a href="mailto:nicolai.haehnle@amd.com" target="_blank">nicolai.haehnle@amd.com</a>> r600 expects the context that created the sampler view to still be alive (there is a per-context list of sampler views). svga currently bails when the context of destruction is not the same as creation. The GL state tracker, which is the only one that runs into the multi-context subtleties (due to share groups), already guarantees that sampler views are destroyed before their context of creation is destroyed. Most drivers are context-agnostic, so the warning message in pipe_sampler_view_release doesn't really make sense. --- src/gallium/auxiliary/util/u_inlines.h | 16 ++++++++++------ src/gallium/include/pipe/p_context.h | 10 ++++++++++ src/mesa/state_tracker/st_sampler_view.c | 1 - 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/gallium/auxiliary/util/u_inlines.h b/src/gallium/auxiliary/util/u_inlines.h index 79f62c32266..790352d7800 100644 --- a/src/gallium/auxiliary/util/u_inlines.h +++ b/src/gallium/auxiliary/util/u_inlines.h @@ -142,45 +142,49 @@ pipe_resource_reference(struct pipe_resource **ptr, struct pipe_resource *tex) struct pipe_resource *next = old_tex->next; old_tex->screen->resource_destroy(old_tex->screen, old_tex); old_tex = next; } while (pipe_reference_described(&old_tex->reference, NULL, (debug_reference_descriptor)debug_describe_resource)); } *ptr = tex; } +/** + * Set *ptr to \p view with proper reference counting. + * + * The caller must guarantee that \p view and *ptr must have been created in + * the same context (if they exist), and that this must be the current context. + */ static inline void pipe_sampler_view_reference(struct pipe_sampler_view **ptr, struct pipe_sampler_view *view) { struct pipe_sampler_view *old_view = *ptr; if (pipe_reference_described(&(*ptr)->reference, &view->reference, (debug_reference_descriptor)debug_describe_sampler_view)) old_view->context->sampler_view_destroy(old_view->context, old_view); *ptr = view; } /** * Similar to pipe_sampler_view_reference() but always set the pointer to - * NULL and pass in an explicit context. Passing an explicit context is a - * work-around for fixing a dangling context pointer problem when textures - * are shared by multiple contexts. XXX fix this someday. + * NULL and pass in the current context explicitly. + * + * If *ptr is non-NULL, it may refer to a view that was created in a different + * context (however, that context must still be alive). */ static inline void pipe_sampler_view_release(struct pipe_context *ctx, struct pipe_sampler_view **ptr) { struct pipe_sampler_view *old_view = *ptr; - if (*ptr && (*ptr)->context != ctx) { - debug_printf_once(("context mis-match in pipe_sampler_view_release()\n")); - } if (pipe_reference_described(&(*ptr)->reference, NULL, (debug_reference_descriptor)debug_describe_sampler_view)) { ctx->sampler_view_destroy(ctx, old_view); } *ptr = NULL; } static inline void pipe_so_target_reference(struct pipe_stream_output_target **ptr, struct pipe_stream_output_target *target) diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h index 4609d4dbf23..087836d1c0c 100644 --- a/src/gallium/include/pipe/p_context.h +++ b/src/gallium/include/pipe/p_context.h @@ -501,20 +501,30 @@ struct pipe_context { void (*fence_server_sync)(struct pipe_context *pipe, struct pipe_fence_handle *fence); /** * Create a view on a texture to be used by a shader stage. */ struct pipe_sampler_view * (*create_sampler_view)(struct pipe_context *ctx, struct pipe_resource *texture, const struct pipe_sampler_view *templat); + /** + * Destroy a view on a texture. + * + * \param ctx the current context + * \param view the view to be destroyed + * + * \note The current context may not be the context in which the view was + * created (view->context). However, the caller must guarantee that + * the context which created the view is still alive. + */ void (*sampler_view_destroy)(struct pipe_context *ctx, struct pipe_sampler_view *view); /** * Get a surface which is a "view" into a resource, used by * render target / depth stencil stages. */ struct pipe_surface *(*create_surface)(struct pipe_context *ctx, struct pipe_resource *resource, diff --git a/src/mesa/state_tracker/st_sampler_view.c b/src/mesa/state_tracker/st_sampler_view.c index fbf0aaeb03a..d1715a888d9 100644 --- a/src/mesa/state_tracker/st_sampler_view.c +++ b/src/mesa/state_tracker/st_sampler_view.c @@ -109,21 +109,20 @@ st_texture_release_sampler_view(struct st_context *st, /** * Release all sampler views attached to the given texture object, regardless * of the context. */ void st_texture_release_all_sampler_views(struct st_context *st, struct st_texture_object *stObj) { GLuint i; - /* XXX This should use sampler_views[i]->pipe, not st->pipe */ for (i = 0; i < stObj->num_sampler_views; ++i) pipe_sampler_view_release(st->pipe, &stObj->sampler_views[i]); } void st_texture_free_sampler_views(struct st_texture_object *stObj) { free(stObj->sampler_views); stObj->sampler_views = NULL; -- 2.11.0 _______________________________________________ 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></blockquote> </div></div> -- Lerne, wie die Welt wirklich ist, Aber vergiss niemals, wie sie sein sollte.<div class="HOEnZb"><div class="h5"> _______________________________________________ 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> </div></div></blockquote></div> </div>