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

[Pohjolainen, Topi]

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?

>  #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:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

> +   } else {
> +      static const char *surface_aux_mode[] = { "AUX_NONE", "AUX_MCS",
> +                                                "AUX_APPEND", "AUX_HIZ",
> +                                                "RSVD", "RSVD"};
> +      aux_str = ralloc_asprintf(NULL, "%s", surface_aux_mode[aux_mode]);
> +   }
>  
>     batch_out(brw, "SURF'", offset, 0, "%s %s %s VALIGN%d HALIGN%d %s\n",
>               get_965_surfacetype(GET_FIELD(surf[0], BRW_SURFACE_TYPE)),
> @@ -288,7 +295,7 @@ static void dump_gen8_surface_state(struct brw_context *brw, uint32_t offset)
>     batch_out(brw, name, offset, 2, "%dx%d [%s]\n",
>               GET_FIELD(surf[2], GEN7_SURFACE_WIDTH) + 1,
>               GET_FIELD(surf[2], GEN7_SURFACE_HEIGHT) + 1,
> -             surface_aux_mode[surf[6] & INTEL_MASK(2, 0)]);
> +             aux_str);
>     batch_out(brw, name, offset, 3, "%d slices (depth), pitch: %d\n",
>               GET_FIELD(surf[3], BRW_SURFACE_DEPTH) + 1,
>               (surf[3] & INTEL_MASK(17, 0)) + 1);
> @@ -303,14 +310,21 @@ static void dump_gen8_surface_state(struct brw_context *brw, uint32_t offset)
>     batch_out(brw, name, offset, 6, "AUX pitch: %d qpitch: %d\n",
>               GET_FIELD(surf[6], GEN8_SURFACE_AUX_QPITCH) << 2,
>               GET_FIELD(surf[6], GEN8_SURFACE_AUX_PITCH) << 2);
> -   batch_out(brw, name, offset, 7, "Clear color: %c%c%c%c\n",
> -             GET_BITS(surf[7], 31, 31) ? 'R' : '-',
> -             GET_BITS(surf[7], 30, 30) ? 'G' : '-',
> -             GET_BITS(surf[7], 29, 29) ? 'B' : '-',
> -             GET_BITS(surf[7], 28, 28) ? 'A' : '-');
> +   if (brw->gen >= 9) {
> +      batch_out(brw, name, offset, 7, "Clear color: R(%x)G(%x)B(%x)A(%x)\n",
> +                surf[12], surf[13], surf[14], surf[15]);
> +   } else {
> +      batch_out(brw, name, offset, 7, "Clear color: %c%c%c%c\n",
> +                GET_BITS(surf[7], 31, 31) ? 'R' : '-',
> +                GET_BITS(surf[7], 30, 30) ? 'G' : '-',
> +                GET_BITS(surf[7], 29, 29) ? 'B' : '-',
> +                GET_BITS(surf[7], 28, 28) ? 'A' : '-');
> +   }
>  
>     for (int i = 8; i < 12; i++)
>        batch_out(brw, name, offset, i, "0x%08x\n", surf[i]);
> +
> +   ralloc_free((void *)aux_str);
>  }
>  
>  static void
> -- 
> 2.4.1
>