Re: [PATCH] drm/i915: Support RGB16161616_64B compressed formats

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 05, 2024 at 11:34:57AM +0530, Melanie Lobo wrote:
> Add support for a RGB64(FP16) format where each color component is a
> 16-bit floating point value. FP16 format which is a binary
> floating-point computer number format that occupies 16 bits in computer
> memory. Platform shall render compression in display engine to receive
> FP16 compressed formats.
> 
> This kernel change was tested with IGT patch,
> https://patchwork.freedesktop.org/series/134353/
> https://lore.kernel.org/all/20240603081607.30930-1-melanie.lobo@intel.com/
> 
> Test-with: 20240603081607.30930-1-melanie.lobo@intel.com
> 
> Credits: Juha-Pekka <juha-pekka.heikkila@intel.com>
> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> Cc: Swati Sharma <swati2.sharma@intel.com>
> Signed-off-by: Melanie Lobo <melanie.lobo@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fb.c            | 5 +++++
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 4 ++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index b6638726949d..91f2def14243 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -91,6 +91,11 @@ static const struct drm_format_info gen12_ccs_formats[] = {
>  	{ .format = DRM_FORMAT_P016, .num_planes = 4,
>  	  .char_per_block = { 2, 4, 1, 1 }, .block_w = { 1, 1, 2, 2 }, .block_h = { 1, 1, 1, 1 },
>  	  .hsub = 2, .vsub = 2, .is_yuv = true },

I'd put the fp16 formats between the other RGB formats and YCbCr
formats.

> +	{ .format = DRM_FORMAT_XRGB16161616F, .depth = 64, .num_planes = 2,

__drm_format_info() leaves depth==0 for fp16 formats. These should
do the same.

> +	  .char_per_block = { 8, 1}, .block_w = { 1, 4}, .block_h = { 1, 2}, .hsub = 1, .vsub = 1},

Everyone else wraps the hsub/vsub to the next line. Also you
consistently leave out the space before '}', unlike every other
line in this file.

> +	{ .format = DRM_FORMAT_ARGB16161616F, .depth = 64, .num_planes = 2,
> +	  .char_per_block = { 8, 1}, .block_w = { 1, 4}, .block_h = { 1, 2},

AUX block width should be 1 for these, block height we don't actually use I
believe, but everone else sets it to one so this should do the same.

> +	  .hsub = 1, .vsub = 1, .has_alpha = true},

You are missing the X/ABGR variants. Also missing the addition of
these formats to gen12_ccs_cc_formats[] and
gen12_flat_ccs_cc_formats[].

See my equivalent patch for the 10bpc formats:
https://patchwork.freedesktop.org/patch/600652/?series=135306&rev=1

>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 1aa70fc35b9d..7719cb04bdf8 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -2242,6 +2242,8 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
>  	case DRM_FORMAT_XBGR8888:
>  	case DRM_FORMAT_ARGB8888:
>  	case DRM_FORMAT_ABGR8888:
> +	case DRM_FORMAT_XRGB16161616F:
> +	case DRM_FORMAT_ARGB16161616F:
>  		if (intel_fb_is_ccs_modifier(modifier))
>  			return true;
>  		fallthrough;
> @@ -2266,8 +2268,6 @@ static bool gen12_plane_format_mod_supported(struct drm_plane *_plane,
>  	case DRM_FORMAT_C8:
>  	case DRM_FORMAT_XBGR16161616F:
>  	case DRM_FORMAT_ABGR16161616F:
> -	case DRM_FORMAT_XRGB16161616F:
> -	case DRM_FORMAT_ARGB16161616F:
>  	case DRM_FORMAT_Y210:
>  	case DRM_FORMAT_Y212:
>  	case DRM_FORMAT_Y216:
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel