[Mesa-dev] [PATCH] panfrost: Jobs must be per context, not per screen
Boris Brezillon
boris.brezillon at collabora.com
Thu Aug 29 13:05:10 UTC 2019
On Thu, 29 Aug 2019 14:51:52 +0200
Rohan Garg <rohan.garg at 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 at 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 at 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 *
More information about the mesa-dev
mailing list