Re: [PATCH 14/14] drm/display: hdmi: Use drm_output_color_format instead of hdmi_colorspace

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

On Wednesday, 25 February 2026 18:03:00 Central European Standard Time Nicolas Frattaroli wrote:
> On Tuesday, 24 February 2026 11:58:53 Central European Standard Time Maxime Ripard wrote:
> > The hdmi_colorspace enum was defined to represent the colorspace value
> > of the HDMI infoframes. It was later used by some HDMI drivers to
> > express the output format they should be setting up.
> > 
> > During the introduction of the HDMI helpers, it then was used to
> > represent it in the drm_connector_hdmi_state structure.
> > 
> > However, it's always been somewhat redundant with the DRM_COLOR_FORMAT_*
> > defines, and now with the drm_output_color_format enum. Let's
> > consolidate around drm_output_color_format in drm_connector_hdmi_state
> > to facilitate the current effort to provide a global output format
> > selection mechanism.
> > 
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/gpu/drm/bridge/inno-hdmi.c                 |   6 +-
> >  drivers/gpu/drm/bridge/ite-it6263.c                |   2 +-
> >  drivers/gpu/drm/display/drm_hdmi_helper.c          |   7 +-
> >  drivers/gpu/drm/display/drm_hdmi_state_helper.c    |  52 ++++--
> >  drivers/gpu/drm/drm_bridge.c                       |   2 +-
> >  drivers/gpu/drm/drm_connector.c                    |  14 +-
> >  drivers/gpu/drm/mediatek/mtk_hdmi_v2.c             |   8 +-
> >  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c             |   2 +-
> >  drivers/gpu/drm/tests/drm_connector_test.c         |  80 ++++-----
> >  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 182 ++++++++++-----------
> >  drivers/gpu/drm/vc4/vc4_hdmi.c                     |  18 +-
> >  drivers/gpu/drm/vc4/vc4_hdmi.h                     |   2 +-
> >  include/drm/display/drm_hdmi_helper.h              |   3 +-
> >  include/drm/drm_connector.h                        |   7 +-
> >  14 files changed, 205 insertions(+), 180 deletions(-)
> > 
> > [... snip ...]
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index 4f5b27fab475c7c733622eb8727927571f3fb8fe..171cd495976a3e16f201fd339d3d42a09dc3b63f 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > [... snip ...]
> >  
> > @@ -1424,25 +1424,25 @@ drm_hdmi_connector_get_broadcast_rgb_name(enum drm_hdmi_broadcast_rgb broadcast_
> >  	return broadcast_rgb_names[broadcast_rgb].name;
> >  }
> >  EXPORT_SYMBOL(drm_hdmi_connector_get_broadcast_rgb_name);
> >  
> >  static const char * const output_format_str[] = {
> > -	[HDMI_COLORSPACE_RGB]		= "RGB",
> > -	[HDMI_COLORSPACE_YUV420]	= "YUV 4:2:0",
> > -	[HDMI_COLORSPACE_YUV422]	= "YUV 4:2:2",
> > -	[HDMI_COLORSPACE_YUV444]	= "YUV 4:4:4",
> > +	[DRM_OUTPUT_COLOR_FORMAT_RGB444]	= "RGB",
> > +	[DRM_OUTPUT_COLOR_FORMAT_YCBCR420]	= "YUV 4:2:0",
> > +	[DRM_OUTPUT_COLOR_FORMAT_YCBCR422]	= "YUV 4:2:2",
> > +	[DRM_OUTPUT_COLOR_FORMAT_YCBCR444]	= "YUV 4:4:4",
> >  };
> >  
> >  /*
> >   * drm_hdmi_connector_get_output_format_name() - Return a string for HDMI connector output format
> >   * @fmt: Output format to compute name of
> >   *
> >   * Returns: the name of the output format, or NULL if the type is not
> >   * valid.
> >   */
> >  const char *
> > -drm_hdmi_connector_get_output_format_name(enum hdmi_colorspace fmt)
> > +drm_hdmi_connector_get_output_format_name(enum drm_output_color_format fmt)
> >  {
> >  	if (fmt >= ARRAY_SIZE(output_format_str))
> >  		return NULL;
> 
> Almost unrelated nit: we might want to also `fmt < 0 ||` here, since the
> base type of enums is a signed int. I almost caught myself using this function
> as a way to quickly check if a supplied color format property from userspace
> was valid, which would've had some unpleasant consequences for the kernel's
> memory.
> 
> Alternatively, I make my own switch-case based "is this a valid enum value"
> function. I probably should do that actually, but my laziness lead me to
> make sure we only have a single source of truth of what counts as a valid
> enum value.

Gah, disregard this, I just realised that this enum shouldn't be used as
the one in the property anyway, because of AUTO, as you already pointed out
in your reply to my series as well.

> 
> >  
> >  	return output_format_str[fmt];
> > [... snip ...]
> 
> Thanks for your work on this series, I was afraid of getting rid of
> DRM_COLOR_FORMAT_* myself because I suspected it was going to be a
> fairly large refactor across every driver. Guess I was both right
> and wrong: it touches many files, but all the replacements are
> fairly simple. :)
> 
> Kind regards,
> Nicolas Frattaroli