1. Patchew
  2. linux
  3. [PATCH v6 00/10] drm/display: generic HDMI CEC helpers
  4. View patch

[PATCH v6 08/10] drm/display: bridge-connector: hook in CEC notifier support

Dmitry Baryshkov posted 10 patches 7 months ago
[PATCH v6 08/10] drm/display: bridge-connector: hook in CEC notifier support
Posted by Dmitry Baryshkov 7 months ago 
Allow HDMI DRM bridges to create CEC notifier. Physical address is
handled automatically by drm_atomic_helper_connector_hdmi_hotplug()
being called from .detect() path.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 24 ++++++++++++++++++++++++
 include/drm/drm_bridge.h                       | 11 +++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 381a0f9d4259bf9f72d3a292b7dcc82e45c61bae..0377dcd691a871643710d667248b05f8eb9e84d6 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -20,6 +20,7 @@
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/display/drm_hdmi_audio_helper.h>
+#include <drm/display/drm_hdmi_cec_helper.h>
 #include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 
@@ -113,6 +114,13 @@ struct drm_bridge_connector {
 	 * &DRM_BRIDGE_OP_DP_AUDIO).
 	 */
 	struct drm_bridge *bridge_dp_audio;
+	/**
+	 * @bridge_hdmi_cec:
+	 *
+	 * The bridge in the chain that implements CEC support, if any (see
+	 * DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER).
+	 */
+	struct drm_bridge *bridge_hdmi_cec;
 };
 
 #define to_drm_bridge_connector(x) \
@@ -662,6 +670,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			bridge_connector->bridge_dp_audio = bridge;
 		}
 
+		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
+			if (bridge_connector->bridge_hdmi_cec)
+				return ERR_PTR(-EBUSY);
+
+			bridge_connector->bridge_hdmi_cec = bridge;
+		}
+
 		if (!drm_bridge_get_next_bridge(bridge))
 			connector_type = bridge->type;
 
@@ -724,6 +739,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 			return ERR_PTR(ret);
 	}
 
+	if (bridge_connector->bridge_hdmi_cec &&
+	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
+		ret = drmm_connector_hdmi_cec_notifier_register(connector,
+								NULL,
+								bridge->hdmi_cec_dev);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
 	drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
 
 	if (bridge_connector->bridge_hpd)
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index db0d374d863b0b1f774d395743f1e29bb84e8937..0e5f6a007d536215bd50e11846433290c2060b9c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -907,6 +907,11 @@ enum drm_bridge_ops {
 	 * flag.
 	 */
 	DRM_BRIDGE_OP_DP_AUDIO = BIT(6),
+	/**
+	 * @DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER: The bridge requires CEC notifier
+	 * to be present.
+	 */
+	DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER = BIT(7),
 };
 
 /**
@@ -1003,6 +1008,12 @@ struct drm_bridge {
 	 */
 	unsigned int max_bpc;
 
+	/**
+	 * @hdmi_cec_dev: device to be used as a containing device for CEC
+	 * functions.
+	 */
+	struct device *hdmi_cec_dev;
+
 	/**
 	 * @hdmi_audio_dev: device to be used as a parent for the HDMI Codec if
 	 * either of @DRM_BRIDGE_OP_HDMI_AUDIO or @DRM_BRIDGE_OP_DP_AUDIO is set.

-- 
2.39.5
Re: [PATCH v6 08/10] drm/display: bridge-connector: hook in CEC notifier support
Posted by Luca Ceresoli 5 months ago 
Hi Dmitry,

On Sat, 17 May 2025 04:59:44 +0300
Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:

> Allow HDMI DRM bridges to create CEC notifier. Physical address is
> handled automatically by drm_atomic_helper_connector_hdmi_hotplug()
> being called from .detect() path.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

While working on drm_bridge_connector_init() for unrelated changes I
stumbled upon something in this patch (now committed) which at a
cursory look appears wrong to me.  Even though I still haven't analyzed
in depth I'm reporting it ASAP so you are aware and can either correct
me or confirm there is a bug.

> @@ -662,6 +670,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  			bridge_connector->bridge_dp_audio = bridge;
>  		}
>  
> +		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
> +			if (bridge_connector->bridge_hdmi_cec)
> +				return ERR_PTR(-EBUSY);
> +
> +			bridge_connector->bridge_hdmi_cec = bridge;
> +		}
> +
>  		if (!drm_bridge_get_next_bridge(bridge))
>  			connector_type = bridge->type;
>  
> @@ -724,6 +739,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  			return ERR_PTR(ret);
>  	}
>  
> +	if (bridge_connector->bridge_hdmi_cec &&
> +	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
> +		ret = drmm_connector_hdmi_cec_notifier_register(connector,
> +								NULL,
> +								bridge->hdmi_cec_dev);

Here you are using the 'bridge' pointer, which is the variable used by
the long drm_for_each_bridge_in_chain() loop at the function top. The
same happens in the following patch. I am not sure this is what was
intended, but I don't understand all the details of your series.

In an older patch [0] you had added a similar change, dereferencing the
same 'bridge' variable after the drm_for_each_bridge_in_chain() loop.
That was a bug fixed by a later patch [1].

Superficially this change (as well as patch 9) appears equally wrong.

Basically the value of 'bridge' here could be NULL or
bridge_connector->bridge_hdmi, depending on the
bridge_connector->bridge_hdmi value.

Is this the what you'd expect?

And if it is, what is the correct fix? Maybe:

	ret = drmm_connector_hdmi_cec_notifier_register(connector,
						NULL,
-						bridge->hdmi_cec_dev);
+						bridge_connector->bridge_hdmi_cec->hdmi_cec_dev);

?

Removing unrelated lines, and adding a few comments, the code flow of
the function is:

struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
						struct drm_encoder *encoder)
{
	struct drm_bridge *bridge, *panel_bridge = NULL;

	drm_for_each_bridge_in_chain(encoder, bridge) {
		/* ...lots of stuff... */

		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
			bridge_connector->bridge_hdmi_cec = bridge;
		}
	}

