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

[Ben Widawsky]

On Wed, Oct 14, 2015 at 08:04:48PM +0300, Pohjolainen, Topi wrote:
> On Wed, Oct 14, 2015 at 09:54:43AM -0700, Ben Widawsky wrote:
> > 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.
> 
> You misunderstood me I think, I'm not questioning your patch or your
> interpretation, or asking you to provide some information that just isn't
> there in the spec. We talked about this quite a bit. I'm just saying that I
> feel that something is missing in the spec.

I've been unable to get any issues addressed in this part of the spec. I have 4
open bugs filed against the doc on this section alone.