> -----Original Message-----
> From: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx>
> Sent: Tuesday, February 15, 2022 6:54 AM
> To: Nanley Chery <nanleychery@xxxxxxxxx>; C, Ramalingam
> <ramalingam.c@xxxxxxxxx>
> Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; Chery, Nanley G
> <nanley.g.chery@xxxxxxxxx>; Auld, Matthew <matthew.auld@xxxxxxxxx>; dri-
> devel <dri-devel@xxxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v5 15/19] drm/i915/dg2: Add DG2 unified
> compression
>
> On 12.2.2022 3.17, Nanley Chery wrote:
> > On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C <ramalingam.c@xxxxxxxxx>
> wrote:
> >>
> >> From: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >>
> >> DG2 unifies render compression and media compression into a single
> >> format for the first time. The programming and buffer layout is
> >> supposed to match compression on older gen12 platforms, but the
> >> actual compression algorithm is different from any previous platform;
> >> as such, we need a new framebuffer modifier to represent buffers in
> >> this format, but otherwise we can re-use the existing gen12 compression
> driver logic.
> >>
> >> v2:
> >> Display version fix [Imre]
> >>
> >> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >> cc: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> >> Signed-off-by: Mika Kahola <mika.kahola@xxxxxxxxx> (v2)
> >> cc: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> >> Signed-off-by: Juha-Pekka Heikkilä <juha-pekka.heikkila@xxxxxxxxx>
> >> Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_fb.c | 13 ++++++++++
> >> .../drm/i915/display/skl_universal_plane.c | 26 ++++++++++++++++---
> >> include/uapi/drm/drm_fourcc.h | 22 ++++++++++++++++
> >> 3 files changed, 57 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> >> b/drivers/gpu/drm/i915/display/intel_fb.c
> >> index 94c57facbb46..4d4d01963f15 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> >> @@ -141,6 +141,14 @@ struct intel_modifier_desc {
> >>
> >> static const struct intel_modifier_desc intel_modifiers[] = {
> >> {
> >> + .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS,
> >> + .display_ver = { 13, 13 },
> >> + .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> INTEL_PLANE_CAP_CCS_MC,
> >> + }, {
> >> + .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> >> + .display_ver = { 13, 13 },
> >> + .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> INTEL_PLANE_CAP_CCS_RC,
> >> + }, {
> >> .modifier = I915_FORMAT_MOD_4_TILED,
> >> .display_ver = { 13, 13 },
> >> .plane_caps = INTEL_PLANE_CAP_TILING_4, @@ -550,6
> >> +558,8 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int
> color_plane)
> >> return 128;
> >> else
> >> return 512;
> >> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> >> + case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> >> case I915_FORMAT_MOD_4_TILED:
> >> /*
> >> * Each 4K tile consists of 64B(8*8) subtiles, with
> >> @@ -752,6 +762,9 @@ unsigned int intel_surf_alignment(const struct
> drm_framebuffer *fb,
> >> case I915_FORMAT_MOD_4_TILED:
> >> case I915_FORMAT_MOD_Yf_TILED:
> >> return 1 * 1024 * 1024;
> >> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> >> + case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> >> + return 16 * 1024;
> >> default:
> >> MISSING_CASE(fb->modifier);
> >> return 0;
> >> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> index 5299dfe68802..c38ae0876c15 100644
> >> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> @@ -764,6 +764,14 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> >> return PLANE_CTL_TILED_Y;
> >> case I915_FORMAT_MOD_4_TILED:
> >> return PLANE_CTL_TILED_4;
> >> + case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> >> + return PLANE_CTL_TILED_4 |
> >> + PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
> >> + PLANE_CTL_CLEAR_COLOR_DISABLE;
> >> + case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> >> + return PLANE_CTL_TILED_4 |
> >> + PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE |
> >> + PLANE_CTL_CLEAR_COLOR_DISABLE;
> >> case I915_FORMAT_MOD_Y_TILED_CCS:
> >> case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> >> return PLANE_CTL_TILED_Y |
> >> PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> >> @@ -2094,6 +2102,10 @@ static bool gen12_plane_has_mc_ccs(struct
> drm_i915_private *i915,
> >> if (IS_ADLP_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
> >> return false;
> >>
> >> + /* Wa_14013215631 */
> >> + if (IS_DG2_DISPLAY_STEP(i915, STEP_A0, STEP_C0))
> >> + return false;
> >> +
> >> return plane_id < PLANE_SPRITE4;
> >> }
> >>
> >> @@ -2335,9 +2347,10 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
> >> case PLANE_CTL_TILED_Y:
> >> plane_config->tiling = I915_TILING_Y;
> >> if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> >> - fb->modifier = DISPLAY_VER(dev_priv) >= 12 ?
> >> - I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS :
> >> - I915_FORMAT_MOD_Y_TILED_CCS;
> >> + if (DISPLAY_VER(dev_priv) >= 12)
> >> + fb->modifier =
> I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS;
> >> + else
> >> + fb->modifier =
> >> + I915_FORMAT_MOD_Y_TILED_CCS;
> >> else if (val & PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)
> >> fb->modifier = I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS;
> >> else
> >> @@ -2345,7 +2358,12 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
> >> break;
> >> case PLANE_CTL_TILED_YF: /* aka PLANE_CTL_TILED_4 on XE_LPD+ */
> >> if (HAS_4TILE(dev_priv)) {
> >> - fb->modifier = I915_FORMAT_MOD_4_TILED;
> >> + if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> >> + fb->modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS;
> >> + else if (val & PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE)
> >> + fb->modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS;
> >> + else
> >> + fb->modifier =
> >> + I915_FORMAT_MOD_4_TILED;
> >> } else {
> >> if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> >> fb->modifier =
> >> I915_FORMAT_MOD_Yf_TILED_CCS; diff --git
> >> a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index
> >> b73fe6797fc3..b8fb7b44c03c 100644
> >> --- 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.
-Nanley
> > ....I think the CCS is technically accessible via the blitter engine,
> > so the part about the plane being "hidden" may need some tweaking.
> >
> >
> > -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
> >>
