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

[Rob Clark]

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?

BR,
-R


> Thanks for debugging!  Note, I'm still on vacation, so I'll be slow at
> replying.
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>