[PATCH 0/3] drm/connector: hdmi: Allow using the YUV420 output format

[PATCH 0/3] drm/connector: hdmi: Allow using the YUV420 output format

[Cristian Ciocaltea]

Provide the basic support to enable using YUV420 as an RGB fallback when
computing the best output format and color depth.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Cristian Ciocaltea (3):
      drm/connector: hdmi: Evaluate limited range after computing format
      drm/connector: hdmi: Add support for YUV420 format verification
      drm/connector: hdmi: Use YUV420 output format as an RGB fallback

 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 55 ++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 10 deletions(-)
---
base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
change-id: 20241130-hdmi-conn-yuv-e1fa596df768

[PATCH 1/3] drm/connector: hdmi: Evaluate limited range after computing format

[Cristian Ciocaltea]

Evaluating the requirement to use a limited RGB quantization range
involves a verification of the output format, among others, but this is
currently performed before actually computing the format, hence relying
on the old connector state.

Move the call to hdmi_is_limited_range() after hdmi_compute_config() to
ensure the verification is done on the updated output format.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index feb7a3a759811aed70c679be8704072093e2a79b..0cbcee7e77cd8dff387044487ce28ee5748f5587 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -494,12 +494,12 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
 		connector_state_get_mode(new_conn_state);
 	int ret;
 
-	new_conn_state->hdmi.is_limited_range = hdmi_is_limited_range(connector, new_conn_state);
-
 	ret = hdmi_compute_config(connector, new_conn_state, mode);
 	if (ret)
 		return ret;
 
+	new_conn_state->hdmi.is_limited_range = hdmi_is_limited_range(connector, new_conn_state);
+
 	ret = hdmi_generate_infoframes(connector, new_conn_state);
 	if (ret)
 		return ret;

-- 
2.47.0

Re: [PATCH 1/3] drm/connector: hdmi: Evaluate limited range after computing format

[Dmitry Baryshkov]

On Sat, Nov 30, 2024 at 01:56:32AM +0200, Cristian Ciocaltea wrote:
> Evaluating the requirement to use a limited RGB quantization range
> involves a verification of the output format, among others, but this is
> currently performed before actually computing the format, hence relying
> on the old connector state.
> 
> Move the call to hdmi_is_limited_range() after hdmi_compute_config() to
> ensure the verification is done on the updated output format.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Fixes: 027d43590649 ("drm/connector: hdmi: Add RGB Quantization Range to the connector state")

> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index feb7a3a759811aed70c679be8704072093e2a79b..0cbcee7e77cd8dff387044487ce28ee5748f5587 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -494,12 +494,12 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
>  		connector_state_get_mode(new_conn_state);
>  	int ret;
>  
> -	new_conn_state->hdmi.is_limited_range = hdmi_is_limited_range(connector, new_conn_state);
> -
>  	ret = hdmi_compute_config(connector, new_conn_state, mode);
>  	if (ret)
>  		return ret;
>  
> +	new_conn_state->hdmi.is_limited_range = hdmi_is_limited_range(connector, new_conn_state);
> +
>  	ret = hdmi_generate_infoframes(connector, new_conn_state);
>  	if (ret)
>  		return ret;
> 
> -- 
> 2.47.0
> 

-- 
With best wishes
Dmitry

[PATCH 2/3] drm/connector: hdmi: Add support for YUV420 format verification

[Cristian Ciocaltea]

Provide the necessary constraints verification in
sink_supports_format_bpc() in order to support handling of YUV420
output format.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 40 +++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 0cbcee7e77cd8dff387044487ce28ee5748f5587..3a55881a544a519bb1254968db891c814f831a0f 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -3,6 +3,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_modes.h>
 #include <drm/drm_print.h>
 
 #include <drm/display/drm_hdmi_helper.h>
@@ -114,6 +115,12 @@ sink_supports_format_bpc(const struct drm_connector *connector,
 		return false;
 	}
 
