[Mesa-dev,v2] vc4: add hash table look-up for exported dmabufs

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

Details

Message ID 20160722202830.21318-1-robh@kernel.org
State New
Headers show
Series "vc4: add hash table look-up for exported dmabufs" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Rob Herring July 22, 2016, 8:28 p.m.
It is necessary to reuse existing BOs when dmabufs are imported. There
are 2 cases that need to be handled. dmabufs can be created/exported and
imported by the same process and can be imported multiple times.
Copying other drivers, add a hash table to track exported BOs so the
BOs get reused.

Cc: Eric Anholt <eric@anholt.net>
Signed-off-by: Rob Herring <robh@kernel.org>
---
v2:
- Avoid taking mutex on unreference if private
- Fix use after free with util_hash_table_remove

 src/gallium/drivers/vc4/vc4_bufmgr.c | 20 +++++++++++++++++++-
 src/gallium/drivers/vc4/vc4_bufmgr.h | 19 ++++++++++++++++++-
 src/gallium/drivers/vc4/vc4_screen.c | 15 +++++++++++++++
 src/gallium/drivers/vc4/vc4_screen.h |  3 +++
 4 files changed, 55 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/vc4/vc4_bufmgr.c b/src/gallium/drivers/vc4/vc4_bufmgr.c
index 21e3bde..f6bacfd 100644
--- a/src/gallium/drivers/vc4/vc4_bufmgr.c
+++ b/src/gallium/drivers/vc4/vc4_bufmgr.c
@@ -28,6 +28,7 @@ 
 #include <xf86drm.h>
 #include <xf86drmMode.h>
 
+#include "util/u_hash_table.h"
 #include "util/u_memory.h"
 #include "util/ralloc.h"
 
@@ -329,10 +330,19 @@  vc4_bo_open_handle(struct vc4_screen *screen,
                    uint32_t winsys_stride,
                    uint32_t handle, uint32_t size)
 {
-        struct vc4_bo *bo = CALLOC_STRUCT(vc4_bo);
+        struct vc4_bo *bo;
 
         assert(size);
 
+        pipe_mutex_lock(screen->bo_handles_mutex);
+
+        bo = util_hash_table_get(screen->bo_handles, (void*)(uintptr_t)handle);
+        if (bo) {
+                pipe_reference(NULL, &bo->reference);
+                goto done;
+        }
+
+        bo = CALLOC_STRUCT(vc4_bo);
         pipe_reference_init(&bo->reference, 1);
         bo->screen = screen;
         bo->handle = handle;
@@ -347,6 +357,10 @@  vc4_bo_open_handle(struct vc4_screen *screen,
         bo->map = malloc(bo->size);
 #endif
 
+        util_hash_table_set(screen->bo_handles, (void *)(uintptr_t)handle, bo);
+
+done:
+        pipe_mutex_unlock(screen->bo_handles_mutex);
         return bo;
 }
 
@@ -399,7 +413,11 @@  vc4_bo_get_dmabuf(struct vc4_bo *bo)
                         bo->handle);
                 return -1;
         }
+
+        pipe_mutex_lock(bo->screen->bo_handles_mutex);
         bo->private = false;
+        util_hash_table_set(bo->screen->bo_handles, (void *)(uintptr_t)bo->handle, bo);
+        pipe_mutex_unlock(bo->screen->bo_handles_mutex);
 
         return fd;
 }
diff --git a/src/gallium/drivers/vc4/vc4_bufmgr.h b/src/gallium/drivers/vc4/vc4_bufmgr.h
index b77506e..c84b850 100644
--- a/src/gallium/drivers/vc4/vc4_bufmgr.h
+++ b/src/gallium/drivers/vc4/vc4_bufmgr.h
@@ -25,6 +25,7 @@ 
 #define VC4_BUFMGR_H
 
 #include <stdint.h>
+#include "util/u_hash_table.h"
 #include "util/u_inlines.h"
 #include "vc4_qir.h"
 
@@ -87,11 +88,27 @@  vc4_bo_reference(struct vc4_bo *bo)
 static inline void
 vc4_bo_unreference(struct vc4_bo **bo)
 {
+        struct vc4_screen *screen;
         if (!*bo)
                 return;
 
-        if (pipe_reference(&(*bo)->reference, NULL))
+        if ((*bo)->private) {
+            /* Avoid the mutex for private BOs */
+            if (pipe_reference(&(*bo)->reference, NULL))
                 vc4_bo_last_unreference(*bo);
+        } else {
+            screen = (*bo)->screen;
+            pipe_mutex_lock(screen->bo_handles_mutex);
+
+            if (pipe_reference(&(*bo)->reference, NULL)) {
+                    util_hash_table_remove(screen->bo_handles,
+                                           (void *)(uintptr_t)(*bo)->handle);
+                    vc4_bo_last_unreference(*bo);
+            }
+
+            pipe_mutex_unlock(screen->bo_handles_mutex);
+        }
+
         *bo = NULL;
 }
 
