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

[Ben Widawsky]

On Fri, Nov 20, 2015 at 11:02:43AM -0800, Chad Versace wrote:
> On Wed 18 Nov 2015, Ben Widawsky wrote:
> > 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.
> 
> We both misunderstand eachother frequently on this topic. And the poor
> hardware documentation doesn't make matters better :/
> 
> Next time we're in the same room, let's try to explain to each other our
> own understanding of the aux surface. That my unravel the subtle (but
> significant) differences between us. I think our understandings mostly
> agree on SKL but diverge for older gens.
> 
> I retract my "CCS_E" comment above. See my other reply to this patch for
> the r-b.

Even though it's airing some dirty laundry, I'm keeping the list on the Cc
because I think it clarifies things and it might benefit Topi and Tapani who
will be doing the render compression work...

I was talking to Topi this morning and he helped me see something which you may
or may not be thinking that helped me understand why you made the request to
name is CCS_E. Summing up my conversation with him, the last round of changes
you had me make (prior to this one) seemed extremely trivial and benign
to me at the time. Those changes were mostly renames, but it had a sweeping
impact on how the patches "worked." With the last round of changes, we are
indeed tracking CCS_E support, which was never my intention and therefore the
big source of confusion for me. With that, I do understand your request to name
things CCS_E even though it's not the direction I wanted to go in the first
place - it's too late now and I didn't put up the fight for the thing that
mattered which turns out to be the simple renaming of the column in
surface_formats[] from ccs to ccs_e.

Thank you for the review. Even with my new found understand, since we're at this
stage, I'd really like to just push things as is. I feel like if I dig any more
it will just unearth more issues with wording or other similar things which
really don't matter to the actual quality of the driver we are shipping.