Re: [Intel-gfx] [PATCH v5 15/19] drm/i915/dg2: Add DG2 unified compression

[Imre Deak <imre.deak@intel.com>]

On Thu, Mar 24, 2022 at 01:40:37AM +0200, Chery, Nanley G wrote:
> > [...]
> > Capturing all the above would you be ok with the following?:
> > 
> > Intel color control surfaces (CCS) for DG2 render compression.
> > 
> > The main surface is Tile 4 and at plane index 0. The CCS data is stored
> > outside of the GEM object in a reserved memory area dedicated for the
> > storage of the CCS data from all GEM objects. The main surface pitch is
> > required to be a multiple of four Tile 4 widths.
> > 
> > 
> > Intel color control surfaces (CCS) for DG2 media compression.
> > 
> > The main surface is Tile 4 and at plane index 0. For semi-planar formats
> > like NV12, the UV plane is Tile 4 at plane index 1. The CCS data both for
> > the main and semi-planar UV planes are stored outside of the GEM object
> 
> This kind of implies that the Y plane is the main surface, but it's not more
> "main" than the UV plane right? Seems like we should specifically call out the
> Y plane for clarity. Maybe something like:
> 
> For semi-planar formats like NV12, the Y and UV planes are Tile 4 and are 
> located at plane indices 0 and 1, respectively. The CCS for all planes are stored 
> outside of the GEM object

Ok, makes sense.

> > in a reserved memory area dedicated for the storage of the CCS data from
> > all GEM objects. The main surface pitch is required to be a multiple of
> > four Tile 4 widths.
> 
> Looks good to me. Main suggestion I have here is to substitute 
> "from all GEM objects" with "for all compressible GEM objects".

"for all RC/RC_CC/MC CCS compressible GEM objects" would be more
precise, in case there are other ways to compress data. Either way looks
ok to me.

> Happy to look at further revisions, but with that change at least,
> Acked-by: Nanley Chery <nanley.g.chery@intel.com>

Thanks. 

--Imre