[Mesa-dev,v5,09/12] radeon: use common pipe_screen ref counting

Submitted by Rob Herring on Aug. 7, 2017, 10:58 p.m.

Details

Message ID 20170807225819.13831-15-robh@kernel.org
State New
Headers show
Series "Common pipe screen ref counting" ( rev: 5 ) in Mesa

Not browsing as part of any series.

Commit Message

Rob Herring Aug. 7, 2017, 10:58 p.m.
Use the common pipe_screen ref counting.

radeon uses the common pipe_screen_{un}reference functions, but amdgpu
is unique in its hashing the dev pointer rather than the fd, so the
common fd hashing cannot be used. However, the same reference counting
can be used instead of the private one. The mutex can be dropped as the
pipe loader serializes the create_screen() and destroy() calls.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: "Marek Olšák" <marek.olsak@amd.com>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
---
 src/gallium/drivers/r300/r300_screen.c            |  3 -
 src/gallium/drivers/r600/r600_pipe.c              |  6 --
 src/gallium/drivers/radeon/radeon_winsys.h        |  8 ---
 src/gallium/drivers/radeonsi/si_pipe.c            |  3 -
 src/gallium/include/pipe/p_screen.h               |  2 +
 src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c     | 44 ++-----------
 src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h     |  1 -
 src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 79 ++---------------------
 src/gallium/winsys/radeon/drm/radeon_drm_winsys.h |  1 -
 9 files changed, 13 insertions(+), 134 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/r300/r300_screen.c b/src/gallium/drivers/r300/r300_screen.c
index ff68ffd7bc65..6991e8396638 100644
--- a/src/gallium/drivers/r300/r300_screen.c
+++ b/src/gallium/drivers/r300/r300_screen.c
@@ -696,9 +696,6 @@  static void r300_destroy_screen(struct pipe_screen* pscreen)
     struct r300_screen* r300screen = r300_screen(pscreen);
     struct radeon_winsys *rws = radeon_winsys(pscreen);
 
-    if (rws && !rws->unref(rws))
-      return;
-
     mtx_destroy(&r300screen->cmask_mutex);
     slab_destroy_parent(&r300screen->pool_transfers);
 
diff --git a/src/gallium/drivers/r600/r600_pipe.c b/src/gallium/drivers/r600/r600_pipe.c
index 023f1b4bd14f..c0f7312e956d 100644
--- a/src/gallium/drivers/r600/r600_pipe.c
+++ b/src/gallium/drivers/r600/r600_pipe.c
@@ -612,12 +612,6 @@  static void r600_destroy_screen(struct pipe_screen* pscreen)
 {
 	struct r600_screen *rscreen = (struct r600_screen *)pscreen;
 
-	if (!rscreen)
-		return;
-
-	if (!rscreen->b.ws->unref(rscreen->b.ws))
-		return;
-
 	if (rscreen->global_pool) {
 		compute_memory_pool_delete(rscreen->global_pool);
 	}
diff --git a/src/gallium/drivers/radeon/radeon_winsys.h b/src/gallium/drivers/radeon/radeon_winsys.h
index 351edcd76f98..6ccd2ddbb0b2 100644
--- a/src/gallium/drivers/radeon/radeon_winsys.h
+++ b/src/gallium/drivers/radeon/radeon_winsys.h
@@ -230,14 +230,6 @@  struct radeon_winsys {
     struct pipe_screen *screen;
 
     /**
-     * Decrement the winsys reference count.
-     *
-     * \param ws  The winsys this function is called for.
-     * \return    True if the winsys and screen should be destroyed.
-     */
-    bool (*unref)(struct radeon_winsys *ws);
-
-    /**
      * Destroy this winsys.
      *
      * \param ws        The winsys this function is called from.
diff --git a/src/gallium/drivers/radeonsi/si_pipe.c b/src/gallium/drivers/radeonsi/si_pipe.c
index 395853c7d9f3..881ed0c58339 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -843,9 +843,6 @@  static void si_destroy_screen(struct pipe_screen* pscreen)
 	};
 	unsigned i;
 
-	if (!sscreen->b.ws->unref(sscreen->b.ws))
-		return;
-
 	util_queue_destroy(&sscreen->shader_compiler_queue);
 	util_queue_destroy(&sscreen->shader_compiler_queue_low_priority);
 
diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
index dbb14ef882c0..8f866f83d763 100644
--- a/src/gallium/include/pipe/p_screen.h
+++ b/src/gallium/include/pipe/p_screen.h
@@ -71,6 +71,8 @@  struct driOptionCache;
 struct pipe_screen {
    struct pipe_reference reference;
    int fd;
+   void *ws;
+
    void (*destroy)( struct pipe_screen * );
 
    const char *(*get_name)( struct pipe_screen * );
diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
index 26c7dacb9df4..258872388ce3 100644
--- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
+++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
@@ -48,7 +48,6 @@ 
 #endif
 
 static struct util_hash_table *dev_tab = NULL;
-static mtx_t dev_tab_mutex = _MTX_INITIALIZER_NP;
 
 /* Helper function to do the ioctls needed for setup and init. */
 static bool do_winsys_init(struct amdgpu_winsys *ws, int fd)
@@ -97,6 +96,7 @@  static void amdgpu_winsys_destroy(struct radeon_winsys *rws)
    pb_cache_deinit(&ws->bo_cache);
    mtx_destroy(&ws->global_bo_list_lock);
    do_winsys_deinit(ws);
+   util_hash_table_remove(dev_tab, ws->dev);
    FREE(rws);
 }
 
@@ -203,26 +203,6 @@  static int compare_dev(void *key1, void *key2)
    return key1 != key2;
 }
 
-static bool amdgpu_winsys_unref(struct radeon_winsys *rws)
-{
-   struct amdgpu_winsys *ws = (struct amdgpu_winsys*)rws;
-   bool destroy;
-
-   /* When the reference counter drops to zero, remove the device pointer
-    * from the table.
-    * This must happen while the mutex is locked, so that
-    * amdgpu_winsys_create in another thread doesn't get the winsys
-    * from the table when the counter drops to 0. */
-   mtx_lock(&dev_tab_mutex);
-
-   destroy = pipe_reference(&ws->reference, NULL);
-   if (destroy && dev_tab)
-      util_hash_table_remove(dev_tab, ws->dev);
-
-   mtx_unlock(&dev_tab_mutex);
-   return destroy;
-}
-
 static const char* amdgpu_get_chip_name(struct radeon_winsys *ws)
 {
    amdgpu_device_handle dev = ((struct amdgpu_winsys *)ws)->dev;
@@ -247,7 +227,6 @@  amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
    drmFreeVersion(version);
 
    /* Look up the winsys from the dev table. */
-   mtx_lock(&dev_tab_mutex);
    if (!dev_tab)
       dev_tab = util_hash_table_create(hash_dev, compare_dev);
 
@@ -255,7 +234,6 @@  amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
     * for the same fd. */
    r = amdgpu_device_initialize(fd, &drm_major, &drm_minor, &dev);
    if (r) {
-      mtx_unlock(&dev_tab_mutex);
       fprintf(stderr, "amdgpu: amdgpu_device_initialize failed.\n");
       return NULL;
    }
@@ -263,15 +241,14 @@  amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
    /* Lookup a winsys if we have already created one for this device. */
    ws = util_hash_table_get(dev_tab, dev);
    if (ws) {
-      pipe_reference(NULL, &ws->reference);
-      mtx_unlock(&dev_tab_mutex);
+      pipe_reference(NULL, &ws->base.screen->reference);
       return &ws->base;
    }
 
    /* Create a new winsys. */
    ws = CALLOC_STRUCT(amdgpu_winsys);
    if (!ws)
-      goto fail;
+      return NULL;
 
    ws->dev = dev;
    ws->info.drm_major = drm_major;
@@ -296,11 +273,7 @@  amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
 
    ws->info.min_alloc_size = 1 << AMDGPU_SLAB_MIN_SIZE_LOG2;
 
-   /* init reference */
-   pipe_reference_init(&ws->reference, 1);
-
    /* Set functions. */
-   ws->base.unref = amdgpu_winsys_unref;
    ws->base.destroy = amdgpu_winsys_destroy;
    ws->base.query_info = amdgpu_winsys_query_info;
    ws->base.cs_request_feature = amdgpu_cs_request_feature;
@@ -319,7 +292,6 @@  amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
    if (!util_queue_init(&ws->cs_queue, "amdgpu_cs", 8, 1,
                         UTIL_QUEUE_INIT_RESIZE_IF_FULL)) {
       amdgpu_winsys_destroy(&ws->base);
-      mtx_unlock(&dev_tab_mutex);
       return NULL;
    }
 
@@ -331,16 +303,12 @@  amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
    ws->base.screen = screen_create(&ws->base, config);
    if (!ws->base.screen) {
       amdgpu_winsys_destroy(&ws->base);
-      mtx_unlock(&dev_tab_mutex);
       return NULL;
    }
 
    util_hash_table_set(dev_tab, dev, ws);
-
-   /* We must unlock the mutex once the winsys is fully initialized, so that
-    * other threads attempting to create the winsys from the same fd will
-    * get a fully initialized winsys and not just half-way initialized. */
-   mtx_unlock(&dev_tab_mutex);
+   ws->base.screen->fd = -1;
+   pipe_reference_init(&ws->base.screen->reference, 1);
 
    return &ws->base;
 
@@ -349,7 +317,5 @@  fail_cache:
    do_winsys_deinit(ws);
 fail_alloc:
    FREE(ws);
-fail:
-   mtx_unlock(&dev_tab_mutex);
    return NULL;
 }
diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
index 7aca612f4523..52f00b33c1a6 100644
--- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
+++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
@@ -47,7 +47,6 @@  struct amdgpu_cs;
 
 struct amdgpu_winsys {
    struct radeon_winsys base;
-   struct pipe_reference reference;
    struct pb_cache bo_cache;
    struct pb_slabs bo_slabs;
 
diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
index 542e5494bebc..d27428598ed2 100644
--- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
+++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
@@ -37,17 +37,15 @@ 
 
 #include "util/u_memory.h"
 #include "util/u_hash_table.h"
+#include "util/u_screen.h"
 
 #include <xf86drm.h>
 #include <stdio.h>
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <radeon_surface.h>
 
-static struct util_hash_table *fd_tab = NULL;
-static mtx_t fd_tab_mutex = _MTX_INITIALIZER_NP;
 
 /* Enable/disable feature access for one command stream.
  * If enable == true, return true on success.
@@ -558,9 +556,6 @@  static void radeon_winsys_destroy(struct radeon_winsys *rws)
     mtx_destroy(&ws->bo_va_mutex);
     mtx_destroy(&ws->bo_fence_lock);
 
-    if (ws->fd >= 0)
-        close(ws->fd);
-
     FREE(rws);
 }
 
@@ -680,49 +675,8 @@  static bool radeon_read_registers(struct radeon_winsys *rws,
     return true;
 }
 
-static unsigned hash_fd(void *key)
-{
-    int fd = pointer_to_intptr(key);
-    struct stat stat;
-    fstat(fd, &stat);
-
-    return stat.st_dev ^ stat.st_ino ^ stat.st_rdev;
-}
-
-static int compare_fd(void *key1, void *key2)
-{
-    int fd1 = pointer_to_intptr(key1);
-    int fd2 = pointer_to_intptr(key2);
-    struct stat stat1, stat2;
-    fstat(fd1, &stat1);
-    fstat(fd2, &stat2);
-
-    return stat1.st_dev != stat2.st_dev ||
-           stat1.st_ino != stat2.st_ino ||
-           stat1.st_rdev != stat2.st_rdev;
-}
-
 DEBUG_GET_ONCE_BOOL_OPTION(thread, "RADEON_THREAD", true)
 
-static bool radeon_winsys_unref(struct radeon_winsys *ws)
-{
-    struct radeon_drm_winsys *rws = (struct radeon_drm_winsys*)ws;
-    bool destroy;
-
-    /* When the reference counter drops to zero, remove the fd from the table.
-     * This must happen while the mutex is locked, so that
-     * radeon_drm_winsys_create in another thread doesn't get the winsys
-     * from the table when the counter drops to 0. */
-    mtx_lock(&fd_tab_mutex);
-
-    destroy = pipe_reference(&rws->reference, NULL);
-    if (destroy && fd_tab)
-        util_hash_table_remove(fd_tab, intptr_to_pointer(rws->fd));
-
-    mtx_unlock(&fd_tab_mutex);
-    return destroy;
-}
-
 #define PTR_TO_UINT(x) ((unsigned)((intptr_t)(x)))
 
 static unsigned handle_hash(void *key)
