[PATCH v3 00/15] drm/connector: hdmi: Allow using the YUV420 output format

[PATCH v3 00/15] 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>
---
Changes in v3:
- Collected R-B tags from Dmitry and Maxime
- Updated debug messages in "drm/connector: hdmi: Add support for YUV420
  format verification" to avoid referencing VIC (Maxime)
- Reworked "drm/connector: hdmi: Use YUV420 output format as an RGB
  fallback" and moved some parts to separate patches:
  * drm/connector: hdmi: Add missing bpc debug info to hdmi_try_format_bpc()
  * drm/connector: hdmi: Rename hdmi_compute_format() internal helper
  * drm/connector: hdmi: Factor out bpc and format computation logic
- Reworked "drm/tests: hdmi: Add macros to simplify EDID setup" by
  renaming the new helpers and moving the conversion to separate
  patches (Maxime):
  * drm/tests: hdmi: Fixup CamelCase warning
  * drm/tests: hdmi: Replace open coded EDID setup
  * drm/tests: hdmi: Drop unused drm_kunit_helper_connector_hdmi_init_funcs()
- Moved connector_hdmi_init_with_edid_funcs() changes from "drm/tests:
  hdmi: Add limited range tests for YUV420 mode") to "drm/tests: hdmi:
  Setup ycbcr_420_allowed before initializing connector"
- Got rid of the floating-point operation in "drm/tests: hdmi: Add max
  TMDS rate fallback tests for YUV420 mode" in order to fix the build
  errors reported by some kernel test robots
- Added new patch "drm/tests: hdmi: Add test for unsuccessful forced
  fallback to YUV420"
- Rebased series onto drm-misc-next from 2025-03-25
- Link to v2: https://lore.kernel.org/r/20250311-hdmi-conn-yuv-v2-0-fbdb94f02562@collabora.com

Changes in v2:
- Provided the missing Fixes tag on first patch (Dmitry)
- Added patch "drm/connector: hdmi: Improve debug message for supported
  format"
- Reworked "drm/connector: hdmi: Use YUV420 output format as an RGB
  fallback" so that the fallback to YUV420 will be attempted only when
  RGB cannot be supported for any of the available color depths (Maxime)
- Added a patch to simplify EDID setup in KUnit cases
- Added two patches extending KUnit coverage for YUV420 by providing
  tests for limited range and max TMDS rate fallback
- Rebased series onto drm-misc-next-2025-03-06
- Link to v1: https://lore.kernel.org/r/20241130-hdmi-conn-yuv-v1-0-254279a08671@collabora.com

---
Cristian Ciocaltea (15):
      drm/connector: hdmi: Evaluate limited range after computing format
      drm/connector: hdmi: Add support for YUV420 format verification
      drm/connector: hdmi: Improve debug message for supported format
      drm/connector: hdmi: Add missing bpc debug info to hdmi_try_format_bpc()
      drm/connector: hdmi: Rename hdmi_compute_format() internal helper
      drm/connector: hdmi: Factor out bpc and format computation logic
      drm/connector: hdmi: Use YUV420 output format as an RGB fallback
      drm/tests: hdmi: Add macros to simplify EDID setup
      drm/tests: hdmi: Fixup CamelCase warning
      drm/tests: hdmi: Replace open coded EDID setup
      drm/tests: hdmi: Drop unused drm_kunit_helper_connector_hdmi_init_funcs()
      drm/tests: hdmi: Setup ycbcr_420_allowed before initializing connector
      drm/tests: hdmi: Add limited range tests for YUV420 mode
      drm/tests: hdmi: Add max TMDS rate fallback tests for YUV420 mode
      drm/tests: hdmi: Add test for unsuccessful forced fallback to YUV420

 drivers/gpu/drm/display/drm_hdmi_state_helper.c    | 114 +++--
 drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 557 ++++++++++++++-------
 drivers/gpu/drm/tests/drm_kunit_edid.h             | 222 ++++++++
 3 files changed, 678 insertions(+), 215 deletions(-)
---
base-commit: 2f9d51740cc30e0d2c8a23a55b1e20cf2513c250
change-id: 20241130-hdmi-conn-yuv-e1fa596df768

[PATCH v3 01/15] 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.

Fixes: 027d43590649 ("drm/connector: hdmi: Add RGB Quantization Range to the connector state")
Reviewed-by: Dmitry Baryshkov <lumag@kernel.org>
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 d9d9948b29e9d5ef9bc9cc9108b3ace4aca2e3ae..45b154c8abb2cc731bf4be472e58815cf47463d4 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -798,12 +798,12 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
 	if (!new_conn_state->crtc || !new_conn_state->best_encoder)
 		return 0;
 
-	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.49.0

[PATCH v3 02/15] 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 | 39 +++++++++++++++++++++++--
 1 file changed, 36 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 45b154c8abb2cc731bf4be472e58815cf47463d4..eb284032ea794838f333ce639243540fca91dbdb 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_audio_helper.h>
@@ -407,6 +408,11 @@ 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, "Mode can be only supported in YUV420 format.\n");
+		return false;
+	}
+
 	switch (format) {
 	case HDMI_COLORSPACE_RGB:
 		drm_dbg_kms(dev, "RGB Format, checking the constraints.\n");
@@ -437,9 +443,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, "Mode cannot be supported in YUV420 format.\n");
+			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.49.0

Re: [PATCH v3 02/15] drm/connector: hdmi: Add support for YUV420 format verification

[Maxime Ripard]

On Wed, 26 Mar 2025 12:19:51 +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>
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v3 03/15] drm/connector: hdmi: Improve debug message for supported format

[Cristian Ciocaltea]

Add the missing 'bpc' string to the debug message indicating the
supported format identified within hdmi_try_format_bpc() helper.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index eb284032ea794838f333ce639243540fca91dbdb..a0cb3163f457635cf27e53b009bd83f85eee9336 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -596,7 +596,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);
 

-- 
2.49.0

[PATCH v3 04/15] drm/connector: hdmi: Add missing bpc debug info to hdmi_try_format_bpc()

[Cristian Ciocaltea]

The very first debug message in hdmi_try_format_bpc() is incomplete, as
it doesn't provide the given bpc in addition to the tried format.

