[Mesa-dev] [PATCH] st/mesa: add a winsys buffers list in st_context
Brian Paul
brianp at vmware.com
Tue Jul 11 22:14:56 UTC 2017
On 07/11/2017 11:15 AM, Charmaine Lee wrote:
> Commit a5e733c6b52e93de3000647d075f5ca2f55fcb71 fixes the dangling
> framebuffer object by unreferencing the window system draw/read buffers
> when context is released. However this can prematurely destroy the
> resources associated with these window system buffers. The problem is
> reproducible with Turbine Demo running with VMware driver. In this case,
> the depth buffer content was lost when the context is rebound to a
> drawable.
>
> To prevent premature destroy of the resources associated with
> window system buffers, this patch maintains a list of these buffers in
> the context, making sure the reference counts of these buffers will not
> reach zero until the associated framebuffer interface objects no
> longer exist. This also helps to avoid unnecessary destruction and
> re-construction of the resources associated with the framebuffer.
>
> Fixes VMware bug 1909807.
> ---
> src/gallium/include/state_tracker/st_api.h | 5 +++
> src/gallium/state_trackers/dri/dri_drawable.c | 4 ++
> src/gallium/state_trackers/wgl/stw_st.c | 4 +-
> src/mesa/state_tracker/st_context.c | 22 ++++++++++
> src/mesa/state_tracker/st_context.h | 7 ++++
> src/mesa/state_tracker/st_manager.c | 59 ++++++++++++++++++++++-----
> src/mesa/state_tracker/st_manager.h | 4 ++
> 7 files changed, 94 insertions(+), 11 deletions(-)
>
> diff --git a/src/gallium/include/state_tracker/st_api.h b/src/gallium/include/state_tracker/st_api.h
> index d641092..3fd5f01 100644
> --- a/src/gallium/include/state_tracker/st_api.h
> +++ b/src/gallium/include/state_tracker/st_api.h
> @@ -311,6 +311,11 @@ struct st_framebuffer_iface
> int32_t stamp;
>
> /**
> + * Identifier that uniquely identifies the framebuffer interface object.
> + */
> + uint32_t ID;
> +
> + /**
> * 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 3c2e307..0cfdc30 100644
> --- a/src/gallium/state_trackers/dri/dri_drawable.c
> +++ b/src/gallium/state_trackers/dri/dri_drawable.c
> @@ -38,6 +38,8 @@
> #include "util/u_memory.h"
> #include "util/u_inlines.h"
>
> +static uint32_t drifb_ID = 0;
> +
> static void
> swap_fences_unref(struct dri_drawable *draw);
>
> @@ -155,6 +157,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);
>
> return GL_TRUE;
> fail:
> @@ -177,6 +180,7 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
>
> swap_fences_unref(drawable);
>
> + drawable->base.ID = 0;
> FREE(drawable);
> }
>
> diff --git a/src/gallium/state_trackers/wgl/stw_st.c b/src/gallium/state_trackers/wgl/stw_st.c
> index 7806a2a..c2844b0 100644
> --- a/src/gallium/state_trackers/wgl/stw_st.c
> +++ b/src/gallium/state_trackers/wgl/stw_st.c
> @@ -46,7 +46,7 @@ struct stw_st_framebuffer {
> unsigned texture_mask;
> };
>
> -
> +static uint32_t stwfb_ID = 0;
>
> /**
> * Is the given mutex held by the calling thread?
> @@ -234,6 +234,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.visual = &stwfb->stvis;
> p_atomic_set(&stwfb->base.stamp, 1);
> @@ -255,6 +256,7 @@ 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;
> FREE(stwfb);
> }
>
> diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c
> index f535139..fb0182f 100644
> --- a/src/mesa/state_tracker/st_context.c
> +++ b/src/mesa/state_tracker/st_context.c
> @@ -38,6 +38,7 @@
> #include "program/prog_cache.h"
> #include "vbo/vbo.h"
> #include "glapi/glapi.h"
> +#include "st_manager.h"
> #include "st_context.h"
> #include "st_debug.h"
> #include "st_cb_bitmap.h"
> @@ -571,6 +572,9 @@ struct st_context *st_create_context(gl_api api, struct pipe_context *pipe,
> _mesa_destroy_context(ctx);
> }
>
> + /* Initialize context's winsys buffers list */
> + LIST_INITHEAD(&st->winsys_buffers);
> +
> return st;
> }
>
> @@ -591,6 +595,19 @@ destroy_tex_sampler_cb(GLuint id, void *data, void *userData)
> void st_destroy_context( struct st_context *st )
> {
> struct gl_context *ctx = st->ctx;
> + struct st_framebuffer *stfb, *next;
> +
> + GET_CURRENT_CONTEXT(curctx);
> + if (curctx == NULL) {
> + boolean ret;
> +
> + /* No current context, but we need one to release
> + * renderbuffer surface when we release framebuffer.
> + * So temporarily bind the context.
> + */
> + ret = _mesa_make_current(ctx, NULL, NULL);
> + (void) ret;
We might as well just call _mesa_make_current() and get rid of the 'ret'
variable.
> + }
>
> /* This must be called first so that glthread has a chance to finish */
> _mesa_glthread_destroy(ctx);
> @@ -604,6 +621,11 @@ void st_destroy_context( struct st_context *st )
> st_reference_prog(st, &st->tep, NULL);
> st_reference_compprog(st, &st->cp, NULL);
>
> + /* release framebuffer in the winsys buffers list */
> + LIST_FOR_EACH_ENTRY_SAFE_REV(stfb, next, &st->winsys_buffers, head) {
> + st_framebuffer_reference(&stfb, NULL);
> + }
> +
> pipe_sampler_view_reference(&st->pixel_xfer.pixelmap_sampler_view, NULL);
> pipe_resource_reference(&st->pixel_xfer.pixelmap_texture, NULL);
>
> diff --git a/src/mesa/state_tracker/st_context.h b/src/mesa/state_tracker/st_context.h
> index af9149e..9bdde68 100644
> --- a/src/mesa/state_tracker/st_context.h
> +++ b/src/mesa/state_tracker/st_context.h
> @@ -33,6 +33,7 @@
> #include "main/fbobject.h"
> #include "state_tracker/st_atom.h"
> #include "util/u_inlines.h"
> +#include "util/list.h"
>
>
> #ifdef __cplusplus
> @@ -277,6 +278,9 @@ struct st_context
> */
> struct st_bound_handles bound_texture_handles[PIPE_SHADER_TYPES];
> struct st_bound_handles bound_image_handles[PIPE_SHADER_TYPES];
> +
> + /* Winsys buffers */
> + struct list_head winsys_buffers;
> };
>
>
> @@ -301,6 +305,9 @@ struct st_framebuffer
> unsigned num_statts;
> int32_t stamp;
> int32_t iface_stamp;
> + uint32_t iface_ID;
> +
> + struct list_head head;
Maybe put a comment on that new field ("list of all framebuffer objects"?)
> };
>
>
> diff --git a/src/mesa/state_tracker/st_manager.c b/src/mesa/state_tracker/st_manager.c
> index 085f54e..de16a3a 100644
> --- a/src/mesa/state_tracker/st_manager.c
> +++ b/src/mesa/state_tracker/st_manager.c
> @@ -57,6 +57,7 @@
> #include "util/u_inlines.h"
> #include "util/u_atomic.h"
> #include "util/u_surface.h"
> +#include "util/list.h"
>
>
> /**
> @@ -459,6 +460,7 @@ st_framebuffer_create(struct st_context *st,
> _mesa_initialize_window_framebuffer(&stfb->Base, &mode);
>
> stfb->iface = stfbi;
> + stfb->iface_ID = stfbi->ID;
> stfb->iface_stamp = p_atomic_read(&stfbi->stamp) - 1;
>
> /* add the color buffer */
> @@ -480,7 +482,7 @@ st_framebuffer_create(struct st_context *st,
> /**
> * Reference a framebuffer.
> */
> -static void
> +void
> st_framebuffer_reference(struct st_framebuffer **ptr,
> struct st_framebuffer *stfb)
> {
> @@ -488,6 +490,29 @@ st_framebuffer_reference(struct st_framebuffer **ptr,
> _mesa_reference_framebuffer((struct gl_framebuffer **) ptr, fb);
> }
>
> +/**
> + * Purge the winsys buffers list to remove any references to
> + * non-existing framebuffer interface objects.
> + */
> +static void
> +st_framebuffers_purge(struct st_context *st)
> +{
> + struct st_framebuffer *stfb, *next;
> +
> + LIST_FOR_EACH_ENTRY_SAFE_REV(stfb, next, &st->winsys_buffers, head) {
> + /**
> + * 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 (stfb->iface->ID != stfb->iface_ID) {
> + LIST_DEL(&stfb->head);
> + st_framebuffer_reference(&stfb, NULL);
> + }
> + }
> +}
> +
> static void
> st_context_flush(struct st_context_iface *stctxi, unsigned flags,
> struct pipe_fence_handle **fence)
> @@ -761,17 +786,26 @@ st_framebuffer_reuse_or_create(struct st_context *st,
> struct gl_framebuffer *fb,
> struct st_framebuffer_iface *stfbi)
> {
> - struct st_framebuffer *cur = st_ws_framebuffer(fb), *stfb = NULL;
> + struct st_framebuffer *cur = NULL, *stfb = NULL;
>
> - /* dummy framebuffers cant be used as st_framebuffer */
> - if (cur && &cur->Base != _mesa_get_incomplete_framebuffer() &&
> - cur->iface == stfbi) {
> - /* reuse the current stfb */
> - st_framebuffer_reference(&stfb, cur);
> + /* Check if there is already a framebuffer object for the specified
> + * framebuffer interface in this context. If there is one, use it.
> + */
> + LIST_FOR_EACH_ENTRY(cur, &st->winsys_buffers, head) {
> + if (cur->iface_ID == stfbi->ID) {
> + st_framebuffer_reference(&stfb, cur);
> + break;
> + }
> }
> - else {
> - /* create a new one */
> - stfb = st_framebuffer_create(st, stfbi);
> +
> + /* If there is not already a framebuffer object, create one */
> + if (stfb == NULL) {
> + cur = st_framebuffer_create(st, stfbi);
> +
> + /* add to the context's winsys buffers list */
> + LIST_ADD(&cur->head, &st->winsys_buffers);
> +
> + st_framebuffer_reference(&stfb, cur);
> }
>
> return stfb;
> @@ -822,6 +856,11 @@ st_api_make_current(struct st_api *stapi, struct st_context_iface *stctxi,
>
> st_framebuffer_reference(&stdraw, NULL);
> st_framebuffer_reference(&stread, NULL);
> +
> + /* Purge the context's winsys_buffers list in case any
> + * of the referenced drawables no longer exist.
> + */
> + st_framebuffers_purge(st);
> }
> else {
> ret = _mesa_make_current(NULL, NULL, NULL);
> diff --git a/src/mesa/state_tracker/st_manager.h b/src/mesa/state_tracker/st_manager.h
> index 65874b0..68adf2f 100644
> --- a/src/mesa/state_tracker/st_manager.h
> +++ b/src/mesa/state_tracker/st_manager.h
> @@ -33,6 +33,7 @@
> #include "pipe/p_compiler.h"
>
> struct st_context;
> +struct st_framebuffer;
>
> void
> st_manager_flush_frontbuffer(struct st_context *st);
> @@ -44,4 +45,7 @@ boolean
> st_manager_add_color_renderbuffer(struct st_context *st, struct gl_framebuffer *fb,
> gl_buffer_index idx);
>
> +void
> +st_framebuffer_reference(struct st_framebuffer **ptr,
> + struct st_framebuffer *stfb);
> #endif /* ST_MANAGER_H */
>
Like I said before, I think there's still a reference counting issue
behind all this, but I'm OK with this solution. Maybe wait a bit and
see if anyone else has comments.
Reviewed-by: Brian Paul <brianp at vmware.com>
More information about the mesa-dev
mailing list