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);
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 */
+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 */
+static void drm_test_check_hdmi_color_format_420_only(struct kunit *test)
+{
+ struct drm_atomic_helper_connector_hdmi_priv *priv;
+ struct drm_connector_state *conn_state;
+ struct drm_display_mode *dank;
+ 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_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
+ KUNIT_ASSERT_NOT_NULL(test, priv);
+
+ conn_state = priv->connector.state;
+
+ dank = find_420_only_mode(&priv->connector);
+ KUNIT_ASSERT_NOT_NULL(test, dank);
+
+ conn_state->color_format = DRM_COLOR_FORMAT_AUTO;
+ ret = hdmi_compute_config(&priv->connector, conn_state, dank);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+ KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_YUV420);
+
+ conn_state->color_format = DRM_COLOR_FORMAT_RGB444;
+ ret = hdmi_compute_config(&priv->connector, conn_state, dank);
+ KUNIT_EXPECT_LT(test, ret, 0);
+
+ conn_state->color_format = DRM_COLOR_FORMAT_YCBCR420;
+ ret = hdmi_compute_config(&priv->connector, conn_state, dank);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+ KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_YUV420);
+};
+
static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode),
KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode_vic_1),
@@ -2028,6 +2134,9 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_8bpc),
KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_10bpc),
KUNIT_CASE(drm_test_check_tmds_char_rate_rgb_12bpc),
+ KUNIT_CASE_PARAM(drm_test_check_hdmi_color_format,
+ check_hdmi_color_format_gen_params),
+ KUNIT_CASE(drm_test_check_hdmi_color_format_420_only),
/*
* TODO: We should have tests to check that a change in the
* format triggers a CRTC mode change just like we do for the
diff --git a/include/drm/display/drm_hdmi_state_helper.h b/include/drm/display/drm_hdmi_state_helper.h
index 2349c0d0f00f..01ae31209820 100644
--- a/include/drm/display/drm_hdmi_state_helper.h
+++ b/include/drm/display/drm_hdmi_state_helper.h
@@ -30,4 +30,8 @@ enum drm_mode_status
drm_hdmi_connector_mode_valid(struct drm_connector *connector,
const struct drm_display_mode *mode);
+int hdmi_compute_config(const struct drm_connector *connector,
+ struct drm_connector_state *conn_state,
+ const struct drm_display_mode *mode);
+
#endif // DRM_HDMI_STATE_HELPER_H_
--
2.52.0On 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
On Friday, 12 December 2025 10:19:13 Central European Standard Time Maxime Ripard wrote: > 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? The only function that calls hdmi_compute_config is the exported drm_atomic_helper_connector_hdmi_check. While I can write tests around drm_atomic_helper_connector_hdmi_check, it'll mean I'll have structure the tests differently, and will accidentally test a lot of other things as well, because it derives other state from the config and has a lot more error paths. So checking that hdmi_compute_config fails as expected won't be possible anymore, just that "_connector_hdmi_check fails. I will rewrite the tests to do this since that appears to be the way to do this, but I'll need to read up on the atomic state APIs and the helper functions I've been using so far a bit to make sure I'm not writing something broken here. I do share your concerns about exporting this function though, I didn't like doing it either. It is a side effect of unit testing not being a first-class citizen of the C language I guess, but maybe it is better to do this as an end-to-end test of the exported function rather than just part of the implementation anyway. > [...] For everything I didn't directly reply to, assume I'll address it in the next revision with no further sobbing to convey from my side. Kind regards, Nicolas Frattaroli
© 2016 - 2026 Red Hat, Inc.