| Message ID | 20231207-kms-hdmi-connector-state-v5-8-6538e19d634d@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/connector: Create HDMI Connector infrastructure | expand |
Hi Dave, On Thu, Dec 14, 2023 at 02:43:37PM +0000, Dave Stevenson wrote: > On Thu, 7 Dec 2023 at 15:50, Maxime Ripard <mripard@kernel.org> 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. > > To have such a significant proportion of the patch being kunit tests > when there was no reference to such in the commit text was slightly > unexpected. Thanks for your review. Does that mean that you would prefer the tests to be in a separate patch? Maxime
Hi Maxime On Fri, 12 Jan 2024 at 13:59, Maxime Ripard <mripard@kernel.org> wrote: > > Hi Dave, > > On Thu, Dec 14, 2023 at 02:43:37PM +0000, Dave Stevenson wrote: > > On Thu, 7 Dec 2023 at 15:50, Maxime Ripard <mripard@kernel.org> 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. > > > > To have such a significant proportion of the patch being kunit tests > > when there was no reference to such in the commit text was slightly > > unexpected. > > Thanks for your review. Does that mean that you would prefer the tests > to be in a separate patch? If there was a need for a respin, then I think ideally yes, or at least a mention in the commit text ("Let's plumb it into the newly created HDMI connector*, and add appropriate unit tests*"). Overall I'm not that fussed though. Dave
On Thu, Dec 07, 2023 at 04:49:31PM +0100, Maxime Ripard 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 <mripard@kernel.org> > --- > 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 | 76 +++++ > drivers/gpu/drm/tests/Makefile | 1 + > .../gpu/drm/tests/drm_atomic_state_helper_test.c | 376 +++++++++++++++++++++ > drivers/gpu/drm/tests/drm_connector_test.c | 117 ++++++- > drivers/gpu/drm/tests/drm_kunit_edid.h | 106 ++++++ > include/drm/drm_connector.h | 36 ++ > 10 files changed, 737 insertions(+), 2 deletions(-) > > 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." > ,,“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_atomic.c b/drivers/gpu/drm/drm_atomic.c > index c31fc0b48c31..1465a7f09a0b 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1142,6 +1142,11 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, > drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc); > drm_printf(p, "\tcolorspace=%s\n", drm_get_colorspace_name(state->colorspace)); > > + if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA || > + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) > + drm_printf(p, "\tbroadcast_rgb=%s\n", > + drm_hdmi_connector_get_broadcast_rgb_name(state->hdmi.broadcast_rgb)); > + > if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) > if (state->writeback_job && state->writeback_job->fb) > drm_printf(p, "\tfb=%d\n", state->writeback_job->fb->base.id); > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > index e69c0cc1c6da..10d98620a358 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -583,6 +583,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset); > void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > struct drm_connector_state *new_state) > { > + new_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; > } > EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); > > @@ -650,6 +651,22 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check); > int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, > struct drm_atomic_state *state) > { > + struct drm_connector_state *old_state = > + drm_atomic_get_old_connector_state(state, connector); > + struct drm_connector_state *new_state = > + drm_atomic_get_new_connector_state(state, connector); > + > + if (old_state->hdmi.broadcast_rgb != new_state->hdmi.broadcast_rgb) { > + struct drm_crtc *crtc = new_state->crtc; > + struct drm_crtc_state *crtc_state; > + > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > + > + crtc_state->mode_changed = true; > + } > + > return 0; > } > EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check); > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index aee4a65d4959..3eb4f4bc8b71 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -818,6 +818,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > state->max_requested_bpc = val; > } else if (property == connector->privacy_screen_sw_state_property) { > state->privacy_screen_sw_state = val; > + } else if (property == connector->broadcast_rgb_property) { > + state->hdmi.broadcast_rgb = val; > } else if (connector->funcs->atomic_set_property) { > return connector->funcs->atomic_set_property(connector, > state, property, val); > @@ -901,6 +903,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > *val = state->max_requested_bpc; > } else if (property == connector->privacy_screen_sw_state_property) { > *val = state->privacy_screen_sw_state; > + } else if (property == connector->broadcast_rgb_property) { > + *val = state->hdmi.broadcast_rgb; > } 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 d9961cce8245..929b0a911f62 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1183,6 +1183,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 > * > @@ -1655,6 +1678,26 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); > /** > * DOC: HDMI connector properties > * > + * Broadcast RGB > + * Indicates the RGB Quantization Range (Full vs Limited) used. > + * Infoframes will be generated according to that value. > + * > + * 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. Unlike the name suggests, > + * this works for any number of bits-per-component. > + * > + * Drivers can set up this property by calling > + * drm_connector_attach_broadcast_rgb_property(). > + * This is a good time to document this in more detail. There might be two different things being affected: 1. The signalling (InfoFrame/SDP/...) 2. The color pipeline processing All values of Broadcast RGB always affect the color pipeline processing such that a full-range input to the CRTC is converted to either full- or limited-range, depending on what the monitor is supposed to accept. When automatic is selected, does that mean that there is no signalling, or that the signalling matches what the monitor is supposed to accept according to the spec? Also, is this really HDMI specific? When full or limited is selected and the monitor doesn't support the signalling, what happens? > * 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 > @@ -2517,6 +2560,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 the property on. > + * > + * 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/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile > index d6183b3d7688..b29ddfd90596 100644 > --- a/drivers/gpu/drm/tests/Makefile > +++ b/drivers/gpu/drm/tests/Makefile > @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST_HELPERS) += \ > drm_kunit_helpers.o > > obj-$(CONFIG_DRM_KUNIT_TEST) += \ > + drm_atomic_state_helper_test.o \ > drm_buddy_test.o \ > drm_cmdline_parser_test.o \ > drm_connector_test.o \ > diff --git a/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c > new file mode 100644 > index 000000000000..21e6f796ee13 > --- /dev/null > +++ b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c > @@ -0,0 +1,376 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +/* > + * Kunit test for drm_atomic_state_helper functions > + */ > + > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_state_helper.h> > +#include <drm/drm_atomic_uapi.h> > +#include <drm/drm_drv.h> > +#include <drm/drm_edid.h> > +#include <drm/drm_connector.h> > +#include <drm/drm_fourcc.h> > +#include <drm/drm_kunit_helpers.h> > +#include <drm/drm_managed.h> > +#include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_probe_helper.h> > + > +#include <drm/drm_print.h> > +#include "../drm_crtc_internal.h" > + > +#include <kunit/test.h> > + > +#include "drm_kunit_edid.h" > + > +struct drm_atomic_helper_connector_hdmi_priv { > + struct drm_device drm; > + struct drm_plane *plane; > + struct drm_crtc *crtc; > + struct drm_encoder encoder; > + struct drm_connector connector; > + > + const char *current_edid; > + size_t current_edid_len; > +}; > + > +#define connector_to_priv(c) \ > + container_of_const(c, struct drm_atomic_helper_connector_hdmi_priv, connector) > + > +static struct drm_display_mode *find_preferred_mode(struct drm_connector *connector) > +{ > + struct drm_device *drm = connector->dev; > + struct drm_display_mode *mode, *preferred; > + > + mutex_lock(&drm->mode_config.mutex); > + preferred = list_first_entry(&connector->modes, struct drm_display_mode, head); > + list_for_each_entry(mode, &connector->modes, head) > + if (mode->type & DRM_MODE_TYPE_PREFERRED) > + preferred = mode; > + mutex_unlock(&drm->mode_config.mutex); > + > + return preferred; > +} > + > +static int light_up_connector(struct kunit *test, > + struct drm_device *drm, > + struct drm_crtc *crtc, > + struct drm_connector *connector, > + struct drm_display_mode *mode, > + struct drm_modeset_acquire_ctx *ctx) > +{ > + struct drm_atomic_state *state; > + struct drm_connector_state *conn_state; > + struct drm_crtc_state *crtc_state; > + int ret; > + > + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > + > + conn_state = drm_atomic_get_connector_state(state, connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state); > + > + ret = drm_atomic_set_crtc_for_connector(conn_state, crtc); > + KUNIT_EXPECT_EQ(test, ret, 0); > + > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); > + > + ret = drm_atomic_set_mode_for_crtc(crtc_state, mode); > + KUNIT_EXPECT_EQ(test, ret, 0); > + > + crtc_state->enable = true; > + crtc_state->active = true; > + > + ret = drm_atomic_commit(state); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + return 0; > +} > + > +static int set_connector_edid(struct kunit *test, struct drm_connector *connector, > + const char *edid, size_t edid_len) > +{ > + struct drm_atomic_helper_connector_hdmi_priv *priv = > + connector_to_priv(connector); > + struct drm_device *drm = connector->dev; > + int ret; > + > + priv->current_edid = edid; > + priv->current_edid_len = edid_len; > + > + mutex_lock(&drm->mode_config.mutex); > + ret = connector->funcs->fill_modes(connector, 4096, 4096); > + mutex_unlock(&drm->mode_config.mutex); > + KUNIT_ASSERT_GT(test, ret, 0); > + > + return 0; > +} > + > +static int dummy_connector_get_modes(struct drm_connector *connector) > +{ > + struct drm_atomic_helper_connector_hdmi_priv *priv = > + connector_to_priv(connector); > + const struct drm_edid *edid; > + unsigned int num_modes; > + > + edid = drm_edid_alloc(priv->current_edid, priv->current_edid_len); > + if (!edid) > + return -EINVAL; > + > + drm_edid_connector_update(connector, edid); > + num_modes = drm_edid_connector_add_modes(connector); > + > + drm_edid_free(edid); > + > + return num_modes; > +} > + > +static const struct drm_connector_helper_funcs dummy_connector_helper_funcs = { > + .atomic_check = drm_atomic_helper_connector_hdmi_check, > + .get_modes = dummy_connector_get_modes, > +}; > + > +static void dummy_hdmi_connector_reset(struct drm_connector *connector) > +{ > + drm_atomic_helper_connector_reset(connector); > + __drm_atomic_helper_connector_hdmi_reset(connector, connector->state); > +} > + > +static const struct drm_connector_funcs dummy_connector_funcs = { > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .fill_modes = drm_helper_probe_single_connector_modes, > + .reset = dummy_hdmi_connector_reset, > +}; > + > +static > +struct drm_atomic_helper_connector_hdmi_priv * > +drm_atomic_helper_connector_hdmi_init(struct kunit *test) > +{ > + struct drm_atomic_helper_connector_hdmi_priv *priv; > + struct drm_connector *conn; > + struct drm_encoder *enc; > + struct drm_device *drm; > + struct device *dev; > + int ret; > + > + dev = drm_kunit_helper_alloc_device(test); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); > + > + priv = drm_kunit_helper_alloc_drm_device(test, dev, > + struct drm_atomic_helper_connector_hdmi_priv, drm, > + DRIVER_MODESET | DRIVER_ATOMIC); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > + test->priv = priv; > + > + drm = &priv->drm; > + priv->plane = drm_kunit_helper_create_primary_plane(test, drm, > + NULL, > + NULL, > + NULL, 0, > + NULL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->plane); > + > + priv->crtc = drm_kunit_helper_create_crtc(test, drm, > + priv->plane, NULL, > + NULL, > + NULL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->crtc); > + > + enc = &priv->encoder; > + ret = drmm_encoder_init(drm, enc, NULL, DRM_MODE_ENCODER_TMDS, NULL); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + enc->possible_crtcs = drm_crtc_mask(priv->crtc); > + > + conn = &priv->connector; > + ret = drmm_connector_hdmi_init(drm, conn, > + &dummy_connector_funcs, > + DRM_MODE_CONNECTOR_HDMIA, > + NULL); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + drm_connector_helper_add(conn, &dummy_connector_helper_funcs); > + drm_connector_attach_encoder(conn, enc); > + > + drm_mode_config_reset(drm); > + > + ret = set_connector_edid(test, conn, > + test_edid_hdmi_1080p_rgb_max_200mhz, > + ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz)); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + return priv; > +} > + > +/* > + * Test that if we change the RGB quantization property to a different > + * value, we trigger a mode change on the connector's CRTC, which will > + * in turn disable/enable the connector. > + */ > +static void drm_test_check_broadcast_rgb_crtc_mode_changed(struct kunit *test) > +{ > + struct drm_atomic_helper_connector_hdmi_priv *priv; > + struct drm_modeset_acquire_ctx *ctx; > + struct drm_connector_state *old_conn_state; > + struct drm_connector_state *new_conn_state; > + struct drm_crtc_state *crtc_state; > + struct drm_atomic_state *state; > + struct drm_display_mode *preferred; > + struct drm_connector *conn; > + struct drm_device *drm; > + struct drm_crtc *crtc; > + int ret; > + > + priv = drm_atomic_helper_connector_hdmi_init(test); > + KUNIT_ASSERT_NOT_NULL(test, priv); > + > + ctx = drm_kunit_helper_acquire_ctx_alloc(test); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > + > + conn = &priv->connector; > + preferred = find_preferred_mode(conn); > + KUNIT_ASSERT_NOT_NULL(test, preferred); > + > + drm = &priv->drm; > + crtc = priv->crtc; > + ret = light_up_connector(test, drm, crtc, conn, preferred, ctx); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > + > + new_conn_state = drm_atomic_get_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > + > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > + > + new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_FULL; > + > + KUNIT_ASSERT_NE(test, > + old_conn_state->hdmi.broadcast_rgb, > + new_conn_state->hdmi.broadcast_rgb); > + > + ret = drm_atomic_check_only(state); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + new_conn_state = drm_atomic_get_new_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > + KUNIT_EXPECT_EQ(test, new_conn_state->hdmi.broadcast_rgb, DRM_HDMI_BROADCAST_RGB_FULL); > + > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); > + KUNIT_EXPECT_TRUE(test, crtc_state->mode_changed); > +} > + > +/* > + * Test that if we set the RGB quantization property to the same value, > + * we don't trigger a mode change on the connector's CRTC and leave the > + * connector unaffected. > + */ > +static void drm_test_check_broadcast_rgb_crtc_mode_not_changed(struct kunit *test) > +{ > + struct drm_atomic_helper_connector_hdmi_priv *priv; > + struct drm_modeset_acquire_ctx *ctx; > + struct drm_connector_state *old_conn_state; > + struct drm_connector_state *new_conn_state; > + struct drm_crtc_state *crtc_state; > + struct drm_atomic_state *state; > + struct drm_display_mode *preferred; > + struct drm_connector *conn; > + struct drm_device *drm; > + struct drm_crtc *crtc; > + int ret; > + > + priv = drm_atomic_helper_connector_hdmi_init(test); > + KUNIT_ASSERT_NOT_NULL(test, priv); > + > + ctx = drm_kunit_helper_acquire_ctx_alloc(test); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > + > + conn = &priv->connector; > + preferred = find_preferred_mode(conn); > + KUNIT_ASSERT_NOT_NULL(test, preferred); > + > + drm = &priv->drm; > + crtc = priv->crtc; > + ret = light_up_connector(test, drm, crtc, conn, preferred, ctx); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > + > + new_conn_state = drm_atomic_get_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > + > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > + > + new_conn_state->hdmi.broadcast_rgb = old_conn_state->hdmi.broadcast_rgb; > + > + ret = drm_atomic_check_only(state); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > + > + new_conn_state = drm_atomic_get_new_connector_state(state, conn); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > + > + KUNIT_EXPECT_EQ(test, > + old_conn_state->hdmi.broadcast_rgb, > + new_conn_state->hdmi.broadcast_rgb); > + > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); > + KUNIT_EXPECT_FALSE(test, crtc_state->mode_changed); > +} > + > +static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = { > + KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_changed), > + KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_not_changed), > + { } > +}; > + > +static struct kunit_suite drm_atomic_helper_connector_hdmi_check_test_suite = { > + .name = "drm_atomic_helper_connector_hdmi_check", > + .test_cases = drm_atomic_helper_connector_hdmi_check_tests, > +}; > + > +/* > + * Test that the value of the Broadcast RGB property out of reset is set > + * to auto. > + */ > +static void drm_test_check_broadcast_rgb_value(struct kunit *test) > +{ > + struct drm_atomic_helper_connector_hdmi_priv *priv; > + struct drm_connector_state *conn_state; > + struct drm_connector *conn; > + > + priv = drm_atomic_helper_connector_hdmi_init(test); > + KUNIT_ASSERT_NOT_NULL(test, priv); > + > + conn = &priv->connector; > + conn_state = conn->state; > + KUNIT_EXPECT_EQ(test, conn_state->hdmi.broadcast_rgb, DRM_HDMI_BROADCAST_RGB_AUTO); > +} > + > +static struct kunit_case drm_atomic_helper_connector_hdmi_reset_tests[] = { > + KUNIT_CASE(drm_test_check_broadcast_rgb_value), > + { } > +}; > + > +static struct kunit_suite drm_atomic_helper_connector_hdmi_reset_test_suite = { > + .name = "drm_atomic_helper_connector_hdmi_reset", > + .test_cases = drm_atomic_helper_connector_hdmi_reset_tests, > +}; > + > +kunit_test_suites( > + &drm_atomic_helper_connector_hdmi_check_test_suite, > + &drm_atomic_helper_connector_hdmi_reset_test_suite, > +); > + > +MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c > index 8f070cacab3b..41d33dea30af 100644 > --- a/drivers/gpu/drm/tests/drm_connector_test.c > +++ b/drivers/gpu/drm/tests/drm_connector_test.c > @@ -12,6 +12,8 @@ > > #include <kunit/test.h> > > +#include "../drm_crtc_internal.h" > + > struct drm_connector_init_priv { > struct drm_device drm; > struct drm_connector connector; > @@ -357,10 +359,123 @@ static struct kunit_suite drm_get_tv_mode_from_name_test_suite = { > .test_cases = drm_get_tv_mode_from_name_tests, > }; > > +struct drm_hdmi_connector_get_broadcast_rgb_name_test { > + unsigned int kind; > + const char *expected_name; > +}; > + > +#define BROADCAST_RGB_TEST(_kind, _name) \ > + { \ > + .kind = _kind, \ > + .expected_name = _name, \ > + } > + > +static void drm_test_drm_hdmi_connector_get_broadcast_rgb_name(struct kunit *test) > +{ > + const struct drm_hdmi_connector_get_broadcast_rgb_name_test *params = > + test->param_value; > + > + KUNIT_EXPECT_STREQ(test, > + drm_hdmi_connector_get_broadcast_rgb_name(params->kind), > + params->expected_name); > +} > + > +static const > +struct drm_hdmi_connector_get_broadcast_rgb_name_test > +drm_hdmi_connector_get_broadcast_rgb_name_valid_tests[] = { > + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_AUTO, "Automatic"), > + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_FULL, "Full"), > + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235"), > +}; > + > +static void > +drm_hdmi_connector_get_broadcast_rgb_name_valid_desc(const struct drm_hdmi_connector_get_broadcast_rgb_name_test *t, > + char *desc) > +{ > + sprintf(desc, "%s", t->expected_name); > +} > + > +KUNIT_ARRAY_PARAM(drm_hdmi_connector_get_broadcast_rgb_name_valid, > + drm_hdmi_connector_get_broadcast_rgb_name_valid_tests, > + drm_hdmi_connector_get_broadcast_rgb_name_valid_desc); > + > +static void drm_test_drm_hdmi_connector_get_broadcast_rgb_name_invalid(struct kunit *test) > +{ > + KUNIT_EXPECT_NULL(test, drm_hdmi_connector_get_broadcast_rgb_name(3)); > +}; > + > +static struct kunit_case drm_hdmi_connector_get_broadcast_rgb_name_tests[] = { > + KUNIT_CASE_PARAM(drm_test_drm_hdmi_connector_get_broadcast_rgb_name, > + drm_hdmi_connector_get_broadcast_rgb_name_valid_gen_params), > + KUNIT_CASE(drm_test_drm_hdmi_connector_get_broadcast_rgb_name_invalid), > + { } > +}; > + > +static struct kunit_suite drm_hdmi_connector_get_broadcast_rgb_name_test_suite = { > + .name = "drm_hdmi_connector_get_broadcast_rgb_name", > + .test_cases = drm_hdmi_connector_get_broadcast_rgb_name_tests, > +}; > + > +static void drm_test_drm_connector_attach_broadcast_rgb_property(struct kunit *test) > +{ > + struct drm_connector_init_priv *priv = test->priv; > + struct drm_connector *connector = &priv->connector; > + struct drm_property *prop; > + int ret; > + > + ret = drmm_connector_init(&priv->drm, connector, > + &dummy_funcs, > + DRM_MODE_CONNECTOR_HDMIA, > + &priv->ddc); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + ret = drm_connector_attach_broadcast_rgb_property(connector); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + prop = connector->broadcast_rgb_property; > + KUNIT_ASSERT_NOT_NULL(test, prop); > + KUNIT_EXPECT_NOT_NULL(test, drm_mode_obj_find_prop_id(&connector->base, prop->base.id)); > +} > + > +static void drm_test_drm_connector_attach_broadcast_rgb_property_hdmi_connector(struct kunit *test) > +{ > + struct drm_connector_init_priv *priv = test->priv; > + struct drm_connector *connector = &priv->connector; > + struct drm_property *prop; > + int ret; > + > + ret = drmm_connector_hdmi_init(&priv->drm, connector, > + &dummy_funcs, > + DRM_MODE_CONNECTOR_HDMIA, > + &priv->ddc); > + KUNIT_EXPECT_EQ(test, ret, 0); > + > + ret = drm_connector_attach_broadcast_rgb_property(connector); > + KUNIT_ASSERT_EQ(test, ret, 0); > + > + prop = connector->broadcast_rgb_property; > + KUNIT_ASSERT_NOT_NULL(test, prop); > + KUNIT_EXPECT_NOT_NULL(test, drm_mode_obj_find_prop_id(&connector->base, prop->base.id)); > +} > + > +static struct kunit_case drm_connector_attach_broadcast_rgb_property_tests[] = { > + KUNIT_CASE(drm_test_drm_connector_attach_broadcast_rgb_property), > + KUNIT_CASE(drm_test_drm_connector_attach_broadcast_rgb_property_hdmi_connector), > + { } > +}; > + > +static struct kunit_suite drm_connector_attach_broadcast_rgb_property_test_suite = { > + .name = "drm_connector_attach_broadcast_rgb_property", > + .init = drm_test_connector_init, > + .test_cases = drm_connector_attach_broadcast_rgb_property_tests, > +}; > + > kunit_test_suites( > &drmm_connector_hdmi_init_test_suite, > &drmm_connector_init_test_suite, > - &drm_get_tv_mode_from_name_test_suite > + &drm_connector_attach_broadcast_rgb_property_test_suite, > + &drm_get_tv_mode_from_name_test_suite, > + &drm_hdmi_connector_get_broadcast_rgb_name_test_suite > ); > > MODULE_AUTHOR("Maxime Ripard <maxime@cerno.tech>"); > diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h > new file mode 100644 > index 000000000000..2bba316de064 > --- /dev/null > +++ b/drivers/gpu/drm/tests/drm_kunit_edid.h > @@ -0,0 +1,106 @@ > +#ifndef DRM_KUNIT_EDID_H_ > +#define DRM_KUNIT_EDID_H_ > + > +/* > + * edid-decode (hex): > + * > + * 00 ff ff ff ff ff ff 00 31 d8 2a 00 00 00 00 00 > + * 00 21 01 03 81 a0 5a 78 02 00 00 00 00 00 00 00 > + * 00 00 00 20 00 00 01 01 01 01 01 01 01 01 01 01 > + * 01 01 01 01 01 01 02 3a 80 18 71 38 2d 40 58 2c > + * 45 00 40 84 63 00 00 1e 00 00 00 fc 00 54 65 73 > + * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 32 > + * 46 1e 46 0f 00 0a 20 20 20 20 20 20 00 00 00 10 > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 92 > + * > + * 02 03 1b 81 e3 05 00 20 41 10 e2 00 4a 6d 03 0c > + * 00 12 34 00 28 20 00 00 00 00 00 00 00 00 00 00 > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0 > + * > + * ---------------- > + * > + * Block 0, Base EDID: > + * EDID Structure Version & Revision: 1.3 > + * Vendor & Product Identification: > + * Manufacturer: LNX > + * Model: 42 > + * Made in: 2023 > + * Basic Display Parameters & Features: > + * Digital display > + * DFP 1.x compatible TMDS > + * Maximum image size: 160 cm x 90 cm > + * Gamma: 2.20 > + * Monochrome or grayscale display > + * First detailed timing is the preferred timing > + * Color Characteristics: > + * Red : 0.0000, 0.0000 > + * Green: 0.0000, 0.0000 > + * Blue : 0.0000, 0.0000 > + * White: 0.0000, 0.0000 > + * Established Timings I & II: > + * DMT 0x04: 640x480 59.940476 Hz 4:3 31.469 kHz 25.175000 MHz > + * Standard Timings: none > + * Detailed Timing Descriptors: > + * DTD 1: 1920x1080 60.000000 Hz 16:9 67.500 kHz 148.500000 MHz (1600 mm x 900 mm) > + * Hfront 88 Hsync 44 Hback 148 Hpol P > + * Vfront 4 Vsync 5 Vback 36 Vpol P > + * Display Product Name: 'Test EDID' > + * Display Range Limits: > + * Monitor ranges (GTF): 50-70 Hz V, 30-70 kHz H, max dotclock 150 MHz > + * Dummy Descriptor: > + * Extension blocks: 1 > + * Checksum: 0x92 > + * > + * ---------------- > + * > + * Block 1, CTA-861 Extension Block: > + * Revision: 3 > + * Underscans IT Video Formats by default > + * Native detailed modes: 1 > + * Colorimetry Data Block: > + * sRGB > + * Video Data Block: > + * VIC 16: 1920x1080 60.000000 Hz 16:9 67.500 kHz 148.500000 MHz > + * Video Capability Data Block: > + * YCbCr quantization: No Data > + * RGB quantization: Selectable (via AVI Q) > + * PT scan behavior: No Data > + * IT scan behavior: Always Underscanned > + * CE scan behavior: Always Underscanned > + * Vendor-Specific Data Block (HDMI), OUI 00-0C-03: > + * Source physical address: 1.2.3.4 > + * Maximum TMDS clock: 200 MHz > + * Extended HDMI video details: > + * Checksum: 0xd0 Unused space in Extension Block: 100 bytes > + */ > +const unsigned char test_edid_hdmi_1080p_rgb_max_200mhz[] = { > + 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x2a, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x21, 0x01, 0x03, 0x81, 0xa0, 0x5a, 0x78, > + 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, > + 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, > + 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x3a, 0x80, 0x18, 0x71, 0x38, > + 0x2d, 0x40, 0x58, 0x2c, 0x45, 0x00, 0x40, 0x84, 0x63, 0x00, 0x00, 0x1e, > + 0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44, > + 0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x32, > + 0x46, 0x00, 0x00, 0xc4, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, > + 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x41, 0x02, 0x03, 0x1b, 0x81, > + 0xe3, 0x05, 0x00, 0x20, 0x41, 0x10, 0xe2, 0x00, 0x4a, 0x6d, 0x03, 0x0c, > + 0x00, 0x12, 0x34, 0x00, 0x28, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > + 0x00, 0x00, 0x00, 0xd0 > +}; > + > +#endif // DRM_KUNIT_EDID_H_ > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 000a2a156619..3867a4c01b78 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -368,6 +368,30 @@ enum drm_panel_orientation { > DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, > }; > > +/** > + * enum drm_hdmi_broadcast_rgb - Broadcast RGB Selection for an HDMI @drm_connector > + */ > +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, > +}; > + > +const char * > +drm_hdmi_connector_get_broadcast_rgb_name(enum drm_hdmi_broadcast_rgb broadcast_rgb); > + > /** > * struct drm_monitor_range_info - Panel's Monitor range in EDID for > * &drm_display_info > @@ -1037,6 +1061,11 @@ struct drm_connector_state { > * @drm_atomic_helper_connector_hdmi_check(). > */ > struct { > + /** > + * @broadcast_rgb: Connector property to pass the > + * Broadcast RGB selection value. > + */ > + enum drm_hdmi_broadcast_rgb broadcast_rgb; > } hdmi; > }; > > @@ -1706,6 +1735,12 @@ struct drm_connector { > */ > struct drm_property *privacy_screen_hw_state_property; > > + /** > + * @broadcast_rgb_property: Connector property to set the > + * Broadcast RGB selection to output with. > + */ > + struct drm_property *broadcast_rgb_property; > + > #define DRM_CONNECTOR_POLL_HPD (1 << 0) > #define DRM_CONNECTOR_POLL_CONNECT (1 << 1) > #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) > @@ -2026,6 +2061,7 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector, > u32 scaling_mode_mask); > int drm_connector_attach_vrr_capable_property( > struct drm_connector *connector); > +int drm_connector_attach_broadcast_rgb_property(struct drm_connector *connector); > int drm_connector_attach_colorspace_property(struct drm_connector *connector); > int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *connector); > bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state, > > -- > 2.43.0 >
On Mon, Jan 15, 2024 at 03:33:08PM +0100, Sebastian Wick wrote: > On Thu, Dec 07, 2023 at 04:49:31PM +0100, Maxime Ripard 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 <mripard@kernel.org> > > --- > > 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 | 76 +++++ > > drivers/gpu/drm/tests/Makefile | 1 + > > .../gpu/drm/tests/drm_atomic_state_helper_test.c | 376 +++++++++++++++++++++ > > drivers/gpu/drm/tests/drm_connector_test.c | 117 ++++++- > > drivers/gpu/drm/tests/drm_kunit_edid.h | 106 ++++++ > > include/drm/drm_connector.h | 36 ++ > > 10 files changed, 737 insertions(+), 2 deletions(-) > > > > 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." > > ,,“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_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index c31fc0b48c31..1465a7f09a0b 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1142,6 +1142,11 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, > > drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc); > > drm_printf(p, "\tcolorspace=%s\n", drm_get_colorspace_name(state->colorspace)); > > > > + if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA || > > + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) > > + drm_printf(p, "\tbroadcast_rgb=%s\n", > > + drm_hdmi_connector_get_broadcast_rgb_name(state->hdmi.broadcast_rgb)); > > + > > if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) > > if (state->writeback_job && state->writeback_job->fb) > > drm_printf(p, "\tfb=%d\n", state->writeback_job->fb->base.id); > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > > index e69c0cc1c6da..10d98620a358 100644 > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > @@ -583,6 +583,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset); > > void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > > struct drm_connector_state *new_state) > > { > > + new_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; > > } > > EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); > > > > @@ -650,6 +651,22 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check); > > int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, > > struct drm_atomic_state *state) > > { > > + struct drm_connector_state *old_state = > > + drm_atomic_get_old_connector_state(state, connector); > > + struct drm_connector_state *new_state = > > + drm_atomic_get_new_connector_state(state, connector); > > + > > + if (old_state->hdmi.broadcast_rgb != new_state->hdmi.broadcast_rgb) { > > + struct drm_crtc *crtc = new_state->crtc; > > + struct drm_crtc_state *crtc_state; > > + > > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > > + if (IS_ERR(crtc_state)) > > + return PTR_ERR(crtc_state); > > + > > + crtc_state->mode_changed = true; > > + } > > + > > return 0; > > } > > EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check); > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > > index aee4a65d4959..3eb4f4bc8b71 100644 > > --- a/drivers/gpu/drm/drm_atomic_uapi.c > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > > @@ -818,6 +818,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > > state->max_requested_bpc = val; > > } else if (property == connector->privacy_screen_sw_state_property) { > > state->privacy_screen_sw_state = val; > > + } else if (property == connector->broadcast_rgb_property) { > > + state->hdmi.broadcast_rgb = val; > > } else if (connector->funcs->atomic_set_property) { > > return connector->funcs->atomic_set_property(connector, > > state, property, val); > > @@ -901,6 +903,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > > *val = state->max_requested_bpc; > > } else if (property == connector->privacy_screen_sw_state_property) { > > *val = state->privacy_screen_sw_state; > > + } else if (property == connector->broadcast_rgb_property) { > > + *val = state->hdmi.broadcast_rgb; > > } 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 d9961cce8245..929b0a911f62 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -1183,6 +1183,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 > > * > > @@ -1655,6 +1678,26 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); > > /** > > * DOC: HDMI connector properties > > * > > + * Broadcast RGB > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > + * Infoframes will be generated according to that value. > > + * > > + * 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. Unlike the name suggests, > > + * this works for any number of bits-per-component. > > + * > > + * Drivers can set up this property by calling > > + * drm_connector_attach_broadcast_rgb_property(). > > + * > > This is a good time to document this in more detail. There might be two > different things being affected: > > 1. The signalling (InfoFrame/SDP/...) > 2. The color pipeline processing > > All values of Broadcast RGB always affect the color pipeline processing > such that a full-range input to the CRTC is converted to either full- or > limited-range, depending on what the monitor is supposed to accept. > > When automatic is selected, does that mean that there is no signalling, > or that the signalling matches what the monitor is supposed to accept > according to the spec? Also, is this really HDMI specific? > > When full or limited is selected and the monitor doesn't support the > signalling, what happens? Forgot to mention: user-space still has no control over RGB vs YCbCr on the cable, so is this only affecting RGB? If not, how does it affect YCbCr? > > > * 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 > > @@ -2517,6 +2560,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 the property on. > > + * > > + * 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/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile > > index d6183b3d7688..b29ddfd90596 100644 > > --- a/drivers/gpu/drm/tests/Makefile > > +++ b/drivers/gpu/drm/tests/Makefile > > @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST_HELPERS) += \ > > drm_kunit_helpers.o > > > > obj-$(CONFIG_DRM_KUNIT_TEST) += \ > > + drm_atomic_state_helper_test.o \ > > drm_buddy_test.o \ > > drm_cmdline_parser_test.o \ > > drm_connector_test.o \ > > diff --git a/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c > > new file mode 100644 > > index 000000000000..21e6f796ee13 > > --- /dev/null > > +++ b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c > > @@ -0,0 +1,376 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +/* > > + * Kunit test for drm_atomic_state_helper functions > > + */ > > + > > +#include <drm/drm_atomic.h> > > +#include <drm/drm_atomic_state_helper.h> > > +#include <drm/drm_atomic_uapi.h> > > +#include <drm/drm_drv.h> > > +#include <drm/drm_edid.h> > > +#include <drm/drm_connector.h> > > +#include <drm/drm_fourcc.h> > > +#include <drm/drm_kunit_helpers.h> > > +#include <drm/drm_managed.h> > > +#include <drm/drm_modeset_helper_vtables.h> > > +#include <drm/drm_probe_helper.h> > > + > > +#include <drm/drm_print.h> > > +#include "../drm_crtc_internal.h" > > + > > +#include <kunit/test.h> > > + > > +#include "drm_kunit_edid.h" > > + > > +struct drm_atomic_helper_connector_hdmi_priv { > > + struct drm_device drm; > > + struct drm_plane *plane; > > + struct drm_crtc *crtc; > > + struct drm_encoder encoder; > > + struct drm_connector connector; > > + > > + const char *current_edid; > > + size_t current_edid_len; > > +}; > > + > > +#define connector_to_priv(c) \ > > + container_of_const(c, struct drm_atomic_helper_connector_hdmi_priv, connector) > > + > > +static struct drm_display_mode *find_preferred_mode(struct drm_connector *connector) > > +{ > > + struct drm_device *drm = connector->dev; > > + struct drm_display_mode *mode, *preferred; > > + > > + mutex_lock(&drm->mode_config.mutex); > > + preferred = list_first_entry(&connector->modes, struct drm_display_mode, head); > > + list_for_each_entry(mode, &connector->modes, head) > > + if (mode->type & DRM_MODE_TYPE_PREFERRED) > > + preferred = mode; > > + mutex_unlock(&drm->mode_config.mutex); > > + > > + return preferred; > > +} > > + > > +static int light_up_connector(struct kunit *test, > > + struct drm_device *drm, > > + struct drm_crtc *crtc, > > + struct drm_connector *connector, > > + struct drm_display_mode *mode, > > + struct drm_modeset_acquire_ctx *ctx) > > +{ > > + struct drm_atomic_state *state; > > + struct drm_connector_state *conn_state; > > + struct drm_crtc_state *crtc_state; > > + int ret; > > + > > + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > > + > > + conn_state = drm_atomic_get_connector_state(state, connector); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state); > > + > > + ret = drm_atomic_set_crtc_for_connector(conn_state, crtc); > > + KUNIT_EXPECT_EQ(test, ret, 0); > > + > > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); > > + > > + ret = drm_atomic_set_mode_for_crtc(crtc_state, mode); > > + KUNIT_EXPECT_EQ(test, ret, 0); > > + > > + crtc_state->enable = true; > > + crtc_state->active = true; > > + > > + ret = drm_atomic_commit(state); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + return 0; > > +} > > + > > +static int set_connector_edid(struct kunit *test, struct drm_connector *connector, > > + const char *edid, size_t edid_len) > > +{ > > + struct drm_atomic_helper_connector_hdmi_priv *priv = > > + connector_to_priv(connector); > > + struct drm_device *drm = connector->dev; > > + int ret; > > + > > + priv->current_edid = edid; > > + priv->current_edid_len = edid_len; > > + > > + mutex_lock(&drm->mode_config.mutex); > > + ret = connector->funcs->fill_modes(connector, 4096, 4096); > > + mutex_unlock(&drm->mode_config.mutex); > > + KUNIT_ASSERT_GT(test, ret, 0); > > + > > + return 0; > > +} > > + > > +static int dummy_connector_get_modes(struct drm_connector *connector) > > +{ > > + struct drm_atomic_helper_connector_hdmi_priv *priv = > > + connector_to_priv(connector); > > + const struct drm_edid *edid; > > + unsigned int num_modes; > > + > > + edid = drm_edid_alloc(priv->current_edid, priv->current_edid_len); > > + if (!edid) > > + return -EINVAL; > > + > > + drm_edid_connector_update(connector, edid); > > + num_modes = drm_edid_connector_add_modes(connector); > > + > > + drm_edid_free(edid); > > + > > + return num_modes; > > +} > > + > > +static const struct drm_connector_helper_funcs dummy_connector_helper_funcs = { > > + .atomic_check = drm_atomic_helper_connector_hdmi_check, > > + .get_modes = dummy_connector_get_modes, > > +}; > > + > > +static void dummy_hdmi_connector_reset(struct drm_connector *connector) > > +{ > > + drm_atomic_helper_connector_reset(connector); > > + __drm_atomic_helper_connector_hdmi_reset(connector, connector->state); > > +} > > + > > +static const struct drm_connector_funcs dummy_connector_funcs = { > > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > > + .fill_modes = drm_helper_probe_single_connector_modes, > > + .reset = dummy_hdmi_connector_reset, > > +}; > > + > > +static > > +struct drm_atomic_helper_connector_hdmi_priv * > > +drm_atomic_helper_connector_hdmi_init(struct kunit *test) > > +{ > > + struct drm_atomic_helper_connector_hdmi_priv *priv; > > + struct drm_connector *conn; > > + struct drm_encoder *enc; > > + struct drm_device *drm; > > + struct device *dev; > > + int ret; > > + > > + dev = drm_kunit_helper_alloc_device(test); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); > > + > > + priv = drm_kunit_helper_alloc_drm_device(test, dev, > > + struct drm_atomic_helper_connector_hdmi_priv, drm, > > + DRIVER_MODESET | DRIVER_ATOMIC); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > > + test->priv = priv; > > + > > + drm = &priv->drm; > > + priv->plane = drm_kunit_helper_create_primary_plane(test, drm, > > + NULL, > > + NULL, > > + NULL, 0, > > + NULL); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->plane); > > + > > + priv->crtc = drm_kunit_helper_create_crtc(test, drm, > > + priv->plane, NULL, > > + NULL, > > + NULL); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->crtc); > > + > > + enc = &priv->encoder; > > + ret = drmm_encoder_init(drm, enc, NULL, DRM_MODE_ENCODER_TMDS, NULL); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + enc->possible_crtcs = drm_crtc_mask(priv->crtc); > > + > > + conn = &priv->connector; > > + ret = drmm_connector_hdmi_init(drm, conn, > > + &dummy_connector_funcs, > > + DRM_MODE_CONNECTOR_HDMIA, > > + NULL); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + drm_connector_helper_add(conn, &dummy_connector_helper_funcs); > > + drm_connector_attach_encoder(conn, enc); > > + > > + drm_mode_config_reset(drm); > > + > > + ret = set_connector_edid(test, conn, > > + test_edid_hdmi_1080p_rgb_max_200mhz, > > + ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz)); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + return priv; > > +} > > + > > +/* > > + * Test that if we change the RGB quantization property to a different > > + * value, we trigger a mode change on the connector's CRTC, which will > > + * in turn disable/enable the connector. > > + */ > > +static void drm_test_check_broadcast_rgb_crtc_mode_changed(struct kunit *test) > > +{ > > + struct drm_atomic_helper_connector_hdmi_priv *priv; > > + struct drm_modeset_acquire_ctx *ctx; > > + struct drm_connector_state *old_conn_state; > > + struct drm_connector_state *new_conn_state; > > + struct drm_crtc_state *crtc_state; > > + struct drm_atomic_state *state; > > + struct drm_display_mode *preferred; > > + struct drm_connector *conn; > > + struct drm_device *drm; > > + struct drm_crtc *crtc; > > + int ret; > > + > > + priv = drm_atomic_helper_connector_hdmi_init(test); > > + KUNIT_ASSERT_NOT_NULL(test, priv); > > + > > + ctx = drm_kunit_helper_acquire_ctx_alloc(test); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > > + > > + conn = &priv->connector; > > + preferred = find_preferred_mode(conn); > > + KUNIT_ASSERT_NOT_NULL(test, preferred); > > + > > + drm = &priv->drm; > > + crtc = priv->crtc; > > + ret = light_up_connector(test, drm, crtc, conn, preferred, ctx); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > > + > > + new_conn_state = drm_atomic_get_connector_state(state, conn); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > > + > > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > > + > > + new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_FULL; > > + > > + KUNIT_ASSERT_NE(test, > > + old_conn_state->hdmi.broadcast_rgb, > > + new_conn_state->hdmi.broadcast_rgb); > > + > > + ret = drm_atomic_check_only(state); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + new_conn_state = drm_atomic_get_new_connector_state(state, conn); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > > + KUNIT_EXPECT_EQ(test, new_conn_state->hdmi.broadcast_rgb, DRM_HDMI_BROADCAST_RGB_FULL); > > + > > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); > > + KUNIT_EXPECT_TRUE(test, crtc_state->mode_changed); > > +} > > + > > +/* > > + * Test that if we set the RGB quantization property to the same value, > > + * we don't trigger a mode change on the connector's CRTC and leave the > > + * connector unaffected. > > + */ > > +static void drm_test_check_broadcast_rgb_crtc_mode_not_changed(struct kunit *test) > > +{ > > + struct drm_atomic_helper_connector_hdmi_priv *priv; > > + struct drm_modeset_acquire_ctx *ctx; > > + struct drm_connector_state *old_conn_state; > > + struct drm_connector_state *new_conn_state; > > + struct drm_crtc_state *crtc_state; > > + struct drm_atomic_state *state; > > + struct drm_display_mode *preferred; > > + struct drm_connector *conn; > > + struct drm_device *drm; > > + struct drm_crtc *crtc; > > + int ret; > > + > > + priv = drm_atomic_helper_connector_hdmi_init(test); > > + KUNIT_ASSERT_NOT_NULL(test, priv); > > + > > + ctx = drm_kunit_helper_acquire_ctx_alloc(test); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); > > + > > + conn = &priv->connector; > > + preferred = find_preferred_mode(conn); > > + KUNIT_ASSERT_NOT_NULL(test, preferred); > > + > > + drm = &priv->drm; > > + crtc = priv->crtc; > > + ret = light_up_connector(test, drm, crtc, conn, preferred, ctx); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); > > + > > + new_conn_state = drm_atomic_get_connector_state(state, conn); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > > + > > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > > + > > + new_conn_state->hdmi.broadcast_rgb = old_conn_state->hdmi.broadcast_rgb; > > + > > + ret = drm_atomic_check_only(state); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + old_conn_state = drm_atomic_get_old_connector_state(state, conn); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); > > + > > + new_conn_state = drm_atomic_get_new_connector_state(state, conn); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); > > + > > + KUNIT_EXPECT_EQ(test, > > + old_conn_state->hdmi.broadcast_rgb, > > + new_conn_state->hdmi.broadcast_rgb); > > + > > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); > > + KUNIT_EXPECT_FALSE(test, crtc_state->mode_changed); > > +} > > + > > +static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = { > > + KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_changed), > > + KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_not_changed), > > + { } > > +}; > > + > > +static struct kunit_suite drm_atomic_helper_connector_hdmi_check_test_suite = { > > + .name = "drm_atomic_helper_connector_hdmi_check", > > + .test_cases = drm_atomic_helper_connector_hdmi_check_tests, > > +}; > > + > > +/* > > + * Test that the value of the Broadcast RGB property out of reset is set > > + * to auto. > > + */ > > +static void drm_test_check_broadcast_rgb_value(struct kunit *test) > > +{ > > + struct drm_atomic_helper_connector_hdmi_priv *priv; > > + struct drm_connector_state *conn_state; > > + struct drm_connector *conn; > > + > > + priv = drm_atomic_helper_connector_hdmi_init(test); > > + KUNIT_ASSERT_NOT_NULL(test, priv); > > + > > + conn = &priv->connector; > > + conn_state = conn->state; > > + KUNIT_EXPECT_EQ(test, conn_state->hdmi.broadcast_rgb, DRM_HDMI_BROADCAST_RGB_AUTO); > > +} > > + > > +static struct kunit_case drm_atomic_helper_connector_hdmi_reset_tests[] = { > > + KUNIT_CASE(drm_test_check_broadcast_rgb_value), > > + { } > > +}; > > + > > +static struct kunit_suite drm_atomic_helper_connector_hdmi_reset_test_suite = { > > + .name = "drm_atomic_helper_connector_hdmi_reset", > > + .test_cases = drm_atomic_helper_connector_hdmi_reset_tests, > > +}; > > + > > +kunit_test_suites( > > + &drm_atomic_helper_connector_hdmi_check_test_suite, > > + &drm_atomic_helper_connector_hdmi_reset_test_suite, > > +); > > + > > +MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>"); > > +MODULE_LICENSE("GPL"); > > diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c > > index 8f070cacab3b..41d33dea30af 100644 > > --- a/drivers/gpu/drm/tests/drm_connector_test.c > > +++ b/drivers/gpu/drm/tests/drm_connector_test.c > > @@ -12,6 +12,8 @@ > > > > #include <kunit/test.h> > > > > +#include "../drm_crtc_internal.h" > > + > > struct drm_connector_init_priv { > > struct drm_device drm; > > struct drm_connector connector; > > @@ -357,10 +359,123 @@ static struct kunit_suite drm_get_tv_mode_from_name_test_suite = { > > .test_cases = drm_get_tv_mode_from_name_tests, > > }; > > > > +struct drm_hdmi_connector_get_broadcast_rgb_name_test { > > + unsigned int kind; > > + const char *expected_name; > > +}; > > + > > +#define BROADCAST_RGB_TEST(_kind, _name) \ > > + { \ > > + .kind = _kind, \ > > + .expected_name = _name, \ > > + } > > + > > +static void drm_test_drm_hdmi_connector_get_broadcast_rgb_name(struct kunit *test) > > +{ > > + const struct drm_hdmi_connector_get_broadcast_rgb_name_test *params = > > + test->param_value; > > + > > + KUNIT_EXPECT_STREQ(test, > > + drm_hdmi_connector_get_broadcast_rgb_name(params->kind), > > + params->expected_name); > > +} > > + > > +static const > > +struct drm_hdmi_connector_get_broadcast_rgb_name_test > > +drm_hdmi_connector_get_broadcast_rgb_name_valid_tests[] = { > > + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_AUTO, "Automatic"), > > + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_FULL, "Full"), > > + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235"), > > +}; > > + > > +static void > > +drm_hdmi_connector_get_broadcast_rgb_name_valid_desc(const struct drm_hdmi_connector_get_broadcast_rgb_name_test *t, > > + char *desc) > > +{ > > + sprintf(desc, "%s", t->expected_name); > > +} > > + > > +KUNIT_ARRAY_PARAM(drm_hdmi_connector_get_broadcast_rgb_name_valid, > > + drm_hdmi_connector_get_broadcast_rgb_name_valid_tests, > > + drm_hdmi_connector_get_broadcast_rgb_name_valid_desc); > > + > > +static void drm_test_drm_hdmi_connector_get_broadcast_rgb_name_invalid(struct kunit *test) > > +{ > > + KUNIT_EXPECT_NULL(test, drm_hdmi_connector_get_broadcast_rgb_name(3)); > > +}; > > + > > +static struct kunit_case drm_hdmi_connector_get_broadcast_rgb_name_tests[] = { > > + KUNIT_CASE_PARAM(drm_test_drm_hdmi_connector_get_broadcast_rgb_name, > > + drm_hdmi_connector_get_broadcast_rgb_name_valid_gen_params), > > + KUNIT_CASE(drm_test_drm_hdmi_connector_get_broadcast_rgb_name_invalid), > > + { } > > +}; > > + > > +static struct kunit_suite drm_hdmi_connector_get_broadcast_rgb_name_test_suite = { > > + .name = "drm_hdmi_connector_get_broadcast_rgb_name", > > + .test_cases = drm_hdmi_connector_get_broadcast_rgb_name_tests, > > +}; > > + > > +static void drm_test_drm_connector_attach_broadcast_rgb_property(struct kunit *test) > > +{ > > + struct drm_connector_init_priv *priv = test->priv; > > + struct drm_connector *connector = &priv->connector; > > + struct drm_property *prop; > > + int ret; > > + > > + ret = drmm_connector_init(&priv->drm, connector, > > + &dummy_funcs, > > + DRM_MODE_CONNECTOR_HDMIA, > > + &priv->ddc); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + ret = drm_connector_attach_broadcast_rgb_property(connector); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + prop = connector->broadcast_rgb_property; > > + KUNIT_ASSERT_NOT_NULL(test, prop); > > + KUNIT_EXPECT_NOT_NULL(test, drm_mode_obj_find_prop_id(&connector->base, prop->base.id)); > > +} > > + > > +static void drm_test_drm_connector_attach_broadcast_rgb_property_hdmi_connector(struct kunit *test) > > +{ > > + struct drm_connector_init_priv *priv = test->priv; > > + struct drm_connector *connector = &priv->connector; > > + struct drm_property *prop; > > + int ret; > > + > > + ret = drmm_connector_hdmi_init(&priv->drm, connector, > > + &dummy_funcs, > > + DRM_MODE_CONNECTOR_HDMIA, > > + &priv->ddc); > > + KUNIT_EXPECT_EQ(test, ret, 0); > > + > > + ret = drm_connector_attach_broadcast_rgb_property(connector); > > + KUNIT_ASSERT_EQ(test, ret, 0); > > + > > + prop = connector->broadcast_rgb_property; > > + KUNIT_ASSERT_NOT_NULL(test, prop); > > + KUNIT_EXPECT_NOT_NULL(test, drm_mode_obj_find_prop_id(&connector->base, prop->base.id)); > > +} > > + > > +static struct kunit_case drm_connector_attach_broadcast_rgb_property_tests[] = { > > + KUNIT_CASE(drm_test_drm_connector_attach_broadcast_rgb_property), > > + KUNIT_CASE(drm_test_drm_connector_attach_broadcast_rgb_property_hdmi_connector), > > + { } > > +}; > > + > > +static struct kunit_suite drm_connector_attach_broadcast_rgb_property_test_suite = { > > + .name = "drm_connector_attach_broadcast_rgb_property", > > + .init = drm_test_connector_init, > > + .test_cases = drm_connector_attach_broadcast_rgb_property_tests, > > +}; > > + > > kunit_test_suites( > > &drmm_connector_hdmi_init_test_suite, > > &drmm_connector_init_test_suite, > > - &drm_get_tv_mode_from_name_test_suite > > + &drm_connector_attach_broadcast_rgb_property_test_suite, > > + &drm_get_tv_mode_from_name_test_suite, > > + &drm_hdmi_connector_get_broadcast_rgb_name_test_suite > > ); > > > > MODULE_AUTHOR("Maxime Ripard <maxime@cerno.tech>"); > > diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h > > new file mode 100644 > > index 000000000000..2bba316de064 > > --- /dev/null > > +++ b/drivers/gpu/drm/tests/drm_kunit_edid.h > > @@ -0,0 +1,106 @@ > > +#ifndef DRM_KUNIT_EDID_H_ > > +#define DRM_KUNIT_EDID_H_ > > + > > +/* > > + * edid-decode (hex): > > + * > > + * 00 ff ff ff ff ff ff 00 31 d8 2a 00 00 00 00 00 > > + * 00 21 01 03 81 a0 5a 78 02 00 00 00 00 00 00 00 > > + * 00 00 00 20 00 00 01 01 01 01 01 01 01 01 01 01 > > + * 01 01 01 01 01 01 02 3a 80 18 71 38 2d 40 58 2c > > + * 45 00 40 84 63 00 00 1e 00 00 00 fc 00 54 65 73 > > + * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 32 > > + * 46 1e 46 0f 00 0a 20 20 20 20 20 20 00 00 00 10 > > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 92 > > + * > > + * 02 03 1b 81 e3 05 00 20 41 10 e2 00 4a 6d 03 0c > > + * 00 12 34 00 28 20 00 00 00 00 00 00 00 00 00 00 > > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > > + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0 > > + * > > + * ---------------- > > + * > > + * Block 0, Base EDID: > > + * EDID Structure Version & Revision: 1.3 > > + * Vendor & Product Identification: > > + * Manufacturer: LNX > > + * Model: 42 > > + * Made in: 2023 > > + * Basic Display Parameters & Features: > > + * Digital display > > + * DFP 1.x compatible TMDS > > + * Maximum image size: 160 cm x 90 cm > > + * Gamma: 2.20 > > + * Monochrome or grayscale display > > + * First detailed timing is the preferred timing > > + * Color Characteristics: > > + * Red : 0.0000, 0.0000 > > + * Green: 0.0000, 0.0000 > > + * Blue : 0.0000, 0.0000 > > + * White: 0.0000, 0.0000 > > + * Established Timings I & II: > > + * DMT 0x04: 640x480 59.940476 Hz 4:3 31.469 kHz 25.175000 MHz > > + * Standard Timings: none > > + * Detailed Timing Descriptors: > > + * DTD 1: 1920x1080 60.000000 Hz 16:9 67.500 kHz 148.500000 MHz (1600 mm x 900 mm) > > + * Hfront 88 Hsync 44 Hback 148 Hpol P > > + * Vfront 4 Vsync 5 Vback 36 Vpol P > > + * Display Product Name: 'Test EDID' > > + * Display Range Limits: > > + * Monitor ranges (GTF): 50-70 Hz V, 30-70 kHz H, max dotclock 150 MHz > > + * Dummy Descriptor: > > + * Extension blocks: 1 > > + * Checksum: 0x92 > > + * > > + * ---------------- > > + * > > + * Block 1, CTA-861 Extension Block: > > + * Revision: 3 > > + * Underscans IT Video Formats by default > > + * Native detailed modes: 1 > > + * Colorimetry Data Block: > > + * sRGB > > + * Video Data Block: > > + * VIC 16: 1920x1080 60.000000 Hz 16:9 67.500 kHz 148.500000 MHz > > + * Video Capability Data Block: > > + * YCbCr quantization: No Data > > + * RGB quantization: Selectable (via AVI Q) > > + * PT scan behavior: No Data > > + * IT scan behavior: Always Underscanned > > + * CE scan behavior: Always Underscanned > > + * Vendor-Specific Data Block (HDMI), OUI 00-0C-03: > > + * Source physical address: 1.2.3.4 > > + * Maximum TMDS clock: 200 MHz > > + * Extended HDMI video details: > > + * Checksum: 0xd0 Unused space in Extension Block: 100 bytes > > + */ > > +const unsigned char test_edid_hdmi_1080p_rgb_max_200mhz[] = { > > + 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x2a, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x21, 0x01, 0x03, 0x81, 0xa0, 0x5a, 0x78, > > + 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, > > + 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, > > + 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x3a, 0x80, 0x18, 0x71, 0x38, > > + 0x2d, 0x40, 0x58, 0x2c, 0x45, 0x00, 0x40, 0x84, 0x63, 0x00, 0x00, 0x1e, > > + 0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44, > > + 0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x32, > > + 0x46, 0x00, 0x00, 0xc4, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, > > + 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x41, 0x02, 0x03, 0x1b, 0x81, > > + 0xe3, 0x05, 0x00, 0x20, 0x41, 0x10, 0xe2, 0x00, 0x4a, 0x6d, 0x03, 0x0c, > > + 0x00, 0x12, 0x34, 0x00, 0x28, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, > > + 0x00, 0x00, 0x00, 0xd0 > > +}; > > + > > +#endif // DRM_KUNIT_EDID_H_ > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index 000a2a156619..3867a4c01b78 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -368,6 +368,30 @@ enum drm_panel_orientation { > > DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, > > }; > > > > +/** > > + * enum drm_hdmi_broadcast_rgb - Broadcast RGB Selection for an HDMI @drm_connector > > + */ > > +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, > > +}; > > + > > +const char * > > +drm_hdmi_connector_get_broadcast_rgb_name(enum drm_hdmi_broadcast_rgb broadcast_rgb); > > + > > /** > > * struct drm_monitor_range_info - Panel's Monitor range in EDID for > > * &drm_display_info > > @@ -1037,6 +1061,11 @@ struct drm_connector_state { > > * @drm_atomic_helper_connector_hdmi_check(). > > */ > > struct { > > + /** > > + * @broadcast_rgb: Connector property to pass the > > + * Broadcast RGB selection value. > > + */ > > + enum drm_hdmi_broadcast_rgb broadcast_rgb; > > } hdmi; > > }; > > > > @@ -1706,6 +1735,12 @@ struct drm_connector { > > */ > > struct drm_property *privacy_screen_hw_state_property; > > > > + /** > > + * @broadcast_rgb_property: Connector property to set the > > + * Broadcast RGB selection to output with. > > + */ > > + struct drm_property *broadcast_rgb_property; > > + > > #define DRM_CONNECTOR_POLL_HPD (1 << 0) > > #define DRM_CONNECTOR_POLL_CONNECT (1 << 1) > > #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) > > @@ -2026,6 +2061,7 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector, > > u32 scaling_mode_mask); > > int drm_connector_attach_vrr_capable_property( > > struct drm_connector *connector); > > +int drm_connector_attach_broadcast_rgb_property(struct drm_connector *connector); > > int drm_connector_attach_colorspace_property(struct drm_connector *connector); > > int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *connector); > > bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state, > > > > -- > > 2.43.0 > >
On Mon, Jan 15, 2024 at 03:33:08PM +0100, Sebastian Wick wrote: > On Thu, Dec 07, 2023 at 04:49:31PM +0100, Maxime Ripard 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 <mripard@kernel.org> > > --- > > 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 | 76 +++++ > > drivers/gpu/drm/tests/Makefile | 1 + > > .../gpu/drm/tests/drm_atomic_state_helper_test.c | 376 +++++++++++++++++++++ > > drivers/gpu/drm/tests/drm_connector_test.c | 117 ++++++- > > drivers/gpu/drm/tests/drm_kunit_edid.h | 106 ++++++ > > include/drm/drm_connector.h | 36 ++ > > 10 files changed, 737 insertions(+), 2 deletions(-) > > > > 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." > > ,,“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_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index c31fc0b48c31..1465a7f09a0b 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1142,6 +1142,11 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, > > drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc); > > drm_printf(p, "\tcolorspace=%s\n", drm_get_colorspace_name(state->colorspace)); > > > > + if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA || > > + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) > > + drm_printf(p, "\tbroadcast_rgb=%s\n", > > + drm_hdmi_connector_get_broadcast_rgb_name(state->hdmi.broadcast_rgb)); > > + > > if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) > > if (state->writeback_job && state->writeback_job->fb) > > drm_printf(p, "\tfb=%d\n", state->writeback_job->fb->base.id); > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > > index e69c0cc1c6da..10d98620a358 100644 > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > @@ -583,6 +583,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset); > > void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > > struct drm_connector_state *new_state) > > { > > + new_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; > > } > > EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); > > > > @@ -650,6 +651,22 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check); > > int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, > > struct drm_atomic_state *state) > > { > > + struct drm_connector_state *old_state = > > + drm_atomic_get_old_connector_state(state, connector); > > + struct drm_connector_state *new_state = > > + drm_atomic_get_new_connector_state(state, connector); > > + > > + if (old_state->hdmi.broadcast_rgb != new_state->hdmi.broadcast_rgb) { > > + struct drm_crtc *crtc = new_state->crtc; > > + struct drm_crtc_state *crtc_state; > > + > > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > > + if (IS_ERR(crtc_state)) > > + return PTR_ERR(crtc_state); > > + > > + crtc_state->mode_changed = true; > > + } > > + > > return 0; > > } > > EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check); > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > > index aee4a65d4959..3eb4f4bc8b71 100644 > > --- a/drivers/gpu/drm/drm_atomic_uapi.c > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > > @@ -818,6 +818,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > > state->max_requested_bpc = val; > > } else if (property == connector->privacy_screen_sw_state_property) { > > state->privacy_screen_sw_state = val; > > + } else if (property == connector->broadcast_rgb_property) { > > + state->hdmi.broadcast_rgb = val; > > } else if (connector->funcs->atomic_set_property) { > > return connector->funcs->atomic_set_property(connector, > > state, property, val); > > @@ -901,6 +903,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > > *val = state->max_requested_bpc; > > } else if (property == connector->privacy_screen_sw_state_property) { > > *val = state->privacy_screen_sw_state; > > + } else if (property == connector->broadcast_rgb_property) { > > + *val = state->hdmi.broadcast_rgb; > > } 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 d9961cce8245..929b0a911f62 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -1183,6 +1183,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 > > * > > @@ -1655,6 +1678,26 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); > > /** > > * DOC: HDMI connector properties > > * > > + * Broadcast RGB > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > + * Infoframes will be generated according to that value. > > + * > > + * 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. Unlike the name suggests, > > + * this works for any number of bits-per-component. > > + * > > + * Drivers can set up this property by calling > > + * drm_connector_attach_broadcast_rgb_property(). > > + * > > This is a good time to document this in more detail. I have the feeling that it already is documented in more detail. But anyway, last time we discussed it the answer was basically to not bother and just merge the thing. So I'm getting some mixed signals here. > There might be two different things being affected: > > 1. The signalling (InfoFrame/SDP/...) > 2. The color pipeline processing > > All values of Broadcast RGB always affect the color pipeline processing > such that a full-range input to the CRTC is converted to either full- or > limited-range, depending on what the monitor is supposed to accept. > > When automatic is selected, does that mean that there is no signalling, > or that the signalling matches what the monitor is supposed to accept > according to the spec? The doc states that "Infoframes will be generated according to that value". Is it ambiguous? > Also, is this really HDMI specific? Probably not, but it can easily be expanded to other connector types when needs be. > When full or limited is selected and the monitor doesn't support the > signalling, what happens? I would expect colors to be off Maxime
On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote: > On Mon, Jan 15, 2024 at 03:33:08PM +0100, Sebastian Wick wrote: > > On Thu, Dec 07, 2023 at 04:49:31PM +0100, Maxime Ripard 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 <mripard@kernel.org> > > > --- > > > 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 | 76 +++++ > > > drivers/gpu/drm/tests/Makefile | 1 + > > > .../gpu/drm/tests/drm_atomic_state_helper_test.c | 376 +++++++++++++++++++++ > > > drivers/gpu/drm/tests/drm_connector_test.c | 117 ++++++- > > > drivers/gpu/drm/tests/drm_kunit_edid.h | 106 ++++++ > > > include/drm/drm_connector.h | 36 ++ > > > 10 files changed, 737 insertions(+), 2 deletions(-) > > > > > > 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." > > > ,,“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_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index c31fc0b48c31..1465a7f09a0b 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -1142,6 +1142,11 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, > > > drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc); > > > drm_printf(p, "\tcolorspace=%s\n", drm_get_colorspace_name(state->colorspace)); > > > > > > + if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA || > > > + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) > > > + drm_printf(p, "\tbroadcast_rgb=%s\n", > > > + drm_hdmi_connector_get_broadcast_rgb_name(state->hdmi.broadcast_rgb)); > > > + > > > if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) > > > if (state->writeback_job && state->writeback_job->fb) > > > drm_printf(p, "\tfb=%d\n", state->writeback_job->fb->base.id); > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > > > index e69c0cc1c6da..10d98620a358 100644 > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > @@ -583,6 +583,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset); > > > void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > > > struct drm_connector_state *new_state) > > > { > > > + new_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; > > > } > > > EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); > > > > > > @@ -650,6 +651,22 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check); > > > int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, > > > struct drm_atomic_state *state) > > > { > > > + struct drm_connector_state *old_state = > > > + drm_atomic_get_old_connector_state(state, connector); > > > + struct drm_connector_state *new_state = > > > + drm_atomic_get_new_connector_state(state, connector); > > > + > > > + if (old_state->hdmi.broadcast_rgb != new_state->hdmi.broadcast_rgb) { > > > + struct drm_crtc *crtc = new_state->crtc; > > > + struct drm_crtc_state *crtc_state; > > > + > > > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > > > + if (IS_ERR(crtc_state)) > > > + return PTR_ERR(crtc_state); > > > + > > > + crtc_state->mode_changed = true; > > > + } > > > + > > > return 0; > > > } > > > EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check); > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > > > index aee4a65d4959..3eb4f4bc8b71 100644 > > > --- a/drivers/gpu/drm/drm_atomic_uapi.c > > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > > > @@ -818,6 +818,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > > > state->max_requested_bpc = val; > > > } else if (property == connector->privacy_screen_sw_state_property) { > > > state->privacy_screen_sw_state = val; > > > + } else if (property == connector->broadcast_rgb_property) { > > > + state->hdmi.broadcast_rgb = val; > > > } else if (connector->funcs->atomic_set_property) { > > > return connector->funcs->atomic_set_property(connector, > > > state, property, val); > > > @@ -901,6 +903,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > > > *val = state->max_requested_bpc; > > > } else if (property == connector->privacy_screen_sw_state_property) { > > > *val = state->privacy_screen_sw_state; > > > + } else if (property == connector->broadcast_rgb_property) { > > > + *val = state->hdmi.broadcast_rgb; > > > } 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 d9961cce8245..929b0a911f62 100644 > > > --- a/drivers/gpu/drm/drm_connector.c > > > +++ b/drivers/gpu/drm/drm_connector.c > > > @@ -1183,6 +1183,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 > > > * > > > @@ -1655,6 +1678,26 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); > > > /** > > > * DOC: HDMI connector properties > > > * > > > + * Broadcast RGB > > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > > + * Infoframes will be generated according to that value. > > > + * > > > + * 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. Unlike the name suggests, > > > + * this works for any number of bits-per-component. > > > + * > > > + * Drivers can set up this property by calling > > > + * drm_connector_attach_broadcast_rgb_property(). > > > + * > > > > This is a good time to document this in more detail. There might be two > > different things being affected: > > > > 1. The signalling (InfoFrame/SDP/...) > > 2. The color pipeline processing > > > > All values of Broadcast RGB always affect the color pipeline processing > > such that a full-range input to the CRTC is converted to either full- or > > limited-range, depending on what the monitor is supposed to accept. > > > > When automatic is selected, does that mean that there is no signalling, > > or that the signalling matches what the monitor is supposed to accept > > according to the spec? Also, is this really HDMI specific? > > > > When full or limited is selected and the monitor doesn't support the > > signalling, what happens? > > Forgot to mention: user-space still has no control over RGB vs YCbCr on > the cable, so is this only affecting RGB? If not, how does it affect > YCbCr? i915 only supports it for RGB indeed: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_hdmi.c#L2150 Maxime
On Mon, Jan 15, 2024 at 04:25:41PM +0100, Maxime Ripard wrote: > On Mon, Jan 15, 2024 at 03:33:08PM +0100, Sebastian Wick wrote: > > On Thu, Dec 07, 2023 at 04:49:31PM +0100, Maxime Ripard 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 <mripard@kernel.org> > > > --- > > > 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 | 76 +++++ > > > drivers/gpu/drm/tests/Makefile | 1 + > > > .../gpu/drm/tests/drm_atomic_state_helper_test.c | 376 +++++++++++++++++++++ > > > drivers/gpu/drm/tests/drm_connector_test.c | 117 ++++++- > > > drivers/gpu/drm/tests/drm_kunit_edid.h | 106 ++++++ > > > include/drm/drm_connector.h | 36 ++ > > > 10 files changed, 737 insertions(+), 2 deletions(-) > > > > > > 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." > > > ,,“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_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index c31fc0b48c31..1465a7f09a0b 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -1142,6 +1142,11 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, > > > drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc); > > > drm_printf(p, "\tcolorspace=%s\n", drm_get_colorspace_name(state->colorspace)); > > > > > > + if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA || > > > + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) > > > + drm_printf(p, "\tbroadcast_rgb=%s\n", > > > + drm_hdmi_connector_get_broadcast_rgb_name(state->hdmi.broadcast_rgb)); > > > + > > > if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) > > > if (state->writeback_job && state->writeback_job->fb) > > > drm_printf(p, "\tfb=%d\n", state->writeback_job->fb->base.id); > > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > > > index e69c0cc1c6da..10d98620a358 100644 > > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > > @@ -583,6 +583,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset); > > > void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > > > struct drm_connector_state *new_state) > > > { > > > + new_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; > > > } > > > EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); > > > > > > @@ -650,6 +651,22 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check); > > > int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, > > > struct drm_atomic_state *state) > > > { > > > + struct drm_connector_state *old_state = > > > + drm_atomic_get_old_connector_state(state, connector); > > > + struct drm_connector_state *new_state = > > > + drm_atomic_get_new_connector_state(state, connector); > > > + > > > + if (old_state->hdmi.broadcast_rgb != new_state->hdmi.broadcast_rgb) { > > > + struct drm_crtc *crtc = new_state->crtc; > > > + struct drm_crtc_state *crtc_state; > > > + > > > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > > > + if (IS_ERR(crtc_state)) > > > + return PTR_ERR(crtc_state); > > > + > > > + crtc_state->mode_changed = true; > > > + } > > > + > > > return 0; > > > } > > > EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check); > > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > > > index aee4a65d4959..3eb4f4bc8b71 100644 > > > --- a/drivers/gpu/drm/drm_atomic_uapi.c > > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > > > @@ -818,6 +818,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > > > state->max_requested_bpc = val; > > > } else if (property == connector->privacy_screen_sw_state_property) { > > > state->privacy_screen_sw_state = val; > > > + } else if (property == connector->broadcast_rgb_property) { > > > + state->hdmi.broadcast_rgb = val; > > > } else if (connector->funcs->atomic_set_property) { > > > return connector->funcs->atomic_set_property(connector, > > > state, property, val); > > > @@ -901,6 +903,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, > > > *val = state->max_requested_bpc; > > > } else if (property == connector->privacy_screen_sw_state_property) { > > > *val = state->privacy_screen_sw_state; > > > + } else if (property == connector->broadcast_rgb_property) { > > > + *val = state->hdmi.broadcast_rgb; > > > } 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 d9961cce8245..929b0a911f62 100644 > > > --- a/drivers/gpu/drm/drm_connector.c > > > +++ b/drivers/gpu/drm/drm_connector.c > > > @@ -1183,6 +1183,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 > > > * > > > @@ -1655,6 +1678,26 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); > > > /** > > > * DOC: HDMI connector properties > > > * > > > + * Broadcast RGB > > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > > + * Infoframes will be generated according to that value. > > > + * > > > + * 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. Unlike the name suggests, > > > + * this works for any number of bits-per-component. > > > + * > > > + * Drivers can set up this property by calling > > > + * drm_connector_attach_broadcast_rgb_property(). > > > + * > > > > This is a good time to document this in more detail. > > I have the feeling that it already is documented in more detail. But > anyway, last time we discussed it the answer was basically to not bother > and just merge the thing. So I'm getting some mixed signals here. I'm all for merging and not trying to improve the property but documenting it in more detail is definitely something I want to see. > > There might be two different things being affected: > > > > 1. The signalling (InfoFrame/SDP/...) > > 2. The color pipeline processing > > > > All values of Broadcast RGB always affect the color pipeline processing > > such that a full-range input to the CRTC is converted to either full- or > > limited-range, depending on what the monitor is supposed to accept. > > > > When automatic is selected, does that mean that there is no signalling, > > or that the signalling matches what the monitor is supposed to accept > > according to the spec? > > The doc states that "Infoframes will be generated according to that > value". Is it ambiguous? > > > Also, is this really HDMI specific? > > Probably not, but it can easily be expanded to other connector types > when needs be. > > > When full or limited is selected and the monitor doesn't support the > > signalling, what happens? > > I would expect colors to be off > > Maxime
On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrjälä wrote: > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote: > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrjälä wrote: > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote: > > > > > > > /** > > > > > > > * DOC: HDMI connector properties > > > > > > > * > > > > > > > + * Broadcast RGB > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > > > > > > + * Infoframes will be generated according to that value. > > > > > > > + * > > > > > > > + * 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. Unlike the name suggests, > > > > > > > + * this works for any number of bits-per-component. > > > > > > > + * > > > > > > > + * Drivers can set up this property by calling > > > > > > > + * drm_connector_attach_broadcast_rgb_property(). > > > > > > > + * > > > > > > > > > > > > This is a good time to document this in more detail. There might be two > > > > > > different things being affected: > > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...) > > > > > > 2. The color pipeline processing > > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing > > > > > > such that a full-range input to the CRTC is converted to either full- or > > > > > > limited-range, depending on what the monitor is supposed to accept. > > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling, > > > > > > or that the signalling matches what the monitor is supposed to accept > > > > > > according to the spec? Also, is this really HDMI specific? > > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the > > > > > > signalling, what happens? > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on > > > > > the cable, so is this only affecting RGB? If not, how does it affect > > > > > YCbCr? > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if > > > > we're using a YCbCr format, i915 will always use a limited range while > > > > vc4 will follow the value of the property. > > > > > > The property is literally called "Broadcast *RGB*". > > > That should explain why it's only affecting RGB. > > > > Right. And the limited range option is called "Limited 16:235" despite > > being usable on bpc > 8 bits. Naming errors occurs, and history happens > > to make names inconsistent too, that's fine and not an argument in > > itself. > > > > > Full range YCbCr is a much rarer beast so we've never bothered > > > to enable it. > > > > vc4 supports it. > > Someone implemented it incorrectly then. Incorrectly according to what documentation / specification? I'm sorry, but I find it super ironic that i915 gets to do its own thing, not document any of it, and when people try to clean things up they get told that we got it all wrong. > > > Eg. with DP it only became possible with the introduction of the VSC > > > SDP (and I don't recall if there's additional capability checks that > > > are also required). With DP MSA signalling full range YCbCr is not > > > possible at all. > > > > This is for HDMI only. > > > > > I don't recall right now what the HDMI requirements are. > > > > HDMI has supported it for a while, and it's defined (for example) in the > > HDMI 1.4 spec in Section 6.6 - Video Quantization Ranges. It supports > > limited and full range on both RGB and YCbCr, as long as the EDIDs state > > so and the Infoframes signal it. > > I think a good reason for not using a simple boolean like this > YCbCr is that it doesn't cover the color encoding part at all, > which is probably more important than the quantization range. > So we need a new property anyway. This isn't what is being discussed here, and as I've shown you, is completely orthogonal as far as HDMI is concerned. Maxime
On Fri, Feb 02, 2024 at 04:49:04PM +0100, Maxime Ripard wrote: > Hi Sebastian, > > On Mon, Jan 15, 2024 at 03:33:08PM +0100, Sebastian Wick wrote: > > > /** > > > * DOC: HDMI connector properties > > > * > > > + * Broadcast RGB > > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > > + * Infoframes will be generated according to that value. > > > + * > > > + * 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. Unlike the name suggests, > > > + * this works for any number of bits-per-component. > > > + * > > > + * Drivers can set up this property by calling > > > + * drm_connector_attach_broadcast_rgb_property(). > > > + * > > > > This is a good time to document this in more detail. There might be two > > different things being affected: > > > > 1. The signalling (InfoFrame/SDP/...) > > 2. The color pipeline processing > > > > All values of Broadcast RGB always affect the color pipeline processing > > such that a full-range input to the CRTC is converted to either full- or > > limited-range, depending on what the monitor is supposed to accept. > > > > When automatic is selected, does that mean that there is no signalling, > > or that the signalling matches what the monitor is supposed to accept > > according to the spec? Also, is this really HDMI specific? > > > > When full or limited is selected and the monitor doesn't support the > > signalling, what happens? > > Leaving the YCbCr vs RGB discussion aside, would this be better ? Yes, it is. Thanks. We do have to resolve the YCbCr vs RGB issue though. > * Broadcast RGB (HDMI specific) > * Indicates the Quantization Range (Full vs Limited) used. The color > * processing pipeline will be adjusted to match the value of the Ah, another thing no note here is that the CRTC as configured by user space must always produce full range pixels. > * property, and the Infoframes will be generated and sent accordingly. > * > * The value of this property can be one of the following: > * > * Automatic: > * The quantization range is selected automatically based on the > * mode according to the HDMI specifications (HDMI 1.4b - Section > * 6.6 - Video Quantization Ranges). > * > * Full: > * Full quantization range is forced. > * > * Limited 16:235: > * Limited quantization range is forced. Unlike the name suggests, > * this works for any number of bits-per-component. > * > * Property values other than Automatic can result in colors being off (if > * limited is selected but the display expects full), or a black screen > * (if full is selected but the display expects limited). > * > * Drivers can set up this property by calling > * drm_connector_attach_broadcast_rgb_property(). > > Thanks! > Maxime
On Tue, Feb 13, 2024 at 10:38:56AM +0200, Ville Syrjälä wrote: > On Mon, Feb 12, 2024 at 05:53:48PM +0100, Maxime Ripard wrote: > > On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrjälä wrote: > > > On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote: > > > > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote: > > > > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote: > > > > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrjälä wrote: > > > > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote: > > > > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrjälä wrote: > > > > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote: > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote: > > > > > > > > > > > > > /** > > > > > > > > > > > > > * DOC: HDMI connector properties > > > > > > > > > > > > > * > > > > > > > > > > > > > + * Broadcast RGB > > > > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > > > > > > > > > > > > + * Infoframes will be generated according to that value. > > > > > > > > > > > > > + * > > > > > > > > > > > > > + * 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. Unlike the name suggests, > > > > > > > > > > > > > + * this works for any number of bits-per-component. > > > > > > > > > > > > > + * > > > > > > > > > > > > > + * Drivers can set up this property by calling > > > > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property(). > > > > > > > > > > > > > + * > > > > > > > > > > > > > > > > > > > > > > > > This is a good time to document this in more detail. There might be two > > > > > > > > > > > > different things being affected: > > > > > > > > > > > > > > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...) > > > > > > > > > > > > 2. The color pipeline processing > > > > > > > > > > > > > > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing > > > > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or > > > > > > > > > > > > limited-range, depending on what the monitor is supposed to accept. > > > > > > > > > > > > > > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling, > > > > > > > > > > > > or that the signalling matches what the monitor is supposed to accept > > > > > > > > > > > > according to the spec? Also, is this really HDMI specific? > > > > > > > > > > > > > > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the > > > > > > > > > > > > signalling, what happens? > > > > > > > > > > > > > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on > > > > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect > > > > > > > > > > > YCbCr? > > > > > > > > > > > > > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if > > > > > > > > > > we're using a YCbCr format, i915 will always use a limited range while > > > > > > > > > > vc4 will follow the value of the property. > > > > > > > > > > > > > > > > > > The property is literally called "Broadcast *RGB*". > > > > > > > > > That should explain why it's only affecting RGB. > > > > > > > > > > > > > > > > Right. And the limited range option is called "Limited 16:235" despite > > > > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens > > > > > > > > to make names inconsistent too, that's fine and not an argument in > > > > > > > > itself. > > > > > > > > > > > > > > > > > Full range YCbCr is a much rarer beast so we've never bothered > > > > > > > > > to enable it. > > > > > > > > > > > > > > > > vc4 supports it. > > > > > > > > > > > > > > Someone implemented it incorrectly then. > > > > > > > > > > > > Incorrectly according to what documentation / specification? I'm sorry, > > > > > > but I find it super ironic that i915 gets to do its own thing, not > > > > > > document any of it, and when people try to clean things up they get told > > > > > > that we got it all wrong. > > > > > > > > > > FWIW, this was an i915 property and if another driver uses the same > > > > > property name it must have the same behavior. Yes, it isn't standardized > > > > > and yes, it's not documented (hence this effort here) but it's still on > > > > > vc4 to make the property compatible. > > > > > > > > How is it not compatible? It's a superset of what i915 provides, but > > > > it's strictly compatible with it. > > > > > > No it is not. > > > > The property is compatible with i915 interpretation of it, whether you > > like it or not. And that's what Sebastian was referring to. > > > > > Eg. what happens if you set the thing to full range for RGB (which you > > > must on many broken monitors), and then the kernel automagically > > > switches to YCbCr (for whatever reason) but the monitor doesn't > > > support full range YCbCr? Answer: you get crap output. > > > > And that part is just moving goalposts. > > No. Allowing users to get correct colors with broken displays > is the sole reason why this property even exists. HDMI 1.4, Section 6.6 - Video Quantization Ranges: If the sink’s EDID declares a selectable YCC Quantization Range (QY=1), then it shall expect limited range pixel values if it receives AVI YQ=0 and it shall expect full range pixel values if it receives AVI YQ=1. For other values of YQ, the sink shall expect pixel values with the default range for the transmitted video format. So, the only concern you have is if the EDID has QY set to 1 but the monitor doesn't actually support it? If so, could we qualify the monitor as a "broken display" and thus would require that property to apply to YUV too? Maxime
On Thu, Feb 15, 2024 at 11:53:17AM +0100, Maxime Ripard wrote: > On Tue, Feb 13, 2024 at 10:38:56AM +0200, Ville Syrjälä wrote: > > On Mon, Feb 12, 2024 at 05:53:48PM +0100, Maxime Ripard wrote: > > > On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrjälä wrote: > > > > On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote: > > > > > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote: > > > > > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote: > > > > > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrjälä wrote: > > > > > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote: > > > > > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrjälä wrote: > > > > > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote: > > > > > > > > > > > > > > /** > > > > > > > > > > > > > > * DOC: HDMI connector properties > > > > > > > > > > > > > > * > > > > > > > > > > > > > > + * Broadcast RGB > > > > > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > > > > > > > > > > > > > + * Infoframes will be generated according to that value. > > > > > > > > > > > > > > + * > > > > > > > > > > > > > > + * 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. Unlike the name suggests, > > > > > > > > > > > > > > + * this works for any number of bits-per-component. > > > > > > > > > > > > > > + * > > > > > > > > > > > > > > + * Drivers can set up this property by calling > > > > > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property(). > > > > > > > > > > > > > > + * > > > > > > > > > > > > > > > > > > > > > > > > > > This is a good time to document this in more detail. There might be two > > > > > > > > > > > > > different things being affected: > > > > > > > > > > > > > > > > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...) > > > > > > > > > > > > > 2. The color pipeline processing > > > > > > > > > > > > > > > > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing > > > > > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or > > > > > > > > > > > > > limited-range, depending on what the monitor is supposed to accept. > > > > > > > > > > > > > > > > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling, > > > > > > > > > > > > > or that the signalling matches what the monitor is supposed to accept > > > > > > > > > > > > > according to the spec? Also, is this really HDMI specific? > > > > > > > > > > > > > > > > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the > > > > > > > > > > > > > signalling, what happens? > > > > > > > > > > > > > > > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on > > > > > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect > > > > > > > > > > > > YCbCr? > > > > > > > > > > > > > > > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if > > > > > > > > > > > we're using a YCbCr format, i915 will always use a limited range while > > > > > > > > > > > vc4 will follow the value of the property. > > > > > > > > > > > > > > > > > > > > The property is literally called "Broadcast *RGB*". > > > > > > > > > > That should explain why it's only affecting RGB. > > > > > > > > > > > > > > > > > > Right. And the limited range option is called "Limited 16:235" despite > > > > > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens > > > > > > > > > to make names inconsistent too, that's fine and not an argument in > > > > > > > > > itself. > > > > > > > > > > > > > > > > > > > Full range YCbCr is a much rarer beast so we've never bothered > > > > > > > > > > to enable it. > > > > > > > > > > > > > > > > > > vc4 supports it. > > > > > > > > > > > > > > > > Someone implemented it incorrectly then. > > > > > > > > > > > > > > Incorrectly according to what documentation / specification? I'm sorry, > > > > > > > but I find it super ironic that i915 gets to do its own thing, not > > > > > > > document any of it, and when people try to clean things up they get told > > > > > > > that we got it all wrong. > > > > > > > > > > > > FWIW, this was an i915 property and if another driver uses the same > > > > > > property name it must have the same behavior. Yes, it isn't standardized > > > > > > and yes, it's not documented (hence this effort here) but it's still on > > > > > > vc4 to make the property compatible. > > > > > > > > > > How is it not compatible? It's a superset of what i915 provides, but > > > > > it's strictly compatible with it. > > > > > > > > No it is not. > > > > > > The property is compatible with i915 interpretation of it, whether you > > > like it or not. And that's what Sebastian was referring to. > > > > > > > Eg. what happens if you set the thing to full range for RGB (which you > > > > must on many broken monitors), and then the kernel automagically > > > > switches to YCbCr (for whatever reason) but the monitor doesn't > > > > support full range YCbCr? Answer: you get crap output. > > > > > > And that part is just moving goalposts. > > > > No. Allowing users to get correct colors with broken displays > > is the sole reason why this property even exists. > > HDMI 1.4, Section 6.6 - Video Quantization Ranges: > > If the sink’s EDID declares a selectable YCC Quantization Range > (QY=1), then it shall expect limited range pixel values if it receives > AVI YQ=0 and it shall expect full range pixel values if it receives > AVI YQ=1. For other values of YQ, the sink shall expect pixel values > with the default range for the transmitted video format. > > So, the only concern you have is if the EDID has QY set to 1 but the > monitor doesn't actually support it? If so, could we qualify the monitor > as a "broken display" and thus would require that property to apply to > YUV too? Sinks that declare a selectable quantization range are not the problem, or at least I don't recall ever seeing one that lied about that. The problem is the sinks that don't have selectable quantization range, and which implement the default rules incorrectly. The only way to get correct colors on those is for the user to override the quantization range manually. Typically TVs get it mostly right (though I have at least one that also expects limited range for 640x480 which is not correct), and many (perhaps even most?) computer displays get it wrong (as in they always assume RGB to be full range). We could in theory quirk those, but the quirk list would be enormous, and fragile to maintain because the user can also shoot themselves in the foot here by frobbing with the "black level"/etc. settings on the display itself. So we'd surely end up with lots of false positives on the quirk list.
On Thu, Feb 22, 2024 at 02:58:45PM +0200, Ville Syrjälä wrote: > On Thu, Feb 22, 2024 at 11:54:04AM +0100, Maxime Ripard wrote: > > On Mon, Feb 19, 2024 at 03:01:44PM +0100, Sebastian Wick wrote: > > > On Thu, Feb 15, 2024 at 12:00:01PM +0100, Maxime Ripard wrote: > > > > On Mon, Feb 12, 2024 at 06:06:18PM +0100, Sebastian Wick wrote: > > > > > On Mon, Feb 12, 2024 at 05:53:48PM +0100, Maxime Ripard wrote: > > > > > > On Mon, Feb 12, 2024 at 05:49:33PM +0200, Ville Syrjälä wrote: > > > > > > > On Mon, Feb 12, 2024 at 11:01:07AM +0100, Maxime Ripard wrote: > > > > > > > > On Fri, Feb 09, 2024 at 09:34:35PM +0100, Sebastian Wick wrote: > > > > > > > > > On Mon, Feb 05, 2024 at 10:39:38AM +0100, Maxime Ripard wrote: > > > > > > > > > > On Fri, Feb 02, 2024 at 06:37:52PM +0200, Ville Syrjälä wrote: > > > > > > > > > > > On Fri, Feb 02, 2024 at 04:59:30PM +0100, Maxime Ripard wrote: > > > > > > > > > > > > On Fri, Feb 02, 2024 at 05:40:47PM +0200, Ville Syrjälä wrote: > > > > > > > > > > > > > On Fri, Feb 02, 2024 at 02:01:39PM +0100, Maxime Ripard wrote: > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Jan 15, 2024 at 03:37:20PM +0100, Sebastian Wick wrote: > > > > > > > > > > > > > > > > > /** > > > > > > > > > > > > > > > > > * DOC: HDMI connector properties > > > > > > > > > > > > > > > > > * > > > > > > > > > > > > > > > > > + * Broadcast RGB > > > > > > > > > > > > > > > > > + * Indicates the RGB Quantization Range (Full vs Limited) used. > > > > > > > > > > > > > > > > > + * Infoframes will be generated according to that value. > > > > > > > > > > > > > > > > > + * > > > > > > > > > > > > > > > > > + * 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. Unlike the name suggests, > > > > > > > > > > > > > > > > > + * this works for any number of bits-per-component. > > > > > > > > > > > > > > > > > + * > > > > > > > > > > > > > > > > > + * Drivers can set up this property by calling > > > > > > > > > > > > > > > > > + * drm_connector_attach_broadcast_rgb_property(). > > > > > > > > > > > > > > > > > + * > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This is a good time to document this in more detail. There might be two > > > > > > > > > > > > > > > > different things being affected: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. The signalling (InfoFrame/SDP/...) > > > > > > > > > > > > > > > > 2. The color pipeline processing > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > All values of Broadcast RGB always affect the color pipeline processing > > > > > > > > > > > > > > > > such that a full-range input to the CRTC is converted to either full- or > > > > > > > > > > > > > > > > limited-range, depending on what the monitor is supposed to accept. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > When automatic is selected, does that mean that there is no signalling, > > > > > > > > > > > > > > > > or that the signalling matches what the monitor is supposed to accept > > > > > > > > > > > > > > > > according to the spec? Also, is this really HDMI specific? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > When full or limited is selected and the monitor doesn't support the > > > > > > > > > > > > > > > > signalling, what happens? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Forgot to mention: user-space still has no control over RGB vs YCbCr on > > > > > > > > > > > > > > > the cable, so is this only affecting RGB? If not, how does it affect > > > > > > > > > > > > > > > YCbCr? > > > > > > > > > > > > > > > > > > > > > > > > > > > > So I dug a bit into both the i915 and vc4 drivers, and it looks like if > > > > > > > > > > > > > > we're using a YCbCr format, i915 will always use a limited range while > > > > > > > > > > > > > > vc4 will follow the value of the property. > > > > > > > > > > > > > > > > > > > > > > > > > > The property is literally called "Broadcast *RGB*". > > > > > > > > > > > > > That should explain why it's only affecting RGB. > > > > > > > > > > > > > > > > > > > > > > > > Right. And the limited range option is called "Limited 16:235" despite > > > > > > > > > > > > being usable on bpc > 8 bits. Naming errors occurs, and history happens > > > > > > > > > > > > to make names inconsistent too, that's fine and not an argument in > > > > > > > > > > > > itself. > > > > > > > > > > > > > > > > > > > > > > > > > Full range YCbCr is a much rarer beast so we've never bothered > > > > > > > > > > > > > to enable it. > > > > > > > > > > > > > > > > > > > > > > > > vc4 supports it. > > > > > > > > > > > > > > > > > > > > > > Someone implemented it incorrectly then. > > > > > > > > > > > > > > > > > > > > Incorrectly according to what documentation / specification? I'm sorry, > > > > > > > > > > but I find it super ironic that i915 gets to do its own thing, not > > > > > > > > > > document any of it, and when people try to clean things up they get told > > > > > > > > > > that we got it all wrong. > > > > > > > > > > > > > > > > > > FWIW, this was an i915 property and if another driver uses the same > > > > > > > > > property name it must have the same behavior. Yes, it isn't standardized > > > > > > > > > and yes, it's not documented (hence this effort here) but it's still on > > > > > > > > > vc4 to make the property compatible. > > > > > > > > > > > > > > > > How is it not compatible? It's a superset of what i915 provides, but > > > > > > > > it's strictly compatible with it. > > > > > > > > > > > > > > No it is not. > > > > > > > > > > > > The property is compatible with i915 interpretation of it, whether you > > > > > > like it or not. And that's what Sebastian was referring to. > > > > > > > > > > > > > Eg. what happens if you set the thing to full range for RGB (which you > > > > > > > must on many broken monitors), and then the kernel automagically > > > > > > > switches to YCbCr (for whatever reason) but the monitor doesn't > > > > > > > support full range YCbCr? Answer: you get crap output. > > > > > > > > > > > > And that part is just moving goalposts. > > > > > > > > > > But it's really not. > > > > > > > > It really is. This whole discussion started by "well it would be nice if > > > > we made that property handled by the core", and we're now at the "we > > > > need to deal with broken YCbCr displays and i915 opinion about them" > > > > stage. After creating documentation, unit tests, etc. It's the textbook > > > > definition of moving goalposts. And while i915 won't be affected by all > > > > that work. > > > > > > Sorry, but what you're saying is just not true. > > > > > > The Broadcast RGB property is an Intel specific property. > > > > No, it's not. vc4 has been using it for a year now. > > > > > It lacked documentation but the user space contract exists and it > > > based on how i915 implemented it. By changing the semantics you're > > > breaking user space. The documentation has to document the current > > > contract between i915 and user space, not whatever you want the > > > property to be like. > > > > > > I get that you're frustrated that you have to do work while i915 doesn't > > > but none of that is relevant for what the property is and how user space > > > expects it to work. > > > > That's not it, really. I don't mind doing the work. I do mind losing > > functionalities on something that was working fine. And getting the > > blame for something that is, at best, just as much of an documentation > > issue on i915 devs. > > We've had a couple of these cases recently where people have taken > some old property implemented by i915 and implemented it differently > in some other driver. Dunno if the reason was that people didn't try > to understand what i915 is doing and why, or they misundestood it, > or they understood it but decided to ignore it anyway. I can't tell for the other cases, but in this particular case it's definitely in the misunderstanding category. And implying that we didn't even try to understand it, or that we didn't consult anyone when the patches were posted on the ML for months doesn't seem fair either. > Unfortunately having undocumented corners in the uapi is simply > a fact of life when dealing with a >15 year old legacy codebase. > Also there were basically no rules regarding properties in the > past, so everyone just added random properties whenever they > felt like it. > > I think going forward we should probably lay down some extra > ground rules; if an old undocumented uapi is being extended > to cover more than one driver we must first figure out what > the de facto semantics are, and document things properly > before anything else gets done. That sounds reasonable, but you (not you personally, but the i915 team in general) also have to engage, you can't just impose that on everyone else, and then just hope they will figure it out perfectly without your help. I think that whole story is a testament to that. Maxime
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." ,,“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_atomic.c b/drivers/gpu/drm/drm_atomic.c index c31fc0b48c31..1465a7f09a0b 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1142,6 +1142,11 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, drm_printf(p, "\tmax_requested_bpc=%d\n", state->max_requested_bpc); drm_printf(p, "\tcolorspace=%s\n", drm_get_colorspace_name(state->colorspace)); + if (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA || + connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) + drm_printf(p, "\tbroadcast_rgb=%s\n", + drm_hdmi_connector_get_broadcast_rgb_name(state->hdmi.broadcast_rgb)); + if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) if (state->writeback_job && state->writeback_job->fb) drm_printf(p, "\tfb=%d\n", state->writeback_job->fb->base.id); diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index e69c0cc1c6da..10d98620a358 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -583,6 +583,7 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset); void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, struct drm_connector_state *new_state) { + new_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; } EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); @@ -650,6 +651,22 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check); int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, struct drm_atomic_state *state) { + struct drm_connector_state *old_state = + drm_atomic_get_old_connector_state(state, connector); + struct drm_connector_state *new_state = + drm_atomic_get_new_connector_state(state, connector); + + if (old_state->hdmi.broadcast_rgb != new_state->hdmi.broadcast_rgb) { + struct drm_crtc *crtc = new_state->crtc; + struct drm_crtc_state *crtc_state; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + + crtc_state->mode_changed = true; + } + return 0; } EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check); diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index aee4a65d4959..3eb4f4bc8b71 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -818,6 +818,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, state->max_requested_bpc = val; } else if (property == connector->privacy_screen_sw_state_property) { state->privacy_screen_sw_state = val; + } else if (property == connector->broadcast_rgb_property) { + state->hdmi.broadcast_rgb = val; } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val); @@ -901,6 +903,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->max_requested_bpc; } else if (property == connector->privacy_screen_sw_state_property) { *val = state->privacy_screen_sw_state; + } else if (property == connector->broadcast_rgb_property) { + *val = state->hdmi.broadcast_rgb; } 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 d9961cce8245..929b0a911f62 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1183,6 +1183,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 * @@ -1655,6 +1678,26 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); /** * DOC: HDMI connector properties * + * Broadcast RGB + * Indicates the RGB Quantization Range (Full vs Limited) used. + * Infoframes will be generated according to that value. + * + * 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. Unlike the name suggests, + * this works for any number of bits-per-component. + * + * Drivers can set up this property by calling + * drm_connector_attach_broadcast_rgb_property(). + * * 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 @@ -2517,6 +2560,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 the property on. + * + * 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/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile index d6183b3d7688..b29ddfd90596 100644 --- a/drivers/gpu/drm/tests/Makefile +++ b/drivers/gpu/drm/tests/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST_HELPERS) += \ drm_kunit_helpers.o obj-$(CONFIG_DRM_KUNIT_TEST) += \ + drm_atomic_state_helper_test.o \ drm_buddy_test.o \ drm_cmdline_parser_test.o \ drm_connector_test.o \ diff --git a/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c new file mode 100644 index 000000000000..21e6f796ee13 --- /dev/null +++ b/drivers/gpu/drm/tests/drm_atomic_state_helper_test.c @@ -0,0 +1,376 @@ +// SPDX-License-Identifier: GPL-2.0 + +/* + * Kunit test for drm_atomic_state_helper functions + */ + +#include <drm/drm_atomic.h> +#include <drm/drm_atomic_state_helper.h> +#include <drm/drm_atomic_uapi.h> +#include <drm/drm_drv.h> +#include <drm/drm_edid.h> +#include <drm/drm_connector.h> +#include <drm/drm_fourcc.h> +#include <drm/drm_kunit_helpers.h> +#include <drm/drm_managed.h> +#include <drm/drm_modeset_helper_vtables.h> +#include <drm/drm_probe_helper.h> + +#include <drm/drm_print.h> +#include "../drm_crtc_internal.h" + +#include <kunit/test.h> + +#include "drm_kunit_edid.h" + +struct drm_atomic_helper_connector_hdmi_priv { + struct drm_device drm; + struct drm_plane *plane; + struct drm_crtc *crtc; + struct drm_encoder encoder; + struct drm_connector connector; + + const char *current_edid; + size_t current_edid_len; +}; + +#define connector_to_priv(c) \ + container_of_const(c, struct drm_atomic_helper_connector_hdmi_priv, connector) + +static struct drm_display_mode *find_preferred_mode(struct drm_connector *connector) +{ + struct drm_device *drm = connector->dev; + struct drm_display_mode *mode, *preferred; + + mutex_lock(&drm->mode_config.mutex); + preferred = list_first_entry(&connector->modes, struct drm_display_mode, head); + list_for_each_entry(mode, &connector->modes, head) + if (mode->type & DRM_MODE_TYPE_PREFERRED) + preferred = mode; + mutex_unlock(&drm->mode_config.mutex); + + return preferred; +} + +static int light_up_connector(struct kunit *test, + struct drm_device *drm, + struct drm_crtc *crtc, + struct drm_connector *connector, + struct drm_display_mode *mode, + struct drm_modeset_acquire_ctx *ctx) +{ + struct drm_atomic_state *state; + struct drm_connector_state *conn_state; + struct drm_crtc_state *crtc_state; + int ret; + + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); + + conn_state = drm_atomic_get_connector_state(state, connector); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state); + + ret = drm_atomic_set_crtc_for_connector(conn_state, crtc); + KUNIT_EXPECT_EQ(test, ret, 0); + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); + + ret = drm_atomic_set_mode_for_crtc(crtc_state, mode); + KUNIT_EXPECT_EQ(test, ret, 0); + + crtc_state->enable = true; + crtc_state->active = true; + + ret = drm_atomic_commit(state); + KUNIT_ASSERT_EQ(test, ret, 0); + + return 0; +} + +static int set_connector_edid(struct kunit *test, struct drm_connector *connector, + const char *edid, size_t edid_len) +{ + struct drm_atomic_helper_connector_hdmi_priv *priv = + connector_to_priv(connector); + struct drm_device *drm = connector->dev; + int ret; + + priv->current_edid = edid; + priv->current_edid_len = edid_len; + + mutex_lock(&drm->mode_config.mutex); + ret = connector->funcs->fill_modes(connector, 4096, 4096); + mutex_unlock(&drm->mode_config.mutex); + KUNIT_ASSERT_GT(test, ret, 0); + + return 0; +} + +static int dummy_connector_get_modes(struct drm_connector *connector) +{ + struct drm_atomic_helper_connector_hdmi_priv *priv = + connector_to_priv(connector); + const struct drm_edid *edid; + unsigned int num_modes; + + edid = drm_edid_alloc(priv->current_edid, priv->current_edid_len); + if (!edid) + return -EINVAL; + + drm_edid_connector_update(connector, edid); + num_modes = drm_edid_connector_add_modes(connector); + + drm_edid_free(edid); + + return num_modes; +} + +static const struct drm_connector_helper_funcs dummy_connector_helper_funcs = { + .atomic_check = drm_atomic_helper_connector_hdmi_check, + .get_modes = dummy_connector_get_modes, +}; + +static void dummy_hdmi_connector_reset(struct drm_connector *connector) +{ + drm_atomic_helper_connector_reset(connector); + __drm_atomic_helper_connector_hdmi_reset(connector, connector->state); +} + +static const struct drm_connector_funcs dummy_connector_funcs = { + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .fill_modes = drm_helper_probe_single_connector_modes, + .reset = dummy_hdmi_connector_reset, +}; + +static +struct drm_atomic_helper_connector_hdmi_priv * +drm_atomic_helper_connector_hdmi_init(struct kunit *test) +{ + struct drm_atomic_helper_connector_hdmi_priv *priv; + struct drm_connector *conn; + struct drm_encoder *enc; + struct drm_device *drm; + struct device *dev; + int ret; + + dev = drm_kunit_helper_alloc_device(test); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); + + priv = drm_kunit_helper_alloc_drm_device(test, dev, + struct drm_atomic_helper_connector_hdmi_priv, drm, + DRIVER_MODESET | DRIVER_ATOMIC); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); + test->priv = priv; + + drm = &priv->drm; + priv->plane = drm_kunit_helper_create_primary_plane(test, drm, + NULL, + NULL, + NULL, 0, + NULL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->plane); + + priv->crtc = drm_kunit_helper_create_crtc(test, drm, + priv->plane, NULL, + NULL, + NULL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->crtc); + + enc = &priv->encoder; + ret = drmm_encoder_init(drm, enc, NULL, DRM_MODE_ENCODER_TMDS, NULL); + KUNIT_ASSERT_EQ(test, ret, 0); + + enc->possible_crtcs = drm_crtc_mask(priv->crtc); + + conn = &priv->connector; + ret = drmm_connector_hdmi_init(drm, conn, + &dummy_connector_funcs, + DRM_MODE_CONNECTOR_HDMIA, + NULL); + KUNIT_ASSERT_EQ(test, ret, 0); + + drm_connector_helper_add(conn, &dummy_connector_helper_funcs); + drm_connector_attach_encoder(conn, enc); + + drm_mode_config_reset(drm); + + ret = set_connector_edid(test, conn, + test_edid_hdmi_1080p_rgb_max_200mhz, + ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz)); + KUNIT_ASSERT_EQ(test, ret, 0); + + return priv; +} + +/* + * Test that if we change the RGB quantization property to a different + * value, we trigger a mode change on the connector's CRTC, which will + * in turn disable/enable the connector. + */ +static void drm_test_check_broadcast_rgb_crtc_mode_changed(struct kunit *test) +{ + struct drm_atomic_helper_connector_hdmi_priv *priv; + struct drm_modeset_acquire_ctx *ctx; + struct drm_connector_state *old_conn_state; + struct drm_connector_state *new_conn_state; + struct drm_crtc_state *crtc_state; + struct drm_atomic_state *state; + struct drm_display_mode *preferred; + struct drm_connector *conn; + struct drm_device *drm; + struct drm_crtc *crtc; + int ret; + + priv = drm_atomic_helper_connector_hdmi_init(test); + KUNIT_ASSERT_NOT_NULL(test, priv); + + ctx = drm_kunit_helper_acquire_ctx_alloc(test); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); + + conn = &priv->connector; + preferred = find_preferred_mode(conn); + KUNIT_ASSERT_NOT_NULL(test, preferred); + + drm = &priv->drm; + crtc = priv->crtc; + ret = light_up_connector(test, drm, crtc, conn, preferred, ctx); + KUNIT_ASSERT_EQ(test, ret, 0); + + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); + + new_conn_state = drm_atomic_get_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); + + old_conn_state = drm_atomic_get_old_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); + + new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_FULL; + + KUNIT_ASSERT_NE(test, + old_conn_state->hdmi.broadcast_rgb, + new_conn_state->hdmi.broadcast_rgb); + + ret = drm_atomic_check_only(state); + KUNIT_ASSERT_EQ(test, ret, 0); + + new_conn_state = drm_atomic_get_new_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); + KUNIT_EXPECT_EQ(test, new_conn_state->hdmi.broadcast_rgb, DRM_HDMI_BROADCAST_RGB_FULL); + + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); + KUNIT_EXPECT_TRUE(test, crtc_state->mode_changed); +} + +/* + * Test that if we set the RGB quantization property to the same value, + * we don't trigger a mode change on the connector's CRTC and leave the + * connector unaffected. + */ +static void drm_test_check_broadcast_rgb_crtc_mode_not_changed(struct kunit *test) +{ + struct drm_atomic_helper_connector_hdmi_priv *priv; + struct drm_modeset_acquire_ctx *ctx; + struct drm_connector_state *old_conn_state; + struct drm_connector_state *new_conn_state; + struct drm_crtc_state *crtc_state; + struct drm_atomic_state *state; + struct drm_display_mode *preferred; + struct drm_connector *conn; + struct drm_device *drm; + struct drm_crtc *crtc; + int ret; + + priv = drm_atomic_helper_connector_hdmi_init(test); + KUNIT_ASSERT_NOT_NULL(test, priv); + + ctx = drm_kunit_helper_acquire_ctx_alloc(test); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx); + + conn = &priv->connector; + preferred = find_preferred_mode(conn); + KUNIT_ASSERT_NOT_NULL(test, preferred); + + drm = &priv->drm; + crtc = priv->crtc; + ret = light_up_connector(test, drm, crtc, conn, preferred, ctx); + KUNIT_ASSERT_EQ(test, ret, 0); + + state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state); + + new_conn_state = drm_atomic_get_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); + + old_conn_state = drm_atomic_get_old_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); + + new_conn_state->hdmi.broadcast_rgb = old_conn_state->hdmi.broadcast_rgb; + + ret = drm_atomic_check_only(state); + KUNIT_ASSERT_EQ(test, ret, 0); + + old_conn_state = drm_atomic_get_old_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, old_conn_state); + + new_conn_state = drm_atomic_get_new_connector_state(state, conn); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, new_conn_state); + + KUNIT_EXPECT_EQ(test, + old_conn_state->hdmi.broadcast_rgb, + new_conn_state->hdmi.broadcast_rgb); + + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state); + KUNIT_EXPECT_FALSE(test, crtc_state->mode_changed); +} + +static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = { + KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_changed), + KUNIT_CASE(drm_test_check_broadcast_rgb_crtc_mode_not_changed), + { } +}; + +static struct kunit_suite drm_atomic_helper_connector_hdmi_check_test_suite = { + .name = "drm_atomic_helper_connector_hdmi_check", + .test_cases = drm_atomic_helper_connector_hdmi_check_tests, +}; + +/* + * Test that the value of the Broadcast RGB property out of reset is set + * to auto. + */ +static void drm_test_check_broadcast_rgb_value(struct kunit *test) +{ + struct drm_atomic_helper_connector_hdmi_priv *priv; + struct drm_connector_state *conn_state; + struct drm_connector *conn; + + priv = drm_atomic_helper_connector_hdmi_init(test); + KUNIT_ASSERT_NOT_NULL(test, priv); + + conn = &priv->connector; + conn_state = conn->state; + KUNIT_EXPECT_EQ(test, conn_state->hdmi.broadcast_rgb, DRM_HDMI_BROADCAST_RGB_AUTO); +} + +static struct kunit_case drm_atomic_helper_connector_hdmi_reset_tests[] = { + KUNIT_CASE(drm_test_check_broadcast_rgb_value), + { } +}; + +static struct kunit_suite drm_atomic_helper_connector_hdmi_reset_test_suite = { + .name = "drm_atomic_helper_connector_hdmi_reset", + .test_cases = drm_atomic_helper_connector_hdmi_reset_tests, +}; + +kunit_test_suites( + &drm_atomic_helper_connector_hdmi_check_test_suite, + &drm_atomic_helper_connector_hdmi_reset_test_suite, +); + +MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>"); +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c index 8f070cacab3b..41d33dea30af 100644 --- a/drivers/gpu/drm/tests/drm_connector_test.c +++ b/drivers/gpu/drm/tests/drm_connector_test.c @@ -12,6 +12,8 @@ #include <kunit/test.h> +#include "../drm_crtc_internal.h" + struct drm_connector_init_priv { struct drm_device drm; struct drm_connector connector; @@ -357,10 +359,123 @@ static struct kunit_suite drm_get_tv_mode_from_name_test_suite = { .test_cases = drm_get_tv_mode_from_name_tests, }; +struct drm_hdmi_connector_get_broadcast_rgb_name_test { + unsigned int kind; + const char *expected_name; +}; + +#define BROADCAST_RGB_TEST(_kind, _name) \ + { \ + .kind = _kind, \ + .expected_name = _name, \ + } + +static void drm_test_drm_hdmi_connector_get_broadcast_rgb_name(struct kunit *test) +{ + const struct drm_hdmi_connector_get_broadcast_rgb_name_test *params = + test->param_value; + + KUNIT_EXPECT_STREQ(test, + drm_hdmi_connector_get_broadcast_rgb_name(params->kind), + params->expected_name); +} + +static const +struct drm_hdmi_connector_get_broadcast_rgb_name_test +drm_hdmi_connector_get_broadcast_rgb_name_valid_tests[] = { + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_AUTO, "Automatic"), + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_FULL, "Full"), + BROADCAST_RGB_TEST(DRM_HDMI_BROADCAST_RGB_LIMITED, "Limited 16:235"), +}; + +static void +drm_hdmi_connector_get_broadcast_rgb_name_valid_desc(const struct drm_hdmi_connector_get_broadcast_rgb_name_test *t, + char *desc) +{ + sprintf(desc, "%s", t->expected_name); +} + +KUNIT_ARRAY_PARAM(drm_hdmi_connector_get_broadcast_rgb_name_valid, + drm_hdmi_connector_get_broadcast_rgb_name_valid_tests, + drm_hdmi_connector_get_broadcast_rgb_name_valid_desc); + +static void drm_test_drm_hdmi_connector_get_broadcast_rgb_name_invalid(struct kunit *test) +{ + KUNIT_EXPECT_NULL(test, drm_hdmi_connector_get_broadcast_rgb_name(3)); +}; + +static struct kunit_case drm_hdmi_connector_get_broadcast_rgb_name_tests[] = { + KUNIT_CASE_PARAM(drm_test_drm_hdmi_connector_get_broadcast_rgb_name, + drm_hdmi_connector_get_broadcast_rgb_name_valid_gen_params), + KUNIT_CASE(drm_test_drm_hdmi_connector_get_broadcast_rgb_name_invalid), + { } +}; + +static struct kunit_suite drm_hdmi_connector_get_broadcast_rgb_name_test_suite = { + .name = "drm_hdmi_connector_get_broadcast_rgb_name", + .test_cases = drm_hdmi_connector_get_broadcast_rgb_name_tests, +}; + +static void drm_test_drm_connector_attach_broadcast_rgb_property(struct kunit *test) +{ + struct drm_connector_init_priv *priv = test->priv; + struct drm_connector *connector = &priv->connector; + struct drm_property *prop; + int ret; + + ret = drmm_connector_init(&priv->drm, connector, + &dummy_funcs, + DRM_MODE_CONNECTOR_HDMIA, + &priv->ddc); + KUNIT_ASSERT_EQ(test, ret, 0); + + ret = drm_connector_attach_broadcast_rgb_property(connector); + KUNIT_ASSERT_EQ(test, ret, 0); + + prop = connector->broadcast_rgb_property; + KUNIT_ASSERT_NOT_NULL(test, prop); + KUNIT_EXPECT_NOT_NULL(test, drm_mode_obj_find_prop_id(&connector->base, prop->base.id)); +} + +static void drm_test_drm_connector_attach_broadcast_rgb_property_hdmi_connector(struct kunit *test) +{ + struct drm_connector_init_priv *priv = test->priv; + struct drm_connector *connector = &priv->connector; + struct drm_property *prop; + int ret; + + ret = drmm_connector_hdmi_init(&priv->drm, connector, + &dummy_funcs, + DRM_MODE_CONNECTOR_HDMIA, + &priv->ddc); + KUNIT_EXPECT_EQ(test, ret, 0); + + ret = drm_connector_attach_broadcast_rgb_property(connector); + KUNIT_ASSERT_EQ(test, ret, 0); + + prop = connector->broadcast_rgb_property; + KUNIT_ASSERT_NOT_NULL(test, prop); + KUNIT_EXPECT_NOT_NULL(test, drm_mode_obj_find_prop_id(&connector->base, prop->base.id)); +} + +static struct kunit_case drm_connector_attach_broadcast_rgb_property_tests[] = { + KUNIT_CASE(drm_test_drm_connector_attach_broadcast_rgb_property), + KUNIT_CASE(drm_test_drm_connector_attach_broadcast_rgb_property_hdmi_connector), + { } +}; + +static struct kunit_suite drm_connector_attach_broadcast_rgb_property_test_suite = { + .name = "drm_connector_attach_broadcast_rgb_property", + .init = drm_test_connector_init, + .test_cases = drm_connector_attach_broadcast_rgb_property_tests, +}; + kunit_test_suites( &drmm_connector_hdmi_init_test_suite, &drmm_connector_init_test_suite, - &drm_get_tv_mode_from_name_test_suite + &drm_connector_attach_broadcast_rgb_property_test_suite, + &drm_get_tv_mode_from_name_test_suite, + &drm_hdmi_connector_get_broadcast_rgb_name_test_suite ); MODULE_AUTHOR("Maxime Ripard <maxime@cerno.tech>"); diff --git a/drivers/gpu/drm/tests/drm_kunit_edid.h b/drivers/gpu/drm/tests/drm_kunit_edid.h new file mode 100644 index 000000000000..2bba316de064 --- /dev/null +++ b/drivers/gpu/drm/tests/drm_kunit_edid.h @@ -0,0 +1,106 @@ +#ifndef DRM_KUNIT_EDID_H_ +#define DRM_KUNIT_EDID_H_ + +/* + * edid-decode (hex): + * + * 00 ff ff ff ff ff ff 00 31 d8 2a 00 00 00 00 00 + * 00 21 01 03 81 a0 5a 78 02 00 00 00 00 00 00 00 + * 00 00 00 20 00 00 01 01 01 01 01 01 01 01 01 01 + * 01 01 01 01 01 01 02 3a 80 18 71 38 2d 40 58 2c + * 45 00 40 84 63 00 00 1e 00 00 00 fc 00 54 65 73 + * 74 20 45 44 49 44 0a 20 20 20 00 00 00 fd 00 32 + * 46 1e 46 0f 00 0a 20 20 20 20 20 20 00 00 00 10 + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 01 92 + * + * 02 03 1b 81 e3 05 00 20 41 10 e2 00 4a 6d 03 0c + * 00 12 34 00 28 20 00 00 00 00 00 00 00 00 00 00 + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 + * 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0 + * + * ---------------- + * + * Block 0, Base EDID: + * EDID Structure Version & Revision: 1.3 + * Vendor & Product Identification: + * Manufacturer: LNX + * Model: 42 + * Made in: 2023 + * Basic Display Parameters & Features: + * Digital display + * DFP 1.x compatible TMDS + * Maximum image size: 160 cm x 90 cm + * Gamma: 2.20 + * Monochrome or grayscale display + * First detailed timing is the preferred timing + * Color Characteristics: + * Red : 0.0000, 0.0000 + * Green: 0.0000, 0.0000 + * Blue : 0.0000, 0.0000 + * White: 0.0000, 0.0000 + * Established Timings I & II: + * DMT 0x04: 640x480 59.940476 Hz 4:3 31.469 kHz 25.175000 MHz + * Standard Timings: none + * Detailed Timing Descriptors: + * DTD 1: 1920x1080 60.000000 Hz 16:9 67.500 kHz 148.500000 MHz (1600 mm x 900 mm) + * Hfront 88 Hsync 44 Hback 148 Hpol P + * Vfront 4 Vsync 5 Vback 36 Vpol P + * Display Product Name: 'Test EDID' + * Display Range Limits: + * Monitor ranges (GTF): 50-70 Hz V, 30-70 kHz H, max dotclock 150 MHz + * Dummy Descriptor: + * Extension blocks: 1 + * Checksum: 0x92 + * + * ---------------- + * + * Block 1, CTA-861 Extension Block: + * Revision: 3 + * Underscans IT Video Formats by default + * Native detailed modes: 1 + * Colorimetry Data Block: + * sRGB + * Video Data Block: + * VIC 16: 1920x1080 60.000000 Hz 16:9 67.500 kHz 148.500000 MHz + * Video Capability Data Block: + * YCbCr quantization: No Data + * RGB quantization: Selectable (via AVI Q) + * PT scan behavior: No Data + * IT scan behavior: Always Underscanned + * CE scan behavior: Always Underscanned + * Vendor-Specific Data Block (HDMI), OUI 00-0C-03: + * Source physical address: 1.2.3.4 + * Maximum TMDS clock: 200 MHz + * Extended HDMI video details: + * Checksum: 0xd0 Unused space in Extension Block: 100 bytes + */ +const unsigned char test_edid_hdmi_1080p_rgb_max_200mhz[] = { + 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x31, 0xd8, 0x2a, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x21, 0x01, 0x03, 0x81, 0xa0, 0x5a, 0x78, + 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, + 0x00, 0x00, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, + 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x02, 0x3a, 0x80, 0x18, 0x71, 0x38, + 0x2d, 0x40, 0x58, 0x2c, 0x45, 0x00, 0x40, 0x84, 0x63, 0x00, 0x00, 0x1e, + 0x00, 0x00, 0x00, 0xfc, 0x00, 0x54, 0x65, 0x73, 0x74, 0x20, 0x45, 0x44, + 0x49, 0x44, 0x0a, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0xfd, 0x00, 0x32, + 0x46, 0x00, 0x00, 0xc4, 0x00, 0x0a, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, + 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x41, 0x02, 0x03, 0x1b, 0x81, + 0xe3, 0x05, 0x00, 0x20, 0x41, 0x10, 0xe2, 0x00, 0x4a, 0x6d, 0x03, 0x0c, + 0x00, 0x12, 0x34, 0x00, 0x28, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0xd0 +}; + +#endif // DRM_KUNIT_EDID_H_ diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 000a2a156619..3867a4c01b78 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -368,6 +368,30 @@ enum drm_panel_orientation { DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, }; +/** + * enum drm_hdmi_broadcast_rgb - Broadcast RGB Selection for an HDMI @drm_connector + */ +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, +}; + +const char * +drm_hdmi_connector_get_broadcast_rgb_name(enum drm_hdmi_broadcast_rgb broadcast_rgb); + /** * struct drm_monitor_range_info - Panel's Monitor range in EDID for * &drm_display_info @@ -1037,6 +1061,11 @@ struct drm_connector_state { * @drm_atomic_helper_connector_hdmi_check(). */ struct { + /** + * @broadcast_rgb: Connector property to pass the + * Broadcast RGB selection value. + */ + enum drm_hdmi_broadcast_rgb broadcast_rgb; } hdmi; }; @@ -1706,6 +1735,12 @@ struct drm_connector { */ struct drm_property *privacy_screen_hw_state_property; + /** + * @broadcast_rgb_property: Connector property to set the + * Broadcast RGB selection to output with. + */ + struct drm_property *broadcast_rgb_property; + #define DRM_CONNECTOR_POLL_HPD (1 << 0) #define DRM_CONNECTOR_POLL_CONNECT (1 << 1) #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2) @@ -2026,6 +2061,7 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector, u32 scaling_mode_mask); int drm_connector_attach_vrr_capable_property( struct drm_connector *connector); +int drm_connector_attach_broadcast_rgb_property(struct drm_connector *connector); int drm_connector_attach_colorspace_property(struct drm_connector *connector); int drm_connector_attach_hdr_output_metadata_property(struct drm_connector *connector); bool drm_connector_atomic_hdr_metadata_equal(struct drm_connector_state *old_state,
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 <mripard@kernel.org> --- 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 | 76 +++++ drivers/gpu/drm/tests/Makefile | 1 + .../gpu/drm/tests/drm_atomic_state_helper_test.c | 376 +++++++++++++++++++++ drivers/gpu/drm/tests/drm_connector_test.c | 117 ++++++- drivers/gpu/drm/tests/drm_kunit_edid.h | 106 ++++++ include/drm/drm_connector.h | 36 ++ 10 files changed, 737 insertions(+), 2 deletions(-)