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

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

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
>