[Mesa-dev] [PATCH] [v2] i965: Add lossless compression to surface format table

[Ben Widawsky]

On Wed, Nov 18, 2015 at 10:28:27AM -0800, Chad Versace wrote:
> On Tue 17 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
> ----------------------------^^^
> Should say "CCS_E".

I don't understand why you are insisting on calling everything CCS_E.
Admittedly, the docs aren't crystal clear here, but here is how I understand it.
For single sample render compression - the thing we're not using yet, you use
CCS_E. For single sample fast clears, you use CCS_D (with the restriction that
such surfaces must be supported for render compression when using the sampler).
For multisample compression, I don't understand the difference. More
confusingly, the bit we're actually setting in the surface state is CCS_D, so
the last change you asked me to make (renaming ccs to ccs_e in the table) didn't
make sense to me - but for the sake of making progress, I just changed it to
make you happy. However, since you're now asking for another version without
leaving an R-B, I really must know what is going on in your head. Can you prove
to me from the docs why it's not accurate to say CCS, or if you must CCS_*, and
why it is MORE accurate to say CCS_E? I will happily change it for you if you
give me a convincing answer.

> 
> > 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.
> 
> Does the above paragraph still apply to the table's ccs_e column?
> I understand your patch series, the ccs_e column behaves identically to
> all other columns:
> 
> 	feature_is_supported == (10 * gen >= table[format].feature)
> 
> The patch's diff looks good to me. My only remaining questions/issues
> with the patch are the ones stated in this message.

-- 
Ben Widawsky, Intel Open Source Technology Center