Can you explain in the comment how/why this fixes the crash? It's not obvious to me.

Patch looks OK, AFAICT.

Reviewed-by: Brian Paul <bri...@vmware.com>



On 07/23/2017 05:37 PM, Charmaine Lee wrote:
With this patch, framebuffer interface hash table is created
per state tracker manager.

Fixes crash with steam.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101876
Fixes: 5124bf98239 ("st/mesa: add destroy_drawable interface")
Tested-by: Christoph Haag <haa...@frickel.club>
---
  src/gallium/include/state_tracker/st_api.h    |  21 +++++
  src/gallium/state_trackers/dri/dri_drawable.c |   1 +
  src/gallium/state_trackers/dri/dri_screen.c   |   3 +
  src/gallium/state_trackers/glx/xlib/xm_api.c  |   3 +
  src/gallium/state_trackers/glx/xlib/xm_st.c   |   1 +
  src/gallium/state_trackers/wgl/stw_device.c   |   3 +
  src/gallium/state_trackers/wgl/stw_st.c       |   1 +
  src/mesa/state_tracker/st_manager.c           | 107 +++++++++++++++++++-------
  8 files changed, 113 insertions(+), 27 deletions(-)

diff --git a/src/gallium/include/state_tracker/st_api.h 
b/src/gallium/include/state_tracker/st_api.h
index 9b660f7..bc62a69 100644
--- a/src/gallium/include/state_tracker/st_api.h
+++ b/src/gallium/include/state_tracker/st_api.h
@@ -284,6 +284,7 @@ struct st_context_attribs
  };

  struct st_context_iface;
+struct st_manager;

  /**
   * Represent a windowing system drawable.
@@ -317,6 +318,11 @@ struct st_framebuffer_iface
     uint32_t ID;

     /**
+    * The state tracker manager that manages this object.
+    */
+   struct st_manager *state_manager;
+
+   /**
      * Available for the state tracker manager to use.
      */
     void *st_manager_private;
@@ -376,6 +382,11 @@ struct st_context_iface
     void *st_manager_private;

     /**
+    * The state tracker manager that manages this object.
+    */
+   struct st_manager *state_manager;
+
+   /**
      * The CSO context associated with this context in case we need to draw
      * something before swap buffers.
      */
@@ -483,6 +494,16 @@ struct st_manager
      */
     void (*set_background_context)(struct st_context_iface *stctxi,
                                    struct util_queue_monitoring *queue_info);
+
+   /**
+    * Destroy any private data used by the state tracker manager.
+    */
+   void (*destroy)(struct st_manager *smapi);
+
+   /**
+    * 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 c7df0f6..9e0dd6b 100644
--- a/src/gallium/state_trackers/dri/dri_drawable.c
+++ b/src/gallium/state_trackers/dri/dri_drawable.c
@@ -158,6 +158,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);
+   drawable->base.state_manager = &screen->base;

     return GL_TRUE;
  fail:
diff --git a/src/gallium/state_trackers/dri/dri_screen.c 
b/src/gallium/state_trackers/dri/dri_screen.c
index 1dd7bd3..59a850b 100644
--- a/src/gallium/state_trackers/dri/dri_screen.c
+++ b/src/gallium/state_trackers/dri/dri_screen.c
@@ -457,6 +457,9 @@ dri_destroy_option_cache(struct dri_screen * screen)
  void
  dri_destroy_screen_helper(struct dri_screen * screen)
  {
+   if (screen->base.destroy)
+      screen->base.destroy(&screen->base);
+
     if (screen->st_api && screen->st_api->destroy)
        screen->st_api->destroy(screen->st_api);

diff --git a/src/gallium/state_trackers/glx/xlib/xm_api.c 
b/src/gallium/state_trackers/glx/xlib/xm_api.c
index e4b1e9d..828253b 100644
--- a/src/gallium/state_trackers/glx/xlib/xm_api.c
+++ b/src/gallium/state_trackers/glx/xlib/xm_api.c
@@ -181,6 +181,9 @@ xmesa_close_display(Display *display)
      *    xmdpy->screen->destroy(xmdpy->screen);
      * }
      */
+
+   if (xmdpy->smapi->destroy)
+      xmdpy->smapi->destroy(xmdpy->smapi);
     free(xmdpy->smapi);

     XFree((char *) info);
diff --git a/src/gallium/state_trackers/glx/xlib/xm_st.c 
b/src/gallium/state_trackers/glx/xlib/xm_st.c
index 6a0f4aa..0c42e65 100644
--- a/src/gallium/state_trackers/glx/xlib/xm_st.c
+++ b/src/gallium/state_trackers/glx/xlib/xm_st.c
@@ -304,6 +304,7 @@ xmesa_create_st_framebuffer(XMesaDisplay xmdpy, XMesaBuffer 
b)
     stfbi->flush_front = xmesa_st_framebuffer_flush_front;
     stfbi->validate = xmesa_st_framebuffer_validate;
     stfbi->ID = p_atomic_inc_return(&xmesa_stfbi_ID);
