* [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors
2025-09-25 17:19 [PATCH 0/2] drm/bridge: bridge-connector: get/put the stored bridges Luca Ceresoli
@ 2025-09-25 17:19 ` Luca Ceresoli
2025-09-25 22:07 ` Dmitry Baryshkov
2025-09-26 7:33 ` Jani Nikula
2025-09-25 17:19 ` [PATCH 2/2] drm/display: bridge-connector: get/put the stored bridges Luca Ceresoli
1 sibling, 2 replies; 6+ messages in thread
From: Luca Ceresoli @ 2025-09-25 17:19 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Dmitry Baryshkov
Cc: Hui Pu, Thomas Petazzoni, dri-devel, linux-kernel, Luca Ceresoli
Some code is going to need connector-specific cleanup actions (namely
drm_bridge_connector will need to put refcounted bridges).
The .destroy callback is appropriate for this task but it is currently
forbidden by drmm_connector_init(). Relax this limitation and document it.
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
The other obvious approach would be adding a separate .cleanup callback for
the cleanup-only actions. I tried both, they both apparently work, so any
arguments and opinions on which approach is best within the overall DRM
design would be very useful here.
---
drivers/gpu/drm/drm_connector.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 272d6254ea4784e97ca894ec4d463beebf9fdbf0..bd0220513a23afcb096b0c4c4d2b957b81f21ee1 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -513,7 +513,8 @@ static void drm_connector_cleanup_action(struct drm_device *dev,
*
* The connector structure should be allocated with drmm_kzalloc().
*
- * The @drm_connector_funcs.destroy hook must be NULL.
+ * The @drm_connector_funcs.destroy hook must only do connector-specific
+ * cleanups if any is needed, not dealloacte the connector.
*
* Returns:
* Zero on success, error code on failure.
@@ -526,9 +527,6 @@ int drmm_connector_init(struct drm_device *dev,
{
int ret;
- if (drm_WARN_ON(dev, funcs && funcs->destroy))
- return -EINVAL;
-
ret = drm_connector_init_and_add(dev, connector, funcs, connector_type, ddc);
if (ret)
return ret;
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] drm/display: bridge-connector: get/put the stored bridges
2025-09-25 17:19 [PATCH 0/2] drm/bridge: bridge-connector: get/put the stored bridges Luca Ceresoli
2025-09-25 17:19 ` [PATCH 1/2] drm/connector: allow a .destroy callback for drmm-allocated connectors Luca Ceresoli
@ 2025-09-25 17:19 ` Luca Ceresoli
1 sibling, 0 replies; 6+ messages in thread
From: Luca Ceresoli @ 2025-09-25 17:19 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Andrzej Hajda, Neil Armstrong, Robert Foss,
Laurent Pinchart, Jonas Karlman, Jernej Skrabec, Dmitry Baryshkov
Cc: Hui Pu, Thomas Petazzoni, dri-devel, linux-kernel, Luca Ceresoli
drm_bridge_connector_init() takes eight pointers to various bridges, some
of which can be identical, and stores them in pointers inside struct
drm_bridge_connector. Get a reference to each of the taken bridges and put
it on cleanup.
This is tricky because the pointers are currently stored directly in the
drm_bridge_connector in the loop, but there is no nice and clean way to put
those pointers on error return paths. To overcome this, store all pointers
in temporary local variables with a cleanup action, and only on success
copy them into struct drm_bridge_connector (getting another ref while
copying).
Additionally four of these pointers (edid, hpd, detect and modes) can be
written in multiple loop iterations, in order to eventually store the last
matching bridge. However when one of those pointers is overwritten once we
need to put the reference that we got during the previous assignment. Add a
drm_bridge_put() before writing them to handle this.
Finally, there is also a function-local panel_bridge pointer taken inside
the loop and used after the loop. Due to the various return points, use a
__free() cleanup action to ensure it is put on return.
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
drivers/gpu/drm/display/drm_bridge_connector.c | 112 +++++++++++++++++--------
1 file changed, 76 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index a5bdd6c1064399ece6b19560f145b877c9e0680e..4bcebbb8c9935fa55010a9d317f3d9e70b2ff7a3 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -268,11 +268,27 @@ static void drm_bridge_connector_reset(struct drm_connector *connector)
connector->state);
}
+static void drm_bridge_connector_destroy(struct drm_connector *connector)
+{
+ struct drm_bridge_connector *bridge_connector =
+ to_drm_bridge_connector(connector);
+
+ drm_bridge_put(bridge_connector->bridge_edid);
+ drm_bridge_put(bridge_connector->bridge_hpd);
+ drm_bridge_put(bridge_connector->bridge_detect);
+ drm_bridge_put(bridge_connector->bridge_modes);
+ drm_bridge_put(bridge_connector->bridge_hdmi);
+ drm_bridge_put(bridge_connector->bridge_hdmi_audio);
+ drm_bridge_put(bridge_connector->bridge_dp_audio);
+ drm_bridge_put(bridge_connector->bridge_hdmi_cec);
+}
+
static const struct drm_connector_funcs drm_bridge_connector_funcs = {
.reset = drm_bridge_connector_reset,
.detect = drm_bridge_connector_detect,
.force = drm_bridge_connector_force,
.fill_modes = drm_helper_probe_single_connector_modes,
+ .destroy = drm_bridge_connector_destroy,
.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
.debugfs_init = drm_bridge_connector_debugfs_init,
@@ -638,7 +654,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
struct drm_bridge_connector *bridge_connector;
struct drm_connector *connector;
struct i2c_adapter *ddc = NULL;
- struct drm_bridge *panel_bridge = NULL;
+ struct drm_bridge *panel_bridge __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_edid __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_hpd __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_detect __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_modes __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_hdmi __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_hdmi_audio __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_dp_audio __free(drm_bridge_put) = NULL;
+ struct drm_bridge *bridge_hdmi_cec __free(drm_bridge_put) = NULL;
unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
unsigned int max_bpc = 8;
bool support_hdcp = false;
@@ -672,22 +696,30 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
if (!bridge->ycbcr_420_allowed)
connector->ycbcr_420_allowed = false;
- if (bridge->ops & DRM_BRIDGE_OP_EDID)
- bridge_connector->bridge_edid = bridge;
- if (bridge->ops & DRM_BRIDGE_OP_HPD)
- bridge_connector->bridge_hpd = bridge;
- if (bridge->ops & DRM_BRIDGE_OP_DETECT)
- bridge_connector->bridge_detect = bridge;
- if (bridge->ops & DRM_BRIDGE_OP_MODES)
- bridge_connector->bridge_modes = bridge;
+ if (bridge->ops & DRM_BRIDGE_OP_EDID) {
+ drm_bridge_put(bridge_edid);
+ bridge_edid = drm_bridge_get(bridge);
+ }
+ if (bridge->ops & DRM_BRIDGE_OP_HPD) {
+ drm_bridge_put(bridge_hpd);
+ bridge_hpd = drm_bridge_get(bridge);
+ }
+ if (bridge->ops & DRM_BRIDGE_OP_DETECT) {
+ drm_bridge_put(bridge_detect);
+ bridge_detect = drm_bridge_get(bridge);
+ }
+ if (bridge->ops & DRM_BRIDGE_OP_MODES) {
+ drm_bridge_put(bridge_modes);
+ bridge_modes = drm_bridge_get(bridge);
+ }
if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
- if (bridge_connector->bridge_hdmi)
+ if (bridge_hdmi)
return ERR_PTR(-EBUSY);
if (!bridge->funcs->hdmi_write_infoframe ||
!bridge->funcs->hdmi_clear_infoframe)
return ERR_PTR(-EINVAL);
- bridge_connector->bridge_hdmi = bridge;
+ bridge_hdmi = drm_bridge_get(bridge);
if (bridge->supported_formats)
supported_formats = bridge->supported_formats;
@@ -696,10 +728,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
}
if (bridge->ops & DRM_BRIDGE_OP_HDMI_AUDIO) {
- if (bridge_connector->bridge_hdmi_audio)
+ if (bridge_hdmi_audio)
return ERR_PTR(-EBUSY);
- if (bridge_connector->bridge_dp_audio)
+ if (bridge_dp_audio)
return ERR_PTR(-EBUSY);
if (!bridge->hdmi_audio_max_i2s_playback_channels &&
@@ -710,14 +742,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
!bridge->funcs->hdmi_audio_shutdown)
return ERR_PTR(-EINVAL);
- bridge_connector->bridge_hdmi_audio = bridge;
+ bridge_hdmi_audio = drm_bridge_get(bridge);
}
if (bridge->ops & DRM_BRIDGE_OP_DP_AUDIO) {
- if (bridge_connector->bridge_dp_audio)
+ if (bridge_dp_audio)
return ERR_PTR(-EBUSY);
- if (bridge_connector->bridge_hdmi_audio)
+ if (bridge_hdmi_audio)
return ERR_PTR(-EBUSY);
if (!bridge->hdmi_audio_max_i2s_playback_channels &&
@@ -728,7 +760,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
!bridge->funcs->dp_audio_shutdown)
return ERR_PTR(-EINVAL);
- bridge_connector->bridge_dp_audio = bridge;
+ bridge_dp_audio = drm_bridge_get(bridge);
}
if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
@@ -739,10 +771,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
}
if (bridge->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
- if (bridge_connector->bridge_hdmi_cec)
+ if (bridge_hdmi_cec)
return ERR_PTR(-EBUSY);
- bridge_connector->bridge_hdmi_cec = bridge;
+ bridge_hdmi_cec = drm_bridge_get(bridge);
if (!bridge->funcs->hdmi_cec_enable ||
!bridge->funcs->hdmi_cec_log_addr ||
@@ -762,7 +794,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
ddc = bridge->ddc;
if (drm_bridge_is_panel(bridge))
- panel_bridge = bridge;
+ panel_bridge = drm_bridge_get(bridge);
if (bridge->support_hdcp)
support_hdcp = true;
@@ -771,13 +803,13 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
if (connector_type == DRM_MODE_CONNECTOR_Unknown)
return ERR_PTR(-EINVAL);
- if (bridge_connector->bridge_hdmi) {
+ if (bridge_hdmi) {
if (!connector->ycbcr_420_allowed)
supported_formats &= ~BIT(HDMI_COLORSPACE_YUV420);
ret = drmm_connector_hdmi_init(drm, connector,
- bridge_connector->bridge_hdmi->vendor,
- bridge_connector->bridge_hdmi->product,
+ bridge_hdmi->vendor,
+ bridge_hdmi->product,
&drm_bridge_connector_funcs,
&drm_bridge_connector_hdmi_funcs,
connector_type, ddc,
@@ -793,15 +825,14 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
return ERR_PTR(ret);
}
- if (bridge_connector->bridge_hdmi_audio ||
- bridge_connector->bridge_dp_audio) {
+ if (bridge_hdmi_audio || bridge_dp_audio) {
struct device *dev;
struct drm_bridge *bridge;
- if (bridge_connector->bridge_hdmi_audio)
- bridge = bridge_connector->bridge_hdmi_audio;
+ if (bridge_hdmi_audio)
+ bridge = bridge_hdmi_audio;
else
- bridge = bridge_connector->bridge_dp_audio;
+ bridge = bridge_dp_audio;
dev = bridge->hdmi_audio_dev;
@@ -815,9 +846,9 @@ 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) {
- struct drm_bridge *bridge = bridge_connector->bridge_hdmi_cec;
+ if (bridge_hdmi_cec &&
+ bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_NOTIFIER) {
+ struct drm_bridge *bridge = bridge_hdmi_cec;
ret = drmm_connector_hdmi_cec_notifier_register(connector,
NULL,
@@ -826,9 +857,9 @@ 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_ADAPTER) {
- struct drm_bridge *bridge = bridge_connector->bridge_hdmi_cec;
+ if (bridge_hdmi_cec &&
+ bridge_hdmi_cec->ops & DRM_BRIDGE_OP_HDMI_CEC_ADAPTER) {
+ struct drm_bridge *bridge = bridge_hdmi_cec;
ret = drmm_connector_hdmi_cec_register(connector,
&drm_bridge_connector_hdmi_cec_funcs,
@@ -841,9 +872,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
- if (bridge_connector->bridge_hpd)
+ if (bridge_hpd)
connector->polled = DRM_CONNECTOR_POLL_HPD;
- else if (bridge_connector->bridge_detect)
+ else if (bridge_detect)
connector->polled = DRM_CONNECTOR_POLL_CONNECT
| DRM_CONNECTOR_POLL_DISCONNECT;
@@ -854,6 +885,15 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
drm_connector_attach_content_protection_property(connector, true);
+ bridge_connector->bridge_edid = drm_bridge_get(bridge_edid);
+ bridge_connector->bridge_hpd = drm_bridge_get(bridge_hpd);
+ bridge_connector->bridge_detect = drm_bridge_get(bridge_detect);
+ bridge_connector->bridge_modes = drm_bridge_get(bridge_modes);
+ bridge_connector->bridge_hdmi = drm_bridge_get(bridge_hdmi);
+ bridge_connector->bridge_hdmi_audio = drm_bridge_get(bridge_hdmi_audio);
+ bridge_connector->bridge_dp_audio = drm_bridge_get(bridge_dp_audio);
+ bridge_connector->bridge_hdmi_cec = drm_bridge_get(bridge_hdmi_cec);
+
return connector;
}
EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread