Re: [PATCH v5 17/17] drm/tests: hdmi: Add tests for the color_format property

[Maxime Ripard <mripard@kernel.org>]

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

On Fri, Nov 28, 2025 at 10:05:53PM +0100, Nicolas Frattaroli wrote:
> Add some KUnit tests to check the color_format property is working as
> expected with the HDMI state helper.
> 
> The added tests check that AUTO results in RGB, and the YCBCR modes
> result in the corresponding YUV modes. An additional test ensures that
> only DRM_COLOR_FORMAT_AUTO falls back to YUV420 with a YUV420-only mode,
> and RGB errors out instead, while explicitly asking for YUV420 still
> works.
> 
> This requires exporting hdmi_compute_config, so that it is accessible
> from the tests.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c    |   3 +-
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 109 +++++++++++++++++++++
>  include/drm/display/drm_hdmi_state_helper.h        |   4 +
>  3 files changed, 115 insertions(+), 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 1800e00b30c5..e86fb837ceaf 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -641,7 +641,7 @@ hdmi_compute_format_bpc(const struct drm_connector *connector,
>  	return -EINVAL;
>  }
>  
> -static int
> +int
>  hdmi_compute_config(const struct drm_connector *connector,
>  		    struct drm_connector_state *conn_state,
>  		    const struct drm_display_mode *mode)
> @@ -680,6 +680,7 @@ hdmi_compute_config(const struct drm_connector *connector,
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL(hdmi_compute_config);

I don't think we need to export hdmi_compute_config directly, and if we
do, it shouldn't be named that way.

The rest of the tests in the suite manage to test everything fine
without exporting it. Is there any reason you can't do it for these
tests?

>  static int hdmi_generate_avi_infoframe(const struct drm_connector *connector,
>  				       struct drm_connector_state *conn_state)
> 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 8bd412735000..e7050cd9cb12 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -55,6 +55,23 @@ static struct drm_display_mode *find_preferred_mode(struct drm_connector *connec
>  	return preferred;
>  }
>  
> +static struct drm_display_mode *find_420_only_mode(struct drm_connector *connector)
> +{
> +	struct drm_device *drm = connector->dev;
> +	struct drm_display_mode *mode;
> +
> +	mutex_lock(&drm->mode_config.mutex);
> +	list_for_each_entry(mode, &connector->modes, head) {
> +		if (drm_mode_is_420_only(&connector->display_info, mode)) {
> +			mutex_unlock(&drm->mode_config.mutex);
> +			return mode;
> +		}
> +	}
> +	mutex_unlock(&drm->mode_config.mutex);
> +
> +	return NULL;
> +}
> +
>  static int set_connector_edid(struct kunit *test, struct drm_connector *connector,
>  			      const void *edid, size_t edid_len)
>  {
> @@ -1999,6 +2016,95 @@ static void drm_test_check_disable_connector(struct kunit *test)
>  	drm_modeset_acquire_fini(&ctx);
>  }
>  
> +struct color_format_test_param {
> +	enum drm_color_format fmt;
> +	enum hdmi_colorspace expected;
> +	const char *desc;
> +};
> +
> +/* Test that AUTO results in RGB, and explicit choices result in those */

We need a bit more details. Which EDID are you using, with which monitor
capabilities?

IIRC, we already have tests to make sure that the fallback happens and
only when it's supposed to. We just need to make sure we have asserts on
the property being auto for those.

This means we only need to test the !auto values, but we need to test it
both ways: that when the property is set and the display supports it,
the format chosen is indeed the one we asked for, but also that when the
property is set but the display doesn't support it, we get an error.

> +static void drm_test_check_hdmi_color_format(struct kunit *test)
> +{
> +	const struct color_format_test_param *param = test->param_value;
> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
> +	struct drm_connector_state *conn_state;
> +	struct drm_display_info *info;
> +	struct drm_display_mode *preferred;
> +	int ret;
> +
> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid_funcs(test,
> +				BIT(HDMI_COLORSPACE_RGB) |
> +				BIT(HDMI_COLORSPACE_YUV422) |
> +				BIT(HDMI_COLORSPACE_YUV420) |
> +				BIT(HDMI_COLORSPACE_YUV444),
> +				12,
> +				&dummy_connector_hdmi_funcs,
> +				test_edid_hdmi_4k_rgb_yuv420_dc_max_340mhz);
> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +	conn_state = priv->connector.state;
> +	info = &priv->connector.display_info;
> +	conn_state->color_format = param->fmt;
> +	KUNIT_ASSERT_TRUE(test, priv->connector.ycbcr_420_allowed);
> +
> +	preferred = find_preferred_mode(&priv->connector);
> +	KUNIT_ASSERT_TRUE(test, drm_mode_is_420(info, preferred));
> +
> +	ret = hdmi_compute_config(&priv->connector, conn_state, preferred);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, param->expected);
> +}
> +
> +static const struct color_format_test_param hdmi_color_format_params[] = {
> +	{ DRM_COLOR_FORMAT_AUTO, HDMI_COLORSPACE_RGB, "AUTO -> RGB" },
> +	{ DRM_COLOR_FORMAT_YCBCR422, HDMI_COLORSPACE_YUV422, "YCBCR422 -> YUV422" },
> +	{ DRM_COLOR_FORMAT_YCBCR420, HDMI_COLORSPACE_YUV420, "YCBCR420 -> YUV420" },
> +	{ DRM_COLOR_FORMAT_YCBCR444, HDMI_COLORSPACE_YUV444, "YCBCR444 -> YUV444" },
> +	{ DRM_COLOR_FORMAT_RGB444, HDMI_COLORSPACE_RGB, "RGB -> RGB" },
> +};
> +
> +KUNIT_ARRAY_PARAM_DESC(check_hdmi_color_format,
> +		  hdmi_color_format_params, desc);
> +
> +
> +/* Test that AUTO falls back to YUV420, and that RGB does not, but YUV420 works */

Same thing, the description needs to be improved here.

Maxime

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