[PATCH RFC] drm/bridge: connector: Support the colorspace property for HDMI

[PATCH RFC] drm/bridge: connector: Support the colorspace property for HDMI

[Jan Claußen]

Bridge connectors for HDMI do not expose the colorspace property,
so userspace cannot signal the colorimetry e.g. BT.2020 for HDR.
KWin requires the property before it offers the HDR toggle.

Attach the property for HDMI bridge connectors, next to the other HDMI
properties set up by drmm_connector_hdmi_init().

Force a modeset when the property changes so the new colorimetry
reaches the sink, as vc4 does in its connector atomic_check.

After toggling HDR on, modetest shows that the correct BT.2020 color
space is loaded. After toggling it off, it loads the default color space
again.

Tested in Plasma 6.7.5 on a Radxa Rock 5B (RK3588) with an LG OLED55B87LC.

Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Jan Claußen <jan.claussen10@web.de>
---
 .../gpu/drm/display/drm_bridge_connector.c    | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 81f3e26f6fdf..0639fe094b10 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -412,6 +412,20 @@ drm_bridge_connector_mode_valid(struct drm_connector *connector,
 static int drm_bridge_connector_atomic_check(struct drm_connector *connector,
 					     struct drm_atomic_commit *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);
+	struct drm_crtc_state *crtc_state;
+
+	if (old_state->colorspace != new_state->colorspace) {
+		crtc_state = drm_atomic_get_crtc_state(state, new_state->crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+
+		crtc_state->mode_changed = true;
+	}
+
 	struct drm_bridge_connector *bridge_connector =
 		to_drm_bridge_connector(connector);
 
@@ -1079,6 +1093,12 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 					       connector_type, ddc);
 		if (ret)
 			return ERR_PTR(ret);
+
+		ret = drm_mode_create_hdmi_colorspace_property(connector, 0);
+		if (ret)
+			return ERR_PTR(ret);
+
+		drm_connector_attach_colorspace_property(connector);
 	} else {
 		ret = drmm_connector_init(drm, connector,
 					  &drm_bridge_connector_funcs,
-- 

Hi,

after testing the patch in the link below,

https://patchew.org/linux/20260805153416.8459-1-royalnet026@gmail.com/

which attaches the bridge connector inside the bind function of the
Rockchip QP driver glue code, I realized that the colors looked washed out,
when the HDR mode was enabled in KDE Plasma. I found a supposed solution,
which was comparing the old and new colorspace enums of the connector state
in the drm_atomic_helper_connector_hdmi_check().

Before hastily sending the patch, I consulted maintainers in the #dri-devel
channel and Javier pointed out that the attachment of the colorspace should
not be done in the glue code of the Rockchip driver since it merely uses the
DRM bridge framework. Furthermore he said the modeset force should not be done
drm_atomic_helper_connector_hdmi_check(), since it was meant to only check
conn_state->hdmi, which the colorspace doesn't belong to. He suggested to
rather implement them in the drm_bridge_connector_atomic_check() and
drm_bridge_connector_init() respectively. This is what patch does.

This is my first patch to the upstream kernel, so I am sending it as RFC.
I found the solution with the assistance of AI tools, but I do have a basic
understanding of the kernel's driver model and API. While I made sure to
carefully read the rules on how to do this correctly, I probably still
haven't done everything right. I am happy to make further changes if necessary. 

If this is however done correctly, it should probably supersede the patch
mentioned above.

Best regards,
Jan

2.56.0

Re: [PATCH RFC] drm/bridge: connector: Support the colorspace property for HDMI

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: NULL pointer dereference in drm_bridge_connector_atomic_check() on disabled connector
- [Low] drm: mixed declarations and code in drm_bridge_connector_atomic_check()
--

commit bf56f41939cb53804040ce55667f5edb2cca09b7
Author: Jan Claußen <jan.claussen10@web.de>

drm/bridge: connector: Support the colorspace property for HDMI

This commit adds support for the colorspace property to HDMI bridge
connectors, allowing userspace to signal colorimetry like BT.2020.
It attaches the property during initialization and forces a modeset
when the colorspace changes.

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 81f3e26f6fdf5..0639fe094b108 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -412,6 +412,20 @@ drm_bridge_connector_mode_valid(struct drm_connector *connector,
>  static int drm_bridge_connector_atomic_check(struct drm_connector *connector,
>  					     struct drm_atomic_commit *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);
> +	struct drm_crtc_state *crtc_state;
> +
> +	if (old_state->colorspace != new_state->colorspace) {
> +		crtc_state = drm_atomic_get_crtc_state(state, new_state->crtc);

[Severity: High]
Can this cause a NULL pointer dereference if the connector is disabled?

Userspace can submit an atomic commit that modifies the colorspace property
on a disconnected or disabled connector. It might also concurrently disable
the connector while modifying the property. In these scenarios,
new_state->crtc will be NULL.

If new_state->crtc is NULL, drm_atomic_get_crtc_state() immediately passes
the pointer to drm_crtc_index(), which dereferences it:

drm_atomic_get_crtc_state(state, crtc) {
    int ret, index = drm_crtc_index(crtc);
    ...

Should the CRTC retrieval be guarded by an if (new_state->crtc) check to
prevent crashes during such commits?

> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		crtc_state->mode_changed = true;
> +	}
> +
>  	struct drm_bridge_connector *bridge_connector =
>  		to_drm_bridge_connector(connector);

[Severity: Low]
This isn't a bug, but does this placement mix declarations and executable
code?

The newly inserted if block pushes the declaration of bridge_connector down,
placing it after executable code. While supported in newer C standards, this
violates the Linux kernel coding style and will trigger warnings from
checkpatch.pl.

Could this declaration be moved to the top of the block?

>  
>  	if (bridge_connector->bridge_hdmi)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011132024.2833-1-jan.claussen10@web.de?part=1