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: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Sent: Tuesday, February 15, 2022 6:54 AM
> To: Nanley Chery <nanleychery@gmail.com>; C, Ramalingam
> <ramalingam.c@intel.com>
> Cc: intel-gfx <intel-gfx@lists.freedesktop.org>; Chery, Nanley G
> <nanley.g.chery@intel.com>; 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 12.2.2022 3.17, Nanley Chery wrote:
> > On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C <ramalingam.c@intel.com>
> wrote:
> >>
> >> From: Matt Roper <matthew.d.roper@intel.com>
> >>
> >> 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@intel.com>
> >> cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com> (v2)
> >> cc: Anshuman Gupta <anshuman.gupta@intel.com>
> >> Signed-off-by: Juha-Pekka Heikkilä <juha-pekka.heikkila@intel.com>
> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> >> ---
> >>   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
> >>