Hi Jani, On Mon, Feb 23, 2026 at 06:17:23PM +0200, Jani Nikula wrote: > On Mon, 16 Feb 2026, Nicolas Frattaroli <[email protected]> > wrote: > > +/** > > + * enum drm_color_format_enum - color model description > > + * > > + * This enum is a high-level description of the component makeup of the > > image > > + * data. It says nothing about how the components are ordered or how many > > bits > > + * they take up (i.e. is unlike MEDIA_BUS_FMT\_ or DRM_FORMAT\_), but > > + * describes the type of components (Luminance-Chrominance vs. RGB) and the > > + * sub-sampling. > > + * > > + * &enum drm_color_format_enum makes statements about the same attribute of > > + * an image as the DRM_COLOR_FORMAT\_ bitfields do. Its purpose is to > > inform > > + * choices made by display protocol specific implementations when it comes > > to > > + * translating it to e.g. &enum hdmi_colorspace or &enum dp_pixelformat, > > both > > + * of which also describe the same attribute of the image at the same > > level of > > + * specificity. > > + * > > + * In precise terms, this enum describes a color model. It makes no > > statements > > + * about the primaries, gamma, or current phase of the moon used in > > conversion > > + * from one to the other. Furthermore, it also makes no statements about > > the > > + * order of components (e.g. RGB vs. BGR), their depth in bits, or their > > binary > > + * packing. > > + */ > > +enum drm_color_format_enum { > > The enum name should not have "enum" in it. That's just not a style > that's being used. > > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_AUTO: The choice of format is left up to the > > + * display protocol implementation. All implementations of the same > > + * display protocol (e.g. HDMI) are supposed to behave the same way, > > + * though display protocols may choose to behave differently compared to > > + * each other (e.g. HDMI's "AUTO" does not have to match DP's "AUTO"). > > + * > > + * Implementations may rely on @DRM_COLOR_FORMAT_ENUM_AUTO to be falsy. > > + */ > > + DRM_COLOR_FORMAT_ENUM_AUTO = 0, > > Ditto for the enumeration names, no ENUM in them please. > > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_RGB444: Image components are encoded as RGB > > + * values of equal resolution. > > + */ > > + DRM_COLOR_FORMAT_ENUM_RGB444, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_YCBCR444: Image components are encoded as > > + * luminance and chrominance of equal resolution. > > + */ > > + DRM_COLOR_FORMAT_ENUM_YCBCR444, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_YCBCR422: Image components are encoded as > > + * luminance and chrominance with the chrominance components having half > > + * the horizontal resolution. > > + */ > > + DRM_COLOR_FORMAT_ENUM_YCBCR422, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_YCBCR420: Image components are encoded as > > + * luminance and chrominance with the chrominance components having half > > + * the horizontal and vertical resolution. > > + */ > > + DRM_COLOR_FORMAT_ENUM_YCBCR420, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_NUM: The number of valid color format values > > + * in this enum. Itself not a valid color format. > > + */ > > + DRM_COLOR_FORMAT_ENUM_NUM, > > + > > + /** > > + * @DRM_COLOR_FORMAT_ENUM_INVALID: Error return value for conversion > > + * functions encountering unexpected inputs. > > + */ > > + DRM_COLOR_FORMAT_ENUM_INVALID = -EINVAL, > > Please don't hide negative error codes inside enums. If you need to > return one from a function, please return the negative error code > directly instead. > > > +}; > > + > > +/* > > + * Constants for specifying bit masks for e.g. providing a list of > > supported > > + * color formats as a single integer. > > + */ > > +#define DRM_COLOR_FORMAT_RGB444 BIT(0) > > +#define DRM_COLOR_FORMAT_YCBCR444 BIT(1) > > +#define DRM_COLOR_FORMAT_YCBCR422 BIT(2) > > +#define DRM_COLOR_FORMAT_YCBCR420 BIT(3) > > I don't think we should define both enum and mask. One or the > other. Moreover, now you have two independent definitions for the same > thing, with nothing to ensure they keep matching. It's a bug waiting to > happen. > > I think the problem is that they were originally defined as bits even > though most places actually use them as single values only. It's > confusing. It would probably have been better to just use enums and > BIT(DRM_COLOR_FORMAT_*) where a mask is needed. > > Maybe that's what should be done as the first step anyway.
I largely agree with the sentiment, and can extend it to the HDMI_COLORSPACE used in drm_connector_hdmi_state. I've been working since yesterday on fixing that up to make Nicolas' life easier. I'll post it sometime today. Maxime
signature.asc
Description: PGP signature