+	if (drm_mode_is_420_only(info, mode) && format != HDMI_COLORSPACE_YUV420) {
+		drm_dbg_kms(dev, "%s format unsupported by the sink for VIC%u.\n",
+			    drm_hdmi_connector_get_output_format_name(format), vic);
+		return false;
+	}
+
 	switch (format) {
 	case HDMI_COLORSPACE_RGB:
 		drm_dbg_kms(dev, "RGB Format, checking the constraints.\n");
@@ -144,9 +151,36 @@ sink_supports_format_bpc(const struct drm_connector *connector,
 		return true;
 
 	case HDMI_COLORSPACE_YUV420:
-		/* TODO: YUV420 is unsupported at the moment. */
-		drm_dbg_kms(dev, "YUV420 format isn't supported yet.\n");
-		return false;
+		drm_dbg_kms(dev, "YUV420 format, checking the constraints.\n");
+
+		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR420)) {
+			drm_dbg_kms(dev, "Sink doesn't support YUV420.\n");
+			return false;
+		}
+
+		if (!drm_mode_is_420(info, mode)) {
+			drm_dbg_kms(dev, "Sink doesn't support YUV420 for VIC%u.\n", vic);
+			return false;
+		}
+
+		if (bpc == 10 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_30)) {
+			drm_dbg_kms(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
+			return false;
+		}
+
+		if (bpc == 12 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) {
+			drm_dbg_kms(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
+			return false;
+		}
+
+		if (bpc == 16 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_48)) {
+			drm_dbg_kms(dev, "16 BPC but sink doesn't support Deep Color 48.\n");
+			return false;
+		}
+
+		drm_dbg_kms(dev, "YUV420 format supported in that configuration.\n");
+
+		return true;
 
 	case HDMI_COLORSPACE_YUV422:
 		drm_dbg_kms(dev, "YUV422 format, checking the constraints.\n");

-- 
2.47.0

Re: [PATCH 2/3] drm/connector: hdmi: Add support for YUV420 format verification

[Dmitry Baryshkov]

On Sat, Nov 30, 2024 at 01:56:33AM +0200, Cristian Ciocaltea wrote:
> Provide the necessary constraints verification in
> sink_supports_format_bpc() in order to support handling of YUV420
> output format.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 40 +++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 3 deletions(-)

LGTM, please add KUnit tests.

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/connector: hdmi: Add support for YUV420 format verification

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2896 bytes --]

On Sat, Nov 30, 2024 at 01:56:33AM +0200, Cristian Ciocaltea wrote:
> Provide the necessary constraints verification in
> sink_supports_format_bpc() in order to support handling of YUV420
> output format.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 40 +++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 0cbcee7e77cd8dff387044487ce28ee5748f5587..3a55881a544a519bb1254968db891c814f831a0f 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -3,6 +3,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_connector.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_modes.h>
>  #include <drm/drm_print.h>
>  
>  #include <drm/display/drm_hdmi_helper.h>
> @@ -114,6 +115,12 @@ sink_supports_format_bpc(const struct drm_connector *connector,
>  		return false;
>  	}
>  
> +	if (drm_mode_is_420_only(info, mode) && format != HDMI_COLORSPACE_YUV420) {
> +		drm_dbg_kms(dev, "%s format unsupported by the sink for VIC%u.\n",
> +			    drm_hdmi_connector_get_output_format_name(format), vic);
> +		return false;
> +	}
> +
>  	switch (format) {
>  	case HDMI_COLORSPACE_RGB:
>  		drm_dbg_kms(dev, "RGB Format, checking the constraints.\n");
> @@ -144,9 +151,36 @@ sink_supports_format_bpc(const struct drm_connector *connector,
>  		return true;
>  
>  	case HDMI_COLORSPACE_YUV420:
> -		/* TODO: YUV420 is unsupported at the moment. */
> -		drm_dbg_kms(dev, "YUV420 format isn't supported yet.\n");
> -		return false;
> +		drm_dbg_kms(dev, "YUV420 format, checking the constraints.\n");
> +
> +		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR420)) {
> +			drm_dbg_kms(dev, "Sink doesn't support YUV420.\n");
> +			return false;
> +		}
> +
> +		if (!drm_mode_is_420(info, mode)) {
> +			drm_dbg_kms(dev, "Sink doesn't support YUV420 for VIC%u.\n", vic);
> +			return false;
> +		}
> +
> +		if (bpc == 10 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_30)) {
> +			drm_dbg_kms(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
> +			return false;
> +		}
> +
> +		if (bpc == 12 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) {
> +			drm_dbg_kms(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
> +			return false;
> +		}
> +
> +		if (bpc == 16 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_48)) {
> +			drm_dbg_kms(dev, "16 BPC but sink doesn't support Deep Color 48.\n");
> +			return false;
> +		}
> +
> +		drm_dbg_kms(dev, "YUV420 format supported in that configuration.\n");
> +
> +		return true;

We also need to check whether the source supports it or not.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 2/3] drm/connector: hdmi: Add support for YUV420 format verification

