[Mesa-dev] [PATCH 05/10] i965/meta/gen9: Individually fast clear color attachments

[Ben Widawsky]

On Wed, Oct 14, 2015 at 02:43:24PM +0300, Pohjolainen, Topi wrote:
> On Wed, Oct 14, 2015 at 11:39:03AM +0200, Neil Roberts wrote:
> > Ben Widawsky <benjamin.widawsky at intel.com> writes:
> > 
> > > The impetus for this patch comes from a seemingly benign statement within the
> > > spec (quoted within the patch). For me, this patch was at some point critical
> > > for getting stable piglit results (though this did not seem to be the case on a
> > > branch Chad was working on).
> > >
> > > It is very important for clearing multiple color buffer attachments and can be
> > > observed in the following piglit tests:
> > > spec/arb_framebuffer_object/fbo-drawbuffers-none glclear
> > > spec/ext_framebuffer_multisample/blit-multiple-render-targets 0
> > >
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 97 +++++++++++++++++++++----
> > >  1 file changed, 84 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > > index 7bf52f0..9e6711e 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > > @@ -427,6 +427,74 @@ use_rectlist(struct brw_context *brw, bool enable)
> > >     brw->ctx.NewDriverState |= BRW_NEW_FRAGMENT_PROGRAM;
> > >  }
> > >  
> > > +/**
> > > + * Individually fast clear each color buffer attachment. On previous gens this
> > > + * isn't required. The motivation for this comes from one line (which seems to
> > > + * be specific to SKL+). The list item is in section titled _MCS Buffer for
> > > + * Render Target(s)_
> > > + *
> > > + *   "Since only one RT is bound with a clear pass, only one RT can be cleared
> > > + *   at a time. To clear multiple RTs, multiple clear passes are required."
> > 
> > This sentence also appears in the HSW PRM so it seems a bit odd if it's
> > only causing problems on SKL. I guess if we get Piglit regressions
> > without it then it makes sense to have the patch. It might be worth just
> > double checking whether this patch is completely necessary. The wording
> > in the commit message seems a little unsure.
> 
> The spec seems to be missing something as the section discussing "Render
> Target Fast Clear" seems to suggest the opposite:
> 
> "The render target(s) is/are bound as they normally would be, with the MCS
>  surface defined in SURFACE_STATE."

I am aware of all this. Neil, yes it is completely necessary for piglit (I don't
know if anything in the real world does this or not).

You are both asking to me to provide something which may be impossible, an
explanation of why the docs and/or hardware are behaving this way. Let me
respond in kind, please provide an alternate patch which fixes:
spec/ext_framebuffer_multisample/blit-multiple-render-targets 0
spec/arb_framebuffer_object/fbo-drawbuffers-none glclear (all subtests)

FWIW Topi, it's also contradicted in 3DSTATE_PS definition.