RE: [PATCH] drm/i915/gt: Disable Redundant HZ Plane expansions for MTL/ARL and DG2

[Chery, Nanley G]

> -Original Message-
> From: Roper, Matthew D 
> Sent: Friday, August 23, 2024 3:26 PM
> To: Bhadane, Dnyaneshwar 
> Cc: intel-gfx@lists.freedesktop.org; Chery, Nanley G 
> 
> Subject: Re: [PATCH] drm/i915/gt: Disable Redundant HZ Plane expansions for 
> MTL/ARL and DG2
> 
> On Fri, Aug 23, 2024 at 03:40:09PM +0530, Dnyaneshwar Bhadane wrote:
> > Program HZ Plane disable bit to 1 to stop sending the redundant
> > plane expansions.
> >
> > Bspec: 68331
> >
> > Signed-off-by: Dnyaneshwar Bhadane 
> > ---
> >  drivers/gpu/drm/i915/gt/intel_gt_regs.h |  5 +++--
> >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 17 +++--
> >  2 files changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h 
> > b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index e42b3a5d4e63..74b633a78eda 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -446,8 +446,9 @@
> >
> >  /* GEN7 chicken */
> >  #define GEN7_COMMON_SLICE_CHICKEN1 _MMIO(0x7010)
> > -#define   GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCC(1 << 10)
> > -#define   GEN9_RHWO_OPTIMIZATION_DISABLE   (1 << 14)
> > +#define   GEN9_RHWO_OPTIMIZATION_DISABLE   REG_BIT(14)
> > +#define   GEN7_CSC1_RHWO_OPT_DISABLE_IN_RCCREG_BIT(10)
> > +#define   HIZ_PLANE_OPTIMIZATION_DISABLE   REG_BIT(9)
> >
> >  #define COMMON_SLICE_CHICKEN2  _MMIO(0x7014)
> >  #define   GEN9_PBE_COMPRESSED_HASH_SELECTION   (1 << 13)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
> > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index bfe6d8fc820f..ff257bb2d15a 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -1550,6 +1550,13 @@ dg2_gt_workarounds_init(struct intel_gt *gt, struct 
> > i915_wa_list *wal)
> >
> > /* Wa_14010648519:dg2 */
> > wa_mcr_write_or(wal, XEHP_L3NODEARBCFG, XEHP_LNESPARE);
> > +
> > +   /*
> > +* DisableHIZPlaneOptimizationForRedundantZPlaneUnit
> > +* This is not WA,THis is required by recommended tuning setting.
> > +*/
> > +   wa_masked_dis(wal,
> > + GEN7_COMMON_SLICE_CHICKEN1, 
> > HIZ_PLANE_OPTIMIZATION_DISABLE);
> 
> The bspec's performance guide page says that this should be done
> selectively, on a workload-specific basis when certain conditions are
> met.  So we don't want to set the value of this bit directly in the KMD
> because we don't know anything about the workloads being executed.  We
> just want to make the register writable from userspace so that they can
> flip this bit themselves when it's appropriate.  The
> {dg2,xelp}_whitelist_build changes you have farther down take care of
> granting userspace access to do this; we can drop the changes here to
> the {dg2,xelpg}_gt_workarounds_init functions.
> 
> From a quick skim of the Mesa source code, it looks like Mesa is only
> setting this register bit right now on the older gen12 platforms to
> address Wa_1808121037 (in src/intel/vulkan/genX_cmd_buffer.c and
> src/gallium/drivers/iris/iris_state.c), but I don't see them setting
> this anywhere that isn't guarded by "#if INTEL_NEEDS_WA_1808121037" yet.
> They might not have seen the update in the performance guide, or they
> might have been waiting for us to whitelist the register on these
> platforms before adding their implementation.
> 
> +Cc Nanley from the Mesa team since he implemented the Wa_1808121037
> code and will probably know best if/how the Mesa code should be updated
> to also address the DG2 + MTL performance tuning setting recommended on
> bspec 68331.
> 

Thanks for the heads up! I'll file an mesa issue about this.

-Nanley

> 
> Matt
> 
> >  }
> >
> >  static void
> > @@ -1570,6 +1577,12 @@ xelpg_gt_workarounds_init(struct intel_gt *gt, 
> > struct i915_wa_list *wal)
> > /* Wa_14015795083 */
> > wa_write_clr(wal, GEN7_MISCCPCTL, 
> > GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
> > }
> > +   /*
> > +* DisableHIZPlaneOptimizationForRedundantZPlaneUnit
> > +* This is not WA, This is required by recommended tuning setting.
> > +*/
> > +   wa_masked_dis(wal,
> > + GEN7_COMMON_SLICE_CHICKEN1, 
> > HIZ_PLANE_OPTIMIZATION_DISABLE);
> >
> > /*
> >  * Unlike older platforms, we no longer setup implicit steering here;
> > @@ -2072,7 +2085,7 @@

RE: [RFC PATCH 0/3] Introducing I915_FORMAT_MOD_4_TILED_XE2_CCS Modifier for Xe2

[Chery, Nanley G]

+1. It would help to have an explicit CCS modifier should an unforeseen corner 
case arise.

Also, as Ken mentioned, CPU mapped accesses require that compressed images be 
resolved beforehand. By having both a 4_TILED modifier and a 4_TILED_XE2_CCS 
modifier, applications can avoid performance penalties if they would like to do 
frequent CPU mapped accesses.

-Nanley

From: Zhang, Jianxun 
Sent: Friday, July 19, 2024 4:17 PM
To: Thomas Hellström ; Joonas Lahtinen 
; Juha-Pekka Heikkila 
; Graunke, Kenneth W 
; Roper, Matthew D 
Cc: intel...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; Chery, 
Nanley G ; Saarinen, Jani ; 
Souza, Jose ; Mathew, Alwin ; 
Syrjala, Ville ; Nikula, Jani 

Subject: Re: [RFC PATCH 0/3] Introducing I915_FORMAT_MOD_4_TILED_XE2_CCS 
Modifier for Xe2




From: Thomas Hellström 
mailto:thomas.hellst...@linux.intel.com>>
Sent: Tuesday, May 14, 2024 9:51 AM
To: Joonas Lahtinen 
mailto:joonas.lahti...@linux.intel.com>>; 
Juha-Pekka Heikkila 
mailto:juhapekka.heikk...@gmail.com>>; Graunke, 
Kenneth W mailto:kenneth.w.grau...@intel.com>>; 
Roper, Matthew D mailto:matthew.d.ro...@intel.com>>
Cc: intel...@lists.freedesktop.org<mailto:intel...@lists.freedesktop.org> 
mailto:intel...@lists.freedesktop.org>>; 
intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org> 
mailto:intel-gfx@lists.freedesktop.org>>; 
Chery, Nanley G mailto:nanley.g.ch...@intel.com>>; 
Saarinen, Jani mailto:jani.saari...@intel.com>>; 
Souza, Jose mailto:jose.so...@intel.com>>; Mathew, Alwin 
mailto:alwin.mat...@intel.com>>; Zhang, Jianxun 
mailto:jianxun.zh...@intel.com>>; Syrjala, Ville 
mailto:ville.syrj...@linux.intel.com>>; Nikula, 
Jani mailto:jani.nik...@intel.com>>
Subject: Re: [RFC PATCH 0/3] Introducing I915_FORMAT_MOD_4_TILED_XE2_CCS 
Modifier for Xe2

On Tue, 2024-05-14 at 12:25 +0300, Joonas Lahtinen wrote:
> Quoting Kenneth Graunke (2024-05-11 03:58:34)
> > On Tuesday, May 7, 2024 3:56:57 PM PDT Matt Roper wrote:
> > > On Mon, May 06, 2024 at 09:52:35PM +0300, Juha-Pekka Heikkila
> > > wrote:
> > > > These patches introduce I915_FORMAT_MOD_4_TILED_XE2_CCS
> > > > modifier, which,
> > > > from the kernel's perspective, behaves similarly to
> > `I915_FORMAT_MOD_4_TILED`.
> > > > This new modifier is primarily intended for user space to
> > > > effectively
> > monitor
> > > > compression status, especially when dealing with a mix of
> > > > compressed and
> > > > uncompressed buffers.
> > > >
> > > > The addition of this modifier facilitates user space in
> > > > managing
> > compression
> > > > status, particularly when utilizing both compressed and
> > > > uncompressed
> > buffers
> > > > concurrently. To leverage compression for these buffers, user
> > > > space
> > > > applications must configure the appropriate Page Attribute
> > > > Table (PAT)
> > index.
> > > > Display engine will treat all Tile4 as if it were compressed
> > > > under all
> > > > circumstances on Xe2 architecture.
> > >
> > > I may have missed some discussion about this, but I thought the
> > > previous
> > > consensus was that we didn't want/need new modifiers for
> > > compression on
> > > Xe2?  If a userspace client (or the display hardware) receives a
> > > buffer
> > > of unknown origin and unknown compression status, it's always
> > > fine to
> > > select a compressed PAT when binding the buffer to read since
> > > even for
> > > uncompressed buffers the CCS metadata will accurately reflect the
> > > compression status.  Unlike Xe1, where generating content without
> > > compression enabled would leave random garbage in the FlatCCS
> > > area, Xe2
> > > will set the corresponding FlatCCS to '0x0' for each block,
> > > indicating
> > > uncompressed data.
> > >
> > > Can you explain more what the benefit of handling these modifiers
> > > explicitly is?
I am not sure if there is another nice way to address this case in the 
eco-system without an explicit modifier even if it is not necessary from kernel 
driver aspect, so I just put it here to get some insights.

I assume we still want to support existing modifiers like 
I915_FORMAT_MOD_4_TILED, then the modifier extension of Vulkan becomes relevant 
(https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_image_drm_format_modifier.html).
 In the section of export, the spec depicts the driver can choose the o

RE: [RFC PATCH 1/3] drm/fourcc: define Intel Xe2 related tile4 ccs modifier

[Chery, Nanley G]

> -Original Message-
> From: Juha-Pekka Heikkila 
> Sent: Monday, May 6, 2024 2:53 PM
> To: intel...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Juha-Pekka Heikkila ; Chery, Nanley G 
> ; Saarinen, Jani
> ; Graunke, Kenneth W ; 
> Souza, Jose ; Mathew, Alwin
> ; Zhang, Jianxun ; Syrjala, 
> Ville ; Nikula, Jani
> 
> Subject: [RFC PATCH 1/3] drm/fourcc: define Intel Xe2 related tile4 ccs 
> modifier
> 
> Add Tile4 type ccs modifier to indicate presence of compression on Xe2
> 
> Signed-off-by: Juha-Pekka Heikkila 
> ---
>  include/uapi/drm/drm_fourcc.h | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 84d502e42961..50db2cc89642 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -702,6 +702,18 @@ extern "C" {
>   */
>  #define I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC fourcc_mod_code(INTEL, 15)
> 
> +/*
> + * Intel Color Control Surfaces (CCS) for graphics ver. 20 render 
> compression.
> + *
> + * The main surface is Tile 4 and at plane index 0. For semi-planar formats
> + * like NV12, the Y and UV planes are Tile 4 and are located at plane indices
> + * 0 and 1, respectively. The CCS for all planes are stored outside of the
> + * GEM object in a reserved memory area dedicated for the storage of the
> + * CCS data for all compressible GEM objects. The main surface pitch is
> + * required to be a multiple of four Tile 4 widths.
> + */

