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.pohjolai...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
