panfrost: Jobs must be per context, not per screen

Submitted by Rohan Garg on Aug. 29, 2019, 12:51 p.m.

Details

Message ID 20190829125156.4975-1-rohan.garg@collabora.com
State New
Headers show
Series "panfrost: Jobs must be per context, not per screen" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Rohan Garg Aug. 29, 2019, 12:51 p.m.
Jobs _must_ only be shared across the same context, having
the last_job tracked in a screen causes use-after-free issues
and memory corruptions.

Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
---
 src/gallium/drivers/panfrost/pan_allocate.c |  2 ++
 src/gallium/drivers/panfrost/pan_bo_cache.c | 16 +++++++++++-----
 src/gallium/drivers/panfrost/pan_context.c  | 10 +++++-----
 src/gallium/drivers/panfrost/pan_context.h  |  6 ++++++
 src/gallium/drivers/panfrost/pan_drm.c      |  6 +++---
 src/gallium/drivers/panfrost/pan_job.c      |  2 ++
 src/gallium/drivers/panfrost/pan_screen.c   |  5 ++---
 src/gallium/drivers/panfrost/pan_screen.h   | 10 ++++------
 8 files changed, 35 insertions(+), 22 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_allocate.c b/src/gallium/drivers/panfrost/pan_allocate.c
index f549c864c70..fb8b18fe718 100644
--- a/src/gallium/drivers/panfrost/pan_allocate.c
+++ b/src/gallium/drivers/panfrost/pan_allocate.c
@@ -74,6 +74,7 @@  panfrost_allocate_transient(struct panfrost_context *ctx, size_t sz)
         unsigned offset = 0;
         bool update_offset = false;
 
+        pthread_mutex_lock(&screen->transient_lock);
         bool has_current = batch->transient_indices.size;
         bool fits_in_current = (batch->transient_offset + sz) < TRANSIENT_SLAB_SIZE;
 
@@ -131,6 +132,7 @@  panfrost_allocate_transient(struct panfrost_context *ctx, size_t sz)
 
         if (update_offset)
                 batch->transient_offset = offset + sz;
+        pthread_mutex_unlock(&screen->transient_lock);
 
         return ret;
 
diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c b/src/gallium/drivers/panfrost/pan_bo_cache.c
index 9dd6b694b72..f2f49437a89 100644
--- a/src/gallium/drivers/panfrost/pan_bo_cache.c
+++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
@@ -24,6 +24,7 @@ 
  *   Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
  */
 #include <xf86drm.h>
+#include <pthread.h>
 #include "drm-uapi/panfrost_drm.h"
 
 #include "pan_screen.h"
@@ -84,7 +85,9 @@  panfrost_bo_cache_fetch(
                 struct panfrost_screen *screen,
                 size_t size, uint32_t flags)
 {
+        pthread_mutex_lock(&screen->bo_cache_lock);
         struct list_head *bucket = pan_bucket(screen, size);
+        struct panfrost_bo *bo = NULL;
 
         /* Iterate the bucket looking for something suitable */
         list_for_each_entry_safe(struct panfrost_bo, entry, bucket, link) {
@@ -106,12 +109,13 @@  panfrost_bo_cache_fetch(
                                 continue;
                         }
                         /* Let's go! */
-                        return entry;
+                        bo = entry;
+                        break;
                 }
         }
+        pthread_mutex_unlock(&screen->bo_cache_lock);
 
-        /* We didn't find anything */
-        return NULL;
+        return bo;
 }
 
 /* Tries to add a BO to the cache. Returns if it was
@@ -122,6 +126,7 @@  panfrost_bo_cache_put(
                 struct panfrost_screen *screen,
                 struct panfrost_bo *bo)
 {
+        pthread_mutex_lock(&screen->bo_cache_lock);
         struct list_head *bucket = pan_bucket(screen, bo->size);
         struct drm_panfrost_madvise madv;
 
@@ -133,6 +138,7 @@  panfrost_bo_cache_put(
 
         /* Add us to the bucket */
         list_addtail(&bo->link, bucket);
+        pthread_mutex_unlock(&screen->bo_cache_lock);
 
         return true;
 }
@@ -147,6 +153,7 @@  void
 panfrost_bo_cache_evict_all(
                 struct panfrost_screen *screen)
 {
+        pthread_mutex_lock(&screen->bo_cache_lock);
         for (unsigned i = 0; i < ARRAY_SIZE(screen->bo_cache); ++i) {
                 struct list_head *bucket = &screen->bo_cache[i];
 
@@ -155,7 +162,6 @@  panfrost_bo_cache_evict_all(
                         panfrost_drm_release_bo(screen, entry, false);
                 }
         }
-
-        return;
+        pthread_mutex_unlock(&screen->bo_cache_lock);
 }
 
diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
index fa9c92af9f6..94ee9b5bdb2 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -1329,9 +1329,6 @@  panfrost_submit_frame(struct panfrost_context *ctx, bool flush_immediate,
                       struct pipe_fence_handle **fence,
                       struct panfrost_job *job)
 {
-        struct pipe_context *gallium = (struct pipe_context *) ctx;
-        struct panfrost_screen *screen = pan_screen(gallium->screen);
-
         panfrost_job_submit(ctx, job);
 
         /* If visual, we can stall a frame */
@@ -1339,8 +1336,8 @@  panfrost_submit_frame(struct panfrost_context *ctx, bool flush_immediate,
         if (!flush_immediate)
                 panfrost_drm_force_flush_fragment(ctx, fence);
 
-        screen->last_fragment_flushed = false;
-        screen->last_job = job;
+        ctx->last_fragment_flushed = false;
+        ctx->last_job = job;
 
         /* If readback, flush now (hurts the pipelined performance) */
         if (flush_immediate)
@@ -2856,6 +2853,9 @@  panfrost_create_context(struct pipe_screen *screen, void *priv, unsigned flags)
         assert(ctx->blitter);
         assert(ctx->blitter_wallpaper);
 
+        ctx->last_fragment_flushed = true;
+        ctx->last_job = NULL;
+
         /* Prepare for render! */
 
         panfrost_job_init(ctx);
diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
index 4c1580b3393..9f96e983a86 100644
--- a/src/gallium/drivers/panfrost/pan_context.h
+++ b/src/gallium/drivers/panfrost/pan_context.h
@@ -203,6 +203,12 @@  struct panfrost_context {
         bool is_t6xx;
 
         uint32_t out_sync;
+
+        /* While we're busy building up the job for frame N, the GPU is
+         * still busy executing frame N-1. So hold a reference to
+         * yesterjob */
+        int last_fragment_flushed;
+        struct panfrost_job *last_job;
 };
 
 /* Corresponds to the CSO */
diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c
index c3693bff56a..f89bc1d26eb 100644
--- a/src/gallium/drivers/panfrost/pan_drm.c
+++ b/src/gallium/drivers/panfrost/pan_drm.c
@@ -349,12 +349,12 @@  panfrost_drm_force_flush_fragment(struct panfrost_context *ctx,
         struct pipe_context *gallium = (struct pipe_context *) ctx;
         struct panfrost_screen *screen = pan_screen(gallium->screen);
 
-        if (!screen->last_fragment_flushed) {
+        if (!ctx->last_fragment_flushed) {
                 drmSyncobjWait(screen->fd, &ctx->out_sync, 1, INT64_MAX, 0, NULL);
-                screen->last_fragment_flushed = true;
+                ctx->last_fragment_flushed = true;
 
                 /* The job finished up, so we're safe to clean it up now */
-                panfrost_free_job(ctx, screen->last_job);
+                panfrost_free_job(ctx, ctx->last_job);
         }
 
         if (fence) {
diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
index 4d8ec2eadc9..72afcdbf358 100644
--- a/src/gallium/drivers/panfrost/pan_job.c
+++ b/src/gallium/drivers/panfrost/pan_job.c
@@ -67,10 +67,12 @@  panfrost_free_job(struct panfrost_context *ctx, struct panfrost_job *job)
         /* Free up the transient BOs we're sitting on */
         struct panfrost_screen *screen = pan_screen(ctx->base.screen);
 
+        pthread_mutex_lock(&screen->transiant_lock);
         util_dynarray_foreach(&job->transient_indices, unsigned, index) {
                 /* Mark it free */
                 BITSET_SET(screen->free_transient, *index);
         }
+        pthread_mutex_unlock(&screen->transiant_lock);
 
         /* Unreference the polygon list */
         panfrost_bo_unreference(ctx->base.screen, job->polygon_list);
diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
index 36c91a1572e..07f60ffee52 100644
--- a/src/gallium/drivers/panfrost/pan_screen.c
+++ b/src/gallium/drivers/panfrost/pan_screen.c
@@ -639,8 +639,10 @@  panfrost_create_screen(int fd, struct renderonly *ro)
                 return NULL;
         }
 
+	pthread_mutex_init(&screen->transiant_lock, NULL);
         util_dynarray_init(&screen->transient_bo, screen);
 
+	pthread_mutex_init(&screen->bo_cache_lock, NULL);
         for (unsigned i = 0; i < ARRAY_SIZE(screen->bo_cache); ++i)
                 list_inithead(&screen->bo_cache[i]);
 
@@ -665,9 +667,6 @@  panfrost_create_screen(int fd, struct renderonly *ro)
         screen->base.fence_finish = panfrost_fence_finish;
         screen->base.set_damage_region = panfrost_resource_set_damage_region;
 
-        screen->last_fragment_flushed = true;
-        screen->last_job = NULL;
-
         panfrost_resource_screen_init(screen);
 
         return &screen->base;
diff --git a/src/gallium/drivers/panfrost/pan_screen.h b/src/gallium/drivers/panfrost/pan_screen.h
index 35fb8de2628..f83cd7562ab 100644
--- a/src/gallium/drivers/panfrost/pan_screen.h
+++ b/src/gallium/drivers/panfrost/pan_screen.h
@@ -104,6 +104,8 @@  struct panfrost_screen {
 
         struct renderonly *ro;
 
+        pthread_mutex_t transient_lock;
+
         /* Transient memory management is based on borrowing fixed-size slabs
          * off the screen (loaning them out to the batch). Dynamic array
          * container of panfrost_bo */
@@ -113,17 +115,13 @@  struct panfrost_screen {
         /* Set of free transient BOs */
         BITSET_DECLARE(free_transient, MAX_TRANSIENT_SLABS);
 
+        pthread_mutex_t bo_cache_lock;
+
         /* The BO cache is a set of buckets with power-of-two sizes ranging
          * from 2^12 (4096, the page size) to 2^(12 + MAX_BO_CACHE_BUCKETS).
          * Each bucket is a linked list of free panfrost_bo objects. */
 
         struct list_head bo_cache[NR_BO_CACHE_BUCKETS];
-
-        /* While we're busy building up the job for frame N, the GPU is
-         * still busy executing frame N-1. So hold a reference to
-         * yesterjob */
-        int last_fragment_flushed;
-        struct panfrost_job *last_job;
 };
 
 static inline struct panfrost_screen *

Comments

On Thu, 29 Aug 2019 14:51:52 +0200
Rohan Garg <rohan.garg@collabora.com> wrote:

> Jobs _must_ only be shared across the same context, having
> the last_job tracked in a screen causes use-after-free issues
> and memory corruptions.

You should probably also mention that transient-pool and bo-cache
related fields should be protected by a mutex as they are shared by all
contexts. Or even better, split that patch in 2:

1/ move last_job, last_fragment_pushed to panfrost_context
2/ protect transient/bo-cache manipulation with mutexes

> 
> Signed-off-by: Rohan Garg <rohan.garg@collabora.com>
> ---
>  src/gallium/drivers/panfrost/pan_allocate.c |  2 ++
>  src/gallium/drivers/panfrost/pan_bo_cache.c | 16 +++++++++++-----
>  src/gallium/drivers/panfrost/pan_context.c  | 10 +++++-----
>  src/gallium/drivers/panfrost/pan_context.h  |  6 ++++++
>  src/gallium/drivers/panfrost/pan_drm.c      |  6 +++---
>  src/gallium/drivers/panfrost/pan_job.c      |  2 ++
>  src/gallium/drivers/panfrost/pan_screen.c   |  5 ++---
>  src/gallium/drivers/panfrost/pan_screen.h   | 10 ++++------
>  8 files changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/src/gallium/drivers/panfrost/pan_allocate.c b/src/gallium/drivers/panfrost/pan_allocate.c
> index f549c864c70..fb8b18fe718 100644
> --- a/src/gallium/drivers/panfrost/pan_allocate.c
> +++ b/src/gallium/drivers/panfrost/pan_allocate.c
> @@ -74,6 +74,7 @@ panfrost_allocate_transient(struct panfrost_context *ctx, size_t sz)
>          unsigned offset = 0;
>          bool update_offset = false;
>  
> +        pthread_mutex_lock(&screen->transient_lock);
>          bool has_current = batch->transient_indices.size;
>          bool fits_in_current = (batch->transient_offset + sz) < TRANSIENT_SLAB_SIZE;
>  
> @@ -131,6 +132,7 @@ panfrost_allocate_transient(struct panfrost_context *ctx, size_t sz)
>  
>          if (update_offset)
>                  batch->transient_offset = offset + sz;
> +        pthread_mutex_unlock(&screen->transient_lock);
>  
>          return ret;
>  
> diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c b/src/gallium/drivers/panfrost/pan_bo_cache.c
> index 9dd6b694b72..f2f49437a89 100644
> --- a/src/gallium/drivers/panfrost/pan_bo_cache.c
> +++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
> @@ -24,6 +24,7 @@
>   *   Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>   */
>  #include <xf86drm.h>
> +#include <pthread.h>
>  #include "drm-uapi/panfrost_drm.h"
>  
>  #include "pan_screen.h"
> @@ -84,7 +85,9 @@ panfrost_bo_cache_fetch(
>                  struct panfrost_screen *screen,
>                  size_t size, uint32_t flags)
>  {
> +        pthread_mutex_lock(&screen->bo_cache_lock);
>          struct list_head *bucket = pan_bucket(screen, size);
> +        struct panfrost_bo *bo = NULL;
>  
>          /* Iterate the bucket looking for something suitable */
>          list_for_each_entry_safe(struct panfrost_bo, entry, bucket, link) {
> @@ -106,12 +109,13 @@ panfrost_bo_cache_fetch(
>                                  continue;
>                          }
>                          /* Let's go! */
> -                        return entry;
> +                        bo = entry;
> +                        break;
>                  }
>          }
> +        pthread_mutex_unlock(&screen->bo_cache_lock);
>  
> -        /* We didn't find anything */
> -        return NULL;
> +        return bo;
>  }
>  
>  /* Tries to add a BO to the cache. Returns if it was
> @@ -122,6 +126,7 @@ panfrost_bo_cache_put(
>                  struct panfrost_screen *screen,
>                  struct panfrost_bo *bo)
>  {
> +        pthread_mutex_lock(&screen->bo_cache_lock);
>          struct list_head *bucket = pan_bucket(screen, bo->size);
>          struct drm_panfrost_madvise madv;
>  
> @@ -133,6 +138,7 @@ panfrost_bo_cache_put(
>  
>          /* Add us to the bucket */
>          list_addtail(&bo->link, bucket);
> +        pthread_mutex_unlock(&screen->bo_cache_lock);
>  
>          return true;
>  }
> @@ -147,6 +153,7 @@ void
>  panfrost_bo_cache_evict_all(
>                  struct panfrost_screen *screen)
>  {
> +        pthread_mutex_lock(&screen->bo_cache_lock);
>          for (unsigned i = 0; i < ARRAY_SIZE(screen->bo_cache); ++i) {
>                  struct list_head *bucket = &screen->bo_cache[i];
>  
> @@ -155,7 +162,6 @@ panfrost_bo_cache_evict_all(
>                          panfrost_drm_release_bo(screen, entry, false);
>                  }
>          }
> -
> -        return;
> +        pthread_mutex_unlock(&screen->bo_cache_lock);
>  }
>  
> diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> index fa9c92af9f6..94ee9b5bdb2 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -1329,9 +1329,6 @@ panfrost_submit_frame(struct panfrost_context *ctx, bool flush_immediate,
>                        struct pipe_fence_handle **fence,
>                        struct panfrost_job *job)
>  {
> -        struct pipe_context *gallium = (struct pipe_context *) ctx;
> -        struct panfrost_screen *screen = pan_screen(gallium->screen);
> -
>          panfrost_job_submit(ctx, job);
>  
>          /* If visual, we can stall a frame */
> @@ -1339,8 +1336,8 @@ panfrost_submit_frame(struct panfrost_context *ctx, bool flush_immediate,
>          if (!flush_immediate)
>                  panfrost_drm_force_flush_fragment(ctx, fence);
>  
> -        screen->last_fragment_flushed = false;
> -        screen->last_job = job;
> +        ctx->last_fragment_flushed = false;
> +        ctx->last_job = job;
>  
>          /* If readback, flush now (hurts the pipelined performance) */
>          if (flush_immediate)
> @@ -2856,6 +2853,9 @@ panfrost_create_context(struct pipe_screen *screen, void *priv, unsigned flags)
>          assert(ctx->blitter);
>          assert(ctx->blitter_wallpaper);
>  
> +        ctx->last_fragment_flushed = true;
> +        ctx->last_job = NULL;
> +
>          /* Prepare for render! */
>  
>          panfrost_job_init(ctx);
> diff --git a/src/gallium/drivers/panfrost/pan_context.h b/src/gallium/drivers/panfrost/pan_context.h
> index 4c1580b3393..9f96e983a86 100644
> --- a/src/gallium/drivers/panfrost/pan_context.h
> +++ b/src/gallium/drivers/panfrost/pan_context.h
> @@ -203,6 +203,12 @@ struct panfrost_context {
>          bool is_t6xx;
>  
>          uint32_t out_sync;
> +
> +        /* While we're busy building up the job for frame N, the GPU is
> +         * still busy executing frame N-1. So hold a reference to
> +         * yesterjob */
> +        int last_fragment_flushed;
> +        struct panfrost_job *last_job;
>  };
>  
>  /* Corresponds to the CSO */
> diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c
> index c3693bff56a..f89bc1d26eb 100644
> --- a/src/gallium/drivers/panfrost/pan_drm.c
> +++ b/src/gallium/drivers/panfrost/pan_drm.c
> @@ -349,12 +349,12 @@ panfrost_drm_force_flush_fragment(struct panfrost_context *ctx,
>          struct pipe_context *gallium = (struct pipe_context *) ctx;
>          struct panfrost_screen *screen = pan_screen(gallium->screen);
>  
> -        if (!screen->last_fragment_flushed) {
> +        if (!ctx->last_fragment_flushed) {
>                  drmSyncobjWait(screen->fd, &ctx->out_sync, 1, INT64_MAX, 0, NULL);
> -                screen->last_fragment_flushed = true;
> +                ctx->last_fragment_flushed = true;
>  
>                  /* The job finished up, so we're safe to clean it up now */
> -                panfrost_free_job(ctx, screen->last_job);
> +                panfrost_free_job(ctx, ctx->last_job);
>          }
>  
>          if (fence) {
> diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
> index 4d8ec2eadc9..72afcdbf358 100644
> --- a/src/gallium/drivers/panfrost/pan_job.c
> +++ b/src/gallium/drivers/panfrost/pan_job.c
> @@ -67,10 +67,12 @@ panfrost_free_job(struct panfrost_context *ctx, struct panfrost_job *job)
>          /* Free up the transient BOs we're sitting on */
>          struct panfrost_screen *screen = pan_screen(ctx->base.screen);
>  
> +        pthread_mutex_lock(&screen->transiant_lock);
>          util_dynarray_foreach(&job->transient_indices, unsigned, index) {
>                  /* Mark it free */
>                  BITSET_SET(screen->free_transient, *index);
>          }
> +        pthread_mutex_unlock(&screen->transiant_lock);
>  
>          /* Unreference the polygon list */
>          panfrost_bo_unreference(ctx->base.screen, job->polygon_list);
> diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
> index 36c91a1572e..07f60ffee52 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.c
> +++ b/src/gallium/drivers/panfrost/pan_screen.c
> @@ -639,8 +639,10 @@ panfrost_create_screen(int fd, struct renderonly *ro)
>                  return NULL;
>          }
>  
> +	pthread_mutex_init(&screen->transiant_lock, NULL);
>          util_dynarray_init(&screen->transient_bo, screen);
>  
> +	pthread_mutex_init(&screen->bo_cache_lock, NULL);
>          for (unsigned i = 0; i < ARRAY_SIZE(screen->bo_cache); ++i)
>                  list_inithead(&screen->bo_cache[i]);
>  
> @@ -665,9 +667,6 @@ panfrost_create_screen(int fd, struct renderonly *ro)
>          screen->base.fence_finish = panfrost_fence_finish;
>          screen->base.set_damage_region = panfrost_resource_set_damage_region;
>  
> -        screen->last_fragment_flushed = true;
> -        screen->last_job = NULL;
> -
>          panfrost_resource_screen_init(screen);
>  
>          return &screen->base;
> diff --git a/src/gallium/drivers/panfrost/pan_screen.h b/src/gallium/drivers/panfrost/pan_screen.h
> index 35fb8de2628..f83cd7562ab 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.h
> +++ b/src/gallium/drivers/panfrost/pan_screen.h
> @@ -104,6 +104,8 @@ struct panfrost_screen {
>  
>          struct renderonly *ro;
>  
> +        pthread_mutex_t transient_lock;
> +
>          /* Transient memory management is based on borrowing fixed-size slabs
>           * off the screen (loaning them out to the batch). Dynamic array
>           * container of panfrost_bo */
> @@ -113,17 +115,13 @@ struct panfrost_screen {
>          /* Set of free transient BOs */
>          BITSET_DECLARE(free_transient, MAX_TRANSIENT_SLABS);
>  
> +        pthread_mutex_t bo_cache_lock;
> +
>          /* The BO cache is a set of buckets with power-of-two sizes ranging
>           * from 2^12 (4096, the page size) to 2^(12 + MAX_BO_CACHE_BUCKETS).
>           * Each bucket is a linked list of free panfrost_bo objects. */
>  
>          struct list_head bo_cache[NR_BO_CACHE_BUCKETS];
> -
> -        /* While we're busy building up the job for frame N, the GPU is
> -         * still busy executing frame N-1. So hold a reference to
> -         * yesterjob */
> -        int last_fragment_flushed;
> -        struct panfrost_job *last_job;
>  };
>  
>  static inline struct panfrost_screen *