Hi,
On Fri, Feb 06, 2026 at 04:26:56PM +0100, Nicolas Frattaroli wrote:
> On Friday, 6 February 2026 15:05:08 Central European Standard Time Maxime
> Ripard wrote:
> > On Wed, Jan 21, 2026 at 03:45:09PM +0100, Nicolas Frattaroli wrote:
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 7eaec37ae1c7..b5604dca728a 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -556,6 +556,16 @@ enum drm_colorspace {
> > > DRM_MODE_COLORIMETRY_COUNT
> > > };
> > >
> > > +enum drm_color_format {
> > > + DRM_COLOR_FORMAT_AUTO = 0,
> > > + DRM_COLOR_FORMAT_RGB444 = BIT(0),
> > > + DRM_COLOR_FORMAT_YCBCR444 = BIT(1),
> > > + DRM_COLOR_FORMAT_YCBCR422 = BIT(2),
> > > + DRM_COLOR_FORMAT_YCBCR420 = BIT(3),
> > > +};
> > > +
> > > +#define DRM_COLOR_FORMAT_COUNT 5
> > > +
> >
> > I don't really see a reason to expose an enum, with a bunch of values
> > that are all mutually exclusive, as a bitmask. It's pretty inconsistent
> > with most (all?) the other similar properties we have.
> >
> > I appreciate you did that to avoid fixing up every driver using those
> > values, but then maybe we don't have to? We could create a userspace
> > facing enum, and convert to DRM_COLOR_FORMAT internally.
>
> This is what the series did at v5 and earlier. IMHO it was kind of
> counter-productive, because we then had two different things for the
> same purpose, and some conversion logic between them. I think it's more
> error prone to do it that way (think: mixing up the two), and doesn't
> have a clear benefit. Just to give a picture of how bad things get:
>
> 1. we have the HDMI color format (aka "HDMI_COLORSPACE")
> 2. we have driver specific output color formats, e.g. the intel ones
> 3. we have DRM_COLOR_FORMAT
> 4. we have the bus formats (multiple per color format)
> 5. we have the DRM plane formats (again, multiple per color format)
>
> Adding a sixth into the mix feels a bit bad because we'll then need to
> justify why we should have another layer of switch-case statements.Yeah, but they are all semantically different: * The userspace one you want to introduce is going to be a superset of all the valid output format for all the output busses we support (so, HDMI + DP + etc.) * plane formats are the input format, we have much more variation there, and we will never output these. We can ignore these. * bus formats are somewhat similar, they are more about the wiring between bridges than anything else, and they are not exposed to userspace. We can ignore these too. * DRM_COLOR_FORMAT are definitely redundant. * The intel color formats are also redundant, but also internal. I would expect them to converge to whatever we come up here eventually (but really don't expect you to do that work). * And HDMI_COLORSPACE is really mandated by the HDMI spec, and is only about HDMI connectors. It will never fully overlap with what we come up with here, if only because HDMI cares about things we don't. So we really have two formats in my opinion: the one exposed through the uapi, and the internal one exposed to driver. In my view, the internal -> uapi conversion is trivial because the uapi one is a superset of the internal one (if only for auto). The uapi -> internal one needs to deal and resolve what auto means, but your code already does that. I don't really care about the internal format, as long as drivers don't have to be smart about it, so auto shouldn't be exposed to drivers. As far as I'm concerned, DRM_COLOR_FORMAT would fit that bill if it wasn't for the fact that it's both a bitmask and an enum depending on the context, which makes it pretty weird and error prone to deal with. Maxime
signature.asc
Description: PGP signature