Add the missing debug information and drop the now redundant message
from hdmi_compute_config().

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

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index a0cb3163f457635cf27e53b009bd83f85eee9336..f54eb5c594cddbd67dfacb5e06d54e9ce7851013 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -578,8 +578,9 @@ hdmi_try_format_bpc(const struct drm_connector *connector,
 	struct drm_device *dev = connector->dev;
 	int ret;
 
-	drm_dbg_kms(dev, "Trying %s output format\n",
-		    drm_hdmi_connector_get_output_format_name(fmt));
+	drm_dbg_kms(dev, "Trying %s output format with %u bpc\n",
+		    drm_hdmi_connector_get_output_format_name(fmt),
+		    bpc);
 
 	if (!sink_supports_format_bpc(connector, info, mode, fmt, bpc)) {
 		drm_dbg_kms(dev, "%s output format not supported with %u bpc\n",
@@ -638,8 +639,6 @@ hdmi_compute_config(const struct drm_connector *connector,
 	int ret;
 
 	for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
-		drm_dbg_kms(dev, "Trying with a %d bpc output\n", bpc);
-
 		ret = hdmi_compute_format(connector, conn_state, mode, bpc);
 		if (ret)
 			continue;

-- 
2.49.0

Re: [PATCH v3 04/15] drm/connector: hdmi: Add missing bpc debug info to hdmi_try_format_bpc()

[Dmitry Baryshkov]

On Wed, Mar 26, 2025 at 12:19:53PM +0200, Cristian Ciocaltea wrote:
> The very first debug message in hdmi_try_format_bpc() is incomplete, as
> it doesn't provide the given bpc in addition to the tried format.
> 
> Add the missing debug information and drop the now redundant message
> from hdmi_compute_config().
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

-- 
With best wishes
Dmitry

[PATCH v3 05/15] drm/connector: hdmi: Rename hdmi_compute_format() internal helper

[Cristian Ciocaltea]

In preparation to introduce support for alternative output formats,
which will require extending hdmi_compute_format() functionality by
setting hdmi.output_bpc in addition to the current hdmi.output_format,
maintain future code readability by renaming the helper to
hdmi_compute_format_bpc().

There are no functional changes intended at this point.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 10 +++++-----
 1 file changed, 5 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 f54eb5c594cddbd67dfacb5e06d54e9ce7851013..160964190d82ac233fdbe34ac54024a007a19872 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -605,10 +605,10 @@ hdmi_try_format_bpc(const struct drm_connector *connector,
 }
 
 static int
-hdmi_compute_format(const struct drm_connector *connector,
-		    struct drm_connector_state *conn_state,
-		    const struct drm_display_mode *mode,
-		    unsigned int bpc)
+hdmi_compute_format_bpc(const struct drm_connector *connector,
+			struct drm_connector_state *conn_state,
+			const struct drm_display_mode *mode,
+			unsigned int bpc)
 {
 	struct drm_device *dev = connector->dev;
 
@@ -639,7 +639,7 @@ hdmi_compute_config(const struct drm_connector *connector,
 	int ret;
 
 	for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
-		ret = hdmi_compute_format(connector, conn_state, mode, bpc);
+		ret = hdmi_compute_format_bpc(connector, conn_state, mode, bpc);
 		if (ret)
 			continue;
 

-- 
2.49.0

[PATCH v3 06/15] drm/connector: hdmi: Factor out bpc and format computation logic

[Cristian Ciocaltea]

In preparation to support fallback to an alternative output format, e.g.
YUV420, when RGB cannot be used for any of the available color depths,
move the bpc try loop out of hdmi_compute_config() and, instead, make it
part of hdmi_compute_format_bpc().  Additionally, add a new parameter to
the latter holding the output format to be checked and eventually set.

This improves code reusability and further extensibility.

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

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 160964190d82ac233fdbe34ac54024a007a19872..6de0abb15ecb36fd4eb98725e2a3835e5e0db134 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -608,42 +608,19 @@ static int
 hdmi_compute_format_bpc(const struct drm_connector *connector,
 			struct drm_connector_state *conn_state,
 			const struct drm_display_mode *mode,
-			unsigned int bpc)
+			unsigned int max_bpc, enum hdmi_colorspace fmt)
 {
 	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;
-	}
-
-	drm_dbg_kms(dev, "Failed. No Format Supported for that bpc count.\n");
-
-	return -EINVAL;
-}
-
-static int
-hdmi_compute_config(const struct drm_connector *connector,
-		    struct drm_connector_state *conn_state,
-		    const struct drm_display_mode *mode)
-{
-	struct drm_device *dev = connector->dev;
-	unsigned int max_bpc = clamp_t(unsigned int,
-				       conn_state->max_bpc,
-				       8, connector->max_bpc);
 	unsigned int bpc;
 	int ret;
 
 	for (bpc = max_bpc; bpc >= 8; bpc -= 2) {
-		ret = hdmi_compute_format_bpc(connector, conn_state, mode, bpc);
-		if (ret)
+		ret = hdmi_try_format_bpc(connector, conn_state, mode, bpc, fmt);
+		if (!ret)
 			continue;
 
 		conn_state->hdmi.output_bpc = bpc;
+		conn_state->hdmi.output_format = fmt;
 
 		drm_dbg_kms(dev,
 			    "Mode %ux%u @ %uHz: Found configuration: bpc: %u, fmt: %s, clock: %llu\n",
@@ -655,9 +632,28 @@ hdmi_compute_config(const struct drm_connector *connector,
 		return 0;
 	}
 
+	drm_dbg_kms(dev, "Failed. %s output format not supported for any bpc count.\n",
+		    drm_hdmi_connector_get_output_format_name(fmt));
+
 	return -EINVAL;
 }
 
+static int
+hdmi_compute_config(const struct drm_connector *connector,
+		    struct drm_connector_state *conn_state,
+		    const struct drm_display_mode *mode)
+{
+	unsigned int max_bpc = clamp_t(unsigned int,
+				       conn_state->max_bpc,
+				       8, connector->max_bpc);
+	int ret;
+
+	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
+				      HDMI_COLORSPACE_RGB);
+
+	return ret;
+}
+
 static int hdmi_generate_avi_infoframe(const struct drm_connector *connector,
 				       struct drm_connector_state *conn_state)
 {

-- 
2.49.0

Re: [PATCH v3 06/15] drm/connector: hdmi: Factor out bpc and format computation logic

[Maxime Ripard]

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

Hi,

On Wed, Mar 26, 2025 at 12:19:55PM +0200, Cristian Ciocaltea wrote:
> In preparation to support fallback to an alternative output format, e.g.
> YUV420, when RGB cannot be used for any of the available color depths,
> move the bpc try loop out of hdmi_compute_config() and, instead, make it
> part of hdmi_compute_format_bpc().  Additionally, add a new parameter to
> the latter holding the output format to be checked and eventually set.
>
> This improves code reusability and further extensibility.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

I think patch 5 could be squashed into this one.

> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 50 ++++++++++++-------------
>  1 file changed, 23 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 160964190d82ac233fdbe34ac54024a007a19872..6de0abb15ecb36fd4eb98725e2a3835e5e0db134 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -608,42 +608,19 @@ static int
>  hdmi_compute_format_bpc(const struct drm_connector *connector,
>  			struct drm_connector_state *conn_state,
>  			const struct drm_display_mode *mode,
> -			unsigned int bpc)
> +			unsigned int max_bpc, enum hdmi_colorspace fmt)
>  {
>  	struct drm_device *dev = connector->dev;
> -
> -	/*
> -	 * TODO: Add support for YCbCr420 output for HDMI 2.0 capable
> -	 * devices, for modes that only support YCbCr420.
> -	 */

And we should fix that comment for now.

Once fixed,
Reviewed-by: Maxime Ripard <mripard@kernel.org>

Maxime

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

Re: [PATCH v3 06/15] drm/connector: hdmi: Factor out bpc and format computation logic

[Cristian Ciocaltea]

Hi Maxime,

On 4/9/25 6:02 PM, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Mar 26, 2025 at 12:19:55PM +0200, Cristian Ciocaltea wrote:
>> In preparation to support fallback to an alternative output format, e.g.
>> YUV420, when RGB cannot be used for any of the available color depths,
>> move the bpc try loop out of hdmi_compute_config() and, instead, make it
>> part of hdmi_compute_format_bpc().  Additionally, add a new parameter to
>> the latter holding the output format to be checked and eventually set.
>>
>> This improves code reusability and further extensibility.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> 
> I think patch 5 could be squashed into this one.

Ack.

>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 50 ++++++++++++-------------
>>  1 file changed, 23 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> index 160964190d82ac233fdbe34ac54024a007a19872..6de0abb15ecb36fd4eb98725e2a3835e5e0db134 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> @@ -608,42 +608,19 @@ static int
>>  hdmi_compute_format_bpc(const struct drm_connector *connector,
>>  			struct drm_connector_state *conn_state,
>>  			const struct drm_display_mode *mode,
>> -			unsigned int bpc)
>> +			unsigned int max_bpc, enum hdmi_colorspace fmt)
>>  {
>>  	struct drm_device *dev = connector->dev;
>> -
>> -	/*
>> -	 * TODO: Add support for YCbCr420 output for HDMI 2.0 capable
>> -	 * devices, for modes that only support YCbCr420.
>> -	 */
> 
> And we should fix that comment for now.

Sorry, I missed to move this hunk to the next patch.

> 
> Once fixed,
> Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks,
Cristian

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

[Cristian Ciocaltea]

Try to make use of YUV420 when computing the best output format and
RGB cannot be supported for any of the available color depths.

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

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 6de0abb15ecb36fd4eb98725e2a3835e5e0db134..3859600c6af4a79f30858adfc9f9a710dfe561a5 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -650,7 +650,17 @@ hdmi_compute_config(const struct drm_connector *connector,
 
 	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
 				      HDMI_COLORSPACE_RGB);
+	if (!ret)
+		return 0;
 
+	if (!connector->ycbcr_420_allowed) {
+		drm_dbg_kms(connector->dev,
+			    "YUV420 output format not allowed for connector.\n");
+		return -EINVAL;
+	}
+
+	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
+				      HDMI_COLORSPACE_YUV420);
 	return ret;
 }
 

-- 
2.49.0

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

[Maxime Ripard]

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

On Wed, Mar 26, 2025 at 12:19:56PM +0200, Cristian Ciocaltea wrote:
> Try to make use of YUV420 when computing the best output format and
> RGB cannot be supported for any of the available color depths.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 6de0abb15ecb36fd4eb98725e2a3835e5e0db134..3859600c6af4a79f30858adfc9f9a710dfe561a5 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -650,7 +650,17 @@ hdmi_compute_config(const struct drm_connector *connector,
>  
>  	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
>  				      HDMI_COLORSPACE_RGB);
> +	if (!ret)
> +		return 0;
>  
> +	if (!connector->ycbcr_420_allowed) {
> +		drm_dbg_kms(connector->dev,
> +			    "YUV420 output format not allowed for connector.\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> +				      HDMI_COLORSPACE_YUV420);
>  	return ret;

I think I'd prefer to log a debug message there and return 0 if it
succeeds, something like

ret = hdmi_compute_format_bpc(..)
if (ret) {
   drm_dbg("YUV420 doesn't work").
   return ret;
}

return 0;

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

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

[Cristian Ciocaltea]

On 4/9/25 6:09 PM, Maxime Ripard wrote:
> On Wed, Mar 26, 2025 at 12:19:56PM +0200, Cristian Ciocaltea wrote:
>> Try to make use of YUV420 when computing the best output format and
>> RGB cannot be supported for any of the available color depths.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> index 6de0abb15ecb36fd4eb98725e2a3835e5e0db134..3859600c6af4a79f30858adfc9f9a710dfe561a5 100644
>> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
>> @@ -650,7 +650,17 @@ hdmi_compute_config(const struct drm_connector *connector,
>>  
>>  	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
>>  				      HDMI_COLORSPACE_RGB);
>> +	if (!ret)
>> +		return 0;
>>  
>> +	if (!connector->ycbcr_420_allowed) {
>> +		drm_dbg_kms(connector->dev,
>> +			    "YUV420 output format not allowed for connector.\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
>> +				      HDMI_COLORSPACE_YUV420);
>>  	return ret;
> 
> I think I'd prefer to log a debug message there and return 0 if it
> succeeds, something like

Sure, will do.

> ret = hdmi_compute_format_bpc(..)
> if (ret) {
>    drm_dbg("YUV420 doesn't work").
>    return ret;
> }
> 
> return 0;

[PATCH v3 08/15] drm/tests: hdmi: Add macros to simplify EDID setup

[Cristian Ciocaltea]

Introduce a few macros to facilitate setting custom (i.e. non-default)
EDID data during connector initialization.

The actual conversion to use the new helpers is handled separately.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 43 +++++++++++-----------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 7ffd666753b10bc991894e238206a3c5328d0e23..bcbd146fb655f4402529e59af09c99dbae7be0bf 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -140,10 +140,12 @@ static const struct drm_connector_funcs dummy_connector_funcs = {
 
 static
 struct drm_atomic_helper_connector_hdmi_priv *
-drm_kunit_helper_connector_hdmi_init_funcs(struct kunit *test,
-					   unsigned int formats,
-					   unsigned int max_bpc,
-					   const struct drm_connector_hdmi_funcs *hdmi_funcs)
+connector_hdmi_init_with_edid_funcs(struct kunit *test,
+				    unsigned int formats,
+				    unsigned int max_bpc,
+				    const struct drm_connector_hdmi_funcs *hdmi_funcs,
+				    const char *edid_data,
+				    size_t edid_len)
 {
 	struct drm_atomic_helper_connector_hdmi_priv *priv;
 	struct drm_connector *conn;
@@ -197,30 +199,27 @@ drm_kunit_helper_connector_hdmi_init_funcs(struct kunit *test,
 
 	drm_mode_config_reset(drm);
 
+	if (edid_data && edid_len) {
+		ret = set_connector_edid(test, &priv->connector, edid_data, edid_len);
+		KUNIT_ASSERT_GT(test, ret, 0);
+	}
+
 	return priv;
 }
 
-static
-struct drm_atomic_helper_connector_hdmi_priv *
-drm_kunit_helper_connector_hdmi_init(struct kunit *test,
-				     unsigned int formats,
-				     unsigned int max_bpc)
-{
-	struct drm_atomic_helper_connector_hdmi_priv *priv;
-	int ret;
+#define drm_kunit_helper_connector_hdmi_init_funcs(test, formats, max_bpc, funcs)		\
+	connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, NULL, 0)
 
-	priv = drm_kunit_helper_connector_hdmi_init_funcs(test,
-							  formats, max_bpc,
-							  &dummy_connector_hdmi_funcs);
-	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+#define drm_kunit_helper_connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, edid) \
+	connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, edid, ARRAY_SIZE(edid))
 
-	ret = set_connector_edid(test, &priv->connector,
-				 test_edid_hdmi_1080p_rgb_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
+#define drm_kunit_helper_connector_hdmi_init_with_edid(test, formats, max_bpc, edid)		\
+	drm_kunit_helper_connector_hdmi_init_with_edid_funcs(test, formats, max_bpc,		\
+							     &dummy_connector_hdmi_funcs, edid)
 
-	return priv;
-}
+#define drm_kunit_helper_connector_hdmi_init(test, formats, max_bpc)				\
+	drm_kunit_helper_connector_hdmi_init_with_edid(test, formats, max_bpc,			\
+						       test_edid_hdmi_1080p_rgb_max_200mhz)
 
 /*
  * Test that if we change the RGB quantization property to a different

-- 
2.49.0

Re: [PATCH v3 08/15] drm/tests: hdmi: Add macros to simplify EDID setup

[Maxime Ripard]

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

On Wed, Mar 26, 2025 at 12:19:57PM +0200, Cristian Ciocaltea wrote:
> Introduce a few macros to facilitate setting custom (i.e. non-default)
> EDID data during connector initialization.
> 
> The actual conversion to use the new helpers is handled separately.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 43 +++++++++++-----------
>  1 file changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index 7ffd666753b10bc991894e238206a3c5328d0e23..bcbd146fb655f4402529e59af09c99dbae7be0bf 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -140,10 +140,12 @@ static const struct drm_connector_funcs dummy_connector_funcs = {
>  
>  static
>  struct drm_atomic_helper_connector_hdmi_priv *
> -drm_kunit_helper_connector_hdmi_init_funcs(struct kunit *test,
> -					   unsigned int formats,
> -					   unsigned int max_bpc,
> -					   const struct drm_connector_hdmi_funcs *hdmi_funcs)
> +connector_hdmi_init_with_edid_funcs(struct kunit *test,
> +				    unsigned int formats,
> +				    unsigned int max_bpc,
> +				    const struct drm_connector_hdmi_funcs *hdmi_funcs,
> +				    const char *edid_data,
> +				    size_t edid_len)
>  {
>  	struct drm_atomic_helper_connector_hdmi_priv *priv;
>  	struct drm_connector *conn;
> @@ -197,30 +199,27 @@ drm_kunit_helper_connector_hdmi_init_funcs(struct kunit *test,
>  
>  	drm_mode_config_reset(drm);
>  
> +	if (edid_data && edid_len) {
> +		ret = set_connector_edid(test, &priv->connector, edid_data, edid_len);
> +		KUNIT_ASSERT_GT(test, ret, 0);
> +	}
> +
>  	return priv;
>  }
>  
> -static
> -struct drm_atomic_helper_connector_hdmi_priv *
> -drm_kunit_helper_connector_hdmi_init(struct kunit *test,
> -				     unsigned int formats,
> -				     unsigned int max_bpc)
> -{
> -	struct drm_atomic_helper_connector_hdmi_priv *priv;
> -	int ret;
> +#define drm_kunit_helper_connector_hdmi_init_funcs(test, formats, max_bpc, funcs)		\
> +	connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, NULL, 0)

Again, we don't need that one. All current users would actually use
drm_kunit_helper_connector_hdmi_init_with_edid_funcs().

Maxime

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

[PATCH v3 09/15] drm/tests: hdmi: Fixup CamelCase warning

[Cristian Ciocaltea]

Rename the reject_100_MHz_connector_hdmi_funcs variable to make
checkpatch.pl happy:

  CHECK: Avoid CamelCase: <reject_100_MHz_connector_hdmi_funcs>

While at it, also rename reject_100MHz_connector_tmds_char_rate_valid()
for consistency.

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

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index bcbd146fb655f4402529e59af09c99dbae7be0bf..284bd9b1418a454d05c4a38263519eb8ae450090 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -89,15 +89,15 @@ static const struct drm_connector_hdmi_funcs reject_connector_hdmi_funcs = {
 };
 
 static enum drm_mode_status
-reject_100MHz_connector_tmds_char_rate_valid(const struct drm_connector *connector,
+reject_100mhz_connector_tmds_char_rate_valid(const struct drm_connector *connector,
 					     const struct drm_display_mode *mode,
 					     unsigned long long tmds_rate)
 {
 	return (tmds_rate > 100ULL * 1000 * 1000) ? MODE_BAD : MODE_OK;
 }
 
-static const struct drm_connector_hdmi_funcs reject_100_MHz_connector_hdmi_funcs = {
-	.tmds_char_rate_valid	= reject_100MHz_connector_tmds_char_rate_valid,
+static const struct drm_connector_hdmi_funcs reject_100mhz_connector_hdmi_funcs = {
+	.tmds_char_rate_valid	= reject_100mhz_connector_tmds_char_rate_valid,
 };
 
 static int dummy_connector_get_modes(struct drm_connector *connector)
@@ -1933,7 +1933,7 @@ static void drm_test_check_mode_valid_reject_rate(struct kunit *test)
 	priv = drm_kunit_helper_connector_hdmi_init_funcs(test,
 							  BIT(HDMI_COLORSPACE_RGB),
 							  8,
-							  &reject_100_MHz_connector_hdmi_funcs);
+							  &reject_100mhz_connector_hdmi_funcs);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	conn = &priv->connector;

-- 
2.49.0

Re: [PATCH v3 09/15] drm/tests: hdmi: Fixup CamelCase warning

[Maxime Ripard]

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

On Wed, Mar 26, 2025 at 12:19:58PM +0200, Cristian Ciocaltea wrote:
> Rename the reject_100_MHz_connector_hdmi_funcs variable to make
> checkpatch.pl happy:
> 
>   CHECK: Avoid CamelCase: <reject_100_MHz_connector_hdmi_funcs>
> 
> While at it, also rename reject_100MHz_connector_tmds_char_rate_valid()
> for consistency.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

In this case, checkpatch is wrong. mhz != MHz.

And since it's not a warning, I'd just ignore it.

Maxime

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

Re: [PATCH v3 09/15] drm/tests: hdmi: Fixup CamelCase warning

[Cristian Ciocaltea]

On 4/9/25 6:13 PM, Maxime Ripard wrote:
> On Wed, Mar 26, 2025 at 12:19:58PM +0200, Cristian Ciocaltea wrote:
>> Rename the reject_100_MHz_connector_hdmi_funcs variable to make
>> checkpatch.pl happy:
>>
>>   CHECK: Avoid CamelCase: <reject_100_MHz_connector_hdmi_funcs>
>>
>> While at it, also rename reject_100MHz_connector_tmds_char_rate_valid()
>> for consistency.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> 
> In this case, checkpatch is wrong. mhz != MHz.
> 
> And since it's not a warning, I'd just ignore it.

I think it also improves consistency a bit, as this is actually the only
place where "[_]MHz" is being used in the file, everywhere else we have
"mhz", without '_' prefix.

But I can still drop the patch if you prefer.

[PATCH v3 10/15] drm/tests: hdmi: Replace open coded EDID setup

[Cristian Ciocaltea]

Make use of the recently introduced macros to reduce boilerplate code
around EDID setup. This also helps dropping the redundant calls to
set_connector_edid().

No functional changes intended.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 220 ++++++++-------------
 1 file changed, 78 insertions(+), 142 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 284bd9b1418a454d05c4a38263519eb8ae450090..7b2aaee5009ce58e6edf2649e2182c43ba834523 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -751,19 +751,15 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    10);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				10,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	preferred = find_preferred_mode(conn);
 	KUNIT_ASSERT_NOT_NULL(test, preferred);
 
@@ -830,19 +826,15 @@ static void drm_test_check_output_bpc_crtc_mode_not_changed(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    10);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				10,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	preferred = find_preferred_mode(conn);
 	KUNIT_ASSERT_NOT_NULL(test, preferred);
 
@@ -904,21 +896,17 @@ static void drm_test_check_output_bpc_dvi(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB) |
-						    BIT(HDMI_COLORSPACE_YUV422) |
-						    BIT(HDMI_COLORSPACE_YUV444),
-						    12);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+					BIT(HDMI_COLORSPACE_RGB) |
+					BIT(HDMI_COLORSPACE_YUV422) |
+					BIT(HDMI_COLORSPACE_YUV444),
+					12,
+					test_edid_dvi_1080p);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_dvi_1080p,
-				 ARRAY_SIZE(test_edid_dvi_1080p));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	info = &conn->display_info;
 	KUNIT_ASSERT_FALSE(test, info->is_hdmi);
 
@@ -958,19 +946,15 @@ static void drm_test_check_tmds_char_rate_rgb_8bpc(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    8);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				8,
+				test_edid_hdmi_1080p_rgb_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	preferred = find_preferred_mode(conn);
 	KUNIT_ASSERT_NOT_NULL(test, preferred);
 	KUNIT_ASSERT_FALSE(test, preferred->flags & DRM_MODE_FLAG_DBLCLK);
@@ -1010,19 +994,15 @@ static void drm_test_check_tmds_char_rate_rgb_10bpc(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    10);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				10,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	preferred = find_preferred_mode(conn);
 	KUNIT_ASSERT_NOT_NULL(test, preferred);
 	KUNIT_ASSERT_FALSE(test, preferred->flags & DRM_MODE_FLAG_DBLCLK);
@@ -1062,19 +1042,15 @@ static void drm_test_check_tmds_char_rate_rgb_12bpc(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    12);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				12,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	preferred = find_preferred_mode(conn);
 	KUNIT_ASSERT_NOT_NULL(test, preferred);
 	KUNIT_ASSERT_FALSE(test, preferred->flags & DRM_MODE_FLAG_DBLCLK);
@@ -1180,19 +1156,15 @@ static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    12);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				12,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	info = &conn->display_info;
 	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
 	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
@@ -1252,21 +1224,17 @@ static void drm_test_check_max_tmds_rate_format_fallback(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB) |
-						    BIT(HDMI_COLORSPACE_YUV422) |
-						    BIT(HDMI_COLORSPACE_YUV444),
-						    12);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB) |
+				BIT(HDMI_COLORSPACE_YUV422) |
+				BIT(HDMI_COLORSPACE_YUV444),
+				12,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	info = &conn->display_info;
 	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
 	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
@@ -1320,20 +1288,16 @@ static void drm_test_check_output_bpc_format_vic_1(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB) |
-						    BIT(HDMI_COLORSPACE_YUV422) |
-						    BIT(HDMI_COLORSPACE_YUV444),
-						    12);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB) |
+				BIT(HDMI_COLORSPACE_YUV422) |
+				BIT(HDMI_COLORSPACE_YUV444),
+				12,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	info = &conn->display_info;
 	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
 	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
@@ -1387,19 +1351,15 @@ static void drm_test_check_output_bpc_format_driver_rgb_only(struct kunit *test)
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    12);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				12,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	info = &conn->display_info;
 	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
 	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
@@ -1457,21 +1417,17 @@ static void drm_test_check_output_bpc_format_display_rgb_only(struct kunit *test
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB) |
-						    BIT(HDMI_COLORSPACE_YUV422) |
-						    BIT(HDMI_COLORSPACE_YUV444),
-						    12);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB) |
+				BIT(HDMI_COLORSPACE_YUV422) |
+				BIT(HDMI_COLORSPACE_YUV444),
+				12,
+				test_edid_hdmi_1080p_rgb_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	info = &conn->display_info;
 	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
 	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
@@ -1530,19 +1486,15 @@ static void drm_test_check_output_bpc_format_driver_8bpc_only(struct kunit *test
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    8);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				8,
+				test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	info = &conn->display_info;
 	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
 	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
@@ -1593,21 +1545,17 @@ static void drm_test_check_output_bpc_format_display_8bpc_only(struct kunit *tes
 	struct drm_crtc *crtc;
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB) |
-						    BIT(HDMI_COLORSPACE_YUV422) |
-						    BIT(HDMI_COLORSPACE_YUV444),
-						    12);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB) |
+				BIT(HDMI_COLORSPACE_YUV422) |
+				BIT(HDMI_COLORSPACE_YUV444),
+				12,
+				test_edid_hdmi_1080p_rgb_max_340mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	drm = &priv->drm;
 	crtc = priv->crtc;
 	conn = &priv->connector;
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_max_340mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_340mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	info = &conn->display_info;
 	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
 	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
@@ -1926,28 +1874,20 @@ static void drm_test_check_mode_valid(struct kunit *test)
 static void drm_test_check_mode_valid_reject_rate(struct kunit *test)
 {
 	struct drm_atomic_helper_connector_hdmi_priv *priv;
-	struct drm_connector *conn;
 	struct drm_display_mode *preferred;
-	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init_funcs(test,
-							  BIT(HDMI_COLORSPACE_RGB),
-							  8,
-							  &reject_100mhz_connector_hdmi_funcs);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid_funcs(test,
+					BIT(HDMI_COLORSPACE_RGB),
+					8,
+					&reject_100mhz_connector_hdmi_funcs,
+					test_edid_hdmi_1080p_rgb_max_200mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
-	conn = &priv->connector;
-
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_max_200mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	/*
 	 * Unlike the drm_test_check_mode_valid() here 1080p is rejected, but
 	 * 480p is allowed.
 	 */
-	preferred = find_preferred_mode(conn);
+	preferred = find_preferred_mode(&priv->connector);
 	KUNIT_ASSERT_NOT_NULL(test, preferred);
 	KUNIT_EXPECT_EQ(test, preferred->hdisplay, 640);
 	KUNIT_EXPECT_EQ(test, preferred->vdisplay, 480);
@@ -1965,12 +1905,14 @@ static void drm_test_check_mode_valid_reject(struct kunit *test)
 	struct drm_atomic_helper_connector_hdmi_priv *priv;
 	struct drm_connector *conn;
 	struct drm_display_mode *preferred;
+	unsigned char no_edid[] = {};
 	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init_funcs(test,
-							  BIT(HDMI_COLORSPACE_RGB),
-							  8,
-							  &reject_connector_hdmi_funcs);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid_funcs(test,
+					BIT(HDMI_COLORSPACE_RGB),
+					8,
+					&reject_connector_hdmi_funcs,
+					no_edid);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	conn = &priv->connector;
@@ -1995,20 +1937,14 @@ static void drm_test_check_mode_valid_reject_max_clock(struct kunit *test)
 	struct drm_atomic_helper_connector_hdmi_priv *priv;
 	struct drm_connector *conn;
 	struct drm_display_mode *preferred;
-	int ret;
 
-	priv = drm_kunit_helper_connector_hdmi_init(test,
-						    BIT(HDMI_COLORSPACE_RGB),
-						    8);
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				8,
+				test_edid_hdmi_1080p_rgb_max_100mhz);
 	KUNIT_ASSERT_NOT_NULL(test, priv);
 
 	conn = &priv->connector;
-
-	ret = set_connector_edid(test, conn,
-				 test_edid_hdmi_1080p_rgb_max_100mhz,
-				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_100mhz));
-	KUNIT_ASSERT_GT(test, ret, 0);
-
 	KUNIT_ASSERT_EQ(test, conn->display_info.max_tmds_clock, 100 * 1000);
 
 	preferred = find_preferred_mode(conn);

-- 
2.49.0

Re: [PATCH v3 10/15] drm/tests: hdmi: Replace open coded EDID setup

[Maxime Ripard]

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

On Wed, Mar 26, 2025 at 12:19:59PM +0200, Cristian Ciocaltea wrote:
> Make use of the recently introduced macros to reduce boilerplate code
> around EDID setup. This also helps dropping the redundant calls to
> set_connector_edid().
> 
> No functional changes intended.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 220 ++++++++-------------
>  1 file changed, 78 insertions(+), 142 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index 284bd9b1418a454d05c4a38263519eb8ae450090..7b2aaee5009ce58e6edf2649e2182c43ba834523 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -751,19 +751,15 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test)
>  	struct drm_crtc *crtc;
>  	int ret;
>  
> -	priv = drm_kunit_helper_connector_hdmi_init(test,
> -						    BIT(HDMI_COLORSPACE_RGB),
> -						    10);
> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
> +				BIT(HDMI_COLORSPACE_RGB),
> +				10,
> +				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
>  	KUNIT_ASSERT_NOT_NULL(test, priv);
>  
>  	drm = &priv->drm;
>  	crtc = priv->crtc;
>  	conn = &priv->connector;
> -	ret = set_connector_edid(test, conn,
> -				 test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
> -				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
> -	KUNIT_ASSERT_GT(test, ret, 0);
> -

Yeah, ok, nvm what I said on the previous patch, it's needed.

>  	preferred = find_preferred_mode(conn);
>  	KUNIT_ASSERT_NOT_NULL(test, preferred);
>  
> @@ -830,19 +826,15 @@ static void drm_test_check_output_bpc_crtc_mode_not_changed(struct kunit *test)
>  	struct drm_crtc *crtc;
>  	int ret;
>  
> -	priv = drm_kunit_helper_connector_hdmi_init(test,
> -						    BIT(HDMI_COLORSPACE_RGB),
> -						    10);
> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
> +				BIT(HDMI_COLORSPACE_RGB),
> +				10,
> +				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);

Alignment is off.

Maxime

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

Re: [PATCH v3 10/15] drm/tests: hdmi: Replace open coded EDID setup

[Cristian Ciocaltea]

On 4/9/25 6:15 PM, Maxime Ripard wrote:
> On Wed, Mar 26, 2025 at 12:19:59PM +0200, Cristian Ciocaltea wrote:
>> Make use of the recently introduced macros to reduce boilerplate code
>> around EDID setup. This also helps dropping the redundant calls to
>> set_connector_edid().
>>
>> No functional changes intended.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 220 ++++++++-------------
>>  1 file changed, 78 insertions(+), 142 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> index 284bd9b1418a454d05c4a38263519eb8ae450090..7b2aaee5009ce58e6edf2649e2182c43ba834523 100644
>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> @@ -751,19 +751,15 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test)
>>  	struct drm_crtc *crtc;
>>  	int ret;
>>  
>> -	priv = drm_kunit_helper_connector_hdmi_init(test,
>> -						    BIT(HDMI_COLORSPACE_RGB),
>> -						    10);
>> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
>> +				BIT(HDMI_COLORSPACE_RGB),
>> +				10,
>> +				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
>>  	KUNIT_ASSERT_NOT_NULL(test, priv);
>>  
>>  	drm = &priv->drm;
>>  	crtc = priv->crtc;
>>  	conn = &priv->connector;
>> -	ret = set_connector_edid(test, conn,
>> -				 test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz,
>> -				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz));
>> -	KUNIT_ASSERT_GT(test, ret, 0);
>> -
> 
> Yeah, ok, nvm what I said on the previous patch, it's needed.
> 
>>  	preferred = find_preferred_mode(conn);
>>  	KUNIT_ASSERT_NOT_NULL(test, preferred);
>>  
>> @@ -830,19 +826,15 @@ static void drm_test_check_output_bpc_crtc_mode_not_changed(struct kunit *test)
>>  	struct drm_crtc *crtc;
>>  	int ret;
>>  
>> -	priv = drm_kunit_helper_connector_hdmi_init(test,
>> -						    BIT(HDMI_COLORSPACE_RGB),
>> -						    10);
>> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
>> +				BIT(HDMI_COLORSPACE_RGB),
>> +				10,
>> +				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
> 
> Alignment is off.

Yeah, that's a compromise to get rid of a bunch of checkpatch complaints:

  WARNING: line length of ... exceeds 100 columns

So we ended up with the following instead:

  CHECK: Alignment should match open parenthesis

But at least it's not a warning anymore.

Alternatively, we could maybe come up with a (very) short name for
drm_kunit_helper_connector_hdmi_init_with_edid(), though I'm not what
would that be :-(

[PATCH v3 11/15] drm/tests: hdmi: Drop unused drm_kunit_helper_connector_hdmi_init_funcs()

[Cristian Ciocaltea]

After updating the code to make use of the new EDID setup helpers,
drm_kunit_helper_connector_hdmi_init_funcs() became unused, hence drop
it.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 7b2aaee5009ce58e6edf2649e2182c43ba834523..1e32694041277a541f0f8941d9c35e8ca9264599 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -207,9 +207,6 @@ connector_hdmi_init_with_edid_funcs(struct kunit *test,
 	return priv;
 }
 
-#define drm_kunit_helper_connector_hdmi_init_funcs(test, formats, max_bpc, funcs)		\
-	connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, NULL, 0)
-
 #define drm_kunit_helper_connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, edid) \
 	connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, edid, ARRAY_SIZE(edid))
 

-- 
2.49.0

Re: [PATCH v3 11/15] drm/tests: hdmi: Drop unused drm_kunit_helper_connector_hdmi_init_funcs()

[Maxime Ripard]

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

On Wed, Mar 26, 2025 at 12:20:00PM +0200, Cristian Ciocaltea wrote:
> After updating the code to make use of the new EDID setup helpers,
> drm_kunit_helper_connector_hdmi_init_funcs() became unused, hence drop
> it.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index 7b2aaee5009ce58e6edf2649e2182c43ba834523..1e32694041277a541f0f8941d9c35e8ca9264599 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -207,9 +207,6 @@ connector_hdmi_init_with_edid_funcs(struct kunit *test,
>  	return priv;
>  }
>  
> -#define drm_kunit_helper_connector_hdmi_init_funcs(test, formats, max_bpc, funcs)		\
> -	connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, NULL, 0)
> -

Oh, so there's still one we don't need.

I really don't like that back and forth. I think we can avoid it by doing something like:

- Create a common __connector_hdmi_init function out of drm_kunit_helper_connector_hdmi_init.

- Add an EDID parameter to that common function. The API of drm_kunit_helper_connector_hdmi_init and
  drm_kunit_helper_connector_hdmi_init_funcs doesn't need to change at that point.

- Create a _with_edid_funcs macro. Note that only that one needs to be a macro.

- Convert the users to _with_edid_funcs, and drop drm_kunit_helper_connector_hdmi_init_funcs.

Maxime

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

Re: [PATCH v3 11/15] drm/tests: hdmi: Drop unused drm_kunit_helper_connector_hdmi_init_funcs()

[Cristian Ciocaltea]

On 4/9/25 6:24 PM, Maxime Ripard wrote:
> On Wed, Mar 26, 2025 at 12:20:00PM +0200, Cristian Ciocaltea wrote:
>> After updating the code to make use of the new EDID setup helpers,
>> drm_kunit_helper_connector_hdmi_init_funcs() became unused, hence drop
>> it.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> index 7b2aaee5009ce58e6edf2649e2182c43ba834523..1e32694041277a541f0f8941d9c35e8ca9264599 100644
>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> @@ -207,9 +207,6 @@ connector_hdmi_init_with_edid_funcs(struct kunit *test,
>>  	return priv;
>>  }
>>  
>> -#define drm_kunit_helper_connector_hdmi_init_funcs(test, formats, max_bpc, funcs)		\
>> -	connector_hdmi_init_with_edid_funcs(test, formats, max_bpc, funcs, NULL, 0)
>> -
> 
> Oh, so there's still one we don't need.
> 
> I really don't like that back and forth. I think we can avoid it by doing something like:
> 
> - Create a common __connector_hdmi_init function out of drm_kunit_helper_connector_hdmi_init.
> 
> - Add an EDID parameter to that common function. The API of drm_kunit_helper_connector_hdmi_init and
>   drm_kunit_helper_connector_hdmi_init_funcs doesn't need to change at that point.
> 
> - Create a _with_edid_funcs macro. Note that only that one needs to be a macro.
> 
> - Convert the users to _with_edid_funcs, and drop drm_kunit_helper_connector_hdmi_init_funcs.

Ack, one question though:

The common function and the macro should be part of the same patch, or
you'd like to have it split?

[PATCH v3 12/15] drm/tests: hdmi: Setup ycbcr_420_allowed before initializing connector

[Cristian Ciocaltea]

Initializing HDMI connector via drmm_connector_hdmi_init() requires its
->ycbcr_420_allowed flag to be adjusted according to the supported
formats passed as function argument, prior to the actual invocation.

In order to allow providing test coverage for YUV420 modes, ensure the
flag is properly setup.

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

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 1e32694041277a541f0f8941d9c35e8ca9264599..6897515189a0649a267196b246944efc92ace336 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -184,6 +184,8 @@ connector_hdmi_init_with_edid_funcs(struct kunit *test,
 	enc->possible_crtcs = drm_crtc_mask(priv->crtc);
 
 	conn = &priv->connector;
+	conn->ycbcr_420_allowed = !!(formats & BIT(HDMI_COLORSPACE_YUV420));
+
 	ret = drmm_connector_hdmi_init(drm, conn,
 				       "Vendor", "Product",
 				       &dummy_connector_funcs,

-- 
2.49.0

Re: [PATCH v3 12/15] drm/tests: hdmi: Setup ycbcr_420_allowed before initializing connector

[Maxime Ripard]

On Wed, 26 Mar 2025 12:20:01 +0200, Cristian Ciocaltea wrote:
> Initializing HDMI connector via drmm_connector_hdmi_init() requires its
> ->ycbcr_420_allowed flag to be adjusted according to the supported
> formats passed as function argument, prior to the actual invocation.
> 
> In order to allow providing test coverage for YUV420 modes, ensure the
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH v3 13/15] drm/tests: hdmi: Add limited range tests for YUV420 mode

[Cristian Ciocaltea]

Provide tests to verify that drm_atomic_helper_connector_hdmi_check()
helper behaviour when using YUV420 output format is to always set the
limited RGB quantization range to 'limited', no matter what the value of
Broadcast RGB property is.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c |  89 +++++++++++++++-
 drivers/gpu/drm/tests/drm_kunit_edid.h             | 112 +++++++++++++++++++++
 2 files changed, 196 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 6897515189a0649a267196b246944efc92ace336..3fae7ccf65309a1d8a4acf12de961713b9163096 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -731,6 +731,88 @@ static void drm_test_check_broadcast_rgb_limited_cea_mode_vic_1(struct kunit *te
 	drm_modeset_acquire_fini(&ctx);
 }
 
+/*
+ * Test that for an HDMI connector, with an HDMI monitor, we will
+ * get a limited RGB Quantization Range with a YUV420 mode, no
+ * matter what the value of the Broadcast RGB property is set to.
+ */
+static void drm_test_check_broadcast_rgb_cea_mode_yuv420(struct kunit *test)
+{
+	struct drm_atomic_helper_connector_hdmi_priv *priv;
+	enum drm_hdmi_broadcast_rgb broadcast_rgb;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_connector_state *conn_state;
+	struct drm_atomic_state *state;
+	struct drm_display_mode *mode;
+	struct drm_connector *conn;
+	struct drm_device *drm;
+	struct drm_crtc *crtc;
+	int ret;
+
+	broadcast_rgb = *(enum drm_hdmi_broadcast_rgb *)test->param_value;
+
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB) |
+				BIT(HDMI_COLORSPACE_YUV420),
+				8,
+				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
+	KUNIT_ASSERT_NOT_NULL(test, priv);
+
+	drm = &priv->drm;
+	crtc = priv->crtc;
+	conn = &priv->connector;
+	KUNIT_ASSERT_TRUE(test, conn->display_info.is_hdmi);
+
+	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
+						     crtc, conn,
+						     mode, &ctx);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+
+	conn_state = drm_atomic_get_connector_state(state, conn);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
+
+	conn_state->hdmi.broadcast_rgb = broadcast_rgb;
+
+	ret = drm_atomic_check_only(state);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	conn_state = drm_atomic_get_connector_state(state, conn);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
+
+	KUNIT_ASSERT_EQ(test, conn_state->hdmi.broadcast_rgb, broadcast_rgb);
+	KUNIT_ASSERT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_YUV420);
+
+	KUNIT_EXPECT_TRUE(test, conn_state->hdmi.is_limited_range);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
+static const enum drm_hdmi_broadcast_rgb check_broadcast_rgb_cea_mode_yuv420_tests[] = {
+	DRM_HDMI_BROADCAST_RGB_AUTO,
+	DRM_HDMI_BROADCAST_RGB_FULL,
+	DRM_HDMI_BROADCAST_RGB_LIMITED,
+};
+
+static void
+check_broadcast_rgb_cea_mode_yuv420_desc(const enum drm_hdmi_broadcast_rgb *broadcast_rgb,
+					 char *desc)
+{
+	sprintf(desc, "%s", drm_hdmi_connector_get_broadcast_rgb_name(*broadcast_rgb));
+}
+
+KUNIT_ARRAY_PARAM(check_broadcast_rgb_cea_mode_yuv420,
+		  check_broadcast_rgb_cea_mode_yuv420_tests,
+		  check_broadcast_rgb_cea_mode_yuv420_desc);
+
 /*
  * Test that if we change the maximum bpc property to a different value,
  * we trigger a mode change on the connector's CRTC, which will in turn
@@ -1650,11 +1732,8 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
 	KUNIT_CASE(drm_test_check_broadcast_rgb_full_cea_mode_vic_1),
 	KUNIT_CASE(drm_test_check_broadcast_rgb_limited_cea_mode),
 	KUNIT_CASE(drm_test_check_broadcast_rgb_limited_cea_mode_vic_1),
-	/*
-	 * TODO: When we'll have YUV output support, we need to check
-	 * that the limited range is always set to limited no matter
-	 * what the value of Broadcast RGB is.
-	 */
+	KUNIT_CASE_PARAM(drm_test_check_broadcast_rgb_cea_mode_yuv420,
+			 check_broadcast_rgb_cea_mode_yuv420_gen_params),
 	KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_changed),
 	KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_not_changed),
 	KUNIT_CASE(drm_test_check_disable_connector),
diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h
index 6358397a5d7ab0bcdea1c824fc9fd382560f4b0e..ff316e6114d65c96b1338cd83bc0d8d9e6e143e9 100644
--- a/drivers/gpu/drm/tests/drm_kunit_edid.h
+++ b/drivers/gpu/drm/tests/drm_kunit_edid.h
@@ -583,4 +583,116 @@ static const unsigned char test_edid_hdmi_1080p_rgb_yuv_dc_max_340mhz[] = {
 	0x00, 0x00, 0x00, 0x8c
 };
 
+/*
+ * edid-decode (hex):
+ *
+ * 00 ff ff ff ff ff ff 00 31 d8 34 00 00 00 00 00
+ * ff 23 01 03 80 60 36 78 0f ee 91 a3 54 4c 99 26
+ * 0f 50 54 20 00 00 01 01 01 01 01 01 01 01 01 01
+ * 01 01 01 01 01 01 02 3a 80 18 71 38 2d 40 58 2c
+ * 45 00 c0 1c 32 00 00 1e 00 00 00 fc 00 54 65 73
+ * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 18
+ * 55 18 5e 11 00 0a 20 20 20 20 20 20 00 00 00 10
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 bb
+ *
+ * 02 03 29 31 42 90 5f 6c 03 0c 00 10 00 78 28 20
+ * 00 00 01 03 6d d8 5d c4 01 28 80 07 00 00 00 00
+ * 00 00 e3 0f 00 00 e2 0e 5f 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ca
+ *
+ * ----------------
+ *
+ * Block 0, Base EDID:
+ *   EDID Structure Version & Revision: 1.3
+ *   Vendor & Product Identification:
+ *     Manufacturer: LNX
+ *     Model: 52
+ *     Model year: 2025
+ *   Basic Display Parameters & Features:
+ *     Digital display
+ *     Maximum image size: 96 cm x 54 cm
+ *     Gamma: 2.20
+ *     RGB color display
+ *     Default (sRGB) color space is primary color space
+ *     First detailed timing is the preferred timing
+ *     Supports GTF timings within operating range
+ *   Color Characteristics:
+ *     Red  : 0.6396, 0.3300
+ *     Green: 0.2998, 0.5996
+ *     Blue : 0.1503, 0.0595
+ *     White: 0.3125, 0.3291
+ *   Established Timings I & II:
+ *     DMT 0x04:   640x480    59.940476 Hz   4:3     31.469 kHz     25.175000 MHz
+ *   Standard Timings: none
+ *   Detailed Timing Descriptors:
+ *     DTD 1:  1920x1080   60.000000 Hz  16:9     67.500 kHz    148.500000 MHz (960 mm x 540 mm)
+ *                  Hfront   88 Hsync  44 Hback  148 Hpol P
+ *                  Vfront    4 Vsync   5 Vback   36 Vpol P
+ *     Display Product Name: 'Test EDID'
+ *     Display Range Limits:
+ *       Monitor ranges (GTF): 24-85 Hz V, 24-94 kHz H, max dotclock 170 MHz
+ *     Dummy Descriptor:
+ *   Extension blocks: 1
+ * Checksum: 0xbb
+ *
+ * ----------------
+ *
+ * Block 1, CTA-861 Extension Block:
+ *   Revision: 3
+ *   Supports YCbCr 4:4:4
+ *   Supports YCbCr 4:2:2
+ *   Native detailed modes: 1
+ *   Video Data Block:
+ *     VIC  16:  1920x1080   60.000000 Hz  16:9     67.500 kHz    148.500000 MHz (native)
+ *     VIC  95:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz
+ *   Vendor-Specific Data Block (HDMI), OUI 00-0C-03:
+ *     Source physical address: 1.0.0.0
+ *     DC_48bit
+ *     DC_36bit
+ *     DC_30bit
+ *     DC_Y444
+ *     Maximum TMDS clock: 200 MHz
+ *     Extended HDMI video details:
+ *   Vendor-Specific Data Block (HDMI Forum), OUI C4-5D-D8:
+ *     Version: 1
+ *     Maximum TMDS Character Rate: 200 MHz
+ *     SCDC Present
+ *     Supports 16-bits/component Deep Color 4:2:0 Pixel Encoding
+ *     Supports 12-bits/component Deep Color 4:2:0 Pixel Encoding
+ *     Supports 10-bits/component Deep Color 4:2:0 Pixel Encoding
+ *   YCbCr 4:2:0 Capability Map Data Block:
+ *     Empty Capability Map
+ *   YCbCr 4:2:0 Video Data Block:
+ *     VIC  95:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz
+ * Checksum: 0xca
+ */
+static const unsigned char test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz[] = {
+	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x34, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0xff, 0x23, 0x01, 0x03, 0x80, 0x60, 0x36, 0x78,
+	0x0f, 0xee, 0x91, 0xa3, 0x54, 0x4c, 0x99, 0x26, 0x0f, 0x50, 0x54, 0x20,
+	0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
+	0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x3a, 0x80, 0x18, 0x71, 0x38,
+	0x2d, 0x40, 0x58, 0x2c, 0x45, 0x00, 0xc0, 0x1c, 0x32, 0x00, 0x00, 0x1e,
+	0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44,
+	0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x18,
+	0x55, 0x18, 0x5e, 0x11, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+	0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0xbb, 0x02, 0x03, 0x29, 0x31,
+	0x42, 0x90, 0x5f, 0x6c, 0x03, 0x0c, 0x00, 0x10, 0x00, 0x78, 0x28, 0x20,
+	0x00, 0x00, 0x01, 0x03, 0x6d, 0xd8, 0x5d, 0xc4, 0x01, 0x28, 0x80, 0x07,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe3, 0x0f, 0x00, 0x00, 0xe2, 0x0e,
+	0x5f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0xca
+};
+
 #endif // DRM_KUNIT_EDID_H_

