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/[email protected]/
>
> Test-with: [email protected]
>
> Credits: Juha-Pekka <[email protected]>
> Cc: Juha-Pekka Heikkila <[email protected]>
> Cc: Bhanuprakash Modem <[email protected]>
> Cc: Swati Sharma <[email protected]>
> Signed-off-by: Melanie Lobo <[email protected]>
> ---
> 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
