Hi Dmitry, On 2025/5/28 4:25, Dmitry Baryshkov wrote:
On Tue, May 27, 2025 at 04:14:47PM +0800, Chaoyi Chen wrote:From: Chaoyi Chen <[email protected]>Convert it to drm bridge driver, it will be convenient for us to migrate the connector part to the display driver later. Considering that some code depend on the connector, the following changes have been made: - Only process edid in &drm_bridge_funcs.edid_read(), so no need to store additional edid info. - Now cdn_dp_get_sink_capability() only focused on reading DPCD_REV. - Update bpc info in cdn_dp_bridge_atomic_enable() instead of cdn_dp_encoder_mode_set(). Actually, the bpc data will be used in cdn_dp_bridge_atomic_enable(). - Switch to use DRM_BRIDGE_OP_DP_AUDIO helpers. This patch also convert to use devm_drm_bridge_alloc() API. Tested with RK3399 EVB IND board. Signed-off-by: Chaoyi Chen <[email protected]> --- Changes in v3: - Link to V2: https://lore.kernel.org/all/[email protected]/ - Switch to use DRM_BRIDGE_OP_DP_AUDIO helpers - Remove the dependency for connector - Remove the extra stored edid - Code cleanup Changes in v2: - Link to V1: https://lore.kernel.org/all/[email protected]/ - Use drm_atomic_get_new_connector_for_encoder() to get connector - Convert to use devm_drm_bridge_alloc() API - Fix typo: cdn_dp_connector_edid_read -> cdn_dp_bridge_edid_read drivers/gpu/drm/rockchip/cdn-dp-core.c | 279 ++++++++++--------------- drivers/gpu/drm/rockchip/cdn-dp-core.h | 9 +- 2 files changed, 110 insertions(+), 178 deletions(-)@@ -595,16 +546,41 @@ static bool cdn_dp_check_link_status(struct cdn_dp_device *dp) static void cdn_dp_audio_handle_plugged_change(struct cdn_dp_device *dp, bool plugged) { - if (dp->codec_dev) - dp->plugged_cb(dp->codec_dev, plugged); + if (dp->sink_has_audio) + drm_connector_hdmi_audio_plugged_notify(dp->connector, plugged);I'd say, notify always and let userspace figure it out via the ELD. Then you shouldn't need sink_has_audio. This would match the behaviour of HDMI drivers.
Oh, I find that there are similar usages in qcom msm driver. Is there any more progress?
}[...]@@ -705,8 +681,6 @@ static int cdn_dp_encoder_atomic_check(struct drm_encoder *encoder,static const struct drm_encoder_helper_funcs cdn_dp_encoder_helper_funcs = {.mode_set = cdn_dp_encoder_mode_set, - .enable = cdn_dp_encoder_enable, - .disable = cdn_dp_encoder_disable, .atomic_check = cdn_dp_encoder_atomic_check,Nit: for the future cleanup, it should probably be possible to get rid of these encoder ops too by moving them to the bridge ops.
Interesting, have these patches been submitted upstream yet?
};[...]@@ -1006,7 +947,8 @@ static void cdn_dp_pd_event_work(struct work_struct *work)out:mutex_unlock(&dp->lock); - drm_connector_helper_hpd_irq_event(&dp->connector); + if (dp->connector) + drm_connector_helper_hpd_irq_event(dp->connector);drm_bridge_hpd_notify(). I think then you don't need dp->connector.
That make sense! Will fix in v4.
}static int cdn_dp_pd_event(struct notifier_block *nb,@@ -1062,26 +1004,35 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)drm_encoder_helper_add(encoder, &cdn_dp_encoder_helper_funcs); - connector = &dp->connector;- connector->polled = DRM_CONNECTOR_POLL_HPD; - connector->dpms = DRM_MODE_DPMS_OFF; - - ret = drm_connector_init(drm_dev, connector, - &cdn_dp_atomic_connector_funcs, - DRM_MODE_CONNECTOR_DisplayPort); - if (ret) { - DRM_ERROR("failed to initialize connector with drm\n"); - goto err_free_encoder; - } + 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); + if (ret) + return ret;- drm_connector_helper_add(connector, &cdn_dp_connector_helper_funcs);+ ret = drm_bridge_attach(encoder, &dp->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); + if (ret) + return ret;- ret = drm_connector_attach_encoder(connector, encoder);- if (ret) { - DRM_ERROR("failed to attach connector and encoder\n"); - goto err_free_connector; + connector = drm_bridge_connector_init(drm_dev, encoder); + if (IS_ERR(connector)) { + ret = PTR_ERR(connector); + dev_err(dp->dev, "failed to init bridge connector: %d\n", ret); + return ret; }+ drm_connector_attach_encoder(connector, encoder);+ for (i = 0; i < dp->ports; i++) { port = dp->port[i];@@ -1101,30 +1052,19 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)schedule_work(&dp->event_work);return 0;- -err_free_connector: - drm_connector_cleanup(connector); -err_free_encoder: - drm_encoder_cleanup(encoder); - return ret; }static void cdn_dp_unbind(struct device *dev, struct device *master, void *data){ struct cdn_dp_device *dp = dev_get_drvdata(dev); struct drm_encoder *encoder = &dp->encoder.encoder; - struct drm_connector *connector = &dp->connector;cancel_work_sync(&dp->event_work);- cdn_dp_encoder_disable(encoder); encoder->funcs->destroy(encoder); - connector->funcs->destroy(connector);pm_runtime_disable(dev);if (dp->fw_loaded) release_firmware(dp->fw); - drm_edid_free(dp->drm_edid); - dp->drm_edid = NULL; }static const struct component_ops cdn_dp_component_ops = {@@ -1171,9 +1111,10 @@ static int cdn_dp_probe(struct platform_device *pdev) int ret; int i;- dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);- if (!dp) - return -ENOMEM; + dp = devm_drm_bridge_alloc(dev, struct cdn_dp_device, bridge, + &cdn_dp_bridge_funcs); + if (IS_ERR(dp)) + return PTR_ERR(dp); dp->dev = dev;match = of_match_node(cdn_dp_dt_ids, pdev->dev.of_node);@@ -1209,19 +1150,11 @@ static int cdn_dp_probe(struct platform_device *pdev) mutex_init(&dp->lock); dev_set_drvdata(dev, dp);- ret = cdn_dp_audio_codec_init(dp, dev);- if (ret) - return ret; - ret = component_add(dev, &cdn_dp_component_ops); if (ret) - goto err_audio_deinit; + return ret;return 0;- -err_audio_deinit: - platform_device_unregister(dp->audio_pdev); - return ret; }static void cdn_dp_remove(struct platform_device *pdev)diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.h b/drivers/gpu/drm/rockchip/cdn-dp-core.h index 17498f576ce7..6aa3ce489382 100644 --- a/drivers/gpu/drm/rockchip/cdn-dp-core.h +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.h @@ -8,6 +8,8 @@ #define _CDN_DP_CORE_H#include <drm/display/drm_dp_helper.h>+#include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h>This include can go to the C file instead of the header.
Okay, will fix in v4.
#include <drm/drm_panel.h> #include <drm/drm_probe_helper.h> #include <sound/hdmi-codec.h> @@ -65,12 +67,12 @@ struct cdn_dp_port { struct cdn_dp_device { struct device *dev; struct drm_device *drm_dev; - struct drm_connector connector; + struct drm_bridge bridge; + struct drm_connector *connector; struct rockchip_encoder encoder; struct drm_display_mode mode; struct platform_device *audio_pdev; struct work_struct event_work; - const struct drm_edid *drm_edid;struct mutex lock;bool connected; @@ -102,8 +104,5 @@ struct cdn_dp_device {u8 dpcd[DP_RECEIVER_CAP_SIZE];bool sink_has_audio; - - hdmi_codec_plugged_cb plugged_cb; - struct device *codec_dev; }; #endif /* _CDN_DP_CORE_H */ -- 2.49.0