The pitch requirement is gone on this generation, isn't it?

-Nanley

> +#define I915_FORMAT_MOD_4_TILED_XE2_CCS fourcc_mod_code(INTEL, 16)
> +
>  /*
>   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>   *
> --
> 2.43.2

Re: [Intel-gfx] [PATCH] drm/fourcc: Document the Intel CCS modifiers' CC plane expected pitch

[Chery, Nanley G]

Actually add Jordan this time :)

> -Original Message-
> From: Chery, Nanley G
> Sent: Friday, June 24, 2022 5:39 PM
> To: Deak, Imre ; dri-de...@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/fourcc: Document the Intel CCS modifiers' CC plane 
> expected pitch
> 
> +Jordan (FYI)
> 
> I think the commit message has an extra "color" next to "CC".
> With or without that dropped,
> 
> Reviewed-by: Nanley Chery 
> 
> Thanks for the fix.
> 
> > -Original Message-
> > From: Deak, Imre 
> > Sent: Thursday, June 23, 2022 10:50 AM
> > To: dri-de...@lists.freedesktop.org
> > Cc: intel-gfx@lists.freedesktop.org; Chery, Nanley G 
> > 
> > Subject: [PATCH] drm/fourcc: Document the Intel CCS modifiers' CC plane 
> > expected pitch
> >
> > The driver expects the pitch of the Intel CCS CC color planes to be
> > 64 bytes aligned, adjust the modifier descriptions accordingly.
> >
> > Cc: Nanley Chery 
> > Signed-off-by: Imre Deak 
> > ---
> >  include/uapi/drm/drm_fourcc.h | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index f1972154a5940..c1b4cfda75075 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -559,7 +559,7 @@ extern "C" {
> >   *
> >   * 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. The clear color structure is 256 bits. The first 128 bits
> > + * be 64 bytes aligned. The clear color structure is 256 bits. The first 
> > 128 bits
> >   * represents Raw Clear Color Red, Green, Blue and Alpha color each 
> > represented
> >   * by 32 bits. The raw clear color is consumed by the 3d engine and 
> > generates
> >   * the converted clear color of size 64 bits. The first 32 bits store the 
> > Lower
> > @@ -612,9 +612,9 @@ extern "C" {
> >   * outside of the GEM object in a reserved memory area dedicated for the
> >   * storage of the CCS data for all RC/RC_CC/MC compressible 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 of clear color data matches the one used for the
> > - * I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its description
> > + * clear color is stored at plane index 1 and the pitch should be 64 bytes
> > + * aligned. The format of the 256 bits of clear color data matches the one 
> > used
> > + * for the I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its 
> > description
> >   * for details.
> >   */
> >  #define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC fourcc_mod_code(INTEL, 12)
> > --
> > 2.30.2

Re: [Intel-gfx] [PATCH] drm/fourcc: Document the Intel CCS modifiers' CC plane expected pitch

[Chery, Nanley G]

+Jordan (FYI)

I think the commit message has an extra "color" next to "CC". 
With or without that dropped,

Reviewed-by: Nanley Chery 

Thanks for the fix.