/* now bridge == NULL */

	if (bridge_connector->bridge_hdmi) {
		bridge = bridge_connector->bridge_hdmi;
	} else {
	}

/* now bridge can be NULL or bridge_connector->bridge_hdmi */

	if (bridge_connector->bridge_hdmi_audio ||
	    bridge_connector->bridge_dp_audio) {
		/* this is the code that got changed by [0] ad fixed by [1] */
		if (bridge_connector->bridge_hdmi_audio)
			bridge = bridge_connector->bridge_hdmi_audio;
		else
			bridge = bridge_connector->bridge_dp_audio;

		dev = bridge->hdmi_audio_dev;

		ret = drm_connector_hdmi_audio_init(connector, dev,
						    &drm_bridge_connector_hdmi_audio_funcs,
						    bridge->hdmi_audio_max_i2s_playback_channels,
						    bridge->hdmi_audio_i2s_formats,
						    bridge->hdmi_audio_spdif_playback,
						    bridge->hdmi_audio_dai_port);
	}

/* This is the code added by this patch */
	if (bridge_connector->bridge_hdmi_cec &&
	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
		ret = drmm_connector_hdmi_cec_notifier_register(connector,
								NULL,
								bridge->hdmi_cec_dev);
		if (ret)
			return ERR_PTR(ret);
	}

/* This is the code added by patch 09/10 */
	if (bridge_connector->bridge_hdmi_cec &&
	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
		ret = drmm_connector_hdmi_cec_register(connector,
						       &drm_bridge_connector_hdmi_cec_funcs,
						       bridge->hdmi_cec_adapter_name,
						       bridge->hdmi_cec_available_las,
						       bridge->hdmi_cec_dev);
		if (ret)
			return ERR_PTR(ret);
	}
}

[0] https://cgit.freedesktop.org/drm-misc/commit/?id=231adeda9f67
    -> hunk @@ -641,11 +705,16 @@
[1] https://cgit.freedesktop.org/drm-misc/commit/?id=10357824151262636fda879845f8b64553541106

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v6 08/10] drm/display: bridge-connector: hook in CEC notifier support
Posted by Dmitry Baryshkov 5 months ago 
On Fri, Jul 18, 2025 at 04:41:56PM +0200, Luca Ceresoli wrote:
> Hi Dmitry,
> 
> On Sat, 17 May 2025 04:59:44 +0300
> Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
> 
> > Allow HDMI DRM bridges to create CEC notifier. Physical address is
> > handled automatically by drm_atomic_helper_connector_hdmi_hotplug()
> > being called from .detect() path.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> 
> While working on drm_bridge_connector_init() for unrelated changes I
> stumbled upon something in this patch (now committed) which at a
> cursory look appears wrong to me.  Even though I still haven't analyzed
> in depth I'm reporting it ASAP so you are aware and can either correct
> me or confirm there is a bug.
> 
> > @@ -662,6 +670,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >  			bridge_connector->bridge_dp_audio = bridge;
> >  		}
> >  
> > +		if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
> > +			if (bridge_connector->bridge_hdmi_cec)
> > +				return ERR_PTR(-EBUSY);
> > +
> > +			bridge_connector->bridge_hdmi_cec = bridge;
> > +		}
> > +
> >  		if (!drm_bridge_get_next_bridge(bridge))
> >  			connector_type = bridge->type;
> >  
> > @@ -724,6 +739,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> >  			return ERR_PTR(ret);
> >  	}
> >  
> > +	if (bridge_connector->bridge_hdmi_cec &&
> > +	    bridge_connector->bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {

You are right, here it should be:

bridge = bridge_connector->bridge_hdmi_cec;

(and a similar change in the next if).

I'll send a patch.


> > +		ret = drmm_connector_hdmi_cec_notifier_register(connector,
> > +								NULL,
> > +								bridge->hdmi_cec_dev);
> 
> Here you are using the 'bridge' pointer, which is the variable used by
> the long drm_for_each_bridge_in_chain() loop at the function top. The
> same happens in the following patch. I am not sure this is what was
> intended, but I don't understand all the details of your series.
> 
> In an older patch [0] you had added a similar change, dereferencing the
> same 'bridge' variable after the drm_for_each_bridge_in_chain() loop.
> That was a bug fixed by a later patch [1].
> 
> Superficially this change (as well as patch 9) appears equally wrong.
> 
> Basically the value of 'bridge' here could be NULL or
> bridge_connector->bridge_hdmi, depending on the
> bridge_connector->bridge_hdmi value.
> 
> Is this the what you'd expect?
> 
> And if it is, what is the correct fix? Maybe:
> 
> 	ret = drmm_connector_hdmi_cec_notifier_register(connector,
> 						NULL,
> -						bridge->hdmi_cec_dev);
> +						bridge_connector->bridge_hdmi_cec->hdmi_cec_dev);
> 
> ?
> 

[skipped]

> 
> [0] https://cgit.freedesktop.org/drm-misc/commit/?id=231adeda9f67
>     -> hunk @@ -641,11 +705,16 @@
> [1] https://cgit.freedesktop.org/drm-misc/commit/?id=10357824151262636fda879845f8b64553541106
> 
> Best regards,
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
With best wishes
Dmitry

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.