[Mesa-dev] [PATCH 1/3] panfrost: Make sure a clear does not re-use a pre-existing batch

[Alyssa Rosenzweig]

To be clear, if we have a batch and do the following operations:

	clear red
	draw 1
	clear green
	draw 2
	flush

All we should see is #2 on a green background, which this patch handles
by the second clear invalidating all the clears/draws that came before
it (provided there is no flush in between). 

I might just be tripped up by the "freeze" name. That really means throw
away / free here, I guess?

Provided that's the idea (and we're not somehow saving the original draw
1), it's Reviewed-by A R <a.r at c.com>

On Fri, Sep 20, 2019 at 04:53:37PM +0200, Boris Brezillon wrote:
> glClear()s are expected to be the first thing GL apps do before drawing
> new things. If there's already an existing batch targetting the same
> FBO that has draws attached to it, we should make sure the new clear
> gets a new batch assigned to guaranteed that the FB content is actually
> cleared with the requested color/depth/stencil values.
> 
> We create a panfrost_get_fresh_batch_for_fbo() helper for that and
> call it from panfrost_clear().
> 
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> ---
>  src/gallium/drivers/panfrost/pan_context.c |  2 +-
>  src/gallium/drivers/panfrost/pan_job.c     | 21 +++++++++++++++++++++
>  src/gallium/drivers/panfrost/pan_job.h     |  3 +++
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> index ac01461a07fe..b2f2a9da7a51 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -162,7 +162,7 @@ panfrost_clear(
>          double depth, unsigned stencil)
>  {
>          struct panfrost_context *ctx = pan_context(pipe);
> -        struct panfrost_batch *batch = panfrost_get_batch_for_fbo(ctx);
> +        struct panfrost_batch *batch = panfrost_get_fresh_batch_for_fbo(ctx);
>  
>          panfrost_batch_add_fbo_bos(batch);
>          panfrost_batch_clear(batch, buffers, color, depth, stencil);
> diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
> index d8330bc133a6..4ec2aa0565d7 100644
> --- a/src/gallium/drivers/panfrost/pan_job.c
> +++ b/src/gallium/drivers/panfrost/pan_job.c
> @@ -298,6 +298,27 @@ panfrost_get_batch_for_fbo(struct panfrost_context *ctx)
>          return batch;
>  }
>  
> +struct panfrost_batch *
> +panfrost_get_fresh_batch_for_fbo(struct panfrost_context *ctx)
> +{
> +        struct panfrost_batch *batch;
> +
> +        batch = panfrost_get_batch(ctx, &ctx->pipe_framebuffer);
> +
> +        /* The batch has no draw/clear queued, let's return it directly.
> +         * Note that it's perfectly fine to re-use a batch with an
> +         * existing clear, we'll just update it with the new clear request.
> +         */
> +        if (!batch->last_job.gpu)
> +                return batch;
> +
> +        /* Otherwise, we need to freeze the existing one and instantiate a new
> +         * one.
> +         */
> +        panfrost_freeze_batch(batch);
> +        return panfrost_get_batch(ctx, &ctx->pipe_framebuffer);
> +}
> +
>  static bool
>  panfrost_batch_fence_is_signaled(struct panfrost_batch_fence *fence)
>  {
> diff --git a/src/gallium/drivers/panfrost/pan_job.h b/src/gallium/drivers/panfrost/pan_job.h
> index e1b1f56a2e64..0bd78bba267a 100644
> --- a/src/gallium/drivers/panfrost/pan_job.h
> +++ b/src/gallium/drivers/panfrost/pan_job.h
> @@ -172,6 +172,9 @@ panfrost_batch_fence_reference(struct panfrost_batch_fence *batch);
>  struct panfrost_batch *
>  panfrost_get_batch_for_fbo(struct panfrost_context *ctx);
>  
> +struct panfrost_batch *
> +panfrost_get_fresh_batch_for_fbo(struct panfrost_context *ctx);
> +
>  void
>  panfrost_batch_init(struct panfrost_context *ctx);
>  
> -- 
> 2.21.0