Hi Dmitry,
On Sat, 17 May 2025 04:59:44 +0300
Dmitry Baryshkov <dmitry.barysh...@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.barysh...@linaro.org>
> Reviewed-by: Maxime Ripard <mrip...@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@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
