Re: [PATCH v9 04/19] drm/display: hdmi-state-helper: Act on color format DRM property

Nicolas Frattaroli
2026-03-02T12:54:00Z

On Monday, 2 March 2026 09:46:06 Central European Standard Time Maxime Ripard wrote:
> Hi,
>
> On Fri, Feb 27, 2026 at 08:20:09PM +0100, Nicolas Frattaroli wrote:
> > With the introduction of the "color format" DRM property, which allows
> > userspace to request a specific color format, the HDMI state helper
> > should implement this.
> >
> > Implement it by translating the requested drm_connector_color_format to
> > a drm_output_color_format enum value as per the logic HDMI should use
> > for this: Auto is translated to RGB, and a fallback to YUV420 is only
> > performed if the original color format was auto.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> > drivers/gpu/drm/display/drm_hdmi_state_helper.c | 28 +++++++++++++++++++++++--
> > 1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > index 9f3b696aceeb..31c6d55fa995 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > @@ -669,10 +669,34 @@ hdmi_compute_config(const struct drm_connector *connector,
> > unsigned int max_bpc = clamp_t(unsigned int,
> > conn_state->max_bpc,
> > 8, connector->max_bpc);
> > + enum drm_output_color_format fmt;
> > int ret;
> >
> > - ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> > - DRM_OUTPUT_COLOR_FORMAT_RGB444);
> > + switch (conn_state->color_format) {
> > + case DRM_CONNECTOR_COLOR_FORMAT_AUTO:
> > + case DRM_CONNECTOR_COLOR_FORMAT_RGB444:
> > + fmt = DRM_OUTPUT_COLOR_FORMAT_RGB444;
> > + break;
> > + case DRM_CONNECTOR_COLOR_FORMAT_YCBCR444:
> > + fmt = DRM_OUTPUT_COLOR_FORMAT_YCBCR444;
> > + break;
> > + case DRM_CONNECTOR_COLOR_FORMAT_YCBCR422:
> > + fmt = DRM_OUTPUT_COLOR_FORMAT_YCBCR422;
> > + break;
> > + case DRM_CONNECTOR_COLOR_FORMAT_YCBCR420:
> > + fmt = DRM_OUTPUT_COLOR_FORMAT_YCBCR420;
> > + break;
> > + default:
> > + drm_dbg_kms(connector->dev, "HDMI does not support color format '%d'.\n",
> > + conn_state->color_format);
> > + return -EINVAL;
> > + }
> > +
> > + ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc, fmt);
> > +
> > + if (conn_state->color_format != DRM_CONNECTOR_COLOR_FORMAT_AUTO)
> > + return ret;
> > +
>
> We discussed it before, and it wasn't as trivial as it should have been,
> but now, I really feel something like the following would be simpler:
>
> if (conn_state->color_format != DRM_CONNECTOR_COLOR_FORMAT_AUTO) {
> enum drm_output_color_format fmt;
>
> switch (conn_state->color_format) {
> case DRM_CONNECTOR_COLOR_FORMAT_AUTO:
> drm_warn(connector->dev, "The format shouldn't be auto here"); // or any better message
> fallthrough;

Why shouldn't it be auto there? This is the function where the auto->rgb
mapping is explicitly handled.

> case DRM_CONNECTOR_COLOR_FORMAT_RGB444:
> fmt = DRM_OUTPUT_COLOR_FORMAT_RGB444;
> break;
> ....
> }
>
> return hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc, fmt);
> }
>
> ret = hdmi_compute_format_bpc(connector, conn_state, mode, max_bpc,
> DRM_OUTPUT_COLOR_FORMAT_RGB444);
>
> It makes it much clearer what the two branches are, and we don't have to
> test for auto multiple times.

Testing for auto multiple times is done for the "4:2:0 fallback on
AUTO only" case. If you fall through from AUTO to RGB and then return
the result of hdmi_compute_format_bpc on RGB, then you will not let
AUTO fall back to 4:2:0. hdmi_compute_format_bpc only does a fallback
for lower bit depths, not different color formats.

As far as I can tell, you're requesting a change of behaviour here that
would require me to adjust the behaviour of every single other HDMI
implementation and modify all the tests that you already gave a reviewed-by,
so I assume this wasn't the intent?

Kind regards,
Nicolas Frattaroli

>
> Maxime
>