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

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

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.

>  
>  	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