[Mesa-dev] [PATCH 6/9] panfrost: Don't emit a new FB desc when setting a new FB state

[Boris Brezillon]

On Fri, 2 Aug 2019 08:12:15 -0700
Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com> wrote:

> So, in the future, we'll want to have multiple jobs for different
> framebuffers independently in flight at the same time. As an
> illustrative example, we would like the app to be able to do a sequence
> like:
> 
> 	bind scanout
> 	clear
> 	draw something
> 	bind FBO
> 	clear
> 	draw something
> 	bind scanout
> 	draw something else
> 	bind FBO
> 	draw something else
> 	bind scanout
> 	draw the fbo
> 	flush
> 
> Ideally, nothing should happen until the final flush. So the order
> submitted to the hardware would be:
> 
> 	FBO:
> 	clear
> 	draw something
> 	draw something else 
> 	draw something else
> 
> 	Scanout:
> 	clear
> 	draw something
> 	draw something else 
> 	draw the fbo
> 
> Unfortunately, right now that panfrost_flush() call makes the order
> instead:
> 
> 	Scanout:
> 	clear
> 	draw something
> 	
> 	FBO:
> 	clear
> 	draw something
> 
> 	Scanout:
> 	clear
> 	wallpaper scanout
> 	draw something else
> 
> 	FBO:
> 	clear
> 	wallpaper fbo
> 	draw something else
> 
> 	Scanout:
> 	clear
> 	wallpaper scanout
> 	draw FBO
> 
> Clearly this is extremely suboptimal (I believe the extra wallpaper
> blits are related to our poor performance on FBO-heavy apps. glmark's
> -bdesktop and -bterrain are suspects here.)
> 
> The solution is to be able to have switching framebuffers be a no-op,
> since all the important stuff is in the panfrost_job/batch. So
> panfrost_set_framebuffer_state is just doing the copy, but no flush and
> almost no logic, so we can switch back and forth freely without
> incurring a flush/clear/wallpaper cycle each time.

Sounds quite interesting :-).

> 
> This patch series is not the right time to focus on pipelined
> performance (though I'd love if somebody gave it some love at some
> point). But I do want to caution from a code smell that will make
> whoever that someone is -- quite possibly you! -- quite grumpy when they
> work on this in future.

Yep, I can imagine this will lead to some heavy refactoring.

> 
> On Fri, Aug 02, 2019 at 12:12:54PM +0200, Boris Brezillon wrote:
> > The FB desc will be emitted/attached on the first draw targetting
> > this new FB.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> > ---
> >  src/gallium/drivers/panfrost/pan_context.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> > index 1091caeb1148..2b7906eea155 100644
> > --- a/src/gallium/drivers/panfrost/pan_context.c
> > +++ b/src/gallium/drivers/panfrost/pan_context.c
> > @@ -2384,6 +2384,9 @@ panfrost_set_framebuffer_state(struct pipe_context *pctx,
> >  
> >          if (!is_scanout || has_draws)
> >                  panfrost_flush(pctx, NULL, PIPE_FLUSH_END_OF_FRAME);
> > +        else
> > +                assert(!ctx->payloads[PIPE_SHADER_VERTEX].postfix.framebuffer &&
> > +                       !ctx->payloads[PIPE_SHADER_FRAGMENT].postfix.framebuffer);
> >  
> >          util_copy_framebuffer_state(&ctx->pipe_framebuffer, fb);
> >  
> > @@ -2391,7 +2394,8 @@ panfrost_set_framebuffer_state(struct pipe_context *pctx,
> >          struct panfrost_screen *screen = pan_screen(ctx->base.screen);
> >  
> >          panfrost_hint_afbc(screen, &ctx->pipe_framebuffer);
> > -        panfrost_attach_vt_framebuffer(ctx, false);
> > +        for (unsigned i = 0; i < PIPE_SHADER_TYPES; ++i)
> > +                ctx->payloads[i].postfix.framebuffer = 0;
> >  }
> >  
> >  static void *
> > -- 
> > 2.21.0
> >