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

["Chery, Nanley G" <nanley.g.chery@intel.com>]

> -----Original Message-----
> From: Deak, Imre <imre.deak@intel.com>
> Sent: Friday, March 18, 2022 10:40 AM
> To: Chery, Nanley G <nanley.g.chery@intel.com>
> Cc: juhapekka.heikkila@gmail.com; Nanley Chery <nanleychery@gmail.com>; C, Ramalingam <ramalingam.c@intel.com>; intel-gfx <intel-
> gfx@lists.freedesktop.org>; Auld, Matthew <matthew.auld@intel.com>; dri-devel <dri-devel@lists.freedesktop.org>
> Subject: Re: [Intel-gfx] [PATCH v5 15/19] drm/i915/dg2: Add DG2 unified compression
> 
> On Thu, Feb 17, 2022 at 05:15:15PM +0000, Chery, Nanley G wrote:
> > > >> [...]
> > > >> --- a/include/uapi/drm/drm_fourcc.h
> > > >> +++ b/include/uapi/drm/drm_fourcc.h
> > > >> @@ -583,6 +583,28 @@ extern "C" {
> > > >>    */
> > > >>   #define I915_FORMAT_MOD_4_TILED         fourcc_mod_code(INTEL, 9)
> > > >>
> > > >> +/*
> > > >> + * Intel color control surfaces (CCS) for DG2 render compression.
> > > >> + *
> > > >> + * DG2 uses a new compression format for render compression. The general
> > > >> + * layout is the same as I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > > >> + * but a new hashing/compression algorithm is used, so a fresh modifier must
> > > >> + * be associated with buffers of this type. Render compression uses 128 byte
> > > >> + * compression blocks.
> > > >
> > > > I think I've seen a way to configure the compression block size on TGL
> > > > at least. I can't find the spec text for that at the moment though...
> > > > Could we omit these mentions?
> > >
> > > Not sure why general possibility of changing compression block size is relevant?
> > > All hw features can be changed but this defines how this modifier is being
> > > implemented.
> >
> > I was concerned about compatibility between the different modes, but I've
> > looked into the restrictions here and don't see any problems with this.
> >
> > > Say you take I915_FORMAT_MOD_4_TILED_DG2_RC_CCS framebuffer including
> > > control surface and copy it out, then come back and restore framebuffer with
> > > same information. It is expected to be valid?
> >
> > > /Juha-Pekka
> > >
> > > >> + */
> > > >> +#define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS fourcc_mod_code(INTEL, 10)
> > > >> +
> > > >
> > > > How about something like:
> > > >
> > > > The main surface is Tile 4 and at plane index 0. The CCS plane is
> > > > hidden from userspace. The main surface pitch is required to be a
> > > > multiple of four Tile 4 widths. The CCS is configured with the render
> > > > compression format associated with the main surface format.
> >
> > Actually, let's omit the last sentence. CCS has always been affected
> > by the main surface format, so I don't think there's a need to mention it
> > specifically for the DG2 modifier.
> >
> > We do need to mention the 4-tile-wide pitch requirement though.
> 
> Agreed, the DG2 layout of planes and the tile format used - both
> different wrt. the GEN12_RC_CCS format - should be described here.
> 
> > -Nanley
> >
> > > > ....I think the CCS is technically accessible via the blitter engine,
> > > > so the part about the plane being "hidden" may need some tweaking.
> 
> Maybe outside of the GEM object? 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

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

> > > > -Nanley
> > > >
> > > >> +/*
> > > >> + * Intel color control surfaces (CCS) for DG2 media compression.
> > > >> + *
> > > >> + * DG2 uses a new compression format for media compression. The
> > > >> +general
> > > >> + * layout is the same as I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS,
> > > >> + * but a new hashing/compression algorithm is used, so a fresh
> > > >> +modifier must
> > > >> + * be associated with buffers of this type. Media compression uses
> > > >> +256 byte
> > > >> + * compression blocks.
> > > >> + */
> > > >> +#define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS
> > > fourcc_mod_code(INTEL,
> > > >> +11)
> > > >> +
> > > >>   /*
> > > >>    * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
> > > >>    *
> > > >> --
> > > >> 2.20.1
> > > >>
> >