Re: [PATCH v8 02/20] drm: Add new general DRM property "color format"

[Maxime Ripard <mripard@kernel.org>]

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

Hi Jani,

On Mon, Feb 23, 2026 at 06:17:23PM +0200, Jani Nikula wrote:
> On Mon, 16 Feb 2026, Nicolas Frattaroli <nicolas.frattaroli@collabora.com> 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

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