[Mesa-dev] [PATCH 6/8] [v2] i965: Add Gen9 surface state decoding

[Pohjolainen, Topi]

On Mon, May 18, 2015 at 10:39:58AM -0700, Ben Widawsky wrote:
> On Mon, May 18, 2015 at 11:02:32AM +0300, Pohjolainen, Topi wrote:
> > On Fri, May 15, 2015 at 10:06:22PM -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.
> > > 
> > > v2: Redo the string for the aux buffer type to address compressed variants.
> > > 
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > Cc: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_defines.h    |  2 ++
> > >  src/mesa/drivers/dri/i965/brw_state_dump.c | 44 ++++++++++++++++++++----------
> > >  2 files changed, 31 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> > > index 8fd5a49..3a5cb81 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > > @@ -608,6 +608,8 @@
> > >  #define GEN8_SURFACE_AUX_MODE_HIZ               3
> > >  
> > >  /* Surface state DW7 */
> > > +#define GEN9_SURFACE_RT_COMPRESSION_SHIFT       30
> > > +#define GEN9_SURFACE_RT_COMPRESSION_MASK        INTEL_MASK(31, 31)
> > 
> > Your shift is 30 and mask is 31:31. Bit 31 is memory compression mode while
> > bit 30 is memory compression enable - I guess you meant to use the latter,
> > so the shift is correct while the mask is not?
> > 
> 
> Oops, thanks.
> 
> > >  #define GEN7_SURFACE_CLEAR_COLOR_SHIFT		28
> > >  #define GEN7_SURFACE_SCS_R_SHIFT                25
> > >  #define GEN7_SURFACE_SCS_R_MASK                 INTEL_MASK(27, 25)
> > > diff --git a/src/mesa/drivers/dri/i965/brw_state_dump.c b/src/mesa/drivers/dri/i965/brw_state_dump.c
> > > index e0245b0..232d0c1 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_state_dump.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_state_dump.c
> > > @@ -66,15 +66,6 @@ static const char *surface_tiling[] = {
> > >     "Y-tiled"
> > >  };
> > >  
> > > -static const char *surface_aux_mode[] = {
> > > -   "AUX_NONE",
> > > -   "AUX_MCS",
> > > -   "AUX_APPEND",
> > > -   "AUX_HIZ",
> > > -   "RSVD",
> > > -   "RSVD"
> > > -};
> > > -
> > >  static void
> > >  batch_out(struct brw_context *brw, const char *name, uint32_t offset,
> > >  	  int index, char *fmt, ...) PRINTFLIKE(5, 6);
> > > @@ -272,6 +263,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[6] & INTEL_MASK(2, 0);
> > > +   const char *aux_str;
> > > +
> > > +   if (brw->gen >= 9 && (aux_mode == 1 || aux_mode == 5)) {
> > > +      bool msrt = GET_BITS(surf[4], 5, 3) > 0;
> > > +      bool compression = GET_FIELD(surf[7], GEN9_SURFACE_RT_COMPRESSION) == 1;
> > > +      aux_str = ralloc_asprintf(NULL, "AUX_CCS_%c (%s, MULTISAMPLE_COUNT%c1)",
> > > +                                (aux_mode == 1) ? 'D' : 'E',
> > > +                                compression ? "Compressed RT" : "Uncompressed",
> > > +                                msrt ? '>' : '=');
> > 
> > If you shift one (1 << msrt) you get the actual number of samples. Should we
> > print that instead? And may be s/MULTISAMPLE_COUNT/SAMPLES/ to shrink the
> > output a little.
> > 
> > Otherwise this looks good to me:
> > 
> 
> I wanted to match the docs as much as possible in this case, since as you
> mentioned in the other email, the docs are somewhat vague where needed. Are you
> okay with me leaving it as is (the multisample info is elsewhere)?

Quite okay :)