[Mesa-dev] [RFC 7/7] radeon: remove screen ref counting
Marek Olšák
maraeo at gmail.com
Mon Jun 20 15:42:22 UTC 2016
On Fri, Jun 17, 2016 at 8:45 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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.
Yes, the algorithm is different. My preference is to keep the current
code that relies on the libdrm_amdgpu behavior. The store behind that
is simple: it's used by both Mesa and the AMD GPU-PRO closed-source
driver.
The general rule is to have 1 screen/winsys pair per physical device,
which can be represented by different types of fds. A screen/winsys
pair can't be used by multiple devices ever.
>
> 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 ;-)
Yes, this is the biggest showstopper, thus I'm not accepting this patch.
Marek
More information about the mesa-dev
mailing list