[Mesa-dev] [PATCH 2/7] i965: Add lossless compression to surface format table

[Chad Versace]

On Wed 11 Nov 2015, Ben Widawsky wrote:
> Background: Prior to Skylake and since Ivybridge Intel hardware has had the
> ability to use a MCS (Multisample Control Surface) as auxiliary data in
> "compression" operations on the surface. This reduces memory bandwidth.  This
> hardware was either used for MSAA compression, and fast clear operations.  On
> Gen8, a similar mechanism exists to allow the hiz buffer to be sampled from, and
> therefore this feature is sometimes referred to more generally as "AUX buffers".
> 
> Skylake adds the ability to have the display engine directly source compressed
> surfaces on top of the ability to sample from them. Inference dictates that
> enabling this display features adding a restriction to the formats which could
> actually be compressed. The current set of surfaces seems to be a subset as
> compared to previous gens (see the next patch). Also, if I had to guess I would
> guess that future gens add support for more surface formats. To make handling
> this a bit easier to read, and more future proof, the support for this is moved
> into the surface formats table.
> 
> Along with the modifications to the table, a helper function is also provided to
> determine if a surface is CCS compatible.  Because fast clears are currently
> disabled on SKL, we can plumb the helper all the way through here, and not
> actually have anything break.
> 
> The logic in the table works a bit differently than the other columns in the
> table and therefore deserves a small mention. For most other features, the GEN
> which began implementing it is set, and it is assumed future gens also support
> this. For this feature, GEN9 actually eliminates support for certain formats. We
> could use this column to determine support for the similar feature on older
> generation hardware. Aside from that being an error prone task which is
> unrelated to enabling this on GEN9, it becomes somewhat tricky to implement
> because of the fact that surface format support diminishes. You'd probably want
> another column to cleanly implement it.
> 
> Requested-by: Chad Versace <chad.versace at intel.com>
> Requested-by: Neil Roberts <neil at linux.intel.com>
> Signed-off-by: Ben Widawsky <benjamin.widawsk at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_context.h         |   2 +
>  src/mesa/drivers/dri/i965/brw_surface_formats.c | 527 +++++++++++++-----------
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c   |   7 +
>  3 files changed, 285 insertions(+), 251 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index 4b2db61..6284c18 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1465,6 +1465,8 @@ void brw_upload_image_surfaces(struct brw_context *brw,
>  /* brw_surface_formats.c */
>  bool brw_render_target_supported(struct brw_context *brw,
>                                   struct gl_renderbuffer *rb);
> +bool brw_losslessly_compressible_format(struct brw_context *brw,
> +                                        uint32_t brw_format);
>  uint32_t brw_depth_format(struct brw_context *brw, mesa_format format);
>  mesa_format brw_lower_mesa_image_format(const struct brw_device_info *devinfo,
>                                          mesa_format format);
> diff --git a/src/mesa/drivers/dri/i965/brw_surface_formats.c b/src/mesa/drivers/dri/i965/brw_surface_formats.c
> index 97fff60..a7cdc13 100644
> --- a/src/mesa/drivers/dri/i965/brw_surface_formats.c
> +++ b/src/mesa/drivers/dri/i965/brw_surface_formats.c
> @@ -39,14 +39,15 @@ struct surface_format_info {
>     int input_vb;
>     int streamed_output_vb;
>     int color_processing;
> +   int lossless_compression_support;

There's no need to place "support" in the name. Every struct member is
a "support" member.

>     const char *name;
>  };
>  
>  /* This macro allows us to write the table almost as it appears in the PRM,
>   * while restructuring it to turn it into the C code we want.
>   */
> -#define SF(sampl, filt, shad, ck, rt, ab, vb, so, color, sf) \
> -   [BRW_SURFACEFORMAT_##sf] = { true, sampl, filt, shad, ck, rt, ab, vb, so, color, #sf},
> +#define SF(sampl, filt, shad, ck, rt, ab, vb, so, color, ccs, sf) \
> +   [BRW_SURFACEFORMAT_##sf] = { true, sampl, filt, shad, ck, rt, ab, vb, so, color, ccs, #sf},
>  
>  #define Y 0
>  #define x 999
> @@ -74,6 +75,7 @@ struct surface_format_info {
>   * VB    - Input Vertex Buffer
>   * SO    - Steamed Output Vertex Buffers (transform feedback)
>   * color - Color Processing
> + * ccs   - Lossless Compression Support (gen9+ only)

Please don't name the new column "ccs". That conflates two closely
related but distinct hardware features.

According the internal hw docs, Broadwell supports AUX_CCS_D (legacy
"clear compression", my personal term), but Broadwell most definitely
does not support AUX_CCS_E (lossless color compression). In other words,
Broadwell supports CCS but not lossless color compression.

>   * sf    - Surface Format
>   *
>   * See page 88 of the Sandybridge PRM VOL4_Part1 PDF.
> @@ -84,257 +86,258 @@ struct surface_format_info {
>   * - VOL2_Part1 section 2.5.11 Format Conversion (vertex fetch).
>   * - VOL4_Part1 section 2.12.2.1.2 Sampler Output Channel Mapping.
>   * - VOL4_Part1 section 3.9.11 Render Target Write.
> + * - Render Target Surface Types [SKL+]
>   */
>  const struct surface_format_info surface_formats[] = {

[...]

> +   SF( x,  x,  x,  x,  x,  x,  Y,  x,  x,    x,   R32_UNORM)
> +   SF( x,  x,  x,  x,  x,  x,  Y,  x,  x,    x,   R32_SNORM)
> +/* smpl filt shad CK  RT  AB  VB  SO  color ccs ccs */
                                                   ^^^^
Stutter of "ccs".

> +/*
> + * True if the underlying hardware format can support lossless color compression
> + * (MSAA compression, or Color Compression for render targets).

The comment needs fixing.  "MSAA compression" is not "lossless color
compression". Skylake supports lossless color compression is only for
single-sample surfaces.

I think the comment should end with a period after "lossless color
compression". That comment would suffice and be correct.

> + */
> +bool
> +brw_losslessly_compressible_format(struct brw_context *brw,
> +                                   uint32_t brw_format)
> +{
> +   const struct surface_format_info *sinfo;
> +   int gen = brw->gen * 10;
> +
> +   /* Prior to gen9, there are no restrictions */
> +   if (brw->gen < 9)
> +      return true;

The pre-gen9 check is wrong. It should return false. Lossless color
compression does not exist before gen9.

Anyway, after you fill the lossless_compression column in the
surface_formats table in the next patch, the 'if' statement above can be
completely removed. The 'if' statement below suffices.

> +
> +   sinfo = &surface_formats[brw_format];
> +   if (gen >= sinfo->lossless_compression_support)
> +      return true;

> +
> +   return false;
> +}