[Mesa-dev] [RFC 7/7] radeon: remove screen ref counting

[Emil Velikov]

On 17 June 2016 at 18:45, Rob Herring <robh at kernel.org> wrote:
> Now that the pipe-loader is reference counting the screen creation, it
> is unnecessary to do in it the winsys/driver.
>
> Signed-off-by: Rob Herring <robh at kernel.org>
> Cc: "Marek Olšák" <marek.olsak at amd.com>
> Cc: Ilia Mirkin <imirkin at 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            |  6 --
>  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c     | 66 +------------------
>  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h     |  1 -
>  src/gallium/winsys/radeon/drm/radeon_drm_winsys.c | 80 +----------------------
>  7 files changed, 3 insertions(+), 167 deletions(-)
>
> diff --git a/src/gallium/drivers/r300/r300_screen.c b/src/gallium/drivers/r300/r300_screen.c
> index 681681b..5d2d955 100644
> --- a/src/gallium/drivers/r300/r300_screen.c
> +++ b/src/gallium/drivers/r300/r300_screen.c
> @@ -674,9 +674,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;
> -
>      pipe_mutex_destroy(r300screen->cmask_mutex);
>
>      if (rws)
> diff --git a/src/gallium/drivers/r600/r600_pipe.c b/src/gallium/drivers/r600/r600_pipe.c
> index a49b00f..66cb78c 100644
> --- a/src/gallium/drivers/r600/r600_pipe.c
> +++ b/src/gallium/drivers/r600/r600_pipe.c
> @@ -566,12 +566,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 c2d1f9e..ffe0d83 100644
> --- a/src/gallium/drivers/radeon/radeon_winsys.h
> +++ b/src/gallium/drivers/radeon/radeon_winsys.h
> @@ -416,14 +416,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 0c601da..f3256fc 100644
> --- a/src/gallium/drivers/radeonsi/si_pipe.c
> +++ b/src/gallium/drivers/radeonsi/si_pipe.c
> @@ -628,12 +628,6 @@ static void si_destroy_screen(struct pipe_screen* pscreen)
>         };
>         unsigned i;
>
> -       if (!sscreen)
> -               return;
> -
> -       if (!sscreen->b.ws->unref(sscreen->b.ws))
> -               return;
> -
>         /* Free shader parts. */
>         for (i = 0; i < ARRAY_SIZE(parts); i++) {
>                 while (parts[i]) {
> diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> index 7016221..39b4a11 100644
> --- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> @@ -34,7 +34,6 @@
>  #include "amdgpu_cs.h"
>  #include "amdgpu_public.h"
>
> -#include "util/u_hash_table.h"
>  #include <amdgpu_drm.h>
>  #include <xf86drm.h>
>  #include <stdio.h>
> @@ -59,9 +58,6 @@
>  #define     CIK__PIPE_CONFIG__ADDR_SURF_P16_32X32_8X16   16
>  #define     CIK__PIPE_CONFIG__ADDR_SURF_P16_32X32_16X16  17
>
> -static struct util_hash_table *dev_tab = NULL;
> -pipe_static_mutex(dev_tab_mutex);
> -
>  static unsigned cik_get_num_tile_pipes(struct amdgpu_gpu_info *info)
>  {
>     unsigned mode2d = info->gb_tile_mode[CIK_TILE_MODE_COLOR_2D];
> @@ -386,20 +382,6 @@ static bool amdgpu_read_registers(struct radeon_winsys *rws,
>                                     0xffffffff, 0, out) == 0;
>  }
>
> -static unsigned hash_dev(void *key)
> -{
> -#if defined(PIPE_ARCH_X86_64)
> -   return pointer_to_intptr(key) ^ (pointer_to_intptr(key) >> 32);
> -#else
> -   return pointer_to_intptr(key);
> -#endif
> -}
> -
As you can see above the hashing algo is different for AMDGPU. Not
familiar with the story behind any of this, so hopefully the AMD folk
will give you some insights.

FWIW I'm inclined to keep the winsys/radeon and winsys/amdgpu
differences separate, although not sure if that's possible.
Note that vmwgfx has almost(?) identical implementation that one could nuke.

Last but not least the biggest and a bit annoying part. As-is the
series will break GL/VDPAU interiop - the 'thing' that inspired all
this work initially.

To avoid that, we need to promote the fd_hash symbol to public, in
combination with the lock (if needed) and ideally a version (for
sanity checking). As we do that we should replace all the existing
symbols in src/gallium/targets/dri-vdpau.dyn with the new one. Plus
one should short circuit the nouveau/radeon/amdgpu machinery to avoid
intermittently breaking things. Don't know how much we care about that
last one ;-)

All that aside, thanks again for looking into this.
Emil