On Wed, 20 Sep 2023 16:35:18 +0200 Maxime Ripard <[email protected]> wrote:
> The i915 driver has a property to force the RGB range of an HDMI output. > The vc4 driver then implemented the same property with the same > semantics. KWin has support for it, and a PR for mutter is also there to > support it. > > Both drivers implementing the same property with the same semantics, > plus the userspace having support for it, is proof enough that it's > pretty much a de-facto standard now and we can provide helpers for it. > > Let's plumb it into the newly created HDMI connector. > > Signed-off-by: Maxime Ripard <[email protected]> > --- > Documentation/gpu/kms-properties.csv | 1 - > drivers/gpu/drm/drm_atomic.c | 5 +++ > drivers/gpu/drm/drm_atomic_state_helper.c | 17 +++++++ > drivers/gpu/drm/drm_atomic_uapi.c | 4 ++ > drivers/gpu/drm/drm_connector.c | 74 > +++++++++++++++++++++++++++++++ > include/drm/drm_connector.h | 39 ++++++++++++++++ > 6 files changed, 139 insertions(+), 1 deletion(-) > > diff --git a/Documentation/gpu/kms-properties.csv > b/Documentation/gpu/kms-properties.csv > index 0f9590834829..caef14c532d4 100644 > --- a/Documentation/gpu/kms-properties.csv > +++ b/Documentation/gpu/kms-properties.csv > @@ -17,7 +17,6 @@ Owner Module/Drivers,Group,Property Name,Type,Property > Values,Object attached,De > ,Virtual GPU,“suggested X”,RANGE,"Min=0, Max=0xffffffff",Connector,property > to suggest an X offset for a connector > ,,“suggested Y”,RANGE,"Min=0, Max=0xffffffff",Connector,property to suggest > an Y offset for a connector > ,Optional,"""aspect ratio""",ENUM,"{ ""None"", ""4:3"", ""16:9"" > }",Connector,TDB > -i915,Generic,"""Broadcast RGB""",ENUM,"{ ""Automatic"", ""Full"", ""Limited > 16:235"" }",Connector,"When this property is set to Limited 16:235 and CTM is > set, the hardware will be programmed with the result of the multiplication of > CTM by the limited range matrix to ensure the pixels normally in the range > 0..1.0 are remapped to the range 16/255..235/255." Hi, have a look at this old doc for the property, and... > ,,“audio”,ENUM,"{ ""force-dvi"", ""off"", ""auto"", ""on"" }",Connector,TBD > ,SDVO-TV,“mode”,ENUM,"{ ""NTSC_M"", ""NTSC_J"", ""NTSC_443"", ""PAL_B"" } > etc.",Connector,TBD > ,,"""left_margin""",RANGE,"Min=0, Max= SDVO dependent",Connector,TBD ... > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index d9a7e101e4e5..b45471d540ac 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1174,6 +1174,29 @@ static const u32 dp_colorspaces = > BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) | > BIT(DRM_MODE_COLORIMETRY_BT2020_YCC); > > +static const struct drm_prop_enum_list broadcast_rgb_names[] = { > + { DRM_HDMI_BROADCAST_RGB_AUTO, "Automatic" }, > + { DRM_HDMI_BROADCAST_RGB_FULL, "Full" }, > + { DRM_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235" }, > +}; > + > +/* > + * drm_hdmi_connector_get_broadcast_rgb_name - Return a string for HDMI > connector RGB broadcast selection > + * @broadcast_rgb: Broadcast RGB selection to compute name of > + * > + * Returns: the name of the Broadcast RGB selection, or NULL if the type > + * is not valid. > + */ > +const char * > +drm_hdmi_connector_get_broadcast_rgb_name(enum drm_hdmi_broadcast_rgb > broadcast_rgb) > +{ > + if (broadcast_rgb > DRM_HDMI_BROADCAST_RGB_LIMITED) > + return NULL; > + > + return broadcast_rgb_names[broadcast_rgb].name; > +} > +EXPORT_SYMBOL(drm_hdmi_connector_get_broadcast_rgb_name); > + > /** > * DOC: standard connector properties > * > @@ -1640,6 +1663,24 @@ > EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); > /** > * DOC: HDMI connector properties > * > + * Broadcast RGB (HDMI Specific): > + * Indicates the RGB Range (Full vs Limited) used. > + * > + * The value of this property can be one of the following: > + * > + * Automatic: > + * RGB Range is selected automatically based on the mode > + * according to the HDMI specifications. > + * > + * Full: > + * Full RGB Range is forced. > + * > + * Limited 16:235: > + * Limited RGB Range is forced. > + * > + * Drivers can set up this property by calling > + * drm_connector_attach_broadcast_rgb_property(). ...compare it to this. There is one crucial detail lost: setting this property does two or three things: it clips conversion input values to [0.0, 1.0] range, programs a conversion matrix to convert full-range RGB to destination RGB, and sends infoframes to indicate the chosen destination RGB. The distinction is important, because use cases like PLUGE calibration (Rec. ITU-R BT.814-4) rely on indicating limited range while pixel values are still able to carry sub-black values. Other procedures might also want to use super-whites. This is impossible with the existing "Broadcast RGB" property, but that is a different matter. The old doc didn't exactly say it sets the infoframe fields either, but I presume it does. I feel the documentation needs to be much more explicit here. > + * > * content type (HDMI specific): > * Indicates content type setting to be used in HDMI infoframes to indicate > * content type for the external device, so that it adjusts its display > @@ -2500,6 +2541,39 @@ int > drm_connector_attach_hdr_output_metadata_property(struct drm_connector *conn > } > EXPORT_SYMBOL(drm_connector_attach_hdr_output_metadata_property); > > +/** > + * drm_connector_attach_broadcast_rgb_property - attach "Broadcast RGB" > property > + * @connector: connector to attach max bpc property on. "max bpc" pasta. > + * > + * This is used to add support for forcing the RGB range on a connector > + * > + * Returns: > + * Zero on success, negative errno on failure. > + */ > +int drm_connector_attach_broadcast_rgb_property(struct drm_connector > *connector) > +{ > + struct drm_device *dev = connector->dev; > + struct drm_property *prop; > + > + prop = connector->broadcast_rgb_property; > + if (!prop) { > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM, > + "Broadcast RGB", > + broadcast_rgb_names, > + > ARRAY_SIZE(broadcast_rgb_names)); > + if (!prop) > + return -EINVAL; > + > + connector->broadcast_rgb_property = prop; > + } > + > + drm_object_attach_property(&connector->base, prop, > + DRM_HDMI_BROADCAST_RGB_AUTO); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_connector_attach_broadcast_rgb_property); > + > /** > * drm_connector_attach_colorspace_property - attach "Colorspace" property > * @connector: connector to attach the property on. > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 5961f2ad48b1..fdcf64ab91a9 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -368,6 +368,33 @@ enum drm_panel_orientation { > DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, > }; > > +/** > + * enum drm_hdmi_broadcast_rgb - Broadcast RGB Selection for a > @drm_hdmi_connector > + * > + * This enum is used to track broadcast RGB selection. There are no > + * separate #defines for the uapi! Why the "no separate defines" comment? That's the norm. The KMS property UAPI works by string names for enum values. Some enum properties might expose C enums for the values, but that is an exception that cannot be fixed due to stable UAPI. Thanks, pq > + */ > +enum drm_hdmi_broadcast_rgb { > + /** > + * @DRM_HDMI_BROADCAST_RGB_AUTO: The RGB range is selected > + * automatically based on the mode. > + */ > + DRM_HDMI_BROADCAST_RGB_AUTO, > + > + /** > + * @DRM_HDMI_BROADCAST_RGB_FULL: Full range RGB is forced. > + */ > + DRM_HDMI_BROADCAST_RGB_FULL, > + > + /** > + * @DRM_HDMI_BROADCAST_RGB_LIMITED: Limited range RGB is forced. > + */ > + DRM_HDMI_BROADCAST_RGB_LIMITED, > +};
pgpyGpbfK4H2g.pgp
Description: OpenPGP digital signature