-- 
2.49.0

Re: [PATCH v3 13/15] drm/tests: hdmi: Add limited range tests for YUV420 mode

[Maxime Ripard]

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

On Wed, Mar 26, 2025 at 12:20:02PM +0200, Cristian Ciocaltea wrote:
> Provide tests to verify that drm_atomic_helper_connector_hdmi_check()
> helper behaviour when using YUV420 output format is to always set the
> limited RGB quantization range to 'limited', no matter what the value of
> Broadcast RGB property is.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c |  89 +++++++++++++++-
>  drivers/gpu/drm/tests/drm_kunit_edid.h             | 112 +++++++++++++++++++++
>  2 files changed, 196 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index 6897515189a0649a267196b246944efc92ace336..3fae7ccf65309a1d8a4acf12de961713b9163096 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -731,6 +731,88 @@ static void drm_test_check_broadcast_rgb_limited_cea_mode_vic_1(struct kunit *te
>  	drm_modeset_acquire_fini(&ctx);
>  }
>  
> +/*
> + * Test that for an HDMI connector, with an HDMI monitor, we will
> + * get a limited RGB Quantization Range with a YUV420 mode, no
> + * matter what the value of the Broadcast RGB property is set to.
> + */
> +static void drm_test_check_broadcast_rgb_cea_mode_yuv420(struct kunit *test)
> +{
> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
> +	enum drm_hdmi_broadcast_rgb broadcast_rgb;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_connector_state *conn_state;
> +	struct drm_atomic_state *state;
> +	struct drm_display_mode *mode;
> +	struct drm_connector *conn;
> +	struct drm_device *drm;
> +	struct drm_crtc *crtc;
> +	int ret;
> +
> +	broadcast_rgb = *(enum drm_hdmi_broadcast_rgb *)test->param_value;
> +
> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
> +				BIT(HDMI_COLORSPACE_RGB) |
> +				BIT(HDMI_COLORSPACE_YUV420),
> +				8,
> +				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +	drm = &priv->drm;
> +	crtc = priv->crtc;
> +	conn = &priv->connector;
> +	KUNIT_ASSERT_TRUE(test, conn->display_info.is_hdmi);
> +
> +	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
> +	KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
> +						     crtc, conn,
> +						     mode, &ctx);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