diff --git a/src/gallium/drivers/vc4/vc4_screen.c b/src/gallium/drivers/vc4/vc4_screen.c
index 29c0f94..82544e0 100644
--- a/src/gallium/drivers/vc4/vc4_screen.c
+++ b/src/gallium/drivers/vc4/vc4_screen.c
@@ -30,6 +30,7 @@ 
 #include "util/u_debug.h"
 #include "util/u_memory.h"
 #include "util/u_format.h"
+#include "util/u_hash_table.h"
 #include "util/ralloc.h"
 
 #include "vc4_screen.h"
@@ -496,6 +497,18 @@  vc4_screen_is_format_supported(struct pipe_screen *pscreen,
         return retval == usage;
 }
 
+#define PTR_TO_UINT(x) ((unsigned)((intptr_t)(x)))
+
+static unsigned handle_hash(void *key)
+{
+    return PTR_TO_UINT(key);
+}
+
+static int handle_compare(void *key1, void *key2)
+{
+    return PTR_TO_UINT(key1) != PTR_TO_UINT(key2);
+}
+
 static bool
 vc4_supports_branches(struct vc4_screen *screen)
 {
@@ -523,6 +536,8 @@  vc4_screen_create(int fd)
 
         screen->fd = fd;
         list_inithead(&screen->bo_cache.time_list);
+        pipe_mutex_init(screen->bo_handles_mutex);
+        screen->bo_handles = util_hash_table_create(handle_hash, handle_compare);
 
         if (vc4_supports_branches(screen))
                 screen->has_control_flow = true;
diff --git a/src/gallium/drivers/vc4/vc4_screen.h b/src/gallium/drivers/vc4/vc4_screen.h
index 6cecca6..16b3a6c 100644
--- a/src/gallium/drivers/vc4/vc4_screen.h
+++ b/src/gallium/drivers/vc4/vc4_screen.h
@@ -73,6 +73,9 @@  struct vc4_screen {
                 uint32_t bo_count;
         } bo_cache;
 
+        struct util_hash_table *bo_handles;
+        pipe_mutex bo_handles_mutex;
+
         uint32_t bo_size;
         uint32_t bo_count;
         bool has_control_flow;

Comments

Rob Herring <robh@kernel.org> writes:

> It is necessary to reuse existing BOs when dmabufs are imported. There
> are 2 cases that need to be handled. dmabufs can be created/exported and
> imported by the same process and can be imported multiple times.
> Copying other drivers, add a hash table to track exported BOs so the
> BOs get reused.
>
> Cc: Eric Anholt <eric@anholt.net>
> Signed-off-by: Rob Herring <robh@kernel.org>

Looks good to me, other than a bit of funny whitespace that I'll fix up.
I built a piglit test for this today (want to go take a look at those?),
and once I get a piglit run through, I'll push the change.
On Mon, Jul 25, 2016 at 8:47 PM, Eric Anholt <eric@anholt.net> wrote:
> Rob Herring <robh@kernel.org> writes:
>
>> It is necessary to reuse existing BOs when dmabufs are imported. There
>> are 2 cases that need to be handled. dmabufs can be created/exported and
>> imported by the same process and can be imported multiple times.
>> Copying other drivers, add a hash table to track exported BOs so the
>> BOs get reused.
>>
>> Cc: Eric Anholt <eric@anholt.net>
>> Signed-off-by: Rob Herring <robh@kernel.org>
>
> Looks good to me, other than a bit of funny whitespace that I'll fix up.
> I built a piglit test for this today (want to go take a look at those?),
> and once I get a piglit run through, I'll push the change.

I don't suppose you have a branch somewhere?  I should probably give
that a try..  I've had
https://trello.com/c/x34U0kTQ/114-teach-piglit-about-libdrm-freedreno-for-dmabuf-tests
on my todo list for a while (but a generic gbm based solution seems
even better than teaching piglit about libdrm_$drivername ;-))

BR,
-R


> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
Rob Clark <robdclark@gmail.com> writes:

> On Mon, Jul 25, 2016 at 8:47 PM, Eric Anholt <eric@anholt.net> wrote:
>> Rob Herring <robh@kernel.org> writes:
>>
>>> It is necessary to reuse existing BOs when dmabufs are imported. There
>>> are 2 cases that need to be handled. dmabufs can be created/exported and
>>> imported by the same process and can be imported multiple times.
>>> Copying other drivers, add a hash table to track exported BOs so the
>>> BOs get reused.
>>>
>>> Cc: Eric Anholt <eric@anholt.net>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>
>> Looks good to me, other than a bit of funny whitespace that I'll fix up.
>> I built a piglit test for this today (want to go take a look at those?),
>> and once I get a piglit run through, I'll push the change.
>
> I don't suppose you have a branch somewhere?  I should probably give
> that a try..  I've had
> https://trello.com/c/x34U0kTQ/114-teach-piglit-about-libdrm-freedreno-for-dmabuf-tests
> on my todo list for a while (but a generic gbm based solution seems
> even better than teaching piglit about libdrm_$drivername ;-))

https://cgit.freedesktop.org/~anholt/piglit/log/?h=dmabuf-refcount