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 <r...@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 <r...@kernel.org>
> Cc: "Marek Olšák" <marek.ol...@amd.com>
> Cc: Ilia Mirkin <imir...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
