Hi Luca, On 3/3/2026 1:49 AM, Luca Ceresoli wrote: > On Mon Jan 19, 2026 at 8:30 AM CET, Chaoyi Chen wrote: >> From: Chaoyi Chen <[email protected]> >> >> The RK3399 has two USB/DP combo PHY and one CDN-DP controller. And >> the CDN-DP can be switched to output to one of the PHYs. If both ports >> are plugged into DP, DP will select the first port for output. >> >> This patch adds support for multiple bridges, enabling users to flexibly >> select the output port. For each PHY port, a separate encoder and bridge >> are registered. >> >> The change is based on the DRM AUX HPD bridge, rather than the >> extcon approach. This requires the DT to correctly describe the >> connections between the first bridge in bridge chain and DP >> controller. For example, the bridge chain may be like this: >> >> PHY aux birdge -> fsa4480 analog audio switch bridge -> >> onnn,nb7vpq904m USB reminder bridge -> USB-C controller AUX HPD bridge >> >> In this case, the connection relationships among the PHY aux bridge >> and the DP contorller need to be described in DT. >> >> In addition, the cdn_dp_parse_next_bridge_dt() will parses it and >> determines whether to register one or two bridges. >> >> Since there is only one DP controller, only one of the PHY ports can >> output at a time. The key is how to switch between different PHYs, >> which is handled by cdn_dp_switch_port() and cdn_dp_enable(). >> >> There are two cases: >> >> 1. Neither bridge is enabled. In this case, both bridges can >> independently read the EDID, and the PHY port may switch before >> reading the EDID. >> >> 2. One bridge is already enabled. In this case, other bridges are not >> allowed to read the EDID. So we will try to return the cached EDID. >> >> Since the scenario of two ports plug in at the same time is rare, >> I don't have a board which support two TypeC connector to test this. >> Therefore, I tested forced switching on a single PHY port, as well as >> output using a fake PHY port alongside a real PHY port. >> >> Signed-off-by: Chaoyi Chen <[email protected]> >> --- >> >> (no changes since v11) >> >> Changes in v10: >> - Fix refcount usage of drm_bridge. >> - Remove unused cdn_dp_next_bridge type. >> >> Changes in v9: >> - Select DRM_AUX_HPD_BRIDGE when using DP driver. >> >> (no changes since v7) >> >> Changes in v6: >> - Rename some variable names. >> - Attach the DP bridge to the next bridge. >> >> Changes in v5: >> - By parsing the HPD bridge chain, set the connector's of_node to the >> of_node corresponding to the USB-C connector. >> - Return EDID cache when other port is already enabled. >> --- >> >> drivers/gpu/drm/rockchip/Kconfig | 1 + >> drivers/gpu/drm/rockchip/cdn-dp-core.c | 325 ++++++++++++++++++++----- >> drivers/gpu/drm/rockchip/cdn-dp-core.h | 18 +- >> 3 files changed, 287 insertions(+), 57 deletions(-) >> >> diff --git a/drivers/gpu/drm/rockchip/Kconfig >> b/drivers/gpu/drm/rockchip/Kconfig >> index b7b025814e72..10d9f29a3d44 100644 >> --- a/drivers/gpu/drm/rockchip/Kconfig >> +++ b/drivers/gpu/drm/rockchip/Kconfig >> @@ -56,6 +56,7 @@ config ROCKCHIP_CDN_DP >> select DRM_DISPLAY_HELPER >> select DRM_BRIDGE_CONNECTOR >> select DRM_DISPLAY_DP_HELPER >> + select DRM_AUX_HPD_BRIDGE >> help >> This selects support for Rockchip SoC specific extensions >> for the cdn DP driver. If you want to enable Dp on >> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c >> b/drivers/gpu/drm/rockchip/cdn-dp-core.c >> index 1e27301584a4..0bc3d248c266 100644 >> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c >> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c >> @@ -27,16 +27,17 @@ >> #include "cdn-dp-core.h" >> #include "cdn-dp-reg.h" >> >> -static inline struct cdn_dp_device *bridge_to_dp(struct drm_bridge *bridge) >> +static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port >> *prev_port, >> + struct cdn_dp_port *port); >> + >> +static inline struct cdn_dp_bridge *bridge_to_dp_bridge(struct drm_bridge >> *bridge) >> { >> - return container_of(bridge, struct cdn_dp_device, bridge); >> + return container_of(bridge, struct cdn_dp_bridge, bridge); >> } >> >> -static inline struct cdn_dp_device *encoder_to_dp(struct drm_encoder >> *encoder) >> +static inline struct cdn_dp_device *bridge_to_dp(struct drm_bridge *bridge) >> { >> - struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder); >> - >> - return container_of(rkencoder, struct cdn_dp_device, encoder); >> + return bridge_to_dp_bridge(bridge)->parent; >> } >> >> #define GRF_SOC_CON9 0x6224 >> @@ -191,14 +192,27 @@ static int cdn_dp_get_sink_count(struct cdn_dp_device >> *dp, u8 *sink_count) >> static struct cdn_dp_port *cdn_dp_connected_port(struct cdn_dp_device *dp) >> { >> struct cdn_dp_port *port; >> - int i, lanes; >> + int i, lanes[MAX_PHY]; >> >> for (i = 0; i < dp->ports; i++) { >> port = dp->port[i]; >> - lanes = cdn_dp_get_port_lanes(port); >> - if (lanes) >> + lanes[i] = cdn_dp_get_port_lanes(port); >> + if (!dp->next_bridge_valid) >> return port; >> } >> + >> + if (dp->next_bridge_valid) { >> + /* If more than one port is available, pick the last active >> port */ >> + if (dp->active_port > 0 && lanes[dp->active_port]) >> + return dp->port[dp->active_port]; >> + >> + /* If the last active port is not available, pick an available >> port in order */ >> + for (i = 0; i < dp->bridge_count; i++) { >> + if (lanes[i]) >> + return dp->port[i]; >> + } >> + } >> + >> return NULL; >> } >> >> @@ -253,12 +267,45 @@ static const struct drm_edid * >> cdn_dp_bridge_edid_read(struct drm_bridge *bridge, struct drm_connector >> *connector) >> { >> struct cdn_dp_device *dp = bridge_to_dp(bridge); >> - const struct drm_edid *drm_edid; >> + struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge); >> + struct cdn_dp_port *port = dp->port[dp_bridge->id]; >> + struct cdn_dp_port *prev_port; >> + const struct drm_edid *drm_edid = NULL; >> + int i, ret; >> >> mutex_lock(&dp->lock); >> + >> + /* More than one port is available */ >> + if (dp->bridge_count > 1 && !port->phy_enabled) { >> + for (i = 0; i < dp->bridge_count; i++) { >> + /* Another port already enable */ >> + if (dp->bridge_list[i] != dp_bridge && >> dp->bridge_list[i]->enabled) >> + goto get_cache; >> + /* Find already enabled port */ >> + if (dp->port[i]->phy_enabled) >> + prev_port = dp->port[i]; >> + } >> + >> + /* Switch to current port */ >> + if (prev_port) { >> + ret = cdn_dp_switch_port(dp, prev_port, port); >> + if (ret) >> + goto get_cache; >> + } >> + } >> + >> drm_edid = drm_edid_read_custom(connector, cdn_dp_get_edid_block, dp); >> + /* replace edid cache */ >> + if (dp->edid_cache[dp_bridge->id]) >> + drm_edid_free(dp->edid_cache[dp_bridge->id]); >> + dp->edid_cache[dp_bridge->id] = drm_edid_dup(drm_edid); >> + >> mutex_unlock(&dp->lock); >> + return drm_edid; >> >> +get_cache: >> + drm_edid = drm_edid_dup(dp->edid_cache[dp_bridge->id]); >> + mutex_unlock(&dp->lock); >> return drm_edid; >> } >> >> @@ -267,12 +314,13 @@ cdn_dp_bridge_mode_valid(struct drm_bridge *bridge, >> const struct drm_display_info *display_info, >> const struct drm_display_mode *mode) >> { >> + struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge); >> struct cdn_dp_device *dp = bridge_to_dp(bridge); >> u32 requested, actual, rate, sink_max, source_max = 0; >> u8 lanes, bpc; >> >> /* If DP is disconnected, every mode is invalid */ >> - if (!dp->connected) >> + if (!dp_bridge->connected || !dp->connected) >> return MODE_BAD; >> >> switch (display_info->bpc) { >> @@ -550,6 +598,54 @@ static bool cdn_dp_check_link_status(struct >> cdn_dp_device *dp) >> return drm_dp_channel_eq_ok(link_status, min(port->lanes, sink_lanes)); >> } >> >> +static int cdn_dp_switch_port(struct cdn_dp_device *dp, struct cdn_dp_port >> *prev_port, >> + struct cdn_dp_port *port) >> +{ >> + int ret; >> + >> + if (dp->active) >> + return 0; >> + >> + ret = cdn_dp_disable_phy(dp, prev_port); >> + if (ret) >> + goto out; >> + ret = cdn_dp_enable_phy(dp, port); >> + if (ret) >> + goto out; >> + >> + ret = cdn_dp_get_sink_capability(dp); >> + if (ret) { >> + cdn_dp_disable_phy(dp, port); >> + goto out; >> + } >> + >> + dp->active = true; >> + dp->lanes = port->lanes; >> + >> + if (!cdn_dp_check_link_status(dp)) { >> + dev_info(dp->dev, "Connected with sink; re-train link\n"); >> + >> + ret = cdn_dp_train_link(dp); >> + if (ret) { >> + dev_err(dp->dev, "Training link failed: %d\n", ret); >> + goto out; >> + } >> + >> + ret = cdn_dp_set_video_status(dp, CONTROL_VIDEO_IDLE); >> + if (ret) { >> + dev_err(dp->dev, "Failed to idle video %d\n", ret); >> + goto out; >> + } >> + >> + ret = cdn_dp_config_video(dp); >> + if (ret) >> + dev_err(dp->dev, "Failed to configure video: %d\n", >> ret); >> + } >> + >> +out: >> + return ret; >> +} >> + >> static void cdn_dp_display_info_update(struct cdn_dp_device *dp, >> struct drm_display_info *display_info) >> { >> @@ -571,6 +667,7 @@ static void cdn_dp_display_info_update(struct >> cdn_dp_device *dp, >> static void cdn_dp_bridge_atomic_enable(struct drm_bridge *bridge, struct >> drm_atomic_state *state) >> { >> struct cdn_dp_device *dp = bridge_to_dp(bridge); >> + struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge); >> struct drm_connector *connector; >> int ret, val; >> >> @@ -580,7 +677,7 @@ static void cdn_dp_bridge_atomic_enable(struct >> drm_bridge *bridge, struct drm_at >> >> cdn_dp_display_info_update(dp, &connector->display_info); >> >> - ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, >> &dp->encoder.encoder); >> + ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, >> &dp_bridge->encoder.encoder); >> if (ret < 0) { >> DRM_DEV_ERROR(dp->dev, "Could not get vop id, %d", ret); >> return; >> @@ -599,6 +696,9 @@ static void cdn_dp_bridge_atomic_enable(struct >> drm_bridge *bridge, struct drm_at >> >> mutex_lock(&dp->lock); >> >> + if (dp->next_bridge_valid) >> + dp->active_port = dp_bridge->id; >> + >> ret = cdn_dp_enable(dp); >> if (ret) { >> DRM_DEV_ERROR(dp->dev, "Failed to enable bridge %d\n", >> @@ -631,6 +731,7 @@ static void cdn_dp_bridge_atomic_enable(struct >> drm_bridge *bridge, struct drm_at >> goto out; >> } >> >> + dp_bridge->enabled = true; >> out: >> mutex_unlock(&dp->lock); >> } >> @@ -638,9 +739,11 @@ static void cdn_dp_bridge_atomic_enable(struct >> drm_bridge *bridge, struct drm_at >> static void cdn_dp_bridge_atomic_disable(struct drm_bridge *bridge, struct >> drm_atomic_state *state) >> { >> struct cdn_dp_device *dp = bridge_to_dp(bridge); >> + struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge); >> int ret; >> >> mutex_lock(&dp->lock); >> + dp_bridge->enabled = false; >> >> if (dp->active) { >> ret = cdn_dp_disable(dp); >> @@ -827,6 +930,16 @@ static int cdn_dp_audio_mute_stream(struct drm_bridge >> *bridge, >> return ret; >> } >> >> +static void cdn_dp_bridge_hpd_notify(struct drm_bridge *bridge, >> + enum drm_connector_status status) >> +{ >> + struct cdn_dp_bridge *dp_bridge = bridge_to_dp_bridge(bridge); >> + struct cdn_dp_device *dp = bridge_to_dp(bridge); >> + >> + dp->bridge_list[dp_bridge->id]->connected = status == >> connector_status_connected; >> + schedule_work(&dp->event_work); >> +} >> + >> static const struct drm_bridge_funcs cdn_dp_bridge_funcs = { >> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, >> .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, >> @@ -837,6 +950,7 @@ static const struct drm_bridge_funcs cdn_dp_bridge_funcs >> = { >> .atomic_disable = cdn_dp_bridge_atomic_disable, >> .mode_valid = cdn_dp_bridge_mode_valid, >> .mode_set = cdn_dp_bridge_mode_set, >> + .hpd_notify = cdn_dp_bridge_hpd_notify, >> >> .dp_audio_prepare = cdn_dp_audio_prepare, >> .dp_audio_mute_stream = cdn_dp_audio_mute_stream, >> @@ -885,7 +999,8 @@ static void cdn_dp_pd_event_work(struct work_struct >> *work) >> { >> struct cdn_dp_device *dp = container_of(work, struct cdn_dp_device, >> event_work); >> - int ret; >> + bool connected; >> + int i, ret; >> >> mutex_lock(&dp->lock); >> >> @@ -944,9 +1059,12 @@ static void cdn_dp_pd_event_work(struct work_struct >> *work) >> >> out: >> mutex_unlock(&dp->lock); >> - drm_bridge_hpd_notify(&dp->bridge, >> - dp->connected ? connector_status_connected >> - : connector_status_disconnected); >> + for (i = 0; i < dp->bridge_count; i++) { >> + connected = dp->connected && dp->bridge_list[i]->connected; >> + drm_bridge_hpd_notify(&dp->bridge_list[i]->bridge, >> + connected ? connector_status_connected >> + : >> connector_status_disconnected); >> + } >> } >> >> static int cdn_dp_pd_event(struct notifier_block *nb, >> @@ -966,28 +1084,16 @@ static int cdn_dp_pd_event(struct notifier_block *nb, >> return NOTIFY_DONE; >> } >> >> -static int cdn_dp_bind(struct device *dev, struct device *master, void >> *data) >> +static int cdn_bridge_add(struct device *dev, >> + struct drm_bridge *bridge, >> + struct drm_bridge *next_bridge, >> + struct drm_encoder *encoder) >> { >> struct cdn_dp_device *dp = dev_get_drvdata(dev); >> - struct drm_encoder *encoder; >> + struct drm_device *drm_dev = dp->drm_dev; >> + struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL; >> struct drm_connector *connector; >> - struct cdn_dp_port *port; >> - struct drm_device *drm_dev = data; >> - int ret, i; >> - >> - ret = cdn_dp_parse_dt(dp); >> - if (ret < 0) >> - return ret; >> - >> - dp->drm_dev = drm_dev; >> - dp->connected = false; >> - dp->active = false; >> - dp->active_port = -1; >> - dp->fw_loaded = false; >> - >> - INIT_WORK(&dp->event_work, cdn_dp_pd_event_work); >> - >> - encoder = &dp->encoder.encoder; >> + int ret; >> >> encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev, >> dev->of_node); >> @@ -1002,26 +1108,35 @@ static int cdn_dp_bind(struct device *dev, struct >> device *master, void *data) >> >> drm_encoder_helper_add(encoder, &cdn_dp_encoder_helper_funcs); >> >> - dp->bridge.ops = >> - DRM_BRIDGE_OP_DETECT | >> - DRM_BRIDGE_OP_EDID | >> - DRM_BRIDGE_OP_HPD | >> - DRM_BRIDGE_OP_DP_AUDIO; >> - dp->bridge.of_node = dp->dev->of_node; >> - dp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort; >> - dp->bridge.hdmi_audio_dev = dp->dev; >> - dp->bridge.hdmi_audio_max_i2s_playback_channels = 8; >> - dp->bridge.hdmi_audio_spdif_playback = 1; >> - dp->bridge.hdmi_audio_dai_port = -1; >> - >> - ret = devm_drm_bridge_add(dev, &dp->bridge); >> + bridge->ops = >> + DRM_BRIDGE_OP_DETECT | >> + DRM_BRIDGE_OP_EDID | >> + DRM_BRIDGE_OP_HPD | >> + DRM_BRIDGE_OP_DP_AUDIO; >> + bridge->of_node = dp->dev->of_node; >> + bridge->type = DRM_MODE_CONNECTOR_DisplayPort; >> + bridge->hdmi_audio_dev = dp->dev; >> + bridge->hdmi_audio_max_i2s_playback_channels = 8; >> + bridge->hdmi_audio_spdif_playback = 1; >> + bridge->hdmi_audio_dai_port = -1; >> + >> + ret = devm_drm_bridge_add(dev, bridge); >> if (ret) >> return ret; >> >> - ret = drm_bridge_attach(encoder, &dp->bridge, NULL, >> DRM_BRIDGE_ATTACH_NO_CONNECTOR); >> + ret = drm_bridge_attach(encoder, bridge, NULL, >> DRM_BRIDGE_ATTACH_NO_CONNECTOR); >> if (ret) >> return ret; >> >> + if (next_bridge) { >> + ret = drm_bridge_attach(encoder, next_bridge, bridge, >> + DRM_BRIDGE_ATTACH_NO_CONNECTOR); >> + if (ret) >> + return ret; >> + >> + last_bridge = drm_bridge_chain_get_last_bridge(bridge->encoder); >> + } >> + >> connector = drm_bridge_connector_init(drm_dev, encoder); >> if (IS_ERR(connector)) { >> ret = PTR_ERR(connector); >> @@ -1029,8 +1144,100 @@ static int cdn_dp_bind(struct device *dev, struct >> device *master, void *data) >> return ret; >> } >> >> + if (last_bridge) >> + connector->fwnode = >> fwnode_handle_get(of_fwnode_handle(last_bridge->of_node)); >> + >> drm_connector_attach_encoder(connector, encoder); >> >> + return 0; >> +} >> + >> +static int cdn_dp_parse_next_bridge_dt(struct cdn_dp_device *dp) >> +{ >> + struct device_node *np = dp->dev->of_node; >> + struct device_node *port __free(device_node) = >> of_graph_get_port_by_id(np, 1); >> + struct drm_bridge *bridge; >> + int count = 0; >> + int ret = 0; >> + int i; >> + >> + /* If device use extcon, do not use hpd bridge */ >> + for (i = 0; i < dp->ports; i++) { >> + if (dp->port[i]->extcon) { >> + dp->bridge_count = 1; >> + return 0; >> + } >> + } >> + >> + /* One endpoint may correspond to one next bridge. */ >> + for_each_of_graph_port_endpoint(port, dp_ep) { >> + struct device_node *next_bridge_node __free(device_node) = >> + of_graph_get_remote_port_parent(dp_ep); >> + >> + bridge = of_drm_find_bridge(next_bridge_node); >> + if (!bridge) { >> + ret = -EPROBE_DEFER; >> + goto out; >> + } >> + >> + drm_bridge_get(bridge); >> + dp->next_bridge_valid = true; >> + dp->next_bridge_list[count] = bridge; > > Correct, but the drm_bridge_get() slightly far away from the assignement is > a bit misleading. I hadn't seen it initially so I suspected a missing get. > > I suggest to do it in a single statement, for clarity: > > dp->next_bridge_list[count] = bridgedrm_bridge_get(bridge); >
I think the bridgedrm_bridge_get here seems to be a typo? Anyway, thank you very much for your efforts on the bridge lifetime. I will fix this in the next version :) > With that changed, at least for the bridge lifetime aspects, you can add to > the next version my: > > Reviewed-by: Luca Ceresoli <[email protected]> > > Luca > > -- > Luca Ceresoli, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > -- Best, Chaoyi