@@ -740,24 +694,14 @@  radeon_drm_winsys_create(int fd, const struct pipe_screen_config *config,
 			 radeon_screen_create_t screen_create)
 {
     struct radeon_drm_winsys *ws;
+    struct pipe_screen *pscreen = pipe_screen_reference(fd);
 
-    mtx_lock(&fd_tab_mutex);
-    if (!fd_tab) {
-        fd_tab = util_hash_table_create(hash_fd, compare_fd);
-    }
-
-    ws = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
-    if (ws) {
-        pipe_reference(NULL, &ws->reference);
-        mtx_unlock(&fd_tab_mutex);
-        return &ws->base;
-    }
+    if (pscreen)
+        return pscreen->ws;
 
     ws = CALLOC_STRUCT(radeon_drm_winsys);
-    if (!ws) {
-        mtx_unlock(&fd_tab_mutex);
+    if (!ws)
         return NULL;
-    }
 
     ws->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
 
@@ -794,11 +738,8 @@  radeon_drm_winsys_create(int fd, const struct pipe_screen_config *config,
             goto fail_slab;
     }
 
-    /* init reference */
-    pipe_reference_init(&ws->reference, 1);
 
     /* Set functions. */
-    ws->base.unref = radeon_winsys_unref;
     ws->base.destroy = radeon_winsys_destroy;
     ws->base.query_info = radeon_query_info;
     ws->base.cs_request_feature = radeon_cs_request_feature;
@@ -835,16 +776,9 @@  radeon_drm_winsys_create(int fd, const struct pipe_screen_config *config,
     ws->base.screen = screen_create(&ws->base, config);
     if (!ws->base.screen) {
         radeon_winsys_destroy(&ws->base);
-        mtx_unlock(&fd_tab_mutex);
         return NULL;
     }
-
-    util_hash_table_set(fd_tab, intptr_to_pointer(ws->fd), ws);
-
-    /* We must unlock the mutex once the winsys is fully initialized, so that
-     * other threads attempting to create the winsys from the same fd will
-     * get a fully initialized winsys and not just half-way initialized. */
-    mtx_unlock(&fd_tab_mutex);
+    ws->base.screen->ws = &ws->base;
 
     return &ws->base;
 
@@ -854,7 +788,6 @@  fail_slab:
 fail_cache:
     pb_cache_deinit(&ws->bo_cache);
 fail1:
-    mtx_unlock(&fd_tab_mutex);
     if (ws->surf_man)
         radeon_surface_manager_free(ws->surf_man);
     if (ws->fd >= 0)
diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
index 362dab223980..46964cae5ad2 100644
--- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
+++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
@@ -50,7 +50,6 @@  enum radeon_generation {
 
 struct radeon_drm_winsys {
     struct radeon_winsys base;
-    struct pipe_reference reference;
     struct pb_cache bo_cache;
     struct pb_slabs bo_slabs;
 

Comments

You already know I can't accept the amdgpu part.

Anyway, I don't know if it's possible to keep the radeon part of the
patch, because there is some shared code you would need to keep as
well.

Marek

On Tue, Aug 8, 2017 at 12:58 AM, Rob Herring <robh@kernel.org> wrote:
> Use the common pipe_screen ref counting.
>
> radeon uses the common pipe_screen_{un}reference functions, but amdgpu
> is unique in its hashing the dev pointer rather than the fd, so the
> common fd hashing cannot be used. However, the same reference counting
> can be used instead of the private one. The mutex can be dropped as the
> pipe loader serializes the create_screen() and destroy() calls.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Cc: "Marek Olšák" <marek.olsak@amd.com>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> ---
>  src/gallium/drivers/r300/r300_screen.c            |  3 -
>  src/gallium/drivers/r600/r600_pipe.c              |  6 --
>  src/gallium/drivers/radeon/radeon_winsys.h        |  8 ---
>  src/gallium/drivers/radeonsi/si_pipe.c            |  3 -
>  src/gallium/include/pipe/p_screen.h               |  2 +
>  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c     | 44 ++-----------
>  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h     |  1 -
>  src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 79 ++---------------------
>  src/gallium/winsys/radeon/drm/radeon_drm_winsys.h |  1 -
>  9 files changed, 13 insertions(+), 134 deletions(-)
>
> diff --git a/src/gallium/drivers/r300/r300_screen.c b/src/gallium/drivers/r300/r300_screen.c
> index ff68ffd7bc65..6991e8396638 100644
> --- a/src/gallium/drivers/r300/r300_screen.c
> +++ b/src/gallium/drivers/r300/r300_screen.c
> @@ -696,9 +696,6 @@ static void r300_destroy_screen(struct pipe_screen* pscreen)
>      struct r300_screen* r300screen = r300_screen(pscreen);
>      struct radeon_winsys *rws = radeon_winsys(pscreen);
>
> -    if (rws && !rws->unref(rws))
> -      return;
> -
>      mtx_destroy(&r300screen->cmask_mutex);
>      slab_destroy_parent(&r300screen->pool_transfers);
>
> diff --git a/src/gallium/drivers/r600/r600_pipe.c b/src/gallium/drivers/r600/r600_pipe.c
> index 023f1b4bd14f..c0f7312e956d 100644
> --- a/src/gallium/drivers/r600/r600_pipe.c
> +++ b/src/gallium/drivers/r600/r600_pipe.c
> @@ -612,12 +612,6 @@ static void r600_destroy_screen(struct pipe_screen* pscreen)
>  {
>         struct r600_screen *rscreen = (struct r600_screen *)pscreen;
>
> -       if (!rscreen)
> -               return;
> -
> -       if (!rscreen->b.ws->unref(rscreen->b.ws))
> -               return;
> -
>         if (rscreen->global_pool) {
>                 compute_memory_pool_delete(rscreen->global_pool);
>         }
> diff --git a/src/gallium/drivers/radeon/radeon_winsys.h b/src/gallium/drivers/radeon/radeon_winsys.h
> index 351edcd76f98..6ccd2ddbb0b2 100644
> --- a/src/gallium/drivers/radeon/radeon_winsys.h
> +++ b/src/gallium/drivers/radeon/radeon_winsys.h
> @@ -230,14 +230,6 @@ struct radeon_winsys {
>      struct pipe_screen *screen;
>
>      /**
> -     * Decrement the winsys reference count.
> -     *
> -     * \param ws  The winsys this function is called for.
> -     * \return    True if the winsys and screen should be destroyed.
> -     */
> -    bool (*unref)(struct radeon_winsys *ws);
> -
> -    /**
>       * Destroy this winsys.
>       *
>       * \param ws        The winsys this function is called from.
> diff --git a/src/gallium/drivers/radeonsi/si_pipe.c b/src/gallium/drivers/radeonsi/si_pipe.c
> index 395853c7d9f3..881ed0c58339 100644
> --- a/src/gallium/drivers/radeonsi/si_pipe.c
> +++ b/src/gallium/drivers/radeonsi/si_pipe.c
> @@ -843,9 +843,6 @@ static void si_destroy_screen(struct pipe_screen* pscreen)
>         };
>         unsigned i;
>
> -       if (!sscreen->b.ws->unref(sscreen->b.ws))
> -               return;
> -
>         util_queue_destroy(&sscreen->shader_compiler_queue);
>         util_queue_destroy(&sscreen->shader_compiler_queue_low_priority);
>
> diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
> index dbb14ef882c0..8f866f83d763 100644
> --- a/src/gallium/include/pipe/p_screen.h
> +++ b/src/gallium/include/pipe/p_screen.h
> @@ -71,6 +71,8 @@ struct driOptionCache;
>  struct pipe_screen {
>     struct pipe_reference reference;
>     int fd;
> +   void *ws;
> +
>     void (*destroy)( struct pipe_screen * );
>
>     const char *(*get_name)( struct pipe_screen * );
> diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> index 26c7dacb9df4..258872388ce3 100644
> --- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> @@ -48,7 +48,6 @@
>  #endif
>
>  static struct util_hash_table *dev_tab = NULL;
> -static mtx_t dev_tab_mutex = _MTX_INITIALIZER_NP;
>
>  /* Helper function to do the ioctls needed for setup and init. */
>  static bool do_winsys_init(struct amdgpu_winsys *ws, int fd)
> @@ -97,6 +96,7 @@ static void amdgpu_winsys_destroy(struct radeon_winsys *rws)
>     pb_cache_deinit(&ws->bo_cache);
>     mtx_destroy(&ws->global_bo_list_lock);
>     do_winsys_deinit(ws);
> +   util_hash_table_remove(dev_tab, ws->dev);
>     FREE(rws);
>  }
>
> @@ -203,26 +203,6 @@ static int compare_dev(void *key1, void *key2)
>     return key1 != key2;
>  }
>
> -static bool amdgpu_winsys_unref(struct radeon_winsys *rws)
> -{
> -   struct amdgpu_winsys *ws = (struct amdgpu_winsys*)rws;
> -   bool destroy;
> -
> -   /* When the reference counter drops to zero, remove the device pointer
> -    * from the table.
> -    * This must happen while the mutex is locked, so that
> -    * amdgpu_winsys_create in another thread doesn't get the winsys
> -    * from the table when the counter drops to 0. */
> -   mtx_lock(&dev_tab_mutex);
> -
> -   destroy = pipe_reference(&ws->reference, NULL);
> -   if (destroy && dev_tab)
> -      util_hash_table_remove(dev_tab, ws->dev);
> -
> -   mtx_unlock(&dev_tab_mutex);
> -   return destroy;
> -}
> -
>  static const char* amdgpu_get_chip_name(struct radeon_winsys *ws)
>  {
>     amdgpu_device_handle dev = ((struct amdgpu_winsys *)ws)->dev;
> @@ -247,7 +227,6 @@ amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
>     drmFreeVersion(version);
>
>     /* Look up the winsys from the dev table. */
> -   mtx_lock(&dev_tab_mutex);
>     if (!dev_tab)
>        dev_tab = util_hash_table_create(hash_dev, compare_dev);
>
> @@ -255,7 +234,6 @@ amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
>      * for the same fd. */
>     r = amdgpu_device_initialize(fd, &drm_major, &drm_minor, &dev);
>     if (r) {
> -      mtx_unlock(&dev_tab_mutex);
>        fprintf(stderr, "amdgpu: amdgpu_device_initialize failed.\n");
>        return NULL;
>     }
> @@ -263,15 +241,14 @@ amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
>     /* Lookup a winsys if we have already created one for this device. */
>     ws = util_hash_table_get(dev_tab, dev);
>     if (ws) {
> -      pipe_reference(NULL, &ws->reference);
> -      mtx_unlock(&dev_tab_mutex);
> +      pipe_reference(NULL, &ws->base.screen->reference);
>        return &ws->base;
>     }
>
>     /* Create a new winsys. */
>     ws = CALLOC_STRUCT(amdgpu_winsys);
>     if (!ws)
> -      goto fail;
> +      return NULL;
>
>     ws->dev = dev;
>     ws->info.drm_major = drm_major;
> @@ -296,11 +273,7 @@ amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
>
>     ws->info.min_alloc_size = 1 << AMDGPU_SLAB_MIN_SIZE_LOG2;
>
> -   /* init reference */
> -   pipe_reference_init(&ws->reference, 1);
> -
>     /* Set functions. */
> -   ws->base.unref = amdgpu_winsys_unref;
>     ws->base.destroy = amdgpu_winsys_destroy;
>     ws->base.query_info = amdgpu_winsys_query_info;
>     ws->base.cs_request_feature = amdgpu_cs_request_feature;
> @@ -319,7 +292,6 @@ amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
>     if (!util_queue_init(&ws->cs_queue, "amdgpu_cs", 8, 1,
>                          UTIL_QUEUE_INIT_RESIZE_IF_FULL)) {
>        amdgpu_winsys_destroy(&ws->base);
> -      mtx_unlock(&dev_tab_mutex);
>        return NULL;
>     }
>
> @@ -331,16 +303,12 @@ amdgpu_winsys_create(int fd, const struct pipe_screen_config *config,
>     ws->base.screen = screen_create(&ws->base, config);
>     if (!ws->base.screen) {
>        amdgpu_winsys_destroy(&ws->base);
> -      mtx_unlock(&dev_tab_mutex);
>        return NULL;
>     }
>
>     util_hash_table_set(dev_tab, dev, ws);
> -
> -   /* We must unlock the mutex once the winsys is fully initialized, so that
> -    * other threads attempting to create the winsys from the same fd will
> -    * get a fully initialized winsys and not just half-way initialized. */
> -   mtx_unlock(&dev_tab_mutex);
> +   ws->base.screen->fd = -1;
> +   pipe_reference_init(&ws->base.screen->reference, 1);
>
>     return &ws->base;
>
> @@ -349,7 +317,5 @@ fail_cache:
>     do_winsys_deinit(ws);
>  fail_alloc:
>     FREE(ws);
> -fail:
> -   mtx_unlock(&dev_tab_mutex);
>     return NULL;
>  }
> diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
> index 7aca612f4523..52f00b33c1a6 100644
> --- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
> +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
> @@ -47,7 +47,6 @@ struct amdgpu_cs;
>
>  struct amdgpu_winsys {
>     struct radeon_winsys base;
> -   struct pipe_reference reference;
>     struct pb_cache bo_cache;
>     struct pb_slabs bo_slabs;
>
> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> index 542e5494bebc..d27428598ed2 100644
> --- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> @@ -37,17 +37,15 @@
>
>  #include "util/u_memory.h"
>  #include "util/u_hash_table.h"
> +#include "util/u_screen.h"
>
>  #include <xf86drm.h>
>  #include <stdio.h>
>  #include <sys/types.h>
> -#include <sys/stat.h>
>  #include <unistd.h>
>  #include <fcntl.h>
>  #include <radeon_surface.h>
>
> -static struct util_hash_table *fd_tab = NULL;
> -static mtx_t fd_tab_mutex = _MTX_INITIALIZER_NP;
>
>  /* Enable/disable feature access for one command stream.
>   * If enable == true, return true on success.
> @@ -558,9 +556,6 @@ static void radeon_winsys_destroy(struct radeon_winsys *rws)
>      mtx_destroy(&ws->bo_va_mutex);
>      mtx_destroy(&ws->bo_fence_lock);
>
> -    if (ws->fd >= 0)
> -        close(ws->fd);
> -
>      FREE(rws);
>  }
>
> @@ -680,49 +675,8 @@ static bool radeon_read_registers(struct radeon_winsys *rws,
>      return true;
>  }
>
> -static unsigned hash_fd(void *key)
> -{
> -    int fd = pointer_to_intptr(key);
> -    struct stat stat;
> -    fstat(fd, &stat);
> -
> -    return stat.st_dev ^ stat.st_ino ^ stat.st_rdev;
> -}
> -
> -static int compare_fd(void *key1, void *key2)
> -{
> -    int fd1 = pointer_to_intptr(key1);
> -    int fd2 = pointer_to_intptr(key2);
> -    struct stat stat1, stat2;
> -    fstat(fd1, &stat1);
> -    fstat(fd2, &stat2);
> -
> -    return stat1.st_dev != stat2.st_dev ||
> -           stat1.st_ino != stat2.st_ino ||
> -           stat1.st_rdev != stat2.st_rdev;
> -}
> -
>  DEBUG_GET_ONCE_BOOL_OPTION(thread, "RADEON_THREAD", true)
>
> -static bool radeon_winsys_unref(struct radeon_winsys *ws)
> -{
> -    struct radeon_drm_winsys *rws = (struct radeon_drm_winsys*)ws;
> -    bool destroy;
> -
> -    /* When the reference counter drops to zero, remove the fd from the table.
> -     * This must happen while the mutex is locked, so that
> -     * radeon_drm_winsys_create in another thread doesn't get the winsys
> -     * from the table when the counter drops to 0. */
> -    mtx_lock(&fd_tab_mutex);
> -
> -    destroy = pipe_reference(&rws->reference, NULL);
> -    if (destroy && fd_tab)
> -        util_hash_table_remove(fd_tab, intptr_to_pointer(rws->fd));
> -
> -    mtx_unlock(&fd_tab_mutex);
> -    return destroy;
> -}
> -
>  #define PTR_TO_UINT(x) ((unsigned)((intptr_t)(x)))
>
>  static unsigned handle_hash(void *key)
> @@ -740,24 +694,14 @@ radeon_drm_winsys_create(int fd, const struct pipe_screen_config *config,
>                          radeon_screen_create_t screen_create)
>  {
>      struct radeon_drm_winsys *ws;
> +    struct pipe_screen *pscreen = pipe_screen_reference(fd);
>
> -    mtx_lock(&fd_tab_mutex);
> -    if (!fd_tab) {
> -        fd_tab = util_hash_table_create(hash_fd, compare_fd);
> -    }
> -
> -    ws = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
> -    if (ws) {
> -        pipe_reference(NULL, &ws->reference);
> -        mtx_unlock(&fd_tab_mutex);
> -        return &ws->base;
> -    }
> +    if (pscreen)
> +        return pscreen->ws;
>
>      ws = CALLOC_STRUCT(radeon_drm_winsys);
> -    if (!ws) {
> -        mtx_unlock(&fd_tab_mutex);
> +    if (!ws)
>          return NULL;
> -    }
>
>      ws->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
>
> @@ -794,11 +738,8 @@ radeon_drm_winsys_create(int fd, const struct pipe_screen_config *config,
>              goto fail_slab;
>      }
>
> -    /* init reference */
> -    pipe_reference_init(&ws->reference, 1);
>
>      /* Set functions. */
> -    ws->base.unref = radeon_winsys_unref;
>      ws->base.destroy = radeon_winsys_destroy;
>      ws->base.query_info = radeon_query_info;
>      ws->base.cs_request_feature = radeon_cs_request_feature;
> @@ -835,16 +776,9 @@ radeon_drm_winsys_create(int fd, const struct pipe_screen_config *config,
>      ws->base.screen = screen_create(&ws->base, config);
>      if (!ws->base.screen) {
>          radeon_winsys_destroy(&ws->base);
> -        mtx_unlock(&fd_tab_mutex);
>          return NULL;
>      }
> -
> -    util_hash_table_set(fd_tab, intptr_to_pointer(ws->fd), ws);
> -
> -    /* We must unlock the mutex once the winsys is fully initialized, so that
> -     * other threads attempting to create the winsys from the same fd will
> -     * get a fully initialized winsys and not just half-way initialized. */
> -    mtx_unlock(&fd_tab_mutex);
> +    ws->base.screen->ws = &ws->base;
>
>      return &ws->base;
>
> @@ -854,7 +788,6 @@ fail_slab:
>  fail_cache:
>      pb_cache_deinit(&ws->bo_cache);
>  fail1:
> -    mtx_unlock(&fd_tab_mutex);
>      if (ws->surf_man)
>          radeon_surface_manager_free(ws->surf_man);
>      if (ws->fd >= 0)
> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
> index 362dab223980..46964cae5ad2 100644
> --- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
> @@ -50,7 +50,6 @@ enum radeon_generation {
>
>  struct radeon_drm_winsys {
>      struct radeon_winsys base;
> -    struct pipe_reference reference;
>      struct pb_cache bo_cache;
>      struct pb_slabs bo_slabs;
>
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Wed, Aug 9, 2017 at 3:19 PM, Marek Olšák <maraeo@gmail.com> wrote:
> You already know I can't accept the amdgpu part.

You don't agree with my explanation in the other patch?

> Anyway, I don't know if it's possible to keep the radeon part of the
> patch, because there is some shared code you would need to keep as
> well.

Yes, I think it should be possible.

Rob
On Wed, Aug 9, 2017 at 10:34 PM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Aug 9, 2017 at 3:19 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> You already know I can't accept the amdgpu part.
>
> You don't agree with my explanation in the other patch?

OK. This patch is:

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Marek