[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 <radhakrishna.sripada at intel.com> 
> Sent: Tuesday, October 22, 2019 5:09 PM
> To: intel-gfx at lists.freedesktop.org
> Cc: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; Ville Syrjala <ville.syrjala at linux.intel.com>; Pandiyan, Dhinakaran <dhinakaran.pandiyan at intel.com>; Kondapally, Kalyan <kalyan.kondapally at intel.com>; Antognolli, Rafael <rafael.antognolli at intel.com>; Chery, Nanley G <nanley.g.chery at intel.com>
> 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 <ville.syrjala at linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Cc: Kalyan Kondapally <kalyan.kondapally at intel.com>
> Cc: Rafael Antognolli <rafael.antognolli at intel.com>
> Cc: Nanley Chery <nanley.g.chery at intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> ---
>  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
> 
>