[Mesa-dev,v4,03/11] gallium: add common pipe_screen reference counting functions

Submitted by Rob Herring on July 22, 2016, 4:22 p.m.

Details

Message ID 20160722162222.21871-4-robh@kernel.org
State New
Headers show
Series "Common pipe screen ref counting" ( rev: 4 ) in Mesa

Not browsing as part of any series.

Commit Message

Rob Herring July 22, 2016, 4:22 p.m.
In order to prevent multiple pipe_screens being created in the same
process, lookup of the DRM FD and reference counting of the pipe_screen
are needed. Several implementations of this exist in various gallium
drivers/winsys already. This creates a common version which is opt-in
for winsys implementations.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 src/gallium/auxiliary/Makefile.sources |   2 +
 src/gallium/auxiliary/util/u_screen.c  | 114 +++++++++++++++++++++++++++++++++
 src/gallium/auxiliary/util/u_screen.h  |  32 +++++++++
 src/gallium/include/pipe/p_screen.h    |   3 +
 4 files changed, 151 insertions(+)
 create mode 100644 src/gallium/auxiliary/util/u_screen.c
 create mode 100644 src/gallium/auxiliary/util/u_screen.h

Patch hide | download patch | download mbox

diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
index e0311bf..197ed36 100644
--- a/src/gallium/auxiliary/Makefile.sources
+++ b/src/gallium/auxiliary/Makefile.sources
@@ -284,6 +284,8 @@  C_SOURCES := \
 	util/u_ringbuffer.h \
 	util/u_sampler.c \
 	util/u_sampler.h \
+	util/u_screen.c \
+	util/u_screen.h \
 	util/u_simple_shaders.c \
 	util/u_simple_shaders.h \
 	util/u_slab.c \
