[1/3] panfrost: Cache BO imports

Submitted by Tomeu Vizoso on July 4, 2019, 8:04 a.m.

Details

Message ID 20190704080443.4153-1-tomeu.vizoso@collabora.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Tomeu Vizoso July 4, 2019, 8:04 a.m.
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@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(-)

Patch hide | download patch | download mbox

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)
+{
+	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;
+		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);
+
 	ret = drmPrimeFDToHandle(screen->fd, fd, &gem_handle);
 	assert(!ret);
 
+        if (ret)
+                goto out_unlock;
+
+        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);
 }
 
@@ -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 *

Comments

Hi Tomeu,

On Thu,  4 Jul 2019 10:04:41 +0200
Tomeu Vizoso <tomeu.vizoso@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@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