[Mesa-dev] [PATCH v3 05/11] nouveau: use common screen ref counting
Rob Herring
robh at kernel.org
Wed Jul 20 22:51:33 UTC 2016
On Wed, Jul 20, 2016 at 5:32 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Wed, Jul 20, 2016 at 6:29 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Wed, Jul 20, 2016 at 6:25 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 68d8317..591cf92 100644
>>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> @@ -408,9 +408,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 303ecf1..7dbf66f 100644
>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>>> @@ -428,9 +428,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 f681631..f789de4 100644
>>> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>>> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>>> @@ -485,9 +485,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 f90572f..9ad3c57 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);
>>
>> I haven't looked at any other bits of the change, but this still has
>> to be the dupfd. I think that's pscreen->fd in your updated series.
>
> Oh, except you can't do that because the screen hasn't been created
> yet. Well, either way, this has to be a dup'd fd.
Right. And it needs to be dup'ed only after checking for an existing
pipe screen. I'll make it the caller of pipe_screen_reference_init
responsibility to dup the fd.
Rob
More information about the mesa-dev
mailing list