[Mesa-dev] [PATCH 05/11] nouveau: use common screen ref counting
Rob Herring
robh at kernel.org
Fri Jun 24 04:20:52 UTC 2016
On Thu, Jun 23, 2016 at 7:01 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> As before, you need to keep the dupfd. There's even a long comment there to
> explain why.
And I responded why I don't think it is:
"Those 2 commits predate commit 13bccee87d63 which dup's the fd for the
DRI2 ST before calling pipe loader functions. XA and vl_winsys_drm do
the same. vl_winsys_dri opens the fd itself. I'm not sure about
vl_winsys_dri3. d3dadapter9 appears to take ownership of the fd. So my
conclusion is still that it shouldn't be necessary to dup the fd."
To put it another way the call stack for DRI2 looks like this where fd
is the original and dup_fd is the dup() one:
driCreateNewScreen2(fd)
dri2_init_screen(fd)
dup(fd)
pipe_loader_drm_probe_fd(dup_fd)
pipe_loader_create_screen()
pipe_nouveau_create_screen(dup_fd)
Why does the fd need to be dup'ed twice? I looks to me like the same
problem was fixed twice, once in some drivers and once in common code
at around the same time. I could be missing something, but I'd like to
understand what.
Rob
>
> On Jun 23, 2016 7:58 PM, "Rob Herring" <robh at kernel.org> wrote:
>>
>> Use the common pipe_screen ref counting and fd hashing functions. The
>> mutex can be dropped as the pipe loader protects the create_screen()
>> calls.
>>
>> Signed-off-by: Rob Herring <robh at kernel.org>
>> Cc: Alexandre Courbot <acourbot at nvidia.com>
>> ---
>> src/gallium/drivers/nouveau/nouveau_screen.c | 6 --
>> src/gallium/drivers/nouveau/nouveau_screen.h | 4 -
>> src/gallium/drivers/nouveau/nv30/nv30_screen.c | 3 -
>> src/gallium/drivers/nouveau/nv50/nv50_screen.c | 3 -
>> src/gallium/drivers/nouveau/nvc0/nvc0_screen.c | 3 -
>> .../winsys/nouveau/drm/nouveau_drm_winsys.c | 89
>> ++--------------------
>> 6 files changed, 7 insertions(+), 101 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.c
>> b/src/gallium/drivers/nouveau/nouveau_screen.c
>> index 2c421cc..41d4bef 100644
>> --- a/src/gallium/drivers/nouveau/nouveau_screen.c
>> +++ b/src/gallium/drivers/nouveau/nouveau_screen.c
>> @@ -159,12 +159,6 @@ nouveau_screen_init(struct nouveau_screen *screen,
>> struct nouveau_device *dev)
>> screen->drm = nouveau_drm(&dev->object);
>> screen->device = dev;
>>
>> - /*
>> - * this is initialized to 1 in nouveau_drm_screen_create after screen
>> - * is fully constructed and added to the global screen list.
>> - */
>> - screen->refcount = -1;
>> -
>> if (dev->chipset < 0xc0) {
>> data = &nv04_data;
>> size = sizeof(nv04_data);
>> diff --git a/src/gallium/drivers/nouveau/nouveau_screen.h
>> b/src/gallium/drivers/nouveau/nouveau_screen.h
>> index 28c4760..55156c3 100644
>> --- a/src/gallium/drivers/nouveau/nouveau_screen.h
>> +++ b/src/gallium/drivers/nouveau/nouveau_screen.h
>> @@ -23,8 +23,6 @@ struct nouveau_screen {
>> struct nouveau_client *client;
>> struct nouveau_pushbuf *pushbuf;
>>
>> - int refcount;
>> -
>> unsigned vidmem_bindings; /* PIPE_BIND_* where VRAM placement is
>> desired */
>> unsigned sysmem_bindings; /* PIPE_BIND_* where GART placement is
>> desired */
>> unsigned lowmem_bindings; /* PIPE_BIND_* that require an address < 4
>> GiB */
>> @@ -119,8 +117,6 @@ nouveau_screen(struct pipe_screen *pscreen)
>> return (struct nouveau_screen *)pscreen;
>> }
>>
>> -bool nouveau_drm_screen_unref(struct nouveau_screen *screen);
>> -
>> bool
>> nouveau_screen_bo_get_handle(struct pipe_screen *pscreen,
>> struct nouveau_bo *bo,
>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> index de798cf..08194fd 100644
>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> @@ -406,9 +406,6 @@ nv30_screen_destroy(struct pipe_screen *pscreen)
>> {
>> struct nv30_screen *screen = nv30_screen(pscreen);
>>
>> - if (!nouveau_drm_screen_unref(&screen->base))
>> - return;
>> -
>> if (screen->base.fence.current) {
>> struct nouveau_fence *current = NULL;
>>
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> index 03f4591..1654d0e 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> @@ -425,9 +425,6 @@ nv50_screen_destroy(struct pipe_screen *pscreen)
>> {
>> struct nv50_screen *screen = nv50_screen(pscreen);
>>
>> - if (!nouveau_drm_screen_unref(&screen->base))
>> - return;
>> -
>> if (screen->base.fence.current) {
>> struct nouveau_fence *current = NULL;
>>
>> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> index b9437b2..712293a 100644
>> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> @@ -481,9 +481,6 @@ nvc0_screen_destroy(struct pipe_screen *pscreen)
>> {
>> struct nvc0_screen *screen = nvc0_screen(pscreen);
>>
>> - if (!nouveau_drm_screen_unref(&screen->base))
>> - return;
>> -
>> if (screen->base.fence.current) {
>> struct nouveau_fence *current = NULL;
>>
>> diff --git a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> index 598ffcb..b5f4247 100644
>> --- a/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> +++ b/src/gallium/winsys/nouveau/drm/nouveau_drm_winsys.c
>> @@ -1,12 +1,9 @@
>> -#include <sys/stat.h>
>> #include <unistd.h>
>> #include "pipe/p_context.h"
>> #include "pipe/p_state.h"
>> #include "util/u_format.h"
>> #include "util/u_memory.h"
>> -#include "util/u_inlines.h"
>> -#include "util/u_hash_table.h"
>> -#include "os/os_thread.h"
>> +#include "util/u_screen.h"
>>
>> #include "nouveau_drm_public.h"
>>
>> @@ -16,47 +13,6 @@
>> #include <nvif/class.h>
>> #include <nvif/cl0080.h>
>>
>> -static struct util_hash_table *fd_tab = NULL;
>> -
>> -pipe_static_mutex(nouveau_screen_mutex);
>> -
>> -bool nouveau_drm_screen_unref(struct nouveau_screen *screen)
>> -{
>> - int ret;
>> - if (screen->refcount == -1)
>> - return true;
>> -
>> - pipe_mutex_lock(nouveau_screen_mutex);
>> - ret = --screen->refcount;
>> - assert(ret >= 0);
>> - if (ret == 0)
>> - util_hash_table_remove(fd_tab,
>> intptr_to_pointer(screen->drm->fd));
>> - pipe_mutex_unlock(nouveau_screen_mutex);
>> - return ret == 0;
>> -}
>> -
>> -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;
>> -}
>> -
>> PUBLIC struct pipe_screen *
>> nouveau_drm_screen_create(int fd)
>> {
>> @@ -64,36 +20,13 @@ nouveau_drm_screen_create(int fd)
>> struct nouveau_device *dev = NULL;
>> struct nouveau_screen *(*init)(struct nouveau_device *);
>> struct nouveau_screen *screen = NULL;
>> - int ret, dupfd;
>> -
>> - pipe_mutex_lock(nouveau_screen_mutex);
>> - if (!fd_tab) {
>> - fd_tab = util_hash_table_create(hash_fd, compare_fd);
>> - if (!fd_tab) {
>> - pipe_mutex_unlock(nouveau_screen_mutex);
>> - return NULL;
>> - }
>> - }
>> -
>> - screen = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
>> - if (screen) {
>> - screen->refcount++;
>> - pipe_mutex_unlock(nouveau_screen_mutex);
>> - return &screen->base;
>> - }
>> + struct pipe_screen *pscreen = pipe_screen_reference(fd);
>> + int ret;
>>
>> - /* Since the screen re-use is based on the device node and not the
>> fd,
>> - * create a copy of the fd to be owned by the device. Otherwise a
>> - * scenario could occur where two screens are created, and the
>> first
>> - * one is shut down, along with the fd being closed. The second
>> - * (identical) screen would now have a reference to the closed fd.
>> We
>> - * avoid this by duplicating the original fd. Note that
>> - * nouveau_device_wrap does not close the fd in case of a device
>> - * creation error.
>> - */
>> - dupfd = dup(fd);
>> + if (pscreen)
>> + return pscreen;
>>
>> - ret = nouveau_drm_new(dupfd, &drm);
>> + ret = nouveau_drm_new(fd, &drm);
>> if (ret)
>> goto err;
>>
>> @@ -135,13 +68,7 @@ nouveau_drm_screen_create(int fd)
>> if (!screen || !screen->base.context_create)
>> goto err;
>>
>> - /* Use dupfd in hash table, to avoid errors if the original fd
>> gets
>> - * closed by its owner. The hash key needs to live at least as
>> long as
>> - * the screen.
>> - */
>> - util_hash_table_set(fd_tab, intptr_to_pointer(dupfd), screen);
>> - screen->refcount = 1;
>> - pipe_mutex_unlock(nouveau_screen_mutex);
>> + pipe_screen_reference_init(&screen->base, fd);
>> return &screen->base;
>>
>> err:
>> @@ -150,8 +77,6 @@ err:
>> } else {
>> nouveau_device_del(&dev);
>> nouveau_drm_del(&drm);
>> - close(dupfd);
>> }
>> - pipe_mutex_unlock(nouveau_screen_mutex);
>> return NULL;
>> }
>> --
>> 2.9.0
>>
>
More information about the mesa-dev
mailing list