drm_kunit_helper_enable_crtc_connector() can return EDEADLK, so you need
to handle it and restart the sequence if it happens.

> +	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
> +
> +	conn_state = drm_atomic_get_connector_state(state, conn);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);

Ditto.

> +	conn_state->hdmi.broadcast_rgb = broadcast_rgb;
> +
> +	ret = drm_atomic_check_only(state);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	conn_state = drm_atomic_get_connector_state(state, conn);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);

Ditto, but I'm not sure you need drm_atomic_get_connector_state() here.
We know at this point that the state is there and we don't need to
allocate it anymore. drm_atomic_get_new_connector_state() will probably
be enough, and that one can't return EDEADLK.

Maxime

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

Re: [PATCH v3 13/15] drm/tests: hdmi: Add limited range tests for YUV420 mode

[Cristian Ciocaltea]

On 4/10/25 10:18 AM, Maxime Ripard wrote:
> On Wed, Mar 26, 2025 at 12:20:02PM +0200, Cristian Ciocaltea wrote:
>> Provide tests to verify that drm_atomic_helper_connector_hdmi_check()
>> helper behaviour when using YUV420 output format is to always set the
>> limited RGB quantization range to 'limited', no matter what the value of
>> Broadcast RGB property is.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c |  89 +++++++++++++++-
>>  drivers/gpu/drm/tests/drm_kunit_edid.h             | 112 +++++++++++++++++++++
>>  2 files changed, 196 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> index 6897515189a0649a267196b246944efc92ace336..3fae7ccf65309a1d8a4acf12de961713b9163096 100644
>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> @@ -731,6 +731,88 @@ static void drm_test_check_broadcast_rgb_limited_cea_mode_vic_1(struct kunit *te
>>  	drm_modeset_acquire_fini(&ctx);
>>  }
>>  
>> +/*
>> + * Test that for an HDMI connector, with an HDMI monitor, we will
>> + * get a limited RGB Quantization Range with a YUV420 mode, no
>> + * matter what the value of the Broadcast RGB property is set to.
>> + */
>> +static void drm_test_check_broadcast_rgb_cea_mode_yuv420(struct kunit *test)
>> +{
>> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
>> +	enum drm_hdmi_broadcast_rgb broadcast_rgb;
>> +	struct drm_modeset_acquire_ctx ctx;
>> +	struct drm_connector_state *conn_state;
>> +	struct drm_atomic_state *state;
>> +	struct drm_display_mode *mode;
>> +	struct drm_connector *conn;
>> +	struct drm_device *drm;
>> +	struct drm_crtc *crtc;
>> +	int ret;
>> +
>> +	broadcast_rgb = *(enum drm_hdmi_broadcast_rgb *)test->param_value;
>> +
>> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
>> +				BIT(HDMI_COLORSPACE_RGB) |
>> +				BIT(HDMI_COLORSPACE_YUV420),
>> +				8,
>> +				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
>> +	KUNIT_ASSERT_NOT_NULL(test, priv);
>> +
>> +	drm = &priv->drm;
>> +	crtc = priv->crtc;
>> +	conn = &priv->connector;
>> +	KUNIT_ASSERT_TRUE(test, conn->display_info.is_hdmi);
>> +
>> +	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
>> +	KUNIT_ASSERT_NOT_NULL(test, mode);
>> +
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
>> +						     crtc, conn,
>> +						     mode, &ctx);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
> 
> drm_kunit_helper_enable_crtc_connector() can return EDEADLK, so you need
> to handle it and restart the sequence if it happens.

Right, there are actually many users of the helper since

6a5c0ad7e08e ("drm/tests: hdmi_state_helpers: Switch to new helper")

Probably a stupid question, as I haven't checked which are the mandatory
operations of the restart sequence, but I wonder if this could be
handled inside the helper instead of trying to fix all test cases.
>> +	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>> +
>> +	conn_state = drm_atomic_get_connector_state(state, conn);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
> 
> Ditto.
> 
>> +	conn_state->hdmi.broadcast_rgb = broadcast_rgb;
>> +
>> +	ret = drm_atomic_check_only(state);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	conn_state = drm_atomic_get_connector_state(state, conn);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
> 
> Ditto, but I'm not sure you need drm_atomic_get_connector_state() here.
> We know at this point that the state is there and we don't need to
> allocate it anymore. drm_atomic_get_new_connector_state() will probably
> be enough 

Will check.

> and that one can't return EDEADLK.

Same question as above, could we handle EDEADLK at helper(s) level to
avoid open coding the restart sequence?

Thanks,
Cristian

[PATCH v3 14/15] drm/tests: hdmi: Add max TMDS rate fallback tests for YUV420 mode

[Cristian Ciocaltea]

Provide tests to verify drm_atomic_helper_connector_hdmi_check() helper
fallback behavior when using YUV420 output.

Also rename drm_test_check_max_tmds_rate_{bpc|format}_fallback() to
better differentiate from the newly introduced *_yuv420() variants.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 152 ++++++++++++++++++++-
 drivers/gpu/drm/tests/drm_kunit_edid.h             | 110 +++++++++++++++
 2 files changed, 258 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 3fae7ccf65309a1d8a4acf12de961713b9163096..99bedb2d6f555b3b140256000dfa7491d2a8f515 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -1224,7 +1224,7 @@ static void drm_test_check_hdmi_funcs_reject_rate(struct kunit *test)
  * Then we will pick the latter, and the computed TMDS character rate
  * will be equal to 1.25 times the mode pixel clock.
  */
-static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
+static void drm_test_check_max_tmds_rate_bpc_fallback_rgb(struct kunit *test)
 {
 	struct drm_atomic_helper_connector_hdmi_priv *priv;
 	struct drm_modeset_acquire_ctx ctx;
@@ -1279,6 +1279,75 @@ static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
 	drm_modeset_acquire_fini(&ctx);
 }
 
+/*
+ * Test that if:
+ * - We have an HDMI connector supporting both RGB and YUV420
+ * - The chosen mode can be supported in YUV420 output format only
+ * - The chosen mode has a TMDS character rate higher than the display
+ *   supports in YUV420/12bpc
+ * - The chosen mode has a TMDS character rate lower than the display
+ *   supports in YUV420/10bpc.
+ *
+ * Then we will pick the latter, and the computed TMDS character rate
+ * will be equal to 1.25 * 0.5 times the mode pixel clock.
+ */
+static void drm_test_check_max_tmds_rate_bpc_fallback_yuv420(struct kunit *test)
+{
+	struct drm_atomic_helper_connector_hdmi_priv *priv;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_connector_state *conn_state;
+	struct drm_display_info *info;
+	struct drm_display_mode *yuv420_only_mode;
+	unsigned long long rate;
+	struct drm_connector *conn;
+	struct drm_device *drm;
+	struct drm_crtc *crtc;
+	int ret;
+
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB) |
+				BIT(HDMI_COLORSPACE_YUV420),
+				12,
+				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
+	KUNIT_ASSERT_NOT_NULL(test, priv);
+
+	drm = &priv->drm;
+	crtc = priv->crtc;
+	conn = &priv->connector;
+	info = &conn->display_info;
+	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
+	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
+	KUNIT_ASSERT_TRUE(test, conn->ycbcr_420_allowed);
+
+	yuv420_only_mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
+	KUNIT_ASSERT_NOT_NULL(test, yuv420_only_mode);
+	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_only(info, yuv420_only_mode));
+
+	rate = drm_hdmi_compute_mode_clock(yuv420_only_mode, 12, HDMI_COLORSPACE_YUV420);
+	KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
+
+	rate = drm_hdmi_compute_mode_clock(yuv420_only_mode, 10, HDMI_COLORSPACE_YUV420);
+	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
+						     crtc, conn,
+						     yuv420_only_mode,
+						     &ctx);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	conn_state = conn->state;
+	KUNIT_ASSERT_NOT_NULL(test, conn_state);
+
+	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 10);
+	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_YUV420);
+	KUNIT_EXPECT_EQ(test, conn_state->hdmi.tmds_char_rate, yuv420_only_mode->clock * 625);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 /*
  * Test that if:
  * - We have an HDMI connector supporting both RGB and YUV422 and up to
@@ -1292,7 +1361,7 @@ static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
  * Then we will prefer to keep the RGB format with a lower bpc over
  * picking YUV422.
  */
-static void drm_test_check_max_tmds_rate_format_fallback(struct kunit *test)
+static void drm_test_check_max_tmds_rate_format_fallback_yuv422(struct kunit *test)
 {
 	struct drm_atomic_helper_connector_hdmi_priv *priv;
 	struct drm_modeset_acquire_ctx ctx;
@@ -1351,6 +1420,79 @@ static void drm_test_check_max_tmds_rate_format_fallback(struct kunit *test)
 	drm_modeset_acquire_fini(&ctx);
 }
 
+/*
+ * Test that if:
+ * - We have an HDMI connector supporting both RGB and YUV420 and up to
+ *   12 bpc
+ * - The chosen mode has a TMDS character rate higher than the display
+ *   supports in RGB/10bpc but lower than the display supports in
+ *   RGB/8bpc
+ * - The chosen mode has a TMDS character rate lower than the display
+ *   supports in YUV420/12bpc.
+ *
+ * Then we will prefer to keep the RGB format with a lower bpc over
+ * picking YUV420.
+ */
+static void drm_test_check_max_tmds_rate_format_fallback_yuv420(struct kunit *test)
+{
+	struct drm_atomic_helper_connector_hdmi_priv *priv;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_connector_state *conn_state;
+	struct drm_display_info *info;
+	struct drm_display_mode *preferred;
+	unsigned long long rate;
+	struct drm_connector *conn;
+	struct drm_device *drm;
+	struct drm_crtc *crtc;
+	int ret;
+
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB) |
+				BIT(HDMI_COLORSPACE_YUV420),
+				12,
+				test_edid_hdmi_4k_rgb_yuv420_dc_max_340mhz);
+	KUNIT_ASSERT_NOT_NULL(test, priv);
+
+	drm = &priv->drm;
+	crtc = priv->crtc;
+	conn = &priv->connector;
+	info = &conn->display_info;
+	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
+	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
+	KUNIT_ASSERT_TRUE(test, conn->ycbcr_420_allowed);
+
+	preferred = find_preferred_mode(conn);
+	KUNIT_ASSERT_NOT_NULL(test, preferred);
+	KUNIT_ASSERT_FALSE(test, preferred->flags & DRM_MODE_FLAG_DBLCLK);
+	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_also(info, preferred));
+
+	rate = drm_hdmi_compute_mode_clock(preferred, 8, HDMI_COLORSPACE_RGB);
+	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
+
+	rate = drm_hdmi_compute_mode_clock(preferred, 10, HDMI_COLORSPACE_RGB);
+	KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
+
+	rate = drm_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_YUV420);
+	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
+						     crtc, conn,
+						     preferred,
+						     &ctx);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	conn_state = conn->state;
+	KUNIT_ASSERT_NOT_NULL(test, conn_state);
+
+	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 8);
+	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 /*
  * Test that if a driver and screen supports RGB and YUV formats, and we
  * try to set the VIC 1 mode, we end up with 8bpc RGB even if we could
@@ -1738,8 +1880,10 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
 	KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_not_changed),
 	KUNIT_CASE(drm_test_check_disable_connector),
 	KUNIT_CASE(drm_test_check_hdmi_funcs_reject_rate),
-	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback),
-	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback),
+	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback_rgb),
+	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback_yuv420),
+	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback_yuv422),
+	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback_yuv420),
 	KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_changed),
 	KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_not_changed),
 	KUNIT_CASE(drm_test_check_output_bpc_dvi),
diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h
index ff316e6114d65c96b1338cd83bc0d8d9e6e143e9..8e9086df20c690f34623d7858c716032d77d0c26 100644
--- a/drivers/gpu/drm/tests/drm_kunit_edid.h
+++ b/drivers/gpu/drm/tests/drm_kunit_edid.h
@@ -695,4 +695,114 @@ static const unsigned char test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz[
 	0x00, 0x00, 0x00, 0xca
 };
 
+/*
+ * edid-decode (hex):
+ *
+ * 00 ff ff ff ff ff ff 00 31 d8 34 00 00 00 00 00
+ * ff 23 01 03 80 60 36 78 0f ee 91 a3 54 4c 99 26
+ * 0f 50 54 20 00 00 01 01 01 01 01 01 01 01 01 01
+ * 01 01 01 01 01 01 04 74 00 30 f2 70 5a 80 b0 58
+ * 8a 00 40 84 63 00 00 1e 00 00 00 fc 00 54 65 73
+ * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 18
+ * 55 18 5e 22 00 0a 20 20 20 20 20 20 00 00 00 10
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 ce
+ *
+ * 02 03 27 31 41 5f 6c 03 0c 00 10 00 78 44 20 00
+ * 00 01 03 6d d8 5d c4 01 44 80 07 00 00 00 00 00
+ * 00 e3 0f 01 00 e1 0e 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 84
+ *
+ * ----------------
+ *
+ * Block 0, Base EDID:
+ *   EDID Structure Version & Revision: 1.3
+ *   Vendor & Product Identification:
+ *     Manufacturer: LNX
+ *     Model: 52
+ *     Model year: 2025
+ *   Basic Display Parameters & Features:
+ *     Digital display
+ *     Maximum image size: 96 cm x 54 cm
+ *     Gamma: 2.20
+ *     RGB color display
+ *     Default (sRGB) color space is primary color space
+ *     First detailed timing is the preferred timing
+ *     Supports GTF timings within operating range
+ *   Color Characteristics:
+ *     Red  : 0.6396, 0.3300
+ *     Green: 0.2998, 0.5996
+ *     Blue : 0.1503, 0.0595
+ *     White: 0.3125, 0.3291
+ *   Established Timings I & II:
+ *     DMT 0x04:   640x480    59.940476 Hz   4:3     31.469 kHz     25.175000 MHz
+ *   Standard Timings: none
+ *   Detailed Timing Descriptors:
+ *     DTD 1:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz (1600 mm x 900 mm)
+ *                  Hfront  176 Hsync  88 Hback  296 Hpol P
+ *                  Vfront    8 Vsync  10 Vback   72 Vpol P
+ *     Display Product Name: 'Test EDID'
+ *     Display Range Limits:
+ *       Monitor ranges (GTF): 24-85 Hz V, 24-94 kHz H, max dotclock 340 MHz
+ *     Dummy Descriptor:
+ *   Extension blocks: 1
+ * Checksum: 0xce
+ *
+ * ----------------
+ *
+ * Block 1, CTA-861 Extension Block:
+ *   Revision: 3
+ *   Supports YCbCr 4:4:4
+ *   Supports YCbCr 4:2:2
+ *   Native detailed modes: 1
+ *   Video Data Block:
+ *     VIC  95:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz
+ *   Vendor-Specific Data Block (HDMI), OUI 00-0C-03:
+ *     Source physical address: 1.0.0.0
+ *     DC_48bit
+ *     DC_36bit
+ *     DC_30bit
+ *     DC_Y444
+ *     Maximum TMDS clock: 340 MHz
+ *     Extended HDMI video details:
+ *   Vendor-Specific Data Block (HDMI Forum), OUI C4-5D-D8:
+ *     Version: 1
+ *     Maximum TMDS Character Rate: 340 MHz
+ *     SCDC Present
+ *     Supports 16-bits/component Deep Color 4:2:0 Pixel Encoding
+ *     Supports 12-bits/component Deep Color 4:2:0 Pixel Encoding
+ *     Supports 10-bits/component Deep Color 4:2:0 Pixel Encoding
+ *   YCbCr 4:2:0 Capability Map Data Block:
+ *     VIC  95:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz
+ *   YCbCr 4:2:0 Video Data Block:
+ * Checksum: 0x84
+ */
+static const unsigned char test_edid_hdmi_4k_rgb_yuv420_dc_max_340mhz[] = {
+	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x34, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0xff, 0x23, 0x01, 0x03, 0x80, 0x60, 0x36, 0x78,
+	0x0f, 0xee, 0x91, 0xa3, 0x54, 0x4c, 0x99, 0x26, 0x0f, 0x50, 0x54, 0x20,
+	0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
+	0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x04, 0x74, 0x00, 0x30, 0xf2, 0x70,
+	0x5a, 0x80, 0xb0, 0x58, 0x8a, 0x00, 0x40, 0x84, 0x63, 0x00, 0x00, 0x1e,
+	0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44,
+	0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x18,
+	0x55, 0x18, 0x5e, 0x22, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+	0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0xce, 0x02, 0x03, 0x27, 0x31,
+	0x41, 0x5f, 0x6c, 0x03, 0x0c, 0x00, 0x10, 0x00, 0x78, 0x44, 0x20, 0x00,
+	0x00, 0x01, 0x03, 0x6d, 0xd8, 0x5d, 0xc4, 0x01, 0x44, 0x80, 0x07, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0xe3, 0x0f, 0x01, 0x00, 0xe1, 0x0e, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x84
+};
+
 #endif // DRM_KUNIT_EDID_H_

-- 
2.49.0

Re: [PATCH v3 14/15] drm/tests: hdmi: Add max TMDS rate fallback tests for YUV420 mode

[Maxime Ripard]

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

On Wed, Mar 26, 2025 at 12:20:03PM +0200, Cristian Ciocaltea wrote:
> Provide tests to verify drm_atomic_helper_connector_hdmi_check() helper
> fallback behavior when using YUV420 output.
> 
> Also rename drm_test_check_max_tmds_rate_{bpc|format}_fallback() to
> better differentiate from the newly introduced *_yuv420() variants.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 152 ++++++++++++++++++++-
>  drivers/gpu/drm/tests/drm_kunit_edid.h             | 110 +++++++++++++++
>  2 files changed, 258 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index 3fae7ccf65309a1d8a4acf12de961713b9163096..99bedb2d6f555b3b140256000dfa7491d2a8f515 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -1224,7 +1224,7 @@ static void drm_test_check_hdmi_funcs_reject_rate(struct kunit *test)
>   * Then we will pick the latter, and the computed TMDS character rate
>   * will be equal to 1.25 times the mode pixel clock.
>   */
> -static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
> +static void drm_test_check_max_tmds_rate_bpc_fallback_rgb(struct kunit *test)
>  {
>  	struct drm_atomic_helper_connector_hdmi_priv *priv;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -1279,6 +1279,75 @@ static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
>  	drm_modeset_acquire_fini(&ctx);
>  }
>  
> +/*
> + * Test that if:
> + * - We have an HDMI connector supporting both RGB and YUV420

I assume the monitor also supports both?

> + * - The chosen mode can be supported in YUV420 output format only
> + * - The chosen mode has a TMDS character rate higher than the display
> + *   supports in YUV420/12bpc
> + * - The chosen mode has a TMDS character rate lower than the display
> + *   supports in YUV420/10bpc.
> + *
> + * Then we will pick the latter, and the computed TMDS character rate
> + * will be equal to 1.25 * 0.5 times the mode pixel clock.
> + */
> +static void drm_test_check_max_tmds_rate_bpc_fallback_yuv420(struct kunit *test)
> +{
> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_connector_state *conn_state;
> +	struct drm_display_info *info;
> +	struct drm_display_mode *yuv420_only_mode;
> +	unsigned long long rate;
> +	struct drm_connector *conn;
> +	struct drm_device *drm;
> +	struct drm_crtc *crtc;
> +	int ret;
> +
> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
> +				BIT(HDMI_COLORSPACE_RGB) |
> +				BIT(HDMI_COLORSPACE_YUV420),
> +				12,
> +				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +	drm = &priv->drm;
> +	crtc = priv->crtc;
> +	conn = &priv->connector;
> +	info = &conn->display_info;
> +	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +	KUNIT_ASSERT_TRUE(test, conn->ycbcr_420_allowed);
> +
> +	yuv420_only_mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
> +	KUNIT_ASSERT_NOT_NULL(test, yuv420_only_mode);
> +	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_only(info, yuv420_only_mode));
> +
> +	rate = drm_hdmi_compute_mode_clock(yuv420_only_mode, 12, HDMI_COLORSPACE_YUV420);
> +	KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
> +
> +	rate = drm_hdmi_compute_mode_clock(yuv420_only_mode, 10, HDMI_COLORSPACE_YUV420);
> +	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
> +						     crtc, conn,
> +						     yuv420_only_mode,
> +						     &ctx);
> +	KUNIT_EXPECT_EQ(test, ret, 0);

We need to handle EDEADLK

> +
> +	conn_state = conn->state;
> +	KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 10);
> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_YUV420);
> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.tmds_char_rate, yuv420_only_mode->clock * 625);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +
>  /*
>   * Test that if:
>   * - We have an HDMI connector supporting both RGB and YUV422 and up to
> @@ -1292,7 +1361,7 @@ static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
>   * Then we will prefer to keep the RGB format with a lower bpc over
>   * picking YUV422.
>   */
> -static void drm_test_check_max_tmds_rate_format_fallback(struct kunit *test)
> +static void drm_test_check_max_tmds_rate_format_fallback_yuv422(struct kunit *test)
>  {
>  	struct drm_atomic_helper_connector_hdmi_priv *priv;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -1351,6 +1420,79 @@ static void drm_test_check_max_tmds_rate_format_fallback(struct kunit *test)
>  	drm_modeset_acquire_fini(&ctx);
>  }
>  
> +/*
> + * Test that if:
> + * - We have an HDMI connector supporting both RGB and YUV420 and up to
> + *   12 bpc
> + * - The chosen mode has a TMDS character rate higher than the display
> + *   supports in RGB/10bpc but lower than the display supports in
> + *   RGB/8bpc
> + * - The chosen mode has a TMDS character rate lower than the display
> + *   supports in YUV420/12bpc.
> + *
> + * Then we will prefer to keep the RGB format with a lower bpc over
> + * picking YUV420.
> + */
> +static void drm_test_check_max_tmds_rate_format_fallback_yuv420(struct kunit *test)

Are you sure about the name here? It looks like we're not testing the
YUV420 fallback at all?

> +{
> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_connector_state *conn_state;
> +	struct drm_display_info *info;
> +	struct drm_display_mode *preferred;
> +	unsigned long long rate;
> +	struct drm_connector *conn;
> +	struct drm_device *drm;
> +	struct drm_crtc *crtc;
> +	int ret;
> +
> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
> +				BIT(HDMI_COLORSPACE_RGB) |
> +				BIT(HDMI_COLORSPACE_YUV420),
> +				12,
> +				test_edid_hdmi_4k_rgb_yuv420_dc_max_340mhz);
> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +	drm = &priv->drm;
> +	crtc = priv->crtc;
> +	conn = &priv->connector;
> +	info = &conn->display_info;
> +	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
> +	KUNIT_ASSERT_TRUE(test, conn->ycbcr_420_allowed);
> +
> +	preferred = find_preferred_mode(conn);
> +	KUNIT_ASSERT_NOT_NULL(test, preferred);
> +	KUNIT_ASSERT_FALSE(test, preferred->flags & DRM_MODE_FLAG_DBLCLK);
> +	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_also(info, preferred));
> +
> +	rate = drm_hdmi_compute_mode_clock(preferred, 8, HDMI_COLORSPACE_RGB);
> +	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +	rate = drm_hdmi_compute_mode_clock(preferred, 10, HDMI_COLORSPACE_RGB);
> +	KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
> +
> +	rate = drm_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_YUV420);
> +	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
> +						     crtc, conn,
> +						     preferred,
> +						     &ctx);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +	conn_state = conn->state;
> +	KUNIT_ASSERT_NOT_NULL(test, conn_state);
> +
> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 8);
> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}

