Re: [PATCH v11 03/22] drm: Add new general DRM property "color format"

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Mar 26, 2026 at 11:16:12AM +0000, Dave Stevenson wrote:
> On Wed, 25 Mar 2026 at 13:43, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Wed, Mar 25, 2026 at 12:49:19PM +0000, Dave Stevenson wrote:
> > > On Tue, 24 Mar 2026 at 16:02, Nicolas Frattaroli
> > > <nicolas.frattaroli@collabora.com> wrote:
> > > >
> > > > Add a new general DRM property named "color format" which can be used by
> > > > userspace to request the display driver to output a particular color
> > > > format.
> > > >
> > > > Possible options are:
> > > >     - auto (setup by default, driver internally picks the color format)
> > > >     - rgb
> > > >     - ycbcr444
> > > >     - ycbcr422
> > > >     - ycbcr420
> > > >
> > > > Drivers should advertise from this list which formats they support.
> > > > Together with this list and EDID data from the sink we should be able
> > > > to relay a list of usable color formats to users to pick from.
> > > >
> > > > Co-developed-by: Werner Sembach <wse@tuxedocomputers.com>
> > > > Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
> > > > Co-developed-by: Andri Yngvason <andri@yngvason.is>
> > > > Signed-off-by: Andri Yngvason <andri@yngvason.is>
> > > > Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> > > > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_atomic_helper.c |   5 ++
> > > >  drivers/gpu/drm/drm_atomic_uapi.c   |  11 ++++
> > > >  drivers/gpu/drm/drm_connector.c     | 108 ++++++++++++++++++++++++++++++++++++
> > > >  include/drm/drm_connector.h         | 104 ++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 228 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 26953ed6b53e..b7753454b777 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -737,6 +737,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > > >                         if (old_connector_state->max_requested_bpc !=
> > > >                             new_connector_state->max_requested_bpc)
> > > >                                 new_crtc_state->connectors_changed = true;
> > > > +
> > > > +                       if (old_connector_state->color_format !=
> > > > +                           new_connector_state->color_format)
> > > > +                               new_crtc_state->connectors_changed = true;
> > > > +
> > > >                 }
> > > >
> > > >                 if (funcs->atomic_check)
> > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > index 5bd5bf6661df..dee510c85e59 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > > @@ -935,6 +935,15 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> > > >                 state->privacy_screen_sw_state = val;
> > > >         } else if (property == connector->broadcast_rgb_property) {
> > > >                 state->hdmi.broadcast_rgb = val;
> > > > +       } else if (property == connector->color_format_property) {
> > > > +               if (val > INT_MAX || !drm_connector_color_format_valid(val)) {
> > > > +                       drm_dbg_atomic(connector->dev,
> > > > +                                      "[CONNECTOR:%d:%s] unknown color format %llu\n",
> > > > +                                      connector->base.id, connector->name, val);
> > > > +                       return -EINVAL;
> > > > +               }
> > > > +
> > > > +               state->color_format = val;
> > > >         } else if (connector->funcs->atomic_set_property) {
> > > >                 return connector->funcs->atomic_set_property(connector,
> > > >                                 state, property, val);
> > > > @@ -1020,6 +1029,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> > > >                 *val = state->privacy_screen_sw_state;
> > > >         } else if (property == connector->broadcast_rgb_property) {
> > > >                 *val = state->hdmi.broadcast_rgb;
> > > > +       } else if (property == connector->color_format_property) {
> > > > +               *val = state->color_format;
> > > >         } else if (connector->funcs->atomic_get_property) {
> > > >                 return connector->funcs->atomic_get_property(connector,
> > > >                                 state, property, val);
> > > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > > index 47dc53c4a738..e848374dee0b 100644
> > > > --- a/drivers/gpu/drm/drm_connector.c
> > > > +++ b/drivers/gpu/drm/drm_connector.c
> > > > @@ -1388,6 +1388,18 @@ static const u32 hdmi_colorspaces =
> > > >         BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) |
> > > >         BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER);
> > > >
> > > > +static const u32 hdmi_colorformats =
> > > > +       BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444) |
> > > > +       BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444) |
> > > > +       BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR422) |
> > > > +       BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420);
> > > > +
> > > > +static const u32 dp_colorformats =
> > > > +       BIT(DRM_OUTPUT_COLOR_FORMAT_RGB444) |
> > > > +       BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444) |
> > > > +       BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR422) |
> > > > +       BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR420);
> > > > +
> > > >  /*
> > > >   * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel Encoding/Colorimetry
> > > >   * Format Table 2-120
> > > > @@ -2940,6 +2952,102 @@ int drm_connector_attach_colorspace_property(struct drm_connector *connector)
> > > >  }
> > > >  EXPORT_SYMBOL(drm_connector_attach_colorspace_property);
> > > >
> > > > +/**
> > > > + * drm_connector_attach_color_format_property - create and attach color format property
> > > > + * @connector: connector to create the color format property on
> > > > + * @supported_color_formats: bitmask of bit-shifted &enum drm_output_color_format
> > > > + *                           values the connector supports
> > > > + *
> > > > + * Called by a driver to create a color format property. The property is
> > > > + * attached to the connector automatically on success.
> > > > + *
> > > > + * @supported_color_formats should only include color formats the connector
> > > > + * type can actually support.
> > > > + *
> > > > + * Returns:
> > > > + * 0 on success, negative errno on error
> > > > + */
> > > > +int drm_connector_attach_color_format_property(struct drm_connector *connector,
> > > > +                                              unsigned long supported_color_formats)
> > > > +{
> > > > +       struct drm_device *dev = connector->dev;
> > > > +       struct drm_prop_enum_list enum_list[DRM_CONNECTOR_COLOR_FORMAT_COUNT];
> > > > +       unsigned int i = 0;
> > > > +       unsigned long fmt;
> > > > +
> > > > +       if (connector->color_format_property)
> > > > +               return 0;
> > > > +
> > > > +       if (!supported_color_formats) {
> > > > +               drm_err(dev, "No supported color formats provided on [CONNECTOR:%d:%s]\n",
> > > > +                       connector->base.id, connector->name);
> > > > +               return -EINVAL;
> > > > +       }
> > > > +
> > > > +       if (supported_color_formats & ~GENMASK(DRM_OUTPUT_COLOR_FORMAT_COUNT - 1, 0)) {
> > > > +               drm_err(dev, "Unknown color formats provided on [CONNECTOR:%d:%s]\n",
> > > > +                       connector->base.id, connector->name);
> > > > +               return -EINVAL;
> > > > +       }
> > > > +
> > > > +       switch (connector->connector_type) {
> > > > +       case DRM_MODE_CONNECTOR_HDMIA:
> > > > +       case DRM_MODE_CONNECTOR_HDMIB:
> > > > +               if (supported_color_formats & ~hdmi_colorformats) {
> > > > +                       drm_err(dev, "Color formats not allowed for HDMI on [CONNECTOR:%d:%s]\n",
> > > > +                               connector->base.id, connector->name);
> > > > +                       return -EINVAL;
> > > > +               }
> > > > +               break;
> > > > +       case DRM_MODE_CONNECTOR_DisplayPort:
> > > > +       case DRM_MODE_CONNECTOR_eDP:
> > > > +               if (supported_color_formats & ~dp_colorformats) {
> > > > +                       drm_err(dev, "Color formats not allowed for DP on [CONNECTOR:%d:%s]\n",
> > > > +                               connector->base.id, connector->name);
> > > > +                       return -EINVAL;
> > > > +               }
> > > > +               break;
> > > > +       }
> > > > +
> > > > +       enum_list[0].name = "AUTO";
> > > > +       enum_list[0].type = DRM_CONNECTOR_COLOR_FORMAT_AUTO;
> > > > +
> > > > +       for_each_set_bit(fmt, &supported_color_formats, DRM_OUTPUT_COLOR_FORMAT_COUNT) {
> > > > +               switch (fmt) {
> > > > +               case DRM_OUTPUT_COLOR_FORMAT_RGB444:
> > > > +                       enum_list[++i].type = DRM_CONNECTOR_COLOR_FORMAT_RGB444;
> > > > +                       break;
> > > > +               case DRM_OUTPUT_COLOR_FORMAT_YCBCR444:
> > > > +                       enum_list[++i].type = DRM_CONNECTOR_COLOR_FORMAT_YCBCR444;
> > > > +                       break;
> > > > +               case DRM_OUTPUT_COLOR_FORMAT_YCBCR422:
> > > > +                       enum_list[++i].type = DRM_CONNECTOR_COLOR_FORMAT_YCBCR422;
> > > > +                       break;
> > > > +               case DRM_OUTPUT_COLOR_FORMAT_YCBCR420:
> > > > +                       enum_list[++i].type = DRM_CONNECTOR_COLOR_FORMAT_YCBCR420;
> > > > +                       break;
> > > > +               default:
> > > > +                       drm_warn(dev, "Unknown supported format %ld on [CONNECTOR:%d:%s]\n",
> > > > +                                fmt, connector->base.id, connector->name);
> > > > +                       continue;
> > > > +               }
> > > > +               enum_list[i].name = drm_hdmi_connector_get_output_format_name(fmt);
> > > > +       }
> > > > +
> > > > +       connector->color_format_property =
> > > > +               drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, "color format",
> > > > +                                        enum_list, i + 1);
> > > > +
> > > > +       if (!connector->color_format_property)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       drm_object_attach_property(&connector->base, connector->color_format_property,
> > > > +                                  DRM_CONNECTOR_COLOR_FORMAT_AUTO);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(drm_connector_attach_color_format_property);
> > > > +
> > > >  /**
> > > >   * drm_connector_atomic_hdr_metadata_equal - checks if the hdr metadata changed
> > > >   * @old_state: old connector state to compare
> > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > > index af8b92d2d5b7..bd549f912b76 100644
> > > > --- a/include/drm/drm_connector.h
> > > > +++ b/include/drm/drm_connector.h
> > > > @@ -571,14 +571,102 @@ enum drm_colorspace {
> > > >   *   YCbCr 4:2:2 output format (ie. with horizontal subsampling)
> > > >   * @DRM_OUTPUT_COLOR_FORMAT_YCBCR420:
> > > >   *   YCbCr 4:2:0 output format (ie. with horizontal and vertical subsampling)
> > > > + * @DRM_OUTPUT_COLOR_FORMAT_COUNT:
> > > > + *   Number of valid output color format values in this enum
> > > >   */
> > > >  enum drm_output_color_format {
> > > >         DRM_OUTPUT_COLOR_FORMAT_RGB444 = 0,
> > > >         DRM_OUTPUT_COLOR_FORMAT_YCBCR444,
> > > >         DRM_OUTPUT_COLOR_FORMAT_YCBCR422,
> > > >         DRM_OUTPUT_COLOR_FORMAT_YCBCR420,
> > > > +       DRM_OUTPUT_COLOR_FORMAT_COUNT,
> > > >  };
> > > >
> > > > +/**
> > > > + * enum drm_connector_color_format - Connector Color Format Request
> > > > + *
> > > > + * This enum, unlike &enum drm_output_color_format, is used to specify requests
> > > > + * for a specific color format on a connector through the DRM "color format"
> > > > + * property. The difference is that it has an "AUTO" value to specify that
> > > > + * no specific choice has been made.
> > > > + */
> > > > +enum drm_connector_color_format {
> > > > +       /**
> > > > +        * @DRM_CONNECTOR_COLOR_FORMAT_AUTO: The driver or display protocol
> > > > +        * helpers should pick a suitable color format. All implementations of a
> > > > +        * specific display protocol must behave the same way with "AUTO", but
> > > > +        * different display protocols do not necessarily have the same "AUTO"
> > > > +        * semantics.
> > > > +        *
> > > > +        * For HDMI, "AUTO" picks RGB, but falls back to YCbCr 4:2:0 if the
> > > > +        * bandwidth required for full-scale RGB is not available, or the mode
> > > > +        * is YCbCr 4:2:0-only, as long as the mode and output both support
> > > > +        * YCbCr 4:2:0.
> > >
> > > Is there a reason you propose dropping back to YCbCr 4:2:0 without
> > > trying YCbCr 4:2:2 first? Minimising the subsampling is surely
> > > beneficial, and vc4 for one can do 4:2:2 but not 4:2:0.
> >
> > On HDMI 4:2:2 is always 12bpc, so it doesn't save any bandwidth
> > compared to 8bpc 4:4:4.
> 
> It does save bandwidth against 10 or 12bpc RGB 4:4:4.
> 
> Or is the implication that max_bpc = 12 and
> DRM_CONNECTOR_COLOR_FORMAT_AUTO should drop bpc down to 8 and select
> RGB in preference to selecting 4:2:2?

Yeah, YCbCr has all kinds of extra complications compared to RGB, so
the policy is to use RGB if possible, and only fall back to YCbCr as a
last resort. And in that case 4:2:0 is the only thing that can help.

-- 
Ville Syrjälä
Intel