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

[Ben Widawsky]

On Wed, Nov 18, 2015 at 03:50:32PM -0800, Ben Widawsky wrote:
> 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.

I've added a spec reference here since I didn't spot it before. Just FYI since
you gave me the r-b already:

'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 adds a restriction to the formats which could
actually be compressed. This is backed up by a blurb in the AUX_CCS_D section
from the RENDER_SURFACE_STATE: "In addition, if the surface is bound to the
sampling engine, Surface Format must be supported for Render Target Compression
for surfaces bound to the sampling engine."' ...

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

-- 
Ben Widawsky, Intel Open Source Technology Center