[Mesa-dev,v4,07/11] amdgpu: use common screen ref counting

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

Details

Message ID 20160722162222.21871-8-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.
Use the common pipe_screen ref count. amdgpu is unique in its hashing
the dev pointer rather than the fd, so the common fd hashing cannot be
used. However, the same reference count can be used instead of the
private one. The mutex can be dropped as the pipe loader protects the
create_screen() calls.

Signed-off-by: Rob Herring <robh@kernel.org>
Cc: "Marek Olšák" <marek.olsak@amd.com>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
---
 src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c | 45 ++++-----------------------
 src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h |  1 -
 2 files changed, 6 insertions(+), 40 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
index 9a04cbe..27293ac 100644
--- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
+++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
@@ -60,8 +60,6 @@ 
 #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];
@@ -329,6 +327,7 @@  static void amdgpu_winsys_destroy(struct radeon_winsys *rws)
    pipe_mutex_destroy(ws->global_bo_list_lock);
    AddrDestroy(ws->addrlib);
    amdgpu_device_deinitialize(ws->dev);
+   util_hash_table_remove(dev_tab, ws->dev);
    FREE(rws);
 }
 
@@ -410,26 +409,6 @@  static int compare_dev(void *key1, void *key2)
 
 DEBUG_GET_ONCE_BOOL_OPTION(thread, "RADEON_THREAD", true)
 
-static bool amdgpu_winsys_unref(struct radeon_winsys *rws)
-{
-   struct amdgpu_winsys *ws = (struct amdgpu_winsys*)rws;
-   bool destroy;
-
-   /* When the reference counter drops to zero, remove the device pointer
-    * from the table.
-    * This must happen while the mutex is locked, so that
-    * amdgpu_winsys_create in another thread doesn't get the winsys
-    * from the table when the counter drops to 0. */
-   pipe_mutex_lock(dev_tab_mutex);
-
-   destroy = pipe_reference(&ws->reference, NULL);
-   if (destroy && dev_tab)
-      util_hash_table_remove(dev_tab, ws->dev);
-
-   pipe_mutex_unlock(dev_tab_mutex);
-   return destroy;
-}
-
 PUBLIC struct radeon_winsys *
 amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
 {
@@ -446,7 +425,6 @@  amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
    drmFreeVersion(version);
 
    /* Look up the winsys from the dev table. */
-   pipe_mutex_lock(dev_tab_mutex);
    if (!dev_tab)
       dev_tab = util_hash_table_create(hash_dev, compare_dev);
 
@@ -454,7 +432,6 @@  amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
     * for the same fd. */
    r = amdgpu_device_initialize(fd, &drm_major, &drm_minor, &dev);
    if (r) {
-      pipe_mutex_unlock(dev_tab_mutex);
       fprintf(stderr, "amdgpu: amdgpu_device_initialize failed.\n");
       return NULL;
    }
@@ -462,17 +439,14 @@  amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
    /* Lookup a winsys if we have already created one for this device. */
    ws = util_hash_table_get(dev_tab, dev);
    if (ws) {
-      pipe_reference(NULL, &ws->reference);
-      pipe_mutex_unlock(dev_tab_mutex);
+      pipe_reference(NULL, &ws->base.screen->reference);
       return &ws->base;
    }
 
    /* Create a new winsys. */
    ws = CALLOC_STRUCT(amdgpu_winsys);
-   if (!ws) {
-      pipe_mutex_unlock(dev_tab_mutex);
+   if (!ws)
       return NULL;
-   }
 
    ws->dev = dev;
    ws->info.drm_major = drm_major;
@@ -486,11 +460,7 @@  amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
                  (ws->info.vram_size + ws->info.gart_size) / 8,
                  amdgpu_bo_destroy, amdgpu_bo_can_reclaim);
 
-   /* init reference */
-   pipe_reference_init(&ws->reference, 1);
-
    /* Set functions. */
-   ws->base.unref = amdgpu_winsys_unref;
    ws->base.destroy = amdgpu_winsys_destroy;
    ws->base.query_info = amdgpu_winsys_query_info;
    ws->base.cs_request_feature = amdgpu_cs_request_feature;
@@ -516,21 +486,18 @@  amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
    ws->base.screen = screen_create(&ws->base);
    if (!ws->base.screen) {
       amdgpu_winsys_destroy(&ws->base);
-      pipe_mutex_unlock(dev_tab_mutex);
       return NULL;
    }
 
    util_hash_table_set(dev_tab, dev, ws);
 
-   /* We must unlock the mutex once the winsys is fully initialized, so that
-    * other threads attempting to create the winsys from the same fd will
-    * get a fully initialized winsys and not just half-way initialized. */
-   pipe_mutex_unlock(dev_tab_mutex);
+   /* init reference */
+   pipe_reference_init(&ws->base.screen->reference, 1);
+   ws->base.screen->fd = -1;
 
    return &ws->base;
 
 fail:
-   pipe_mutex_unlock(dev_tab_mutex);
    pb_cache_deinit(&ws->bo_cache);
    FREE(ws);
    return NULL;
diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
index 8489530..b45c2d7 100644
--- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
+++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
@@ -42,7 +42,6 @@  struct amdgpu_cs;
 
 struct amdgpu_winsys {
    struct radeon_winsys base;
-   struct pipe_reference reference;
    struct pb_cache bo_cache;
 
    amdgpu_device_handle dev;

Comments

On 22 July 2016 at 17:22, Rob Herring <robh@kernel.org> wrote:

>     /* Set functions. */
> -   ws->base.unref = amdgpu_winsys_unref;
With this in place we're NULL refer in the driver(s). Although you've
removed the callers with next patch, IMHO it's worth adding a NULL
check here ?

-Emil
The fd table and reference counting in the winsys is required by the
GL-VDPAU interop.

radeon_drm_winsys_create and amdgpu_winsys_create are publicly
exported by both *_dri.so and libvdpau_*.so, and whichever is loaded
first will effectively provide the gallium driver implementation for
both of them. The second loaded lib can't create its own winsys &
screen & contexts because of the public *_winsys_create functions
always invoking the first loaded lib.

Given that, I'm rejecting patches 7 & 8, because they obviously break
the GL-VDPAU interop by ignoring this ld-based inter-lib interaction
that's very important to us.

It looks like Nouveau relies on the same interaction too.

Marek


On Fri, Jul 22, 2016 at 6:22 PM, Rob Herring <robh@kernel.org> wrote:
> Use the common pipe_screen ref count. amdgpu is unique in its hashing
> the dev pointer rather than the fd, so the common fd hashing cannot be
> used. However, the same reference count can be used instead of the
> private one. The mutex can be dropped as the pipe loader protects the
> create_screen() calls.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Cc: "Marek Olšák" <marek.olsak@amd.com>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> ---
>  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c | 45 ++++-----------------------
>  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h |  1 -
>  2 files changed, 6 insertions(+), 40 deletions(-)
>
> diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> index 9a04cbe..27293ac 100644
> --- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c
> @@ -60,8 +60,6 @@
>  #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];
> @@ -329,6 +327,7 @@ static void amdgpu_winsys_destroy(struct radeon_winsys *rws)
>     pipe_mutex_destroy(ws->global_bo_list_lock);
>     AddrDestroy(ws->addrlib);
>     amdgpu_device_deinitialize(ws->dev);
> +   util_hash_table_remove(dev_tab, ws->dev);
>     FREE(rws);
>  }
>
> @@ -410,26 +409,6 @@ static int compare_dev(void *key1, void *key2)
>
>  DEBUG_GET_ONCE_BOOL_OPTION(thread, "RADEON_THREAD", true)
>
> -static bool amdgpu_winsys_unref(struct radeon_winsys *rws)
> -{
> -   struct amdgpu_winsys *ws = (struct amdgpu_winsys*)rws;
> -   bool destroy;
> -
> -   /* When the reference counter drops to zero, remove the device pointer
> -    * from the table.
> -    * This must happen while the mutex is locked, so that
> -    * amdgpu_winsys_create in another thread doesn't get the winsys
> -    * from the table when the counter drops to 0. */
> -   pipe_mutex_lock(dev_tab_mutex);
> -
> -   destroy = pipe_reference(&ws->reference, NULL);
> -   if (destroy && dev_tab)
> -      util_hash_table_remove(dev_tab, ws->dev);
> -
> -   pipe_mutex_unlock(dev_tab_mutex);
> -   return destroy;
> -}
> -
>  PUBLIC struct radeon_winsys *
>  amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
>  {
> @@ -446,7 +425,6 @@ amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
>     drmFreeVersion(version);
>
>     /* Look up the winsys from the dev table. */
> -   pipe_mutex_lock(dev_tab_mutex);
>     if (!dev_tab)
>        dev_tab = util_hash_table_create(hash_dev, compare_dev);
>
> @@ -454,7 +432,6 @@ amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
>      * for the same fd. */
>     r = amdgpu_device_initialize(fd, &drm_major, &drm_minor, &dev);
>     if (r) {
> -      pipe_mutex_unlock(dev_tab_mutex);
>        fprintf(stderr, "amdgpu: amdgpu_device_initialize failed.\n");
>        return NULL;
>     }
> @@ -462,17 +439,14 @@ amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
>     /* Lookup a winsys if we have already created one for this device. */
>     ws = util_hash_table_get(dev_tab, dev);
>     if (ws) {
> -      pipe_reference(NULL, &ws->reference);
> -      pipe_mutex_unlock(dev_tab_mutex);
> +      pipe_reference(NULL, &ws->base.screen->reference);
>        return &ws->base;
>     }
>
>     /* Create a new winsys. */
>     ws = CALLOC_STRUCT(amdgpu_winsys);
> -   if (!ws) {
> -      pipe_mutex_unlock(dev_tab_mutex);
> +   if (!ws)
>        return NULL;
> -   }
>
>     ws->dev = dev;
>     ws->info.drm_major = drm_major;
> @@ -486,11 +460,7 @@ amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
>                   (ws->info.vram_size + ws->info.gart_size) / 8,
>                   amdgpu_bo_destroy, amdgpu_bo_can_reclaim);
>
> -   /* init reference */
> -   pipe_reference_init(&ws->reference, 1);
> -
>     /* Set functions. */
> -   ws->base.unref = amdgpu_winsys_unref;
>     ws->base.destroy = amdgpu_winsys_destroy;
>     ws->base.query_info = amdgpu_winsys_query_info;
>     ws->base.cs_request_feature = amdgpu_cs_request_feature;
> @@ -516,21 +486,18 @@ amdgpu_winsys_create(int fd, radeon_screen_create_t screen_create)
>     ws->base.screen = screen_create(&ws->base);
>     if (!ws->base.screen) {
>        amdgpu_winsys_destroy(&ws->base);
> -      pipe_mutex_unlock(dev_tab_mutex);
>        return NULL;
>     }
>
>     util_hash_table_set(dev_tab, dev, ws);
>
> -   /* We must unlock the mutex once the winsys is fully initialized, so that
> -    * other threads attempting to create the winsys from the same fd will
> -    * get a fully initialized winsys and not just half-way initialized. */
> -   pipe_mutex_unlock(dev_tab_mutex);
> +   /* init reference */
> +   pipe_reference_init(&ws->base.screen->reference, 1);
> +   ws->base.screen->fd = -1;
>
>     return &ws->base;
>
>  fail:
> -   pipe_mutex_unlock(dev_tab_mutex);
>     pb_cache_deinit(&ws->bo_cache);
>     FREE(ws);
>     return NULL;
> diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
> index 8489530..b45c2d7 100644
> --- a/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
> +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h
> @@ -42,7 +42,6 @@ struct amdgpu_cs;
>
>  struct amdgpu_winsys {
>     struct radeon_winsys base;
> -   struct pipe_reference reference;
>     struct pb_cache bo_cache;
>
>     amdgpu_device_handle dev;
> --
> 2.9.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Fri, Jul 29, 2016 at 12:51 PM, Marek Olšák <maraeo@gmail.com> wrote:
> The fd table and reference counting in the winsys is required by the
> GL-VDPAU interop.
>
> radeon_drm_winsys_create and amdgpu_winsys_create are publicly
> exported by both *_dri.so and libvdpau_*.so, and whichever is loaded
> first will effectively provide the gallium driver implementation for
> both of them. The second loaded lib can't create its own winsys &
> screen & contexts because of the public *_winsys_create functions
> always invoking the first loaded lib.

Yes, I'm aware of this as it was discussed in the RFC version, and it
was my intent to maintain that. I believe this version should support
this case as the ref counting is done within
radeon_drm_winsys_create/amdgpu_winsys_create. Where specifically do
you think it is broken?

Rob
On Fri, Jul 29, 2016 at 8:01 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 29, 2016 at 12:51 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> The fd table and reference counting in the winsys is required by the
>> GL-VDPAU interop.
>>
>> radeon_drm_winsys_create and amdgpu_winsys_create are publicly
>> exported by both *_dri.so and libvdpau_*.so, and whichever is loaded
>> first will effectively provide the gallium driver implementation for
>> both of them. The second loaded lib can't create its own winsys &
>> screen & contexts because of the public *_winsys_create functions
>> always invoking the first loaded lib.
>
> Yes, I'm aware of this as it was discussed in the RFC version, and it
> was my intent to maintain that. I believe this version should support
> this case as the ref counting is done within
> radeon_drm_winsys_create/amdgpu_winsys_create. Where specifically do
> you think it is broken?

Hm, maybe not.

Where is fd == -1 handled in the common code? Will this not match all
devices to one hash table entry and one screen?

Marek
On 29 July 2016 at 20:08, Marek Olšák <maraeo@gmail.com> wrote:
> On Fri, Jul 29, 2016 at 8:01 PM, Rob Herring <robh@kernel.org> wrote:
>> On Fri, Jul 29, 2016 at 12:51 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>> The fd table and reference counting in the winsys is required by the
>>> GL-VDPAU interop.
>>>
>>> radeon_drm_winsys_create and amdgpu_winsys_create are publicly
>>> exported by both *_dri.so and libvdpau_*.so, and whichever is loaded
>>> first will effectively provide the gallium driver implementation for
>>> both of them. The second loaded lib can't create its own winsys &
>>> screen & contexts because of the public *_winsys_create functions
>>> always invoking the first loaded lib.
>>
>> Yes, I'm aware of this as it was discussed in the RFC version, and it
>> was my intent to maintain that. I believe this version should support
>> this case as the ref counting is done within
>> radeon_drm_winsys_create/amdgpu_winsys_create. Where specifically do
>> you think it is broken?
>
> Hm, maybe not.
>
> Where is fd == -1 handled in the common code? Will this not match all
> devices to one hash table entry and one screen?
>
Afaict Rob H has tried to be nice here, using the common code for
amdgpu yet it's not a perfect fit.
Fwiw, I'd just opt out of converting/touching amdgpu for now.

Alternatively one could lift the amdgpu implementation to u_screen*
having a second hash table/associated compare_foo. Thus during
pipe_screen_reference() and/or pipe_screen_reference_init() one can
say "use compare opaque", which is tracked in pipe_screen/elsewhere
and the corresponding table is managed.

Then again I'd imagine one can think of other route to archiving the
ultimate goal.
Emil