+   stfbi->state_manager = xmdpy->smapi;
     p_atomic_set(&stfbi->stamp, 1);
     stfbi->st_manager_private = (void *) xstfb;

diff --git a/src/gallium/state_trackers/wgl/stw_device.c 
b/src/gallium/state_trackers/wgl/stw_device.c
index 6c0f14d..b88e110 100644
--- a/src/gallium/state_trackers/wgl/stw_device.c
+++ b/src/gallium/state_trackers/wgl/stw_device.c
@@ -199,6 +199,9 @@ stw_cleanup(void)
     DeleteCriticalSection(&stw_dev->fb_mutex);
     DeleteCriticalSection(&stw_dev->ctx_mutex);

+   if (stw_dev->smapi->destroy)
+      stw_dev->smapi->destroy(stw_dev->smapi);
+
     FREE(stw_dev->smapi);
     stw_dev->stapi->destroy(stw_dev->stapi);

diff --git a/src/gallium/state_trackers/wgl/stw_st.c 
b/src/gallium/state_trackers/wgl/stw_st.c
index 85a8b17..5e165c8 100644
--- a/src/gallium/state_trackers/wgl/stw_st.c
+++ b/src/gallium/state_trackers/wgl/stw_st.c
@@ -235,6 +235,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.state_manager = stw_dev->smapi;

     stwfb->base.visual = &stwfb->stvis;
     p_atomic_set(&stwfb->base.stamp, 1);
diff --git a/src/mesa/state_tracker/st_manager.c 
b/src/mesa/state_tracker/st_manager.c
index ebc7ca8..834bcc9 100644
--- a/src/mesa/state_tracker/st_manager.c
+++ b/src/mesa/state_tracker/st_manager.c
@@ -61,9 +61,13 @@
  #include "util/list.h"

  struct hash_table;
-static struct hash_table *st_fbi_ht; /* framebuffer iface objects hash table */
+struct st_manager_private
+{
+   struct hash_table *stfbi_ht; /* framebuffer iface objects hash table */
+   mtx_t st_mutex;
+};

-static mtx_t st_mutex = _MTX_INITIALIZER_NP;
+static void st_manager_destroy(struct st_manager *);

  /**
   * Map an attachment to a buffer index.
@@ -511,45 +515,63 @@ st_framebuffer_iface_equal(const void *a, const void *b)


  static boolean
-st_framebuffer_iface_lookup(const struct st_framebuffer_iface *stfbi)
+st_framebuffer_iface_lookup(struct st_manager *smapi,
+                            const struct st_framebuffer_iface *stfbi)
  {
+   struct st_manager_private *smPriv =
+      (struct st_manager_private *)smapi->st_manager_private;
     struct hash_entry *entry;

-   mtx_lock(&st_mutex);
-   entry = _mesa_hash_table_search(st_fbi_ht, stfbi);
-   mtx_unlock(&st_mutex);
+   assert(smPriv);
+   assert(smPriv->stfbi_ht);
+
+   mtx_lock(&smPriv->st_mutex);
+   entry = _mesa_hash_table_search(smPriv->stfbi_ht, stfbi);
+   mtx_unlock(&smPriv->st_mutex);

     return entry != NULL;
  }


  static boolean
-st_framebuffer_iface_insert(struct st_framebuffer_iface *stfbi)
+st_framebuffer_iface_insert(struct st_manager *smapi,
+                            struct st_framebuffer_iface *stfbi)
  {
+   struct st_manager_private *smPriv =
+      (struct st_manager_private *)smapi->st_manager_private;
     struct hash_entry *entry;

-   mtx_lock(&st_mutex);
-   entry = _mesa_hash_table_insert(st_fbi_ht, stfbi, stfbi);
-   mtx_unlock(&st_mutex);
+   assert(smPriv);
+   assert(smPriv->stfbi_ht);
+
+   mtx_lock(&smPriv->st_mutex);
+   entry = _mesa_hash_table_insert(smPriv->stfbi_ht, stfbi, stfbi);
+   mtx_unlock(&smPriv->st_mutex);

     return entry != NULL;
  }


  static void
-st_framebuffer_iface_remove(struct st_framebuffer_iface *stfbi)
+st_framebuffer_iface_remove(struct st_manager *smapi,
+                            struct st_framebuffer_iface *stfbi)
  {
+   struct st_manager_private *smPriv =
+      (struct st_manager_private *)smapi->st_manager_private;
     struct hash_entry *entry;

-   mtx_lock(&st_mutex);
-   entry = _mesa_hash_table_search(st_fbi_ht, stfbi);
+   if (!smPriv || !smPriv->stfbi_ht);
+      return;
+
+   mtx_lock(&smPriv->st_mutex);
+   entry = _mesa_hash_table_search(smPriv->stfbi_ht, stfbi);
     if (!entry)
        goto unlock;

-   _mesa_hash_table_remove(st_fbi_ht, entry);
+   _mesa_hash_table_remove(smPriv->stfbi_ht, entry);

  unlock:
-   mtx_unlock(&st_mutex);
+   mtx_unlock(&smPriv->st_mutex);
  }


@@ -561,7 +583,10 @@ static void
  st_api_destroy_drawable(struct st_api *stapi,
                          struct st_framebuffer_iface *stfbi)
  {
-   st_framebuffer_iface_remove(stfbi);
+   if (stfbi)
+      return;
+
+   st_framebuffer_iface_remove(stfbi->state_manager, stfbi);
  }


@@ -572,16 +597,24 @@ st_api_destroy_drawable(struct st_api *stapi,
  static void
  st_framebuffers_purge(struct st_context *st)
  {
+   struct st_context_iface *st_iface = &st->iface;
+   struct st_manager *smapi = st_iface->state_manager;
     struct st_framebuffer *stfb, *next;

+   assert(smapi);
+
     LIST_FOR_EACH_ENTRY_SAFE_REV(stfb, next, &st->winsys_buffers, head) {
+      struct st_framebuffer_iface *stfbi = stfb->iface;
+
+      assert(stfbi);
+
        /**
         * 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 (!st_framebuffer_iface_lookup(stfb->iface)) {
+      if (!st_framebuffer_iface_lookup(smapi, stfbi)) {
           LIST_DEL(&stfb->head);
           st_framebuffer_reference(&stfb, NULL);
        }
@@ -778,6 +811,21 @@ st_api_create_context(struct st_api *stapi, struct 
st_manager *smapi,
        return NULL;
     }

+   /* Create a hash table for the framebuffer interface objects
+    * if it has not been created for this st manager.
+    */
+   if (smapi->st_manager_private == NULL) {
+      struct st_manager_private *smPriv;
+
+      smPriv = CALLOC_STRUCT(st_manager_private);
+      mtx_init(&smPriv->st_mutex, mtx_plain);
+      smPriv->stfbi_ht = _mesa_hash_table_create(NULL,
+                                                 st_framebuffer_iface_hash,
+                                                 st_framebuffer_iface_equal);
+      smapi->st_manager_private = smPriv;
+      smapi->destroy = st_manager_destroy;
+   }
+
     if (attribs->flags & ST_CONTEXT_FLAG_ROBUST_ACCESS)
        ctx_flags |= PIPE_CONTEXT_ROBUST_BUFFER_ACCESS;

