Re: [RFC PATCH 1/3] drm/color: Add RGB Color encodings

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Harry Wentland <harry.wentland@amd.com>
Cc: Deepak.Sharma@amd.com, Krunoslav.Kovac@amd.com,
	mcasas@google.com, Shashank.Sharma@amd.com,
	dri-devel@lists.freedesktop.org, Shirish.S@amd.com,
	sebastian@sebastianwick.net, Uma Shankar <uma.shankar@intel.com>,
	hersenxs.wu@amd.com, amd-gfx@lists.freedesktop.org,
	laurentiu.palcu@oss.nxp.com, Bhawanpreet.Lakha@amd.com,
	Nicholas.Kazlauskas@amd.com, Vitaly.Prosyak@amd.com
Subject: Re: [RFC PATCH 1/3] drm/color: Add RGB Color encodings
Date: Mon, 26 Apr 2021 21:07:14 +0300	[thread overview]
Message-ID: <YIcBUl+94sHJsT8B@intel.com> (raw)
In-Reply-To: <20210426173852.484368-2-harry.wentland@amd.com>

On Mon, Apr 26, 2021 at 01:38:50PM -0400, Harry Wentland wrote:
> From: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
> 
> Add the following color encodings
> - RGB versions for BT601, BT709, BT2020
> - DCI-P3: Used for digital movies
> 
> Signed-off-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 4 ++++
>  include/drm/drm_color_mgmt.h     | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index bb14f488c8f6..a183ebae2941 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -469,6 +469,10 @@ static const char * const color_encoding_name[] = {
>  	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
>  	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
>  	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
> +	[DRM_COLOR_RGB_BT601] = "ITU-R BT.601 RGB",
> +	[DRM_COLOR_RGB_BT709] = "ITU-R BT.709 RGB",
> +	[DRM_COLOR_RGB_BT2020] = "ITU-R BT.2020 RGB",
> +	[DRM_COLOR_P3] = "DCI-P3",

These are a totally different thing than the YCbCr stuff.
The YCbCr stuff just specifies the YCbCr<->RGB converison matrix,
whereas these are I guess supposed to specify the primaries/whitepoint?
But without specifying what we're converting *to* these mean absolutely
nothing. Ie. I don't think they belong in this property.

The previous proposals around this topic have suggested a new
property to specify a conversion matrix either explicitly, or
via a separate enum (which would specify both the src and dst
colorspaces). I've always argued the enum approach is needed
anyway since not all hardware has a programmable matrix for
this. But a fully programmable matrix could be nice for tone
mapping purposes/etc, so we may want to make sure both are
possible.

As for the transfer func, the proposals so far have mostly just
been to expose a programmable degamma/gamma LUTs for each plane.
But considering how poor the current gamma uapi is we've thrown
around some ideas how to allow the kernel to properly expose the
hw capabilities. This is one of those ideas:
https://lists.freedesktop.org/archives/dri-devel/2019-April/212886.html
I think that basic idea could be also be extended to allow fixed
curves in case the hw doesn't have a fully programmable LUT. But
dunno if that's relevant for your hw.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel