[PATCH v4 00/10] Synopsys DisplayPort Controller improvements for Rockchip platforms

[PATCH v4 00/10] Synopsys DisplayPort Controller improvements for Rockchip platforms

[Sebastian Reichel]

This patch series updates the Synopsys Designware DisplayPort bridge
together with the only existing user: The Rockchip RK3576/RK3588:

 1. follow-up bridges (PHY, USB-C connector)
    this is needed to get USB-C DP AltMode working; I've followed the
    Qualcomm driver as reference

 2. runtime PM
    the initial driver has been upstreamed without RPM; add it to
    avoid wasting power when nothing is plugged

 3. audio
    the initial driver has been upstreamed without audio support;
    this adds all missing bits for audio with single stream transport

To properly make use of the bridge code the following USBDP PHY series
is also needed:

https://lore.kernel.org/all/20260714-rockchip-usbdp-cleanup-v13-0-6cb3e769d4c5@collabora.com/

There are two parts, which possibly need some discussion:

 1. I added a dedicated bridge callback for out-of-band hotplug events,
    which is separate from the hotplug_notify. I have a feeling, that
    there might be a better solution, but haven't found it.

 2. The DT binding for audio support works perfectly fine, but is not
    really ready for MST. I don't intend to implement MST right now,
    but the binding should obviously take it into consideration to
    avoid breaking it in the future. I've put different options into
    the patch description and went with the easy one, which is easy to
    test.

Changes in PATCHv4:
 - Link to v3: https://lore.kernel.org/all/20260612-synopsys-dw-dp-improvements-v3-0-dc61e6352508@collabora.com/
 - Drop "Simplify driver data setting" (effectively reverted later on)
 - Add "Fix incorrect resource lifetimes in bind callback", which
   affects all following patches quite a bit (Sashiko)
 - Update "Add follow-up bridge support" to mention that fatal errors
   are fine (Sashiko)
 - Update "Add out-of-band HPD notify handler" to use
   drm_for_each_bridge_in_chain() instead of
   drm_for_each_bridge_in_chain_scoped() required after rebase to
   latest drm-misc-next
 - Add 100ms sleep in hpd detect functions after runtime resume in the
   "Add Runtime PM support" patch (Andy Yan)
 - Keep runtime PM enabled if DP is used without USB-C to avoid missing
   HPD signals in "Add Runtime PM support" patch (Andy Yan)
 - Update patch description of DT binding patch and drop RFC tag
 - Use pm_runtime_resume_and_get() in the audio startup function, which
   allows easy error handling (Sashiko)
 - Avoid updating audio_interface in the audio prepare function when
   an error related early exit happens (Sashiko)
 - Add error handling for clock prepare calls in audio prepare function
   (Sashiko)

Changes in PATCHv3:
- Link to v2: https://lore.kernel.org/r/20260501-synopsys-dw-dp-improvements-v2-0-d7e7f6bac77f@collabora.com
- Collect Reviewed-by on "Simplify driver data setting" patch (Andy Yan)
- Use of_drm_get_bridge_by_endpoint instead of devm_drm_of_get_bridge (Luca Ceresoli)
- Use FIELD_PREP_WM16 instead of FIELD_PREP_WM16_CONST (Chaoyi Chen)
- Rebase to latest drm-misc-next (Dropped Cristian's patches)

Changes in PATCHv2:
 * Link to v1: https://lore.kernel.org/r/20260326-synopsys-dw-dp-improvements-v1-0-501849162290@collabora.com
 * rebased to latest drm-misc-next
 * prepended the Patches from Cristian's cleanup series, as they also
   needed a rebase and should be merged first
   https://lore.kernel.org/dri-devel/20260327-drm-rk-fixes-v3-0-fd2e6900c08c@collabora.com/
 * fix issue with the audio stream (un)prepare handling in last patch

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
Sebastian Reichel (10):
      drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback
      drm/bridge: synopsys: dw-dp: Support MEDIA_BUS_FMT_FIXED
      drm/bridge: synopsys: dw-dp: Add follow-up bridge support
      drm/bridge: Add out-of-band HPD notify handler
      drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD
      drm/rockchip: dw_dp: Implement out-of-band HPD handling
      drm/bridge: synopsys: dw-dp: Add Runtime PM support
      drm/rockchip: dw_dp: Add runtime PM support
      dt-bindings: display: rockchip: dw-dp: Fix sound DAI cells
      drm/bridge: synopsys: dw-dp: Add audio support

 .../bindings/display/rockchip/rockchip,dw-dp.yaml  |   9 +-
 drivers/gpu/drm/bridge/synopsys/dw-dp.c            | 456 ++++++++++++++++++---
 drivers/gpu/drm/display/drm_bridge_connector.c     |   6 +
 drivers/gpu/drm/rockchip/dw_dp-rockchip.c          | 223 ++++++++--
 include/drm/bridge/dw_dp.h                         |  11 +-
 include/drm/drm_bridge.h                           |  14 +
 6 files changed, 629 insertions(+), 90 deletions(-)
---
base-commit: 7296004bd2ceb1284fb557399f0134361f8e34af
change-id: 20260325-synopsys-dw-dp-improvements-7da2e98df1dd

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

[PATCH v4 01/10] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback

[Sebastian Reichel]

Currently the Synopsys DesignWare DP controller driver's bind function
requests lots of resources using device managed functions. These are
free'd on driver removal instead of at unbind time. Fix this discrepancy
by introducing a new probe helper function and moving over most of the
early resource acquisitions.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-dp.c   | 128 +++++++++++++++---------------
 drivers/gpu/drm/rockchip/dw_dp-rockchip.c |  53 ++++++++-----
 include/drm/bridge/dw_dp.h                |   5 +-
 3 files changed, 99 insertions(+), 87 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
index 3445c82e6f50..368e1f85e625 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
@@ -1954,20 +1954,72 @@ static const struct regmap_config dw_dp_regmap_config = {
 	.rd_table = &dw_dp_readable_table,
 };
 
-static void dw_dp_phy_exit(void *data)
+int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
 {
-	struct dw_dp *dp = data;
+	struct device *dev = dp->dev;
+	struct drm_bridge *bridge;
+	int ret;
+
+	bridge = &dp->bridge;
+	bridge->of_node = dev->of_node;
+	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
+	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
+	bridge->ycbcr_420_allowed = true;
+
+	drm_bridge_add(bridge);
+
+	dp->aux.dev = dev;
+	dp->aux.drm_dev = encoder->dev;
+	dp->aux.name = dev_name(dev);
+	dp->aux.transfer = dw_dp_aux_transfer;
+	ret = drm_dp_aux_register(&dp->aux);
+	if (ret) {
+		dev_err_probe(dev, ret, "Aux register failed\n");
+		goto unregister_bridge;
+	}
+
+	ret = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to attach bridge\n");
+		goto unregister_aux;
+	}
+
+	dw_dp_init_hw(dp);
 
+	ret = phy_init(dp->phy);
+	if (ret) {
+		dev_err_probe(dev, ret, "phy init failed\n");
+		goto unregister_aux;
+	}
+
+	enable_irq(dp->irq);
+
+	return 0;
+
+unregister_aux:
+	drm_dp_aux_unregister(&dp->aux);
+
+unregister_bridge:
+	drm_bridge_remove(bridge);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dw_dp_bind);
+
+void dw_dp_unbind(struct dw_dp *dp)
+{
+	disable_irq(dp->irq);
 	phy_exit(dp->phy);
+	drm_dp_aux_unregister(&dp->aux);
+	drm_bridge_remove(&dp->bridge);
 }
+EXPORT_SYMBOL_GPL(dw_dp_unbind);
 
-struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
-			 const struct dw_dp_plat_data *plat_data)
+struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_data *plat_data)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct dw_dp *dp;
-	struct drm_bridge *bridge;
+	struct device *dev = &pdev->dev;
 	void __iomem *res;
+	struct dw_dp *dp;
 	int ret;
 
 	dp = devm_drm_bridge_alloc(dev, struct dw_dp, bridge, &dw_dp_bridge_funcs);
@@ -1976,9 +2028,8 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
 
 	dp->dev = dev;
 	dp->pixel_mode = plat_data->pixel_mode;
-
 	dp->plat_data.max_link_rate = plat_data->max_link_rate;
-	bridge = &dp->bridge;
+
 	mutex_init(&dp->irq_lock);
 	INIT_WORK(&dp->hpd_work, dw_dp_hpd_work);
 	init_completion(&dp->complete);
@@ -2035,69 +2086,20 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
 		return ERR_CAST(dp->rstc);
 	}
 
-	bridge->of_node = dev->of_node;
-	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
-	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
-	bridge->ycbcr_420_allowed = true;
-
-	ret = devm_drm_bridge_add(dev, bridge);
-	if (ret)
-		return ERR_PTR(ret);
-
-	dp->aux.dev = dev;
-	dp->aux.drm_dev = encoder->dev;
-	dp->aux.name = dev_name(dev);
-	dp->aux.transfer = dw_dp_aux_transfer;
-	ret = drm_dp_aux_register(&dp->aux);
-	if (ret) {
-		dev_err_probe(dev, ret, "Aux register failed\n");
-		return ERR_PTR(ret);
-	}
-
-	ret = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
-	if (ret) {
-		dev_err_probe(dev, ret, "Failed to attach bridge\n");
-		goto unregister_aux;
-	}
-
-	dw_dp_init_hw(dp);
-
-	ret = phy_init(dp->phy);
-	if (ret) {
-		dev_err_probe(dev, ret, "phy init failed\n");
-		goto unregister_aux;
-	}
-
-	ret = devm_add_action_or_reset(dev, dw_dp_phy_exit, dp);
-	if (ret)
-		goto unregister_aux;
-
 	dp->irq = platform_get_irq(pdev, 0);
-	if (dp->irq < 0) {
-		ret = dp->irq;
-		goto unregister_aux;
-	}
+	if (dp->irq < 0)
+		return ERR_PTR(dp->irq);
 
 	ret = devm_request_threaded_irq(dev, dp->irq, NULL, dw_dp_irq,
-					IRQF_ONESHOT, dev_name(dev), dp);
+					IRQF_ONESHOT | IRQF_NO_AUTOEN, dev_name(dev), dp);
 	if (ret) {
 		dev_err_probe(dev, ret, "failed to request irq\n");
-		goto unregister_aux;
+		return ERR_PTR(ret);
 	}
 
 	return dp;
-
-unregister_aux:
-	drm_dp_aux_unregister(&dp->aux);
-	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(dw_dp_bind);
-
-void dw_dp_unbind(struct dw_dp *dp)
-{
-	drm_dp_aux_unregister(&dp->aux);
-}
-EXPORT_SYMBOL_GPL(dw_dp_unbind);
+EXPORT_SYMBOL_GPL(dw_dp_probe);
 
 MODULE_AUTHOR("Andy Yan <andyshrk@163.com>");
 MODULE_DESCRIPTION("DW DP Core Library");
diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
index b23efb153c9e..005938dc66c9 100644
--- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
@@ -26,7 +26,7 @@
 struct rockchip_dw_dp {
 	struct dw_dp *base;
 	struct device *dev;
-	struct rockchip_encoder encoder;
+	struct rockchip_encoder *encoder;
 };
 
 static int dw_dp_encoder_atomic_check(struct drm_encoder *encoder,
@@ -73,37 +73,28 @@ static const struct drm_encoder_helper_funcs dw_dp_encoder_helper_funcs = {
 
 static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *data)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	const struct dw_dp_plat_data *plat_data;
+	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
 	struct drm_device *drm_dev = data;
-	struct rockchip_dw_dp *dp;
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	int ret;
 
-	dp = drmm_kzalloc(drm_dev, sizeof(*dp), GFP_KERNEL);
-	if (!dp)
+	dp->encoder = drmm_kzalloc(drm_dev, sizeof(*dp->encoder), GFP_KERNEL);
+	if (!dp->encoder)
 		return -ENOMEM;
 
-	dp->dev = dev;
-	platform_set_drvdata(pdev, dp);
-
-	plat_data = of_device_get_match_data(dev);
-	if (!plat_data)
-		return -ENODEV;
-
-	encoder = &dp->encoder.encoder;
+	encoder = &dp->encoder->encoder;
 	encoder->possible_crtcs = drm_of_find_possible_crtcs(drm_dev, dev->of_node);
-	rockchip_drm_encoder_set_crtc_endpoint_id(&dp->encoder, dev->of_node, 0, 0);
+	rockchip_drm_encoder_set_crtc_endpoint_id(dp->encoder, dev->of_node, 0, 0);
 
 	ret = drmm_encoder_init(drm_dev, encoder, NULL, DRM_MODE_ENCODER_TMDS, NULL);
 	if (ret)
 		return ret;
 	drm_encoder_helper_add(encoder, &dw_dp_encoder_helper_funcs);
 
-	dp->base = dw_dp_bind(dev, encoder, plat_data);
-	if (IS_ERR(dp->base))
-		return PTR_ERR(dp->base);
+	ret = dw_dp_bind(dp->base, encoder);
+	if (ret)
+		return ret;
 
 	connector = drm_bridge_connector_init(drm_dev, encoder);
 	if (IS_ERR(connector)) {
@@ -128,12 +119,30 @@ static const struct component_ops dw_dp_rockchip_component_ops = {
 	.unbind = dw_dp_rockchip_unbind,
 };
 
-static int dw_dp_probe(struct platform_device *pdev)
+static int dw_dp_rockchip_probe(struct platform_device *pdev)
 {
+	const struct dw_dp_plat_data *plat_data;
+	struct device *dev = &pdev->dev;
+	struct rockchip_dw_dp *dp;
+
+	plat_data = of_device_get_match_data(dev);
+	if (!plat_data)
+		return -ENODEV;
+
+	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
+	if (!dp)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, dp);
+	dp->dev = dev;
+
+	dp->base = dw_dp_probe(pdev, plat_data);
+	if (IS_ERR(dp->base))
+		return PTR_ERR(dp->base);
+
 	return component_add(&pdev->dev, &dw_dp_rockchip_component_ops);
 }
 
-static void dw_dp_remove(struct platform_device *pdev)
+static void dw_dp_rockchip_remove(struct platform_device *pdev)
 {
 	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
 }
@@ -161,8 +170,8 @@ static const struct of_device_id dw_dp_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_dp_of_match);
 
 struct platform_driver dw_dp_driver = {
-	.probe	= dw_dp_probe,
-	.remove = dw_dp_remove,
+	.probe	= dw_dp_rockchip_probe,
+	.remove = dw_dp_rockchip_remove,
 	.driver = {
 		.name = "dw-dp",
 		.of_match_table = dw_dp_of_match,
diff --git a/include/drm/bridge/dw_dp.h b/include/drm/bridge/dw_dp.h
index 22105c3e8e4d..a82412a9e769 100644
--- a/include/drm/bridge/dw_dp.h
+++ b/include/drm/bridge/dw_dp.h
@@ -22,7 +22,8 @@ struct dw_dp_plat_data {
 	u8 pixel_mode;
 };
 
-struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
-			 const struct dw_dp_plat_data *plat_data);
+int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder);
 void dw_dp_unbind(struct dw_dp *dp);
+
+struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_data *plat_data);
 #endif /* __DW_DP__ */

-- 
2.53.0

Re: [PATCH v4 01/10] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing `cancel_work_sync(&dp->hpd_work)` during unbind allows the workqueue to execute after the device is unbound and removed, leading to a Use-After-Free.
--

--- Patch [1]: [PATCH v4 01/10] drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback ---
commit ad3d9fe60dfef89b2bbe919cc696921df60e683f
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Fix incorrect resource lifetimes in bind callback

This commit introduces a new probe helper function to acquire resources early
rather than during the bind callback. This aligns resource allocation with
device lifecycle instead of driver bind and unbind time.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
[ ... ]
> +void dw_dp_unbind(struct dw_dp *dp)
> +{
> +	disable_irq(dp->irq);
>  	phy_exit(dp->phy);
> +	drm_dp_aux_unregister(&dp->aux);
> +	drm_bridge_remove(&dp->bridge);
>  }

[Severity: Critical]
This is a pre-existing issue, but could this leave pending work items
running after the device is unbound?

If an HPD interrupt fires right before or during dw_dp_unbind(), the
interrupt handler schedules dp->hpd_work. While disable_irq() prevents
new interrupts, it doesn't cancel already pending work.

When the driver is subsequently removed and the devm-allocated dp structure
is freed, the system workqueue could execute dw_dp_hpd_work() and trigger a
use-after-free on the released memory.

Would it be appropriate to add cancel_work_sync(&dp->hpd_work) here during
the teardown sequence?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=1

[PATCH v4 02/10] drm/bridge: synopsys: dw-dp: Support MEDIA_BUS_FMT_FIXED

[Sebastian Reichel]

