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

[Boris Brezillon]

On Fri, 20 Sep 2019 15:45:33 -0400
Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:

> 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?

Nope. Freeze means "stop queuing new draws to this batch". I guess we
could free the batch as well if the result of the previous draws/clear
are really overwritten by this new clear, but that implies checking the
new clear flags to make sure they target the same depth/stencil/color
combination. On the other hand, I'm wondering if it's really the
driver's job to try to optimize silly things the apps might do. I mean,
the sequence you describe does not look like a wise thing to do since
the "clear red+draw 1" end up being overwritten by "clear green + draw
2".

> 
> 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