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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

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