[PATCH v3] drm/rockchip: cdn-dp: Convert to drm bridge
Chaoyi Chen
chaoyi.chen at rock-chips.com
Wed May 28 01:56:56 UTC 2025
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 <chaoyi.chen at rock-chips.com>
>>
>> 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 <chaoyi.chen at rock-chips.com>
>> ---
>>
>> Changes in v3:
>> - Link to V2: https://lore.kernel.org/all/20250523011310.120-1-kernel@airkyi.com/
>> - 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/20250507035148.415-1-kernel@airkyi.com/
>> - 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
>>
More information about the dri-devel
mailing list