> -Original Message-
> From: Deak, Imre 
> Sent: Thursday, June 23, 2022 10:50 AM
> To: dri-de...@lists.freedesktop.org
> Cc: intel-gfx@lists.freedesktop.org; Chery, Nanley G 
> 
> Subject: [PATCH] drm/fourcc: Document the Intel CCS modifiers' CC plane 
> expected pitch
> 
> The driver expects the pitch of the Intel CCS CC color planes to be
> 64 bytes aligned, adjust the modifier descriptions accordingly.
> 
> Cc: Nanley Chery 
> Signed-off-by: Imre Deak 
> ---
>  include/uapi/drm/drm_fourcc.h | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index f1972154a5940..c1b4cfda75075 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -559,7 +559,7 @@ extern "C" {
>   *
>   * 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. The clear color structure is 256 bits. The first 128 bits
> + * be 64 bytes aligned. The clear color structure is 256 bits. The first 128 
> bits
>   * represents Raw Clear Color Red, Green, Blue and Alpha color each 
> represented
>   * by 32 bits. The raw clear color is consumed by the 3d engine and generates
>   * the converted clear color of size 64 bits. The first 32 bits store the 
> Lower
> @@ -612,9 +612,9 @@ extern "C" {
>   * outside of the GEM object in a reserved memory area dedicated for the
>   * storage of the CCS data for all RC/RC_CC/MC compressible 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 of clear color data matches the one used for the
> - * I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its description
> + * clear color is stored at plane index 1 and the pitch should be 64 bytes
> + * aligned. The format of the 256 bits of clear color data matches the one 
> used
> + * for the I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC modifier, see its 
> description
>   * for details.
>   */
>  #define I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC fourcc_mod_code(INTEL, 12)
> --
> 2.30.2

Re: [Intel-gfx] [PATCH v5 13/19] drm/i915: Introduce new Tile 4 format

[Chery, Nanley G]

Hi all,

Found an error in this description..

> From: Stanislav Lisovskiy 
> stanislav.lisovs...@intel.com
>
> This tiling layout uses 4KB tiles in a row-major layout. It has the same
> shape as Tile Y at two granularities: 4KB (128B x 32) and 64B (16B x 4). It
> only differs from Tile Y at the 256B granularity in between. At this
> granularity, Tile Y has a shape of 16B x 32 rows, but this tiling has a shape
> of 64B x 8 rows.
>

256B should be 512B (same feedback for the modifier description).

Regards,
Nanley

> Reviewed-by: Imre Deak imre.d...@intel.com
> Acked-by: Nanley Chery 
> nanley.g.ch...@intel.com
> Signed-off-by: Stanislav Lisovskiy 
> stanislav.lisovs...@intel.com
> ---
>  include/uapi/drm/drm_fourcc.h | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index fc0c1454d275..b73fe6797fc3 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -572,6 +572,17 @@ extern "C" {
>   */
>  #define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
>
> +/*
> + * Intel Tile 4 layout
> + *
> + * This is a tiled layout using 4KB tiles in a row-major layout. It has the 
> same
> + * shape as Tile Y at two granularities: 4KB (128B x 32) and 64B (16B x 4). 
> It
> + * only differs from Tile Y at the 256B granularity in between. At this
> + * granularity, Tile Y has a shape of 16B x 32 rows, but this tiling has a 
> shape
> + * of 64B x 8 rows.
> + */
> +#define I915_FORMAT_MOD_4_TILED fourcc_mod_code(INTEL, 9)
> +
>  /*
>   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>   *
>

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

[Chery, Nanley G]

> -Original Message-
> From: Deak, Imre 
> Sent: Monday, March 21, 2022 6:20 AM
> To: Chery, Nanley G ; Juha-Pekka Heikkila 
> 
> Cc: Nanley Chery ; C, Ramalingam 
> ; intel-gfx ;
> Auld, Matthew ; dri-devel 
> 
> 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
> > > 

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

[Chery, Nanley G]

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

This kind of implies that the Y plane is the main surface, but it's not more
"main" than the UV plane right? Seems like we should specifically call out the
Y plane for clarity. Maybe something like:

For semi-planar formats like NV12, the Y and UV planes are Tile 4 and are 
located at plane indices 0 and 1, respectively. The CCS for all planes are 
stored 
outside of the GEM object

> in a reserved memory area dedicated for the storag

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

[Chery, Nanley G]

> -Original Message-
> From: Juha-Pekka Heikkila 
> Sent: Tuesday, February 15, 2022 6:54 AM
> To: Nanley Chery ; C, Ramalingam
> 
> Cc: intel-gfx ; Chery, Nanley G
> ; Auld, Matthew ; dri-
> devel 
> Subject: Re: [Intel-gfx] [PATCH v5 15/19] drm/i915/dg2: Add DG2 unified
> compression
> 
> On 12.2.2022 3.17, Nanley Chery wrote:
> > On Tue, Feb 1, 2022 at 2:42 AM Ramalingam C 
> wrote:
> >>
> >> From: Matt Roper 
> >>
> >> DG2 unifies render compression and media compression into a single
> >> format for the first time.  The programming and buffer layout is
> >> supposed to match compression on older gen12 platforms, but the
> >> actual compression algorithm is different from any previous platform;
> >> as such, we need a new framebuffer modifier to represent buffers in
> >> this format, but otherwise we can re-use the existing gen12 compression
> driver logic.
> >>
> >> v2:
> >>Display version fix [Imre]
> >>
> >> Signed-off-by: Matt Roper 
> >> cc: Radhakrishna Sripada 
> >> Signed-off-by: Mika Kahola  (v2)
> >> cc: Anshuman Gupta 
> >> Signed-off-by: Juha-Pekka Heikkilä 
> >> Signed-off-by: Ramalingam C 
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_fb.c   | 13 ++
> >>   .../drm/i915/display/skl_universal_plane.c| 26 ---
> >>   include/uapi/drm/drm_fourcc.h | 22 
> >>   3 files changed, 57 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> >> b/drivers/gpu/drm/i915/display/intel_fb.c
> >> index 94c57facbb46..4d4d01963f15 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> >> @@ -141,6 +141,14 @@ struct intel_modifier_desc {
> >>
> >>   static const struct intel_modifier_desc intel_modifiers[] = {
> >>  {
> >> +   .modifier = I915_FORMAT_MOD_4_TILED_DG2_MC_CCS,
> >> +   .display_ver = { 13, 13 },
> >> +   .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> INTEL_PLANE_CAP_CCS_MC,
> >> +   }, {
> >> +   .modifier = I915_FORMAT_MOD_4_TILED_DG2_RC_CCS,
> >> +   .display_ver = { 13, 13 },
> >> +   .plane_caps = INTEL_PLANE_CAP_TILING_4 |
> INTEL_PLANE_CAP_CCS_RC,
> >> +   }, {
> >>  .modifier = I915_FORMAT_MOD_4_TILED,
> >>  .display_ver = { 13, 13 },
> >>  .plane_caps = INTEL_PLANE_CAP_TILING_4, @@ -550,6
> >> +558,8 @@ intel_tile_width_bytes(const struct drm_framebuffer *fb, int
> color_plane)
> >>  return 128;
> >>  else
> >>  return 512;
> >> +   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> >> +   case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> >>  case I915_FORMAT_MOD_4_TILED:
> >>  /*
> >>   * Each 4K tile consists of 64B(8*8) subtiles, with
> >> @@ -752,6 +762,9 @@ unsigned int intel_surf_alignment(const struct
> drm_framebuffer *fb,
> >>  case I915_FORMAT_MOD_4_TILED:
> >>  case I915_FORMAT_MOD_Yf_TILED:
> >>  return 1 * 1024 * 1024;
> >> +   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> >> +   case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> >> +   return 16 * 1024;
> >>  default:
> >>  MISSING_CASE(fb->modifier);
> >>  return 0;
> >> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> index 5299dfe68802..c38ae0876c15 100644
> >> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> >> @@ -764,6 +764,14 @@ static u32 skl_plane_ctl_tiling(u64 fb_modifier)
> >>  return PLANE_CTL_TILED_Y;
> >>  case I915_FORMAT_MOD_4_TILED:
> >>  return PLANE_CTL_TILED_4;
> >> +   case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> >> +   return PLANE_CTL_TILED_4 |
> >> +   PLANE_CTL_RENDER_DECOMPRESSION_ENABLE |
> >> +   PLANE_CTL_CLEAR_COLOR_DISABLE;
> >> +   case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> >> +   return

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

[Chery, Nanley G]

> -Original Message-
> From: Juha-Pekka Heikkila 
> Sent: Tuesday, February 15, 2022 9:32 AM
> To: Chery, Nanley G ; Nanley Chery
> ; C, Ramalingam 
> Cc: intel-gfx ; Auld, Matthew
> ; dri-devel 
> 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 
> >> Sent: Tuesday, February 15, 2022 8:15 AM
> >> To: Chery, Nanley G ; Nanley Chery
> >> ; C, Ramalingam 
> >> Cc: intel-gfx ; Auld, Matthew
> >> ; dri-devel 
> >> 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 
> >>>> Sent: Tuesday, February 15, 2022 6:56 AM
> >>>> To: Nanley Chery ; C, Ramalingam
> >>>> 
> >>>> Cc: intel-gfx ; Chery, Nanley G
> >>>> ; Auld, Matthew ;
> >>>> dri- devel 
> >>>> 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
> >>>>> 
> >>>> wrote:
> >>>>>>
> >>>>>> From: Mika Kahola 
> >>>>>>
> >>>>>> 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 
> >>>>>> cc: Anshuman Gupta 
> >>>>>> Signed-off-by: Juha-Pekka Heikkilä
> >>>>>> 
> >>>>>> Signed-off-by: Ramalingam C 
> >>>>>> ---
> >>>>>> 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_T

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

[Chery, Nanley G]

> -Original Message-
> From: Juha-Pekka Heikkila 
> Sent: Tuesday, February 15, 2022 8:15 AM
> To: Chery, Nanley G ; Nanley Chery
> ; C, Ramalingam 
> Cc: intel-gfx ; Auld, Matthew
> ; dri-devel 
> 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 
> >> Sent: Tuesday, February 15, 2022 6:56 AM
> >> To: Nanley Chery ; C, Ramalingam
> >> 
> >> Cc: intel-gfx ; Chery, Nanley G
> >> ; Auld, Matthew ;
> >> dri- devel 
> >> 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 
> >> wrote:
> >>>>
> >>>> From: Mika Kahola 
> >>>>
> >>>> 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 
> >>>> cc: Anshuman Gupta 
> >>>> Signed-off-by: Juha-Pekka Heikkilä 
> >>>> Signed-off-by: Ramalingam C 
> >>>> ---
> >>>>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 @@ stat

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

[Chery, Nanley G]

> -Original Message-
> From: Juha-Pekka Heikkila 
> Sent: Tuesday, February 15, 2022 6:56 AM
> To: Nanley Chery ; C, Ramalingam
> 
> Cc: intel-gfx ; Chery, Nanley G
> ; Auld, Matthew ; dri-
> devel 
> 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 
> wrote:
> >>
> >> From: Mika Kahola 
> >>
> >> 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 
> >> cc: Anshuman Gupta 
> >> Signed-off-by: Juha-Pekka Heikkilä 
> >> Signed-off-by: Ramalingam C 
> >> ---
> >>   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

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Introduce new Tile 4 format

[Chery, Nanley G]

> -Original Message-
> From: Lisovskiy, Stanislav 
> Sent: Thursday, December 9, 2021 5:47 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org; Lisovskiy, Stanislav
> ; Saarinen, Jani ; C,
> Ramalingam ; ville.syrj...@linux.intel.com; Deak,
> Imre ; Chery, Nanley G 
> Subject: [PATCH 1/2] drm/i915: Introduce new Tile 4 format
> 

We want this patch to be 2/2, right? That way, we expose public kernel support 
for the format after the kernel gains internal support for it. 

With that fixed, this patch is:

Acked-by: Nanley Chery 

Alternatively, you could apply the ack to the prior combined patch if you'd 
like.

-Nanley



> This tiling layout uses 4KB tiles in a row-major layout. It has the same 
> shape as
> Tile Y at two granularities: 4KB (128B x 32) and 64B (16B x 4). It only 
> differs from
> Tile Y at the 256B granularity in between. At this granularity, Tile Y has a 
> shape
> of 16B x 32 rows, but this tiling has a shape of 64B x 8 rows.
> 
> Signed-off-by: Stanislav Lisovskiy 
> ---
>  include/uapi/drm/drm_fourcc.h | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 7f652c96845b..a146c6df1066 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -565,6 +565,17 @@ extern "C" {
>   */
>  #define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC
> fourcc_mod_code(INTEL, 8)
> 
> +/*
> + * Intel Tile 4 layout
> + *
> + * This is a tiled layout using 4KB tiles in a row-major layout. It has
> +the same
> + * shape as Tile Y at two granularities: 4KB (128B x 32) and 64B (16B x
> +4). It
> + * only differs from Tile Y at the 256B granularity in between. At this
> + * granularity, Tile Y has a shape of 16B x 32 rows, but this tiling
> +has a shape
> + * of 64B x 8 rows.
> + */
> +#define I915_FORMAT_MOD_4_TILED fourcc_mod_code(INTEL, 9)
> +
>  /*
>   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>   *
> --
> 2.24.1.485.gad05a3d8e5

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dg2: Tile 4 plane format support

[Chery, Nanley G]

> -Original Message-
> From: Lisovskiy, Stanislav 
> Sent: Thursday, December 9, 2021 5:47 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org; Lisovskiy, Stanislav
> ; Saarinen, Jani ; C,
> Ramalingam ; ville.syrj...@linux.intel.com; Deak,
> Imre ; Chery, Nanley G 
> Subject: [PATCH 2/2] drm/i915/dg2: Tile 4 plane format support
> 
> Tile4 in bspec format is 4K tile organized into 64B subtiles with same basic 
> shape
> as for legacy TileY which will be supported by Display13.
> 
> v2: - Moved Tile4 assocating struct for modifier/display to
>   the beginning(Imre Deak)
> - Removed unneeded case I915_FORMAT_MOD_4_TILED modifier
>   checks(Imre Deak)
> - Fixed I915_FORMAT_MOD_4_TILED to be 9 instead of 12
>   (Imre Deak)
> 
> v3: - Rebased patch on top of new changes related to plane_caps.
> - Added static assert to check that PLANE_CTL_TILING_YF
>   matches PLANE_CTL_TILING_4(Nanley Chery)
> - Fixed naming and layout description for Tile 4 in drm uapi
>   header(Nanley Chery)
> 
> v4: - Extracted drm_fourcc changes to separate patch(Nanley Chery)
> 

Oh, when we discussed this I didn't realize until later that you were asking 
for feedback on whether or not the fourcc changes needed to be split out.
I think either way (combined or separate) is fine. Sorry for the confusion.

-Nanley

> Cc: Matt Roper 
> Cc: Maarten Lankhorst 
> Signed-off-by: Stanislav Lisovskiy 
> Signed-off-by: Matt Roper 
> Signed-off-by: Juha-Pekka Heikkilä 
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
>  drivers/gpu/drm/i915/display/intel_fb.c   | 15 +++-
>  drivers/gpu/drm/i915/display/intel_fb.h   |  1 +
>  drivers/gpu/drm/i915/display/intel_fbc.c  |  1 +
>  .../drm/i915/display/intel_plane_initial.c|  1 +
>  .../drm/i915/display/skl_universal_plane.c| 23 ---
>  drivers/gpu/drm/i915/i915_drv.h   |  1 +
>  drivers/gpu/drm/i915/i915_pci.c   |  1 +
>  drivers/gpu/drm/i915/i915_reg.h   |  1 +
>  drivers/gpu/drm/i915/intel_device_info.h  |  1 +
>  drivers/gpu/drm/i915/intel_pm.c   |  1 +
>  11 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 128d4943a43b..83253c62b6d6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -,6 +,7 @@ static int intel_atomic_check_async(struct
> intel_atomic_state *state, struct int
>   case I915_FORMAT_MOD_X_TILED:
>   case I915_FORMAT_MOD_Y_TILED:
>   case I915_FORMAT_MOD_Yf_TILED:
> + case I915_FORMAT_MOD_4_TILED:
>   break;
>   default:
>   drm_dbg_kms(&i915->drm,
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> b/drivers/gpu/drm/i915/display/intel_fb.c
> index 23cfe2e5ce2a..94c57facbb46 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -135,11 +135,16 @@ struct intel_modifier_desc {
>INTEL_PLANE_CAP_CCS_MC)
>  #define INTEL_PLANE_CAP_TILING_MASK  (INTEL_PLANE_CAP_TILING_X
> | \
>INTEL_PLANE_CAP_TILING_Y | \
> -  INTEL_PLANE_CAP_TILING_Yf)
> +  INTEL_PLANE_CAP_TILING_Yf | \
> +  INTEL_PLANE_CAP_TILING_4)
>  #define INTEL_PLANE_CAP_TILING_NONE  0
> 
>  static const struct intel_modifier_desc intel_modifiers[] = {
>   {
> + .modifier = I915_FORMAT_MOD_4_TILED,
> + .display_ver = { 13, 13 },
> + .plane_caps = INTEL_PLANE_CAP_TILING_4,
> + }, {
>   .modifier = I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS,
>   .display_ver = { 12, 13 },
>   .plane_caps = INTEL_PLANE_CAP_TILING_Y |
> INTEL_PLANE_CAP_CCS_MC, @@ -545,6 +550,12 @@
> intel_tile_width_bytes(const struct drm_framebuffer *fb, int color_plane)
>   return 128;
>   else
>   return 512;
> + case I915_FORMAT_MOD_4_TILED:
> + /*
> +  * Each 4K tile consists of 64B(8*8) subtiles, with
> +  * same shape as Y Tile(i.e 4*16B OWords)
> +  */
> + return 128;
>   case I915_FORMAT_MOD_Y_TILED_CCS:
>   if (intel_fb_is_ccs_aux_plane(fb, color_plane))
>   return 128;
> @@ -650,6 +661,7 @@ static unsigned int intel_f

Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane format support

[Chery, Nanley G]

> -Original Message-
> From: Lisovskiy, Stanislav 
> Sent: Tuesday, November 23, 2021 10:23 AM
> To: Chery, Nanley G 
> Cc: intel-gfx@lists.freedesktop.org; Deak, Imre 
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane format support
> 
> On Tue, Nov 23, 2021 at 05:06:22PM +0200, Chery, Nanley G wrote:
> >
> >
> > > -Original Message-
> > > From: Lisovskiy, Stanislav 
> > > Sent: Tuesday, November 23, 2021 8:37 AM
> > > To: Chery, Nanley G 
> > > Cc: Nanley Chery ;
> > > intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane format
> > > support
> > >
> > > On Tue, Nov 23, 2021 at 02:41:20PM +0200, Chery, Nanley G wrote:
> > > >
> > > >
> > > > > -Original Message-
> > > > > From: Lisovskiy, Stanislav 
> > > > > Sent: Tuesday, November 23, 2021 3:14 AM
> > > > > To: Nanley Chery 
> > > > > Cc: intel-gfx@lists.freedesktop.org; Chery, Nanley G
> > > > > 
> > > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane
> > > > > format support
> > > > >
> > > > > On Mon, Nov 22, 2021 at 05:08:31PM -0500, Nanley Chery wrote:
> > > > > > Hi Stanislav,
> > > > > >
> > > > > > Are there IGT tests for this modifier?
> > > > >
> > > > > Hi Nanley
> > > > >
> > > > > Yes, there should be plenty of those, not sure they are all sent
> > > > > to upstream though.
> > > > > We have a separate team doing this.
> > > > > That modifier should be added to kms_plane_multiple and many
> > > > > others
> > > > >
> > > >
> > > > Okay, I'll be on the lookout for them.
> > > >
> > > > > Stan
> > > > >
> > > >
> > > > Looks like you missed the other review comments I left in my prior 
> > > > email.
> > >
> > > Oh, sorry just saw those. I pushed this already, I will check those
> > > anyway and sent additional patch, if needed.
> > >
> >
> > Where has this been pushed to? We're still requiring an Ack from
> userspace/mesa to get new modifiers upstream, right?
> 
> It was pushed to drm-intel-next. To be honest, I was not aware that this 
> requires
> userspace/mesa ack.
> 

Yeah, I don't think much of the process has been documented. Might be good for 
us
to discuss and/or put something together. The ack was something we did in the 
last
cycle and it seemed like something we were going to do for this one as well.

> How do we proceed then? Should I revert and push the fixed version or do we
> wait until IGT part gets merged?
> 

Reverting and waiting for IGT tests to at least be on the list seems like a 
reasonable
plan to me. I also think it's a good idea to continue waiting on an ack from 
mesa as
well. I'll be OOO this week for holidays, but I'm happy to discuss the latter 
bit when
I get back.

-Nanley

> Stan
> 
> >
> > -Nanley
> >
> > > Stan
> > >
> > > >
> > > > -Nanley
> > > >
> > > > > >
> > > > > > On Mon, Nov 22, 2021 at 4:14 PM Stanislav Lisovskiy
> > > > > >  wrote:
> > > > > > >
> > > > > > > TileF(Tile4 in bspec) format is 4K tile organized into 64B
> > > > > > > subtiles with same basic shape as for legacy TileY which
> > > > > > > will be supported by Display13.
> > > > > > >
> > > > > > > v2: - Fixed wrong case condition(Jani Nikula)
> > > > > > > - Increased I915_FORMAT_MOD_F_TILED up to 12(Imre Deak)
> > > > > > >
> > > > > > > v3: - s/I915_TILING_F/TILING_4/g
> > > > > > > - s/I915_FORMAT_MOD_F_TILED/I915_FORMAT_MOD_4_TILED/g
> > > > > > > - Removed unneeded fencing code
> > > > > > >
> > > > > > > v4: - Rebased, fixed merge conflict with new table-oriented
> > > > > > >   format modifier checking(Stan)
> > > > > > > - Replaced the rest of "Tile F" mentions to "Tile
> > > > > > > 4"(Stan)
> > > > > > >
> > > > > > > v5: - Still had to remove some Tile F mentionings
> > > > > &g

Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane format support

[Chery, Nanley G]

> -Original Message-
> From: Lisovskiy, Stanislav 
> Sent: Tuesday, November 23, 2021 8:37 AM
> To: Chery, Nanley G 
> Cc: Nanley Chery ; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane format support
> 
> On Tue, Nov 23, 2021 at 02:41:20PM +0200, Chery, Nanley G wrote:
> >
> >
> > > -Original Message-
> > > From: Lisovskiy, Stanislav 
> > > Sent: Tuesday, November 23, 2021 3:14 AM
> > > To: Nanley Chery 
> > > Cc: intel-gfx@lists.freedesktop.org; Chery, Nanley G
> > > 
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane format
> > > support
> > >
> > > On Mon, Nov 22, 2021 at 05:08:31PM -0500, Nanley Chery wrote:
> > > > Hi Stanislav,
> > > >
> > > > Are there IGT tests for this modifier?
> > >
> > > Hi Nanley
> > >
> > > Yes, there should be plenty of those, not sure they are all sent to
> > > upstream though.
> > > We have a separate team doing this.
> > > That modifier should be added to kms_plane_multiple and many others
> > >
> >
> > Okay, I'll be on the lookout for them.
> >
> > > Stan
> > >
> >
> > Looks like you missed the other review comments I left in my prior email.
> 
> Oh, sorry just saw those. I pushed this already, I will check those anyway
> and sent additional patch, if needed.
> 

Where has this been pushed to? We're still requiring an Ack from userspace/mesa 
to get new modifiers upstream, right?

-Nanley

> Stan
> 
> >
> > -Nanley
> >
> > > >
> > > > On Mon, Nov 22, 2021 at 4:14 PM Stanislav Lisovskiy
> > > >  wrote:
> > > > >
> > > > > TileF(Tile4 in bspec) format is 4K tile organized into
> > > > > 64B subtiles with same basic shape as for legacy TileY
> > > > > which will be supported by Display13.
> > > > >
> > > > > v2: - Fixed wrong case condition(Jani Nikula)
> > > > > - Increased I915_FORMAT_MOD_F_TILED up to 12(Imre Deak)
> > > > >
> > > > > v3: - s/I915_TILING_F/TILING_4/g
> > > > > - s/I915_FORMAT_MOD_F_TILED/I915_FORMAT_MOD_4_TILED/g
> > > > > - Removed unneeded fencing code
> > > > >
> > > > > v4: - Rebased, fixed merge conflict with new table-oriented
> > > > >   format modifier checking(Stan)
> > > > > - Replaced the rest of "Tile F" mentions to "Tile 4"(Stan)
> > > > >
> > > > > v5: - Still had to remove some Tile F mentionings
> > > > > - Moved has_4tile from adlp to DG2(Ramalingam C)
> > > > > - Check specifically for DG2, but not the Display13(Imre)
> > > > >
> > > > > v6: - Moved Tile4 assocating struct for modifier/display to
> > > > >   the beginning(Imre Deak)
> > > > > - Removed unneeded case I915_FORMAT_MOD_4_TILED modifier
> > > > >   checks(Imre Deak)
> > > > > - Fixed I915_FORMAT_MOD_4_TILED to be 9 instead of 12
> > > > >   (Imre Deak)
> > > > >
> > > > > v7: - Fixed display_ver to { 13, 13 }(Imre Deak)
> > > > > - Removed redundant newline(Imre Deak)
> > > > >
> > > > > Reviewed-by: Imre Deak 
> > > > > Cc: Imre Deak 
> > > > > Cc: Matt Roper 
> > > > > Cc: Maarten Lankhorst 
> > > > > Signed-off-by: Stanislav Lisovskiy 
> > > > > Signed-off-by: Matt Roper 
> > > > > Signed-off-by: Juha-Pekka Heikkilä 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
> > > > >  drivers/gpu/drm/i915/display/intel_fb.c   |  9 +
> > > > >  drivers/gpu/drm/i915/display/intel_fbc.c  |  1 +
> > > > >  .../drm/i915/display/intel_plane_initial.c|  1 +
> > > > >  .../drm/i915/display/skl_universal_plane.c| 20 
> > > > > +++
> > > > >  drivers/gpu/drm/i915/i915_drv.h   |  1 +
> > > > >  drivers/gpu/drm/i915/i915_pci.c   |  1 +
> > > > >  drivers/gpu/drm/i915/i915_reg.h   |  1 +
> > > > >  drivers/gpu/drm/i915/intel_device_info.h  |  1 +
> > > > >  drivers/gpu/drm/i915/intel_pm.c   |  1 +
> > > > >  include/uapi/drm/drm_f

Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane format support

[Chery, Nanley G]

> -Original Message-
> From: Lisovskiy, Stanislav 
> Sent: Tuesday, November 23, 2021 3:14 AM
> To: Nanley Chery 
> Cc: intel-gfx@lists.freedesktop.org; Chery, Nanley G
> 
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/dg2: Tile 4 plane format support
> 
> On Mon, Nov 22, 2021 at 05:08:31PM -0500, Nanley Chery wrote:
> > Hi Stanislav,
> >
> > Are there IGT tests for this modifier?
> 
> Hi Nanley
> 
> Yes, there should be plenty of those, not sure they
> are all sent to upstream though.
> We have a separate team doing this.
> That modifier should be added to kms_plane_multiple
> and many others
> 

Okay, I'll be on the lookout for them.

> Stan
> 

Looks like you missed the other review comments I left in my prior email.

-Nanley

> >
> > On Mon, Nov 22, 2021 at 4:14 PM Stanislav Lisovskiy
> >  wrote:
> > >
> > > TileF(Tile4 in bspec) format is 4K tile organized into
> > > 64B subtiles with same basic shape as for legacy TileY
> > > which will be supported by Display13.
> > >
> > > v2: - Fixed wrong case condition(Jani Nikula)
> > > - Increased I915_FORMAT_MOD_F_TILED up to 12(Imre Deak)
> > >
> > > v3: - s/I915_TILING_F/TILING_4/g
> > > - s/I915_FORMAT_MOD_F_TILED/I915_FORMAT_MOD_4_TILED/g
> > > - Removed unneeded fencing code
> > >
> > > v4: - Rebased, fixed merge conflict with new table-oriented
> > >   format modifier checking(Stan)
> > > - Replaced the rest of "Tile F" mentions to "Tile 4"(Stan)
> > >
> > > v5: - Still had to remove some Tile F mentionings
> > > - Moved has_4tile from adlp to DG2(Ramalingam C)
> > > - Check specifically for DG2, but not the Display13(Imre)
> > >
> > > v6: - Moved Tile4 assocating struct for modifier/display to
> > >   the beginning(Imre Deak)
> > > - Removed unneeded case I915_FORMAT_MOD_4_TILED modifier
> > >   checks(Imre Deak)
> > > - Fixed I915_FORMAT_MOD_4_TILED to be 9 instead of 12
> > >   (Imre Deak)
> > >
> > > v7: - Fixed display_ver to { 13, 13 }(Imre Deak)
> > > - Removed redundant newline(Imre Deak)
> > >
> > > Reviewed-by: Imre Deak 
> > > Cc: Imre Deak 
> > > Cc: Matt Roper 
> > > Cc: Maarten Lankhorst 
> > > Signed-off-by: Stanislav Lisovskiy 
> > > Signed-off-by: Matt Roper 
> > > Signed-off-by: Juha-Pekka Heikkilä 
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
> > >  drivers/gpu/drm/i915/display/intel_fb.c   |  9 +
> > >  drivers/gpu/drm/i915/display/intel_fbc.c  |  1 +
> > >  .../drm/i915/display/intel_plane_initial.c|  1 +
> > >  .../drm/i915/display/skl_universal_plane.c| 20 +++
> > >  drivers/gpu/drm/i915/i915_drv.h   |  1 +
> > >  drivers/gpu/drm/i915/i915_pci.c   |  1 +
> > >  drivers/gpu/drm/i915/i915_reg.h   |  1 +
> > >  drivers/gpu/drm/i915/intel_device_info.h  |  1 +
> > >  drivers/gpu/drm/i915/intel_pm.c   |  1 +
> > >  include/uapi/drm/drm_fourcc.h |  8 
> > >  11 files changed, 37 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> > > index f3c9208a30b1..7429965d3682 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -7766,6 +7766,7 @@ static int intel_atomic_check_async(struct
> intel_atomic_state *state, struct int
> > > case I915_FORMAT_MOD_X_TILED:
> > > case I915_FORMAT_MOD_Y_TILED:
> > > case I915_FORMAT_MOD_Yf_TILED:
> > > +   case I915_FORMAT_MOD_4_TILED:
> > > break;
> > > default:
> > > drm_dbg_kms(&i915->drm,
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
> b/drivers/gpu/drm/i915/display/intel_fb.c
> > > index c4a743d0913f..b7f1ef62072c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > > @@ -139,6 +139,9 @@ struct intel_modifier_desc {
> > >
> > >  static const struct intel_modifier_desc intel_modifiers[] = {
> > > {
> > > +   .modifier = I915_FORMAT_MOD_4_TILED,
>

Re: [Intel-gfx] [PATCH v7 1/3] drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color

[Chery, Nanley G]

> -Original Message-
> From: Imre Deak 
> Sent: Thursday, January 14, 2021 12:13 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Sripada, Radhakrishna ; Ville Syrjala
> ; Pandiyan, Dhinakaran
> ; Kondapally, Kalyan
> ; Rafael Antognolli
> ; Chery, Nanley G
> ; Daniel Vetter ; Nikula,
> Jani 
> Subject: [PATCH v7 1/3] drm/framebuffer: Format modifier for Intel Gen 12
> render compression with Clear Color
> 
> From: Radhakrishna Sripada 
> 
> Gen12 display can decompress surfaces compressed by render engine with
> Clear Color, add a new modifier as the driver needs to know the surface
> was compressed by render engine.
> 
> V2: Description changes as suggested by Rafael.
> V3: Mention the Clear Color size of 64 bits in the comments(DK)
> v4: Fix trailing whitespaces
> v5: Explain Clear Color in the documentation.
> v6: Documentation Nitpicks(Nanley)
> 
> Cc: Ville Syrjala 
> Cc: Dhinakaran Pandiyan 
> Cc: Kalyan Kondapally 
> Cc: Rafael Antognolli 
> Cc: Nanley Chery 
> Signed-off-by: Radhakrishna Sripada 
> Signed-off-by: Imre Deak 
> Acked-by: Daniel Vetter 
> Acked-by: Jani Nikula 
> ---
>  include/uapi/drm/drm_fourcc.h | 19 +++
>  1 file changed, 19 insertions(+)
> 

Acked-by: Nanley Chery 

> diff --git a/include/uapi/drm/drm_fourcc.h
> b/include/uapi/drm/drm_fourcc.h
> index 5f42a14481bd..f76de49c768f 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -527,6 +527,25 @@ extern "C" {
>   */
>  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS
> fourcc_mod_code(INTEL, 7)
> 
> +/*
> + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
> + * compression.
> + *
> + * 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. The clear color structure is 256 bits. The first 128 bits
> + * represents Raw Clear Color Red, Green, Blue and Alpha color each
> represented
> + * by 32 bits. The raw clear color is consumed by the 3d engine and
> generates
> + * the converted clear color of size 64 bits. The first 32 bits store the 
> Lower
> + * Converted Clear Color value and the next 32 bits store the Higher
> Converted
> + * Clear Color value when applicable. The Converted Clear Color values are
> + * consumed by the DE. The last 64 bits are used to store Color Discard
> Enable
> + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache line
> + * corresponds to an area of 4x1 tiles in the main surface. The main surface
> + * pitch is required to be a multiple of 4 tile widths.
> + */
> +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC
> fourcc_mod_code(INTEL, 8)
> +
>  /*
>   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>   *
> --
> 2.25.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color

[Chery, Nanley G]

> -Original Message-
> From: Imre Deak 
> Sent: Tuesday, December 1, 2020 4:05 AM
> To: Chery, Nanley G ; Chris Wilson  wilson.co.uk>; Ville Syrjälä 
> Cc: Daniel Vetter ; intel-gfx@lists.freedesktop.org; Nikula,
> Jani ; Daniel Vetter ;
> Kondapally, Kalyan ; Pandiyan, Dhinakaran
> ; dri-de...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/framebuffer: Format modifier for
> Intel Gen 12 render compression with Clear Color
> 
> Hi Nanley,
> 
> thanks for the review.
> 
> +Ville, Chris.
> 
> On Tue, Dec 01, 2020 at 02:18:26AM +0200, Chery, Nanley G wrote:
> > Hi Imre,
> >
> > I have a question and a couple comments:
> >
> > Is the map of the clear color address creating a new synchronization
> > point between the GPU and CPU? If so, I wonder how this will impact
> > performance.
> 
> The kmap to read the clear value is not adding any sync overhead if
> that's what you mean. But the clear value must be in place before we
> read it out and that should be guaranteed by the flush we do anyway to wait
> for the render result (even considering the explicit L3/RT flush, depth
> stall the spec requires for fast clears).
> 
> However now that you mention: atm the kmap/readout happens after the
> explicit but before the implicit fence-wait. I think it should happen
> after the implicit fence-wait.
> 
> Ville, Chris, could you confirm the above and also that the above flush
> is enough to ensure the CPU read is coherent?
> 
> > There was some talk of asynchronously updating the clear color
> > register a while back.
> 
> Couldn't find anything with a quick search, do you have a pointer? Just
> before the flip we must wait for the render results anyway, as we do
> now, so not sure how it could be optimized.
> 
 
There were some offline discussions, so I don't have a reference unfortunately.
Though, given what you shared above it seems like it's actually not an issue.

> > We probably don't have to update the header, but we noticed in our
> > testing that the clear color prefers an alignment greater than 64B.
> > Unfortunately, I can't find any bspec note about this. As long as the
> > buffer creators are aware though, I think we should be fine. I don't
> > know if this is the best forum to bring it up, but I thought I'd
> > share.
> 
> Yes, would be good to clarify this and get it also to the spec. Then the
> driver should also check the alignment of the 3rd FB plane.
> 

I plan to run some more tests and file a bug in the spec.

I see that the IGT test only clears the fb once. Just to confirm, is the 
clear color offset read from on every frame? Userspace would like to be 
able to pass different clear colors for an fb. 

-Nanley

> > Seems like the upper converted clear color is untested due to the lack
> > of RGBX16 support. I suppose that if there are any issues there, they
> > can be fixed later...
> 
> Yes, a 64bpp RC-CC subtest in IGT is missing, should be easy to add
> that.
> 
> --Imre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/framebuffer: Format modifier for Intel Gen 12 render compression with Clear Color

[Chery, Nanley G]

> -Original Message-
> From: Imre Deak 
> Sent: Friday, November 27, 2020 10:06 AM
> To: Daniel Vetter ; Chery, Nanley G
> 
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani ; 
> Daniel
> Vetter ; Rafael Antognolli
> ; Kondapally, Kalyan
> ; Pandiyan, Dhinakaran
> ; dri-de...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/framebuffer: Format modifier for 
> Intel
> Gen 12 render compression with Clear Color
> 
> On Fri, Nov 27, 2020 at 04:19:20PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 27, 2020 at 04:31:00PM +0200, Imre Deak wrote:
> > > Hi Daniel, Jani,
> > >
> > > is it ok to merge this patch along with 2/2 via the i915 tree?
> >
> > Ack from mesa (userspace in general, but mesa is kinda mandatory) is
> > missing I think. With that
> > Acked-by: Daniel Vetter 
> 
> Thanks.
> 
> Nanley, could you ACK the patchset if they look ok from Mesa's POV? It
> works as expected at least with the igt/kms_ccs RC-CC subtest.
> 

Hi Imre,
 
I have a question and a couple comments:

Is the map of the clear color address creating a new synchronization point 
between the GPU and CPU? If so, I wonder how this will impact performance. 
There was some talk of asynchronously updating the clear color register a while 
back. 

We probably don't have to update the header, but we noticed in our testing that 
the clear color prefers an alignment greater than 64B. Unfortunately, I can't 
find any bspec note about this. As long as the buffer creators are aware 
though, I think we should be fine. I don't know if this is the best forum to 
bring it up, but I thought I'd share.

Seems like the upper converted clear color is untested due to the lack of 
RGBX16 support. I suppose that if there are any issues there, they can be fixed 
later...

-Nanley

> --Imre
> 
> > > On Mon, Nov 23, 2020 at 08:26:30PM +0200, Imre Deak wrote:
> > > > From: Radhakrishna Sripada 
> > > >
> > > > Gen12 display can decompress surfaces compressed by render engine with
> > > > Clear Color, add a new modifier as the driver needs to know the surface
> > > > was compressed by render engine.
> > > >
> > > > V2: Description changes as suggested by Rafael.
> > > > V3: Mention the Clear Color size of 64 bits in the comments(DK)
> > > > v4: Fix trailing whitespaces
> > > > v5: Explain Clear Color in the documentation.
> > > > v6: Documentation Nitpicks(Nanley)
> > > >
> > > > Cc: Ville Syrjala 
> > > > Cc: Dhinakaran Pandiyan 
> > > > Cc: Kalyan Kondapally 
> > > > Cc: Rafael Antognolli 
> > > > Cc: Nanley Chery 
> > > > Signed-off-by: Radhakrishna Sripada 
> > > > Signed-off-by: Imre Deak 
> > > > ---
> > > >  include/uapi/drm/drm_fourcc.h | 19 +++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/include/uapi/drm/drm_fourcc.h
> b/include/uapi/drm/drm_fourcc.h
> > > > index ca48ed0e6bc1..0a1b2c4c4bee 100644
> > > > --- a/include/uapi/drm/drm_fourcc.h
> > > > +++ b/include/uapi/drm/drm_fourcc.h
> > > > @@ -527,6 +527,25 @@ extern "C" {
> > > >   */
> > > >  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS
> fourcc_mod_code(INTEL, 7)
> > > >
> > > > +/*
> > > > + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
> > > > + * compression.
> > > > + *
> > > > + * 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. The clear color structure is 256 bits. The first 128 
> > > > bits
> > > > + * represents Raw Clear Color Red, Green, Blue and Alpha color each
> represented
> > > > + * by 32 bits. The raw clear color is consumed by the 3d engine and
> generates
> > > > + * the converted clear color of size 64 bits. The first 32 bits store 
> > > > the
> Lower
> > > > + * Converted Clear Color value and the next 32 bits store the Higher
> Converted
> > > > + * Clear Color value when applicable. The Converted Clear Color values
> are
> > > > + * consumed by the DE. The last 64 bits are used to store Color Discard
> Enable
> > > > + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache
> line
> > > > + * corresponds to an area of 4x1 t

Re: [Intel-gfx] [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 12 render compression with Clear Color

[Chery, Nanley G]

> From: Chery, Nanley G
> Sent: Tuesday, November 12, 2019 9:40 AM
> To: Sripada, Radhakrishna; intel-gfx@lists.freedesktop.org
> Cc: Pandiyan, Dhinakaran; Syrjala, Ville; Sharma, Shashank; Antognolli, 
> Rafael; Ville Syrjala; Ekstrand, Jason
> Subject: RE: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel 
> Gen 12 render compression with Clear Color
> 
> > From: Sripada, Radhakrishna
> > Sent: Friday, November 01, 2019 12:00 AM
> > To: Chery, Nanley G; intel-gfx@lists.freedesktop.org
> > Cc: Pandiyan, Dhinakaran; Syrjala, Ville; Sharma, Shashank; Antognolli, 
> > Rafael; Ville Syrjala; Ekstrand, Jason
> > Subject: RE: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for 
> > Intel Gen 12 render compression with Clear Color
> >
> > Hi Nanley,
> >
> > > -Original Message-
> > > From: Chery, Nanley G
> > > Sent: Tuesday, October 29, 2019 5:05 PM
> > > To: Sripada, Radhakrishna ; intel-
> > > g...@lists.freedesktop.org
> > > Cc: Pandiyan, Dhinakaran ; Syrjala, Ville
> > > ; Sharma, Shashank
> > > ; Antognolli, Rafael
> > > ; Ville Syrjala 
> > > ;
> > > Ekstrand, Jason 
> > > Subject: RE: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for 
> > > Intel
> > > Gen 12 render compression with Clear Color
> > >
> > > +Jason
> > >
> > > Maybe Jason and/or others have some thoughts on the following? Given the
> > > detailed description of the clear color struct, it seems like we'll have 
> > > to define
> > > a new modifier if the struct fields change in a future generation.
> > >
> > > On negative is that we might have to make new modifiers that provide no
> > > additional benefit (assuming the new/changed fields are unused). On
> > > positive is that it's probably a good thing that we mentioned the Raw 
> > > clear
> > > color fields because I think 3D uses it during rendering operations and 
> > > we'll
> > > be sharing from 3D-to-3D. Maybe we should specify how the channels
> > > should be formatted?
> > >
> > > I haven't thought through how fields like Color Discard Enable affect 
> > > buffer
> > > sharing...
> > >

After some more thought, I don't think it's a problem to have a detailed
description here. We have a lot of space allocated for making new modifiers
(and the header suggests doing so!).

We could go into more detail about the non-DE-specific fields, but I suppose
users can access PRMs.

> > > Another comment: I noticed that none of the Y+CCS modifiers explicitly 
> > > state
> > > that the CCS must be in the same BO as the Y-tiled main surface. We should
> > > at least fix that for newly defined modifiers right?
> > I have not tried to use separate bo for the different surfaces. That is 
> > probably worth a try.
> > I wonder if we are using that for any of the modifiers to have an explicit 
> > comment?
> 
> I didn't think it was possible to use a separate BO for the aux data. AFAICT
> the location of the aux data is specified as an offset from the main surface
> using the PLANE_AUX_DIST register.
> 

Great, I got some clarification that this is actually possible. 

Lastly, can we replace: 

* The raw clear color is consumed by the 3d engine and generates
* the converted clear color of size 64 bits.

with something like:

* The 3D engine can use the raw clear color and the surface format to
* generate a converted clear color of size 64 bits.

so that the subject is constant throughout the sentence and so that it's a bit
more obvious what is in the Converted Clear Color field? Also, we don't
explicitly state that the Converted Clear Color comes right after the raw clear
color, but I'd think the implication is strong enough.

With the above suggestion implemented, this patch is
Reviewed-by: Nanley Chery 

-Nanley

> -Nanley
> 
> >
> > Thanks,
> > RK
> > >
> > > -Nanley
> > >
> > > > 
> > > > From: Sripada, Radhakrishna
> > > > Sent: Monday, October 28, 2019 1:40 PM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Cc: Pandiyan, Dhinakaran; Syrjala, Ville; Sharma, Shashank;
> > > > Antognolli, Rafael; Chery, Nanley G; Sripada, Radhakrishna; Ville
> > > > Syrjala; Kondapally, Kalyan
> > > > Subject: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for
> > > > Intel Gen 12 render compression with Clear Color
> > > >
> > >

Re: [Intel-gfx] [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 12 render compression with Clear Color

[Chery, Nanley G]

> From: Sripada, Radhakrishna
> Sent: Friday, November 01, 2019 12:00 AM
> To: Chery, Nanley G; intel-gfx@lists.freedesktop.org
> Cc: Pandiyan, Dhinakaran; Syrjala, Ville; Sharma, Shashank; Antognolli, 
> Rafael; Ville Syrjala; Ekstrand, Jason
> Subject: RE: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel 
> Gen 12 render compression with Clear Color
> 
> Hi Nanley,
> 
> > -Original Message-
> > From: Chery, Nanley G
> > Sent: Tuesday, October 29, 2019 5:05 PM
> > To: Sripada, Radhakrishna ; intel-
> > g...@lists.freedesktop.org
> > Cc: Pandiyan, Dhinakaran ; Syrjala, Ville
> > ; Sharma, Shashank
> > ; Antognolli, Rafael
> > ; Ville Syrjala 
> > ;
> > Ekstrand, Jason 
> > Subject: RE: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel
> > Gen 12 render compression with Clear Color
> >
> > +Jason
> >
> > Maybe Jason and/or others have some thoughts on the following? Given the
> > detailed description of the clear color struct, it seems like we'll have to 
> > define
> > a new modifier if the struct fields change in a future generation.
> >
> > On negative is that we might have to make new modifiers that provide no
> > additional benefit (assuming the new/changed fields are unused). On
> > positive is that it's probably a good thing that we mentioned the Raw clear
> > color fields because I think 3D uses it during rendering operations and 
> > we'll
> > be sharing from 3D-to-3D. Maybe we should specify how the channels
> > should be formatted?
> >
> > I haven't thought through how fields like Color Discard Enable affect buffer
> > sharing...
> >
> > Another comment: I noticed that none of the Y+CCS modifiers explicitly state
> > that the CCS must be in the same BO as the Y-tiled main surface. We should
> > at least fix that for newly defined modifiers right?
> I have not tried to use separate bo for the different surfaces. That is 
> probably worth a try.
> I wonder if we are using that for any of the modifiers to have an explicit 
> comment?

I didn't think it was possible to use a separate BO for the aux data. AFAICT
the location of the aux data is specified as an offset from the main surface
using the PLANE_AUX_DIST register.

-Nanley

> 
> Thanks,
> RK
> >
> > -Nanley
> >
> > > 
> > > From: Sripada, Radhakrishna
> > > Sent: Monday, October 28, 2019 1:40 PM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: Pandiyan, Dhinakaran; Syrjala, Ville; Sharma, Shashank;
> > > Antognolli, Rafael; Chery, Nanley G; Sripada, Radhakrishna; Ville
> > > Syrjala; Kondapally, Kalyan
> > > Subject: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for
> > > Intel Gen 12 render compression with Clear Color
> > >
> > > Gen12 display can decompress surfaces compressed by render engine with
> > > Clear Color, add a new modifier as the driver needs to know the
> > > surface was compressed by render engine.
> > >
> > > V2: Description changes as suggested by Rafael.
> > > V3: Mention the Clear Color size of 64 bits in the comments(DK)
> > > v4: Fix trailing whitespaces
> > > v5: Explain Clear Color in the documentation.
> > > v6: Documentation Nitpicks(Nanley)
> > >
> > > Cc: Ville Syrjala 
> > > Cc: Dhinakaran Pandiyan 
> > > Cc: Kalyan Kondapally 
> > > Cc: Rafael Antognolli 
> > > Cc: Nanley Chery 
> > > Signed-off-by: Radhakrishna Sripada 
> > > ---
> > >  include/uapi/drm/drm_fourcc.h | 19 +++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/include/uapi/drm/drm_fourcc.h
> > > b/include/uapi/drm/drm_fourcc.h index 1aa6d468c048..4aa7f3f9712a
> > > 100644
> > > --- a/include/uapi/drm/drm_fourcc.h
> > > +++ b/include/uapi/drm/drm_fourcc.h
> > > @@ -434,6 +434,25 @@ extern "C" {
> > >   */
> > >  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS
> > fourcc_mod_code(INTEL,
> > > 7)
> > >
> > > +/*
> > > + * Intel Color Control Surface with Clear Color (CCS) for Gen-12
> > > +render
> > > + * compression.
> > > + *
> > > + * 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. The clear color structure is 256 bits. The first 1

Re: [Intel-gfx] [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 12 render compression with Clear Color

[Chery, Nanley G]

+Jason 

Maybe Jason and/or others have some thoughts on the following? Given the
detailed description of the clear color struct, it seems like we'll have to
define a new modifier if the struct fields change in a future generation.

On negative is that we might have to make new modifiers that provide no
additional benefit (assuming the new/changed fields are unused). On positive is
that it's probably a good thing that we mentioned the Raw clear color fields
because I think 3D uses it during rendering operations and we'll be sharing
from 3D-to-3D. Maybe we should specify how the channels should be formatted? 

I haven't thought through how fields like Color Discard Enable affect buffer
sharing...

Another comment: I noticed that none of the Y+CCS modifiers explicitly state
that the CCS must be in the same BO as the Y-tiled main surface. We should at
least fix that for newly defined modifiers right?

-Nanley

> 
> From: Sripada, Radhakrishna
> Sent: Monday, October 28, 2019 1:40 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Pandiyan, Dhinakaran; Syrjala, Ville; Sharma, Shashank; Antognolli, 
> Rafael; Chery, Nanley G; Sripada, Radhakrishna; Ville Syrjala; Kondapally, 
> Kalyan
> Subject: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 
> 12 render compression with Clear Color
> 
> Gen12 display can decompress surfaces compressed by render engine with
> Clear Color, add a new modifier as the driver needs to know the surface
> was compressed by render engine.
> 
> V2: Description changes as suggested by Rafael.
> V3: Mention the Clear Color size of 64 bits in the comments(DK)
> v4: Fix trailing whitespaces
> v5: Explain Clear Color in the documentation.
> v6: Documentation Nitpicks(Nanley)
> 
> Cc: Ville Syrjala 
> Cc: Dhinakaran Pandiyan 
> Cc: Kalyan Kondapally 
> Cc: Rafael Antognolli 
> Cc: Nanley Chery 
> Signed-off-by: Radhakrishna Sripada 
> ---
>  include/uapi/drm/drm_fourcc.h | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 1aa6d468c048..4aa7f3f9712a 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -434,6 +434,25 @@ extern "C" {
>   */
>  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
> 
> +/*
> + * Intel Color Control Surface with Clear Color (CCS) for Gen-12 render
> + * compression.
> + *
> + * 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. The clear color structure is 256 bits. The first 128 bits
> + * represents Raw Clear Color Red, Green, Blue and Alpha color each 
> represented
> + * by 32 bits. The raw clear color is consumed by the 3d engine and generates
> + * the converted clear color of size 64 bits. The first 32 bits store the 
> Lower
> + * Converted Clear Color value and the next 32 bits store the Higher 
> Converted
> + * Clear Color value when applicable. The Converted Clear Color values are
> + * consumed by the DE. The last 64 bits are used to store Color Discard 
> Enable
> + * and Depth Clear Value Valid which are ignored by the DE. A CCS cache line
> + * corresponds to an area of 4x1 tiles in the main surface. The main surface
> + * pitch is required to be a multiple of 4 tile widths.
> + */
> +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
> +
>  /*
>   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>   *
> --
> 2.20.1
> 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 12 render compression with Clear Color

[Chery, Nanley G]

Hi RK,

> -Original Message-
> From: Sripada, Radhakrishna  
> Sent: Tuesday, October 22, 2019 5:09 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Sripada, Radhakrishna ; Ville Syrjala 
> ; Pandiyan, Dhinakaran 
> ; Kondapally, Kalyan 
> ; Antognolli, Rafael 
> ; Chery, Nanley G 
> Subject: [PATCH v5 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 
> 12 render compression with Clear Color
> 
> Gen12 display can decompress surfaces compressed by render engine with Clear 
> Color, add
> a new modifier as the driver needs to know the surface was compressed by 
> render engine.
> 
> V2: Description changes as suggested by Rafael.
> V3: Mention the Clear Color size of 64 bits in the comments(DK)
> v4: Fix trailing whitespaces
> v5: Explain Clear Color in the documentation.
> 
> Cc: Ville Syrjala 
> Cc: Dhinakaran Pandiyan 
> Cc: Kalyan Kondapally 
> Cc: Rafael Antognolli 
> Cc: Nanley Chery 
> Signed-off-by: Radhakrishna Sripada 
> ---
>  include/uapi/drm/drm_fourcc.h | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 1aa6d468c048..6b4d36e0ccd0 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -434,6 +434,24 @@ extern "C" {
>   */
>  #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
>  
> +/*
> + * Intel color control surfaces Clear Color(CCS_CC) for Gen-12 render 
> compression.

A couple nit-picks here:
- It's not clear to me why "clear color" is capitalized but "color control 
surfaces" is not.
- It also isn't clear why "surfaces" is plural here but singular in the 
Y_TILED_CCS comment.
- A space is missing between "Color" and "(CCS_CC)". 
- Perhaps insert a "with" between "color control surface" and "clear color"?

> + *
> + * The main surface is Y-tiled and is at plane index 0 whereas CCS_CC is 
> linear
> + * and at index 1. The clear color is stored at index 2, and the pitch should

I thought "CCS_CC" would be referring to the CCS with clear color, not the CCS
by itself. I can't find this term in the BSpec. Perhaps we shouldn't create a
new one here to avoid confusion?
 
-Nanley

> + * be ignored. The clear color structure is 256 bits. The first 128 bits 
> represents
> + * Raw Clear Color Red, Green, Blue and Alpha color each represented by 32 
> bits.
> + * The raw clear color is consumed by the 3d engine and generates the 
> converted
> + * clear color of size 64 bits. The first 32 bits store the Lower Converted 
> Clear
> + * Color value and the next 32 bits store the Higher Converted Clear Color 
> value
> + * when applicable. The Converted Clear Color values are consumed by the DE. 
> The
> + * last 64 bits are used to store Color Discard Enable and Depth Clear Value 
> Valid
> + * which are ignored by the DE. A CCS_CC cache line corresponds to an area 
> of 4x1
> + * tiles in the main surface. The main surface pitch is required to be a 
> multiple
> + * of 4 tile widths.
> + */
> +#define I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC fourcc_mod_code(INTEL, 8)
> +
>  /*
>   * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
>   *
> -- 
> 2.20.1
> 
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx