[Mesa-dev] [PATCH 1/6] gallium: clarify the constraints on sampler_view_destroy
Nicolai Hähnle
nicolai.haehnle at amd.com
Wed Oct 11 07:41:07 UTC 2017
On 11.10.2017 01:25, Gurchetan Singh wrote:
> "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."
>
> 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.
st/mesa creates sampler views of two types:
1. There are sampler views associated to normal textures, which are
created in st_sampler_view.c. Those sampler views are always created by
the context that uses them; when the context is destroyed, they are all
released (via st_destroy_context --> destroy_tex_sampler_cb).
2. Temporary sampler views are created in some places (e.g. for blits),
but those are released immediately after setting them in the
pipe_context via cso.
I'm not so sure about pipe_surface objects, nor about other state trackers.
Unfortunately, none of your links is particularly informative about the
sequence of events that lead to whatever problems they were trying to fix :/
All I can say is that I've never seen a crash related to a sampler view
being destroyed after its owning context with Mesa master, but I realize
that may not be too helpful...
Cheers,
Nicolai
>
> [1]
> https://lists.freedesktop.org/archives/mesa-dev/2011-September/011578.html
> [2]
> https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/files/10.3-state_tracker-gallium-fix-crash-with-st_renderbuffer.patch
> [3]
> 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
> [4]
> https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/files/12.1-radeonsi-sampler_view_destroy.patch
>
> On Tue, Oct 10, 2017 at 2:42 PM, Nicolai Hähnle <nhaehnle at gmail.com
> <mailto:nhaehnle at gmail.com>> wrote:
>
> Same as on IRC:
>
> On 10.10.2017 04:06, Marek Olšák wrote:
>
> Is there any difference with piglit/drawoverhead?
>
>
> Yes, there is.
>
>
> If yes, would this be useful?
> https://patchwork.freedesktop.org/patch/41241/
> <https://patchwork.freedesktop.org/patch/41241/>
>
>
> 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
>
>
>
> Marek
>
>
> On Fri, Oct 6, 2017 at 10:38 PM, Nicolai Hähnle
> <nhaehnle at gmail.com <mailto:nhaehnle at gmail.com>> wrote:
>
> From: Nicolai Hähnle <nicolai.haehnle at amd.com
> <mailto:nicolai.haehnle at amd.com>>
>
> 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
> mesa-dev at lists.freedesktop.org
> <mailto:mesa-dev at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>
>
> --
> Lerne, wie die Welt wirklich ist,
> Aber vergiss niemals, wie sie sein sollte.
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>
More information about the mesa-dev
mailing list