[Mesa-dev] [PATCH] st/mesa: add a winsys buffers list in st_context

Andy Furniss adf.lists at gmail.com
Thu Jul 13 16:46:58 UTC 2017


OK, thanks, I missed that.

Brian Paul wrote:
> A patch to fix this was already posted and reviewed.  I'll push it soon 
> since I think Michel is off-line this time of day.
> 
> -Brian
> 
> On 07/13/2017 09:17 AM, Andy Furniss wrote:
>> This breaks startx on radeonsi for me on a R9 285.
>>
>> [  5297.130] (II) glamor: OpenGL accelerated X.org driver based.
>> [  5297.132] (II) glamor: EGL version 1.5 (DRI2):
>> [  5297.133] (EE)
>> [  5297.133] (EE) Backtrace:
>> [  5297.151] (EE) 0: /usr/libexec/Xorg (OsSigHandler+0x29) [0x584069]
>> [  5297.164] (EE) 1: /lib/libc.so.6 (killpg+0x40) [0x7f667a45768f]
>> [  5297.165] (EE) 2: /usr/lib/dri/radeonsi_dri.so
>> (st_framebuffer_reuse_or_create.isra.8+0x19a) [0x7f6673aa441a]
>> [  5297.166] (EE) 3: /usr/lib/dri/radeonsi_dri.so
>> (st_api_make_current+0x2e) [0x7f6673aa463e]
>> [  5297.168] (EE) 4: /usr/lib/dri/radeonsi_dri.so (driBindContext+0x36)
>> [0x7f6673bdfdd6]
>> [  5297.168] (EE) 5: /usr/lib/../lib64/libEGL.so.1
>> (dri2_make_current+0x113) [0x7f666bb23c23]
>> [  5297.168] (EE) 6: /usr/lib/../lib64/libEGL.so.1
>> (eglMakeCurrent+0x2a3) [0x7f666bb15123]
>> [  5297.181] (EE) 7: /usr/lib/xorg/modules/libglamoregl.so
>> (glamor_egl_init+0x2c9) [0x7f666c03c8a9]
>> [  5297.189] (EE) 8: /usr/lib/xorg/modules/drivers/amdgpu_drv.so
>> (amdgpu_glamor_pre_init+0x4f) [0x7f667524266f]
>> [  5297.190] (EE) 9: /usr/lib/xorg/modules/drivers/amdgpu_drv.so
>> (AMDGPUPreInit_KMS+0x70d) [0x7f667523aa7d]
>> [  5297.191] (EE) 10: /usr/libexec/Xorg (InitOutput+0xa9b) [0x477f1b]
>> [  5297.192] (EE) 11: /usr/libexec/Xorg (dix_main+0x1c6) [0x439106]
>> [  5297.194] (EE) 12: /lib/libc.so.6 (__libc_start_main+0xf0)
>> [0x7f667a4445e0]
>> [  5297.194] (EE) 13: /usr/libexec/Xorg (_start+0x29) [0x424649]
>> [  5297.194] (EE)
>> [  5297.194] (EE) Segmentation fault at address 0x490
>> [  5297.195] (EE)
>> Fatal server error:
>> [  5297.195] (EE) Caught signal 11 (Segmentation fault). Server aborting
>>
>> 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;
>>> +   }
>>>      /* 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;
>>>   };
>>> 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 */
>>>
>>
>> _______________________________________________
>> 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