>  /*
>   * Test that if a driver and screen supports RGB and YUV formats, and we
>   * try to set the VIC 1 mode, we end up with 8bpc RGB even if we could
> @@ -1738,8 +1880,10 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
>  	KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_not_changed),
>  	KUNIT_CASE(drm_test_check_disable_connector),
>  	KUNIT_CASE(drm_test_check_hdmi_funcs_reject_rate),
> -	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback),
> -	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback),

These name changes belong in a separate patch

> +	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback_rgb),
> +	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback_yuv420),
> +	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback_yuv422),
> +	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback_yuv420),

We should also test what happens when the monitor or connector doesn't
support YUV420, and we'd still need to fallback. In such a case, we
should return an error.

>  	KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_changed),
>  	KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_not_changed),
>  	KUNIT_CASE(drm_test_check_output_bpc_dvi),
> diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h
> index ff316e6114d65c96b1338cd83bc0d8d9e6e143e9..8e9086df20c690f34623d7858c716032d77d0c26 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_edid.h
> +++ b/drivers/gpu/drm/tests/drm_kunit_edid.h
> @@ -695,4 +695,114 @@ static const unsigned char test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz[
>  	0x00, 0x00, 0x00, 0xca
>  };
>  
> +/*
> + * edid-decode (hex):
> + *
> + * 00 ff ff ff ff ff ff 00 31 d8 34 00 00 00 00 00
> + * ff 23 01 03 80 60 36 78 0f ee 91 a3 54 4c 99 26
> + * 0f 50 54 20 00 00 01 01 01 01 01 01 01 01 01 01
> + * 01 01 01 01 01 01 04 74 00 30 f2 70 5a 80 b0 58
> + * 8a 00 40 84 63 00 00 1e 00 00 00 fc 00 54 65 73
> + * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 18
> + * 55 18 5e 22 00 0a 20 20 20 20 20 20 00 00 00 10
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 ce
> + *
> + * 02 03 27 31 41 5f 6c 03 0c 00 10 00 78 44 20 00
> + * 00 01 03 6d d8 5d c4 01 44 80 07 00 00 00 00 00
> + * 00 e3 0f 01 00 e1 0e 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 84
> + *
> + * ----------------
> + *
> + * Block 0, Base EDID:
> + *   EDID Structure Version & Revision: 1.3
> + *   Vendor & Product Identification:
> + *     Manufacturer: LNX
> + *     Model: 52
> + *     Model year: 2025
> + *   Basic Display Parameters & Features:
> + *     Digital display
> + *     Maximum image size: 96 cm x 54 cm
> + *     Gamma: 2.20
> + *     RGB color display
> + *     Default (sRGB) color space is primary color space
> + *     First detailed timing is the preferred timing
> + *     Supports GTF timings within operating range
> + *   Color Characteristics:
> + *     Red  : 0.6396, 0.3300
> + *     Green: 0.2998, 0.5996
> + *     Blue : 0.1503, 0.0595
> + *     White: 0.3125, 0.3291
> + *   Established Timings I & II:
> + *     DMT 0x04:   640x480    59.940476 Hz   4:3     31.469 kHz     25.175000 MHz
> + *   Standard Timings: none
> + *   Detailed Timing Descriptors:
> + *     DTD 1:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz (1600 mm x 900 mm)
> + *                  Hfront  176 Hsync  88 Hback  296 Hpol P
> + *                  Vfront    8 Vsync  10 Vback   72 Vpol P
> + *     Display Product Name: 'Test EDID'
> + *     Display Range Limits:
> + *       Monitor ranges (GTF): 24-85 Hz V, 24-94 kHz H, max dotclock 340 MHz
> + *     Dummy Descriptor:
> + *   Extension blocks: 1
> + * Checksum: 0xce
> + *
> + * ----------------
> + *
> + * Block 1, CTA-861 Extension Block:
> + *   Revision: 3
> + *   Supports YCbCr 4:4:4
> + *   Supports YCbCr 4:2:2
> + *   Native detailed modes: 1
> + *   Video Data Block:
> + *     VIC  95:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz
> + *   Vendor-Specific Data Block (HDMI), OUI 00-0C-03:
> + *     Source physical address: 1.0.0.0
> + *     DC_48bit
> + *     DC_36bit
> + *     DC_30bit
> + *     DC_Y444
> + *     Maximum TMDS clock: 340 MHz
> + *     Extended HDMI video details:
> + *   Vendor-Specific Data Block (HDMI Forum), OUI C4-5D-D8:
> + *     Version: 1
> + *     Maximum TMDS Character Rate: 340 MHz
> + *     SCDC Present
> + *     Supports 16-bits/component Deep Color 4:2:0 Pixel Encoding
> + *     Supports 12-bits/component Deep Color 4:2:0 Pixel Encoding
> + *     Supports 10-bits/component Deep Color 4:2:0 Pixel Encoding
> + *   YCbCr 4:2:0 Capability Map Data Block:
> + *     VIC  95:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz
> + *   YCbCr 4:2:0 Video Data Block:
> + * Checksum: 0x84
> + */
> +static const unsigned char test_edid_hdmi_4k_rgb_yuv420_dc_max_340mhz[] = {
> +	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x34, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0xff, 0x23, 0x01, 0x03, 0x80, 0x60, 0x36, 0x78,
> +	0x0f, 0xee, 0x91, 0xa3, 0x54, 0x4c, 0x99, 0x26, 0x0f, 0x50, 0x54, 0x20,
> +	0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
> +	0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x04, 0x74, 0x00, 0x30, 0xf2, 0x70,
> +	0x5a, 0x80, 0xb0, 0x58, 0x8a, 0x00, 0x40, 0x84, 0x63, 0x00, 0x00, 0x1e,
> +	0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44,
> +	0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x18,
> +	0x55, 0x18, 0x5e, 0x22, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
> +	0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0xce, 0x02, 0x03, 0x27, 0x31,
> +	0x41, 0x5f, 0x6c, 0x03, 0x0c, 0x00, 0x10, 0x00, 0x78, 0x44, 0x20, 0x00,
> +	0x00, 0x01, 0x03, 0x6d, 0xd8, 0x5d, 0xc4, 0x01, 0x44, 0x80, 0x07, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0xe3, 0x0f, 0x01, 0x00, 0xe1, 0x0e, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x00, 0x00, 0x84
> +};

This should be a separate patch as well, but most importantly it's
pretty hard to know the difference with the one you introduced in a
previous patch. We should document it better than just with edid-decode.
Also, how did you generate it?

Maxime

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

Re: [PATCH v3 14/15] drm/tests: hdmi: Add max TMDS rate fallback tests for YUV420 mode

[Cristian Ciocaltea]

On 4/10/25 11:35 AM, Maxime Ripard wrote:
> On Wed, Mar 26, 2025 at 12:20:03PM +0200, Cristian Ciocaltea wrote:
>> Provide tests to verify drm_atomic_helper_connector_hdmi_check() helper
>> fallback behavior when using YUV420 output.
>>
>> Also rename drm_test_check_max_tmds_rate_{bpc|format}_fallback() to
>> better differentiate from the newly introduced *_yuv420() variants.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 152 ++++++++++++++++++++-
>>  drivers/gpu/drm/tests/drm_kunit_edid.h             | 110 +++++++++++++++
>>  2 files changed, 258 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> index 3fae7ccf65309a1d8a4acf12de961713b9163096..99bedb2d6f555b3b140256000dfa7491d2a8f515 100644
>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> @@ -1224,7 +1224,7 @@ static void drm_test_check_hdmi_funcs_reject_rate(struct kunit *test)
>>   * Then we will pick the latter, and the computed TMDS character rate
>>   * will be equal to 1.25 times the mode pixel clock.
>>   */
>> -static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
>> +static void drm_test_check_max_tmds_rate_bpc_fallback_rgb(struct kunit *test)
>>  {
>>  	struct drm_atomic_helper_connector_hdmi_priv *priv;
>>  	struct drm_modeset_acquire_ctx ctx;
>> @@ -1279,6 +1279,75 @@ static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
>>  	drm_modeset_acquire_fini(&ctx);
>>  }
>>  
>> +/*
>> + * Test that if:
>> + * - We have an HDMI connector supporting both RGB and YUV420
> 
> I assume the monitor also supports both?

Yes, I will improve the comment a bit.

>> + * - The chosen mode can be supported in YUV420 output format only
>> + * - The chosen mode has a TMDS character rate higher than the display
>> + *   supports in YUV420/12bpc
>> + * - The chosen mode has a TMDS character rate lower than the display
>> + *   supports in YUV420/10bpc.
>> + *
>> + * Then we will pick the latter, and the computed TMDS character rate
>> + * will be equal to 1.25 * 0.5 times the mode pixel clock.
>> + */
>> +static void drm_test_check_max_tmds_rate_bpc_fallback_yuv420(struct kunit *test)
>> +{
>> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
>> +	struct drm_modeset_acquire_ctx ctx;
>> +	struct drm_connector_state *conn_state;
>> +	struct drm_display_info *info;
>> +	struct drm_display_mode *yuv420_only_mode;
>> +	unsigned long long rate;
>> +	struct drm_connector *conn;
>> +	struct drm_device *drm;
>> +	struct drm_crtc *crtc;
>> +	int ret;
>> +
>> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
>> +				BIT(HDMI_COLORSPACE_RGB) |
>> +				BIT(HDMI_COLORSPACE_YUV420),
>> +				12,
>> +				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
>> +	KUNIT_ASSERT_NOT_NULL(test, priv);
>> +
>> +	drm = &priv->drm;
>> +	crtc = priv->crtc;
>> +	conn = &priv->connector;
>> +	info = &conn->display_info;
>> +	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
>> +	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
>> +	KUNIT_ASSERT_TRUE(test, conn->ycbcr_420_allowed);
>> +
>> +	yuv420_only_mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
>> +	KUNIT_ASSERT_NOT_NULL(test, yuv420_only_mode);
>> +	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_only(info, yuv420_only_mode));
>> +
>> +	rate = drm_hdmi_compute_mode_clock(yuv420_only_mode, 12, HDMI_COLORSPACE_YUV420);
>> +	KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
>> +
>> +	rate = drm_hdmi_compute_mode_clock(yuv420_only_mode, 10, HDMI_COLORSPACE_YUV420);
>> +	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
>> +
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
>> +						     crtc, conn,
>> +						     yuv420_only_mode,
>> +						     &ctx);
>> +	KUNIT_EXPECT_EQ(test, ret, 0);
> 
> We need to handle EDEADLK

Yeah, that's the open disscussion in the previous patch.

>> +
>> +	conn_state = conn->state;
>> +	KUNIT_ASSERT_NOT_NULL(test, conn_state);
>> +
>> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 10);
>> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_YUV420);
>> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.tmds_char_rate, yuv420_only_mode->clock * 625);
>> +
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +}
>> +
>>  /*
>>   * Test that if:
>>   * - We have an HDMI connector supporting both RGB and YUV422 and up to
>> @@ -1292,7 +1361,7 @@ static void drm_test_check_max_tmds_rate_bpc_fallback(struct kunit *test)
>>   * Then we will prefer to keep the RGB format with a lower bpc over
>>   * picking YUV422.
>>   */
>> -static void drm_test_check_max_tmds_rate_format_fallback(struct kunit *test)
>> +static void drm_test_check_max_tmds_rate_format_fallback_yuv422(struct kunit *test)
>>  {
>>  	struct drm_atomic_helper_connector_hdmi_priv *priv;
>>  	struct drm_modeset_acquire_ctx ctx;
>> @@ -1351,6 +1420,79 @@ static void drm_test_check_max_tmds_rate_format_fallback(struct kunit *test)
>>  	drm_modeset_acquire_fini(&ctx);
>>  }
>>  
>> +/*
>> + * Test that if:
>> + * - We have an HDMI connector supporting both RGB and YUV420 and up to
>> + *   12 bpc
>> + * - The chosen mode has a TMDS character rate higher than the display
>> + *   supports in RGB/10bpc but lower than the display supports in
>> + *   RGB/8bpc
>> + * - The chosen mode has a TMDS character rate lower than the display
>> + *   supports in YUV420/12bpc.
>> + *
>> + * Then we will prefer to keep the RGB format with a lower bpc over
>> + * picking YUV420.
>> + */
>> +static void drm_test_check_max_tmds_rate_format_fallback_yuv420(struct kunit *test)
> 
> Are you sure about the name here? It looks like we're not testing the
> YUV420 fallback at all?

The name is indead missleading, the '_yuv420' suffix is mainly used to
differentiate this test case from the above using '_yuv422' - both keep
RGB format eventually.

What about replacing '_format_fallback_' with '_bpc_fallback_ignore_' in
both cases?  E.g.:

  drm_test_check_max_tmds_rate_bpc_fallback_ignore_yuv422()
  drm_test_check_max_tmds_rate_bpc_fallback_ignore_yuv420()

