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

[Ben Widawsky]

On Wed, Nov 18, 2015 at 11:10:12AM +0200, Pohjolainen, Topi wrote:
> On Tue, Nov 17, 2015 at 05:30:06PM -0800, 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
> 
> This says:
> 
>     ... either ... , and ... 
> 
> should it have been
> 
>     ... either ... or ... 
> 

The latter, thanks.

> 
> All in all, I really appreciate the thorough explanation here in this commit,
> just had to check. I know I'm late with my comments, so bare with me.
> 
> > 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
> 
> s/adding/adds/ ?

Yes.

> 
> > 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
> 
> I have difficulty here also: the sentence compares table to other columns in
> the table ("... logic in the table ... than other columns...").
> 
> Did you mean to say that a particular _column_ in the table behaves
> differently than the others?

Yeah, I need to fix this, It's not actually true with the last change that Chad
requested me to make. It has no distinction from other columns in the table now.

> 
> > 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
> 
> And here you refer to the newly added column? Comparing the contents of that
> column to the supported render targets (column RT) gives you the delta between
> gen9 and 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.
> 
> And this is what you did, right?

No. What I meant by this is you could have a field which I'll call lossless
compression (there is debate about whether that's accurate, but to me it's a
good name) and it could apply for all generations which support it. For example,
RGBA8 would be 70 (because it has supported this since gen7) and L16_UNORM would
be x (because it's not 4, 8, or 16 cpp).

It turns out that doesn't work so easily because there are formats which work on
gen7, 7.5 and 8, but do not work on gen9. The table doesn't have range support
built it, so you'd need two columns to do it right, the legacy compression, and
the newer version of the compression.

I'll be dropping this whole part of the commit message to address the last
change in the patch, since it is causing confusion.

> 
> Again, sorry for the 20 questions.
> 

That's fine.

> Otherwise the patch makes sense to me:
> 
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> 
> (You probably want Chad and Neil to give their consent also).
> 

Heh, Neil basically said the same thing. It's a game of hot potato where Chad
always ends up with the potato. I fixed all the mistakes you spotted other than
the chunk I dropped. Thanks.

[snip]