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

Maxime Ripard
2026-03-05T09:33:04Z

[-- Attachment #1: Type: text/plain, Size: 4455 bytes --]

On Mon, Mar 02, 2026 at 01:53:34PM +0100, Nicolas Frattaroli wrote:
> 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.

We just tested above that it wasn't, so if we took that branch but it's
still auto, something is very wrong :)

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

The part above wasn't meant to be the whole function but only the part
covered by your patch. My point is you should have a test of whether we
have auto or not. If we don't we use whatever we have and early return.
If we have auto, we do RGB then YUV420 like we used to.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]