> 
>> +{
>> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
>> +	struct drm_modeset_acquire_ctx ctx;
>> +	struct drm_connector_state *conn_state;
>> +	struct drm_display_info *info;
>> +	struct drm_display_mode *preferred;
>> +	unsigned long long rate;
>> +	struct drm_connector *conn;
>> +	struct drm_device *drm;
>> +	struct drm_crtc *crtc;
>> +	int ret;
>> +
>> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
>> +				BIT(HDMI_COLORSPACE_RGB) |
>> +				BIT(HDMI_COLORSPACE_YUV420),
>> +				12,
>> +				test_edid_hdmi_4k_rgb_yuv420_dc_max_340mhz);
>> +	KUNIT_ASSERT_NOT_NULL(test, priv);
>> +
>> +	drm = &priv->drm;
>> +	crtc = priv->crtc;
>> +	conn = &priv->connector;
>> +	info = &conn->display_info;
>> +	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
>> +	KUNIT_ASSERT_GT(test, info->max_tmds_clock, 0);
>> +	KUNIT_ASSERT_TRUE(test, conn->ycbcr_420_allowed);
>> +
>> +	preferred = find_preferred_mode(conn);
>> +	KUNIT_ASSERT_NOT_NULL(test, preferred);
>> +	KUNIT_ASSERT_FALSE(test, preferred->flags & DRM_MODE_FLAG_DBLCLK);
>> +	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_also(info, preferred));
>> +
>> +	rate = drm_hdmi_compute_mode_clock(preferred, 8, HDMI_COLORSPACE_RGB);
>> +	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
>> +
>> +	rate = drm_hdmi_compute_mode_clock(preferred, 10, HDMI_COLORSPACE_RGB);
>> +	KUNIT_ASSERT_GT(test, rate, info->max_tmds_clock * 1000);
>> +
>> +	rate = drm_hdmi_compute_mode_clock(preferred, 12, HDMI_COLORSPACE_YUV420);
>> +	KUNIT_ASSERT_LT(test, rate, info->max_tmds_clock * 1000);
>> +
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
>> +						     crtc, conn,
>> +						     preferred,
>> +						     &ctx);
>> +	KUNIT_EXPECT_EQ(test, ret, 0);
>> +
>> +	conn_state = conn->state;
>> +	KUNIT_ASSERT_NOT_NULL(test, conn_state);
>> +
>> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_bpc, 8);
>> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
>> +
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +}
> 
>>  /*
>>   * Test that if a driver and screen supports RGB and YUV formats, and we
>>   * try to set the VIC 1 mode, we end up with 8bpc RGB even if we could
>> @@ -1738,8 +1880,10 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
>>  	KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_not_changed),
>>  	KUNIT_CASE(drm_test_check_disable_connector),
>>  	KUNIT_CASE(drm_test_check_hdmi_funcs_reject_rate),
>> -	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback),
>> -	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback),
> 
> These name changes belong in a separate patch

Ack.

>> +	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback_rgb),
>> +	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback_yuv420),
>> +	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback_yuv422),
>> +	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback_yuv420),
> 
> We should also test what happens when the monitor or connector doesn't
> support YUV420, and we'd still need to fallback. In such a case, we
> should return an error.

Ack.

>>  	KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_changed),
>>  	KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_not_changed),
>>  	KUNIT_CASE(drm_test_check_output_bpc_dvi),
>> diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h
>> index ff316e6114d65c96b1338cd83bc0d8d9e6e143e9..8e9086df20c690f34623d7858c716032d77d0c26 100644
>> --- a/drivers/gpu/drm/tests/drm_kunit_edid.h
>> +++ b/drivers/gpu/drm/tests/drm_kunit_edid.h
>> @@ -695,4 +695,114 @@ static const unsigned char test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz[
>>  	0x00, 0x00, 0x00, 0xca
>>  };
>>  
>> +/*
>> + * edid-decode (hex):
>> + *
>> + * 00 ff ff ff ff ff ff 00 31 d8 34 00 00 00 00 00
>> + * ff 23 01 03 80 60 36 78 0f ee 91 a3 54 4c 99 26
>> + * 0f 50 54 20 00 00 01 01 01 01 01 01 01 01 01 01
>> + * 01 01 01 01 01 01 04 74 00 30 f2 70 5a 80 b0 58
>> + * 8a 00 40 84 63 00 00 1e 00 00 00 fc 00 54 65 73
>> + * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 18
>> + * 55 18 5e 22 00 0a 20 20 20 20 20 20 00 00 00 10
>> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 ce
>> + *
>> + * 02 03 27 31 41 5f 6c 03 0c 00 10 00 78 44 20 00
>> + * 00 01 03 6d d8 5d c4 01 44 80 07 00 00 00 00 00
>> + * 00 e3 0f 01 00 e1 0e 00 00 00 00 00 00 00 00 00
>> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 84
>> + *
>> + * ----------------
>> + *
>> + * Block 0, Base EDID:
>> + *   EDID Structure Version & Revision: 1.3
>> + *   Vendor & Product Identification:
>> + *     Manufacturer: LNX
>> + *     Model: 52
>> + *     Model year: 2025
>> + *   Basic Display Parameters & Features:
>> + *     Digital display
>> + *     Maximum image size: 96 cm x 54 cm
>> + *     Gamma: 2.20
>> + *     RGB color display
>> + *     Default (sRGB) color space is primary color space
>> + *     First detailed timing is the preferred timing
>> + *     Supports GTF timings within operating range
>> + *   Color Characteristics:
>> + *     Red  : 0.6396, 0.3300
>> + *     Green: 0.2998, 0.5996
>> + *     Blue : 0.1503, 0.0595
>> + *     White: 0.3125, 0.3291
>> + *   Established Timings I & II:
>> + *     DMT 0x04:   640x480    59.940476 Hz   4:3     31.469 kHz     25.175000 MHz
>> + *   Standard Timings: none
>> + *   Detailed Timing Descriptors:
>> + *     DTD 1:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz (1600 mm x 900 mm)
>> + *                  Hfront  176 Hsync  88 Hback  296 Hpol P
>> + *                  Vfront    8 Vsync  10 Vback   72 Vpol P
>> + *     Display Product Name: 'Test EDID'
>> + *     Display Range Limits:
>> + *       Monitor ranges (GTF): 24-85 Hz V, 24-94 kHz H, max dotclock 340 MHz
>> + *     Dummy Descriptor:
>> + *   Extension blocks: 1
>> + * Checksum: 0xce
>> + *
>> + * ----------------
>> + *
>> + * Block 1, CTA-861 Extension Block:
>> + *   Revision: 3
>> + *   Supports YCbCr 4:4:4
>> + *   Supports YCbCr 4:2:2
>> + *   Native detailed modes: 1
>> + *   Video Data Block:
>> + *     VIC  95:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz
>> + *   Vendor-Specific Data Block (HDMI), OUI 00-0C-03:
>> + *     Source physical address: 1.0.0.0
>> + *     DC_48bit
>> + *     DC_36bit
>> + *     DC_30bit
>> + *     DC_Y444
>> + *     Maximum TMDS clock: 340 MHz
>> + *     Extended HDMI video details:
>> + *   Vendor-Specific Data Block (HDMI Forum), OUI C4-5D-D8:
>> + *     Version: 1
>> + *     Maximum TMDS Character Rate: 340 MHz
>> + *     SCDC Present
>> + *     Supports 16-bits/component Deep Color 4:2:0 Pixel Encoding
>> + *     Supports 12-bits/component Deep Color 4:2:0 Pixel Encoding
>> + *     Supports 10-bits/component Deep Color 4:2:0 Pixel Encoding
>> + *   YCbCr 4:2:0 Capability Map Data Block:
>> + *     VIC  95:  3840x2160   30.000000 Hz  16:9     67.500 kHz    297.000000 MHz
>> + *   YCbCr 4:2:0 Video Data Block:
>> + * Checksum: 0x84
>> + */
>> +static const unsigned char test_edid_hdmi_4k_rgb_yuv420_dc_max_340mhz[] = {
>> +	0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x34, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0xff, 0x23, 0x01, 0x03, 0x80, 0x60, 0x36, 0x78,
>> +	0x0f, 0xee, 0x91, 0xa3, 0x54, 0x4c, 0x99, 0x26, 0x0f, 0x50, 0x54, 0x20,
>> +	0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
>> +	0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x04, 0x74, 0x00, 0x30, 0xf2, 0x70,
>> +	0x5a, 0x80, 0xb0, 0x58, 0x8a, 0x00, 0x40, 0x84, 0x63, 0x00, 0x00, 0x1e,
>> +	0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44,
>> +	0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x18,
>> +	0x55, 0x18, 0x5e, 0x22, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
>> +	0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0xce, 0x02, 0x03, 0x27, 0x31,
>> +	0x41, 0x5f, 0x6c, 0x03, 0x0c, 0x00, 0x10, 0x00, 0x78, 0x44, 0x20, 0x00,
>> +	0x00, 0x01, 0x03, 0x6d, 0xd8, 0x5d, 0xc4, 0x01, 0x44, 0x80, 0x07, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0xe3, 0x0f, 0x01, 0x00, 0xe1, 0x0e, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +	0x00, 0x00, 0x00, 0x84
>> +};
> 
> This should be a separate patch as well, but most importantly it's
> pretty hard to know the difference with the one you introduced in a
> previous patch. We should document it better than just with edid-decode.

Ack.

> Also, how did you generate it?

Oh, that was a quite painful experience :-(.  Started from a full
feature EDID (grabbed from real HW), then used wxedid to drop unneeded
stuff up to the bare minimum (while also trying to deal with some
crashes or unexpected behavior of the tool).  And finally raw editing
the hex data since wxedid is currently not able to handle
'Vendor-Specific Data Block (HDMI Forum), OUI C4-5D-D8'.

[PATCH v3 15/15] drm/tests: hdmi: Add test for unsuccessful forced fallback to YUV420

[Cristian Ciocaltea]

Provide test to verify a forced fallback to YUV420 output cannot succeed
when driver doesn't advertise YUV420 support.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index 99bedb2d6f555b3b140256000dfa7491d2a8f515..c2976b42aa2aacd2a68a871bffe97e795ca713d4 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -1493,6 +1493,51 @@ static void drm_test_check_max_tmds_rate_format_fallback_yuv420(struct kunit *te
 	drm_modeset_acquire_fini(&ctx);
 }
 
+/*
+ * Test that if a driver supports only RGB, but the chosen mode can be
+ * supported by the screen only in YUV420 output format, we end up with
+ * an unsuccessful forced fallback attempt.
+ */
+static void drm_test_check_driver_unsupported_fallback_yuv420(struct kunit *test)
+{
+	struct drm_atomic_helper_connector_hdmi_priv *priv;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_display_info *info;
+	struct drm_display_mode *yuv420_only_mode;
+	struct drm_connector *conn;
+	struct drm_device *drm;
+	struct drm_crtc *crtc;
+	int ret;
+
+	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
+				BIT(HDMI_COLORSPACE_RGB),
+				12,
+				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
+	KUNIT_ASSERT_NOT_NULL(test, priv);
+
+	drm = &priv->drm;
+	crtc = priv->crtc;
+	conn = &priv->connector;
+	info = &conn->display_info;
+	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
+	KUNIT_ASSERT_FALSE(test, conn->ycbcr_420_allowed);
+
+	yuv420_only_mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
+	KUNIT_ASSERT_NOT_NULL(test, yuv420_only_mode);
+	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_only(info, yuv420_only_mode));
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
+						     crtc, conn,
+						     yuv420_only_mode,
+						     &ctx);
+	KUNIT_EXPECT_LT(test, ret, 0);
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+}
+
 /*
  * Test that if a driver and screen supports RGB and YUV formats, and we
  * try to set the VIC 1 mode, we end up with 8bpc RGB even if we could
@@ -1884,6 +1929,7 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
 	KUNIT_CASE(drm_test_check_max_tmds_rate_bpc_fallback_yuv420),
 	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback_yuv422),
 	KUNIT_CASE(drm_test_check_max_tmds_rate_format_fallback_yuv420),
+	KUNIT_CASE(drm_test_check_driver_unsupported_fallback_yuv420),
 	KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_changed),
 	KUNIT_CASE(drm_test_check_output_bpc_crtc_mode_not_changed),
 	KUNIT_CASE(drm_test_check_output_bpc_dvi),

-- 
2.49.0

Re: [PATCH v3 15/15] drm/tests: hdmi: Add test for unsuccessful forced fallback to YUV420

[Maxime Ripard]

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

Hi,

On Wed, Mar 26, 2025 at 12:20:04PM +0200, Cristian Ciocaltea wrote:
> Provide test to verify a forced fallback to YUV420 output cannot succeed
> when driver doesn't advertise YUV420 support.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index 99bedb2d6f555b3b140256000dfa7491d2a8f515..c2976b42aa2aacd2a68a871bffe97e795ca713d4 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -1493,6 +1493,51 @@ static void drm_test_check_max_tmds_rate_format_fallback_yuv420(struct kunit *te
>  	drm_modeset_acquire_fini(&ctx);
>  }
>  
> +/*
> + * Test that if a driver supports only RGB, but the chosen mode can be
> + * supported by the screen only in YUV420 output format, we end up with
> + * an unsuccessful forced fallback attempt.

What do you mean by "forced"?

> + */
> +static void drm_test_check_driver_unsupported_fallback_yuv420(struct kunit *test)
> +{
> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_display_info *info;
> +	struct drm_display_mode *yuv420_only_mode;
> +	struct drm_connector *conn;
> +	struct drm_device *drm;
> +	struct drm_crtc *crtc;
> +	int ret;
> +
> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
> +				BIT(HDMI_COLORSPACE_RGB),
> +				12,
> +				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +	drm = &priv->drm;
> +	crtc = priv->crtc;
> +	conn = &priv->connector;
> +	info = &conn->display_info;
> +	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> +	KUNIT_ASSERT_FALSE(test, conn->ycbcr_420_allowed);
> +
> +	yuv420_only_mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
> +	KUNIT_ASSERT_NOT_NULL(test, yuv420_only_mode);
> +	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_only(info, yuv420_only_mode));
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
> +						     crtc, conn,
> +						     yuv420_only_mode,
> +						     &ctx);
> +	KUNIT_EXPECT_LT(test, ret, 0);

I think that's where your current approach falls a bit short. You should
really craft the state yourself and check the returned value of
drm_atomic_check_only(), not rely on
drm_kunit_helper_enable_crtc_connector() doing the right thing, when it
doesn't really tell you :)

> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +}
> +

We still need to do the same with a driver that supports both, but the
monitor doesn't.

Maxime

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

Re: [PATCH v3 15/15] drm/tests: hdmi: Add test for unsuccessful forced fallback to YUV420

[Cristian Ciocaltea]

On 4/10/25 12:21 PM, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Mar 26, 2025 at 12:20:04PM +0200, Cristian Ciocaltea wrote:
>> Provide test to verify a forced fallback to YUV420 output cannot succeed
>> when driver doesn't advertise YUV420 support.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 46 ++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> index 99bedb2d6f555b3b140256000dfa7491d2a8f515..c2976b42aa2aacd2a68a871bffe97e795ca713d4 100644
>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> @@ -1493,6 +1493,51 @@ static void drm_test_check_max_tmds_rate_format_fallback_yuv420(struct kunit *te
>>  	drm_modeset_acquire_fini(&ctx);
>>  }
>>  
>> +/*
>> + * Test that if a driver supports only RGB, but the chosen mode can be
>> + * supported by the screen only in YUV420 output format, we end up with
>> + * an unsuccessful forced fallback attempt.
> 
> What do you mean by "forced"?

I wanted to describe the context where there's no alternative other than
doing the fallback.  Should we replace with "mandatory" maybe?

>> + */
>> +static void drm_test_check_driver_unsupported_fallback_yuv420(struct kunit *test)
>> +{
>> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
>> +	struct drm_modeset_acquire_ctx ctx;
>> +	struct drm_display_info *info;
>> +	struct drm_display_mode *yuv420_only_mode;
>> +	struct drm_connector *conn;
>> +	struct drm_device *drm;
>> +	struct drm_crtc *crtc;
>> +	int ret;
>> +
>> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
>> +				BIT(HDMI_COLORSPACE_RGB),
>> +				12,
>> +				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
>> +	KUNIT_ASSERT_NOT_NULL(test, priv);
>> +
>> +	drm = &priv->drm;
>> +	crtc = priv->crtc;
>> +	conn = &priv->connector;
>> +	info = &conn->display_info;
>> +	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
>> +	KUNIT_ASSERT_FALSE(test, conn->ycbcr_420_allowed);
>> +
>> +	yuv420_only_mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
>> +	KUNIT_ASSERT_NOT_NULL(test, yuv420_only_mode);
>> +	KUNIT_ASSERT_TRUE(test, drm_mode_is_420_only(info, yuv420_only_mode));
>> +
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
>> +						     crtc, conn,
>> +						     yuv420_only_mode,
>> +						     &ctx);
>> +	KUNIT_EXPECT_LT(test, ret, 0);
> 
> I think that's where your current approach falls a bit short. You should
> really craft the state yourself and check the returned value of
> drm_atomic_check_only(), not rely on
> drm_kunit_helper_enable_crtc_connector() doing the right thing, when it
> doesn't really tell you :)

Ack.

> 
>> +	drm_modeset_drop_locks(&ctx);
>> +	drm_modeset_acquire_fini(&ctx);
>> +}
>> +
> 
> We still need to do the same with a driver that supports both, but the
> monitor doesn't.

Ack.

Thanks,
Cristian