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 <mrip...@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#L2150Maxime
signature.asc
Description: PGP signature
