[Mesa-dev] [PATCH v2 1/2] panfrost: Jobs must be per context, not per screen

[Boris Brezillon]

On Fri, 30 Aug 2019 18:00:12 +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.
> 
> Signed-off-by: Rohan Garg <rohan.garg at collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon at collabora.com>

> ---
>  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_screen.c  |  3 ---
>  src/gallium/drivers/panfrost/pan_screen.h  |  6 ------
>  5 files changed, 14 insertions(+), 17 deletions(-)
> 
> 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_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
> index 36c91a1572e..5c288f52bbd 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.c
> +++ b/src/gallium/drivers/panfrost/pan_screen.c
> @@ -665,9 +665,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..7991b395f54 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.h
> +++ b/src/gallium/drivers/panfrost/pan_screen.h
> @@ -118,12 +118,6 @@ struct panfrost_screen {
>           * 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 *