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 <[email protected]> > > --- > > 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 >
