[Mesa-dev] [PATCH 1/3] panfrost: Cache BO imports
Boris Brezillon
boris.brezillon at collabora.com
Thu Jul 4 08:55:02 UTC 2019
Hi Tomeu,
On Thu, 4 Jul 2019 10:04:41 +0200
Tomeu Vizoso <tomeu.vizoso at collabora.com> wrote:
> If two jobs use the same GEM object at the same time, the job that
> finishes first will (previous to this commit) close the GEM object, even
> if there's a job still referencing it.
>
> To prevent this, have all jobs use the same panfrost_bo for a given GEM
> object, so it's only closed once the last job is done with it.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> ---
> src/gallium/drivers/panfrost/pan_allocate.h | 2 +-
> src/gallium/drivers/panfrost/pan_drm.c | 46 +++++++++++++++++++--
> src/gallium/drivers/panfrost/pan_resource.c | 20 ++++++++-
> src/gallium/drivers/panfrost/pan_screen.h | 6 +++
> 4 files changed, 68 insertions(+), 6 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/pan_allocate.h b/src/gallium/drivers/panfrost/pan_allocate.h
> index 20ba204dee86..2dfa913b8c4d 100644
> --- a/src/gallium/drivers/panfrost/pan_allocate.h
> +++ b/src/gallium/drivers/panfrost/pan_allocate.h
> @@ -59,7 +59,7 @@ struct panfrost_transfer {
> };
>
> struct panfrost_bo {
> - struct pipe_reference reference;
> + int refcnt;
>
> /* Mapping for the entire object (all levels) */
> uint8_t *cpu;
> diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c
> index b89f8e66a877..9648ac1d452d 100644
> --- a/src/gallium/drivers/panfrost/pan_drm.c
> +++ b/src/gallium/drivers/panfrost/pan_drm.c
> @@ -103,7 +103,12 @@ panfrost_drm_create_bo(struct panfrost_screen *screen, size_t size,
> // TODO map and unmap on demand?
> panfrost_drm_mmap_bo(screen, bo);
>
> - pipe_reference_init(&bo->reference, 1);
> + p_atomic_set(&bo->refcnt, 1);
> +
> + pthread_mutex_lock(&screen->handle_table_lock);
> + _mesa_hash_table_insert(screen->handle_table, &bo->gem_handle, bo);
> + pthread_mutex_unlock(&screen->handle_table_lock);
> +
> return bo;
> }
>
> @@ -116,6 +121,9 @@ panfrost_drm_release_bo(struct panfrost_screen *screen, struct panfrost_bo *bo)
> if (!bo)
> return;
>
> + pthread_mutex_lock(&screen->handle_table_lock);
> + _mesa_hash_table_remove_key(screen->handle_table, &bo->gem_handle);
> +
> panfrost_drm_munmap_bo(screen, bo);
>
> ret = drmIoctl(screen->fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
> @@ -125,6 +133,8 @@ panfrost_drm_release_bo(struct panfrost_screen *screen, struct panfrost_bo *bo)
> }
>
> ralloc_free(bo);
> +
> + pthread_mutex_unlock(&screen->handle_table_lock);
> }
>
> void
> @@ -150,17 +160,41 @@ panfrost_drm_free_slab(struct panfrost_screen *screen, struct panfrost_memory *m
> mem->bo = NULL;
> }
>
> +/* lookup a buffer, call w/ table_lock held: */
> +static struct panfrost_bo *lookup_bo(struct hash_table *tbl, uint32_t key)
I tend to add _locked() suffixes to functions that are supposed to be
called with a lock held, just for people who don't read comments (like
me :)).
> +{
> + struct panfrost_bo *bo = NULL;
> + struct hash_entry *entry = _mesa_hash_table_search(tbl, &key);
> + if (entry) {
> + /* found, incr refcnt and return: */
> + bo = entry->data;
You need:
/* BO is about to freed, don't return it. */
if (!p_atomic_read(&bo->refcnt))
return NULL;
here (see below for a detailed explanation about the race).
> + panfrost_bo_reference(bo);
> + }
> + return bo;
> +}
> +
> struct panfrost_bo *
> panfrost_drm_import_bo(struct panfrost_screen *screen, int fd)
> {
> - struct panfrost_bo *bo = rzalloc(screen, struct panfrost_bo);
> + struct panfrost_bo *bo = NULL;
> struct drm_panfrost_get_bo_offset get_bo_offset = {0,};
> int ret;
> unsigned gem_handle;
>
> + pthread_mutex_lock(&screen->handle_table_lock);
> +
Unrelated/nit: we should really agree on an indentation model (tab vs
spaces). I keep trying to adapt to the context surrounding my changes
(using tabs when tabs are used nearby, spaces otherwise), but now
we're starting to have a mix of tabs and spaces inside the same
functions.
> ret = drmPrimeFDToHandle(screen->fd, fd, &gem_handle);
> assert(!ret);
>
> + if (ret)
> + goto out_unlock;
> +
Can't we take the lock here instead?
> + bo = lookup_bo(screen->handle_table, gem_handle);
> + if (bo)
> + goto out_unlock;
> +
> + bo = rzalloc(screen, struct panfrost_bo);
> +
> get_bo_offset.handle = gem_handle;
> ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_GET_BO_OFFSET, &get_bo_offset);
> assert(!ret);
> @@ -169,10 +203,16 @@ panfrost_drm_import_bo(struct panfrost_screen *screen, int fd)
> bo->gpu = (mali_ptr) get_bo_offset.offset;
> bo->size = lseek(fd, 0, SEEK_END);
> assert(bo->size > 0);
> - pipe_reference_init(&bo->reference, 1);
> + p_atomic_set(&bo->refcnt, 1);
>
> // TODO map and unmap on demand?
> panfrost_drm_mmap_bo(screen, bo);
> +
> + _mesa_hash_table_insert(screen->handle_table, &bo->gem_handle, bo);
> +
> +out_unlock:
> + pthread_mutex_unlock(&screen->handle_table_lock);
> +
> return bo;
> }
>
> diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c
> index b651fcffb111..beef26a5ded0 100644
> --- a/src/gallium/drivers/panfrost/pan_resource.c
> +++ b/src/gallium/drivers/panfrost/pan_resource.c
> @@ -47,6 +47,18 @@
> #include "pan_util.h"
> #include "pan_tiling.h"
>
> +static uint32_t
> +u32_hash(const void *key)
> +{
> + return _mesa_hash_data(key, sizeof(uint32_t));
> +}
> +
> +static bool
> +u32_equals(const void *key1, const void *key2)
> +{
> + return *(const uint32_t *)key1 == *(const uint32_t *)key2;
> +}
> +
> static struct pipe_resource *
> panfrost_resource_from_handle(struct pipe_screen *pscreen,
> const struct pipe_resource *templat,
> @@ -434,7 +446,7 @@ panfrost_resource_create(struct pipe_screen *screen,
> void
> panfrost_bo_reference(struct panfrost_bo *bo)
> {
> - pipe_reference(NULL, &bo->reference);
> + p_atomic_inc(&bo->refcnt);
> }
>
> void
> @@ -442,7 +454,7 @@ panfrost_bo_unreference(struct pipe_screen *screen, struct panfrost_bo *bo)
> {
> /* When the reference count goes to zero, we need to cleanup */
>
> - if (pipe_reference(&bo->reference, NULL))
> + if (p_atomic_dec_zero(&bo->refcnt))
> panfrost_drm_release_bo(pan_screen(screen), bo);
I think there's a race here: the BO has reached a refcnt of 0 but it's
still present in the hash table, and the lock is not held, meaning
that someone might retrieve the BO just before we had the chance to
remove it and in this case we end up in a use-after-free situation.
See above for a proposal to fix the race.
> }
>
> @@ -795,11 +807,15 @@ panfrost_resource_screen_init(struct panfrost_screen *pscreen)
> panfrost_slab_can_reclaim,
> panfrost_slab_alloc,
> panfrost_slab_free);
> +
> + pscreen->handle_table = _mesa_hash_table_create(NULL, u32_hash, u32_equals);
> + pthread_mutex_init(&pscreen->handle_table_lock, NULL);
> }
>
> void
> panfrost_resource_screen_deinit(struct panfrost_screen *pscreen)
> {
> + _mesa_hash_table_destroy(pscreen->handle_table, NULL);
> pb_slabs_deinit(&pscreen->slabs);
> }
>
> diff --git a/src/gallium/drivers/panfrost/pan_screen.h b/src/gallium/drivers/panfrost/pan_screen.h
> index 9bcea6114285..db47e63fda6c 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.h
> +++ b/src/gallium/drivers/panfrost/pan_screen.h
> @@ -29,9 +29,12 @@
> #ifndef PAN_SCREEN_H
> #define PAN_SCREEN_H
>
> +#include <pthread.h>
> +
> #include "pipe/p_screen.h"
> #include "pipe/p_defines.h"
> #include "renderonly/renderonly.h"
> +#include "util/hash_table.h"
>
> #include <panfrost-misc.h>
> #include "pan_allocate.h"
> @@ -63,6 +66,9 @@ struct panfrost_screen {
> * yesterjob */
> int last_fragment_flushed;
> struct panfrost_job *last_job;
> +
> + pthread_mutex_t handle_table_lock;
> + struct hash_table *handle_table;
> };
>
> static inline struct panfrost_screen *
Regards,
Boris
More information about the mesa-dev
mailing list