[Mesa-dev] [PATCH 5/7] i965: Add Gen9 surface state decoding

[Pohjolainen, Topi]

On Fri, May 15, 2015 at 08:26:33PM -0700, Ben Widawsky wrote:
> On Fri, May 15, 2015 at 08:22:29PM -0700, Ben Widawsky wrote:
> > On Fri, Apr 24, 2015 at 09:05:44PM +0300, Pohjolainen, Topi wrote:
> > > On Thu, Apr 23, 2015 at 04:50:02PM -0700, Ben Widawsky wrote:
> > > > Gen9 surface state is very similar to the previous generation. The important
> > > > changes here are aux mode, and the way clear colors work.
> > > > 
> > > > NOTE: There are some things intentionally left out of this decoding.
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > > ---
> > > >  src/mesa/drivers/dri/i965/brw_state_dump.c | 36 ++++++++++++++++++++++++------
> > > >  1 file changed, 29 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/src/mesa/drivers/dri/i965/brw_state_dump.c b/src/mesa/drivers/dri/i965/brw_state_dump.c
> > > > index 642bdc8..60e6b05 100644
> > > > --- a/src/mesa/drivers/dri/i965/brw_state_dump.c
> > > > +++ b/src/mesa/drivers/dri/i965/brw_state_dump.c
> > > > @@ -491,6 +491,22 @@ static void dump_gen8_surface_state(struct brw_context *brw, uint32_t offset)
> > > >  {
> > > >     const char *name = "SURF";
> > > >     uint32_t *surf = brw->batch.bo->virtual + offset;
> > > > +   int aux_mode = surf[7] & INTEL_MASK(2, 0);
> > > 
> > > Same question as in the previous patch, surf[6] ?
> > > 
> > > > +   const char *aux_str;
> > > > +
> > > > +   if (brw->gen >= 9) {
> > > > +      bool msrt = GET_BITS(surf[4], 5, 3) > 0;
> > > > +      switch (aux_mode) {
> > > > +      case 5:
> > > > +         aux_str = msrt ? "AUX_CCS_E [MCS]" : "AUX_CCS_D [MCS]";
> > > 
> > > The way I read the spec, I would have written this as:
> > > 
> > >             aux_str = msrt ? "AUX_CCS_E [MCS]" : "AUX_CCS_E [CCS]";
> > > 
> > > Ande the one below:
> > > 
> > >             aux_str = msrt ? "AUX_CCS_D [MCS]" : "AUX_CCS_D [CCS]";
> > > 
> > > But maybe I'm misreading.
> > > 
> > 
> > You are right, sorta... First, this was definitely a copy and paste error. I
> > believe there are actually 3 things to program here:
> > 
> > if (render_compression || something else)?
> > 	CCS
> > else if (msrt)
> > 	MCS/MSAA
> > else
> > 	MCS/1x (fast clear)
> > 
> 
> Okay, I suck today with quickly changing my mind after sending a mail... So, I
> was regurgitating what Chad told me earlier today, but I now think he misspoke.
> There is no more MCS/1x surface, it's just the CCS with compression disabled. So
> the 3 different options remain, but the wording is a bit different, maybe this:
> 
> if (render_compression || something else)?
> 	CCS
> else if (msrt)
>  	MCS (MSAA)
>  else
>  	CCS (fast clear)
> 
> > What do you think?

I think you got it right in your revision. I think those a dozen odd notes in
bspec are hard to capture explicitly in the output and the reader needs to
understand quite a bit of the surfaces types anyway for it all to make sense.

So I think it suffices to have all the necessary bits of information there,
and you have :)