On Tuesday, 10 February 2026 18:24:46 Central European Standard Time Maxime Ripard wrote: > Hi, > > On Sat, Feb 07, 2026 at 08:55:16PM +0100, Nicolas Frattaroli wrote: > > On Friday, 6 February 2026 15:08:46 Central European Standard Time Maxime > > Ripard wrote: > > > On Wed, Jan 21, 2026 at 03:45:10PM +0100, Nicolas Frattaroli wrote: > > > > While the two enums have similar values, they're not identical, and > > > > HDMI's enum is defined as per the HDMI standard. > > > > > > > > Add a simple conversion function from DRM to HDMI. Unexpected inputs > > > > aren't handled in any clever way, DRM_COLOR_FORMAT_AUTO and any other > > > > value that doesn't cleanly map to HDMI just gets returned as > > > > HDMI_COLORSPACE_RGB. > > > > > > > > Add a second conversion function that gets a DRM_COLOR_FORMAT from an > > > > HDMI_COLORSPACE as well. In this case, reserved HDMI values that can't > > > > be converted will result in an -EINVAL return value. > > > > > > > > Co-developed-by: Marius Vlad <[email protected]> > > > > Signed-off-by: Marius Vlad <[email protected]> > > > > Signed-off-by: Nicolas Frattaroli <[email protected]> > > > > --- > > > > include/drm/drm_connector.h | 54 > > > > +++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 54 insertions(+) > > > > > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > > > index b5604dca728a..ffeb42f3b4a3 100644 > > > > --- a/include/drm/drm_connector.h > > > > +++ b/include/drm/drm_connector.h > > > > @@ -2612,6 +2612,60 @@ int > > > > drm_connector_attach_color_format_property(struct drm_connector > > > > *connector); > > > > > > > > const char *drm_get_color_format_name(enum drm_color_format color_fmt); > > > > > > > > +/** > > > > + * drm_color_format_to_hdmi_colorspace - convert DRM color format to > > > > HDMI > > > > + * @fmt: the &enum drm_color_format to convert > > > > + * > > > > + * Convert a given &enum drm_color_format to an equivalent > > > > + * &enum hdmi_colorspace. For non-representable values and > > > > + * %DRM_COLOR_FORMAT_AUTO, the value %HDMI_COLORSPACE_RGB is returned. > > > > + * > > > > + * Returns: the corresponding &enum hdmi_colorspace value > > > > + */ > > > > +static inline enum hdmi_colorspace __pure > > > > +drm_color_format_to_hdmi_colorspace(enum drm_color_format fmt) > > > > +{ > > > > + switch (fmt) { > > > > + default: > > > > + case DRM_COLOR_FORMAT_AUTO: > > > > + case DRM_COLOR_FORMAT_RGB444: > > > > + return HDMI_COLORSPACE_RGB; > > > > > > I don't think that's correct. What auto ends up as totally depends on > > > the atomic state it comes with. > > > > > > At the very least, you should output a warning there, because that case > > > should never happen. > > > > Yeah, my hope was to keep this function __pure so that the compiler > > has maximum freedom to do whatever. With a WARN, it's got side-effects > > now, and we're no longer pure. With a status return value and an output > > parameter, it's no longer pure either, because the output parameter is > > not local memory. > > > > The limiting factor here is that as I understand correctly, I can't > > really extend the hdmi_colorspace enum, as it's basically 1:1 from > > the standard. Doing this would be the ideal solution, because we'd > > keep the function pure and without surprise conversions happening. > > I feel like this kind of loops back into the other two reviews I did: > you paint yourself into a corner by having auto in the enum, and by > passing it directly to that function. > > If, instead, you don't allow auto in the drm_color_format enum, and > resolve auto in the hdmi_compute_config function instead of passing it > directly, then we don't have to deal with it here.
You're right. Though that means I need to figure out how I'll handle the conversion from enum to bitfield for !HDMI cases. I guess for DP, I can do this in drm_dp_helper or something. I don't know if i915 or amdgpu call into that at all though, but it's as good of a time as any to start doing so now if not. Just also learned about the existence of `enum dp_pixelformat`, which imho has the best naming out of all of these enums. (I hope "HDMI Colorspace" to describe pixel formats is straight from the standard because if it's something we in the kernel came up with then I will put renaming it on my list of long-term kernel tasks to get around to some day.) And yes I will add the necessary KUnit tests for the DP helper side of things as well. :) Nothing registers the property for DSI so we should be good on that front for now. Thanks for the reviews. Kind regards, Nicolas Frattaroli > > Looking at hdmi_colorspace_get_name in drivers/video/hdmi.c, it returns > > "Invalid" for any value not in the enum itself. Would it be allowable > > to tack an HDMI_COLORSPACE_INVALID at the end of the enum with perhaps > > a negative value, or is there a different approach you'd prefer? > > And again, if we only ever have to deal with RGB, YUV420, 444 or 422, > then we always have valid values for HDMI_COLORSPACE. > > Plus, the hdmi_colorspace enum matches what the hdmi spec defines, so we > can't really extend it, and most importantly, hdmi_colorspace_get_name() > is only ever used for debugging / logging purposes, it's never in the > "functional" path. > > Maxime >
