Rob Clark <robdcl...@gmail.com> writes: > On Sat, Jun 25, 2016 at 11:33 PM, Eric Anholt <e...@anholt.net> wrote: >> Rob Herring <r...@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 <e...@anholt.net> >>> Signed-off-by: Rob Herring <r...@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.
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev