Re: [Intel-gfx] [PATCH v5 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color

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

> -----Original Message-----
> From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Sent: Tuesday, February 15, 2022 6:56 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 16/19] uapi/drm/dg2: Introduce format
> modifier for DG2 clear color
> 
> On 12.2.2022 3.19, Nanley Chery wrote:
> > On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C <ramalingam.c@intel.com>
> wrote:
> >>
> >> From: Mika Kahola <mika.kahola@intel.com>
> >>
> >> DG2 clear color render compression uses Tile4 layout. Therefore, we
> >> need to define a new format modifier for uAPI to support clear color
> rendering.
> >>
> >> v2:
> >>    Display version is fixed. [Imre]
> >>    KDoc is enhanced for cc modifier. [Nanley & Lionel]
> >>
> >> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> 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            |  8 ++++++++
> >>   drivers/gpu/drm/i915/display/skl_universal_plane.c |  9 ++++++++-
> >>   include/uapi/drm/drm_fourcc.h                      | 10 ++++++++++
> >>   3 files changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> >> b/drivers/gpu/drm/i915/display/intel_fb.c
> >> index 4d4d01963f15..3df6ef5ffec5 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> >> @@ -144,6 +144,12 @@ 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_CC,
> >> +               .display_ver = { 13, 13 },
> >> +               .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> >> + INTEL_PLANE_CAP_CCS_RC_CC,
> >> +
> >> +               .ccs.cc_planes = BIT(1),
> >>          }, {
> >>                  .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> >>                  .display_ver = { 13, 13 }, @@ -559,6 +565,7 @@
> >> intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane)
> >>                  else
> >>                          return 512;
> >>          case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> >> +       case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> >>          case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> >>          case I915_FORMAT_MOD_4_TILED:
> >>                  /*
> >> @@ -763,6 +770,7 @@ unsigned int intel_surf_alignment(const struct
> drm_framebuffer *fb,
> >>          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_RC_CCS_CC:
> >>          case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> >>                  return 16 * 1024;
> >>          default:
> >> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> index c38ae0876c15..b4dced1907c5 100644
> >> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> @@ -772,6 +772,8 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> >>                  return PLANE_CTL_TILED_4 |
> >>                          PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE |
> >>                          PLANE_CTL_CLEAR_COLOR_DISABLE;
> >> +       case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> >> +               return PLANE_CTL_TILED_4 |
> >> + PLANE_CTL_RENDER_DECOMPRESSION_ENABLE;
> >>          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;
> >> @@ -2358,10 +2360,15 @@ 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)) {
> >> -                       if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> >> +                       u32 rc_mask = PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
> >> +                                     PLANE_CTL_CLEAR_COLOR_DISABLE;
> >> +
> >> +                       if ((val & rc_mask) == rc_mask)
> >>                                  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 if (val & PLANE_CTL_RENDER_DECOMPRESSION_ENABLE)
> >> +                               fb->modifier =
> >> + I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC;
> >>                          else
> >>                                  fb->modifier = I915_FORMAT_MOD_4_TILED;
> >>                  } else {
> >> diff --git a/include/uapi/drm/drm_fourcc.h
> >> b/include/uapi/drm/drm_fourcc.h index b8fb7b44c03c..697614ea4b84
> >> 100644
> >> --- a/include/uapi/drm/drm_fourcc.h
> >> +++ b/include/uapi/drm/drm_fourcc.h
> >> @@ -605,6 +605,16 @@ extern "C" {
> >>    */
> >>   #define I915_FORMAT_MOD_4_TILED_DG2_MC_CCS
> fourcc_mod_code(INTEL,
> >> 11)
> >>
> >> +/*
> >> + * Intel color control surfaces (CCS) for DG2 clear color render compression.
> >> + *
> >> + * DG2 uses a unified compression format for clear color render
> compression.
> >
> > What's unified about DG2's compression format? If this doesn't affect
> > the layout, maybe we should drop this sentence.
> >
> >> + * The general layout is a tiled layout using 4Kb tiles i.e. Tile4 layout.
> >> + *
> >
> > This also needs a pitch aligned to four tiles, right? I think we can
> > save some effort by referencing the DG2_RC_CCS modifier here.
> >
> >> + * Fast clear color value expected by HW is located in fb at offset
> >> + 0 of plane#1
> >
> > Why is the expected offset hardcoded to 0 instead of relying on the
> > offset provided by the modifier API? This looks like a bug.
> 
> Hi Nanley,
> 
> can you elaborate a bit, which offset from modifier API that applies to cc surface?
> 

Hi Juha-Pekka,

On the kernel-side of things, I'm thinking of drm_mode_fb_cmd2::offsets[1].

-Nanley

> >
> > We should probably give some info about the relevant fields in the
> > fast clear plane (like what's done in the GEN12_RC_CCS_CC modifier).
> 
> agree, that's totally missing here.
> 
> /Juha-Pekka
> 
> >
> >> + */
> >> +#define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC
> fourcc_mod_code(INTEL,
> >> +12)
> >> +
> >>   /*
> >>    * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
> >>    *
> >> --
> >> 2.20.1
> >>