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

[Ben Widawsky]

On Fri, Nov 13, 2015 at 12:29:47PM -0800, Chad Versace wrote:
> 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.
> 

Fine.

> >     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.
> 

I am sorry you found it confusing. Apparently you are confused by all the code I
write... what would you like me to name it? I'm done with trying to predict how
you would have written the patches. Going for on this patch series, just tell me
what you prefer so we can avoid the excessive back and forth.

> >   * 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".

This will get fixed with whatever rename you choose.

> 
> > +/*
> > + * 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.
> 

Okay.

> > + */
> > +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.
> 

Essentially you're asking me to invert the logic in the table iirc. That should
be fine, there was some reason I didn't do this in the first place, but I can't
think of what it was now. I'll make the change after you respond to the above.

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