| Message ID | 20220201104132.3050-17-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dg2: Enabling 64k page size and flat ccs | expand |
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. 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). -Nanley > + */ > +#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 >
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? > > 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 >>
> -----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 > >>
On 15.2.2022 17.02, Chery, Nanley G wrote: > > >> -----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]. > Hi Nanley, this offset is coming from userspace on creation of framebuffer, at that moment from userspace caller can point to offset of desire. Normally offset[0] is set at 0 and then offset[n] at plane n start which is not stated to have to be exactly after plane n-1 end. Or did I misunderstand what you meant? For cc plane this offset likely will not be zero anyway and caller can move it as see fit to have cc plane (plane[1]) location[0] at place where wanted to have it. /Juha-Pekka > >>> >>> 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 >>>> >
> -----Original Message----- > From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > Sent: Tuesday, February 15, 2022 8:15 AM > To: Chery, Nanley G <nanley.g.chery@intel.com>; Nanley Chery > <nanleychery@gmail.com>; C, Ramalingam <ramalingam.c@intel.com> > Cc: 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 16/19] uapi/drm/dg2: Introduce format > modifier for DG2 clear color > > On 15.2.2022 17.02, Chery, Nanley G wrote: > > > > > >> -----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]. > > > > Hi Nanley, > > this offset is coming from userspace on creation of framebuffer, at that moment > from userspace caller can point to offset of desire. Normally offset[0] is set at 0 > and then offset[n] at plane n start which is not stated to have to be exactly after > plane n-1 end. Or did I misunderstand what you meant? > Perhaps, at least, I'm not sure what you're meaning to say. This modifier description seems to say that the drm_mode_fb_cmd2::offsets value for the clear color plane must be zero. Are you saying that it's correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't match mesa's expectations. -Nanley > For cc plane this offset likely will not be zero anyway and caller can move it as see > fit to have cc plane (plane[1]) location[0] at place where wanted to have it. > > /Juha-Pekka > > > > >>> > >>> 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 > >>>> > >
On 15.2.2022 18.44, Chery, Nanley G wrote: > > >> -----Original Message----- >> From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> >> Sent: Tuesday, February 15, 2022 8:15 AM >> To: Chery, Nanley G <nanley.g.chery@intel.com>; Nanley Chery >> <nanleychery@gmail.com>; C, Ramalingam <ramalingam.c@intel.com> >> Cc: 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 16/19] uapi/drm/dg2: Introduce format >> modifier for DG2 clear color >> >> On 15.2.2022 17.02, Chery, Nanley G wrote: >>> >>> >>>> -----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]. >>> >> >> Hi Nanley, >> >> this offset is coming from userspace on creation of framebuffer, at that moment >> from userspace caller can point to offset of desire. Normally offset[0] is set at 0 >> and then offset[n] at plane n start which is not stated to have to be exactly after >> plane n-1 end. Or did I misunderstand what you meant? >> > > Perhaps, at least, I'm not sure what you're meaning to say. This modifier description > seems to say that the drm_mode_fb_cmd2::offsets value for the clear color plane > must be zero. Are you saying that it's correct? This doesn't match the > GEN12_RC_CCS_CC behavior and doesn't match mesa's expectations. > It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 of plane#1". Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's nothing stated about that offset. These offsets are just offsets to bo which contain the framebuffer information hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc information is found starting at drm_mode_fb_cmd2::offsets[1][0] /Juha-Pekka > >> For cc plane this offset likely will not be zero anyway and caller can move it as see >> fit to have cc plane (plane[1]) location[0] at place where wanted to have it. >> >> /Juha-Pekka >> >>> >>>>> >>>>> 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 >>>>>> >>> >
> -----Original Message----- > From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > Sent: Tuesday, February 15, 2022 9:32 AM > To: Chery, Nanley G <nanley.g.chery@intel.com>; Nanley Chery > <nanleychery@gmail.com>; C, Ramalingam <ramalingam.c@intel.com> > Cc: 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 16/19] uapi/drm/dg2: Introduce format > modifier for DG2 clear color > > On 15.2.2022 18.44, Chery, Nanley G wrote: > > > > > >> -----Original Message----- > >> From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > >> Sent: Tuesday, February 15, 2022 8:15 AM > >> To: Chery, Nanley G <nanley.g.chery@intel.com>; Nanley Chery > >> <nanleychery@gmail.com>; C, Ramalingam <ramalingam.c@intel.com> > >> Cc: 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 16/19] uapi/drm/dg2: Introduce > >> format modifier for DG2 clear color > >> > >> On 15.2.2022 17.02, Chery, Nanley G wrote: > >>> > >>> > >>>> -----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]. > >>> > >> > >> Hi Nanley, > >> > >> this offset is coming from userspace on creation of framebuffer, at > >> that moment from userspace caller can point to offset of desire. > >> Normally offset[0] is set at 0 and then offset[n] at plane n start > >> which is not stated to have to be exactly after plane n-1 end. Or did I > misunderstand what you meant? > >> > > > > Perhaps, at least, I'm not sure what you're meaning to say. This > > modifier description seems to say that the drm_mode_fb_cmd2::offsets > > value for the clear color plane must be zero. Are you saying that it's > > correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't > match mesa's expectations. > > > > It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must > be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 > of plane#1". > Yes, it doesn't say that exactly, but that's what it seems to say. With every other modifier, it's implied that the data for the plane begins at the offset specified through the modifier API. So, explicitly mentioning it here (and with that wording) conveys a new requirement. > Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's > nothing stated about that offset. > Technically, plane #1's location is specified to be the combination of ::handles[1] and ::offsets[1]. In practice though, I can imagine that there are areas of the stack that are implicitly requiring that all ::handles[] entries match. > These offsets are just offsets to bo which contain the framebuffer information > hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc > information is found starting at drm_mode_fb_cmd2::offsets[1][0] > If the clear color handling is the same as GEN12_RC_CCS_CC (apart for the plane index), I propose that we drop this sentence due to avoid any confusion. This offset discussion raises another question. The description says that the value expected by HW is at offset 0. I'm assuming "HW" is referring to the render engine? The kernel is still giving the display engine the packed values at ::offsets[1] + 16B right? -Nanley > /Juha-Pekka > > > > >> For cc plane this offset likely will not be zero anyway and caller > >> can move it as see fit to have cc plane (plane[1]) location[0] at place where > wanted to have it. > >> > >> /Juha-Pekka > >> > >>> > >>>>> > >>>>> 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 > >>>>>> > >>> > >
On 15.2.2022 20.24, Chery, Nanley G wrote: > > >> -----Original Message----- >> From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> >> Sent: Tuesday, February 15, 2022 9:32 AM >> To: Chery, Nanley G <nanley.g.chery@intel.com>; Nanley Chery >> <nanleychery@gmail.com>; C, Ramalingam <ramalingam.c@intel.com> >> Cc: 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 16/19] uapi/drm/dg2: Introduce format >> modifier for DG2 clear color >> >> On 15.2.2022 18.44, Chery, Nanley G wrote: >>> >>> >>>> -----Original Message----- >>>> From: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> >>>> Sent: Tuesday, February 15, 2022 8:15 AM >>>> To: Chery, Nanley G <nanley.g.chery@intel.com>; Nanley Chery >>>> <nanleychery@gmail.com>; C, Ramalingam <ramalingam.c@intel.com> >>>> Cc: 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 16/19] uapi/drm/dg2: Introduce >>>> format modifier for DG2 clear color >>>> >>>> On 15.2.2022 17.02, Chery, Nanley G wrote: >>>>> >>>>> >>>>>> -----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]. >>>>> >>>> >>>> Hi Nanley, >>>> >>>> this offset is coming from userspace on creation of framebuffer, at >>>> that moment from userspace caller can point to offset of desire. >>>> Normally offset[0] is set at 0 and then offset[n] at plane n start >>>> which is not stated to have to be exactly after plane n-1 end. Or did I >> misunderstand what you meant? >>>> >>> >>> Perhaps, at least, I'm not sure what you're meaning to say. This >>> modifier description seems to say that the drm_mode_fb_cmd2::offsets >>> value for the clear color plane must be zero. Are you saying that it's >>> correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't >> match mesa's expectations. >>> >> >> It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must >> be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 >> of plane#1". >> > > Yes, it doesn't say that exactly, but that's what it seems to say. With every other > modifier, it's implied that the data for the plane begins at the offset specified > through the modifier API. So, explicitly mentioning it here (and with that wording) > conveys a new requirement. I don't have objections on changing this description but for reference gen12 version of the same says "The main surface is Y-tiled and is at plane index 0 whereas CCS is linear and at index 1. The clear color is stored at index 2, and the pitch should be ignored.", only plane indexes are mentioned. I anyway wrote neither of these descriptions. > >> Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's >> nothing stated about that offset. >> > > Technically, plane #1's location is specified to be the combination of ::handles[1] > and ::offsets[1]. In practice though, I can imagine that there are areas of the stack > that are implicitly requiring that all ::handles[] entries match. I didn't think we needed to go deeper as you started to just talk about how drm_mode_fb_cmd2::offsets[1] not being used. Let's not waste time. > >> These offsets are just offsets to bo which contain the framebuffer information >> hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc >> information is found starting at drm_mode_fb_cmd2::offsets[1][0] >> > > If the clear color handling is the same as GEN12_RC_CCS_CC (apart for the plane > index), I propose that we drop this sentence due to avoid any confusion. > But it need to defined as part of the modifier. It's the modifier features which are being described here. > This offset discussion raises another question. The description says that the value > expected by HW is at offset 0. I'm assuming "HW" is referring to the render engine? > The kernel is still giving the display engine the packed values at ::offsets[1] + 16B right? Generally answer is yes but these parts you can see in patch "[PATCH v5 17/19] drm/i915/dg2: Flat CCS Support" and should be discussed there. Here "HW" should probably be changed something meaningful though. /Juha-Pekka >> >>> >>>> For cc plane this offset likely will not be zero anyway and caller >>>> can move it as see fit to have cc plane (plane[1]) location[0] at place where >> wanted to have it. >>>> >>>> /Juha-Pekka >>>> >>>>> >>>>>>> >>>>>>> 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 >>>>>>>> >>>>> >>> >
Hi Nanley, JP, On Tue, Feb 15, 2022 at 09:34:22PM +0200, Juha-Pekka Heikkila wrote: > [...] > > > > > > > > > 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. Unified here probably refers to the fact the DG2 render engine is capable of generating both a render and a media compressed surface as opposed to earlier platforms. The display engine still needs to know which compression format the FB uses, hence we need both an RC and MC modifier. Based on this I also think we can drop the mention of unified compression. > > > > > > > > > + * 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]. > > > > > > > > > > Hi Nanley, > > > > > > > > > > this offset is coming from userspace on creation of framebuffer, at > > > > > that moment from userspace caller can point to offset of desire. > > > > > Normally offset[0] is set at 0 and then offset[n] at plane n start > > > > > which is not stated to have to be exactly after plane n-1 end. Or did I > > > > > misunderstand what you meant? > > > > > > > > Perhaps, at least, I'm not sure what you're meaning to say. This > > > > modifier description seems to say that the drm_mode_fb_cmd2::offsets > > > > value for the clear color plane must be zero. Are you saying that it's > > > > correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't > > > > match mesa's expectations. > > > > > > It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must > > > be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 > > > of plane#1". > > > > Yes, it doesn't say that exactly, but that's what it seems to say. With every other > > modifier, it's implied that the data for the plane begins at the offset specified > > through the modifier API. So, explicitly mentioning it here (and with that wording) > > conveys a new requirement. > > I don't have objections on changing this description but for reference gen12 > version of the same says "The main surface is Y-tiled and is at plane index > 0 whereas CCS is linear and at index 1. The clear color is stored at index > 2, and the pitch should be ignored.", only plane indexes are mentioned. I > anyway wrote neither of these descriptions. > > > > Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's > > > nothing stated about that offset. > > > > Technically, plane #1's location is specified to be the combination of ::handles[1] > > and ::offsets[1]. In practice though, I can imagine that there are areas of the stack > > that are implicitly requiring that all ::handles[] entries match. The FB modifier API requires all ::handles[] to match, that is all planes must be contained in one GEM object. > I didn't think we needed to go deeper as you started to just talk about how > drm_mode_fb_cmd2::offsets[1] not being used. Let's not waste time. > > > > These offsets are just offsets to bo which contain the framebuffer information > > > hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc > > > information is found starting at drm_mode_fb_cmd2::offsets[1][0] > > > > If the clear color handling is the same as GEN12_RC_CCS_CC (apart for the plane > > index), I propose that we drop this sentence due to avoid any confusion. > > But it need to defined as part of the modifier. It's the modifier features > which are being described here. > > > This offset discussion raises another question. The description says that the value > > expected by HW is at offset 0. I'm assuming "HW" is referring to the render engine? > > The kernel is still giving the display engine the packed values at ::offsets[1] + 16B right? > > Generally answer is yes but these parts you can see in patch "[PATCH v5 > 17/19] drm/i915/dg2: Flat CCS Support" and should be discussed there. Here > "HW" should probably be changed something meaningful though. The 256 bit clear color format starting at plane index 1 matches the one described at I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC . So yes, "HW" refers to the render engine and display consumes the 64 bit data at ::offset[1] + 16 bytes (and DE ignores the 64 bit data starting at ::offset[1] + 24 bytes. The following captures all the above, would it be ok?: Intel Color Control Surface with Clear Color (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. The clear color is stored at plane index 1 and the pitch should be ignored. The format of the 256 bits clear color data matches the one used for the I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its description for details. --Imre
> -----Original Message----- > From: Deak, Imre <imre.deak@intel.com> > Sent: Monday, March 21, 2022 6:20 AM > To: Chery, Nanley G <nanley.g.chery@intel.com>; Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > Cc: 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 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color > > Hi Nanley, JP, > > On Tue, Feb 15, 2022 at 09:34:22PM +0200, Juha-Pekka Heikkila wrote: > > [...] > > > > > > > > > > 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. > > Unified here probably refers to the fact the DG2 render engine is > capable of generating both a render and a media compressed surface as > opposed to earlier platforms. The display engine still needs to know > which compression format the FB uses, hence we need both an RC and MC > modifier. Based on this I also think we can drop the mention of unified > compression. > > > > > > > > > > > + * 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]. > > > > > > > > > > > > Hi Nanley, > > > > > > > > > > > > this offset is coming from userspace on creation of framebuffer, at > > > > > > that moment from userspace caller can point to offset of desire. > > > > > > Normally offset[0] is set at 0 and then offset[n] at plane n start > > > > > > which is not stated to have to be exactly after plane n-1 end. Or did I > > > > > > misunderstand what you meant? > > > > > > > > > > Perhaps, at least, I'm not sure what you're meaning to say. This > > > > > modifier description seems to say that the drm_mode_fb_cmd2::offsets > > > > > value for the clear color plane must be zero. Are you saying that it's > > > > > correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't > > > > > match mesa's expectations. > > > > > > > > It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must > > > > be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 > > > > of plane#1". > > > > > > Yes, it doesn't say that exactly, but that's what it seems to say. With every other > > > modifier, it's implied that the data for the plane begins at the offset specified > > > through the modifier API. So, explicitly mentioning it here (and with that wording) > > > conveys a new requirement. > > > > I don't have objections on changing this description but for reference gen12 > > version of the same says "The main surface is Y-tiled and is at plane index > > 0 whereas CCS is linear and at index 1. The clear color is stored at index > > 2, and the pitch should be ignored.", only plane indexes are mentioned. I > > anyway wrote neither of these descriptions. > > > > > > Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's > > > > nothing stated about that offset. > > > > > > Technically, plane #1's location is specified to be the combination of ::handles[1] > > > and ::offsets[1]. In practice though, I can imagine that there are areas of the stack > > > that are implicitly requiring that all ::handles[] entries match. > > The FB modifier API requires all ::handles[] to match, that is all > planes must be contained in one GEM object. > This is a requirement for i915, or for all drm drivers? I couldn't find anything in the generic DRM headers or docs requiring this. Feel free to ping me about this offline. > > I didn't think we needed to go deeper as you started to just talk about how > > drm_mode_fb_cmd2::offsets[1] not being used. Let's not waste time. > > > > > > These offsets are just offsets to bo which contain the framebuffer information > > > > hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc > > > > information is found starting at drm_mode_fb_cmd2::offsets[1][0] > > > > > > If the clear color handling is the same as GEN12_RC_CCS_CC (apart for the plane > > > index), I propose that we drop this sentence due to avoid any confusion. > > > > But it need to defined as part of the modifier. It's the modifier features > > which are being described here. > > > > > This offset discussion raises another question. The description says that the value > > > expected by HW is at offset 0. I'm assuming "HW" is referring to the render engine? > > > The kernel is still giving the display engine the packed values at ::offsets[1] + 16B right? > > > > Generally answer is yes but these parts you can see in patch "[PATCH v5 > > 17/19] drm/i915/dg2: Flat CCS Support" and should be discussed there. Here > > "HW" should probably be changed something meaningful though. > > The 256 bit clear color format starting at plane index 1 matches the one > described at I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC . So yes, "HW" refers > to the render engine and display consumes the 64 bit data at > ::offset[1] + 16 bytes (and DE ignores the 64 bit data starting at > ::offset[1] + 24 bytes. > > The following captures all the above, would it be ok?: > > Intel Color Control Surface with Clear Color (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 ^ "for all compressible" ? (since SMEM objects don't have this) > required to be a multiple of four Tile 4 widths. The clear color is stored > at plane index 1 and the pitch should be ignored. The format of the 256 > bits clear color data matches the one used for the ^ "256 bits of"? > I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its description > for details. > Looks good to me. With the above minor changes, Acked-by: Nanley Chery <nanley.g.chery@intel.com> > --Imre
On Thu, Mar 24, 2022 at 01:42:33AM +0200, Chery, Nanley G wrote: > > -----Original Message----- > > From: Deak, Imre <imre.deak@intel.com> > > Sent: Monday, March 21, 2022 6:20 AM > > To: Chery, Nanley G <nanley.g.chery@intel.com>; Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > Cc: 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 16/19] uapi/drm/dg2: Introduce format modifier for DG2 clear color > > > > Hi Nanley, JP, > > > > On Tue, Feb 15, 2022 at 09:34:22PM +0200, Juha-Pekka Heikkila wrote: > > > [...] > > > > > > > > > > > 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. > > > > Unified here probably refers to the fact the DG2 render engine is > > capable of generating both a render and a media compressed surface as > > opposed to earlier platforms. The display engine still needs to know > > which compression format the FB uses, hence we need both an RC and MC > > modifier. Based on this I also think we can drop the mention of unified > > compression. > > > > > > > > > > > > > + * 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]. > > > > > > > > > > > > > > Hi Nanley, > > > > > > > > > > > > > > this offset is coming from userspace on creation of framebuffer, at > > > > > > > that moment from userspace caller can point to offset of desire. > > > > > > > Normally offset[0] is set at 0 and then offset[n] at plane n start > > > > > > > which is not stated to have to be exactly after plane n-1 end. Or did I > > > > > > > misunderstand what you meant? > > > > > > > > > > > > Perhaps, at least, I'm not sure what you're meaning to say. This > > > > > > modifier description seems to say that the drm_mode_fb_cmd2::offsets > > > > > > value for the clear color plane must be zero. Are you saying that it's > > > > > > correct? This doesn't match the GEN12_RC_CCS_CC behavior and doesn't > > > > > > match mesa's expectations. > > > > > > > > > > It doesn't say "drm_mode_fb_cmd2::offsets value for the clear color plane must > > > > > be zero", it says "Fast clear color value expected by HW is located in fb at offset 0 > > > > > of plane#1". > > > > > > > > Yes, it doesn't say that exactly, but that's what it seems to say. With every other > > > > modifier, it's implied that the data for the plane begins at the offset specified > > > > through the modifier API. So, explicitly mentioning it here (and with that wording) > > > > conveys a new requirement. > > > > > > I don't have objections on changing this description but for reference gen12 > > > version of the same says "The main surface is Y-tiled and is at plane index > > > 0 whereas CCS is linear and at index 1. The clear color is stored at index > > > 2, and the pitch should be ignored.", only plane indexes are mentioned. I > > > anyway wrote neither of these descriptions. > > > > > > > > Plane#1 location is pointed by drm_mode_fb_cmd2::offsets[1] and there's > > > > > nothing stated about that offset. > > > > > > > > Technically, plane #1's location is specified to be the combination of ::handles[1] > > > > and ::offsets[1]. In practice though, I can imagine that there are areas of the stack > > > > that are implicitly requiring that all ::handles[] entries match. > > > > The FB modifier API requires all ::handles[] to match, that is all > > planes must be contained in one GEM object. > > This is a requirement for i915, or for all drm drivers? I couldn't find anything in the > generic DRM headers or docs requiring this. Feel free to ping me about this offline. It's only an i915 requirement actually. IIUC, it was added for the SKL CCS modifiers (2e2adb05736c3), where SKL has a restriction on the location of the CCS (and UV) planes. The feasible way to conform to these limits was to require all planes to reside in the same GEM object. For DG2 there was no plan to expose the CCS plane, so wrt. that this restriction didn't make a difference. > > > I didn't think we needed to go deeper as you started to just talk about how > > > drm_mode_fb_cmd2::offsets[1] not being used. Let's not waste time. > > > > > > > > These offsets are just offsets to bo which contain the framebuffer information > > > > > hence drm_mode_fb_cmd2::offsets[1] can be changed as one wish and cc > > > > > information is found starting at drm_mode_fb_cmd2::offsets[1][0] > > > > > > > > If the clear color handling is the same as GEN12_RC_CCS_CC (apart for the plane > > > > index), I propose that we drop this sentence due to avoid any confusion. > > > > > > But it need to defined as part of the modifier. It's the modifier features > > > which are being described here. > > > > > > > This offset discussion raises another question. The description says that the value > > > > expected by HW is at offset 0. I'm assuming "HW" is referring to the render engine? > > > > The kernel is still giving the display engine the packed values at ::offsets[1] + 16B right? > > > > > > Generally answer is yes but these parts you can see in patch "[PATCH v5 > > > 17/19] drm/i915/dg2: Flat CCS Support" and should be discussed there. Here > > > "HW" should probably be changed something meaningful though. > > > > The 256 bit clear color format starting at plane index 1 matches the one > > described at I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC . So yes, "HW" refers > > to the render engine and display consumes the 64 bit data at > > ::offset[1] + 16 bytes (and DE ignores the 64 bit data starting at > > ::offset[1] + 24 bytes. > > > > The following captures all the above, would it be ok?: > > > > Intel Color Control Surface with Clear Color (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 > ^ > "for all compressible" ? (since SMEM objects don't have this) Makes sense, optionally also mentioning that "for all RC/RC_CC/MC CCS compressible GEM objects". > > required to be a multiple of four Tile 4 widths. The clear color is stored > > at plane index 1 and the pitch should be ignored. The format of the 256 > > bits clear color data matches the one used for the > ^ > "256 bits of"? Ok. > > I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its description > > for details. > > Looks good to me. With the above minor changes, > Acked-by: Nanley Chery <nanley.g.chery@intel.com> Thanks. --Imre
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. + * The general layout is a tiled layout using 4Kb tiles i.e. Tile4 layout. + * + * Fast clear color value expected by HW is located in fb at offset 0 of plane#1 + */ +#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 *