Add support for MEDIA_BUS_FMT_FIXED, which is e.g. requested for USB-C
DP chains as the last bridge in the chain (aux-hpd-bridge) does not
implement atomic_get_output_bus_fmts(), which results in the generic
drm_atomic_bridge_chain_select_bus_fmts() code using MEDIA_BUS_FMT_FIXED
instead.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-dp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
index 368e1f85e625..aca6816febe6 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
@@ -1528,6 +1528,7 @@ static int dw_dp_bridge_atomic_check(struct drm_bridge *bridge,
 				     struct drm_connector_state *conn_state)
 {
 	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+	unsigned int out_bus_format = bridge_state->output_bus_cfg.format;
 	struct dw_dp *dp = bridge_to_dp(bridge);
 	struct dw_dp_bridge_state *state;
 	const struct dw_dp_output_format *fmt;
@@ -1538,7 +1539,10 @@ static int dw_dp_bridge_atomic_check(struct drm_bridge *bridge,
 	state = to_dw_dp_bridge_state(bridge_state);
 	mode = &state->mode;
 
-	fmt = dw_dp_get_output_format(bridge_state->output_bus_cfg.format);
+	if (out_bus_format == MEDIA_BUS_FMT_FIXED)
+		out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+	fmt = dw_dp_get_output_format(out_bus_format);
 	if (!fmt)
 		return -EINVAL;
 

-- 
2.53.0

Re: [PATCH v4 02/10] drm/bridge: synopsys: dw-dp: Support MEDIA_BUS_FMT_FIXED

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The bridge configures itself for RGB888 when MEDIA_BUS_FMT_FIXED is requested, but fails to propagate this requirement to the upstream encoder during bus format negotiation.
- [High] Forcing 24 bpp (RGB888) when MEDIA_BUS_FMT_FIXED is requested bypasses DP link bandwidth validation, allowing display modes that exceed the physical link capacity.
--

--- Patch [2]: [PATCH v4 02/10] drm/bridge: synopsys: dw-dp: Support MEDIA_BUS_FMT_FIXED ---
commit 98faee8d93ff4e450a6ec04ce4882b2871508172
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Support MEDIA_BUS_FMT_FIXED

Add support for MEDIA_BUS_FMT_FIXED, which is requested when the downstream
bridge does not implement atomic_get_output_bus_fmts(). This patch coerces
MEDIA_BUS_FMT_FIXED to MEDIA_BUS_FMT_RGB888_1X24 in the atomic check phase.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 368e1f85e6253..aca6816febe67 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1538,7 +1539,10 @@ static int dw_dp_bridge_atomic_check(struct drm_bridge *bridge,
>  	state = to_dw_dp_bridge_state(bridge_state);
>  	mode = &state->mode;
>  
> -	fmt = dw_dp_get_output_format(bridge_state->output_bus_cfg.format);
> +	if (out_bus_format == MEDIA_BUS_FMT_FIXED)
> +		out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;

[Severity: High]
Will this coercion fail to propagate upstream? 

The dw-dp driver still uses the generic helper
drm_atomic_helper_bridge_propagate_bus_fmt for its .atomic_get_input_bus_fmts
hook. This helper propagates the originally requested MEDIA_BUS_FMT_FIXED
format to the upstream encoder, rather than the locally overridden RGB888
format. 

If MEDIA_BUS_FMT_FIXED is propagated to an upstream encoder (like Rockchip
VOP), could it choose an incompatible default output format (like YUV422)
since dw-dp operates strictly in RGB888? Should this driver implement a
custom .atomic_get_input_bus_fmts hook to propagate MEDIA_BUS_FMT_RGB888_1X24
instead?

[Severity: High]
Could forcing MEDIA_BUS_FMT_RGB888_1X24 (24 bpp) here bypass DP link
bandwidth validation? 

When the downstream bridge lacks an .atomic_get_output_bus_fmts() hook, the
DRM core skips the output format validation. Since dw_dp_bridge_mode_valid()
only verifies bandwidth against a minimum possible bpp (e.g., 18 bpp), could
we end up allowing display modes that physically fit at 18 bpp but exceed the
DP link capacity at 24 bpp? 

Should this code call dw_dp_bandwidth_ok() after forcing the format to ensure
the hardware link can actually support 24 bpp?

> +
> +	fmt = dw_dp_get_output_format(out_bus_format);
>  	if (!fmt)
>  		return -EINVAL;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=2

[PATCH v4 03/10] drm/bridge: synopsys: dw-dp: Add follow-up bridge support

[Sebastian Reichel]

Add support to use USB-C connectors with the DP altmode helper code on
devicetree based platforms. To get this working there must be a DRM
bridge chain from the DisplayPort controller to the USB-C connector.
E.g. on Rockchip RK3576:

root@rk3576 # cat /sys/kernel/debug/dri/0/encoder-0/bridges
bridge[0]: dw_dp_bridge_funcs
        refcount: 7
        type: [10] DP
        OF: /soc/dp@27e40000:rockchip,rk3576-dp
        ops: [0x47] detect edid hpd
bridge[1]: drm_aux_bridge_funcs
        refcount: 4
        type: [0] Unknown
        OF: /soc/phy@2b010000:rockchip,rk3576-usbdp-phy
        ops: [0x0]
bridge[2]: drm_aux_hpd_bridge_funcs
        refcount: 5
        type: [10] DP
        OF: /soc/i2c@2ac50000/typec-portc@22/connector:usb-c-connector
        ops: [0x4] hpd

It's fine to fatally error out when there is no follow-up bridge
as the Rockchip Designware Displayport controller is the only
user of the bridge helper and has the port marked as required
in its binding.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-dp.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
index aca6816febe6..0e8ecd73a000 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
@@ -329,6 +329,8 @@ struct dw_dp {
 	struct dw_dp_plat_data plat_data;
 	u8 pixel_mode;
 
+	struct drm_bridge *next_bridge;
+
 	DECLARE_BITMAP(sdp_reg_bank, SDP_REG_BANK_SIZE);
 };
 
@@ -1988,18 +1990,35 @@ int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
 		goto unregister_aux;
 	}
 
+	dp->next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, 0);
+	if (IS_ERR(dp->next_bridge)) {
+		ret = PTR_ERR(dp->next_bridge);
+		dev_err_probe(dev, ret, "failed to get follow-up bridge.\n");
+		goto unregister_aux;
+	}
+
+	ret = drm_bridge_attach(encoder, dp->next_bridge, bridge,
+				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret) {
+		dev_err_probe(dev, ret, "Failed to attach next bridge\n");
+		goto put_next_bridge;
+	}
+
 	dw_dp_init_hw(dp);
 
 	ret = phy_init(dp->phy);
 	if (ret) {
 		dev_err_probe(dev, ret, "phy init failed\n");
-		goto unregister_aux;
+		goto put_next_bridge;
 	}
 
 	enable_irq(dp->irq);
 
 	return 0;
 
+put_next_bridge:
+	drm_bridge_put(dp->next_bridge);
+
 unregister_aux:
 	drm_dp_aux_unregister(&dp->aux);
 
@@ -2014,6 +2033,7 @@ void dw_dp_unbind(struct dw_dp *dp)
 {
 	disable_irq(dp->irq);
 	phy_exit(dp->phy);
+	drm_bridge_put(dp->next_bridge);
 	drm_dp_aux_unregister(&dp->aux);
 	drm_bridge_remove(&dp->bridge);
 }

-- 
2.53.0

Re: [PATCH v4 03/10] drm/bridge: synopsys: dw-dp: Add follow-up bridge support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `hpd_work` workqueue item is not cancelled during driver unbind, leading to a Use-After-Free if it executes after the device is removed.
--

--- Patch [3]: [PATCH v4 03/10] drm/bridge: synopsys: dw-dp: Add follow-up bridge support ---
commit de5a52e4cba577233c79861335c4c6a6060e2e33
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Add follow-up bridge support
    
This commit adds support for using USB-C connectors with the DP altmode 
helper code on devicetree based platforms by attaching a follow-up bridge 
from the DP controller to the USB-C connector.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index aca6816febe67..0e8ecd73a0002 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -2014,6 +2033,7 @@ void dw_dp_unbind(struct dw_dp *dp)
>  {
>  	disable_irq(dp->irq);
>  	phy_exit(dp->phy);
> +	drm_bridge_put(dp->next_bridge);
>  	drm_dp_aux_unregister(&dp->aux);
>  	drm_bridge_remove(&dp->bridge);
>  }

[Severity: High]
This is a pre-existing issue, but does dw_dp_unbind() leave pending work
items that could cause a use-after-free?

If an HPD interrupt fires right before or during driver unbind, it might
schedule dp->hpd_work. The unbind function calls disable_irq(dp->irq), which
synchronizes with the running handler but does not flush the workqueue.

When the device is subsequently removed and the devm-managed dp structure is
freed, could a delayed execution of dw_dp_hpd_work() dereference the freed
dp pointer? Would it be appropriate to call cancel_work_sync(&dp->hpd_work)
here during teardown to ensure the work item is safely cancelled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=3

[PATCH v4 04/10] drm/bridge: Add out-of-band HPD notify handler

[Sebastian Reichel]

For DP bridges, that can be used for DP AltMode, it might be necessary
to enforce HPD status. There is an existing ->oob_hotplug_event() on
the DRM connector, but it currently just calls into hpd_notify().

As DP bridge drivers usually also implement .detect and that also
generates calls into hpd_notify, this is a bad place to force the
HPD status as the follow-up detect call might force it off again
resulting in all follow-up calls to the detection routine also
failing.

Avoid this by having a dedicated function for OOB events.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c |  6 ++++++
 include/drm/drm_bridge.h                       | 14 ++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 8b54069fa53a..632cc3ae3b54 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -180,6 +180,12 @@ static void drm_bridge_connector_oob_hotplug_event(struct drm_connector *connect
 	struct drm_bridge_connector *bridge_connector =
 		to_drm_bridge_connector(connector);
 
+	/* Notify all bridges in the pipeline of hotplug events. */
+	drm_for_each_bridge_in_chain(bridge_connector->encoder, bridge) {
+		if (bridge->funcs->oob_notify)
+			bridge->funcs->oob_notify(bridge, connector, status);
+	}
+
 	drm_bridge_connector_handle_hpd(bridge_connector, status);
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 3ac84ed57ab2..f38010d2edd1 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -540,6 +540,20 @@ struct drm_bridge_funcs {
 	 */
 	void (*hpd_disable)(struct drm_bridge *bridge);
 
+	/**
+	 * @oob_notify:
+	 *
+	 * Notify the bridge of out of band hot plug detection.
+	 *
+	 * This callback is optional, it may be implemented by bridges that
+	 * need to be notified of display connection or disconnection for
+	 * internal reasons. One use case is to force the DP controllers HPD
+	 * signal for USB-C DP AltMode.
+	 */
+	void (*oob_notify)(struct drm_bridge *bridge,
+			   struct drm_connector *connector,
+			   enum drm_connector_status status);
+
 	/**
 	 * @hdmi_tmds_char_rate_valid:
 	 *

-- 
2.53.0

[PATCH v4 05/10] drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD

[Sebastian Reichel]

Add support for USB-C DP AltMode out-of-band hotplug handling. The
handling itself is implemented in the platform specific driver as the
registers to force HPD state are not part of the Designware DisplayPort
IP itself. Instead the platform integration might provide the necessary
functionality to mux the HPD signal.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-dp.c | 38 +++++++++++++++++++++++++++++++++
 include/drm/bridge/dw_dp.h              |  3 +++
 2 files changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
index 0e8ecd73a000..8eb2699dc7ab 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
@@ -1819,6 +1819,19 @@ static struct drm_bridge_state *dw_dp_bridge_atomic_duplicate_state(struct drm_b
 	return &state->base;
 }
 
+static void dw_dp_bridge_oob_notify(struct drm_bridge *bridge,
+				    struct drm_connector *connector,
+				    enum drm_connector_status status)
+{
+	bool hpd_high = status != connector_status_disconnected;
+	struct dw_dp *dp = bridge_to_dp(bridge);
+
+	if (dp->plat_data.hpd_sw_cfg)
+		dp->plat_data.hpd_sw_cfg(dp->plat_data.data, hpd_high);
+	else
+		dev_err_once(dp->dev, "Missing platform handler for OOB HPD handling\n");
+}
+
 static const struct drm_bridge_funcs dw_dp_bridge_funcs = {
 	.atomic_duplicate_state = dw_dp_bridge_atomic_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -1831,6 +1844,7 @@ static const struct drm_bridge_funcs dw_dp_bridge_funcs = {
 	.atomic_disable = dw_dp_bridge_atomic_disable,
 	.detect = dw_dp_bridge_detect,
 	.edid_read = dw_dp_bridge_edid_read,
+	.oob_notify = dw_dp_bridge_oob_notify,
 };
 
 static int dw_dp_link_retrain(struct dw_dp *dp)
@@ -1960,6 +1974,19 @@ static const struct regmap_config dw_dp_regmap_config = {
 	.rd_table = &dw_dp_readable_table,
 };
 
+static bool dw_dp_is_routed_to_usb_c(struct drm_encoder *encoder)
+{
+	struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL;
+	struct fwnode_handle *fwnode;
+
+	last_bridge = drm_bridge_chain_get_last_bridge(encoder);
+	if (!last_bridge)
+		return false;
+
+	fwnode = of_fwnode_handle(last_bridge->of_node);
+	return fwnode_device_is_compatible(fwnode, "usb-c-connector");
+}
+
 int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
 {
 	struct device *dev = dp->dev;
@@ -2004,6 +2031,13 @@ int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
 		goto put_next_bridge;
 	}
 
+	if (dw_dp_is_routed_to_usb_c(encoder)) {
+		dev_dbg(dev, "USB-C mode\n");
+
+		if (dp->plat_data.hpd_sw_sel)
+			dp->plat_data.hpd_sw_sel(dp->plat_data.data, 1);
+	}
+
 	dw_dp_init_hw(dp);
 
 	ret = phy_init(dp->phy);
@@ -2052,6 +2086,10 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
 
 	dp->dev = dev;
 	dp->pixel_mode = plat_data->pixel_mode;
+
+	dp->plat_data.hpd_sw_sel = plat_data->hpd_sw_sel;
+	dp->plat_data.hpd_sw_cfg = plat_data->hpd_sw_cfg;
+	dp->plat_data.data = plat_data->data;
 	dp->plat_data.max_link_rate = plat_data->max_link_rate;
 
 	mutex_init(&dp->irq_lock);
diff --git a/include/drm/bridge/dw_dp.h b/include/drm/bridge/dw_dp.h
index a82412a9e769..79b2cdf0df99 100644
--- a/include/drm/bridge/dw_dp.h
+++ b/include/drm/bridge/dw_dp.h
@@ -20,6 +20,9 @@ enum {
 struct dw_dp_plat_data {
 	u32 max_link_rate;
 	u8 pixel_mode;
+	void *data;
+	void (*hpd_sw_sel)(void *data, bool hpd);
+	void (*hpd_sw_cfg)(void *data, bool hpd);
 };
 
 int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder);

-- 
2.53.0

Re: [PATCH v4 05/10] drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing Workqueue Cancellation / Use-After-Free in teardown path
--

--- Patch [5]: [PATCH v4 05/10] drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD ---
commit 27d73c3f273597742127e6851884d387b3ffe886
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD

This patch adds support for USB-C DP AltMode out-of-band hotplug handling
by allowing platform-specific drivers to implement HPD software
configuration and selection.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 0e8ecd73a0002..8eb2699dc7ab0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -2052,6 +2086,10 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
>  
>  	dp->dev = dev;
>  	dp->pixel_mode = plat_data->pixel_mode;
> +
> +	dp->plat_data.hpd_sw_sel = plat_data->hpd_sw_sel;
> +	dp->plat_data.hpd_sw_cfg = plat_data->hpd_sw_cfg;
> +	dp->plat_data.data = plat_data->data;
>  	dp->plat_data.max_link_rate = plat_data->max_link_rate;
>  
>  	mutex_init(&dp->irq_lock);

[Severity: High]
This is a pre-existing issue, but is there a missing workqueue cancellation
in the driver teardown path?

In dw_dp_probe(), the driver initializes a work item for hotplug detection
shortly after the modified code block:

    INIT_WORK(&dp->hpd_work, dw_dp_hpd_work);

However, looking at dw_dp_unbind(), the work is never canceled:

    void dw_dp_unbind(struct dw_dp *dp)
    {
        disable_irq(dp->irq);
        phy_exit(dp->phy);
        drm_bridge_put(dp->next_bridge);
        drm_dp_aux_unregister(&dp->aux);
        drm_bridge_remove(&dp->bridge);
    }

Since the dp structure is automatically freed by devres after the remove
callback completes, could this result in a use-after-free if the IRQ
handler schedules the work right before unbound is called, and the work
executes after the dp structure is deallocated? 

Would it make sense to add a cancel_work_sync(&dp->hpd_work) call to the
teardown path to ensure the work item finishes safely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=5

[PATCH v4 06/10] drm/rockchip: dw_dp: Implement out-of-band HPD handling

[Sebastian Reichel]

Implement out-of-band hotplug handling, which will be used to receive
external hotplug information from the USB-C state machine. This is
currently handled by the USBDP PHY, which brings quite some trouble
as the register being accessed requires the power-domain from the DP
controller and also requires custom TypeC HPD info parsing in the
USBDP PHY driver.

In contrast to the USBDP PHY this does not just enable the hotplug
signal when a DP AltMode capable adapter is plugged in, but instead
properly detects if a cable is plugged in for things like USB-C to
HDMI adapters.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_dp-rockchip.c | 123 ++++++++++++++++++++++++++++--
 1 file changed, 118 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
index 005938dc66c9..0ecbad24dd4e 100644
--- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
@@ -7,9 +7,12 @@
  */
 
 #include <linux/component.h>
+#include <linux/hw_bitfield.h>
 #include <linux/media-bus-format.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/videodev2.h>
 
 #include <drm/bridge/dw_dp.h>
@@ -23,12 +26,54 @@
 
 #include "rockchip_drm_drv.h"
 
+#define ROCKCHIP_MAX_CTRLS 2
+
+#define ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL BIT(10)
+#define ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG BIT(11)
+
+struct rockchip_dw_dp_plat_data {
+	u8 num_ctrls;
+	u32 ctrl_ids[ROCKCHIP_MAX_CTRLS];
+	u32 max_link_rate;
+	u8 pixel_mode;
+	u32 hpd_reg[ROCKCHIP_MAX_CTRLS];
+};
+
 struct rockchip_dw_dp {
 	struct dw_dp *base;
 	struct device *dev;
+	const struct rockchip_dw_dp_plat_data *pdata;
+	struct regmap *vo_grf;
 	struct rockchip_encoder *encoder;
+	int id;
+	bool hpd_sel;
+	bool hpd_cfg;
 };
 
+static void dw_dp_rockchip_hpd_sw_sel(void *data, bool force_hpd_from_sw)
+{
+	struct rockchip_dw_dp *dp = data;
+	u32 hpd_reg = dp->pdata->hpd_reg[dp->id];
+
+	dp->hpd_sel = force_hpd_from_sw;
+
+	regmap_write(dp->vo_grf, hpd_reg,
+		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, dp->hpd_sel));
+}
+
+static void dw_dp_rockchip_hpd_sw_cfg(void *data, bool hpd)
+{
+	struct rockchip_dw_dp *dp = data;
+	u32 hpd_reg = dp->pdata->hpd_reg[dp->id];
+
+	dev_dbg(dp->dev, "Force HPD connected=%s\n", str_yes_no(hpd));
+
+	dp->hpd_cfg = hpd;
+
+	regmap_write(dp->vo_grf, hpd_reg,
+		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG, dp->hpd_cfg));
+}
+
 static int dw_dp_encoder_atomic_check(struct drm_encoder *encoder,
 				      struct drm_crtc_state *crtc_state,
 				      struct drm_connector_state *conn_state)
@@ -71,6 +116,34 @@ static const struct drm_encoder_helper_funcs dw_dp_encoder_helper_funcs = {
 	.atomic_check		= dw_dp_encoder_atomic_check,
 };
 
+static struct regmap *dp_dp_rockchip_get_vo_grf(struct rockchip_dw_dp *dp)
+{
+	struct device_node *np = dev_of_node(dp->dev);
+	struct of_phandle_args args;
+	struct regmap *regmap;
+	int ret;
+
+	ret = of_parse_phandle_with_args(np, "phys", "#phy-cells", 0, &args);
+	if (ret)
+		return ERR_PTR(-ENODEV);
+
+	/*
+	 * Limit this workaround to RK3576 and RK3588, new platforms should
+	 * add a VO GRF phandle in the DisplayPort DT node.
+	 */
+	if (!of_device_is_compatible(args.np, "rockchip,rk3576-usbdp-phy") &&
+	    !of_device_is_compatible(args.np, "rockchip,rk3588-usbdp-phy")) {
+		regmap = ERR_PTR(-ENODEV);
+		goto out_put_node;
+	}
+
+	regmap = syscon_regmap_lookup_by_phandle(args.np, "rockchip,vo-grf");
+
+out_put_node:
+	of_node_put(args.np);
+	return regmap;
+}
+
 static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *data)
 {
 	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
@@ -121,19 +194,53 @@ static const struct component_ops dw_dp_rockchip_component_ops = {
 
 static int dw_dp_rockchip_probe(struct platform_device *pdev)
 {
-	const struct dw_dp_plat_data *plat_data;
+	const struct rockchip_dw_dp_plat_data *plat_data_const;
+	struct dw_dp_plat_data *plat_data;
 	struct device *dev = &pdev->dev;
 	struct rockchip_dw_dp *dp;
+	struct resource *res;
+	int id;
 
-	plat_data = of_device_get_match_data(dev);
-	if (!plat_data)
+	plat_data_const = device_get_match_data(dev);
+	if (!plat_data_const)
 		return -ENODEV;
 
+	plat_data = devm_kzalloc(dev, sizeof(*plat_data), GFP_KERNEL);
+	if (!plat_data)
+		return -ENOMEM;
+
 	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
 	if (!dp)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, dp);
 	dp->dev = dev;
+	dp->pdata = plat_data_const;
+
+	res = platform_get_mem_or_io(pdev, 0);
+	if (!res)
+		return -ENODEV;
+
+	/* find the DisplayPort ID from the io address */
+	dp->id = -ENODEV;
+	for (id = 0; id < plat_data_const->num_ctrls; id++) {
+		if (res->start == plat_data_const->ctrl_ids[id]) {
+			dp->id = id;
+			break;
+		}
+	}
+
+	if (dp->id < 0)
+		return dp->id;
+
+	dp->vo_grf = dp_dp_rockchip_get_vo_grf(dp);
+	if (IS_ERR(dp->vo_grf))
+		return PTR_ERR(dp->vo_grf);
+
+	plat_data->max_link_rate = plat_data_const->max_link_rate;
+	plat_data->pixel_mode = plat_data_const->pixel_mode;
+	plat_data->hpd_sw_sel = dw_dp_rockchip_hpd_sw_sel;
+	plat_data->hpd_sw_cfg = dw_dp_rockchip_hpd_sw_cfg;
+	plat_data->data = dp;
 
 	dp->base = dw_dp_probe(pdev, plat_data);
 	if (IS_ERR(dp->base))
@@ -147,14 +254,20 @@ static void dw_dp_rockchip_remove(struct platform_device *pdev)
 	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
 }
 
-static const struct dw_dp_plat_data rk3588_dp_plat_data = {
+static const struct rockchip_dw_dp_plat_data rk3588_dp_plat_data = {
+	.num_ctrls = 2,
+	.ctrl_ids = {0xfde50000, 0xfde60000},
 	.max_link_rate = 810000,
 	.pixel_mode = DW_DP_MP_QUAD_PIXEL,
+	.hpd_reg = {0x0000, 0x0008},
 };
 
-static const struct dw_dp_plat_data rk3576_dp_plat_data = {
+static const struct rockchip_dw_dp_plat_data rk3576_dp_plat_data = {
+	.num_ctrls = 1,
+	.ctrl_ids = {0x27e40000},
 	.max_link_rate = 810000,
 	.pixel_mode = DW_DP_MP_DUAL_PIXEL,
+	.hpd_reg = {0x0000},
 };
 
 static const struct of_device_id dw_dp_of_match[] = {

-- 
2.53.0

Re: [PATCH v4 06/10] drm/rockchip: dw_dp: Implement out-of-band HPD handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The VO GRF regmap lookup fails to parse the DisplayPort DT node for new platforms, contradicting its documented design.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but missing cancel_work_sync() for hpd_work in dw_dp_unbind() causes a Use-After-Free.
--

--- Patch [6]: [PATCH v4 06/10] drm/rockchip: dw_dp: Implement out-of-band HPD handling ---
commit 78e1ce0dbd7ef8a004080f25b4449ba54b71de3a
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/rockchip: dw_dp: Implement out-of-band HPD handling

This patch implements out-of-band hotplug handling to receive external
hotplug information from the USB-C state machine. It uses the vo-grf
registers to control HPD signal routing and configuration.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index 005938dc66c9f..0ecbad24dd4e8 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -71,6 +116,34 @@ static const struct drm_encoder_helper_funcs dw_dp_encoder_helper_funcs = {
>  	.atomic_check		= dw_dp_encoder_atomic_check,
>  };
>  
> +static struct regmap *dp_dp_rockchip_get_vo_grf(struct rockchip_dw_dp *dp)
> +{
> +	struct device_node *np = dev_of_node(dp->dev);
> +	struct of_phandle_args args;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_args(np, "phys", "#phy-cells", 0, &args);
> +	if (ret)
> +		return ERR_PTR(-ENODEV);
> +
> +	/*
> +	 * Limit this workaround to RK3576 and RK3588, new platforms should
> +	 * add a VO GRF phandle in the DisplayPort DT node.
> +	 */
> +	if (!of_device_is_compatible(args.np, "rockchip,rk3576-usbdp-phy") &&
> +	    !of_device_is_compatible(args.np, "rockchip,rk3588-usbdp-phy")) {
> +		regmap = ERR_PTR(-ENODEV);
> +		goto out_put_node;
> +	}
> +
> +	regmap = syscon_regmap_lookup_by_phandle(args.np, "rockchip,vo-grf");

[Severity: Medium]
Will this fail to probe on new platforms? 

The comment states new platforms should add a VO GRF phandle in the
DisplayPort DT node, but the code only looks up "rockchip,vo-grf" on the
phy node (args.np) after explicitly rejecting any phy other than rk3576 and
rk3588. 

Should it try to parse the property from the DisplayPort node (np) instead?

> +
> +out_put_node:
> +	of_node_put(args.np);
> +	return regmap;
> +}
[ ... ]

While reviewing this patch, I also noticed a pre-existing issue in the
related dw_dp_unbind() teardown path:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_unbind() {
	...
	disable_irq(dp->irq);
	phy_exit(dp->phy);
	drm_bridge_put(dp->next_bridge);
	drm_dp_aux_unregister(&dp->aux);
	drm_bridge_remove(&dp->bridge);
}

[Severity: High]
This isn't a bug introduced by this patch, but can this cause a
use-after-free if an HPD interrupt fires right before disable_irq() is
called? 

If dp->hpd_work is scheduled and the devm-allocated struct dw_dp is freed
during device removal, the background worker might execute dw_dp_hpd_work()
on freed memory. 

Does dw_dp_unbind() need to call cancel_work_sync(&dp->hpd_work)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=6

[PATCH v4 07/10] drm/bridge: synopsys: dw-dp: Add Runtime PM support

[Sebastian Reichel]

Add runtime PM stubs to the Synopsys DesignWare DisplayPort bridge
driver. Support is not enabled automatically and must be hooked up
in the vendor specific glue code.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-dp.c | 50 +++++++++++++++++++++++++++++++++
 include/drm/bridge/dw_dp.h              |  3 ++
 2 files changed, 53 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
index 8eb2699dc7ab..636b9e105297 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
@@ -328,6 +328,7 @@ struct dw_dp {
 	struct dw_dp_link link;
 	struct dw_dp_plat_data plat_data;
 	u8 pixel_mode;
+	bool usbc_mode;
 
 	struct drm_bridge *next_bridge;
 
@@ -1467,6 +1468,8 @@ static ssize_t dw_dp_aux_transfer(struct drm_dp_aux *aux,
 	if (WARN_ON(msg->size > 16))
 		return -E2BIG;
 
+	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
+
 	switch (msg->request & ~DP_AUX_I2C_MOT) {
 	case DP_AUX_NATIVE_WRITE:
 	case DP_AUX_I2C_WRITE:
@@ -1657,6 +1660,8 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 	struct drm_connector_state *conn_state;
 	int ret;
 
+	pm_runtime_get_sync(dp->dev);
+
 	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
 	if (!connector) {
 		dev_err(dp->dev, "failed to get connector\n");
@@ -1711,6 +1716,7 @@ static void dw_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 	dw_dp_link_disable(dp);
 	bitmap_zero(dp->sdp_reg_bank, SDP_REG_BANK_SIZE);
 	dw_dp_reset(dp);
+	pm_runtime_put_autosuspend(dp->dev);
 }
 
 static bool dw_dp_hpd_detect_link(struct dw_dp *dp, struct drm_connector *connector)
@@ -1731,6 +1737,14 @@ static enum drm_connector_status dw_dp_bridge_detect(struct drm_bridge *bridge,
 {
 	struct dw_dp *dp = bridge_to_dp(bridge);
 
+	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
+
+	/*
+	 * HPD_HOT_PLUG bit is asserted only after the sink holds HPD high for
+	 * at least 100ms. Thus we need to wait 100ms after runtime PM resume.
+	 */
+	msleep(110);
+
 	if (!dw_dp_hpd_detect(dp))
 		return connector_status_disconnected;
 
@@ -2032,10 +2046,15 @@ int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
 	}
 
 	if (dw_dp_is_routed_to_usb_c(encoder)) {
+		dp->usbc_mode = true;
 		dev_dbg(dev, "USB-C mode\n");
 
 		if (dp->plat_data.hpd_sw_sel)
 			dp->plat_data.hpd_sw_sel(dp->plat_data.data, 1);
+	} else {
+		dp->usbc_mode = false;
+		/* Keep runtime PM enabled to have working native HPD IRQ */
+		pm_runtime_get_sync(dp->dev);
 	}
 
 	dw_dp_init_hw(dp);
@@ -2065,6 +2084,8 @@ EXPORT_SYMBOL_GPL(dw_dp_bind);
 
 void dw_dp_unbind(struct dw_dp *dp)
 {
+	if (dp->usbc_mode)
+		pm_runtime_put_autosuspend(dp->dev);
 	disable_irq(dp->irq);
 	phy_exit(dp->phy);
 	drm_bridge_put(dp->next_bridge);
@@ -2163,6 +2184,35 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
 }
 EXPORT_SYMBOL_GPL(dw_dp_probe);
 
+int dw_dp_runtime_suspend(struct dw_dp *dp)
+{
+	clk_disable_unprepare(dp->aux_clk);
+	clk_disable_unprepare(dp->apb_clk);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_dp_runtime_suspend);
+
+int dw_dp_runtime_resume(struct dw_dp *dp)
+{
+	int ret;
+
+	ret = clk_prepare_enable(dp->apb_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(dp->aux_clk);
+	if (ret) {
+		clk_disable_unprepare(dp->apb_clk);
+		return ret;
+	}
+
+	dw_dp_init_hw(dp);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_dp_runtime_resume);
+
 MODULE_AUTHOR("Andy Yan <andyshrk@163.com>");
 MODULE_DESCRIPTION("DW DP Core Library");
 MODULE_LICENSE("GPL");
diff --git a/include/drm/bridge/dw_dp.h b/include/drm/bridge/dw_dp.h
index 79b2cdf0df99..f20d2c2c40ca 100644
--- a/include/drm/bridge/dw_dp.h
+++ b/include/drm/bridge/dw_dp.h
@@ -29,4 +29,7 @@ int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder);
 void dw_dp_unbind(struct dw_dp *dp);
 
 struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_data *plat_data);
+
+int dw_dp_runtime_suspend(struct dw_dp *dp);
+int dw_dp_runtime_resume(struct dw_dp *dp);
 #endif /* __DW_DP__ */

-- 
2.53.0

Re: [PATCH v4 07/10] drm/bridge: synopsys: dw-dp: Add Runtime PM support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:

New issues:
- [High] Inverted boolean condition in `dw_dp_unbind()` causes runtime PM usage count leak and unbalanced puts.
- [High] Missing API return value check for `pm_runtime_get_sync()` causes unsafe hardware access on failure.
- [High] Sleeping function `pm_runtime_get_sync()` called from atomic context.
- [High] Missing runtime PM reference release on error path in `dw_dp_bind()`.
- [High] Asynchronous `pm_runtime_put_autosuspend()` is incorrectly used in the teardown path.
- [High] Clock disable imbalance between `devm_clk_get_enabled()` and explicit `clk_disable_unprepare()` in runtime suspend.
- [Medium] Unconditional sleep in `dw_dp_bridge_detect()` causes severe performance degradation during display polling.

Pre-existing issues:
- [Critical] Use-After-Free due to missing cancellation of `hpd_work` in teardown.
- [High] Preexisting atomic context violation in `dw_dp_bridge_atomic_disable()`.
--

--- Patch [7]: [PATCH v4 07/10] drm/bridge: synopsys: dw-dp: Add Runtime PM support ---
commit e300d93b1e1336324cbbd71c5f0b1b81334c7912
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
Date:   Tue Jul 21 17:52:37 2026 +0000

    drm/bridge: synopsys: dw-dp: Add Runtime PM support
    
    Add runtime PM stubs to the Synopsys DesignWare DisplayPort bridge
    driver. Support is not enabled automatically and must be hooked up
    in the vendor specific glue code.
    
    Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 8eb2699dc7ab0..636b9e1052978 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1467,6 +1468,8 @@ static ssize_t dw_dp_aux_transfer(struct drm_dp_aux *aux,
>  	if (WARN_ON(msg->size > 16))
>  		return -E2BIG;
>  
> +	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
> +

[Severity: High]
Does the ACQUIRE macro wrapper silently ignore the return value of the
underlying get_sync operation?

If the device fails to power on or clock enablement fails, the driver might
proceed to access hardware registers anyway. Could we handle the error here 
and abort the transfer instead of proceeding?

>  	switch (msg->request & ~DP_AUX_I2C_MOT) {
>  	case DP_AUX_NATIVE_WRITE:
>  	case DP_AUX_I2C_WRITE:

[ ... ]

> @@ -1657,6 +1660,8 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct drm_connector_state *conn_state;
>  	int ret;
>  
> +	pm_runtime_get_sync(dp->dev);
> +

[Severity: High]
Are we allowed to call a sleeping function from atomic context here?

The dw_dp_bridge_atomic_enable() callback executes in atomic context where
sleeping is forbidden. However, pm_runtime_get_sync() triggers
dw_dp_runtime_resume(), which calls clk_prepare_enable() and sleeps.

Also, similar to the ACQUIRE wrapper above, the return value of
pm_runtime_get_sync() is ignored here. If it fails, will the driver proceed
to access hardware registers while the device is unclocked?

>  	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
>  	if (!connector) {
>  		dev_err(dp->dev, "failed to get connector\n");

[ ... ]

> @@ -1711,6 +1716,7 @@ static void dw_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  	dw_dp_link_disable(dp);
>  	bitmap_zero(dp->sdp_reg_bank, SDP_REG_BANK_SIZE);
>  	dw_dp_reset(dp);
> +	pm_runtime_put_autosuspend(dp->dev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does dw_dp_reset() sleep inside
an atomic context?

The dw_dp_bridge_atomic_disable() callback runs in atomic context, but
dw_dp_reset() unconditionally calls usleep_range() and
regmap_read_poll_timeout(). Should this hardware reset sequence be moved out
of the atomic path or use non-sleeping delays?

>  static bool dw_dp_hpd_detect_link(struct dw_dp *dp, struct drm_connector *connector)
> @@ -1731,6 +1737,14 @@ static enum drm_connector_status dw_dp_bridge_detect(struct drm_bridge *bridge,
>  {
>  	struct dw_dp *dp = bridge_to_dp(bridge);
>  
> +	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
> +
> +	/*
> +	 * HPD_HOT_PLUG bit is asserted only after the sink holds HPD high for
> +	 * at least 100ms. Thus we need to wait 100ms after runtime PM resume.
> +	 */
> +	msleep(110);
> +

[Severity: Medium]
Will this sleep degrade display polling performance?

By using the ACQUIRE wrapper, the return value of pm_runtime_get_sync() is
discarded, making it impossible to differentiate a fresh resume (returning 0)
from an already-active device (returning 1).

As a result, this 110ms sleep executes unconditionally on every detect call,
even when the device is already active (such as in native DP mode where it is
kept active permanently).

>  	if (!dw_dp_hpd_detect(dp))
>  		return connector_status_disconnected;
>  

[ ... ]

> @@ -2032,10 +2046,15 @@ int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
>  	}
>  
>  	if (dw_dp_is_routed_to_usb_c(encoder)) {
> +		dp->usbc_mode = true;
>  		dev_dbg(dev, "USB-C mode\n");
>  
>  		if (dp->plat_data.hpd_sw_sel)
>  			dp->plat_data.hpd_sw_sel(dp->plat_data.data, 1);
> +	} else {
> +		dp->usbc_mode = false;
> +		/* Keep runtime PM enabled to have working native HPD IRQ */
> +		pm_runtime_get_sync(dp->dev);
>  	}
>  
>  	dw_dp_init_hw(dp);

[Severity: High]
Does the error path for hardware initialization following this block leak the
runtime PM reference?

If dp->usbc_mode is false, pm_runtime_get_sync() increments the usage count.
If phy_init() fails shortly after dw_dp_init_hw(), the function jumps to the
err_put_next_bridge label and returns without balancing this get_sync
operation, causing the PM reference to permanently leak.

> @@ -2065,6 +2084,8 @@ EXPORT_SYMBOL_GPL(dw_dp_bind);
>  
>  void dw_dp_unbind(struct dw_dp *dp)
>  {
> +	if (dp->usbc_mode)
> +		pm_runtime_put_autosuspend(dp->dev);

[Severity: High]
Is this boolean condition inverted?

In dw_dp_bind(), pm_runtime_get_sync() is called when usbc_mode is false.
By checking for true here, we call put on a reference we never acquired
(causing an underflow/warning for USB-C) and leak the reference when native DP
mode is used.

[Severity: High]
Is it safe to use the asynchronous pm_runtime_put_autosuspend() variant in a
teardown path?

When device removal or pm_runtime_disable() follows immediately, the pending
async idle/suspend work can be cancelled by the PM core. This could leave the
hardware active indefinitely. Should a synchronous pm_runtime_put_sync() be
used here instead?

>  	disable_irq(dp->irq);

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free risk because
hpd_work is not canceled?

The hpd_work struct is allocated via devm_kzalloc. In this teardown path, the
IRQ is disabled, but cancel_work_sync(&dp->hpd_work) is never called. If the
work was scheduled by an interrupt just before disable_irq(), the work queue
thread could execute after the devm subsystem frees the memory.

>  	phy_exit(dp->phy);
>  	drm_bridge_put(dp->next_bridge);

[ ... ]

> @@ -2163,6 +2184,35 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
>  }
>  EXPORT_SYMBOL_GPL(dw_dp_probe);
>  
> +int dw_dp_runtime_suspend(struct dw_dp *dp)
> +{
> +	clk_disable_unprepare(dp->aux_clk);
> +	clk_disable_unprepare(dp->apb_clk);
> +
> +	return 0;
> +}

[Severity: High]
Will manually disabling these clocks here cause an imbalance on device teardown?

If the apb_clk and aux_clk clocks are acquired using devm_clk_get_enabled(),
a cleanup action is registered to call clk_disable_unprepare() upon device
unbind. If the device is runtime suspended when it is unbound, the devm
teardown mechanism will invoke clk_disable_unprepare() again on these
already-disabled clocks, dropping the enable counters below zero.

> +EXPORT_SYMBOL_GPL(dw_dp_runtime_suspend);
> +
> +int dw_dp_runtime_resume(struct dw_dp *dp)
> +{
> +	int ret;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=7

[PATCH v4 08/10] drm/rockchip: dw_dp: Add runtime PM support

[Sebastian Reichel]

Add support for runtime PM to the Rockchip RK3576/3588 Synopsys
DesignWare DisplayPort driver.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/rockchip/dw_dp-rockchip.c | 53 +++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
index 0ecbad24dd4e..d7e1704fd980 100644
--- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
@@ -12,6 +12,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/videodev2.h>
 
@@ -57,6 +58,8 @@ static void dw_dp_rockchip_hpd_sw_sel(void *data, bool force_hpd_from_sw)
 
 	dp->hpd_sel = force_hpd_from_sw;
 
+	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
+
 	regmap_write(dp->vo_grf, hpd_reg,
 		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, dp->hpd_sel));
 }
@@ -70,6 +73,8 @@ static void dw_dp_rockchip_hpd_sw_cfg(void *data, bool hpd)
 
 	dp->hpd_cfg = hpd;
 
+	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
+
 	regmap_write(dp->vo_grf, hpd_reg,
 		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG, dp->hpd_cfg));
 }
@@ -169,14 +174,25 @@ static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *
 	if (ret)
 		return ret;
 
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 500);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	connector = drm_bridge_connector_init(drm_dev, encoder);
 	if (IS_ERR(connector)) {
-		dw_dp_unbind(dp->base);
-		return dev_err_probe(dev, PTR_ERR(connector),
-				     "Failed to init bridge connector\n");
+		ret = PTR_ERR(connector);
+		dev_err_probe(dev, ret, "Failed to init bridge connector\n");
+		goto error;
 	}
 
 	return 0;
+
+error:
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_disable(dev);
+	dw_dp_unbind(dp->base);
+	return ret;
 }
 
 static void dw_dp_rockchip_unbind(struct device *dev, struct device *master,
@@ -184,6 +200,8 @@ static void dw_dp_rockchip_unbind(struct device *dev, struct device *master,
 {
 	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
 
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_disable(dev);
 	dw_dp_unbind(dp->base);
 }
 
@@ -254,6 +272,34 @@ static void dw_dp_rockchip_remove(struct platform_device *pdev)
 	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
 }
 
+static int dw_dp_rockchip_runtime_suspend(struct device *dev)
+{
+	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
+
+	return dw_dp_runtime_suspend(dp->base);
+}
+
+static int dw_dp_rockchip_runtime_resume(struct device *dev)
+{
+	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
+	u32 hpd_reg = dp->pdata->hpd_reg[dp->id];
+	int ret;
+
+	ret = dw_dp_runtime_resume(dp->base);
+	if (ret)
+		return ret;
+
+	regmap_write(dp->vo_grf, hpd_reg,
+		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, dp->hpd_sel) |
+		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG, dp->hpd_cfg));
+
+	return 0;
+}
+
+static const struct dev_pm_ops dw_dp_pm_ops = {
+	RUNTIME_PM_OPS(dw_dp_rockchip_runtime_suspend, dw_dp_rockchip_runtime_resume, NULL)
+};
+
 static const struct rockchip_dw_dp_plat_data rk3588_dp_plat_data = {
 	.num_ctrls = 2,
 	.ctrl_ids = {0xfde50000, 0xfde60000},
@@ -288,5 +334,6 @@ struct platform_driver dw_dp_driver = {
 	.driver = {
 		.name = "dw-dp",
 		.of_match_table = dw_dp_of_match,
+		.pm = pm_ptr(&dw_dp_pm_ops),
 	},
 };

-- 
2.53.0

Re: [PATCH v4 08/10] drm/rockchip: dw_dp: Add runtime PM support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Critical] Threaded IRQ handler accesses unclocked hardware registers if triggered while runtime suspended.
- [High] Double disable of devm-managed clocks due to uncoordinated runtime PM suspend.
- [Medium] Incorrect runtime PM teardown sequence prevents asynchronous suspends.
- [Medium] Missing idle trigger leaves the device fully powered on after initialization.
- [Medium] Calling dw_dp_bind() before pm_runtime_enable() causes -EACCES errors from the PM core.

Pre-existing issues:
- [High] Inverted logic for runtime PM reference management causes usage count leaks and underflows.
- [High] Missing runtime PM reference drop on the bind error path.
--

--- Patch [8]: [PATCH v4 08/10] drm/rockchip: dw_dp: Add runtime PM support ---
commit 73aba26d330e327d28e953d044baf166117572f7
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/rockchip: dw_dp: Add runtime PM support

This patch adds support for runtime PM to the Rockchip RK3576/3588 Synopsys
DesignWare DisplayPort driver.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index 0ecbad24dd4e8..d7e1704fd9806 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -169,14 +174,25 @@ static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *
>  	if (ret)
>  		return ret;
>  
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);

[Severity: Medium]
Because dw_dp_bind() is called immediately before this block, does it
cause -EACCES errors from the PM core?

Inside dw_dp_bind(), the core driver calls pm_runtime_get_sync(dp->dev).
Since runtime PM is not yet enabled for the device at that point, the PM
core will return -EACCES.

[Severity: High]
This is a pre-existing issue, but does dw_dp_bind() leak a runtime PM
reference on its error path?

In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind():

    if (!dp->usbc_mode)
        pm_runtime_get_sync(dp->dev);

    ...

    ret = phy_init(dp->phy);
    if (ret) {
        dev_err_probe(dev, ret, "phy init failed\n");
        goto put_next_bridge;
    }

If phy_init() fails, the function jumps to put_next_bridge and returns
without dropping the reference acquired earlier, permanently preventing
the device from suspending.

[Severity: Medium]
Does this initialization sequence miss an idle trigger?

The driver calls pm_runtime_set_active() and pm_runtime_enable(), but does
not queue an idle check. The runtime PM core does not automatically queue
one on enable, which might leave the device fully powered on until the
first display modeset cycle. Could a manual call to pm_request_idle() be
needed to start the autosuspend timer?

>  	connector = drm_bridge_connector_init(drm_dev, encoder);
>  	if (IS_ERR(connector)) {
> -		dw_dp_unbind(dp->base);
> -		return dev_err_probe(dev, PTR_ERR(connector),
> -				     "Failed to init bridge connector\n");
> +		ret = PTR_ERR(connector);
> +		dev_err_probe(dev, ret, "Failed to init bridge connector\n");
> +		goto error;
>  	}
>  
>  	return 0;
> +
> +error:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
> +	dw_dp_unbind(dp->base);
> +	return ret;
>  }
>  
>  static void dw_dp_rockchip_unbind(struct device *dev, struct device *master,
> @@ -184,6 +200,8 @@ static void dw_dp_rockchip_unbind(struct device *dev, struct device *master,
>  {
>  	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
>  
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
>  	dw_dp_unbind(dp->base);
>  }

[Severity: Medium]
Does disabling runtime PM before calling dw_dp_unbind() prevent
asynchronous suspends?

By calling pm_runtime_disable() first, when dw_dp_unbind() subsequently
calls pm_runtime_put_autosuspend(), the asynchronous idle request is
rejected because runtime PM is already disabled. This leaves the device
permanently in the RPM_ACTIVE state and the hardware remains powered on
after the driver is unbound.

[Severity: High]
Could this teardown sequence result in a double disable of devm-managed
clocks?

The driver acquires clocks using devm_clk_get_enabled() during probe,
which registers a devres action to automatically disable them on removal.

However, dw_dp_runtime_suspend() manually calls clk_disable_unprepare().
If the device is already in the RPM_SUSPENDED state when unbound,
pm_runtime_disable() leaves it suspended. When devres subsequently
attempts to disable the already-disabled clocks, it will cause an
underflow.

[Severity: High]
This is a pre-existing issue, but is the logic for dropping the runtime
PM reference inverted in dw_dp_unbind()?

In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind():

    if (!dp->usbc_mode) {
        pm_runtime_get_sync(dp->dev);
    }

But in dw_dp_unbind():

    if (dp->usbc_mode)
        pm_runtime_put_autosuspend(dp->dev);

This means standard DP (usbc_mode = false) leaks the reference it
acquired, while USB-C DP (usbc_mode = true) attempts to drop a reference
it never acquired, causing usage count leaks and underflows.

[ ... ]
> @@ -254,6 +272,34 @@ static void dw_dp_rockchip_remove(struct platform_device *pdev)
>  	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
>  }
>  
> +static int dw_dp_rockchip_runtime_suspend(struct device *dev)
> +{
> +	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
> +
> +	return dw_dp_runtime_suspend(dp->base);
> +}

[Severity: Critical]
Does leaving the IRQ enabled during runtime suspend risk a kernel panic?

This patch introduces runtime PM support, which can now trigger an issue
in the bridge driver. dw_dp_runtime_suspend() disables the apb_clk.
However, the driver leaves dp->irq enabled.

If a spurious interrupt or HPD event fires while the device is in the
RPM_SUSPENDED state, dw_dp_irq() will directly read
DW_DP_GENERAL_INTERRUPT via regmap:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_irq() {
    ...
    regmap_read(dp->regmap, DW_DP_GENERAL_INTERRUPT, &value);
}

Accessing these unclocked hardware registers without first checking the
runtime PM status (e.g., using pm_runtime_get_if_active()) will cause a
synchronous external abort or data abort.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=8

[PATCH v4 09/10] dt-bindings: display: rockchip: dw-dp: Fix sound DAI cells

[Sebastian Reichel]

The RK3588 and RK3576 DesignWare DisplayPort controllers both have two
possible DAI interfaces: I2S and S/PDIF. Thus an argument is needed to
to select the right interface.

In addition to that the RK3576 has the same IP as RK3588, but configured
with Multi Stream Transport (MST) enabled for up to 3 displays and thus
has a total of 6 DAI interfaces (I2S and S/PDIF for each possible
stream). Meanwhile the RK3588 does not support MST and thus has only 2
DAI interfaces.

The binding update from this patch has only been tested with the simple
single stream transport (SST) setup as the Linux driver does not yet
support MST.

For MST it is unclear what a good binding should look like. Technically
there are a couple of options:

1. Stay with the single cell, but add more numbers. I.e.
   0 = I2S on stream 0,
   1 = S/PDIF on stream 0
   2 = I2S on stream 1,
   3 = S/PDIF on stream 1
   4 = I2S on stream 2,
   5 = S/PDIF on stream 2

2. Add yet another cell, so that there is one argument for the stream ID
   and one argument to select between S/PDIF and I2S. I.e.
   0 0 = I2S on stream 0
   0 1 = S/PDIF on stream 0
   1 0 = I2S on stream 1
   1 1 = S/PDIF on stream 1
   2 0 = I2S on stream 2
   2 1 = S/PDIF on stream 2

3. Create one subnode for each stream in the DP controller with its
   own graph for the video streams. This would mean the MST binding
   is quite different from the SST one.

As it is unclear which way is the most sensible at this point in time,
just go with the most simple option for now and revisit this once MST
is added.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml     | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
index 2b0d9e23e943..c4f8959dd65d 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
@@ -25,7 +25,7 @@ description: |
   * Supports up to 8/10 bits per color component
   * Supports RBG, YCbCr4:4:4, YCbCr4:2:2, YCbCr4:2:0
   * Pixel clock up to 594MHz
-  * I2S, SPDIF audio interface
+  * I2S, S/PDIF audio interface
 
 properties:
   compatible:
@@ -46,7 +46,7 @@ properties:
       - description: DisplayPort AUX clock
       - description: HDCP clock
       - description: I2S interface clock
-      - description: SPDIF interfce clock
+      - description: S/PDIF interfce clock
 
   clock-names:
     minItems: 3
@@ -83,7 +83,8 @@ properties:
     maxItems: 1
 
   "#sound-dai-cells":
-    const: 0
+    const: 1
+    description: 0 for I2S, 1 for S/PDIF
 
 required:
   - compatible
@@ -144,7 +145,7 @@ examples:
         resets = <&cru SRST_DP0>;
         phys = <&usbdp_phy0 PHY_TYPE_DP>;
         power-domains = <&power RK3588_PD_VO0>;
-        #sound-dai-cells = <0>;
+        #sound-dai-cells = <1>;
 
         ports {
           #address-cells = <1>;

-- 
2.53.0

Re: [PATCH v4 09/10] dt-bindings: display: rockchip: dw-dp: Fix sound DAI cells

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unconditional ABI breakage of `#sound-dai-cells` in Device Tree binding.
--

--- Patch [9]: [PATCH v4 09/10] dt-bindings: display: rockchip: dw-dp: Fix sound DAI cells ---
commit 3829785f7403edd4d5c1e524c76b4c78f5cf26da
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

dt-bindings: display: rockchip: dw-dp: Fix sound DAI cells

This commit updates the rockchip dw-dp binding to change #sound-dai-cells
from 0 to 1, allowing the selection between I2S and S/PDIF. The example is
also updated to reflect this new required cell count.

> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
> index 2b0d9e23e9432..c4f8959dd65da 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
[ ... ]
> @@ -83,7 +83,8 @@ properties:
>      maxItems: 1
>  
>    "#sound-dai-cells":
> -    const: 0
> +    const: 1
> +    description: 0 for I2S, 1 for S/PDIF

[Severity: Medium]
Will this unconditional constraint change cause a regression by breaking
Device Tree ABI backward compatibility?

Existing in-tree device trees such as
arch/arm64/boot/dts/rockchip/rk3588-base.dtsi still define the property with
0 cells:

arch/arm64/boot/dts/rockchip/rk3588-base.dtsi:dp0 {
	...
	resets = <&cru SRST_DP0>;
	#sound-dai-cells = <0>;
	status = "disabled";
}

A similar definition exists in
arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi for dp1.

Could this update lead to immediate dtbs_check validation failures and break
audio initialization for users booting with older, previously valid DTBs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=9

Re: [PATCH v4 09/10] dt-bindings: display: rockchip: dw-dp: Fix sound DAI cells

[Krzysztof Kozlowski]

On Tue, Jul 21, 2026 at 07:52:39PM +0200, Sebastian Reichel wrote:
> The RK3588 and RK3576 DesignWare DisplayPort controllers both have two
> possible DAI interfaces: I2S and S/PDIF. Thus an argument is needed to
> to select the right interface.
> 
> In addition to that the RK3576 has the same IP as RK3588, but configured
> with Multi Stream Transport (MST) enabled for up to 3 displays and thus
> has a total of 6 DAI interfaces (I2S and S/PDIF for each possible
> stream). Meanwhile the RK3588 does not support MST and thus has only 2
> DAI interfaces.
> 
> The binding update from this patch has only been tested with the simple
> single stream transport (SST) setup as the Linux driver does not yet
> support MST.
> 
> For MST it is unclear what a good binding should look like. Technically
> there are a couple of options:
> 
> 1. Stay with the single cell, but add more numbers. I.e.
>    0 = I2S on stream 0,
>    1 = S/PDIF on stream 0
>    2 = I2S on stream 1,
>    3 = S/PDIF on stream 1
>    4 = I2S on stream 2,
>    5 = S/PDIF on stream 2

This looks simpler and anyway the numbers are abstract, so they do not
need to match "stream X".

> 
> 2. Add yet another cell, so that there is one argument for the stream ID
>    and one argument to select between S/PDIF and I2S. I.e.
>    0 0 = I2S on stream 0
>    0 1 = S/PDIF on stream 0
>    1 0 = I2S on stream 1
>    1 1 = S/PDIF on stream 1
>    2 0 = I2S on stream 2
>    2 1 = S/PDIF on stream 2
> 
> 3. Create one subnode for each stream in the DP controller with its
>    own graph for the video streams. This would mean the MST binding
>    is quite different from the SST one.
> 
> As it is unclear which way is the most sensible at this point in time,
> just go with the most simple option for now and revisit this once MST
> is added.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml     | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
> index 2b0d9e23e943..c4f8959dd65d 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-dp.yaml
> @@ -25,7 +25,7 @@ description: |
>    * Supports up to 8/10 bits per color component
>    * Supports RBG, YCbCr4:4:4, YCbCr4:2:2, YCbCr4:2:0
>    * Pixel clock up to 594MHz
> -  * I2S, SPDIF audio interface
> +  * I2S, S/PDIF audio interface
>  
>  properties:
>    compatible:
> @@ -46,7 +46,7 @@ properties:
>        - description: DisplayPort AUX clock
>        - description: HDCP clock
>        - description: I2S interface clock
> -      - description: SPDIF interfce clock
> +      - description: S/PDIF interfce clock
>  
>    clock-names:
>      minItems: 3
> @@ -83,7 +83,8 @@ properties:
>      maxItems: 1
>  
>    "#sound-dai-cells":
> -    const: 0
> +    const: 1
> +    description: 0 for I2S, 1 for S/PDIF

This is ABI change, which might be backwards compatible or not, but
commit msg should explain that. I can easily imagine drivers where
written that way that they will fail with an old DTB.

Best regards,
Krzysztof

[PATCH v4 10/10] drm/bridge: synopsys: dw-dp: Add audio support

[Sebastian Reichel]

Implement audio support for the Synopsys DesignWare DisplayPort
controller.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-dp.c | 230 +++++++++++++++++++++++++++++++-
 1 file changed, 229 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
index 636b9e105297..8d6a52135e54 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
@@ -23,17 +23,21 @@
 #include <drm/drm_bridge.h>
 #include <drm/drm_bridge_connector.h>
 #include <drm/display/drm_dp_helper.h>
+#include <drm/display/drm_hdmi_audio_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_of.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
 
+#include <sound/hdmi-codec.h>
+
 #define DW_DP_VERSION_NUMBER			0x0000
 #define DW_DP_VERSION_TYPE			0x0004
 #define DW_DP_ID				0x0008
 
 #define DW_DP_CONFIG_REG1			0x0100
+#define AUDIO_SELECT				GENMASK(2, 1)
 #define DW_DP_CONFIG_REG2			0x0104
 #define DW_DP_CONFIG_REG3			0x0108
 
@@ -110,6 +114,10 @@
 #define HBR_MODE_ENABLE				BIT(10)
 #define AUDIO_DATA_WIDTH			GENMASK(9, 5)
 #define AUDIO_DATA_IN_EN			GENMASK(4, 1)
+#define AUDIO_DATA_IN_EN_CHANNEL12		BIT(0)
+#define AUDIO_DATA_IN_EN_CHANNEL34		BIT(1)
+#define AUDIO_DATA_IN_EN_CHANNEL56		BIT(2)
+#define AUDIO_DATA_IN_EN_CHANNEL78		BIT(3)
 #define AUDIO_INF_SELECT			BIT(0)
 
 #define DW_DP_SDP_VERTICAL_CTRL			0x0500
@@ -253,6 +261,8 @@
 
 #define SDP_REG_BANK_SIZE			16
 
+#define DW_DP_SDP_VERSION			0x12
+
 struct dw_dp_link_caps {
 	bool enhanced_framing;
 	bool tps3_supported;
@@ -305,6 +315,19 @@ struct dw_dp_hotplug {
 	bool long_hpd;
 };
 
+enum dw_dp_audio_interface_support {
+	DW_DP_AUDIO_I2S_ONLY = 0,
+	DW_DP_AUDIO_SPDIF_ONLY = 1,
+	DW_DP_AUDIO_I2S_AND_SPDIF = 2,
+	DW_DP_AUDIO_NONE = 3,
+};
+
+enum dw_dp_audio_interface {
+	DW_DP_AUDIO_I2S = 0,
+	DW_DP_AUDIO_SPDIF = 1,
+	DW_DP_AUDIO_UNUSED,
+};
+
 struct dw_dp {
 	struct drm_bridge bridge;
 	struct device *dev;
@@ -320,6 +343,8 @@ struct dw_dp {
 	int irq;
 	struct work_struct hpd_work;
 	struct dw_dp_hotplug hotplug;
+	enum dw_dp_audio_interface audio_interface;
+	int audio_channels;
 	/* Serialize hpd status access */
 	struct mutex irq_lock;
 
@@ -1846,6 +1871,195 @@ static void dw_dp_bridge_oob_notify(struct drm_bridge *bridge,
 		dev_err_once(dp->dev, "Missing platform handler for OOB HPD handling\n");
 }
 
+static int dw_dp_audio_infoframe_send(struct dw_dp *dp)
+{
+	struct hdmi_audio_infoframe frame;
+	struct dw_dp_sdp sdp;
+	int ret;
+
+	ret = hdmi_audio_infoframe_init(&frame);
+	if (ret < 0)
+		return ret;
+
+	frame.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
+	frame.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
+	frame.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
+	frame.channels = dp->audio_channels;
+
+	ret = hdmi_audio_infoframe_pack_for_dp(&frame, &sdp.base, DW_DP_SDP_VERSION);
+	if (ret < 0)
+		return ret;
+
+	sdp.flags = DW_DP_SDP_VERTICAL_INTERVAL;
+
+	return dw_dp_send_sdp(dp, &sdp);
+}
+
+static int dw_dp_audio_startup(struct drm_bridge *bridge,
+			       struct drm_connector *connector)
+{
+	struct dw_dp *dp = bridge_to_dp(bridge);
+
+	dev_dbg(dp->dev, "audio startup\n");
+
+	return pm_runtime_resume_and_get(dp->dev);
+}
+
+static void dw_dp_audio_unprepare(struct drm_bridge *bridge,
+				  struct drm_connector *connector)
+{
+	struct dw_dp *dp = bridge_to_dp(bridge);
+
+	/* Disable all audio streams */
+	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1, AUDIO_DATA_IN_EN,
+			   FIELD_PREP(AUDIO_DATA_IN_EN, 0));
+
+	if (dp->audio_interface == DW_DP_AUDIO_SPDIF)
+		clk_disable_unprepare(dp->spdif_clk);
+	else if (dp->audio_interface == DW_DP_AUDIO_I2S)
+		clk_disable_unprepare(dp->i2s_clk);
+
+	dp->audio_interface = DW_DP_AUDIO_UNUSED;
+}
+
+static int dw_dp_audio_prepare(struct drm_bridge *bridge,
+			       struct drm_connector *connector,
+			       struct hdmi_codec_daifmt *daifmt,
+			       struct hdmi_codec_params *params)
+{
+	struct dw_dp *dp = bridge_to_dp(bridge);
+	u8 audio_data_in_en, supported_audio_interfaces;
+	enum dw_dp_audio_interface audio_interface;
+	u32 cfg1;
+	int ret;
+
+	/*
+	 * prepare might be called multiple times, so release the clocks
+	 * from previous calls to keep the calls in balance.
+	 */
+	if (dp->audio_interface != DW_DP_AUDIO_UNUSED)
+		dw_dp_audio_unprepare(bridge, connector);
+
+	dp->audio_channels = params->cea.channels;
+	switch (params->cea.channels) {
+	case 1:
+	case 2:
+		audio_data_in_en = AUDIO_DATA_IN_EN_CHANNEL12;
+		break;
+	case 8:
+		audio_data_in_en = AUDIO_DATA_IN_EN_CHANNEL12 |
+				   AUDIO_DATA_IN_EN_CHANNEL34 |
+				   AUDIO_DATA_IN_EN_CHANNEL56 |
+				   AUDIO_DATA_IN_EN_CHANNEL78;
+		break;
+	default:
+		dev_err(dp->dev, "invalid audio channels %d\n", dp->audio_channels);
+		return -EINVAL;
+	}
+
+	switch (daifmt->fmt) {
+	case HDMI_SPDIF:
+		audio_interface = DW_DP_AUDIO_SPDIF;
+		break;
+	case HDMI_I2S:
+		/*
+		 * It is recommended to use SPDIF instead of I2S, since I2S mode requires
+		 * manually inserting PCUV control bits from userspace and this is done
+		 * automatically in hardware for SPDIF mode.
+		 */
+		audio_interface = DW_DP_AUDIO_I2S;
+		break;
+	default:
+		dev_err(dp->dev, "invalid DAI format %d\n", daifmt->fmt);
+		return -EINVAL;
+	}
+
+	regmap_read(dp->regmap, DW_DP_CONFIG_REG1, &cfg1);
+	supported_audio_interfaces = FIELD_GET(AUDIO_SELECT, cfg1);
+
+	if (supported_audio_interfaces != DW_DP_AUDIO_I2S_AND_SPDIF &&
+	    supported_audio_interfaces != audio_interface) {
+		dev_err(dp->dev, "unsupported DAI %d\n", daifmt->fmt);
+		return -EINVAL;
+	}
+
+	ret = clk_prepare_enable(dp->spdif_clk);
+	if (ret)
+		return ret;
+
+	clk_prepare_enable(dp->i2s_clk);
+	if (ret) {
+		clk_disable_unprepare(dp->spdif_clk);
+		return ret;
+	}
+
+	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1,
+			   AUDIO_DATA_IN_EN | NUM_CHANNELS | AUDIO_DATA_WIDTH |
+			   AUDIO_INF_SELECT | HBR_MODE_ENABLE,
+			   FIELD_PREP(AUDIO_DATA_IN_EN, audio_data_in_en) |
+			   FIELD_PREP(NUM_CHANNELS, dp->audio_channels - 1) |
+			   FIELD_PREP(AUDIO_DATA_WIDTH, params->sample_width) |
+			   FIELD_PREP(AUDIO_INF_SELECT, audio_interface) |
+			   FIELD_PREP(HBR_MODE_ENABLE, 0));
+
+	/* Wait for inf switch */
+	usleep_range(20, 40);
+
+	if (audio_interface == DW_DP_AUDIO_I2S)
+		clk_disable_unprepare(dp->spdif_clk);
+	else if (audio_interface == DW_DP_AUDIO_SPDIF)
+		clk_disable_unprepare(dp->i2s_clk);
+
+	dp->audio_interface = audio_interface;
+
+	/*
+	 * Send audio stream during vertical and horizontal blanking periods.
+	 * Send out audio timestamp SDP once per video frame during the vertical
+	 * blanking period
+	 */
+	regmap_update_bits(dp->regmap, DW_DP_SDP_VERTICAL_CTRL,
+			   EN_AUDIO_STREAM_SDP | EN_AUDIO_TIMESTAMP_SDP,
+			   FIELD_PREP(EN_AUDIO_STREAM_SDP, 1) |
+			   FIELD_PREP(EN_AUDIO_TIMESTAMP_SDP, 1));
+	regmap_update_bits(dp->regmap, DW_DP_SDP_HORIZONTAL_CTRL,
+			   EN_AUDIO_STREAM_SDP,
+			   FIELD_PREP(EN_AUDIO_STREAM_SDP, 1));
+
+	ret = dw_dp_audio_infoframe_send(dp);
+	if (ret < 0)
+		dev_err(dp->dev, "failed to send audio infoframe\n");
+
+	dev_dbg(dp->dev, "audio prepare with %d channels using DAI=%d\n",
+		dp->audio_channels, dp->audio_interface);
+
+	return 0;
+}
+
+static void dw_dp_audio_shutdown(struct drm_bridge *bridge,
+				 struct drm_connector *connector)
+{
+	struct dw_dp *dp = bridge_to_dp(bridge);
+
+	dev_dbg(dp->dev, "audio shutdown\n");
+
+	dw_dp_audio_unprepare(bridge, connector);
+	pm_runtime_put_autosuspend(dp->dev);
+}
+
+static int dw_dp_audio_mute_stream(struct drm_bridge *bridge,
+				   struct drm_connector *connector,
+				   bool enable, int direction)
+{
+	struct dw_dp *dp = bridge_to_dp(bridge);
+
+	dev_dbg(dp->dev, "audio %smute\n", enable ? "" : "un");
+
+	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1, AUDIO_MUTE,
+			   FIELD_PREP(AUDIO_MUTE, enable));
+
+	return 0;
+}
+
 static const struct drm_bridge_funcs dw_dp_bridge_funcs = {
 	.atomic_duplicate_state = dw_dp_bridge_atomic_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -1859,6 +2073,11 @@ static const struct drm_bridge_funcs dw_dp_bridge_funcs = {
 	.detect = dw_dp_bridge_detect,
 	.edid_read = dw_dp_bridge_edid_read,
 	.oob_notify = dw_dp_bridge_oob_notify,
+
+	.dp_audio_startup = dw_dp_audio_startup,
+	.dp_audio_prepare = dw_dp_audio_prepare,
+	.dp_audio_shutdown = dw_dp_audio_shutdown,
+	.dp_audio_mute_stream = dw_dp_audio_mute_stream,
 };
 
 static int dw_dp_link_retrain(struct dw_dp *dp)
@@ -2009,10 +2228,19 @@ int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
 
 	bridge = &dp->bridge;
 	bridge->of_node = dev->of_node;
-	bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
+	bridge->ops = DRM_BRIDGE_OP_DP_AUDIO |
+		      DRM_BRIDGE_OP_DETECT |
+		      DRM_BRIDGE_OP_EDID |
+		      DRM_BRIDGE_OP_HPD;
 	bridge->type = DRM_MODE_CONNECTOR_DisplayPort;
 	bridge->ycbcr_420_allowed = true;
 
+	dp->audio_interface = DW_DP_AUDIO_UNUSED;
+	bridge->hdmi_audio_dev = dev;
+	bridge->hdmi_audio_max_i2s_playback_channels = 8;
+	bridge->hdmi_audio_dai_port = 1;
+	bridge->hdmi_audio_spdif_playback = true;
+
 	drm_bridge_add(bridge);
 
 	dp->aux.dev = dev;

-- 
2.53.0

Re: [PATCH v4 10/10] drm/bridge: synopsys: dw-dp: Add audio support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Audio infoframe preparation permanently leaks an SDP register bank slot on every call, leading to exhaustion of slots and duplicated Audio InfoFrames.
- [Medium] The return value of `clk_prepare_enable(dp->i2s_clk)` is not assigned to `ret`, rendering the subsequent error check dead code and ignoring failures.
- [Medium] TOCTOU race condition in `dw_dp_send_sdp` for SDP register bank allocation when called concurrently from audio and display contexts.
- [Medium] A 32-bit audio sample width will be silently truncated to 0 when configured in the hardware register via FIELD_PREP.
- [Medium] Incomplete channel count support blocks valid ALSA streams from playing.
--

--- Patch [10]: [PATCH v4 10/10] drm/bridge: synopsys: dw-dp: Add audio support ---
commit 9935aa2c50aed1038e27452a987227adb8e93b00
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Add audio support

Implement audio support for the Synopsys DesignWare DisplayPort
controller.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 636b9e1052978..8d6a52135e54d 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1846,6 +1871,195 @@ static void dw_dp_bridge_oob_notify(struct drm_bridge *bridge,
>  		dev_err_once(dp->dev, "Missing platform handler for OOB HPD handling\n");
>  }
>  
> +static int dw_dp_audio_infoframe_send(struct dw_dp *dp)
> +{
> +	struct hdmi_audio_infoframe frame;
> +	struct dw_dp_sdp sdp;
> +	int ret;
> +
> +	ret = hdmi_audio_infoframe_init(&frame);
> +	if (ret < 0)
> +		return ret;
> +
> +	frame.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
> +	frame.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
> +	frame.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
> +	frame.channels = dp->audio_channels;
> +
> +	ret = hdmi_audio_infoframe_pack_for_dp(&frame, &sdp.base, DW_DP_SDP_VERSION);
> +	if (ret < 0)
> +		return ret;
> +
> +	sdp.flags = DW_DP_SDP_VERTICAL_INTERVAL;
> +
> +	return dw_dp_send_sdp(dp, &sdp);

[Severity: Medium]
Is there a race condition when allocating SDP register bank slots? 

When dw_dp_audio_infoframe_send() is called, it routes to dw_dp_send_sdp() 
which performs a non-atomic bitmap allocation on the shared dp->sdp_reg_bank 
state:

    nr = find_first_zero_bit(dp->sdp_reg_bank, SDP_REG_BANK_SIZE);
    if (nr < SDP_REG_BANK_SIZE)
        set_bit(nr, dp->sdp_reg_bank);

If a DRM atomic commit and an ALSA stream start execute concurrently on 
different CPUs, could they allocate the same SDP slot and corrupt the 
hardware configuration?

> +}
> +
> +static int dw_dp_audio_startup(struct drm_bridge *bridge,
> +			       struct drm_connector *connector)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +
> +	dev_dbg(dp->dev, "audio startup\n");
> +
> +	return pm_runtime_resume_and_get(dp->dev);
> +}
> +
> +static void dw_dp_audio_unprepare(struct drm_bridge *bridge,
> +				  struct drm_connector *connector)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +
> +	/* Disable all audio streams */
> +	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1, AUDIO_DATA_IN_EN,
> +			   FIELD_PREP(AUDIO_DATA_IN_EN, 0));

[Severity: High]
Does dw_dp_audio_unprepare() miss freeing the SDP register bank slot 
allocated during audio prepare? 

When dw_dp_audio_infoframe_send() calls dw_dp_send_sdp(), a slot in 
dp->sdp_reg_bank is allocated and the corresponding EN_VERTICAL_SDP 
hardware bits are set. 

Since the cleanup path here doesn't clear that bit or disable the hardware 
bits, won't repeatedly starting and stopping an ALSA stream exhaust all 16 
hardware slots and permanently break audio infoframe updates with -EBUSY?

> +
> +	if (dp->audio_interface == DW_DP_AUDIO_SPDIF)
> +		clk_disable_unprepare(dp->spdif_clk);

[ ... ]

> +static int dw_dp_audio_prepare(struct drm_bridge *bridge,
> +			       struct drm_connector *connector,
> +			       struct hdmi_codec_daifmt *daifmt,
> +			       struct hdmi_codec_params *params)
> +{
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +	u8 audio_data_in_en, supported_audio_interfaces;
> +	enum dw_dp_audio_interface audio_interface;
> +	u32 cfg1;
> +	int ret;
> +
> +	/*
> +	 * prepare might be called multiple times, so release the clocks
> +	 * from previous calls to keep the calls in balance.
> +	 */
> +	if (dp->audio_interface != DW_DP_AUDIO_UNUSED)
> +		dw_dp_audio_unprepare(bridge, connector);
> +
> +	dp->audio_channels = params->cea.channels;
> +	switch (params->cea.channels) {
> +	case 1:
> +	case 2:
> +		audio_data_in_en = AUDIO_DATA_IN_EN_CHANNEL12;
> +		break;
> +	case 8:
> +		audio_data_in_en = AUDIO_DATA_IN_EN_CHANNEL12 |
> +				   AUDIO_DATA_IN_EN_CHANNEL34 |
> +				   AUDIO_DATA_IN_EN_CHANNEL56 |
> +				   AUDIO_DATA_IN_EN_CHANNEL78;
> +		break;
> +	default:
> +		dev_err(dp->dev, "invalid audio channels %d\n", dp->audio_channels);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Does this switch statement need to handle 4 and 6 channel audio streams? 

Since the bridge advertises support for up to 8 channels via 
hdmi_audio_max_i2s_playback_channels = 8, the ALSA framework might 
negotiate 4.0 or 5.1 surround sound configurations. 

It seems these valid channel counts would be rejected here with -EINVAL, 
blocking their playback.

> +
> +	switch (daifmt->fmt) {

[ ... ]

> +	ret = clk_prepare_enable(dp->spdif_clk);
> +	if (ret)
> +		return ret;
> +
> +	clk_prepare_enable(dp->i2s_clk);
> +	if (ret) {

[Severity: Medium]
Does this ignore the return value of clk_prepare_enable() for the I2S clock? 

Since the result isn't assigned to ret, the subsequent error check appears 
to be evaluating the return value from the previous spdif_clk enable call 
(which is already known to be 0). 

If the I2S clock fails to enable, it looks like this will silently proceed 
with an unclocked interface and cause an unbalanced clock state when 
dw_dp_audio_unprepare() is called.

> +		clk_disable_unprepare(dp->spdif_clk);
> +		return ret;
> +	}
> +
> +	regmap_update_bits(dp->regmap, DW_DP_AUD_CONFIG1,
> +			   AUDIO_DATA_IN_EN | NUM_CHANNELS | AUDIO_DATA_WIDTH |
> +			   AUDIO_INF_SELECT | HBR_MODE_ENABLE,
> +			   FIELD_PREP(AUDIO_DATA_IN_EN, audio_data_in_en) |
> +			   FIELD_PREP(NUM_CHANNELS, dp->audio_channels - 1) |
> +			   FIELD_PREP(AUDIO_DATA_WIDTH, params->sample_width) |

[Severity: Medium]
Can a 32-bit audio sample width get silently truncated here? 

The AUDIO_DATA_WIDTH mask is defined as GENMASK(9, 5) which is 5 bits wide. 
If userspace negotiates a 32-bit audio stream (which is allowed by default), 
the value 32 (binary 100000, 6 bits) will be truncated to 0 by FIELD_PREP, 
potentially misconfiguring the hardware.

> +			   FIELD_PREP(AUDIO_INF_SELECT, audio_interface) |
> +			   FIELD_PREP(HBR_MODE_ENABLE, 0));
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=10