@@ -846,6 +894,7 @@ st_api_create_context(struct st_api *stapi, struct 
st_manager *smapi,
     st->iface.st_context_private = (void *) smapi;
     st->iface.cso_context = st->cso_context;
     st->iface.pipe = st->pipe;
+   st->iface.state_manager = smapi;

     *error = ST_CONTEXT_SUCCESS;
     return &st->iface;
@@ -888,7 +937,7 @@ st_framebuffer_reuse_or_create(struct st_context *st,
           /* add the referenced framebuffer interface object to
            * the framebuffer interface object hash table.
            */
-         if (!st_framebuffer_iface_insert(stfbi)) {
+         if (!st_framebuffer_iface_insert(stfbi->state_manager, stfbi)) {
              st_framebuffer_reference(&cur, NULL);
              return NULL;
           }
@@ -964,8 +1013,6 @@ st_api_make_current(struct st_api *stapi, struct 
st_context_iface *stctxi,
  static void
  st_api_destroy(struct st_api *stapi)
  {
-   _mesa_hash_table_destroy(st_fbi_ht, NULL);
-   mtx_destroy(&st_mutex);
  }

  /**
@@ -1051,6 +1098,19 @@ st_manager_add_color_renderbuffer(struct st_context *st,
     return TRUE;
  }

+static void
+st_manager_destroy(struct st_manager *smapi)
+{
+   struct st_manager_private *smPriv = smapi->st_manager_private;
+
+   if (smPriv && smPriv->stfbi_ht) {
+      _mesa_hash_table_destroy(smPriv->stfbi_ht, NULL);
+      mtx_destroy(&smPriv->st_mutex);
+      free(smPriv);
+      smapi->st_manager_private = NULL;
+   }
+}
+
  static unsigned
  get_version(struct pipe_screen *screen,
              struct st_config_options *options, gl_api api)
@@ -1106,12 +1166,5 @@ static const struct st_api st_gl_api = {
  struct st_api *
  st_gl_api_create(void)
  {
-   /* Create a hash table for all the framebuffer interface objects */
-
-   mtx_init(&st_mutex, mtx_plain);
-   st_fbi_ht = _mesa_hash_table_create(NULL,
-                                       st_framebuffer_iface_hash,
-                                       st_framebuffer_iface_equal);
-
     return (struct st_api *) &st_gl_api;
  }


_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to