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

From: "Chery, Nanley G" <nanley.g.chery@intel.com>
To: "Sripada, Radhakrishna" <radhakrishna.sripada@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: "Syrjala, Ville" <ville.syrjala@intel.com>,
	"Ekstrand, Jason" <jason.ekstrand@intel.com>,
	"Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Subject: Re: [PATCH v6 09/10] drm/framebuffer/tgl: Format modifier for Intel Gen 12 render compression with Clear Color
Date: Wed, 30 Oct 2019 00:05:13 +0000	[thread overview]
Message-ID: <8B74F386D1656545A7968FB3C487BECC2B6F3C52@ORSMSX116.amr.corp.intel.com> (raw)
In-Reply-To: <20191028204041.13507-10-radhakrishna.sripada@intel.com>

+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 <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
> Cc: Rafael Antognolli <rafael.antognolli@intel.com>
> Cc: Nanley Chery <nanley.g.chery@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
>  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