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

[Eric Anholt]

Rob Clark <robdclark at gmail.com> writes:

> On Sat, Jun 25, 2016 at 11:33 PM, Eric Anholt <eric at anholt.net> wrote:
>> Rob Herring <robh at 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 at anholt.net>
>>> Signed-off-by: Rob Herring <robh at kernel.org>
>>> ---
>>> With this and the fd hashing to get a single screen, the flickery screen
>>> is gone and Android is somewhat working. Several apps though hang, don't
>>> render, and then exit. I also see CMA allocation errors, but not
>>> correlating to the app problems.
>>>
>>> Also, flink names need similar hash table look-up as well. Maybe that's
>>> a don't care for vc4? In any case, I don't have the setup to test that.
>>>
>>> Rob
>>>
>>>  src/gallium/drivers/vc4/vc4_bufmgr.c | 20 +++++++++++++++++++-
>>>  src/gallium/drivers/vc4/vc4_bufmgr.h | 12 +++++++++++-
>>>  src/gallium/drivers/vc4/vc4_screen.c | 15 +++++++++++++++
>>>  src/gallium/drivers/vc4/vc4_screen.h |  3 +++
>>>  4 files changed, 48 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/vc4/vc4_bufmgr.c b/src/gallium/drivers/vc4/vc4_bufmgr.c
>>> index 21e3bde..d91157b 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;
>>>  }
>>>
>>> @@ -401,6 +415,10 @@ vc4_bo_get_dmabuf(struct vc4_bo *bo)
>>>          }
>>>          bo->private = false;
>>>
>>> +        pipe_mutex_lock(bo->screen->bo_handles_mutex);
>>> +        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..0896b30 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,20 @@ 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))
>>> +        screen = (*bo)->screen;
>>> +        pipe_mutex_lock(screen->bo_handles_mutex);
>>> +
>>> +        if (pipe_reference(&(*bo)->reference, NULL)) {
>>>                  vc4_bo_last_unreference(*bo);
>>> +                util_hash_table_remove(screen->bo_handles,
>>> +                                       (void *)(uintptr_t)(*bo)->handle);
>>
>> I think you're use-after-freeing bo here.  Just stick it before
>> last_unreference()?
>>
>>> +        }
>>> +
>>> +        pipe_mutex_unlock(screen->bo_handles_mutex);
>>>          *bo = NULL;
>>>  }
>>
>> Taking a mutex on every unref sucks -- it's a *really* hot path, and it
>> kind of defeats the point of doing these pipe_reference atomics.  We
>> should be able to skip the mutex in the bo->private case, since any
>> flinked/dmabuf BO will be !private.  Think you could give that a shot?
>
> or just move the mutex inside the if (pipe_reference(...)).. so you
> only take it on final unref?

Then you can have the refcount go to 0 and then back to 1 (when it gets
pulled out of the hash table before you grabbed the mutex), which I
assumed the API would prevent.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160626/4df21b21/attachment.sig>