[Cristian Ciocaltea]

Hi Maxime,

On 12/2/24 12:50 PM, Maxime Ripard wrote:
> On Sat, Nov 30, 2024 at 01:56:33AM +0200, Cristian Ciocaltea wrote:
>> Provide the necessary constraints verification in
>> sink_supports_format_bpc() in order to support handling of YUV420
>> output format.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 40 +++++++++++++++++++++++--
>>  1 file changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> index 0cbcee7e77cd8dff387044487ce28ee5748f5587..3a55881a544a519bb1254968db891c814f831a0f 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> @@ -3,6 +3,7 @@
>>  #include <drm/drm_atomic.h>
>>  #include <drm/drm_connector.h>
>>  #include <drm/drm_edid.h>
>> +#include <drm/drm_modes.h>
>>  #include <drm/drm_print.h>
>>  
>>  #include <drm/display/drm_hdmi_helper.h>
>> @@ -114,6 +115,12 @@ sink_supports_format_bpc(const struct drm_connector *connector,
>>  		return false;
>>  	}
>>  
>> +	if (drm_mode_is_420_only(info, mode) && format != HDMI_COLORSPACE_YUV420) {
>> +		drm_dbg_kms(dev, "%s format unsupported by the sink for VIC%u.\n",
>> +			    drm_hdmi_connector_get_output_format_name(format), vic);
>> +		return false;
>> +	}
>> +
>>  	switch (format) {
>>  	case HDMI_COLORSPACE_RGB:
>>  		drm_dbg_kms(dev, "RGB Format, checking the constraints.\n");
>> @@ -144,9 +151,36 @@ sink_supports_format_bpc(const struct drm_connector *connector,
>>  		return true;
>>  
>>  	case HDMI_COLORSPACE_YUV420:
>> -		/* TODO: YUV420 is unsupported at the moment. */
>> -		drm_dbg_kms(dev, "YUV420 format isn't supported yet.\n");
>> -		return false;
>> +		drm_dbg_kms(dev, "YUV420 format, checking the constraints.\n");
>> +
>> +		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR420)) {
>> +			drm_dbg_kms(dev, "Sink doesn't support YUV420.\n");
>> +			return false;
>> +		}
>> +
>> +		if (!drm_mode_is_420(info, mode)) {
>> +			drm_dbg_kms(dev, "Sink doesn't support YUV420 for VIC%u.\n", vic);
>> +			return false;
>> +		}
>> +
>> +		if (bpc == 10 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_30)) {
>> +			drm_dbg_kms(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
>> +			return false;
>> +		}
>> +
>> +		if (bpc == 12 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) {
>> +			drm_dbg_kms(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
>> +			return false;
>> +		}
>> +
>> +		if (bpc == 16 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_48)) {
>> +			drm_dbg_kms(dev, "16 BPC but sink doesn't support Deep Color 48.\n");
>> +			return false;
>> +		}
>> +
>> +		drm_dbg_kms(dev, "YUV420 format supported in that configuration.\n");
>> +
>> +		return true;
> 
> We also need to check whether the source supports it or not.

I assumed the following check does already handle that:

	if (!(connector->hdmi.supported_formats & BIT(format))) {
		drm_dbg_kms(dev, "%s format unsupported by the connector.\n",

Is there anything else missing?

Thanks,
Cristian

Re: [PATCH 2/3] drm/connector: hdmi: Add support for YUV420 format verification

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3526 bytes --]

On Mon, Dec 02, 2024 at 01:15:36PM +0200, Cristian Ciocaltea wrote:
> Hi Maxime,
> 
> On 12/2/24 12:50 PM, Maxime Ripard wrote:
> > On Sat, Nov 30, 2024 at 01:56:33AM +0200, Cristian Ciocaltea wrote:
> >> Provide the necessary constraints verification in
> >> sink_supports_format_bpc() in order to support handling of YUV420
> >> output format.
> >>
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >> ---
> >>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 40 +++++++++++++++++++++++--
> >>  1 file changed, 37 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >> index 0cbcee7e77cd8dff387044487ce28ee5748f5587..3a55881a544a519bb1254968db891c814f831a0f 100644
> >> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> >> @@ -3,6 +3,7 @@
> >>  #include <drm/drm_atomic.h>
> >>  #include <drm/drm_connector.h>
> >>  #include <drm/drm_edid.h>
> >> +#include <drm/drm_modes.h>
> >>  #include <drm/drm_print.h>
> >>  
> >>  #include <drm/display/drm_hdmi_helper.h>
> >> @@ -114,6 +115,12 @@ sink_supports_format_bpc(const struct drm_connector *connector,
> >>  		return false;
> >>  	}
> >>  
> >> +	if (drm_mode_is_420_only(info, mode) && format != HDMI_COLORSPACE_YUV420) {
> >> +		drm_dbg_kms(dev, "%s format unsupported by the sink for VIC%u.\n",
> >> +			    drm_hdmi_connector_get_output_format_name(format), vic);
> >> +		return false;
> >> +	}
> >> +
> >>  	switch (format) {
> >>  	case HDMI_COLORSPACE_RGB:
> >>  		drm_dbg_kms(dev, "RGB Format, checking the constraints.\n");
> >> @@ -144,9 +151,36 @@ sink_supports_format_bpc(const struct drm_connector *connector,
> >>  		return true;
> >>  
> >>  	case HDMI_COLORSPACE_YUV420:
> >> -		/* TODO: YUV420 is unsupported at the moment. */
> >> -		drm_dbg_kms(dev, "YUV420 format isn't supported yet.\n");
> >> -		return false;
> >> +		drm_dbg_kms(dev, "YUV420 format, checking the constraints.\n");
> >> +
> >> +		if (!(info->color_formats & DRM_COLOR_FORMAT_YCBCR420)) {
> >> +			drm_dbg_kms(dev, "Sink doesn't support YUV420.\n");
> >> +			return false;
> >> +		}
> >> +
> >> +		if (!drm_mode_is_420(info, mode)) {
> >> +			drm_dbg_kms(dev, "Sink doesn't support YUV420 for VIC%u.\n", vic);
> >> +			return false;
> >> +		}
> >> +
> >> +		if (bpc == 10 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_30)) {
> >> +			drm_dbg_kms(dev, "10 BPC but sink doesn't support Deep Color 30.\n");
> >> +			return false;
> >> +		}
> >> +
> >> +		if (bpc == 12 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) {
> >> +			drm_dbg_kms(dev, "12 BPC but sink doesn't support Deep Color 36.\n");
> >> +			return false;
> >> +		}
> >> +
> >> +		if (bpc == 16 && !(info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_48)) {
> >> +			drm_dbg_kms(dev, "16 BPC but sink doesn't support Deep Color 48.\n");
> >> +			return false;
> >> +		}
> >> +
> >> +		drm_dbg_kms(dev, "YUV420 format supported in that configuration.\n");
> >> +
> >> +		return true;
> > 
> > We also need to check whether the source supports it or not.
> 
> I assumed the following check does already handle that:
> 
> 	if (!(connector->hdmi.supported_formats & BIT(format))) {
> 		drm_dbg_kms(dev, "%s format unsupported by the connector.\n",
> 
> Is there anything else missing?

You're right, sorry for the noise :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[PATCH 3/3] drm/connector: hdmi: Use YUV420 output format as an RGB fallback

[Cristian Ciocaltea]

Introduce the switch to YUV420 when computing the best output format and
RGB cannot be supported for a given color depth.

While at it, add a minor improvement to the debug message indicating the
supported format.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 3a55881a544a519bb1254968db891c814f831a0f..b4e865e0680f35fd2d849536789f6c1f98a48258 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -304,7 +304,7 @@ hdmi_try_format_bpc(const struct drm_connector *connector,
 		return false;
 	}
 
-	drm_dbg_kms(dev, "%s output format supported with %u (TMDS char rate: %llu Hz)\n",
+	drm_dbg_kms(dev, "%s output format supported with %u bpc (TMDS char rate: %llu Hz)\n",
 		    drm_hdmi_connector_get_output_format_name(fmt),
 		    bpc, conn_state->hdmi.tmds_char_rate);
 
@@ -319,15 +319,16 @@ hdmi_compute_format(const struct drm_connector *connector,
 {
 	struct drm_device *dev = connector->dev;
 
-	/*
-	 * TODO: Add support for YCbCr420 output for HDMI 2.0 capable
-	 * devices, for modes that only support YCbCr420.
-	 */
 	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_RGB)) {
 		conn_state->hdmi.output_format = HDMI_COLORSPACE_RGB;
 		return 0;
 	}
 
+	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_YUV420)) {
+		conn_state->hdmi.output_format = HDMI_COLORSPACE_YUV420;
+		return 0;
+	}
+
 	drm_dbg_kms(dev, "Failed. No Format Supported for that bpc count.\n");
 
 	return -EINVAL;

-- 
2.47.0

Re: [PATCH 3/3] drm/connector: hdmi: Use YUV420 output format as an RGB fallback

[Dmitry Baryshkov]

On Sat, Nov 30, 2024 at 01:56:34AM +0200, Cristian Ciocaltea wrote:
> Introduce the switch to YUV420 when computing the best output format and
> RGB cannot be supported for a given color depth.
> 
> While at it, add a minor improvement to the debug message indicating the
> supported format.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 3a55881a544a519bb1254968db891c814f831a0f..b4e865e0680f35fd2d849536789f6c1f98a48258 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -304,7 +304,7 @@ hdmi_try_format_bpc(const struct drm_connector *connector,
>  		return false;
>  	}
>  
> -	drm_dbg_kms(dev, "%s output format supported with %u (TMDS char rate: %llu Hz)\n",
> +	drm_dbg_kms(dev, "%s output format supported with %u bpc (TMDS char rate: %llu Hz)\n",

Correct, but irrelevant. Please split to a separate commit.

>  		    drm_hdmi_connector_get_output_format_name(fmt),
>  		    bpc, conn_state->hdmi.tmds_char_rate);
>  
> @@ -319,15 +319,16 @@ hdmi_compute_format(const struct drm_connector *connector,
>  {
>  	struct drm_device *dev = connector->dev;
>  
> -	/*
> -	 * TODO: Add support for YCbCr420 output for HDMI 2.0 capable
> -	 * devices, for modes that only support YCbCr420.
> -	 */
>  	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_RGB)) {
>  		conn_state->hdmi.output_format = HDMI_COLORSPACE_RGB;
>  		return 0;
>  	}
>  
> +	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_YUV420)) {
> +		conn_state->hdmi.output_format = HDMI_COLORSPACE_YUV420;
> +		return 0;
> +	}

Should we prefer YCbCr 4:2:0 over RGB?

> +
>  	drm_dbg_kms(dev, "Failed. No Format Supported for that bpc count.\n");
>  
>  	return -EINVAL;
> 
> -- 
> 2.47.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/connector: hdmi: Use YUV420 output format as an RGB fallback

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3304 bytes --]

On Sat, Nov 30, 2024 at 01:56:34AM +0200, Cristian Ciocaltea wrote:
> Introduce the switch to YUV420 when computing the best output format and
> RGB cannot be supported for a given color depth.
> 
> While at it, add a minor improvement to the debug message indicating the
> supported format.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 3a55881a544a519bb1254968db891c814f831a0f..b4e865e0680f35fd2d849536789f6c1f98a48258 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -304,7 +304,7 @@ hdmi_try_format_bpc(const struct drm_connector *connector,
>  		return false;
>  	}
>  
> -	drm_dbg_kms(dev, "%s output format supported with %u (TMDS char rate: %llu Hz)\n",
> +	drm_dbg_kms(dev, "%s output format supported with %u bpc (TMDS char rate: %llu Hz)\n",
>  		    drm_hdmi_connector_get_output_format_name(fmt),
>  		    bpc, conn_state->hdmi.tmds_char_rate);
>  
> @@ -319,15 +319,16 @@ hdmi_compute_format(const struct drm_connector *connector,
>  {
>  	struct drm_device *dev = connector->dev;
>  
> -	/*
> -	 * TODO: Add support for YCbCr420 output for HDMI 2.0 capable
> -	 * devices, for modes that only support YCbCr420.
> -	 */

It's something that I had in the back of my mind for a while, but we're
at the point where we need to discuss this I guess :)

Not all HDMI controllers are HDMI2.0+ compliant, and we need to gatekeep
this to the fact the controller supports it.

This will also be useful for things like scrambling support. And
probably to provide some TMDS rate check based on the standard a given
controller supports, since most of the drivers have that check
duplicated everywhere.

I don't really have an opinion on how to do this, so I guess it's really
up for debate. The alternatives I could think of are either to add a new
parameter to the init function, or to create a new callback to query the
driver for its capabilities.

The former doesn't seem great since the parameters set is pretty
extensive already. The latter doesn't seem super idiomatic in KMS, but
it's a common pattern in the rest of the kernel, so maybe it's a good
idea still.

>  	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_RGB)) {
>  		conn_state->hdmi.output_format = HDMI_COLORSPACE_RGB;
>  		return 0;
>  	}
>  
> +	if (hdmi_try_format_bpc(connector, conn_state, mode, bpc, HDMI_COLORSPACE_YUV420)) {
> +		conn_state->hdmi.output_format = HDMI_COLORSPACE_YUV420;
> +		return 0;
> +	}
> +

During our discussions when we merged this infrastructure, the goal was
to align our behaviour to Intel's. The discussion also pointed out that
we want to degrade the bpc before falling back to a YUV format.

So we need to first try RGB with any bpc, and then try YUV with any BPC
if it didn't work.

We also need plenty of tests based on whether the source supports
YUV420, the sink has YUV420-only modes, that the fallback occurs
properly, etc.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 0/3] drm/connector: hdmi: Allow using the YUV420 output format

[Dmitry Baryshkov]

On Sat, Nov 30, 2024 at 01:56:31AM +0200, Cristian Ciocaltea wrote:
> Provide the basic support to enable using YUV420 as an RGB fallback when
> computing the best output format and color depth.

The HDMI Connector functionality has pretty good KUnit coverage. Please
expand KUnits to cover your patches too.

> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> Cristian Ciocaltea (3):
>       drm/connector: hdmi: Evaluate limited range after computing format
>       drm/connector: hdmi: Add support for YUV420 format verification
>       drm/connector: hdmi: Use YUV420 output format as an RGB fallback
> 
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 55 ++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 10 deletions(-)
> ---
> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
> change-id: 20241130-hdmi-conn-yuv-e1fa596df768
> 

-- 
With best wishes
Dmitry

Re: [PATCH 0/3] drm/connector: hdmi: Allow using the YUV420 output format

[Cristian Ciocaltea]

On 11/30/24 10:38 AM, Dmitry Baryshkov wrote:
> On Sat, Nov 30, 2024 at 01:56:31AM +0200, Cristian Ciocaltea wrote:
>> Provide the basic support to enable using YUV420 as an RGB fallback when
>> computing the best output format and color depth.
> 
> The HDMI Connector functionality has pretty good KUnit coverage. Please
> expand KUnits to cover your patches too.

Sure, will handle this in v2.

Thanks for reviewing,
Cristian