diff --git a/src/gallium/auxiliary/util/u_screen.c b/src/gallium/auxiliary/util/u_screen.c
new file mode 100644
index 0000000..47bad11
--- /dev/null
+++ b/src/gallium/auxiliary/util/u_screen.c
@@ -0,0 +1,114 @@ 
+/*
+ * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
+ * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
+ * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+ * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+ * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * Functions for managing pipe_screen's
+ */
+
+#include <sys/stat.h>
+
+#include "os/os_thread.h"
+
+#include "pipe/p_screen.h"
+#include "util/u_hash_table.h"
+#include "util/u_inlines.h"
+#include "util/u_pointer.h"
+#include "util/u_screen.h"
+
+static struct util_hash_table *fd_tab = NULL;
+pipe_static_mutex(fd_tab_mutex);
+
+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;
+}
+
+struct pipe_screen *
+pipe_screen_reference(int fd)
+{
+   struct pipe_screen *pscreen;
+
+   if (!fd_tab) {
+      fd_tab = util_hash_table_create(hash_fd, compare_fd);
+      return NULL;
+   }
+
+   pipe_mutex_lock(fd_tab_mutex);
+   pscreen = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
+   if (pscreen)
+      pipe_reference(NULL, &pscreen->reference);
+   pipe_mutex_unlock(fd_tab_mutex);
+
+   return pscreen;
+}
+
+boolean
+pipe_screen_unreference(struct pipe_screen *pscreen)
+{
+   boolean destroy;
+
+   if (!pscreen)
+      return FALSE;
+
+   /* Work-around until all pipe_screens have ref counting */
+   if (!pipe_is_referenced(&pscreen->reference)) {
+      pscreen->destroy(pscreen);
+      return TRUE;
+   }
+
+   pipe_mutex_lock(fd_tab_mutex);
+   destroy = pipe_reference(&pscreen->reference, NULL);
+   if (destroy) {
+      pscreen->destroy(pscreen);
+      util_hash_table_remove(fd_tab, intptr_to_pointer(pscreen->fd));
+      close(pscreen->fd);
+   }
+   pipe_mutex_unlock(fd_tab_mutex);
+   return destroy;
+}
+
+
+void pipe_screen_reference_init(struct pipe_screen *pscreen, int fd)
+{
+   pscreen->fd = fd;
+   pipe_reference_init(&pscreen->reference, 1);
+   util_hash_table_set(fd_tab, intptr_to_pointer(pscreen->fd), pscreen);
+}
diff --git a/src/gallium/auxiliary/util/u_screen.h b/src/gallium/auxiliary/util/u_screen.h
new file mode 100644
index 0000000..fc91782
--- /dev/null
+++ b/src/gallium/auxiliary/util/u_screen.h
@@ -0,0 +1,32 @@ 
+/*
+ * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
+ * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
+ * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+ * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+ * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+#include "pipe/p_defines.h"
+
+struct pipe_screen;
+
+struct pipe_screen *pipe_screen_reference(int fd);
+
+boolean pipe_screen_unreference(struct pipe_screen *pscreen);
+
+void pipe_screen_reference_init(struct pipe_screen *pscreen, int fd);
diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
index 755291a..1119bde 100644
--- a/src/gallium/include/pipe/p_screen.h
+++ b/src/gallium/include/pipe/p_screen.h
@@ -41,6 +41,7 @@ 
 #include "pipe/p_compiler.h"
 #include "pipe/p_format.h"
 #include "pipe/p_defines.h"
+#include "pipe/p_state.h"
 #include "pipe/p_video_enums.h"
 
 
@@ -66,6 +67,8 @@  struct pipe_memory_info;
  * context.
  */
 struct pipe_screen {
+   struct pipe_reference reference;
+   int fd;
    void (*destroy)( struct pipe_screen * );
 
    const char *(*get_name)( struct pipe_screen * );

Comments

On Fri, Jul 22, 2016 at 12:22 PM, Rob Herring <robh@kernel.org> wrote:
> In order to prevent multiple pipe_screens being created in the same
> process, lookup of the DRM FD and reference counting of the pipe_screen
> are needed. Several implementations of this exist in various gallium
> drivers/winsys already. This creates a common version which is opt-in
> for winsys implementations.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  src/gallium/auxiliary/Makefile.sources |   2 +
>  src/gallium/auxiliary/util/u_screen.c  | 114 +++++++++++++++++++++++++++++++++
>  src/gallium/auxiliary/util/u_screen.h  |  32 +++++++++
>  src/gallium/include/pipe/p_screen.h    |   3 +
>  4 files changed, 151 insertions(+)
>  create mode 100644 src/gallium/auxiliary/util/u_screen.c
>  create mode 100644 src/gallium/auxiliary/util/u_screen.h
>
> diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
> index e0311bf..197ed36 100644
> --- a/src/gallium/auxiliary/Makefile.sources
> +++ b/src/gallium/auxiliary/Makefile.sources
> @@ -284,6 +284,8 @@ C_SOURCES := \
>         util/u_ringbuffer.h \
>         util/u_sampler.c \
>         util/u_sampler.h \
> +       util/u_screen.c \
> +       util/u_screen.h \
>         util/u_simple_shaders.c \
>         util/u_simple_shaders.h \
>         util/u_slab.c \
> diff --git a/src/gallium/auxiliary/util/u_screen.c b/src/gallium/auxiliary/util/u_screen.c
> new file mode 100644
> index 0000000..47bad11
> --- /dev/null
> +++ b/src/gallium/auxiliary/util/u_screen.c
> @@ -0,0 +1,114 @@
> +/*
> + * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> + * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * Functions for managing pipe_screen's
> + */
> +
> +#include <sys/stat.h>
> +
> +#include "os/os_thread.h"
> +
> +#include "pipe/p_screen.h"
> +#include "util/u_hash_table.h"
> +#include "util/u_inlines.h"
> +#include "util/u_pointer.h"
> +#include "util/u_screen.h"
> +
> +static struct util_hash_table *fd_tab = NULL;
> +pipe_static_mutex(fd_tab_mutex);
> +
> +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;
> +}
> +
> +struct pipe_screen *
> +pipe_screen_reference(int fd)
> +{
> +   struct pipe_screen *pscreen;
> +
> +   if (!fd_tab) {
> +      fd_tab = util_hash_table_create(hash_fd, compare_fd);

Do you need to grab the fd_tab_mutex around this? What if two
pipe_screen_reference() calls race to be the first ones?

> +      return NULL;
> +   }
> +
> +   pipe_mutex_lock(fd_tab_mutex);
> +   pscreen = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
> +   if (pscreen)
> +      pipe_reference(NULL, &pscreen->reference);
> +   pipe_mutex_unlock(fd_tab_mutex);
> +
> +   return pscreen;
> +}
> +
> +boolean
> +pipe_screen_unreference(struct pipe_screen *pscreen)
> +{
> +   boolean destroy;
> +
> +   if (!pscreen)
> +      return FALSE;
> +
> +   /* Work-around until all pipe_screens have ref counting */
> +   if (!pipe_is_referenced(&pscreen->reference)) {
> +      pscreen->destroy(pscreen);
> +      return TRUE;
> +   }
> +
> +   pipe_mutex_lock(fd_tab_mutex);
> +   destroy = pipe_reference(&pscreen->reference, NULL);
> +   if (destroy) {
> +      pscreen->destroy(pscreen);
> +      util_hash_table_remove(fd_tab, intptr_to_pointer(pscreen->fd));
> +      close(pscreen->fd);

It seems a little odd that you're closing a fd that you didn't
open/dup in this library. It's a bit of asymmetry in the calling
convention.

Your call if you want to flip it, but if you don't, I'd appreciate a
bit of doc somewhere about it.

> +   }
> +   pipe_mutex_unlock(fd_tab_mutex);
> +   return destroy;
> +}
> +
> +
> +void pipe_screen_reference_init(struct pipe_screen *pscreen, int fd)
> +{
> +   pscreen->fd = fd;
> +   pipe_reference_init(&pscreen->reference, 1);
> +   util_hash_table_set(fd_tab, intptr_to_pointer(pscreen->fd), pscreen);
> +}
> diff --git a/src/gallium/auxiliary/util/u_screen.h b/src/gallium/auxiliary/util/u_screen.h
> new file mode 100644
> index 0000000..fc91782
> --- /dev/null
> +++ b/src/gallium/auxiliary/util/u_screen.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> + * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + */
> +#include "pipe/p_defines.h"
> +
> +struct pipe_screen;
> +
> +struct pipe_screen *pipe_screen_reference(int fd);
> +
> +boolean pipe_screen_unreference(struct pipe_screen *pscreen);
> +
> +void pipe_screen_reference_init(struct pipe_screen *pscreen, int fd);
> diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
> index 755291a..1119bde 100644
> --- a/src/gallium/include/pipe/p_screen.h
> +++ b/src/gallium/include/pipe/p_screen.h
> @@ -41,6 +41,7 @@
>  #include "pipe/p_compiler.h"
>  #include "pipe/p_format.h"
>  #include "pipe/p_defines.h"
> +#include "pipe/p_state.h"
>  #include "pipe/p_video_enums.h"
>
>
> @@ -66,6 +67,8 @@ struct pipe_memory_info;
>   * context.
>   */
>  struct pipe_screen {
> +   struct pipe_reference reference;
> +   int fd;
>     void (*destroy)( struct pipe_screen * );
>
>     const char *(*get_name)( struct pipe_screen * );
> --
> 2.9.2
>
On Fri, Jul 22, 2016 at 11:46 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Fri, Jul 22, 2016 at 12:22 PM, Rob Herring <robh@kernel.org> wrote:
>> In order to prevent multiple pipe_screens being created in the same
>> process, lookup of the DRM FD and reference counting of the pipe_screen
>> are needed. Several implementations of this exist in various gallium
>> drivers/winsys already. This creates a common version which is opt-in
>> for winsys implementations.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  src/gallium/auxiliary/Makefile.sources |   2 +
>>  src/gallium/auxiliary/util/u_screen.c  | 114 +++++++++++++++++++++++++++++++++
>>  src/gallium/auxiliary/util/u_screen.h  |  32 +++++++++
>>  src/gallium/include/pipe/p_screen.h    |   3 +
>>  4 files changed, 151 insertions(+)
>>  create mode 100644 src/gallium/auxiliary/util/u_screen.c
>>  create mode 100644 src/gallium/auxiliary/util/u_screen.h
>>
>> diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
>> index e0311bf..197ed36 100644
>> --- a/src/gallium/auxiliary/Makefile.sources
>> +++ b/src/gallium/auxiliary/Makefile.sources
>> @@ -284,6 +284,8 @@ C_SOURCES := \
>>         util/u_ringbuffer.h \
>>         util/u_sampler.c \
>>         util/u_sampler.h \
>> +       util/u_screen.c \
>> +       util/u_screen.h \
>>         util/u_simple_shaders.c \
>>         util/u_simple_shaders.h \
>>         util/u_slab.c \
>> diff --git a/src/gallium/auxiliary/util/u_screen.c b/src/gallium/auxiliary/util/u_screen.c
>> new file mode 100644
>> index 0000000..47bad11
>> --- /dev/null
>> +++ b/src/gallium/auxiliary/util/u_screen.c
>> @@ -0,0 +1,114 @@
>> +/*
>> + * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the
>> + * "Software"), to deal in the Software without restriction, including
>> + * without limitation the rights to use, copy, modify, merge, publish,
>> + * distribute, sub license, and/or sell copies of the Software, and to
>> + * permit persons to whom the Software is furnished to do so, subject to
>> + * the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> + * next paragraph) shall be included in all copies or substantial portions
>> + * of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>> + * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +/**
>> + * Functions for managing pipe_screen's
>> + */
>> +
>> +#include <sys/stat.h>
>> +
>> +#include "os/os_thread.h"
>> +
>> +#include "pipe/p_screen.h"
>> +#include "util/u_hash_table.h"
>> +#include "util/u_inlines.h"
>> +#include "util/u_pointer.h"
>> +#include "util/u_screen.h"
>> +
>> +static struct util_hash_table *fd_tab = NULL;
>> +pipe_static_mutex(fd_tab_mutex);
>> +
>> +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;
>> +}
>> +
>> +struct pipe_screen *
>> +pipe_screen_reference(int fd)
>> +{
>> +   struct pipe_screen *pscreen;
>> +
>> +   if (!fd_tab) {
>> +      fd_tab = util_hash_table_create(hash_fd, compare_fd);
>
> Do you need to grab the fd_tab_mutex around this? What if two
> pipe_screen_reference() calls race to be the first ones?

No, but only because the loader_mutex serializes things. That's not
obvious though so putting fd_tab_mutex around it would make this
function more robust.

>> +      return NULL;
>> +   }
>> +
>> +   pipe_mutex_lock(fd_tab_mutex);
>> +   pscreen = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
>> +   if (pscreen)
>> +      pipe_reference(NULL, &pscreen->reference);
>> +   pipe_mutex_unlock(fd_tab_mutex);
>> +
>> +   return pscreen;
>> +}
>> +
>> +boolean
>> +pipe_screen_unreference(struct pipe_screen *pscreen)
>> +{
>> +   boolean destroy;
>> +
>> +   if (!pscreen)
>> +      return FALSE;
>> +
>> +   /* Work-around until all pipe_screens have ref counting */
>> +   if (!pipe_is_referenced(&pscreen->reference)) {
>> +      pscreen->destroy(pscreen);
>> +      return TRUE;
>> +   }
>> +
>> +   pipe_mutex_lock(fd_tab_mutex);
>> +   destroy = pipe_reference(&pscreen->reference, NULL);
>> +   if (destroy) {
>> +      pscreen->destroy(pscreen);
>> +      util_hash_table_remove(fd_tab, intptr_to_pointer(pscreen->fd));
>> +      close(pscreen->fd);
>
> It seems a little odd that you're closing a fd that you didn't
> open/dup in this library. It's a bit of asymmetry in the calling
> convention.

Yes, the asymmetry in general compared to the RFC bugs me a bit too.
The flip side is duplicating the close in all the destroy() functions
seems pointless.

Also, I just noticed I have a use after free problem here. The
destroy() call needs to be last.

Rob
On Fri, Jul 22, 2016 at 2:01 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 22, 2016 at 11:46 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Fri, Jul 22, 2016 at 12:22 PM, Rob Herring <robh@kernel.org> wrote:
>>> In order to prevent multiple pipe_screens being created in the same
>>> process, lookup of the DRM FD and reference counting of the pipe_screen
>>> are needed. Several implementations of this exist in various gallium
>>> drivers/winsys already. This creates a common version which is opt-in
>>> for winsys implementations.
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  src/gallium/auxiliary/Makefile.sources |   2 +
>>>  src/gallium/auxiliary/util/u_screen.c  | 114 +++++++++++++++++++++++++++++++++
>>>  src/gallium/auxiliary/util/u_screen.h  |  32 +++++++++
>>>  src/gallium/include/pipe/p_screen.h    |   3 +
>>>  4 files changed, 151 insertions(+)
>>>  create mode 100644 src/gallium/auxiliary/util/u_screen.c
>>>  create mode 100644 src/gallium/auxiliary/util/u_screen.h
>>>
>>> diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
>>> index e0311bf..197ed36 100644
>>> --- a/src/gallium/auxiliary/Makefile.sources
>>> +++ b/src/gallium/auxiliary/Makefile.sources
>>> @@ -284,6 +284,8 @@ C_SOURCES := \
>>>         util/u_ringbuffer.h \
>>>         util/u_sampler.c \
>>>         util/u_sampler.h \
>>> +       util/u_screen.c \
>>> +       util/u_screen.h \
>>>         util/u_simple_shaders.c \
>>>         util/u_simple_shaders.h \
>>>         util/u_slab.c \
>>> diff --git a/src/gallium/auxiliary/util/u_screen.c b/src/gallium/auxiliary/util/u_screen.c
>>> new file mode 100644
>>> index 0000000..47bad11
>>> --- /dev/null
>>> +++ b/src/gallium/auxiliary/util/u_screen.c
>>> @@ -0,0 +1,114 @@
>>> +/*
>>> + * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the
>>> + * "Software"), to deal in the Software without restriction, including
>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>> + * distribute, sub license, and/or sell copies of the Software, and to
>>> + * permit persons to whom the Software is furnished to do so, subject to
>>> + * the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the
>>> + * next paragraph) shall be included in all copies or substantial portions
>>> + * of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>>> + * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
>>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +/**
>>> + * Functions for managing pipe_screen's
>>> + */
>>> +
>>> +#include <sys/stat.h>
>>> +
>>> +#include "os/os_thread.h"
>>> +
>>> +#include "pipe/p_screen.h"
>>> +#include "util/u_hash_table.h"
>>> +#include "util/u_inlines.h"
>>> +#include "util/u_pointer.h"
>>> +#include "util/u_screen.h"
>>> +
>>> +static struct util_hash_table *fd_tab = NULL;
>>> +pipe_static_mutex(fd_tab_mutex);
>>> +
>>> +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;
>>> +}
>>> +
>>> +struct pipe_screen *
>>> +pipe_screen_reference(int fd)
>>> +{
>>> +   struct pipe_screen *pscreen;
>>> +
>>> +   if (!fd_tab) {
>>> +      fd_tab = util_hash_table_create(hash_fd, compare_fd);
>>
>> Do you need to grab the fd_tab_mutex around this? What if two
>> pipe_screen_reference() calls race to be the first ones?
>
> No, but only because the loader_mutex serializes things. That's not
> obvious though so putting fd_tab_mutex around it would make this
> function more robust.
>
>>> +      return NULL;
>>> +   }
>>> +
>>> +   pipe_mutex_lock(fd_tab_mutex);
>>> +   pscreen = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
>>> +   if (pscreen)
>>> +      pipe_reference(NULL, &pscreen->reference);
>>> +   pipe_mutex_unlock(fd_tab_mutex);
>>> +
>>> +   return pscreen;
>>> +}
>>> +
>>> +boolean
>>> +pipe_screen_unreference(struct pipe_screen *pscreen)
>>> +{
>>> +   boolean destroy;
>>> +
>>> +   if (!pscreen)
>>> +      return FALSE;
>>> +
>>> +   /* Work-around until all pipe_screens have ref counting */
>>> +   if (!pipe_is_referenced(&pscreen->reference)) {
>>> +      pscreen->destroy(pscreen);
>>> +      return TRUE;
>>> +   }
>>> +
>>> +   pipe_mutex_lock(fd_tab_mutex);
>>> +   destroy = pipe_reference(&pscreen->reference, NULL);
>>> +   if (destroy) {
>>> +      pscreen->destroy(pscreen);
>>> +      util_hash_table_remove(fd_tab, intptr_to_pointer(pscreen->fd));
>>> +      close(pscreen->fd);
>>
>> It seems a little odd that you're closing a fd that you didn't
>> open/dup in this library. It's a bit of asymmetry in the calling
>> convention.
>
> Yes, the asymmetry in general compared to the RFC bugs me a bit too.
> The flip side is duplicating the close in all the destroy() functions
> seems pointless.
>
> Also, I just noticed I have a use after free problem here. The
> destroy() call needs to be last.

Yeah, so it actually occurs to me that at least nouveau was most
likely leaking its dup'd fd in the current code. One issue is that the
fd has to be alive until it is removed from the fd table, otherwise
we'll have fstat issues.

Anyways, this is fine as-is. And yes, you need to flip the screen
destroy order. Although closing the fd before calling destroy could
also cause annoyance - Perhaps save off the fd before calling destroy,
and then close it?

  -ilia
On Fri, Jul 22, 2016 at 6:22 PM, Rob Herring <robh@kernel.org> wrote:
> In order to prevent multiple pipe_screens being created in the same
> process, lookup of the DRM FD and reference counting of the pipe_screen
> are needed. Several implementations of this exist in various gallium
> drivers/winsys already. This creates a common version which is opt-in
> for winsys implementations.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  src/gallium/auxiliary/Makefile.sources |   2 +
>  src/gallium/auxiliary/util/u_screen.c  | 114 +++++++++++++++++++++++++++++++++
>  src/gallium/auxiliary/util/u_screen.h  |  32 +++++++++
>  src/gallium/include/pipe/p_screen.h    |   3 +
>  4 files changed, 151 insertions(+)
>  create mode 100644 src/gallium/auxiliary/util/u_screen.c
>  create mode 100644 src/gallium/auxiliary/util/u_screen.h
>
> diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
> index e0311bf..197ed36 100644
> --- a/src/gallium/auxiliary/Makefile.sources
> +++ b/src/gallium/auxiliary/Makefile.sources
> @@ -284,6 +284,8 @@ C_SOURCES := \
>         util/u_ringbuffer.h \
>         util/u_sampler.c \
>         util/u_sampler.h \
> +       util/u_screen.c \
> +       util/u_screen.h \
>         util/u_simple_shaders.c \
>         util/u_simple_shaders.h \
>         util/u_slab.c \
> diff --git a/src/gallium/auxiliary/util/u_screen.c b/src/gallium/auxiliary/util/u_screen.c
> new file mode 100644
> index 0000000..47bad11
> --- /dev/null
> +++ b/src/gallium/auxiliary/util/u_screen.c
> @@ -0,0 +1,114 @@
> +/*
> + * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> + * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * Functions for managing pipe_screen's
> + */
> +
> +#include <sys/stat.h>
> +
> +#include "os/os_thread.h"
> +
> +#include "pipe/p_screen.h"
> +#include "util/u_hash_table.h"
> +#include "util/u_inlines.h"
> +#include "util/u_pointer.h"
> +#include "util/u_screen.h"
> +
> +static struct util_hash_table *fd_tab = NULL;
> +pipe_static_mutex(fd_tab_mutex);
> +
> +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;
> +}

I think fstat won't work for amdgpu. We moved away from it some time
ago, though it's not documented why. libdrm_amdgpu uses
drmGetPrimaryDeviceNameFromFd instead, so I think Mesa should too.

Marek
On 28 July 2016 at 13:45, Marek Olšák <maraeo@gmail.com> wrote:
> On Fri, Jul 22, 2016 at 6:22 PM, Rob Herring <robh@kernel.org> wrote:
>> In order to prevent multiple pipe_screens being created in the same
>> process, lookup of the DRM FD and reference counting of the pipe_screen
>> are needed. Several implementations of this exist in various gallium
>> drivers/winsys already. This creates a common version which is opt-in
>> for winsys implementations.
>>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>> ---
>>  src/gallium/auxiliary/Makefile.sources |   2 +
>>  src/gallium/auxiliary/util/u_screen.c  | 114 +++++++++++++++++++++++++++++++++
>>  src/gallium/auxiliary/util/u_screen.h  |  32 +++++++++
>>  src/gallium/include/pipe/p_screen.h    |   3 +
>>  4 files changed, 151 insertions(+)
>>  create mode 100644 src/gallium/auxiliary/util/u_screen.c
>>  create mode 100644 src/gallium/auxiliary/util/u_screen.h
>>
>> diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
>> index e0311bf..197ed36 100644
>> --- a/src/gallium/auxiliary/Makefile.sources
>> +++ b/src/gallium/auxiliary/Makefile.sources
>> @@ -284,6 +284,8 @@ C_SOURCES := \
>>         util/u_ringbuffer.h \
>>         util/u_sampler.c \
>>         util/u_sampler.h \
>> +       util/u_screen.c \
>> +       util/u_screen.h \
>>         util/u_simple_shaders.c \
>>         util/u_simple_shaders.h \
>>         util/u_slab.c \
>> diff --git a/src/gallium/auxiliary/util/u_screen.c b/src/gallium/auxiliary/util/u_screen.c
>> new file mode 100644
>> index 0000000..47bad11
>> --- /dev/null
>> +++ b/src/gallium/auxiliary/util/u_screen.c
>> @@ -0,0 +1,114 @@
>> +/*
>> + * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the
>> + * "Software"), to deal in the Software without restriction, including
>> + * without limitation the rights to use, copy, modify, merge, publish,
>> + * distribute, sub license, and/or sell copies of the Software, and to
>> + * permit persons to whom the Software is furnished to do so, subject to
>> + * the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> + * next paragraph) shall be included in all copies or substantial portions
>> + * of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>> + * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +/**
>> + * Functions for managing pipe_screen's
>> + */
>> +
>> +#include <sys/stat.h>
>> +
>> +#include "os/os_thread.h"
>> +
>> +#include "pipe/p_screen.h"
>> +#include "util/u_hash_table.h"
>> +#include "util/u_inlines.h"
>> +#include "util/u_pointer.h"
>> +#include "util/u_screen.h"
>> +
>> +static struct util_hash_table *fd_tab = NULL;
>> +pipe_static_mutex(fd_tab_mutex);
>> +
>> +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;
>> +}
>
> I think fstat won't work for amdgpu. We moved away from it some time
> ago, though it's not documented why. libdrm_amdgpu uses
> drmGetPrimaryDeviceNameFromFd instead, so I think Mesa should too.
>
libdrm_amdgpu uses a different scheme than others where it allows
sharing between card and renderD.
It stores a reference to both nodes (if applicable) does not nessesary
acrobatics, if needed, in order to present a single/unique
amdgpu_device_handle per actual HW device.

Whether that's possible/good idea for others is to be confirmed, so
let's keep that as follow-up work ?
Emil
On 22 July 2016 at 19:01, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 22, 2016 at 11:46 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Fri, Jul 22, 2016 at 12:22 PM, Rob Herring <robh@kernel.org> wrote:
>>> In order to prevent multiple pipe_screens being created in the same
>>> process, lookup of the DRM FD and reference counting of the pipe_screen
>>> are needed. Several implementations of this exist in various gallium
>>> drivers/winsys already. This creates a common version which is opt-in
>>> for winsys implementations.
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  src/gallium/auxiliary/Makefile.sources |   2 +
>>>  src/gallium/auxiliary/util/u_screen.c  | 114 +++++++++++++++++++++++++++++++++
>>>  src/gallium/auxiliary/util/u_screen.h  |  32 +++++++++
>>>  src/gallium/include/pipe/p_screen.h    |   3 +
>>>  4 files changed, 151 insertions(+)
>>>  create mode 100644 src/gallium/auxiliary/util/u_screen.c
>>>  create mode 100644 src/gallium/auxiliary/util/u_screen.h
>>>
>>> diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
>>> index e0311bf..197ed36 100644
>>> --- a/src/gallium/auxiliary/Makefile.sources
>>> +++ b/src/gallium/auxiliary/Makefile.sources
>>> @@ -284,6 +284,8 @@ C_SOURCES := \
>>>         util/u_ringbuffer.h \
>>>         util/u_sampler.c \
>>>         util/u_sampler.h \
>>> +       util/u_screen.c \
>>> +       util/u_screen.h \
>>>         util/u_simple_shaders.c \
>>>         util/u_simple_shaders.h \
>>>         util/u_slab.c \
>>> diff --git a/src/gallium/auxiliary/util/u_screen.c b/src/gallium/auxiliary/util/u_screen.c
>>> new file mode 100644
>>> index 0000000..47bad11
>>> --- /dev/null
>>> +++ b/src/gallium/auxiliary/util/u_screen.c
>>> @@ -0,0 +1,114 @@
>>> +/*
>>> + * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the
>>> + * "Software"), to deal in the Software without restriction, including
>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>> + * distribute, sub license, and/or sell copies of the Software, and to
>>> + * permit persons to whom the Software is furnished to do so, subject to
>>> + * the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the
>>> + * next paragraph) shall be included in all copies or substantial portions
>>> + * of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>>> + * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
>>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +/**
>>> + * Functions for managing pipe_screen's
>>> + */
>>> +
>>> +#include <sys/stat.h>
>>> +
>>> +#include "os/os_thread.h"
>>> +
>>> +#include "pipe/p_screen.h"
>>> +#include "util/u_hash_table.h"
>>> +#include "util/u_inlines.h"
>>> +#include "util/u_pointer.h"
>>> +#include "util/u_screen.h"
>>> +
>>> +static struct util_hash_table *fd_tab = NULL;
>>> +pipe_static_mutex(fd_tab_mutex);
>>> +
>>> +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;
>>> +}
>>> +
>>> +struct pipe_screen *
>>> +pipe_screen_reference(int fd)
>>> +{
>>> +   struct pipe_screen *pscreen;
>>> +
>>> +   if (!fd_tab) {
>>> +      fd_tab = util_hash_table_create(hash_fd, compare_fd);
>>
>> Do you need to grab the fd_tab_mutex around this? What if two
>> pipe_screen_reference() calls race to be the first ones?
>
> No, but only because the loader_mutex serializes things. That's not
> obvious though so putting fd_tab_mutex around it would make this
> function more robust.
>
>>> +      return NULL;
>>> +   }
>>> +
>>> +   pipe_mutex_lock(fd_tab_mutex);
>>> +   pscreen = util_hash_table_get(fd_tab, intptr_to_pointer(fd));
>>> +   if (pscreen)
>>> +      pipe_reference(NULL, &pscreen->reference);
>>> +   pipe_mutex_unlock(fd_tab_mutex);
>>> +
>>> +   return pscreen;
>>> +}
>>> +
>>> +boolean
>>> +pipe_screen_unreference(struct pipe_screen *pscreen)
>>> +{
>>> +   boolean destroy;
>>> +
>>> +   if (!pscreen)
>>> +      return FALSE;
>>> +
>>> +   /* Work-around until all pipe_screens have ref counting */
>>> +   if (!pipe_is_referenced(&pscreen->reference)) {
>>> +      pscreen->destroy(pscreen);
>>> +      return TRUE;
>>> +   }
>>> +
>>> +   pipe_mutex_lock(fd_tab_mutex);
>>> +   destroy = pipe_reference(&pscreen->reference, NULL);
>>> +   if (destroy) {
>>> +      pscreen->destroy(pscreen);
>>> +      util_hash_table_remove(fd_tab, intptr_to_pointer(pscreen->fd));
>>> +      close(pscreen->fd);
>>
>> It seems a little odd that you're closing a fd that you didn't
>> open/dup in this library. It's a bit of asymmetry in the calling
>> convention.
>
> Yes, the asymmetry in general compared to the RFC bugs me a bit too.
> The flip side is duplicating the close in all the destroy() functions
> seems pointless.
>
> Also, I just noticed I have a use after free problem here. The
> destroy() call needs to be last.
>
Haven't checked if all the drivers will need the close() and/or how
likely are things to say boom, so let's keep that as separate patch ?

Thanks
Emil
On Mon, Jan 9, 2017 at 7:38 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 28 July 2016 at 13:45, Marek Olšák <maraeo@gmail.com> wrote:
>> On Fri, Jul 22, 2016 at 6:22 PM, Rob Herring <robh@kernel.org> wrote:
>>> In order to prevent multiple pipe_screens being created in the same
>>> process, lookup of the DRM FD and reference counting of the pipe_screen
>>> are needed. Several implementations of this exist in various gallium
>>> drivers/winsys already. This creates a common version which is opt-in
>>> for winsys implementations.
>>>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  src/gallium/auxiliary/Makefile.sources |   2 +
>>>  src/gallium/auxiliary/util/u_screen.c  | 114 +++++++++++++++++++++++++++++++++
>>>  src/gallium/auxiliary/util/u_screen.h  |  32 +++++++++
>>>  src/gallium/include/pipe/p_screen.h    |   3 +
>>>  4 files changed, 151 insertions(+)
>>>  create mode 100644 src/gallium/auxiliary/util/u_screen.c
>>>  create mode 100644 src/gallium/auxiliary/util/u_screen.h
>>>
>>> diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
>>> index e0311bf..197ed36 100644
>>> --- a/src/gallium/auxiliary/Makefile.sources
>>> +++ b/src/gallium/auxiliary/Makefile.sources
>>> @@ -284,6 +284,8 @@ C_SOURCES := \
>>>         util/u_ringbuffer.h \
>>>         util/u_sampler.c \
>>>         util/u_sampler.h \
>>> +       util/u_screen.c \
>>> +       util/u_screen.h \
>>>         util/u_simple_shaders.c \
>>>         util/u_simple_shaders.h \
>>>         util/u_slab.c \
>>> diff --git a/src/gallium/auxiliary/util/u_screen.c b/src/gallium/auxiliary/util/u_screen.c
>>> new file mode 100644
>>> index 0000000..47bad11
>>> --- /dev/null
>>> +++ b/src/gallium/auxiliary/util/u_screen.c
>>> @@ -0,0 +1,114 @@
>>> +/*
>>> + * Copyright 2016 Linaro, Ltd., Rob Herring <robh@kernel.org>
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the
>>> + * "Software"), to deal in the Software without restriction, including
>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>> + * distribute, sub license, and/or sell copies of the Software, and to
>>> + * permit persons to whom the Software is furnished to do so, subject to
>>> + * the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the
>>> + * next paragraph) shall be included in all copies or substantial portions
>>> + * of the Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>> + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>>> + * IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR
>>> + * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>>> + * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>>> + * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>> + */
>>> +
>>> +/**
>>> + * Functions for managing pipe_screen's
>>> + */
>>> +
>>> +#include <sys/stat.h>
>>> +
>>> +#include "os/os_thread.h"
>>> +
>>> +#include "pipe/p_screen.h"
>>> +#include "util/u_hash_table.h"
>>> +#include "util/u_inlines.h"
>>> +#include "util/u_pointer.h"
>>> +#include "util/u_screen.h"
>>> +
>>> +static struct util_hash_table *fd_tab = NULL;
>>> +pipe_static_mutex(fd_tab_mutex);
>>> +
>>> +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;
>>> +}
>>
>> I think fstat won't work for amdgpu. We moved away from it some time
>> ago, though it's not documented why. libdrm_amdgpu uses
>> drmGetPrimaryDeviceNameFromFd instead, so I think Mesa should too.
>>
> libdrm_amdgpu uses a different scheme than others where it allows
> sharing between card and renderD.
> It stores a reference to both nodes (if applicable) does not nessesary
> acrobatics, if needed, in order to present a single/unique
> amdgpu_device_handle per actual HW device.
>
> Whether that's possible/good idea for others is to be confirmed, so
> let's keep that as follow-up work ?

I wouldn't like to change the behavior for amdgpu. A possible
workaround is to let drivers supply the hash table key. Then, amdgpu
can use the device handle and other drivers can use fd.

Marek