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? BR, -R > Thanks for debugging! Note, I'm still on vacation, so I'll be slow at > replying. > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev