[PATCH 00/24] drm/msm: introduce the struct msm_display interface

[PATCH 00/24] drm/msm: introduce the struct msm_display interface

[Dmitry Baryshkov]

The msm KMS core and its DPU/MDP5/MDP4 backends reach into the concrete
DSI, DP and HDMI sub-blocks all over the place. Connector creation is
open-coded in each backend and in each sub-block, the bonded-DSI dispatch
is duplicated three times with subtle variations, and every cross-module
query (command mode, wide bus, DSC config, tear-check source, peripheral
flush, snapshot, ...) is a type-specific msm_dsi_*()/msm_dp_*()/
msm_hdmi_*() helper reached through an interface-type switch. In the
other direction the sub-blocks cache drm_connector pointers that the core
has to hand back to them, coupling each sub-block to a connector it does
not own.

Refactor the MSM display / encoder interface. Introduce a small struct
msm_display interface, embedded in struct msm_dsi, struct msm_dp and
struct hdmi, and fold the per-sub-block operations into it one callback
at a time: modeset_init(), snapshot(), the wide-bus / command-mode / DSC
/ TE-source / peripheral-flush / bonded / encoder queries. Once every
cross-module call goes through the interface, struct msm_kms can store
the sub-blocks as struct msm_display pointers and drop the concrete
accessors entirely.

On top of that, move connector creation into common code: each backend
now only builds the (genuinely backend-specific) encoder and attaches the
bridge chain, and a shared msm_kms_init_connectors() walks the encoders
once the bridges are in place and creates a bridge connector for each.
The last DisplayPort-specific bit, attaching the DP subconnector
property, is generalised into drm_bridge_connector_init() so it is
applied to any DisplayPort bridge connector rather than open-coded in
msm.

The series incorporates two patches by Yongxing Mou from [1], one patch
is added as is, another one has minor fix.

[1] https://lore.kernel.org/dri-devel/20260720-dp_mstclean-v8-0-bea261bf4e7d@oss.qualcomm.com/

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
Dmitry Baryshkov (22):
      drm/msm/dp: reject YUV420-only modes without VSC SDP support
      drm/msm/dp: drop the always-true yuv_supported argument
      drm/msm: create the display connectors from common code
      drm/msm: introduce the struct msm_display interface
      drm/msm: route the display snapshot through the msm_display interface
      drm/msm/hdmi: capture the HDMI registers in the display snapshot
      drm/msm: add the wide_bus_enabled callback to msm_display
      drm/msm: add the needs_periph_flush callback to msm_display
      drm/msm: add the is_cmd_mode callback to msm_display
      drm/msm: add the get_dsc_config callback to msm_display
      drm/msm: add the get_te_source callback to msm_display
      drm/msm: add is_bonded and needs_encoder callbacks to msm_display
      drm/msm/hdmi: use dev_get_drvdata() in msm_hdmi_unbind()
      drm/msm: store the display sub-blocks as struct msm_display
      drm/msm/dp: do not reject wide-bus modes while a YUV420 mode is active
      drm/msm/hdmi: cache is_hdmi instead of storing the connector
      drm/msm/dp: drop redundant panel->connector
      drm/msm/dp: use drm_display_info in mode_valid callbacks
      drm/msm/dp: guard subconnector setup on the connector type
      drm/msm/dp: stop storing the connector in struct msm_dp
      drm/msm: create the bridge connectors from common code
      drm/bridge-connector: attach the DP subconnector property

Yongxing Mou (2):
      drm/msm/dp: remove cached drm_edid from panel
      drm/msm/dp: drop deprecated .mode_set() and use .atomic_pre_enable

 drivers/gpu/drm/display/drm_bridge_connector.c    |   4 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c       |  64 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |  52 +---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c          |  26 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c          |  11 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c |  19 +-
 drivers/gpu/drm/msm/dp/dp_display.c               | 353 +++++++++++++---------
 drivers/gpu/drm/msm/dp/dp_display.h               |   7 +-
 drivers/gpu/drm/msm/dp/dp_drm.c                   |  30 +-
 drivers/gpu/drm/msm/dp/dp_drm.h                   |  10 +-
 drivers/gpu/drm/msm/dp/dp_panel.c                 |  62 +---
 drivers/gpu/drm/msm/dp/dp_panel.h                 |  15 +-
 drivers/gpu/drm/msm/dsi/dsi.c                     |  71 ++++-
 drivers/gpu/drm/msm/dsi/dsi.h                     |   8 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c             |  18 +-
 drivers/gpu/drm/msm/hdmi/hdmi.c                   | 101 +++++--
 drivers/gpu/drm/msm/hdmi/hdmi.h                   |   7 +-
 drivers/gpu/drm/msm/hdmi/hdmi_audio.c             |   2 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c            |  16 +-
 drivers/gpu/drm/msm/msm_drv.h                     | 135 +++------
 drivers/gpu/drm/msm/msm_kms.c                     |  90 ++++++
 drivers/gpu/drm/msm/msm_kms.h                     |   9 +-
 22 files changed, 636 insertions(+), 474 deletions(-)
---
base-commit: b2128290c29902315e632ea59e0504d6bc9e9b42
change-id: 20260722-drm-msm-display-interface-5bccec4b69d5

Best regards,
--  
With best wishes
Dmitry

[PATCH 01/24] drm/msm/dp: reject YUV420-only modes without VSC SDP support

[Dmitry Baryshkov]

DP conveys YUV 420 colorimetry through a VSC SDP. A sink that advertises
a mode as YUV-420-only therefore cannot be driven at all unless the panel
supports VSC SDP, yet msm_dp_bridge_mode_valid() only used the VSC SDP
capability to decide whether to halve the pixel clock, otherwise letting
such modes through to be validated (and possibly accepted) at the full
RGB clock the sink cannot display.

Reject 420-only modes with MODE_NO_420 when the panel does not support
VSC SDP. With those modes filtered out, being a 420-only mode implies VSC
SDP support, so the YUV-420 test reduces to drm_mode_is_420_only(): drop
msm_dp_is_yuv_420_enabled() and call the DRM helper directly at its two
callers (the DPU encoder already has the connector from the atomic state).

Fixes: df9cf852ca30 ("drm/msm/dp: account for widebus and yuv420 during mode validation")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  3 +--
 drivers/gpu/drm/msm/dp/dp_display.c         | 28 ++++++++++++----------------
 drivers/gpu/drm/msm/msm_drv.h               |  8 --------
 3 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 778e231d4967..1f20695f81e3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -710,8 +710,7 @@ void dpu_encoder_update_topology(struct drm_encoder *drm_enc,
 		if (fb && MSM_FORMAT_IS_YUV(msm_framebuffer_format(fb)))
 			topology->num_cdm++;
 	} else if (disp_info->intf_type == INTF_DP) {
-		if (msm_dp_is_yuv_420_enabled(priv->kms->dp[disp_info->h_tile_instance[0]],
-					      adj_mode))
+		if (drm_mode_is_420_only(&connector->display_info, adj_mode))
 			topology->num_cdm++;
 	}
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index dc6f33809ca5..e0c44eef3aba 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -698,6 +698,7 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
 	u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
 	struct msm_dp *dp;
 	int mode_pclk_khz = mode->clock;
+	bool is_yuv_420;
 
 	dp = to_dp_bridge(bridge)->msm_dp_display;
 
@@ -709,9 +710,16 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 	link_info = &msm_dp_display->panel->link_info;
 
-	if ((drm_mode_is_420_only(&dp->connector->display_info, mode) &&
-	     msm_dp_display->panel->vsc_sdp_supported) ||
-	     msm_dp_wide_bus_available(dp))
+	is_yuv_420 = drm_mode_is_420_only(&dp->connector->display_info, mode);
+
+	/*
+	 * YUV 420 is carried over DP by signalling the colorimetry through a
+	 * VSC SDP, so a 420-only mode cannot be driven without VSC SDP support.
+	 */
+	if (is_yuv_420 && !msm_dp_display->panel->vsc_sdp_supported)
+		return MODE_NO_420;
+
+	if (is_yuv_420 || msm_dp_wide_bus_available(dp))
 		mode_pclk_khz /= 2;
 
 	if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)
@@ -1277,22 +1285,10 @@ void __exit msm_dp_unregister(void)
 	platform_driver_unregister(&msm_dp_display_driver);
 }
 
-bool msm_dp_is_yuv_420_enabled(const struct msm_dp *msm_dp_display,
-			       const struct drm_display_mode *mode)
-{
-	struct msm_dp_display_private *dp;
-	const struct drm_display_info *info;
-
-	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
-	info = &msm_dp_display->connector->display_info;
-
-	return dp->panel->vsc_sdp_supported && drm_mode_is_420_only(info, mode);
-}
-
 bool msm_dp_needs_periph_flush(const struct msm_dp *msm_dp_display,
 			       const struct drm_display_mode *mode)
 {
-	return msm_dp_is_yuv_420_enabled(msm_dp_display, mode);
+	return drm_mode_is_420_only(&msm_dp_display->connector->display_info, mode);
 }
 
 bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 3787db8770ad..3d5679be488e 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -356,8 +356,6 @@ void __exit msm_dp_unregister(void);
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			 struct drm_encoder *encoder, bool yuv_supported);
 void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
-bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display,
-			       const struct drm_display_mode *mode);
 bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
 			       const struct drm_display_mode *mode);
 bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
@@ -382,12 +380,6 @@ static inline void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm
 {
 }
 
-static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display,
-					     const struct drm_display_mode *mode)
-{
-	return false;
-}
-
 static inline bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
 					     const struct drm_display_mode *mode)
 {

-- 
2.47.3

Re: [PATCH 01/24] drm/msm/dp: reject YUV420-only modes without VSC SDP support

[sashiko-bot]

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

New issues:
- [High] The patch relies on mode filtering to replace explicit VSC SDP support checks, but fails to apply this filtering to eDP, causing a DPU/DP state mismatch.

Pre-existing issues:
- [High] msm_dp_wide_bus_available() incorrectly uses the currently active hardware state to determine if wide bus can be used for a proposed mode, potentially rejecting valid high-bandwidth modes.
--

--- Patch [1]: [PATCH 01/24] drm/msm/dp: reject YUV420-only modes without VSC SDP support ---
commit 978b7d550fcfcaed1db8ec32849e99ffcd2880a7
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dp: reject YUV420-only modes without VSC SDP support

This patch updates DP mode validation to explicitly reject YUV420-only
modes if the panel lacks VSC SDP support. By filtering these out, the
DPU encoder logic is simplified to assume any validated 420-only mode
has VSC SDP support.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 778e231d49677..1f20695f81e35 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -710,8 +710,7 @@ void dpu_encoder_update_topology(struct drm_encoder *drm_enc,
>  		if (fb && MSM_FORMAT_IS_YUV(msm_framebuffer_format(fb)))
>  			topology->num_cdm++;
>  	} else if (disp_info->intf_type == INTF_DP) {
> -		if (msm_dp_is_yuv_420_enabled(priv->kms->dp[disp_info->h_tile_instance[0]],
> -					      adj_mode))
> +		if (drm_mode_is_420_only(&connector->display_info, adj_mode))
>  			topology->num_cdm++;

[Severity: High]
Does this change break mode validation for eDP displays by assuming VSC SDP
support is always present for 420-only modes?

This patch adds mode filtering to msm_dp_bridge_mode_valid() to reject modes
without VSC SDP, so that dpu_encoder_update_topology() can assume all
420-only modes are fully supported.

However, the eDP validation function, msm_edp_bridge_mode_valid(), does not
appear to have the same filtering logic applied.

If an eDP sink advertises a YUV420-only mode without VSC SDP support, will it
pass validation, causing dpu_encoder_update_topology() to allocate a CDM
block (expecting YUV420) while the DP controller is still configured for RGB?

>  	}
>  }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index dc6f33809ca5f..e0c44eef3abab 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -709,9 +710,16 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
>  	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
>  	link_info = &msm_dp_display->panel->link_info;
>  
> -	if ((drm_mode_is_420_only(&dp->connector->display_info, mode) &&
> -	     msm_dp_display->panel->vsc_sdp_supported) ||
> -	     msm_dp_wide_bus_available(dp))
> +	is_yuv_420 = drm_mode_is_420_only(&dp->connector->display_info, mode);
> +
> +	/*
> +	 * YUV 420 is carried over DP by signalling the colorimetry through a
> +	 * VSC SDP, so a 420-only mode cannot be driven without VSC SDP support.
> +	 */
> +	if (is_yuv_420 && !msm_dp_display->panel->vsc_sdp_supported)
> +		return MODE_NO_420;
> +
> +	if (is_yuv_420 || msm_dp_wide_bus_available(dp))
>  		mode_pclk_khz /= 2;

[Severity: High]
This is a pre-existing issue, but does msm_dp_wide_bus_available() check
the active hardware state rather than the proposed mode?

During mode validation in msm_dp_bridge_mode_valid(), the code checks:
	if (is_yuv_420 || msm_dp_wide_bus_available(dp))

However, msm_dp_wide_bus_available() evaluates the currently active modeset
instead of the proposed mode:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_wide_bus_available() {
	...
	if (dp->msm_dp_mode.out_fmt_is_yuv_420)
		return false;
	...
}

If userspace probes a high-bandwidth RGB mode while the current display state
is YUV420, could this incorrectly restrict hardware capability based on
the transient state and improperly reject valid modes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=1

[PATCH 02/24] drm/msm/dp: drop the always-true yuv_supported argument

[Dmitry Baryshkov]

msm_dp_modeset_init() and msm_dp_bridge_init() take a yuv_supported
argument, which the DPU driver fills in from the presence of a CDM block
in the hardware catalog. All platforms that support DisplayPort have a
CDM block, so this argument is always true and only obscures the code.

Drop the argument and set the bridge's ycbcr_420_allowed flag
unconditionally.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +---
 drivers/gpu/drm/msm/dp/dp_display.c     | 4 ++--
 drivers/gpu/drm/msm/dp/dp_drm.c         | 5 +++--
 drivers/gpu/drm/msm/dp/dp_drm.h         | 3 +--
 drivers/gpu/drm/msm/msm_drv.h           | 5 ++---
 5 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index da3556eb6ecc..ede935a426d1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -653,7 +653,6 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 {
 	struct drm_encoder *encoder = NULL;
 	struct msm_display_info info;
-	bool yuv_supported;
 	int rc;
 	int i;
 
@@ -672,8 +671,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 			return PTR_ERR(encoder);
 		}
 
-		yuv_supported = !!dpu_kms->catalog->cdm;
-		rc = msm_dp_modeset_init(priv->kms->dp[i], dev, encoder, yuv_supported);
+		rc = msm_dp_modeset_init(priv->kms->dp[i], dev, encoder);
 		if (rc) {
 			DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
 			return rc;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index e0c44eef3aba..5ae5eed7e010 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1318,7 +1318,7 @@ void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *r
 }
 
 int msm_dp_modeset_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
-			struct drm_encoder *encoder, bool yuv_supported)
+			struct drm_encoder *encoder)
 {
 	struct msm_dp_display_private *msm_dp_priv;
 	int ret;
@@ -1327,7 +1327,7 @@ int msm_dp_modeset_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
 
 	msm_dp_priv = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
 
-	ret = msm_dp_bridge_init(msm_dp_display, dev, encoder, yuv_supported);
+	ret = msm_dp_bridge_init(msm_dp_display, dev, encoder);
 	if (ret) {
 		DRM_DEV_ERROR(dev->dev,
 			"failed to create dp bridge: %d\n", ret);
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 06881bfd6181..4f615b45fff7 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -243,7 +243,7 @@ static const struct drm_bridge_funcs msm_edp_bridge_ops = {
 };
 
 int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
-		   struct drm_encoder *encoder, bool yuv_supported)
+		   struct drm_encoder *encoder)
 {
 	int rc;
 	struct msm_dp_bridge *msm_dp_bridge;
@@ -259,7 +259,8 @@ int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
 
 	bridge = &msm_dp_bridge->bridge;
 	bridge->type = msm_dp_display->connector_type;
-	bridge->ycbcr_420_allowed = yuv_supported;
+	/* all platforms which support DP have a CDM block for YUV output */
+	bridge->ycbcr_420_allowed = true;
 
 	/*
 	 * Many ops only make sense for DP. Why?
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index 041aa026ae2e..3b046822f05e 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -22,8 +22,7 @@ struct msm_dp_bridge {
 struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
 					    struct drm_encoder *encoder);
 int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
-		   struct drm_encoder *encoder,
-		   bool yuv_supported);
+		   struct drm_encoder *encoder);
 
 enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
 					       struct drm_connector *connector);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 3d5679be488e..0ec0622d9217 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -354,7 +354,7 @@ struct msm_dp;
 int __init msm_dp_register(void);
 void __exit msm_dp_unregister(void);
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
-			 struct drm_encoder *encoder, bool yuv_supported);
+			 struct drm_encoder *encoder);
 void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
 bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
 			       const struct drm_display_mode *mode);
@@ -370,8 +370,7 @@ static inline void __exit msm_dp_unregister(void)
 }
 static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
 				       struct drm_device *dev,
-				       struct drm_encoder *encoder,
-				       bool yuv_supported)
+				       struct drm_encoder *encoder)
 {
 	return -EINVAL;
 }

-- 
2.47.3

[PATCH 03/24] drm/msm: create the display connectors from common code

[Dmitry Baryshkov]

Each KMS backend (mdp4, mdp5, dpu1) open-codes the loop over the DSI, DP
and HDMI sub-blocks and the calls to msm_dsi_modeset_init(),
msm_dp_modeset_init() and msm_hdmi_modeset_init() that build the bridges
and connectors. The dispatch, including the bonded-DSI handling, is
duplicated three times with subtle variations.

The encoder each sub-block is attached to is genuinely backend-specific
(the DPU builds a display-info descriptor, MDP5 requests a CTL, MDP4 uses
fixed CRTC masks), but the connector setup that follows is not. Have each
backend only create the encoder and record it in struct msm_kms, and move
the msm_*_modeset_init() dispatch into a common msm_kms_init_connectors()
called from msm_drm_kms_init() after the backend's ->hw_init(), where the
encoders and, on DPU, the GEM address space (kms->vm) that
msm_dsi_host_modeset_init() allocates the DSI TX buffer from are set up.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 34 ++++--------------
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 18 +++-------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  9 +++--
 drivers/gpu/drm/msm/msm_kms.c            | 60 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_kms.h            |  3 ++
 5 files changed, 78 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ede935a426d1..42dff1a98292 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -627,21 +627,11 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 			return PTR_ERR(encoder);
 		}
 
-		rc = msm_dsi_modeset_init(priv->kms->dsi[i], dev, encoder);
-		if (rc) {
-			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
-				i, rc);
-			break;
-		}
-
-		if (msm_dsi_is_bonded_dsi(priv->kms->dsi[i]) && priv->kms->dsi[other]) {
-			rc = msm_dsi_modeset_init(priv->kms->dsi[other], dev, encoder);
-			if (rc) {
-				DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
-					other, rc);
-				break;
-			}
-		}
+		/*
+		 * A bonded pair is driven by a single encoder; the connector
+		 * setup for both hosts is handled from common code later.
+		 */
+		priv->kms->dsi_encoder[i] = encoder;
 	}
 
 	return rc;
@@ -653,7 +643,6 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 {
 	struct drm_encoder *encoder = NULL;
 	struct msm_display_info info;
-	int rc;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(priv->kms->dp); i++) {
@@ -671,11 +660,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 			return PTR_ERR(encoder);
 		}
 
-		rc = msm_dp_modeset_init(priv->kms->dp[i], dev, encoder);
-		if (rc) {
-			DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
-			return rc;
-		}
+		priv->kms->dp_encoder[i] = encoder;
 	}
 
 	return 0;
@@ -687,7 +672,6 @@ static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
 {
 	struct drm_encoder *encoder = NULL;
 	struct msm_display_info info;
-	int rc;
 
 	if (!priv->kms->hdmi)
 		return 0;
@@ -703,11 +687,7 @@ static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
 		return PTR_ERR(encoder);
 	}
 
-	rc = msm_hdmi_modeset_init(priv->kms->hdmi, dev, encoder);
-	if (rc) {
-		DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
-		return rc;
-	}
+	priv->kms->hdmi_encoder = encoder;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 7726edb0d4ed..cd566a287f6f 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -243,14 +243,9 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms,
 		/* DTV can be hooked to DMA_E: */
 		encoder->possible_crtcs = 1 << 1;
 
-		if (priv->kms->hdmi) {
-			/* Construct bridge/connector for HDMI: */
-			ret = msm_hdmi_modeset_init(priv->kms->hdmi, dev, encoder);
-			if (ret) {
-				DRM_DEV_ERROR(dev->dev, "failed to initialize HDMI: %d\n", ret);
-				return ret;
-			}
-		}
+		/* the bridge/connector for HDMI is set up from common code */
+		if (priv->kms->hdmi)
+			priv->kms->hdmi_encoder = encoder;
 
 		break;
 	case DRM_MODE_ENCODER_DSI:
@@ -271,12 +266,7 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms,
 		/* TODO: Add DMA_S later? */
 		encoder->possible_crtcs = 1 << DMA_P;
 
-		ret = msm_dsi_modeset_init(priv->kms->dsi[dsi_id], dev, encoder);
-		if (ret) {
-			DRM_DEV_ERROR(dev->dev, "failed to initialize DSI: %d\n",
-				ret);
-			return ret;
-		}
+		priv->kms->dsi_encoder[dsi_id] = encoder;
 
 		break;
 	default:
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 0a004ab9fc85..68b71a7c6666 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -330,7 +330,7 @@ static int modeset_init_intf(struct mdp5_kms *mdp5_kms,
 			break;
 		}
 
-		ret = msm_hdmi_modeset_init(priv->kms->hdmi, dev, encoder);
+		priv->kms->hdmi_encoder = encoder;
 		break;
 	case INTF_DSI:
 	{
@@ -360,10 +360,9 @@ static int modeset_init_intf(struct mdp5_kms *mdp5_kms,
 			break;
 		}
 
-		ret = msm_dsi_modeset_init(priv->kms->dsi[dsi_id], dev, encoder);
-		if (!ret)
-			mdp5_encoder_set_intf_mode(encoder,
-						   msm_dsi_is_cmd_mode(priv->kms->dsi[dsi_id]));
+		priv->kms->dsi_encoder[dsi_id] = encoder;
+		mdp5_encoder_set_intf_mode(encoder,
+					   msm_dsi_is_cmd_mode(priv->kms->dsi[dsi_id]));
 
 		break;
 	}
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index e5d0ea629448..87530145e82b 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -260,6 +260,57 @@ void msm_drm_kms_uninit(struct device *dev)
 		kms->funcs->destroy(kms);
 }
 
+/*
+ * Set up the bridges and connectors for the display sub-blocks, using the
+ * encoders the backend created in ->kms_init().
+ */
+static int msm_kms_init_connectors(struct drm_device *ddev)
+{
+	struct msm_drm_private *priv = ddev->dev_private;
+	struct msm_kms *kms = priv->kms;
+	int i, ret;
+
+	/*
+	 * The slave link of a bonded DSI pair shares its master's encoder and
+	 * creates no connector of its own; msm_dsi_modeset_init() handles that
+	 * internally, so it needs no special-casing here.
+	 */
+	for (i = 0; i < ARRAY_SIZE(kms->dsi); i++) {
+		if (!kms->dsi[i])
+			continue;
+
+		ret = msm_dsi_modeset_init(kms->dsi[i], ddev, kms->dsi_encoder[i]);
+		if (ret) {
+			DRM_DEV_ERROR(ddev->dev,
+				      "modeset_init failed for dsi[%d]: %d\n", i, ret);
+			return ret;
+		}
+	}
+
+	for (i = 0; i < ARRAY_SIZE(kms->dp); i++) {
+		if (!kms->dp[i])
+			continue;
+
+		ret = msm_dp_modeset_init(kms->dp[i], ddev, kms->dp_encoder[i]);
+		if (ret) {
+			DRM_DEV_ERROR(ddev->dev,
+				      "modeset_init failed for dp[%d]: %d\n", i, ret);
+			return ret;
+		}
+	}
+
+	if (kms->hdmi) {
+		ret = msm_hdmi_modeset_init(kms->hdmi, ddev, kms->hdmi_encoder);
+		if (ret) {
+			DRM_DEV_ERROR(ddev->dev,
+				      "modeset_init failed for HDMI: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(dev);
@@ -298,6 +349,15 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
 		goto err_msm_uninit;
 	}
 
+	/*
+	 * The backend created the encoders for each display sub-block in
+	 * ->hw_init(); now set up the bridges and connectors for them from
+	 * common code.
+	 */
+	ret = msm_kms_init_connectors(ddev);
+	if (ret)
+		goto err_msm_uninit;
+
 	drm_helper_move_panel_connectors_to_head(ddev);
 
 	drm_for_each_crtc(crtc, ddev) {
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index f25b31e502d2..06646091604a 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -140,10 +140,13 @@ struct msm_kms {
 	struct drm_device *dev;
 
 	struct hdmi *hdmi;
+	struct drm_encoder *hdmi_encoder;
 
 	struct msm_dsi *dsi[MSM_DSI_CONTROLLER_COUNT];
+	struct drm_encoder *dsi_encoder[MSM_DSI_CONTROLLER_COUNT];
 
 	struct msm_dp *dp[MSM_DP_CONTROLLER_COUNT];
+	struct drm_encoder *dp_encoder[MSM_DP_CONTROLLER_COUNT];
 
 	/* irq number to be passed on to msm_irq_install */
 	int irq;

-- 
2.47.3

Re: [PATCH 03/24] drm/msm: create the display connectors from common code

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional calls to modeset_init functions with potentially NULL encoders causes kernel crash or probe failure.
--

--- Patch [3]: [PATCH 03/24] drm/msm: create the display connectors from common code ---
commit a0a4ea009e59812e9ecb6b60251e9294d0b779b8
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: create the display connectors from common code

This commit moves the bridge and connector setup logic from the individual
KMS backends into a common msm_kms_init_connectors() function. The backend
is now only responsible for creating the encoder, which is then used by the
common code.

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index e5d0ea6294484..87530145e82b0 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
[ ... ]
> @@ -260,6 +260,57 @@ void msm_drm_kms_uninit(struct device *dev)
>  		kms->funcs->destroy(kms);
>  }
>  
> +/*
> + * Set up the bridges and connectors for the display sub-blocks, using the
> + * encoders the backend created in ->kms_init().
> + */
> +static int msm_kms_init_connectors(struct drm_device *ddev)
> +{
> +	struct msm_drm_private *priv = ddev->dev_private;
> +	struct msm_kms *kms = priv->kms;
> +	int i, ret;
[ ... ]
> +	if (kms->hdmi) {
> +		ret = msm_hdmi_modeset_init(kms->hdmi, ddev, kms->hdmi_encoder);

[Severity: High]
Will this cause a NULL pointer dereference if the encoder was not created?

If the device tree defines an HDMI node (so kms->hdmi is populated), but the
KMS backend's hardware configuration doesn't support the interface, encoder
creation is skipped and kms->hdmi_encoder remains NULL.

If msm_hdmi_modeset_init() receives a NULL encoder and there is no external
bridge, it eventually calls drm_bridge_connector_init():

drivers/gpu/drm/msm/hdmi/hdmi.c:msm_hdmi_modeset_init() {
    ...
    hdmi->connector = drm_bridge_connector_init(hdmi->dev, encoder);
    ...
}

This unconditionally dereferences the encoder to access the bridge chain:

drivers/gpu/drm/display/drm_bridge_connector.c:drm_bridge_connector_init() {
    ...
    drm_for_each_bridge_in_chain(encoder, bridge) {
    ...
}

Should there be a check ensuring the encoder was actually populated before
calling msm_hdmi_modeset_init() (and similarly for DP and DSI above this)?

> +		if (ret) {
> +			DRM_DEV_ERROR(ddev->dev,
> +				      "modeset_init failed for HDMI: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=3

[PATCH 04/24] drm/msm: introduce the struct msm_display interface

[Dmitry Baryshkov]

The common connector setup in msm_kms_init_connectors() calls the
type-specific msm_dsi_modeset_init(), msm_dp_modeset_init() and
msm_hdmi_modeset_init() functions, so it has to know about each concrete
sub-block type.

Introduce a small struct msm_display interface with a modeset_init()
callback, embedded in struct hdmi, struct msm_dsi and struct msm_dp. Each
sub-block registers its ops and provides a msm_*_get_display() accessor;
the common code then dispatches through display->funcs->modeset_init()
without knowing the concrete type. Further per-sub-block operations will
be folded into this interface in following patches.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 17 ++++++++--
 drivers/gpu/drm/msm/dp/dp_display.h |  2 ++
 drivers/gpu/drm/msm/dsi/dsi.c       | 18 ++++++++--
 drivers/gpu/drm/msm/dsi/dsi.h       |  2 ++
 drivers/gpu/drm/msm/hdmi/hdmi.c     | 13 +++++++-
 drivers/gpu/drm/msm/hdmi/hdmi.h     |  2 ++
 drivers/gpu/drm/msm/msm_drv.h       | 50 ++++++++++++++++++----------
 drivers/gpu/drm/msm/msm_kms.c       | 66 ++++++++++++++++++-------------------
 8 files changed, 115 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 5ae5eed7e010..79dd556e96c2 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -203,6 +203,8 @@ void msm_dp_display_signal_audio_complete(struct msm_dp *msm_dp_display)
 	complete_all(&dp->audio_comp);
 }
 
+static const struct msm_display_funcs msm_dp_display_funcs;
+
 static int msm_dp_display_bind(struct device *dev, struct device *master,
 			   void *data)
 {
@@ -212,6 +214,7 @@ static int msm_dp_display_bind(struct device *dev, struct device *master,
 	struct drm_device *drm = priv->dev;
 
 	dp->msm_dp_display.drm_dev = drm;
+	dp->msm_dp_display.display.funcs = &msm_dp_display_funcs;
 	priv->kms->dp[dp->id] = &dp->msm_dp_display;
 
 	dp->drm_dev = drm;
@@ -1317,9 +1320,10 @@ void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *r
 		DRM_ERROR("failed to initialize debug, rc = %d\n", rc);
 }
 
-int msm_dp_modeset_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
-			struct drm_encoder *encoder)
+static int msm_dp_modeset_init(struct msm_display *display,
+			       struct drm_device *dev, struct drm_encoder *encoder)
 {
+	struct msm_dp *msm_dp_display = container_of(display, struct msm_dp, display);
 	struct msm_dp_display_private *msm_dp_priv;
 	int ret;
 
@@ -1348,6 +1352,15 @@ int msm_dp_modeset_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
 	return 0;
 }
 
+static const struct msm_display_funcs msm_dp_display_funcs = {
+	.modeset_init = msm_dp_modeset_init,
+};
+
+struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
+{
+	return msm_dp_display ? &msm_dp_display->display : NULL;
+}
+
 void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 				 struct drm_atomic_commit *state)
 {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 0b65e16c790d..20a71afb1f90 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -12,6 +12,8 @@
 #define DP_MAX_PIXEL_CLK_KHZ	675000
 
 struct msm_dp {
+	struct msm_display display;
+
 	struct drm_device *drm_dev;
 	struct platform_device *pdev;
 	struct drm_connector *connector;
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 3c9f01ed6271..6cd844d08733 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -115,6 +115,8 @@ static struct msm_dsi *dsi_init(struct platform_device *pdev)
 	return ERR_PTR(ret);
 }
 
+static const struct msm_display_funcs msm_dsi_display_funcs;
+
 static int dsi_bind(struct device *dev, struct device *master, void *data)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(master);
@@ -136,6 +138,7 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
 		msm_dsi->next_bridge = ext_bridge;
 	}
 
+	msm_dsi->display.funcs = &msm_dsi_display_funcs;
 	priv->kms->dsi[msm_dsi->id] = msm_dsi;
 
 	return 0;
@@ -230,9 +233,11 @@ void __exit msm_dsi_unregister(void)
 	platform_driver_unregister(&dsi_driver);
 }
 
-int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
-			 struct drm_encoder *encoder)
+static int msm_dsi_modeset_init(struct msm_display *display,
+				struct drm_device *dev,
+				struct drm_encoder *encoder)
 {
+	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
 	int ret;
 
 	msm_dsi->dev = dev;
@@ -262,6 +267,15 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 	return 0;
 }
 
+static const struct msm_display_funcs msm_dsi_display_funcs = {
+	.modeset_init = msm_dsi_modeset_init,
+};
+
+struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
+{
+	return msm_dsi ? &msm_dsi->display : NULL;
+}
+
 void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
 {
 	msm_dsi_host_snapshot(disp_state, msm_dsi->host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 93c028a122f3..1d1bda0682db 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -32,6 +32,8 @@ enum msm_dsi_phy_usecase {
 #define DSI_BUS_CLK_MAX		4
 
 struct msm_dsi {
+	struct msm_display display;
+
 	struct drm_device *dev;
 	struct platform_device *pdev;
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 474006084633..03367c6ba428 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -159,9 +159,10 @@ static int msm_hdmi_init(struct hdmi *hdmi)
  * should be handled in msm_hdmi_init() so that failure happens from
  * hdmi sub-device's probe.
  */
-int msm_hdmi_modeset_init(struct hdmi *hdmi,
+static int msm_hdmi_modeset_init(struct msm_display *display,
 		struct drm_device *dev, struct drm_encoder *encoder)
 {
+	struct hdmi *hdmi = container_of(display, struct hdmi, display);
 	int ret;
 
 	hdmi->dev = dev;
@@ -210,6 +211,15 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 	return ret;
 }
 
+static const struct msm_display_funcs msm_hdmi_display_funcs = {
+	.modeset_init = msm_hdmi_modeset_init,
+};
+
+struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
+{
+	return hdmi ? &hdmi->display : NULL;
+}
+
 /*
  * The hdmi device:
  */
@@ -243,6 +253,7 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 	err = msm_hdmi_init(hdmi);
 	if (err)
 		return err;
+	hdmi->display.funcs = &msm_hdmi_display_funcs;
 	priv->kms->hdmi = hdmi;
 
 	return 0;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 49433f7727c3..f24444610fe1 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -31,6 +31,8 @@ struct hdmi_audio {
 struct hdmi_hdcp_ctrl;
 
 struct hdmi {
+	struct msm_display display;
+
 	struct drm_device *dev;
 	struct platform_device *pdev;
 
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 0ec0622d9217..dbd955091ce7 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -274,17 +274,39 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 	.fbdev_probe = NULL
 #endif
 
+struct msm_display;
+
+/**
+ * struct msm_display_funcs - ops provided by a display sub-block (DSI/DP/HDMI)
+ * @modeset_init: create the bridge and connector for the sub-block, attaching
+ *                to @encoder.
+ */
+struct msm_display_funcs {
+	int (*modeset_init)(struct msm_display *display, struct drm_device *dev,
+			    struct drm_encoder *encoder);
+};
+
+/**
+ * struct msm_display - common interface to a display sub-block
+ * @funcs: sub-block provided ops
+ *
+ * Embedded in each sub-block (&struct hdmi, &struct msm_dsi, &struct msm_dp)
+ * so that common KMS code can set up their connectors without knowing the
+ * concrete type.
+ */
+struct msm_display {
+	const struct msm_display_funcs *funcs;
+};
+
 struct hdmi;
 #ifdef CONFIG_DRM_MSM_HDMI
-int msm_hdmi_modeset_init(struct hdmi *hdmi, struct drm_device *dev,
-		struct drm_encoder *encoder);
+struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi);
 void __init msm_hdmi_register(void);
 void __exit msm_hdmi_unregister(void);
 #else
-static inline int msm_hdmi_modeset_init(struct hdmi *hdmi, struct drm_device *dev,
-		struct drm_encoder *encoder)
+static inline struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
 {
-	return -EINVAL;
+	return NULL;
 }
 static inline void __init msm_hdmi_register(void) {}
 static inline void __exit msm_hdmi_unregister(void) {}
@@ -296,8 +318,7 @@ int dsi_dev_attach(struct platform_device *pdev);
 void dsi_dev_detach(struct platform_device *pdev);
 void __init msm_dsi_register(void);
 void __exit msm_dsi_unregister(void);
-int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
-			 struct drm_encoder *encoder);
+struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi);
 void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi);
 bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
@@ -312,11 +333,9 @@ static inline void __init msm_dsi_register(void)
 static inline void __exit msm_dsi_unregister(void)
 {
 }
-static inline int msm_dsi_modeset_init(struct msm_dsi *msm_dsi,
-				       struct drm_device *dev,
-				       struct drm_encoder *encoder)
+static inline struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
 {
-	return -EINVAL;
+	return NULL;
 }
 static inline void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
 {
@@ -353,8 +372,7 @@ struct msm_dp;
 #ifdef CONFIG_DRM_MSM_DP
 int __init msm_dp_register(void);
 void __exit msm_dp_unregister(void);
-int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
-			 struct drm_encoder *encoder);
+struct msm_display *msm_dp_get_display(struct msm_dp *dp_display);
 void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
 bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
 			       const struct drm_display_mode *mode);
@@ -368,11 +386,9 @@ static inline int __init msm_dp_register(void)
 static inline void __exit msm_dp_unregister(void)
 {
 }
-static inline int msm_dp_modeset_init(struct msm_dp *dp_display,
-				       struct drm_device *dev,
-				       struct drm_encoder *encoder)
+static inline struct msm_display *msm_dp_get_display(struct msm_dp *dp_display)
 {
-	return -EINVAL;
+	return NULL;
 }
 
 static inline void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display)
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index 87530145e82b..3f855f9b3d06 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -260,9 +260,24 @@ void msm_drm_kms_uninit(struct device *dev)
 		kms->funcs->destroy(kms);
 }
 
+static int msm_display_modeset_init(struct msm_display *display,
+				    struct drm_device *ddev,
+				    struct drm_encoder *encoder)
+{
+	if (!display)
+		return 0;
+
+	return display->funcs->modeset_init(display, ddev, encoder);
+}
+
 /*
- * Set up the bridges and connectors for the display sub-blocks, using the
- * encoders the backend created in ->kms_init().
+ * Set up the bridges and connectors for the display sub-blocks, dispatching
+ * through the common display interface using the encoders the backend created
+ * in ->kms_init().
+ *
+ * The slave link of a bonded DSI pair shares its master's encoder and creates
+ * no connector of its own; its ->modeset_init() handles that internally, so it
+ * needs no special-casing here.
  */
 static int msm_kms_init_connectors(struct drm_device *ddev)
 {
@@ -270,45 +285,30 @@ static int msm_kms_init_connectors(struct drm_device *ddev)
 	struct msm_kms *kms = priv->kms;
 	int i, ret;
 
-	/*
-	 * The slave link of a bonded DSI pair shares its master's encoder and
-	 * creates no connector of its own; msm_dsi_modeset_init() handles that
-	 * internally, so it needs no special-casing here.
-	 */
 	for (i = 0; i < ARRAY_SIZE(kms->dsi); i++) {
-		if (!kms->dsi[i])
-			continue;
-
-		ret = msm_dsi_modeset_init(kms->dsi[i], ddev, kms->dsi_encoder[i]);
-		if (ret) {
-			DRM_DEV_ERROR(ddev->dev,
-				      "modeset_init failed for dsi[%d]: %d\n", i, ret);
-			return ret;
-		}
+		ret = msm_display_modeset_init(msm_dsi_get_display(kms->dsi[i]),
+					       ddev, kms->dsi_encoder[i]);
+		if (ret)
+			goto fail;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(kms->dp); i++) {
-		if (!kms->dp[i])
-			continue;
-
-		ret = msm_dp_modeset_init(kms->dp[i], ddev, kms->dp_encoder[i]);
-		if (ret) {
-			DRM_DEV_ERROR(ddev->dev,
-				      "modeset_init failed for dp[%d]: %d\n", i, ret);
-			return ret;
-		}
+		ret = msm_display_modeset_init(msm_dp_get_display(kms->dp[i]),
+					       ddev, kms->dp_encoder[i]);
+		if (ret)
+			goto fail;
 	}
 
-	if (kms->hdmi) {
-		ret = msm_hdmi_modeset_init(kms->hdmi, ddev, kms->hdmi_encoder);
-		if (ret) {
-			DRM_DEV_ERROR(ddev->dev,
-				      "modeset_init failed for HDMI: %d\n", ret);
-			return ret;
-		}
-	}
+	ret = msm_display_modeset_init(msm_hdmi_get_display(kms->hdmi),
+				       ddev, kms->hdmi_encoder);
+	if (ret)
+		goto fail;
 
 	return 0;
+
+fail:
+	DRM_DEV_ERROR(ddev->dev, "modeset_init failed: %d\n", ret);
+	return ret;
 }
 
 int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)

-- 
2.47.3

[PATCH 05/24] drm/msm: route the display snapshot through the msm_display interface

[Dmitry Baryshkov]

The display snapshot code calls msm_dsi_snapshot() and msm_dp_snapshot()
directly, so it has to know the concrete sub-block type of each entry.

Add a mandatory snapshot() callback to struct msm_display_funcs and turn
the DSI and DP snapshot helpers into implementations of it. HDMI did not
have a snapshot helper before; give it an empty implementation for now (a
following patch fills it in). The snapshot code now dispatches through the
interface for all three sub-blocks.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 19 +++++++++++--------
 drivers/gpu/drm/msm/dp/dp_display.c               |  5 ++++-
 drivers/gpu/drm/msm/dsi/dsi.c                     | 16 ++++++++++------
 drivers/gpu/drm/msm/hdmi/hdmi.c                   |  6 ++++++
 drivers/gpu/drm/msm/msm_drv.h                     | 14 +++++---------
 5 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index 850246fc89e1..b42a92c2ffad 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -131,6 +131,7 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 {
 	struct msm_drm_private *priv;
 	struct drm_device *drm_dev;
+	struct msm_display *display;
 	struct msm_kms *kms;
 	int i;
 
@@ -139,19 +140,21 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 	kms = priv->kms;
 
 	for (i = 0; i < ARRAY_SIZE(kms->dp); i++) {
-		if (!kms->dp[i])
-			continue;
-
-		msm_dp_snapshot(disp_state, kms->dp[i]);
+		display = msm_dp_get_display(kms->dp[i]);
+		if (display)
+			display->funcs->snapshot(display, disp_state);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(kms->dsi); i++) {
-		if (!kms->dsi[i])
-			continue;
-
-		msm_dsi_snapshot(disp_state, kms->dsi[i]);
+		display = msm_dsi_get_display(kms->dsi[i]);
+		if (display)
+			display->funcs->snapshot(display, disp_state);
 	}
 
+	display = msm_hdmi_get_display(kms->hdmi);
+	if (display)
+		display->funcs->snapshot(display, disp_state);
+
 	if (kms->funcs->snapshot)
 		kms->funcs->snapshot(disp_state, kms);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 79dd556e96c2..f4fa86d1b9c7 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -783,8 +783,10 @@ int msm_dp_display_get_test_bpp(struct msm_dp *dp)
 		msm_dp_display->link->test_video.test_bit_depth);
 }
 
-void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
+static void msm_dp_snapshot(struct msm_display *display,
+			    struct msm_disp_state *disp_state)
 {
+	struct msm_dp *dp = container_of(display, struct msm_dp, display);
 	struct msm_dp_display_private *msm_dp_display;
 
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
@@ -1354,6 +1356,7 @@ static int msm_dp_modeset_init(struct msm_display *display,
 
 static const struct msm_display_funcs msm_dp_display_funcs = {
 	.modeset_init = msm_dp_modeset_init,
+	.snapshot = msm_dp_snapshot,
 };
 
 struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 6cd844d08733..c86142f66237 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -267,8 +267,18 @@ static int msm_dsi_modeset_init(struct msm_display *display,
 	return 0;
 }
 
+static void msm_dsi_snapshot(struct msm_display *display,
+			     struct msm_disp_state *disp_state)
+{
+	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
+
+	msm_dsi_host_snapshot(disp_state, msm_dsi->host);
+	msm_dsi_phy_snapshot(disp_state, msm_dsi->phy);
+}
+
 static const struct msm_display_funcs msm_dsi_display_funcs = {
 	.modeset_init = msm_dsi_modeset_init,
+	.snapshot = msm_dsi_snapshot,
 };
 
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
@@ -276,9 +286,3 @@ struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
 	return msm_dsi ? &msm_dsi->display : NULL;
 }
 
-void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
-{
-	msm_dsi_host_snapshot(disp_state, msm_dsi->host);
-	msm_dsi_phy_snapshot(disp_state, msm_dsi->phy);
-}
-
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 03367c6ba428..350867ada0cf 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -211,8 +211,14 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
 	return ret;
 }
 
+static void msm_hdmi_snapshot(struct msm_display *display,
+			      struct msm_disp_state *disp_state)
+{
+}
+
 static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.modeset_init = msm_hdmi_modeset_init,
+	.snapshot = msm_hdmi_snapshot,
 };
 
 struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index dbd955091ce7..d25d783b4f11 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -275,15 +275,20 @@ int msm_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
 #endif
 
 struct msm_display;
+struct msm_disp_state;
 
 /**
  * struct msm_display_funcs - ops provided by a display sub-block (DSI/DP/HDMI)
  * @modeset_init: create the bridge and connector for the sub-block, attaching
  *                to @encoder.
+ * @snapshot:     capture the sub-block's state for a display snapshot;
+ *                mandatory.
  */
 struct msm_display_funcs {
 	int (*modeset_init)(struct msm_display *display, struct drm_device *dev,
 			    struct drm_encoder *encoder);
+	void (*snapshot)(struct msm_display *display,
+			 struct msm_disp_state *disp_state);
 };
 
 /**
@@ -319,7 +324,6 @@ void dsi_dev_detach(struct platform_device *pdev);
 void __init msm_dsi_register(void);
 void __exit msm_dsi_unregister(void);
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi);
-void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi);
 bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
@@ -337,9 +341,6 @@ static inline struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
 {
 	return NULL;
 }
-static inline void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
-{
-}
 static inline bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi)
 {
 	return false;
@@ -373,7 +374,6 @@ struct msm_dp;
 int __init msm_dp_register(void);
 void __exit msm_dp_unregister(void);
 struct msm_display *msm_dp_get_display(struct msm_dp *dp_display);
-void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);
 bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
 			       const struct drm_display_mode *mode);
 bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
@@ -391,10 +391,6 @@ static inline struct msm_display *msm_dp_get_display(struct msm_dp *dp_display)
 	return NULL;
 }
 
-static inline void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display)
-{
-}
-
 static inline bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
 					     const struct drm_display_mode *mode)
 {

-- 
2.47.3

Re: [PATCH 05/24] drm/msm: route the display snapshot through the msm_display interface

[sashiko-bot]

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

Pre-existing issues:
- [High] TOCTOU race condition checking `dp->power_on` locklessly, which can lead to an unclocked MMIO read and system crash.
--

--- Patch [5]: [PATCH 05/24] drm/msm: route the display snapshot through the msm_display interface ---
commit ccee48fbd111a360e4e514435d662edafb46b06b
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: route the display snapshot through the msm_display interface

This commit refactors the display snapshot code to use a mandatory snapshot()
callback in struct msm_display_funcs. DSI and DP snapshot helpers are
turned into implementations of this callback, and an empty one is provided
for HDMI.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 79dd556e96c27..f4fa86d1b9c77 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -783,8 +783,10 @@ int msm_dp_display_get_test_bpp(struct msm_dp *dp)
>  		msm_dp_display->link->test_video.test_bit_depth);
>  }
>  
> -void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
> +static void msm_dp_snapshot(struct msm_display *display,
> +			    struct msm_disp_state *disp_state)
>  {
> +	struct msm_dp *dp = container_of(display, struct msm_dp, display);
>  	struct msm_dp_display_private *msm_dp_display;

[Severity: High]
This is a pre-existing issue, but could the lockless check of dp->power_on
further down in msm_dp_snapshot() lead to an unclocked MMIO read?

If a concurrent modeset disable or hotplug unplug event disables the DP
controller's clocks and sets dp->power_on to false right after the lockless
if (!dp->power_on) check, the snapshot thread might proceed to read the
hardware registers in msm_disp_snapshot_add_block() without the clocks.

Reading MMIO registers while the DP clocks are disabled causes an AHB bus
hang, leading to a fatal system crash (SError) on Snapdragon hardware.

Should pm_runtime_get_sync() be used around the hardware access here,
similar to how it is handled in msm_dsi_host_snapshot()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=5

[PATCH 06/24] drm/msm/hdmi: capture the HDMI registers in the display snapshot

[Dmitry Baryshkov]

The HDMI block was never included in the display snapshot, so its
register state is missing when a snapshot is captured for debugging.

Fill in the previously empty HDMI snapshot callback to dump the HDMI core
register block. The size of the block is now obtained via
msm_ioremap_size() at probe time and stored in the hdmi structure. As
with DP, the registers are only accessed while the block is powered on,
to avoid an unclocked access.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 14 +++++++++++++-
 drivers/gpu/drm/msm/hdmi/hdmi.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 350867ada0cf..72ab2e92964e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -17,6 +17,7 @@
 
 #include "msm_kms.h"
 #include "hdmi.h"
+#include "disp/msm_disp_snapshot.h"
 
 void msm_hdmi_set_mode(struct hdmi *hdmi, bool power_on)
 {
@@ -214,6 +215,17 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
 static void msm_hdmi_snapshot(struct msm_display *display,
 			      struct msm_disp_state *disp_state)
 {
+	struct hdmi *hdmi = container_of(display, struct hdmi, display);
+
+	/*
+	 * Accessing the HDMI registers requires the clocks to be running,
+	 * which is only guaranteed while the block is powered on.
+	 */
+	if (!hdmi->power_on)
+		return;
+
+	msm_disp_snapshot_add_block(disp_state, hdmi->mmio_size,
+				    hdmi->mmio, "hdmi");
 }
 
 static const struct msm_display_funcs msm_hdmi_display_funcs = {
@@ -310,7 +322,7 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 		hdmi->next_bridge = NULL;
 	}
 
-	hdmi->mmio = msm_ioremap(pdev, "core_physical");
+	hdmi->mmio = msm_ioremap_size(pdev, "core_physical", &hdmi->mmio_size);
 	if (IS_ERR(hdmi->mmio)) {
 		ret = PTR_ERR(hdmi->mmio);
 		goto err_put_bridge;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index f24444610fe1..869f55d4704a 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -48,6 +48,7 @@ struct hdmi {
 	unsigned long pixclock;
 
 	void __iomem *mmio;
+	phys_addr_t mmio_size;
 	void __iomem *qfprom_mmio;
 	phys_addr_t mmio_phy_addr;
 

-- 
2.47.3

Re: [PATCH 06/24] drm/msm/hdmi: capture the HDMI registers in the display snapshot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Time-of-check to time-of-use (TOCTOU) race condition when checking `hdmi->power_on` without synchronization, potentially leading to unclocked MMIO access and a kernel panic.
--

--- Patch [6]: [PATCH 06/24] drm/msm/hdmi: capture the HDMI registers in the display snapshot ---
commit ab4d516f006653666c039568efe30c3c3e320faa
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/hdmi: capture the HDMI registers in the display snapshot

This commit fills in the previously empty HDMI snapshot callback to dump 
the HDMI core register block. The size of the block is obtained via 
msm_ioremap_size() at probe time, and registers are only accessed while 
the block is powered on.

> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 350867ada0cf1..72ab2e92964e7 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c

[ ... ]

> @@ -214,6 +215,17 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
>  static void msm_hdmi_snapshot(struct msm_display *display,
>  			      struct msm_disp_state *disp_state)
>  {
> +	struct hdmi *hdmi = container_of(display, struct hdmi, display);
> +
> +	/*
> +	 * Accessing the HDMI registers requires the clocks to be running,
> +	 * which is only guaranteed while the block is powered on.
> +	 */
> +	if (!hdmi->power_on)
> +		return;
> +
> +	msm_disp_snapshot_add_block(disp_state, hdmi->mmio_size,
> +				    hdmi->mmio, "hdmi");

[Severity: High]
Is it possible to hit a Time-of-check to time-of-use (TOCTOU) race condition
here?

If a concurrent atomic commit thread executes
msm_hdmi_bridge_atomic_post_disable(), takes hdmi->state_mutex, disables the
clocks, and sets hdmi->power_on = false, this lockless check could pass before
the clocks are actually disabled.

Would the subsequent msm_disp_snapshot_add_block() then attempt to read
unclocked hardware registers, potentially causing a synchronous external abort
and kernel panic?

Does this access need to be protected by hdmi->state_mutex to ensure the
clocks remain enabled during the entire snapshot capture?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=6

[PATCH 07/24] drm/msm: add the wide_bus_enabled callback to msm_display

[Dmitry Baryshkov]

dpu_encoder_is_widebus_enabled() switches on the interface type to call
either msm_dsi_wide_bus_enabled() or msm_dp_wide_bus_available(), two
differently named sub-block functions.

Replace them with a single mandatory wide_bus_enabled() callback on
struct msm_display_funcs. DSI and DP implement it (DP keeps its internal
msm_dp_wide_bus_available() helper, now declared privately in
dp_display.h); HDMI, which never drives a wide bus, returns false.

Add a dpu_encoder_get_display() helper that maps a struct msm_display_info
to its struct msm_display, so the DPU encoder can look up the sub-block
and call the callback without open-coding the interface-type switch. It is
reused by the following callback conversions.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 42 +++++++++++++++++++++--------
 drivers/gpu/drm/msm/dp/dp_display.c         |  8 ++++++
 drivers/gpu/drm/msm/dp/dp_display.h         |  1 +
 drivers/gpu/drm/msm/dsi/dsi.c               |  5 +++-
 drivers/gpu/drm/msm/hdmi/hdmi.c             |  6 +++++
 drivers/gpu/drm/msm/msm_drv.h               | 14 +++-------
 6 files changed, 53 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1f20695f81e3..ce358c1c9757 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -268,25 +268,45 @@ bool dpu_encoder_needs_periph_flush(struct dpu_encoder_phys *phys_enc)
 	       msm_dp_needs_periph_flush(priv->kms->dp[disp_info->h_tile_instance[0]], mode);
 }
 
+/**
+ * dpu_encoder_get_display - find the display sub-block driving an interface
+ * @dev:        the DRM device
+ * @disp_info:  the interface description
+ *
+ * Return: the &struct msm_display (DSI/DP/HDMI) driving the interface, or NULL
+ * for interfaces (e.g. writeback) that have no such sub-block.
+ */
+static struct msm_display *dpu_encoder_get_display(struct drm_device *dev,
+		const struct msm_display_info *disp_info)
+{
+	struct msm_drm_private *priv = dev->dev_private;
+	int index = disp_info->h_tile_instance[0];
+
+	switch (disp_info->intf_type) {
+	case INTF_DP:
+		return msm_dp_get_display(priv->kms->dp[index]);
+	case INTF_DSI:
+		return msm_dsi_get_display(priv->kms->dsi[index]);
+	case INTF_HDMI:
+		return msm_hdmi_get_display(priv->kms->hdmi);
+	default:
+		return NULL;
+	}
+}
+
 /**
  * dpu_encoder_is_widebus_enabled - return bool value if widebus is enabled
  * @drm_enc:    Pointer to previously created drm encoder structure
  */
 bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
 {
-	const struct dpu_encoder_virt *dpu_enc;
-	struct msm_drm_private *priv = drm_enc->dev->dev_private;
-	const struct msm_display_info *disp_info;
-	int index;
+	const struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+	struct msm_display *display;
 
-	dpu_enc = to_dpu_encoder_virt(drm_enc);
-	disp_info = &dpu_enc->disp_info;
-	index = disp_info->h_tile_instance[0];
+	display = dpu_encoder_get_display(drm_enc->dev, &dpu_enc->disp_info);
 
-	if (disp_info->intf_type == INTF_DP)
-		return msm_dp_wide_bus_available(priv->kms->dp[index]);
-	else if (disp_info->intf_type == INTF_DSI)
-		return msm_dsi_wide_bus_enabled(priv->kms->dsi[index]);
+	if (display)
+		return display->funcs->wide_bus_enabled(display);
 
 	return false;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index f4fa86d1b9c7..04d291a93d20 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1354,9 +1354,17 @@ static int msm_dp_modeset_init(struct msm_display *display,
 	return 0;
 }
 
+static bool msm_dp_display_wide_bus_enabled(struct msm_display *display)
+{
+	struct msm_dp *dp = container_of(display, struct msm_dp, display);
+
+	return msm_dp_wide_bus_available(dp);
+}
+
 static const struct msm_display_funcs msm_dp_display_funcs = {
 	.modeset_init = msm_dp_modeset_init,
 	.snapshot = msm_dp_snapshot,
+	.wide_bus_enabled = msm_dp_display_wide_bus_enabled,
 };
 
 struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 20a71afb1f90..8c34a473c7db 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -34,6 +34,7 @@ int msm_dp_display_get_test_bpp(struct msm_dp *msm_dp_display);
 void msm_dp_display_signal_audio_start(struct msm_dp *msm_dp_display);
 void msm_dp_display_signal_audio_complete(struct msm_dp *msm_dp_display);
 void msm_dp_display_set_psr(struct msm_dp *dp, bool enter);
+bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
 void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *dentry, bool is_edp);
 
 #endif /* _DP_DISPLAY_H_ */
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index c86142f66237..ce3b261123ea 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -17,8 +17,10 @@ struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
 	return msm_dsi_host_get_dsc_config(msm_dsi->host);
 }
 
-bool msm_dsi_wide_bus_enabled(struct msm_dsi *msm_dsi)
+static bool msm_dsi_wide_bus_enabled(struct msm_display *display)
 {
+	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
+
 	return msm_dsi_host_is_wide_bus_enabled(msm_dsi->host);
 }
 
@@ -279,6 +281,7 @@ static void msm_dsi_snapshot(struct msm_display *display,
 static const struct msm_display_funcs msm_dsi_display_funcs = {
 	.modeset_init = msm_dsi_modeset_init,
 	.snapshot = msm_dsi_snapshot,
+	.wide_bus_enabled = msm_dsi_wide_bus_enabled,
 };
 
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 72ab2e92964e..5f24f778c7ef 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -228,9 +228,15 @@ static void msm_hdmi_snapshot(struct msm_display *display,
 				    hdmi->mmio, "hdmi");
 }
 
+static bool msm_hdmi_wide_bus_enabled(struct msm_display *display)
+{
+	return false;
+}
+
 static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.modeset_init = msm_hdmi_modeset_init,
 	.snapshot = msm_hdmi_snapshot,
+	.wide_bus_enabled = msm_hdmi_wide_bus_enabled,
 };
 
 struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index d25d783b4f11..519571170207 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -283,12 +283,15 @@ struct msm_disp_state;
  *                to @encoder.
  * @snapshot:     capture the sub-block's state for a display snapshot;
  *                mandatory.
+ * @wide_bus_enabled: whether the sub-block drives a wide (2 pixel/clock) bus;
+ *                    mandatory.
  */
 struct msm_display_funcs {
 	int (*modeset_init)(struct msm_display *display, struct drm_device *dev,
 			    struct drm_encoder *encoder);
 	void (*snapshot)(struct msm_display *display,
 			 struct msm_disp_state *disp_state);
+	bool (*wide_bus_enabled)(struct msm_display *display);
 };
 
 /**
@@ -327,7 +330,6 @@ struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
-bool msm_dsi_wide_bus_enabled(struct msm_dsi *msm_dsi);
 struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
 const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi);
 #else
@@ -353,10 +355,6 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
 {
 	return false;
 }
-static inline bool msm_dsi_wide_bus_enabled(struct msm_dsi *msm_dsi)
-{
-	return false;
-}
 
 static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
 {
@@ -376,7 +374,6 @@ void __exit msm_dp_unregister(void);
 struct msm_display *msm_dp_get_display(struct msm_dp *dp_display);
 bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
 			       const struct drm_display_mode *mode);
-bool msm_dp_wide_bus_available(const struct msm_dp *dp_display);
 
 #else
 static inline int __init msm_dp_register(void)
@@ -397,11 +394,6 @@ static inline bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
 	return false;
 }
 
-static inline bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
-{
-	return false;
-}
-
 #endif
 
 #ifdef CONFIG_DRM_MSM_MDP4

-- 
2.47.3

[PATCH 08/24] drm/msm: add the needs_periph_flush callback to msm_display

[Dmitry Baryshkov]

dpu_encoder_needs_periph_flush() switches on the interface type to call
msm_dp_needs_periph_flush() on the DP sub-block directly, hardcoding that
only DP interfaces ever need a peripheral flush.

Add a mandatory needs_periph_flush() callback to struct msm_display_funcs.
DP implements it (keeping its internal msm_dp_needs_periph_flush() helper,
now declared privately); DSI and HDMI, which never need a peripheral
flush, return false. The DPU encoder now looks up the sub-block and calls
the one callback.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 49 +++++++++++++++--------------
 drivers/gpu/drm/msm/dp/dp_display.c         | 13 ++++++--
 drivers/gpu/drm/msm/dsi/dsi.c               |  7 +++++
 drivers/gpu/drm/msm/hdmi/hdmi.c             |  7 +++++
 drivers/gpu/drm/msm/msm_drv.h               | 11 ++-----
 5 files changed, 54 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index ce358c1c9757..09bf959b2ca7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -245,29 +245,6 @@ u32 dpu_encoder_get_drm_fmt(struct dpu_encoder_phys *phys_enc)
 	return DRM_FORMAT_RGB888;
 }
 
-/**
- * dpu_encoder_needs_periph_flush - return true if physical encoder requires
- *	peripheral flush
- * @phys_enc: Pointer to physical encoder structure
- */
-bool dpu_encoder_needs_periph_flush(struct dpu_encoder_phys *phys_enc)
-{
-	struct drm_encoder *drm_enc;
-	struct dpu_encoder_virt *dpu_enc;
-	struct msm_display_info *disp_info;
-	struct msm_drm_private *priv;
-	struct drm_display_mode *mode;
-
-	drm_enc = phys_enc->parent;
-	dpu_enc = to_dpu_encoder_virt(drm_enc);
-	disp_info = &dpu_enc->disp_info;
-	priv = drm_enc->dev->dev_private;
-	mode = &phys_enc->cached_mode;
-
-	return phys_enc->hw_intf->cap->type == INTF_DP &&
-	       msm_dp_needs_periph_flush(priv->kms->dp[disp_info->h_tile_instance[0]], mode);
-}
-
 /**
  * dpu_encoder_get_display - find the display sub-block driving an interface
  * @dev:        the DRM device
@@ -294,6 +271,32 @@ static struct msm_display *dpu_encoder_get_display(struct drm_device *dev,
 	}
 }
 
+/**
+ * dpu_encoder_needs_periph_flush - return true if physical encoder requires
+ *	peripheral flush
+ * @phys_enc: Pointer to physical encoder structure
+ */
+bool dpu_encoder_needs_periph_flush(struct dpu_encoder_phys *phys_enc)
+{
+	struct drm_encoder *drm_enc;
+	struct dpu_encoder_virt *dpu_enc;
+	struct msm_display_info *disp_info;
+	struct drm_display_mode *mode;
+	struct msm_display *display;
+
+	drm_enc = phys_enc->parent;
+	dpu_enc = to_dpu_encoder_virt(drm_enc);
+	disp_info = &dpu_enc->disp_info;
+	mode = &phys_enc->cached_mode;
+
+	display = dpu_encoder_get_display(drm_enc->dev, disp_info);
+
+	if (display)
+		return display->funcs->needs_periph_flush(display, mode);
+
+	return false;
+}
+
 /**
  * dpu_encoder_is_widebus_enabled - return bool value if widebus is enabled
  * @drm_enc:    Pointer to previously created drm encoder structure
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 04d291a93d20..773dee9d0be4 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1290,8 +1290,8 @@ void __exit msm_dp_unregister(void)
 	platform_driver_unregister(&msm_dp_display_driver);
 }
 
-bool msm_dp_needs_periph_flush(const struct msm_dp *msm_dp_display,
-			       const struct drm_display_mode *mode)
+static bool msm_dp_needs_periph_flush(const struct msm_dp *msm_dp_display,
+				      const struct drm_display_mode *mode)
 {
 	return drm_mode_is_420_only(&msm_dp_display->connector->display_info, mode);
 }
@@ -1361,10 +1361,19 @@ static bool msm_dp_display_wide_bus_enabled(struct msm_display *display)
 	return msm_dp_wide_bus_available(dp);
 }
 
+static bool msm_dp_display_needs_periph_flush(struct msm_display *display,
+					      const struct drm_display_mode *mode)
+{
+	struct msm_dp *dp = container_of(display, struct msm_dp, display);
+
+	return msm_dp_needs_periph_flush(dp, mode);
+}
+
 static const struct msm_display_funcs msm_dp_display_funcs = {
 	.modeset_init = msm_dp_modeset_init,
 	.snapshot = msm_dp_snapshot,
 	.wide_bus_enabled = msm_dp_display_wide_bus_enabled,
+	.needs_periph_flush = msm_dp_display_needs_periph_flush,
 };
 
 struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index ce3b261123ea..085e202bf7e6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -24,6 +24,12 @@ static bool msm_dsi_wide_bus_enabled(struct msm_display *display)
 	return msm_dsi_host_is_wide_bus_enabled(msm_dsi->host);
 }
 
+static bool msm_dsi_needs_periph_flush(struct msm_display *display,
+				       const struct drm_display_mode *mode)
+{
+	return false;
+}
+
 static int dsi_get_phy(struct msm_dsi *msm_dsi)
 {
 	struct platform_device *pdev = msm_dsi->pdev;
@@ -282,6 +288,7 @@ static const struct msm_display_funcs msm_dsi_display_funcs = {
 	.modeset_init = msm_dsi_modeset_init,
 	.snapshot = msm_dsi_snapshot,
 	.wide_bus_enabled = msm_dsi_wide_bus_enabled,
+	.needs_periph_flush = msm_dsi_needs_periph_flush,
 };
 
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 5f24f778c7ef..72f7403265f9 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -233,10 +233,17 @@ static bool msm_hdmi_wide_bus_enabled(struct msm_display *display)
 	return false;
 }
 
+static bool msm_hdmi_needs_periph_flush(struct msm_display *display,
+					const struct drm_display_mode *mode)
+{
+	return false;
+}
+
 static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.modeset_init = msm_hdmi_modeset_init,
 	.snapshot = msm_hdmi_snapshot,
 	.wide_bus_enabled = msm_hdmi_wide_bus_enabled,
+	.needs_periph_flush = msm_hdmi_needs_periph_flush,
 };
 
 struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 519571170207..806d72ed393b 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -285,6 +285,7 @@ struct msm_disp_state;
  *                mandatory.
  * @wide_bus_enabled: whether the sub-block drives a wide (2 pixel/clock) bus;
  *                    mandatory.
+ * @needs_periph_flush: whether @mode requires a peripheral flush; mandatory.
  */
 struct msm_display_funcs {
 	int (*modeset_init)(struct msm_display *display, struct drm_device *dev,
@@ -292,6 +293,8 @@ struct msm_display_funcs {
 	void (*snapshot)(struct msm_display *display,
 			 struct msm_disp_state *disp_state);
 	bool (*wide_bus_enabled)(struct msm_display *display);
+	bool (*needs_periph_flush)(struct msm_display *display,
+				   const struct drm_display_mode *mode);
 };
 
 /**
@@ -372,8 +375,6 @@ struct msm_dp;
 int __init msm_dp_register(void);
 void __exit msm_dp_unregister(void);
 struct msm_display *msm_dp_get_display(struct msm_dp *dp_display);
-bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
-			       const struct drm_display_mode *mode);
 
 #else
 static inline int __init msm_dp_register(void)
@@ -388,12 +389,6 @@ static inline struct msm_display *msm_dp_get_display(struct msm_dp *dp_display)
 	return NULL;
 }
 
-static inline bool msm_dp_needs_periph_flush(const struct msm_dp *dp_display,
-					     const struct drm_display_mode *mode)
-{
-	return false;
-}
-
 #endif
 
 #ifdef CONFIG_DRM_MSM_MDP4

-- 
2.47.3

[PATCH 09/24] drm/msm: add the is_cmd_mode callback to msm_display

[Dmitry Baryshkov]

The DPU and MDP5 backends query whether a DSI link runs in command mode by
calling msm_dsi_is_cmd_mode() directly on the concrete sub-block while
building their encoders.

Add a mandatory is_cmd_mode() callback to struct msm_display_funcs; DSI
implements it, DP and HDMI (always video) return false. The backends
dispatch through the callback.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 4 +++-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 4 +++-
 drivers/gpu/drm/msm/dp/dp_display.c      | 6 ++++++
 drivers/gpu/drm/msm/dsi/dsi.c            | 4 +++-
 drivers/gpu/drm/msm/hdmi/hdmi.c          | 6 ++++++
 drivers/gpu/drm/msm/msm_drv.h            | 7 ++-----
 6 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 42dff1a98292..7928bc1a73f3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -583,6 +583,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 {
 	struct drm_encoder *encoder = NULL;
 	struct msm_display_info info;
+	struct msm_display *display;
 	int i, rc = 0;
 
 	if (!(priv->kms->dsi[0] || priv->kms->dsi[1]))
@@ -613,7 +614,8 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 		if (msm_dsi_is_bonded_dsi(priv->kms->dsi[i]))
 			info.h_tile_instance[info.num_of_h_tiles++] = other;
 
-		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->kms->dsi[i]);
+		display = msm_dsi_get_display(priv->kms->dsi[i]);
+		info.is_cmd_mode = display->funcs->is_cmd_mode(display);
 
 		rc = dpu_kms_dsi_set_te_source(&info, priv->kms->dsi[i]);
 		if (rc) {
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 68b71a7c6666..e372d295b80f 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -337,6 +337,7 @@ static int modeset_init_intf(struct mdp5_kms *mdp5_kms,
 		const struct mdp5_cfg_hw *hw_cfg =
 					mdp5_cfg_get_hw_config(mdp5_kms->cfg);
 		int dsi_id = get_dsi_id_from_intf(hw_cfg, intf->num);
+		struct msm_display *display;
 
 		if ((dsi_id >= ARRAY_SIZE(priv->kms->dsi)) || (dsi_id < 0)) {
 			DRM_DEV_ERROR(dev->dev, "failed to find dsi from intf %d\n",
@@ -360,9 +361,10 @@ static int modeset_init_intf(struct mdp5_kms *mdp5_kms,
 			break;
 		}
 
+		display = msm_dsi_get_display(priv->kms->dsi[dsi_id]);
 		priv->kms->dsi_encoder[dsi_id] = encoder;
 		mdp5_encoder_set_intf_mode(encoder,
-					   msm_dsi_is_cmd_mode(priv->kms->dsi[dsi_id]));
+					   display->funcs->is_cmd_mode(display));
 
 		break;
 	}
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 773dee9d0be4..425526db7ef9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1369,11 +1369,17 @@ static bool msm_dp_display_needs_periph_flush(struct msm_display *display,
 	return msm_dp_needs_periph_flush(dp, mode);
 }
 
+static bool msm_dp_display_is_cmd_mode(struct msm_display *display)
+{
+	return false;
+}
+
 static const struct msm_display_funcs msm_dp_display_funcs = {
 	.modeset_init = msm_dp_modeset_init,
 	.snapshot = msm_dp_snapshot,
 	.wide_bus_enabled = msm_dp_display_wide_bus_enabled,
 	.needs_periph_flush = msm_dp_display_needs_periph_flush,
+	.is_cmd_mode = msm_dp_display_is_cmd_mode,
 };
 
 struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 085e202bf7e6..bda778dbcc5b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -5,8 +5,9 @@
 
 #include "dsi.h"
 
-bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi)
+static bool msm_dsi_is_cmd_mode(struct msm_display *display)
 {
+	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
 	unsigned long host_flags = msm_dsi_host_get_mode_flags(msm_dsi->host);
 
 	return !(host_flags & MIPI_DSI_MODE_VIDEO);
@@ -289,6 +290,7 @@ static const struct msm_display_funcs msm_dsi_display_funcs = {
 	.snapshot = msm_dsi_snapshot,
 	.wide_bus_enabled = msm_dsi_wide_bus_enabled,
 	.needs_periph_flush = msm_dsi_needs_periph_flush,
+	.is_cmd_mode = msm_dsi_is_cmd_mode,
 };
 
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 72f7403265f9..828cb89ebf9a 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -239,11 +239,17 @@ static bool msm_hdmi_needs_periph_flush(struct msm_display *display,
 	return false;
 }
 
+static bool msm_hdmi_is_cmd_mode(struct msm_display *display)
+{
+	return false;
+}
+
 static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.modeset_init = msm_hdmi_modeset_init,
 	.snapshot = msm_hdmi_snapshot,
 	.wide_bus_enabled = msm_hdmi_wide_bus_enabled,
 	.needs_periph_flush = msm_hdmi_needs_periph_flush,
+	.is_cmd_mode = msm_hdmi_is_cmd_mode,
 };
 
 struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 806d72ed393b..5320e4977af9 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -286,6 +286,7 @@ struct msm_disp_state;
  * @wide_bus_enabled: whether the sub-block drives a wide (2 pixel/clock) bus;
  *                    mandatory.
  * @needs_periph_flush: whether @mode requires a peripheral flush; mandatory.
+ * @is_cmd_mode:  whether the sub-block runs in command mode; mandatory.
  */
 struct msm_display_funcs {
 	int (*modeset_init)(struct msm_display *display, struct drm_device *dev,
@@ -295,6 +296,7 @@ struct msm_display_funcs {
 	bool (*wide_bus_enabled)(struct msm_display *display);
 	bool (*needs_periph_flush)(struct msm_display *display,
 				   const struct drm_display_mode *mode);
+	bool (*is_cmd_mode)(struct msm_display *display);
 };
 
 /**
@@ -330,7 +332,6 @@ void dsi_dev_detach(struct platform_device *pdev);
 void __init msm_dsi_register(void);
 void __exit msm_dsi_unregister(void);
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi);
-bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
 struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
@@ -346,10 +347,6 @@ static inline struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
 {
 	return NULL;
 }
-static inline bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi)
-{
-	return false;
-}
 static inline bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi)
 {
 	return false;

-- 
2.47.3

[PATCH 10/24] drm/msm: add the get_dsc_config callback to msm_display

[Dmitry Baryshkov]

dpu_encoder_get_dsc_config() special-cases INTF_DSI to fetch the DSC
config from the concrete DSI sub-block via msm_dsi_get_dsc_config().

Add a mandatory get_dsc_config() callback to struct msm_display_funcs; DSI
returns its host DSC config, DP and HDMI return NULL. The DPU encoder now
uses the generic display lookup and the callback, dropping the
interface-type special case.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 ++++----
 drivers/gpu/drm/msm/dp/dp_display.c         | 6 ++++++
 drivers/gpu/drm/msm/dsi/dsi.c               | 5 ++++-
 drivers/gpu/drm/msm/hdmi/hdmi.c             | 6 ++++++
 drivers/gpu/drm/msm/msm_drv.h               | 8 ++------
 5 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 09bf959b2ca7..569f3759d02c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -666,12 +666,12 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
  */
 struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
 {
-	struct msm_drm_private *priv = drm_enc->dev->dev_private;
 	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
-	int index = dpu_enc->disp_info.h_tile_instance[0];
+	struct msm_display *display = dpu_encoder_get_display(drm_enc->dev,
+							     &dpu_enc->disp_info);
 
-	if (dpu_enc->disp_info.intf_type == INTF_DSI)
-		return msm_dsi_get_dsc_config(priv->kms->dsi[index]);
+	if (display)
+		return display->funcs->get_dsc_config(display);
 
 	return NULL;
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 425526db7ef9..047295d46c02 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1374,12 +1374,18 @@ static bool msm_dp_display_is_cmd_mode(struct msm_display *display)
 	return false;
 }
 
+static struct drm_dsc_config *msm_dp_display_get_dsc_config(struct msm_display *display)
+{
+	return NULL;
+}
+
 static const struct msm_display_funcs msm_dp_display_funcs = {
 	.modeset_init = msm_dp_modeset_init,
 	.snapshot = msm_dp_snapshot,
 	.wide_bus_enabled = msm_dp_display_wide_bus_enabled,
 	.needs_periph_flush = msm_dp_display_needs_periph_flush,
 	.is_cmd_mode = msm_dp_display_is_cmd_mode,
+	.get_dsc_config = msm_dp_display_get_dsc_config,
 };
 
 struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index bda778dbcc5b..e42f5b929b84 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -13,8 +13,10 @@ static bool msm_dsi_is_cmd_mode(struct msm_display *display)
 	return !(host_flags & MIPI_DSI_MODE_VIDEO);
 }
 
-struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
+static struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_display *display)
 {
+	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
+
 	return msm_dsi_host_get_dsc_config(msm_dsi->host);
 }
 
@@ -291,6 +293,7 @@ static const struct msm_display_funcs msm_dsi_display_funcs = {
 	.wide_bus_enabled = msm_dsi_wide_bus_enabled,
 	.needs_periph_flush = msm_dsi_needs_periph_flush,
 	.is_cmd_mode = msm_dsi_is_cmd_mode,
+	.get_dsc_config = msm_dsi_get_dsc_config,
 };
 
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 828cb89ebf9a..918fc9c88a19 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -244,12 +244,18 @@ static bool msm_hdmi_is_cmd_mode(struct msm_display *display)
 	return false;
 }
 
+static struct drm_dsc_config *msm_hdmi_get_dsc_config(struct msm_display *display)
+{
+	return NULL;
+}
+
 static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.modeset_init = msm_hdmi_modeset_init,
 	.snapshot = msm_hdmi_snapshot,
 	.wide_bus_enabled = msm_hdmi_wide_bus_enabled,
 	.needs_periph_flush = msm_hdmi_needs_periph_flush,
 	.is_cmd_mode = msm_hdmi_is_cmd_mode,
+	.get_dsc_config = msm_hdmi_get_dsc_config,
 };
 
 struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 5320e4977af9..cda9849811b1 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -287,6 +287,7 @@ struct msm_disp_state;
  *                    mandatory.
  * @needs_periph_flush: whether @mode requires a peripheral flush; mandatory.
  * @is_cmd_mode:  whether the sub-block runs in command mode; mandatory.
+ * @get_dsc_config: return the DSC config for the sub-block, or NULL; mandatory.
  */
 struct msm_display_funcs {
 	int (*modeset_init)(struct msm_display *display, struct drm_device *dev,
@@ -297,6 +298,7 @@ struct msm_display_funcs {
 	bool (*needs_periph_flush)(struct msm_display *display,
 				   const struct drm_display_mode *mode);
 	bool (*is_cmd_mode)(struct msm_display *display);
+	struct drm_dsc_config *(*get_dsc_config)(struct msm_display *display);
 };
 
 /**
@@ -334,7 +336,6 @@ void __exit msm_dsi_unregister(void);
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
-struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
 const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
@@ -356,11 +357,6 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
 	return false;
 }
 
-static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
-{
-	return NULL;
-}
-
 static inline const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
 {
 	return NULL;

-- 
2.47.3

[PATCH 11/24] drm/msm: add the get_te_source callback to msm_display

[Dmitry Baryshkov]

The DPU backend reads the tear-check source name of a DSI link through
msm_dsi_get_te_source() on the concrete sub-block while building the
display info.

Add a mandatory get_te_source() callback to struct msm_display_funcs; DSI
returns its te_source string, DP and HDMI return NULL. dpu_kms_dsi_set_te_
source() now takes a struct msm_display and dispatches through the
callback.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 6 +++---
 drivers/gpu/drm/msm/dp/dp_display.c     | 6 ++++++
 drivers/gpu/drm/msm/dsi/dsi.c           | 8 ++++++++
 drivers/gpu/drm/msm/dsi/dsi_manager.c   | 5 -----
 drivers/gpu/drm/msm/hdmi/hdmi.c         | 6 ++++++
 drivers/gpu/drm/msm/msm_drv.h           | 8 ++------
 6 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7928bc1a73f3..8dacde316033 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -555,9 +555,9 @@ static const char *dpu_vsync_sources[] = {
 };
 
 static int dpu_kms_dsi_set_te_source(struct msm_display_info *info,
-				     struct msm_dsi *dsi)
+				     struct msm_display *display)
 {
-	const char *te_source = msm_dsi_get_te_source(dsi);
+	const char *te_source = display->funcs->get_te_source(display);
 	int i;
 
 	if (!te_source) {
@@ -617,7 +617,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 		display = msm_dsi_get_display(priv->kms->dsi[i]);
 		info.is_cmd_mode = display->funcs->is_cmd_mode(display);
 
-		rc = dpu_kms_dsi_set_te_source(&info, priv->kms->dsi[i]);
+		rc = dpu_kms_dsi_set_te_source(&info, display);
 		if (rc) {
 			DPU_ERROR("failed to identify TE source for dsi display\n");
 			return rc;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 047295d46c02..95810e3b01a7 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1379,6 +1379,11 @@ static struct drm_dsc_config *msm_dp_display_get_dsc_config(struct msm_display *
 	return NULL;
 }
 
+static const char *msm_dp_display_get_te_source(struct msm_display *display)
+{
+	return NULL;
+}
+
 static const struct msm_display_funcs msm_dp_display_funcs = {
 	.modeset_init = msm_dp_modeset_init,
 	.snapshot = msm_dp_snapshot,
@@ -1386,6 +1391,7 @@ static const struct msm_display_funcs msm_dp_display_funcs = {
 	.needs_periph_flush = msm_dp_display_needs_periph_flush,
 	.is_cmd_mode = msm_dp_display_is_cmd_mode,
 	.get_dsc_config = msm_dp_display_get_dsc_config,
+	.get_te_source = msm_dp_display_get_te_source,
 };
 
 struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index e42f5b929b84..e07d1a964c7c 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -20,6 +20,13 @@ static struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_display *display
 	return msm_dsi_host_get_dsc_config(msm_dsi->host);
 }
 
+static const char *msm_dsi_get_te_source(struct msm_display *display)
+{
+	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
+
+	return msm_dsi->te_source;
+}
+
 static bool msm_dsi_wide_bus_enabled(struct msm_display *display)
 {
 	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
@@ -294,6 +301,7 @@ static const struct msm_display_funcs msm_dsi_display_funcs = {
 	.needs_periph_flush = msm_dsi_needs_periph_flush,
 	.is_cmd_mode = msm_dsi_is_cmd_mode,
 	.get_dsc_config = msm_dsi_get_dsc_config,
+	.get_te_source = msm_dsi_get_te_source,
 };
 
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 959daa88b1d0..2a5326f1fa8b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -614,8 +614,3 @@ bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
 {
 	return IS_MASTER_DSI_LINK(msm_dsi->id);
 }
-
-const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
-{
-	return msm_dsi->te_source;
-}
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 918fc9c88a19..6e077747ebce 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -249,6 +249,11 @@ static struct drm_dsc_config *msm_hdmi_get_dsc_config(struct msm_display *displa
 	return NULL;
 }
 
+static const char *msm_hdmi_get_te_source(struct msm_display *display)
+{
+	return NULL;
+}
+
 static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.modeset_init = msm_hdmi_modeset_init,
 	.snapshot = msm_hdmi_snapshot,
@@ -256,6 +261,7 @@ static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.needs_periph_flush = msm_hdmi_needs_periph_flush,
 	.is_cmd_mode = msm_hdmi_is_cmd_mode,
 	.get_dsc_config = msm_hdmi_get_dsc_config,
+	.get_te_source = msm_hdmi_get_te_source,
 };
 
 struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index cda9849811b1..989b0a37d6aa 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -288,6 +288,7 @@ struct msm_disp_state;
  * @needs_periph_flush: whether @mode requires a peripheral flush; mandatory.
  * @is_cmd_mode:  whether the sub-block runs in command mode; mandatory.
  * @get_dsc_config: return the DSC config for the sub-block, or NULL; mandatory.
+ * @get_te_source: return the tear-check source name, or NULL; mandatory.
  */
 struct msm_display_funcs {
 	int (*modeset_init)(struct msm_display *display, struct drm_device *dev,
@@ -299,6 +300,7 @@ struct msm_display_funcs {
 				   const struct drm_display_mode *mode);
 	bool (*is_cmd_mode)(struct msm_display *display);
 	struct drm_dsc_config *(*get_dsc_config)(struct msm_display *display);
+	const char *(*get_te_source)(struct msm_display *display);
 };
 
 /**
@@ -336,7 +338,6 @@ void __exit msm_dsi_unregister(void);
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
-const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
 {
@@ -356,11 +357,6 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
 {
 	return false;
 }
-
-static inline const char *msm_dsi_get_te_source(struct msm_dsi *msm_dsi)
-{
-	return NULL;
-}
 #endif
 
 struct msm_dp;

-- 
2.47.3

[PATCH 12/24] drm/msm: add is_bonded and needs_encoder callbacks to msm_display

[Dmitry Baryshkov]

The DPU DSI setup calls msm_dsi_is_bonded_dsi() and msm_dsi_is_master_dsi()
on the concrete sub-block to decide whether to create an encoder (the slave
of a bonded pair shares the master's) and whether to add a second tile.

Add two mandatory callbacks: is_bonded() and needs_encoder(). DSI
implements them in terms of its internal bonded/master helpers (now
declared privately in dsi.h); DP and HDMI are never bonded and always need
their own encoder. The DPU backend uses the callbacks, dropping the last
concrete msm_dsi_*() queries from its display setup.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c |  8 ++++----
 drivers/gpu/drm/msm/dp/dp_display.c     | 12 ++++++++++++
 drivers/gpu/drm/msm/dsi/dsi.c           | 15 +++++++++++++++
 drivers/gpu/drm/msm/dsi/dsi.h           |  2 ++
 drivers/gpu/drm/msm/hdmi/hdmi.c         | 12 ++++++++++++
 drivers/gpu/drm/msm/msm_drv.h           | 17 +++++++----------
 6 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 8dacde316033..2eeaef963e1a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -603,18 +603,18 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 		if (!priv->kms->dsi[i])
 			continue;
 
-		if (msm_dsi_is_bonded_dsi(priv->kms->dsi[i]) &&
-		    !msm_dsi_is_master_dsi(priv->kms->dsi[i]))
+		display = msm_dsi_get_display(priv->kms->dsi[i]);
+
+		if (!display->funcs->needs_encoder(display))
 			continue;
 
 		memset(&info, 0, sizeof(info));
 		info.intf_type = INTF_DSI;
 
 		info.h_tile_instance[info.num_of_h_tiles++] = i;
-		if (msm_dsi_is_bonded_dsi(priv->kms->dsi[i]))
+		if (display->funcs->is_bonded(display))
 			info.h_tile_instance[info.num_of_h_tiles++] = other;
 
-		display = msm_dsi_get_display(priv->kms->dsi[i]);
 		info.is_cmd_mode = display->funcs->is_cmd_mode(display);
 
 		rc = dpu_kms_dsi_set_te_source(&info, display);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 95810e3b01a7..914cf9dd0f21 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1384,6 +1384,16 @@ static const char *msm_dp_display_get_te_source(struct msm_display *display)
 	return NULL;
 }
 
+static bool msm_dp_display_is_bonded(struct msm_display *display)
+{
+	return false;
+}
+
+static bool msm_dp_display_needs_encoder(struct msm_display *display)
+{
+	return true;
+}
+
 static const struct msm_display_funcs msm_dp_display_funcs = {
 	.modeset_init = msm_dp_modeset_init,
 	.snapshot = msm_dp_snapshot,
@@ -1392,6 +1402,8 @@ static const struct msm_display_funcs msm_dp_display_funcs = {
 	.is_cmd_mode = msm_dp_display_is_cmd_mode,
 	.get_dsc_config = msm_dp_display_get_dsc_config,
 	.get_te_source = msm_dp_display_get_te_source,
+	.is_bonded = msm_dp_display_is_bonded,
+	.needs_encoder = msm_dp_display_needs_encoder,
 };
 
 struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index e07d1a964c7c..69e57ab47258 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -27,6 +27,19 @@ static const char *msm_dsi_get_te_source(struct msm_display *display)
 	return msm_dsi->te_source;
 }
 
+static bool msm_dsi_is_bonded(struct msm_display *display)
+{
+	return msm_dsi_is_bonded_dsi(container_of(display, struct msm_dsi, display));
+}
+
+static bool msm_dsi_needs_encoder(struct msm_display *display)
+{
+	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
+
+	/* the slave of a bonded pair shares the master's encoder */
+	return !(msm_dsi_is_bonded_dsi(msm_dsi) && !msm_dsi_is_master_dsi(msm_dsi));
+}
+
 static bool msm_dsi_wide_bus_enabled(struct msm_display *display)
 {
 	struct msm_dsi *msm_dsi = container_of(display, struct msm_dsi, display);
@@ -302,6 +315,8 @@ static const struct msm_display_funcs msm_dsi_display_funcs = {
 	.is_cmd_mode = msm_dsi_is_cmd_mode,
 	.get_dsc_config = msm_dsi_get_dsc_config,
 	.get_te_source = msm_dsi_get_te_source,
+	.is_bonded = msm_dsi_is_bonded,
+	.needs_encoder = msm_dsi_needs_encoder,
 };
 
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 1d1bda0682db..d594a7399f54 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -50,6 +50,8 @@ struct msm_dsi {
 };
 
 /* dsi manager */
+bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
+bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
 int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
 				   struct drm_encoder *encoder);
 int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg);
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 6e077747ebce..506a9856459e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -254,6 +254,16 @@ static const char *msm_hdmi_get_te_source(struct msm_display *display)
 	return NULL;
 }
 
+static bool msm_hdmi_is_bonded(struct msm_display *display)
+{
+	return false;
+}
+
+static bool msm_hdmi_needs_encoder(struct msm_display *display)
+{
+	return true;
+}
+
 static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.modeset_init = msm_hdmi_modeset_init,
 	.snapshot = msm_hdmi_snapshot,
@@ -262,6 +272,8 @@ static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.is_cmd_mode = msm_hdmi_is_cmd_mode,
 	.get_dsc_config = msm_hdmi_get_dsc_config,
 	.get_te_source = msm_hdmi_get_te_source,
+	.is_bonded = msm_hdmi_is_bonded,
+	.needs_encoder = msm_hdmi_needs_encoder,
 };
 
 struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 989b0a37d6aa..378d2cc80b66 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -289,6 +289,11 @@ struct msm_disp_state;
  * @is_cmd_mode:  whether the sub-block runs in command mode; mandatory.
  * @get_dsc_config: return the DSC config for the sub-block, or NULL; mandatory.
  * @get_te_source: return the tear-check source name, or NULL; mandatory.
+ * @is_bonded:    whether the sub-block is part of a bonded (tiled) pair;
+ *                mandatory.
+ * @needs_encoder: whether the sub-block needs its own encoder created (false
+ *                 for e.g. the slave of a bonded pair, which shares the
+ *                 master's encoder); mandatory.
  */
 struct msm_display_funcs {
 	int (*modeset_init)(struct msm_display *display, struct drm_device *dev,
@@ -301,6 +306,8 @@ struct msm_display_funcs {
 	bool (*is_cmd_mode)(struct msm_display *display);
 	struct drm_dsc_config *(*get_dsc_config)(struct msm_display *display);
 	const char *(*get_te_source)(struct msm_display *display);
+	bool (*is_bonded)(struct msm_display *display);
+	bool (*needs_encoder)(struct msm_display *display);
 };
 
 /**
@@ -336,8 +343,6 @@ void dsi_dev_detach(struct platform_device *pdev);
 void __init msm_dsi_register(void);
 void __exit msm_dsi_unregister(void);
 struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi);
-bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
-bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
 {
@@ -349,14 +354,6 @@ static inline struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
 {
 	return NULL;
 }
-static inline bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi)
-{
-	return false;
-}
-static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
-{
-	return false;
-}
 #endif
 
 struct msm_dp;

-- 
2.47.3

[PATCH 13/24] drm/msm/hdmi: use dev_get_drvdata() in msm_hdmi_unbind()

[Dmitry Baryshkov]

msm_hdmi_unbind() fetches its struct hdmi indirectly through
priv->kms->hdmi. The hdmi structure is already stored as the driver data
of the HDMI device (and retrieved that way in msm_hdmi_bind()), so use
dev_get_drvdata() here too. The hdmi pointer is always valid at unbind
time, so the priv->kms->hdmi check can be dropped as well.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 506a9856459e..0327098287d1 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -324,11 +324,10 @@ static void msm_hdmi_unbind(struct device *dev, struct device *master,
 		void *data)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(master);
+	struct hdmi *hdmi = dev_get_drvdata(dev);
 
-	if (priv->kms->hdmi) {
-		msm_hdmi_destroy(priv->kms->hdmi);
-		priv->kms->hdmi = NULL;
-	}
+	msm_hdmi_destroy(hdmi);
+	priv->kms->hdmi = NULL;
 }
 
 static const struct component_ops msm_hdmi_ops = {

-- 
2.47.3

Re: [PATCH 13/24] drm/msm/hdmi: use dev_get_drvdata() in msm_hdmi_unbind()

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free of `hdmi` and `hdmi->i2c` due to missing IRQ teardown in `msm_hdmi_unbind`.
--

--- Patch [13]: [PATCH 13/24] drm/msm/hdmi: use dev_get_drvdata() in msm_hdmi_unbind() ---
commit fd38b018e0f710b01405e26519af9581c27b6208
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/hdmi: use dev_get_drvdata() in msm_hdmi_unbind()

This commit updates msm_hdmi_unbind() to fetch the hdmi structure directly
using dev_get_drvdata(dev) rather than indirectly through the private kms
pointer. It also removes a NULL check, as the hdmi pointer is guaranteed to
be valid at unbind time.

> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 506a9856459e1..0327098287d14 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -324,11 +324,10 @@ static void msm_hdmi_unbind(struct device *dev, struct device *master,
>  		void *data)
>  {
>  	struct msm_drm_private *priv = dev_get_drvdata(master);
> +	struct hdmi *hdmi = dev_get_drvdata(dev);
>  
> -	if (priv->kms->hdmi) {
> -		msm_hdmi_destroy(priv->kms->hdmi);

[Severity: High]
This is a pre-existing issue, but does this unbind sequence leave an active
IRQ handler that can cause a use-after-free?

In msm_hdmi_modeset_init(), the IRQ is requested using devm_request_irq()
tied to the DRM master device (dev->dev), not the HDMI component device:

msm_hdmi_modeset_init() {
    ...
    ret = devm_request_irq(dev->dev, hdmi->irq,
                msm_hdmi_irq, IRQF_TRIGGER_HIGH,
                "hdmi_isr", hdmi);
    ...
}

When the HDMI platform device is unbound, msm_hdmi_unbind() is called, which
in turn calls msm_hdmi_destroy(hdmi) here and frees hdmi->i2c. Later, the
HDMI device's devres runs and frees the hdmi structure itself. 

Because the DRM master device is not removed during component unbind, its
devres cleanup does not run, and the IRQ remains registered and active.

If a pending or spurious interrupt triggers msm_hdmi_irq() after this point,
will it dereference the freed hdmi structure and call msm_hdmi_i2c_irq()
with the freed hdmi->i2c pointer?

> -		priv->kms->hdmi = NULL;
> -	}
> +	msm_hdmi_destroy(hdmi);
> +	priv->kms->hdmi = NULL;
>  }
>  
>  static const struct component_ops msm_hdmi_ops = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=13

[PATCH 14/24] drm/msm: store the display sub-blocks as struct msm_display

[Dmitry Baryshkov]

With every cross-module query of the DSI, DP and HDMI sub-blocks now going
through the msm_display interface, struct msm_kms no longer needs to know
their concrete types.

Store the sub-blocks as struct msm_display pointers in struct msm_kms;
each sub-driver registers &obj->display. The msm_*_get_display() accessors
are no longer needed and are removed, and callers use kms->dsi[]/dp[]/hdmi
directly.

No functional change intended.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c       |  6 +++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           |  2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c          |  2 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c |  6 +++---
 drivers/gpu/drm/msm/dp/dp_display.c               |  7 +------
 drivers/gpu/drm/msm/dsi/dsi.c                     |  7 +------
 drivers/gpu/drm/msm/hdmi/hdmi.c                   |  7 +------
 drivers/gpu/drm/msm/msm_drv.h                     | 15 ---------------
 drivers/gpu/drm/msm/msm_kms.c                     | 11 +++++------
 drivers/gpu/drm/msm/msm_kms.h                     |  6 +++---
 10 files changed, 19 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 569f3759d02c..aa8a9b7c1beb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -261,11 +261,11 @@ static struct msm_display *dpu_encoder_get_display(struct drm_device *dev,
 
 	switch (disp_info->intf_type) {
 	case INTF_DP:
-		return msm_dp_get_display(priv->kms->dp[index]);
+		return priv->kms->dp[index];
 	case INTF_DSI:
-		return msm_dsi_get_display(priv->kms->dsi[index]);
+		return priv->kms->dsi[index];
 	case INTF_HDMI:
-		return msm_hdmi_get_display(priv->kms->hdmi);
+		return priv->kms->hdmi;
 	default:
 		return NULL;
 	}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 2eeaef963e1a..ec5e02c395f6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -603,7 +603,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 		if (!priv->kms->dsi[i])
 			continue;
 
-		display = msm_dsi_get_display(priv->kms->dsi[i]);
+		display = priv->kms->dsi[i];
 
 		if (!display->funcs->needs_encoder(display))
 			continue;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index e372d295b80f..c91915030111 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -361,7 +361,7 @@ static int modeset_init_intf(struct mdp5_kms *mdp5_kms,
 			break;
 		}
 
-		display = msm_dsi_get_display(priv->kms->dsi[dsi_id]);
+		display = priv->kms->dsi[dsi_id];
 		priv->kms->dsi_encoder[dsi_id] = encoder;
 		mdp5_encoder_set_intf_mode(encoder,
 					   display->funcs->is_cmd_mode(display));
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index b42a92c2ffad..d376c3cdd8b5 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -140,18 +140,18 @@ void msm_disp_snapshot_capture_state(struct msm_disp_state *disp_state)
 	kms = priv->kms;
 
 	for (i = 0; i < ARRAY_SIZE(kms->dp); i++) {
-		display = msm_dp_get_display(kms->dp[i]);
+		display = kms->dp[i];
 		if (display)
 			display->funcs->snapshot(display, disp_state);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(kms->dsi); i++) {
-		display = msm_dsi_get_display(kms->dsi[i]);
+		display = kms->dsi[i];
 		if (display)
 			display->funcs->snapshot(display, disp_state);
 	}
 
-	display = msm_hdmi_get_display(kms->hdmi);
+	display = kms->hdmi;
 	if (display)
 		display->funcs->snapshot(display, disp_state);
 
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 914cf9dd0f21..1baeb9401cfa 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -215,7 +215,7 @@ static int msm_dp_display_bind(struct device *dev, struct device *master,
 
 	dp->msm_dp_display.drm_dev = drm;
 	dp->msm_dp_display.display.funcs = &msm_dp_display_funcs;
-	priv->kms->dp[dp->id] = &dp->msm_dp_display;
+	priv->kms->dp[dp->id] = &dp->msm_dp_display.display;
 
 	dp->drm_dev = drm;
 	dp->aux->drm_dev = drm;
@@ -1406,11 +1406,6 @@ static const struct msm_display_funcs msm_dp_display_funcs = {
 	.needs_encoder = msm_dp_display_needs_encoder,
 };
 
-struct msm_display *msm_dp_get_display(struct msm_dp *msm_dp_display)
-{
-	return msm_dp_display ? &msm_dp_display->display : NULL;
-}
-
 void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 				 struct drm_atomic_commit *state)
 {
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 69e57ab47258..3bcea5f5b5a3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -170,7 +170,7 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
 	}
 
 	msm_dsi->display.funcs = &msm_dsi_display_funcs;
-	priv->kms->dsi[msm_dsi->id] = msm_dsi;
+	priv->kms->dsi[msm_dsi->id] = &msm_dsi->display;
 
 	return 0;
 }
@@ -319,8 +319,3 @@ static const struct msm_display_funcs msm_dsi_display_funcs = {
 	.needs_encoder = msm_dsi_needs_encoder,
 };
 
-struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
-{
-	return msm_dsi ? &msm_dsi->display : NULL;
-}
-
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 0327098287d1..1d5a4b63f3c8 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -276,11 +276,6 @@ static const struct msm_display_funcs msm_hdmi_display_funcs = {
 	.needs_encoder = msm_hdmi_needs_encoder,
 };
 
-struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
-{
-	return hdmi ? &hdmi->display : NULL;
-}
-
 /*
  * The hdmi device:
  */
@@ -315,7 +310,7 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 	if (err)
 		return err;
 	hdmi->display.funcs = &msm_hdmi_display_funcs;
-	priv->kms->hdmi = hdmi;
+	priv->kms->hdmi = &hdmi->display;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 378d2cc80b66..3d9ae49cba49 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -324,14 +324,9 @@ struct msm_display {
 
 struct hdmi;
 #ifdef CONFIG_DRM_MSM_HDMI
-struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi);
 void __init msm_hdmi_register(void);
 void __exit msm_hdmi_unregister(void);
 #else
-static inline struct msm_display *msm_hdmi_get_display(struct hdmi *hdmi)
-{
-	return NULL;
-}
 static inline void __init msm_hdmi_register(void) {}
 static inline void __exit msm_hdmi_unregister(void) {}
 #endif
@@ -342,7 +337,6 @@ int dsi_dev_attach(struct platform_device *pdev);
 void dsi_dev_detach(struct platform_device *pdev);
 void __init msm_dsi_register(void);
 void __exit msm_dsi_unregister(void);
-struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi);
 #else
 static inline void __init msm_dsi_register(void)
 {
@@ -350,17 +344,12 @@ static inline void __init msm_dsi_register(void)
 static inline void __exit msm_dsi_unregister(void)
 {
 }
-static inline struct msm_display *msm_dsi_get_display(struct msm_dsi *msm_dsi)
-{
-	return NULL;
-}
 #endif
 
 struct msm_dp;
 #ifdef CONFIG_DRM_MSM_DP
 int __init msm_dp_register(void);
 void __exit msm_dp_unregister(void);
-struct msm_display *msm_dp_get_display(struct msm_dp *dp_display);
 
 #else
 static inline int __init msm_dp_register(void)
@@ -370,10 +359,6 @@ static inline int __init msm_dp_register(void)
 static inline void __exit msm_dp_unregister(void)
 {
 }
-static inline struct msm_display *msm_dp_get_display(struct msm_dp *dp_display)
-{
-	return NULL;
-}
 
 #endif
 
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index 3f855f9b3d06..1a081e15a0a8 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -286,21 +286,20 @@ static int msm_kms_init_connectors(struct drm_device *ddev)
 	int i, ret;
 
 	for (i = 0; i < ARRAY_SIZE(kms->dsi); i++) {
-		ret = msm_display_modeset_init(msm_dsi_get_display(kms->dsi[i]),
-					       ddev, kms->dsi_encoder[i]);
+		ret = msm_display_modeset_init(kms->dsi[i], ddev,
+					       kms->dsi_encoder[i]);
 		if (ret)
 			goto fail;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(kms->dp); i++) {
-		ret = msm_display_modeset_init(msm_dp_get_display(kms->dp[i]),
-					       ddev, kms->dp_encoder[i]);
+		ret = msm_display_modeset_init(kms->dp[i], ddev,
+					       kms->dp_encoder[i]);
 		if (ret)
 			goto fail;
 	}
 
-	ret = msm_display_modeset_init(msm_hdmi_get_display(kms->hdmi),
-				       ddev, kms->hdmi_encoder);
+	ret = msm_display_modeset_init(kms->hdmi, ddev, kms->hdmi_encoder);
 	if (ret)
 		goto fail;
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 06646091604a..cbb9c9dcc193 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -139,13 +139,13 @@ struct msm_kms {
 	const struct msm_kms_funcs *funcs;
 	struct drm_device *dev;
 
-	struct hdmi *hdmi;
+	struct msm_display *hdmi;
 	struct drm_encoder *hdmi_encoder;
 
-	struct msm_dsi *dsi[MSM_DSI_CONTROLLER_COUNT];
+	struct msm_display *dsi[MSM_DSI_CONTROLLER_COUNT];
 	struct drm_encoder *dsi_encoder[MSM_DSI_CONTROLLER_COUNT];
 
-	struct msm_dp *dp[MSM_DP_CONTROLLER_COUNT];
+	struct msm_display *dp[MSM_DP_CONTROLLER_COUNT];
 	struct drm_encoder *dp_encoder[MSM_DP_CONTROLLER_COUNT];
 
 	/* irq number to be passed on to msm_irq_install */

-- 
2.47.3

[PATCH 15/24] drm/msm/dp: do not reject wide-bus modes while a YUV420 mode is active

[Dmitry Baryshkov]

msm_dp_bridge_mode_valid() halves the candidate mode's pixel clock when
the sink either uses YUV 420 output or drives the wide bus, so that modes
relying on those to stay under DP_MAX_PIXEL_CLK_KHZ are accepted. The
wide bus part is queried through msm_dp_wide_bus_available(), which
returns false whenever the currently committed mode uses YUV 420 output:
it inspects the stored msm_dp_mode.out_fmt_is_yuv_420 of the active mode,
not the mode being validated.

Consequently, while a YUV 420 mode is active, an RGB mode that needs the
wide bus to fit under DP_MAX_PIXEL_CLK_KHZ has its pixel clock left
un-halved and is wrongly rejected as MODE_CLOCK_HIGH.

The candidate mode's YUV 420 status is already evaluated as is_yuv_420,
and the wide bus is disabled precisely for YUV 420 output, so halving the
pixel clock for either case is equivalent to halving it when the
candidate is YUV 420 or the controller supports the wide bus. Test
wide_bus_supported directly, so the decision no longer depends on the
format of the active mode.

Fixes: df9cf852ca30 ("drm/msm/dp: account for widebus and yuv420 during mode validation")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 1baeb9401cfa..bd1354f2c15d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -722,7 +722,7 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
 	if (is_yuv_420 && !msm_dp_display->panel->vsc_sdp_supported)
 		return MODE_NO_420;
 
-	if (is_yuv_420 || msm_dp_wide_bus_available(dp))
+	if (is_yuv_420 || msm_dp_display->wide_bus_supported)
 		mode_pclk_khz /= 2;
 
 	if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)

-- 
2.47.3

Re: [PATCH 15/24] drm/msm/dp: do not reject wide-bus modes while a YUV420 mode is active

[sashiko-bot]

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

Pre-existing issues:
- [High] The state-dependent mode validation bug fixed in `msm_dp_bridge_mode_valid` was missed in its eDP counterpart `msm_edp_bridge_mode_valid`.
- [High] Halving `mode_pclk_khz` for RGB wide-bus modes causes the DP link bandwidth requirement to be incorrectly evaluated as 50% of its true value, allowing invalid modes to pass validation.
--

--- Patch [15]: [PATCH 15/24] drm/msm/dp: do not reject wide-bus modes while a YUV420 mode is active ---
commit 6dbb162c934d82f2d73ac5d225ee02599200aeac
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Subject: drm/msm/dp: do not reject wide-bus modes while a YUV420 mode is active

This commit fixes a bug in msm_dp_bridge_mode_valid() where candidate modes
requiring wide bus to stay under DP_MAX_PIXEL_CLK_KHZ were wrongly rejected
if the currently active mode used YUV 420 output. It replaces the
state-dependent msm_dp_wide_bus_available() check with a direct check of
wide_bus_supported.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -722,7 +722,7 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
>  	if (is_yuv_420 && !msm_dp_display->panel->vsc_sdp_supported)
>  		return MODE_NO_420;
>  
> -	if (is_yuv_420 || msm_dp_wide_bus_available(dp))
> +	if (is_yuv_420 || msm_dp_display->wide_bus_supported)
>  		mode_pclk_khz /= 2;

[Severity: High]
This isn't a bug introduced by this patch, but should the same fix be
applied to the eDP counterpart?

Looking at msm_edp_bridge_mode_valid() in drivers/gpu/drm/msm/dp/dp_drm.c:

	if (msm_dp_wide_bus_available(dp))
		mode_pclk_khz /= 2;

Since eDP bridges also have ycbcr_420_allowed = true, could an eDP 
sink enter a YUV 420 mode and cause subsequent candidate eDP modes requiring 
wide bus to be incorrectly rejected?

[Severity: High]
This is a pre-existing issue, but does halving mode_pclk_khz here corrupt 
the link bandwidth validation further down in this function?

After mode_pclk_khz is halved, it is used to calculate mode_rate_khz:

	mode_bpp = msm_dp_panel_get_mode_bpp(msm_dp_display->panel,
			mode_bpp, mode_pclk_khz);

	mode_rate_khz = mode_pclk_khz * mode_bpp;
	supported_rate_khz = link_info->num_lanes * link_info->rate * 8;

	if (mode_rate_khz > supported_rate_khz)
		return MODE_BAD;

Since the physical DP serial link over the cable does not compress RGB data,
doesn't the physical link still need to transmit the full clock rate? Does 
calculating mode_rate_khz with the halved clock allow modes to pass that 
exceed the physical DP lanes' capacity?

>  
>  	if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=15

[PATCH 16/24] drm/msm/dp: remove cached drm_edid from panel

[Dmitry Baryshkov]

From: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

The cached drm_edid seems unnecessary here. Use the drm_edid pointer
directly in the plug stage instead of caching it. Remove the cached
drm_edid and the corresponding oneliner to simplify the code.

Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 28 +++++++++++-------
 drivers/gpu/drm/msm/dp/dp_panel.c   | 59 ++++---------------------------------
 drivers/gpu/drm/msm/dp/dp_panel.h   | 13 +++-----
 3 files changed, 27 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bd1354f2c15d..cd4923d1a17e 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -272,6 +272,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
 	const struct drm_display_info *info = &connector->display_info;
 	int rc = 0;
 	u8 dpcd[DP_RECEIVER_CAP_SIZE];
+	const struct drm_edid *drm_edid = NULL;
 
 	rc = drm_dp_read_dpcd_caps(dp->aux, dpcd);
 	if (rc)
@@ -279,10 +280,20 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
 
 	dp->link->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
 
-	rc = msm_dp_panel_read_sink_caps(dp->panel, connector);
+	rc = msm_dp_panel_read_link_caps(dp->panel, connector);
 	if (rc)
 		goto end;
 
+	drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
+	drm_edid_connector_update(connector, drm_edid);
+
+	if (!drm_edid) {
+		DRM_ERROR("panel edid read failed\n");
+		/* check edid read fail is due to unplug */
+		if (!msm_dp_aux_is_link_connected(dp->aux))
+			return -ETIMEDOUT;
+	}
+
 	msm_dp_link_process_request(dp->link);
 
 	if (!dp->msm_dp_display.is_edp)
@@ -294,7 +305,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
 	dp->msm_dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
 
 	dp->audio_supported = info->has_audio;
-	msm_dp_panel_handle_sink_request(dp->panel);
+	msm_dp_panel_handle_sink_request(dp->panel, drm_edid);
 
 	/*
 	 * set sink to normal operation mode -- D0
@@ -305,6 +316,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
 	msm_dp_link_reset_phy_params_vx_px(dp->link);
 
 end:
+	drm_edid_free(drm_edid);
 	return rc;
 }
 
@@ -456,7 +468,7 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 
 	/* Don't forget modes for eDP */
 	if (!dp->msm_dp_display.is_edp)
-		msm_dp_panel_unplugged(dp->panel, dp->msm_dp_display.connector);
+		drm_edid_connector_update(dp->msm_dp_display.connector, NULL);
 
 	/* triggered by irq_hdp with sink_count = 0 */
 	if (dp->link->sink_count == 0)
@@ -518,7 +530,6 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
 static void msm_dp_display_deinit_sub_modules(struct msm_dp_display_private *dp)
 {
 	msm_dp_audio_put(dp->audio);
-	msm_dp_panel_put(dp->panel);
 	msm_dp_aux_put(dp->aux);
 }
 
@@ -569,7 +580,7 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
 		rc = PTR_ERR(dp->ctrl);
 		DRM_ERROR("failed to initialize ctrl, rc = %d\n", rc);
 		dp->ctrl = NULL;
-		goto error_ctrl;
+		goto error_link;
 	}
 
 	dp->audio = msm_dp_audio_get(dp->msm_dp_display.pdev, dp->link_base);
@@ -577,13 +588,11 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
 		rc = PTR_ERR(dp->audio);
 		pr_err("failed to initialize audio, rc = %d\n", rc);
 		dp->audio = NULL;
-		goto error_ctrl;
+		goto error_link;
 	}
 
 	return rc;
 
-error_ctrl:
-	msm_dp_panel_put(dp->panel);
 error_link:
 	msm_dp_aux_put(dp->aux);
 error:
@@ -755,8 +764,7 @@ int msm_dp_display_get_modes(struct msm_dp *dp)
 
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 
-	return msm_dp_panel_get_modes(msm_dp_display->panel,
-		dp->connector);
+	return drm_edid_connector_add_modes(msm_dp_display->panel->connector);
 }
 
 bool msm_dp_display_check_video_test(struct msm_dp *dp)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 5b4954e7cb74..bde4a772d22c 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -232,8 +232,8 @@ static u32 msm_dp_panel_get_supported_bpp(struct msm_dp_panel *msm_dp_panel,
 	return min_supported_bpp;
 }
 
-int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
-	struct drm_connector *connector)
+int msm_dp_panel_read_link_caps(struct msm_dp_panel *msm_dp_panel,
+				struct drm_connector *connector)
 {
 	int rc, bw_code;
 	int count;
@@ -271,36 +271,9 @@ int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
 
 	rc = drm_dp_read_downstream_info(panel->aux, msm_dp_panel->dpcd,
 					 msm_dp_panel->downstream_ports);
-	if (rc)
-		return rc;
-
-	drm_edid_free(msm_dp_panel->drm_edid);
-
-	msm_dp_panel->drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc);
-
-	drm_edid_connector_update(connector, msm_dp_panel->drm_edid);
-
-	if (!msm_dp_panel->drm_edid) {
-		DRM_ERROR("panel edid read failed\n");
-		/* check edid read fail is due to unplug */
-		if (!msm_dp_aux_is_link_connected(panel->aux)) {
-			rc = -ETIMEDOUT;
-			goto end;
-		}
-	}
-
-end:
 	return rc;
 }
 
-void msm_dp_panel_unplugged(struct msm_dp_panel *msm_dp_panel,
-			    struct drm_connector *connector)
-{
-	drm_edid_connector_update(connector, NULL);
-	drm_edid_free(msm_dp_panel->drm_edid);
-	msm_dp_panel->drm_edid = NULL;
-}
-
 u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
 		u32 mode_edid_bpp, u32 mode_pclk_khz)
 {
@@ -324,22 +297,6 @@ u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
 	return bpp;
 }
 
-int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
-	struct drm_connector *connector)
-{
-	if (!msm_dp_panel) {
-		DRM_ERROR("invalid input\n");
-		return -EINVAL;
-	}
-
-	if (msm_dp_panel->drm_edid) {
-		drm_edid_connector_update(connector, msm_dp_panel->drm_edid);
-		return drm_edid_connector_add_modes(connector);
-	}
-
-	return 0;
-}
-
 static u8 msm_dp_panel_get_edid_checksum(const struct edid *edid)
 {
 	edid += edid->extensions;
@@ -347,7 +304,8 @@ static u8 msm_dp_panel_get_edid_checksum(const struct edid *edid)
 	return edid->checksum;
 }
 
-void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel)
+void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel,
+				      const struct drm_edid *drm_edid)
 {
 	struct msm_dp_panel_private *panel;
 
@@ -360,7 +318,7 @@ void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel)
 
 	if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
 		/* FIXME: get rid of drm_edid_raw() */
-		const struct edid *edid = drm_edid_raw(msm_dp_panel->drm_edid);
+		const struct edid *edid = drm_edid_raw(drm_edid);
 		u8 checksum;
 
 		if (edid)
@@ -757,10 +715,3 @@ struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux
 	return msm_dp_panel;
 }
 
-void msm_dp_panel_put(struct msm_dp_panel *msm_dp_panel)
-{
-	if (!msm_dp_panel)
-		return;
-
-	drm_edid_free(msm_dp_panel->drm_edid);
-}
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 9173e90a5053..53b7b4463551 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -33,7 +33,6 @@ struct msm_dp_panel {
 	u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 
 	struct msm_dp_link_info link_info;
-	const struct drm_edid *drm_edid;
 	struct drm_connector *connector;
 	struct msm_dp_display_mode msm_dp_mode;
 	struct msm_dp_panel_psr psr_cap;
@@ -47,15 +46,12 @@ struct msm_dp_panel {
 int msm_dp_panel_init_panel_info(struct msm_dp_panel *msm_dp_panel);
 int msm_dp_panel_deinit(struct msm_dp_panel *msm_dp_panel);
 int msm_dp_panel_timing_cfg(struct msm_dp_panel *msm_dp_panel, bool wide_bus_en);
-int msm_dp_panel_read_sink_caps(struct msm_dp_panel *msm_dp_panel,
-		struct drm_connector *connector);
-void msm_dp_panel_unplugged(struct msm_dp_panel *msm_dp_panel,
-			    struct drm_connector *connector);
+int msm_dp_panel_read_link_caps(struct msm_dp_panel *msm_dp_panel,
+				struct drm_connector *connector);
 u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel, u32 mode_max_bpp,
 			u32 mode_pclk_khz);
-int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
-		struct drm_connector *connector);
-void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel);
+void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel,
+				      const struct drm_edid *drm_edid);
 void msm_dp_panel_tpg_config(struct msm_dp_panel *msm_dp_panel, bool enable);
 
 void msm_dp_panel_clear_dsc_dto(struct msm_dp_panel *msm_dp_panel);
@@ -94,5 +90,4 @@ struct msm_dp_panel *msm_dp_panel_get(struct device *dev, struct drm_dp_aux *aux
 			      struct msm_dp_link *link,
 			      void __iomem *link_base,
 			      void __iomem *p0_base);
-void msm_dp_panel_put(struct msm_dp_panel *msm_dp_panel);
 #endif /* _DP_PANEL_H_ */

-- 
2.47.3

Re: [PATCH 16/24] drm/msm/dp: remove cached drm_edid from panel

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless modification of `connector->edid_blob_ptr` in HPD handlers.
--

--- Patch [16]: [PATCH 16/24] drm/msm/dp: remove cached drm_edid from panel ---
commit 379cd36eb86a5e848dbe71b567a862219cda4dab
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
Subject: drm/msm/dp: remove cached drm_edid from panel

This commit removes the cached drm_edid from the DP panel structure.
Instead of caching it, the EDID is read and updated directly in the plug
stage to simplify the code.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -272,6 +272,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  	const struct drm_display_info *info = &connector->display_info;
>  	int rc = 0;
>  	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> +	const struct drm_edid *drm_edid = NULL;
>  
>  	rc = drm_dp_read_dpcd_caps(dp->aux, dpcd);
>  	if (rc)
> @@ -279,10 +280,20 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
>  
>  	dp->link->lttpr_count = msm_dp_display_lttpr_init(dp, dpcd);
>  
> -	rc = msm_dp_panel_read_sink_caps(dp->panel, connector);
> +	rc = msm_dp_panel_read_link_caps(dp->panel, connector);
>  	if (rc)
>  		goto end;
>  
> +	drm_edid = drm_edid_read_ddc(connector, &dp->aux->ddc);
> +	drm_edid_connector_update(connector, drm_edid);

[Severity: High]
This isn't a bug introduced by this patch, but does calling
drm_edid_connector_update() here in the hotplug handler occur without holding
drm_mode_config.mutex?

If this is called from an IRQ thread holding only the driver-local
dp->plugged_lock, drm_edid_connector_update() will replace
connector->edid_blob_ptr and decrement the old blob's reference count.

Concurrently, if userspace reads the EDID via sysfs or queries modes via
ioctl, it will acquire drm_mode_config.mutex and directly dereference the
blob. Can this lead to a Use-After-Free if the EDID blob is freed while
readers are still accessing it?

[ ... ]
> @@ -456,7 +468,7 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
>  
>  	/* Don't forget modes for eDP */
>  	if (!dp->msm_dp_display.is_edp)
> -		msm_dp_panel_unplugged(dp->panel, dp->msm_dp_display.connector);
> +		drm_edid_connector_update(dp->msm_dp_display.connector, NULL);

[Severity: High]
This is a pre-existing issue, but a similar lockless update to the
connector's EDID property blob seems to happen here during an unplug event.

Does this unplug path also need to hold drm_mode_config.mutex to prevent a
potential Use-After-Free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=16

[PATCH 17/24] drm/msm/dp: drop deprecated .mode_set() and use .atomic_pre_enable

[Dmitry Baryshkov]

From: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

The bridge .mode_set() callback is deprecated. Remove it and move the
mode setup logic to .atomic_pre_enable(), where the adjusted_mode is
available from the atomic CRTC state.

.atomic_pre_enable() is used rather than .atomic_enable() because the DPU
encoder's .atomic_enable() reads the output mode's YUV420 / wide bus
state through the msm_display callbacks, and it runs after all bridges'
.atomic_pre_enable() but before their .atomic_enable(). Programming the
mode from the DP bridge's .atomic_enable() would leave the encoder
reading the previously committed mode's state.

Drop msm_dp_mode from msm_dp_display_private and store the mode directly
in the panel, as it was only used as a temporary cache.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Assisted-by: Claude:claude-opus-4-8
[DB: moved to atomic_pre_enable]
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 103 ++++++++++++++++--------------------
 drivers/gpu/drm/msm/dp/dp_drm.c     |   4 +-
 drivers/gpu/drm/msm/dp/dp_drm.h     |   5 +-
 3 files changed, 51 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index cd4923d1a17e..7f55cace596f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -63,7 +63,6 @@ struct msm_dp_display_private {
 	struct msm_dp_panel   *panel;
 	struct msm_dp_ctrl    *ctrl;
 
-	struct msm_dp_display_mode msm_dp_mode;
 	struct msm_dp msm_dp_display;
 
 	/* wait for audio signaling */
@@ -600,16 +599,33 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
 }
 
 static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
-			       struct msm_dp_display_mode *mode)
+				   const struct drm_display_mode *adjusted_mode,
+				   struct msm_dp_panel *msm_dp_panel)
 {
 	struct msm_dp_display_private *dp;
+	u32 bpp;
 
 	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
 
-	drm_mode_copy(&dp->panel->msm_dp_mode.drm_mode, &mode->drm_mode);
-	dp->panel->msm_dp_mode.bpp = mode->bpp;
-	dp->panel->msm_dp_mode.out_fmt_is_yuv_420 = mode->out_fmt_is_yuv_420;
-	msm_dp_panel_init_panel_info(dp->panel);
+	drm_mode_copy(&msm_dp_panel->msm_dp_mode.drm_mode, adjusted_mode);
+	if (msm_dp_display_check_video_test(msm_dp_display))
+		bpp = msm_dp_display_get_test_bpp(msm_dp_display);
+	else
+		bpp = msm_dp_panel->connector->display_info.bpc * 3;
+
+	msm_dp_panel->msm_dp_mode.bpp = bpp ? bpp : 24; /* Default bpp */
+	msm_dp_panel->msm_dp_mode.v_active_low =
+		!!(adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC);
+	msm_dp_panel->msm_dp_mode.h_active_low =
+		!!(adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC);
+	msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420 =
+		drm_mode_is_420_only(&msm_dp_panel->connector->display_info, adjusted_mode) &&
+		msm_dp_panel->vsc_sdp_supported;
+	msm_dp_panel_init_panel_info(msm_dp_panel);
+
+	/* populate wide_bus_support to different layers */
+	dp->ctrl->wide_bus_en =
+		msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420 ? false : dp->wide_bus_supported;
 	return 0;
 }
 
@@ -1310,7 +1326,7 @@ bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
 
 	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
 
-	if (dp->msm_dp_mode.out_fmt_is_yuv_420)
+	if (dp->panel->msm_dp_mode.out_fmt_is_yuv_420)
 		return false;
 
 	return dp->wide_bus_supported;
@@ -1414,6 +1430,30 @@ static const struct msm_display_funcs msm_dp_display_funcs = {
 	.needs_encoder = msm_dp_display_needs_encoder,
 };
 
+void msm_dp_bridge_atomic_pre_enable(struct drm_bridge *drm_bridge,
+				     struct drm_atomic_commit *state)
+{
+	struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(drm_bridge);
+	struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
+	struct msm_dp_display_private *msm_dp_display;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+
+	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
+
+	crtc = drm_atomic_get_new_crtc_for_encoder(state, drm_bridge->encoder);
+	if (!crtc)
+		return;
+	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+	/*
+	 * The DPU encoder's .atomic_enable() reads the mode's YUV420 / wide bus
+	 * state and runs before the bridge's .atomic_enable(), so the mode must
+	 * be programmed here, in .atomic_pre_enable().
+	 */
+	msm_dp_display_set_mode(dp, &crtc_state->adjusted_mode, msm_dp_display->panel);
+}
+
 void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 				 struct drm_atomic_commit *state)
 {
@@ -1424,10 +1464,6 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 	bool force_link_train = false;
 
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
-	if (!msm_dp_display->msm_dp_mode.drm_mode.clock) {
-		DRM_ERROR("invalid params\n");
-		return;
-	}
 
 	if (dp->is_edp)
 		msm_dp_hpd_plug_handle(msm_dp_display);
@@ -1440,12 +1476,6 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 	if (msm_dp_display->link->sink_count == 0)
 		return;
 
-	rc = msm_dp_display_set_mode(dp, &msm_dp_display->msm_dp_mode);
-	if (rc) {
-		DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
-		return;
-	}
-
 	if (!dp->power_on) {
 		msm_dp_display_host_phy_init(msm_dp_display);
 		force_link_train = true;
@@ -1500,45 +1530,6 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
 	pm_runtime_put_sync(&dp->pdev->dev);
 }
 
-void msm_dp_bridge_mode_set(struct drm_bridge *drm_bridge,
-			const struct drm_display_mode *mode,
-			const struct drm_display_mode *adjusted_mode)
-{
-	struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(drm_bridge);
-	struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
-	struct msm_dp_display_private *msm_dp_display;
-	struct msm_dp_panel *msm_dp_panel;
-
-	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
-	msm_dp_panel = msm_dp_display->panel;
-
-	memset(&msm_dp_display->msm_dp_mode, 0x0, sizeof(struct msm_dp_display_mode));
-
-	if (msm_dp_display_check_video_test(dp))
-		msm_dp_display->msm_dp_mode.bpp = msm_dp_display_get_test_bpp(dp);
-	else /* Default num_components per pixel = 3 */
-		msm_dp_display->msm_dp_mode.bpp = dp->connector->display_info.bpc * 3;
-
-	if (!msm_dp_display->msm_dp_mode.bpp)
-		msm_dp_display->msm_dp_mode.bpp = 24; /* Default bpp */
-
-	drm_mode_copy(&msm_dp_display->msm_dp_mode.drm_mode, adjusted_mode);
-
-	msm_dp_display->msm_dp_mode.v_active_low =
-		!!(msm_dp_display->msm_dp_mode.drm_mode.flags & DRM_MODE_FLAG_NVSYNC);
-
-	msm_dp_display->msm_dp_mode.h_active_low =
-		!!(msm_dp_display->msm_dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
-
-	msm_dp_display->msm_dp_mode.out_fmt_is_yuv_420 =
-		drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode) &&
-		msm_dp_panel->vsc_sdp_supported;
-
-	/* populate wide_bus_support to different layers */
-	msm_dp_display->ctrl->wide_bus_en =
-		msm_dp_display->msm_dp_mode.out_fmt_is_yuv_420 ? false : msm_dp_display->wide_bus_supported;
-}
-
 void msm_dp_bridge_hpd_enable(struct drm_bridge *bridge)
 {
 	struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(bridge);
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 4f615b45fff7..da005babe411 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -53,10 +53,10 @@ static const struct drm_bridge_funcs msm_dp_bridge_ops = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state   = drm_atomic_helper_bridge_destroy_state,
 	.atomic_create_state           = drm_atomic_helper_bridge_create_state,
+	.atomic_pre_enable      = msm_dp_bridge_atomic_pre_enable,
 	.atomic_enable          = msm_dp_bridge_atomic_enable,
 	.atomic_disable         = msm_dp_bridge_atomic_disable,
 	.atomic_post_disable    = msm_dp_bridge_atomic_post_disable,
-	.mode_set     = msm_dp_bridge_mode_set,
 	.mode_valid   = msm_dp_bridge_mode_valid,
 	.get_modes    = msm_dp_bridge_get_modes,
 	.detect       = msm_dp_bridge_detect,
@@ -230,10 +230,10 @@ static void msm_edp_bridge_debugfs_init(struct drm_bridge *bridge, struct dentry
 }
 
 static const struct drm_bridge_funcs msm_edp_bridge_ops = {
+	.atomic_pre_enable = msm_dp_bridge_atomic_pre_enable,
 	.atomic_enable = msm_edp_bridge_atomic_enable,
 	.atomic_disable = msm_edp_bridge_atomic_disable,
 	.atomic_post_disable = msm_edp_bridge_atomic_post_disable,
-	.mode_set = msm_dp_bridge_mode_set,
 	.mode_valid = msm_edp_bridge_mode_valid,
 	.atomic_create_state = drm_atomic_helper_bridge_create_state,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index 3b046822f05e..4c6b14dd52ac 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -26,6 +26,8 @@ int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
 
 enum drm_connector_status msm_dp_bridge_detect(struct drm_bridge *bridge,
 					       struct drm_connector *connector);
+void msm_dp_bridge_atomic_pre_enable(struct drm_bridge *drm_bridge,
+				     struct drm_atomic_commit *state);
 void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 				 struct drm_atomic_commit *state);
 void msm_dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
@@ -35,9 +37,6 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
 enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
 					  const struct drm_display_info *info,
 					  const struct drm_display_mode *mode);
-void msm_dp_bridge_mode_set(struct drm_bridge *drm_bridge,
-			const struct drm_display_mode *mode,
-			const struct drm_display_mode *adjusted_mode);
 void msm_dp_bridge_hpd_enable(struct drm_bridge *bridge);
 void msm_dp_bridge_hpd_disable(struct drm_bridge *bridge);
 void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,

-- 
2.47.3

[PATCH 18/24] drm/msm/hdmi: cache is_hdmi instead of storing the connector

[Dmitry Baryshkov]

The HDMI driver stored the drm_connector in struct hdmi solely so that a
handful of call sites could consult connector->display_info.is_hdmi, and
so that the hotplug work could hand a connector to drm_bridge_detect().

Several of those call sites run outside of an atomic commit (the HPD
enable/disable paths in msm_hdmi_set_mode(), and the audio update path),
so they have no drm_atomic_state to derive the connector from and thus
relied on the stored pointer.

The only information actually consumed is the is_hdmi boolean. Cache it
in struct hdmi and refresh it from a new .hpd_notify bridge callback:
drm_bridge_connector_detect() updates the connector's EDID (and hence
display_info.is_hdmi) via drm_atomic_helper_connector_hdmi_hotplug()
before invoking .hpd_notify, so the cached value is always up to date.

With that, the stored connector becomes local to msm_hdmi_modeset_init():
the connector object is owned by the DRM core after creation, so there is
no need to keep a driver-side reference. The hotplug work now calls
msm_hdmi_bridge_detect() directly, which ignores its connector argument
anyway.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c        | 17 ++++++++---------
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  4 +++-
 drivers/gpu/drm/msm/hdmi/hdmi_audio.c  |  2 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 16 ++++++++++++++--
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 1d5a4b63f3c8..62961dddd8f5 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -27,7 +27,7 @@ void msm_hdmi_set_mode(struct hdmi *hdmi, bool power_on)
 	spin_lock_irqsave(&hdmi->reg_lock, flags);
 	if (power_on) {
 		ctrl |= HDMI_CTRL_ENABLE;
-		if (!hdmi->connector->display_info.is_hdmi) {
+		if (!hdmi->is_hdmi) {
 			ctrl |= HDMI_CTRL_HDMI;
 			hdmi_write(hdmi, REG_HDMI_CTRL, ctrl);
 			ctrl &= ~HDMI_CTRL_HDMI;
@@ -164,6 +164,7 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
 		struct drm_device *dev, struct drm_encoder *encoder)
 {
 	struct hdmi *hdmi = container_of(display, struct hdmi, display);
+	struct drm_connector *connector = NULL;
 	int ret;
 
 	hdmi->dev = dev;
@@ -184,11 +185,11 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
 		}
 	}
 
-	hdmi->connector = drm_bridge_connector_init(hdmi->dev, encoder);
-	if (IS_ERR(hdmi->connector)) {
-		ret = PTR_ERR(hdmi->connector);
+	connector = drm_bridge_connector_init(hdmi->dev, encoder);
+	if (IS_ERR(connector)) {
+		ret = PTR_ERR(connector);
 		DRM_DEV_ERROR(dev->dev, "failed to create HDMI connector: %d\n", ret);
-		hdmi->connector = NULL;
+		connector = NULL;
 		goto fail;
 	}
 
@@ -204,10 +205,8 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
 	return 0;
 
 fail:
-	if (hdmi->connector) {
-		hdmi->connector->funcs->destroy(hdmi->connector);
-		hdmi->connector = NULL;
-	}
+	if (connector)
+		connector->funcs->destroy(connector);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 869f55d4704a..d4adaa6ccdd5 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -47,6 +47,9 @@ struct hdmi {
 	struct mutex state_mutex; /* protects two booleans */
 	unsigned long pixclock;
 
+	/* cached from the connector's EDID, updated on hotplug */
+	bool is_hdmi;
+
 	void __iomem *mmio;
 	phys_addr_t mmio_size;
 	void __iomem *qfprom_mmio;
@@ -62,7 +65,6 @@ struct hdmi {
 	struct device *phy_dev;
 
 	struct i2c_adapter *i2c;
-	struct drm_connector *connector;
 	struct drm_bridge *bridge;
 
 	struct drm_bridge *next_bridge;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_audio.c b/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
index 249c167ab04d..ce1864f3f659 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
@@ -19,7 +19,7 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 	bool enabled = audio->enabled;
 	u32 acr_pkt_ctrl, vbi_pkt_ctrl, aud_pkt_ctrl, audio_config;
 
-	if (!hdmi->connector->display_info.is_hdmi)
+	if (!hdmi->is_hdmi)
 		return -EINVAL;
 
 	DBG("audio: enabled=%d, channels=%d, rate=%d",
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 7abb9243dba5..84a423af73e9 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -345,7 +345,7 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
 	if (hdmi->power_on) {
 		power_off(bridge);
 		hdmi->power_on = false;
-		if (hdmi->connector->display_info.is_hdmi)
+		if (hdmi->is_hdmi)
 			msm_hdmi_audio_update(hdmi);
 		msm_hdmi_phy_resource_disable(phy);
 	}
@@ -452,6 +452,17 @@ static enum drm_mode_status msm_hdmi_bridge_tmds_char_rate_valid(const struct dr
 	return 0;
 }
 
+static void msm_hdmi_bridge_hpd_notify(struct drm_bridge *bridge,
+				       struct drm_connector *connector,
+				       enum drm_connector_status status)
+{
+	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
+	struct hdmi *hdmi = hdmi_bridge->hdmi;
+
+	/* called after the EDID update, so display_info is up to date */
+	hdmi->is_hdmi = connector->display_info.is_hdmi;
+}
+
 static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -460,6 +471,7 @@ static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
 	.atomic_post_disable = msm_hdmi_bridge_atomic_post_disable,
 	.edid_read = msm_hdmi_bridge_edid_read,
 	.detect = msm_hdmi_bridge_detect,
+	.hpd_notify = msm_hdmi_bridge_hpd_notify,
 	.hpd_enable = msm_hdmi_hpd_enable,
 	.hpd_disable = msm_hdmi_hpd_disable,
 	.hdmi_tmds_char_rate_valid = msm_hdmi_bridge_tmds_char_rate_valid,
@@ -482,7 +494,7 @@ msm_hdmi_hotplug_work(struct work_struct *work)
 		container_of(work, struct hdmi_bridge, hpd_work);
 	struct drm_bridge *bridge = &hdmi_bridge->base;
 
-	drm_bridge_hpd_notify(bridge, drm_bridge_detect(bridge, hdmi_bridge->hdmi->connector));
+	drm_bridge_hpd_notify(bridge, msm_hdmi_bridge_detect(bridge, NULL));
 }
 
 /* initialize bridge */

-- 
2.47.3

Re: [PATCH 18/24] drm/msm/hdmi: cache is_hdmi instead of storing the connector

[sashiko-bot]

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

New issues:
- [High] Missing State Synchronization when connector state is forced via sysfs.
- [Medium] Data Race on `hdmi->is_hdmi` boolean.

Pre-existing issues:
- [High] NULL Pointer Dereference on drmm-managed connector destroy hook.
--

--- Patch [18]: [PATCH 18/24] drm/msm/hdmi: cache is_hdmi instead of storing the connector ---
commit c8e64d4ebbe7d3c6a0a2cb94dd5d7a4af528ed77
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/hdmi: cache is_hdmi instead of storing the connector

This commit updates the HDMI driver to cache the is_hdmi boolean from the
EDID rather than retaining a driver-side pointer to the drm_connector. The
cached value is refreshed via a new hpd_notify bridge callback.

> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 1d5a4b63f3c8f..62961dddd8f5e 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c

[ ... ]

> @@ -204,10 +205,8 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
>  	return 0;
>  
>  fail:
> -	if (hdmi->connector) {
> -		hdmi->connector->funcs->destroy(hdmi->connector);
> -		hdmi->connector = NULL;
> -	}
> +	if (connector)
> +		connector->funcs->destroy(connector);

[Severity: High]
This is a pre-existing issue, but does calling the destroy hook here cause a
NULL pointer dereference? 
    
The connector is created earlier in msm_hdmi_modeset_init() via
drm_bridge_connector_init(), which uses the drmm-managed API
drmm_connector_init(). The DRM-managed API explicitly requires funcs->destroy
to be NULL, otherwise it returns -EINVAL. 
    
This means connector->funcs->destroy is guaranteed to be NULL when this error
path is taken, which would cause a panic when dereferenced.

>  
>  	return ret;
>  }

[ ... ]

> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 7abb9243dba57..84a423af73e93 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c

[ ... ]

> @@ -452,6 +452,17 @@ static enum drm_mode_status msm_hdmi_bridge_tmds_char_rate_valid(const struct dr
>  	return 0;
>  }
>  
> +static void msm_hdmi_bridge_hpd_notify(struct drm_bridge *bridge,
> +				       struct drm_connector *connector,
> +				       enum drm_connector_status status)
> +{
> +	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> +	struct hdmi *hdmi = hdmi_bridge->hdmi;
> +
> +	/* called after the EDID update, so display_info is up to date */
> +	hdmi->is_hdmi = connector->display_info.is_hdmi;

[Severity: High]
Can the cached hdmi->is_hdmi state become stale if the connector is forced
via sysfs? 

When userspace forces the connector state (for example, by echoing on to the
status file), the DRM core sets the status and invokes 
connector->funcs->force(), which maps to drm_bridge_connector_force(). 

This force operation successfully updates the EDID and display_info.is_hdmi,
but it bypasses the standard detect hook and does not invoke the hpd_notify
callback. Will this missing state synchronization cause HDMI-specific hardware
programming to incorrectly fall back to DVI mode and drop audio?

[Severity: Medium]
Could there be a data race when accessing hdmi->is_hdmi? 

This variable is updated here in msm_hdmi_bridge_hpd_notify() from the
asynchronous hotplug workqueue thread. Concurrently, it is read in
msm_hdmi_set_mode() under the reg_lock spinlock, and in msm_hdmi_audio_update()
from audio subsystem paths. 

Since there is no common lock protecting this cross-thread state access, and
neither READ_ONCE() nor WRITE_ONCE() is used, will this trigger KCSAN
warnings?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=18

[PATCH 19/24] drm/msm/dp: drop redundant panel->connector

[Dmitry Baryshkov]

struct msm_dp_panel kept its own copy of the drm_connector, a plain alias
of msm_dp.connector assigned once in msm_dp_modeset_init(). It was only
dereferenced in three places, each of which either already has a
connector at hand or is reached from a caller that does:

 - msm_dp_display_set_mode() runs from the .atomic_enable bridge
   callback, which can fetch the connector from the atomic state;
 - msm_dp_display_get_modes() is called from the .get_modes bridge
   callback, which is passed the connector;
 - msm_dp_panel_handle_sink_request() is called from
   msm_dp_display_process_hpd_high(), which holds the connector.

Pass the connector down to those functions and drop the duplicate field.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 27 +++++++++++----------------
 drivers/gpu/drm/msm/dp/dp_display.h |  3 ++-
 drivers/gpu/drm/msm/dp/dp_drm.c     |  2 +-
 drivers/gpu/drm/msm/dp/dp_panel.c   |  5 +++--
 drivers/gpu/drm/msm/dp/dp_panel.h   |  4 ++--
 5 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 7f55cace596f..461420a72820 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -304,7 +304,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
 	dp->msm_dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled;
 
 	dp->audio_supported = info->has_audio;
-	msm_dp_panel_handle_sink_request(dp->panel, drm_edid);
+	msm_dp_panel_handle_sink_request(dp->panel, drm_edid, connector);
 
 	/*
 	 * set sink to normal operation mode -- D0
@@ -600,7 +600,8 @@ static int msm_dp_init_sub_modules(struct msm_dp_display_private *dp)
 
 static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 				   const struct drm_display_mode *adjusted_mode,
-				   struct msm_dp_panel *msm_dp_panel)
+				   struct msm_dp_panel *msm_dp_panel,
+				   struct drm_connector *connector)
 {
 	struct msm_dp_display_private *dp;
 	u32 bpp;
@@ -611,7 +612,7 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 	if (msm_dp_display_check_video_test(msm_dp_display))
 		bpp = msm_dp_display_get_test_bpp(msm_dp_display);
 	else
-		bpp = msm_dp_panel->connector->display_info.bpc * 3;
+		bpp = connector->display_info.bpc * 3;
 
 	msm_dp_panel->msm_dp_mode.bpp = bpp ? bpp : 24; /* Default bpp */
 	msm_dp_panel->msm_dp_mode.v_active_low =
@@ -619,7 +620,7 @@ static int msm_dp_display_set_mode(struct msm_dp *msm_dp_display,
 	msm_dp_panel->msm_dp_mode.h_active_low =
 		!!(adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC);
 	msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420 =
-		drm_mode_is_420_only(&msm_dp_panel->connector->display_info, adjusted_mode) &&
+		drm_mode_is_420_only(&connector->display_info, adjusted_mode) &&
 		msm_dp_panel->vsc_sdp_supported;
 	msm_dp_panel_init_panel_info(msm_dp_panel);
 
@@ -769,18 +770,14 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-int msm_dp_display_get_modes(struct msm_dp *dp)
+int msm_dp_display_get_modes(struct msm_dp *dp, struct drm_connector *connector)
 {
-	struct msm_dp_display_private *msm_dp_display;
-
 	if (!dp) {
 		DRM_ERROR("invalid params\n");
 		return 0;
 	}
 
-	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
-
-	return drm_edid_connector_add_modes(msm_dp_display->panel->connector);
+	return drm_edid_connector_add_modes(connector);
 }
 
 bool msm_dp_display_check_video_test(struct msm_dp *dp)
@@ -1350,13 +1347,10 @@ static int msm_dp_modeset_init(struct msm_display *display,
 			       struct drm_device *dev, struct drm_encoder *encoder)
 {
 	struct msm_dp *msm_dp_display = container_of(display, struct msm_dp, display);
-	struct msm_dp_display_private *msm_dp_priv;
 	int ret;
 
 	msm_dp_display->drm_dev = dev;
 
-	msm_dp_priv = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
-
 	ret = msm_dp_bridge_init(msm_dp_display, dev, encoder);
 	if (ret) {
 		DRM_DEV_ERROR(dev->dev,
@@ -1373,8 +1367,6 @@ static int msm_dp_modeset_init(struct msm_display *display,
 		return ret;
 	}
 
-	msm_dp_priv->panel->connector = msm_dp_display->connector;
-
 	return 0;
 }
 
@@ -1438,9 +1430,12 @@ void msm_dp_bridge_atomic_pre_enable(struct drm_bridge *drm_bridge,
 	struct msm_dp_display_private *msm_dp_display;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
+	struct drm_connector *connector;
 
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 
+	connector = drm_atomic_get_new_connector_for_encoder(state, drm_bridge->encoder);
+
 	crtc = drm_atomic_get_new_crtc_for_encoder(state, drm_bridge->encoder);
 	if (!crtc)
 		return;
@@ -1451,7 +1446,7 @@ void msm_dp_bridge_atomic_pre_enable(struct drm_bridge *drm_bridge,
 	 * state and runs before the bridge's .atomic_enable(), so the mode must
 	 * be programmed here, in .atomic_pre_enable().
 	 */
-	msm_dp_display_set_mode(dp, &crtc_state->adjusted_mode, msm_dp_display->panel);
+	msm_dp_display_set_mode(dp, &crtc_state->adjusted_mode, msm_dp_display->panel, connector);
 }
 
 void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 8c34a473c7db..f1523dbe33d5 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -28,7 +28,8 @@ struct msm_dp {
 	bool psr_supported;
 };
 
-int msm_dp_display_get_modes(struct msm_dp *msm_dp_display);
+int msm_dp_display_get_modes(struct msm_dp *msm_dp_display,
+			     struct drm_connector *connector);
 bool msm_dp_display_check_video_test(struct msm_dp *msm_dp_display);
 int msm_dp_display_get_test_bpp(struct msm_dp *msm_dp_display);
 void msm_dp_display_signal_audio_start(struct msm_dp *msm_dp_display);
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index da005babe411..8dbe7d682ddf 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -32,7 +32,7 @@ static int msm_dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connect
 	dp = to_dp_bridge(bridge)->msm_dp_display;
 
 	/* pluggable case assumes EDID is read when HPD */
-	rc = msm_dp_display_get_modes(dp);
+	rc = msm_dp_display_get_modes(dp, connector);
 	if (rc <= 0) {
 		DRM_ERROR("failed to get DP sink modes, rc=%d\n", rc);
 		return rc;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index bde4a772d22c..30a44e7e8e62 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -305,7 +305,8 @@ static u8 msm_dp_panel_get_edid_checksum(const struct edid *edid)
 }
 
 void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel,
-				      const struct drm_edid *drm_edid)
+				      const struct drm_edid *drm_edid,
+				      struct drm_connector *connector)
 {
 	struct msm_dp_panel_private *panel;
 
@@ -324,7 +325,7 @@ void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel,
 		if (edid)
 			checksum = msm_dp_panel_get_edid_checksum(edid);
 		else
-			checksum = msm_dp_panel->connector->real_edid_checksum;
+			checksum = connector->real_edid_checksum;
 
 		msm_dp_link_send_edid_checksum(panel->link, checksum);
 		msm_dp_link_send_test_response(panel->link);
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 53b7b4463551..f73e11e57ced 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -33,7 +33,6 @@ struct msm_dp_panel {
 	u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 
 	struct msm_dp_link_info link_info;
-	struct drm_connector *connector;
 	struct msm_dp_display_mode msm_dp_mode;
 	struct msm_dp_panel_psr psr_cap;
 	bool video_test;
@@ -51,7 +50,8 @@ int msm_dp_panel_read_link_caps(struct msm_dp_panel *msm_dp_panel,
 u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel, u32 mode_max_bpp,
 			u32 mode_pclk_khz);
 void msm_dp_panel_handle_sink_request(struct msm_dp_panel *msm_dp_panel,
-				      const struct drm_edid *drm_edid);
+				      const struct drm_edid *drm_edid,
+				      struct drm_connector *connector);
 void msm_dp_panel_tpg_config(struct msm_dp_panel *msm_dp_panel, bool enable);
 
 void msm_dp_panel_clear_dsc_dto(struct msm_dp_panel *msm_dp_panel);

-- 
2.47.3

[PATCH 20/24] drm/msm/dp: use drm_display_info in mode_valid callbacks

[Dmitry Baryshkov]

The .mode_valid bridge callback is passed the sink's drm_display_info
directly, so msm_dp_bridge_mode_valid() and msm_edp_bridge_mode_valid()
have no need to reach into the stored connector for it. Use the supplied
info pointer and drop the now-pointless !dp->connector guard.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 6 +++---
 drivers/gpu/drm/msm/dp/dp_drm.c     | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 461420a72820..3397a3a47ef3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -731,7 +731,7 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
 
 	dp = to_dp_bridge(bridge)->msm_dp_display;
 
-	if (!dp || !mode_pclk_khz || !dp->connector) {
+	if (!dp || !mode_pclk_khz) {
 		DRM_ERROR("invalid params\n");
 		return -EINVAL;
 	}
@@ -739,7 +739,7 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 	link_info = &msm_dp_display->panel->link_info;
 
-	is_yuv_420 = drm_mode_is_420_only(&dp->connector->display_info, mode);
+	is_yuv_420 = drm_mode_is_420_only(info, mode);
 
 	/*
 	 * YUV 420 is carried over DP by signalling the colorimetry through a
@@ -754,7 +754,7 @@ enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
 	if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)
 		return MODE_CLOCK_HIGH;
 
-	mode_bpp = dp->connector->display_info.bpc * num_components;
+	mode_bpp = info->bpc * num_components;
 	if (!mode_bpp)
 		mode_bpp = default_bpp;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 8dbe7d682ddf..6b55753da0f7 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -203,7 +203,7 @@ static enum drm_mode_status msm_edp_bridge_mode_valid(struct drm_bridge *bridge,
 
 	dp = to_dp_bridge(bridge)->msm_dp_display;
 
-	if (!dp || !mode_pclk_khz || !dp->connector) {
+	if (!dp || !mode_pclk_khz) {
 		DRM_ERROR("invalid params\n");
 		return -EINVAL;
 	}

-- 
2.47.3

[PATCH 21/24] drm/msm/dp: guard subconnector setup on the connector type

[Dmitry Baryshkov]

The DP subconnector property only applies to full DisplayPort outputs.
This was expressed as "!is_edp", which infers the property's
applicability indirectly from the port flavour rather than from the
connector it is attached to. Besides eDP, that is also wrong for an
HDMI output driven by the DP controller through a DP-to-HDMI bridge:
such a sink is not eDP either, so it would be handed a DP subconnector
property it should never get.

Key the DP subconnector calls off the connector type directly
(DRM_MODE_CONNECTOR_DisplayPort) instead. This ties the decision to the
object the property lives on and correctly covers both cases, skipping
eDP as well as HDMI (via a DP-to-HDMI bridge) outputs.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
 drivers/gpu/drm/msm/dp/dp_drm.c     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3397a3a47ef3..4f6da636f7c6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -295,7 +295,7 @@ static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
 
 	msm_dp_link_process_request(dp->link);
 
-	if (!dp->msm_dp_display.is_edp)
+	if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
 		drm_dp_set_subconnector_property(connector,
 						 connector_status_connected,
 						 dp->panel->dpcd,
@@ -477,7 +477,7 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 	 * We don't need separate work for disconnect as
 	 * connect/attention interrupts are disabled
 	 */
-	if (!dp->msm_dp_display.is_edp)
+	if (dp->msm_dp_display.connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
 		drm_dp_set_subconnector_property(dp->msm_dp_display.connector,
 						 connector_status_disconnected,
 						 dp->panel->dpcd,
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 6b55753da0f7..00653b0364dc 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -323,7 +323,7 @@ struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
 	if (IS_ERR(connector))
 		return connector;
 
-	if (!msm_dp_display->is_edp)
+	if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
 		drm_connector_attach_dp_subconnector_property(connector);
 
 	return connector;

-- 
2.47.3

[PATCH 22/24] drm/msm/dp: stop storing the connector in struct msm_dp

[Dmitry Baryshkov]

struct msm_dp cached the drm_connector so that the asynchronous HPD
handlers, the YUV-420 query and debugfs could reach it. Keeping this
pointer means any common code that creates the connector has to hand it
back to the DP sub-block, and it couples the sub-block to a connector it
does not own.

Every consumer can instead obtain the connector from a context that
already has it:

 - the HPD handler chain (plug/unplug/irq -> process_hpd_high,
   handle_plugged_change) is only ever entered from the bridge
   .hpd_notify callback and the atomic enable/post_disable callbacks,
   all of which carry (or can derive from the atomic state) the
   connector; thread it through;

 - the .is_yuv_mode op is called from the DPU encoder, which already
   fetches the connector from the atomic state - pass it in;

 - .needs_periph_flush is evaluated at flush time, after ->mode_set()
   has cached the result in msm_dp_mode.out_fmt_is_yuv_420, so read the
   cached flag rather than the live connector;

 - debugfs only needs a connector to report status, so look it up from
   the sub-block's encoder.

With that, drop the stored connector.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 108 ++++++++++++++++++++++++------------
 drivers/gpu/drm/msm/dp/dp_display.h |   1 -
 2 files changed, 74 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 4f6da636f7c6..44503789d454 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -265,9 +265,9 @@ static int msm_dp_display_lttpr_init(struct msm_dp_display_private *dp, u8 *dpcd
 	return lttpr_count;
 }
 
-static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp)
+static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp,
+					   struct drm_connector *connector)
 {
-	struct drm_connector *connector = dp->msm_dp_display.connector;
 	const struct drm_display_info *info = &connector->display_info;
 	int rc = 0;
 	u8 dpcd[DP_RECEIVER_CAP_SIZE];
@@ -403,7 +403,8 @@ static int msm_dp_display_handle_irq_hpd(struct msm_dp_display_private *dp)
 	return 0;
 }
 
-static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
+static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp,
+				  struct drm_connector *connector)
 {
 	int ret;
 	struct platform_device *pdev = dp->msm_dp_display.pdev;
@@ -424,7 +425,7 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
 
 	msm_dp_display_host_phy_init(dp);
 
-	ret = msm_dp_display_process_hpd_high(dp);
+	ret = msm_dp_display_process_hpd_high(dp, connector);
 
 	drm_dbg_dp(dp->drm_dev, "After, type=%d sink_count=%d\n",
 			dp->msm_dp_display.connector_type,
@@ -436,7 +437,7 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
 };
 
 static void msm_dp_display_handle_plugged_change(struct msm_dp *msm_dp_display,
-		bool plugged)
+		struct drm_connector *connector, bool plugged)
 {
 	struct msm_dp_display_private *dp;
 
@@ -445,11 +446,11 @@ static void msm_dp_display_handle_plugged_change(struct msm_dp *msm_dp_display,
 
 	/* notify audio subsystem only if sink supports audio */
 	if (dp->audio_supported)
-		drm_connector_hdmi_audio_plugged_notify(msm_dp_display->connector,
-							plugged);
+		drm_connector_hdmi_audio_plugged_notify(connector, plugged);
 }
 
-static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
+static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp,
+				    struct drm_connector *connector)
 {
 	struct platform_device *pdev = dp->msm_dp_display.pdev;
 
@@ -467,7 +468,7 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 
 	/* Don't forget modes for eDP */
 	if (!dp->msm_dp_display.is_edp)
-		drm_edid_connector_update(dp->msm_dp_display.connector, NULL);
+		drm_edid_connector_update(connector, NULL);
 
 	/* triggered by irq_hdp with sink_count = 0 */
 	if (dp->link->sink_count == 0)
@@ -477,14 +478,14 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 	 * We don't need separate work for disconnect as
 	 * connect/attention interrupts are disabled
 	 */
-	if (dp->msm_dp_display.connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
-		drm_dp_set_subconnector_property(dp->msm_dp_display.connector,
+	if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+		drm_dp_set_subconnector_property(connector,
 						 connector_status_disconnected,
 						 dp->panel->dpcd,
 						 dp->panel->downstream_ports);
 
 	/* signal the disconnect event early to ensure proper teardown */
-	msm_dp_display_handle_plugged_change(&dp->msm_dp_display, false);
+	msm_dp_display_handle_plugged_change(&dp->msm_dp_display, connector, false);
 
 	drm_dbg_dp(dp->drm_dev, "After, type=%d, sink_count=%d\n",
 			dp->msm_dp_display.connector_type,
@@ -498,7 +499,8 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 	return 0;
 }
 
-static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
+static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp,
+				 struct drm_connector *connector)
 {
 	u32 sink_request;
 	int rc = 0;
@@ -514,7 +516,7 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
 		sink_request = dp->link->sink_request;
 		drm_dbg_dp(dp->drm_dev, "sink_request=%d\n", sink_request);
 		if (sink_request & DS_PORT_STATUS_CHANGED)
-			rc = msm_dp_display_process_hpd_high(dp);
+			rc = msm_dp_display_process_hpd_high(dp, connector);
 		else
 			rc = msm_dp_display_handle_irq_hpd(dp);
 	}
@@ -648,7 +650,8 @@ static int msm_dp_display_enable(struct msm_dp_display_private *dp, bool force_l
 	return rc;
 }
 
-static int msm_dp_display_post_enable(struct msm_dp *msm_dp_display)
+static int msm_dp_display_post_enable(struct msm_dp *msm_dp_display,
+				      struct drm_connector *connector)
 {
 	struct msm_dp_display_private *dp;
 	u32 rate;
@@ -663,7 +666,7 @@ static int msm_dp_display_post_enable(struct msm_dp *msm_dp_display)
 	}
 
 	/* signal the connect event late to synchronize video and display */
-	msm_dp_display_handle_plugged_change(msm_dp_display, true);
+	msm_dp_display_handle_plugged_change(msm_dp_display, connector, true);
 
 	if (msm_dp_display->psr_supported)
 		msm_dp_ctrl_config_psr(dp->ctrl);
@@ -671,7 +674,8 @@ static int msm_dp_display_post_enable(struct msm_dp *msm_dp_display)
 	return 0;
 }
 
-static int msm_dp_display_disable(struct msm_dp_display_private *dp)
+static int msm_dp_display_disable(struct msm_dp_display_private *dp,
+				  struct drm_connector *connector)
 {
 	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
 
@@ -681,7 +685,7 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp)
 	/* wait only if audio was enabled */
 	if (msm_dp_display->audio_enabled) {
 		/* signal the disconnect event */
-		msm_dp_display_handle_plugged_change(msm_dp_display, false);
+		msm_dp_display_handle_plugged_change(msm_dp_display, connector, false);
 		if (!wait_for_completion_timeout(&dp->audio_comp,
 				HZ * 5))
 			DRM_ERROR("audio comp timeout\n");
@@ -1314,7 +1318,15 @@ void __exit msm_dp_unregister(void)
 static bool msm_dp_needs_periph_flush(const struct msm_dp *msm_dp_display,
 				      const struct drm_display_mode *mode)
 {
-	return drm_mode_is_420_only(&msm_dp_display->connector->display_info, mode);
+	struct msm_dp_display_private *dp = container_of(msm_dp_display,
+			struct msm_dp_display_private, msm_dp_display);
+
+	/*
+	 * The YUV-420 output format is determined during ->mode_set(), which
+	 * has already run by the time this is queried at flush time; use the
+	 * cached value so no connector lookup is needed here.
+	 */
+	return dp->panel->msm_dp_mode.out_fmt_is_yuv_420;
 }
 
 bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
@@ -1329,6 +1341,25 @@ bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
 	return dp->wide_bus_supported;
 }
 
+static struct drm_connector *
+msm_dp_display_get_connector(struct msm_dp *msm_dp_display)
+{
+	struct drm_encoder *encoder = msm_dp_display->bridge->encoder;
+	struct drm_connector_list_iter conn_iter;
+	struct drm_connector *connector, *found = NULL;
+
+	drm_connector_list_iter_begin(msm_dp_display->drm_dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		if (drm_connector_has_possible_encoder(connector, encoder)) {
+			found = connector;
+			break;
+		}
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	return found;
+}
+
 void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *root, bool is_edp)
 {
 	struct msm_dp_display_private *dp;
@@ -1338,7 +1369,9 @@ void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *r
 	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
 	dev = &dp->msm_dp_display.pdev->dev;
 
-	rc = msm_dp_debug_init(dev, dp->panel, dp->link, dp->msm_dp_display.connector, root, is_edp);
+	rc = msm_dp_debug_init(dev, dp->panel, dp->link,
+			       msm_dp_display_get_connector(msm_dp_display),
+			       root, is_edp);
 	if (rc)
 		DRM_ERROR("failed to initialize debug, rc = %d\n", rc);
 }
@@ -1347,6 +1380,7 @@ static int msm_dp_modeset_init(struct msm_display *display,
 			       struct drm_device *dev, struct drm_encoder *encoder)
 {
 	struct msm_dp *msm_dp_display = container_of(display, struct msm_dp, display);
+	struct drm_connector *connector;
 	int ret;
 
 	msm_dp_display->drm_dev = dev;
@@ -1358,12 +1392,11 @@ static int msm_dp_modeset_init(struct msm_display *display,
 		return ret;
 	}
 
-	msm_dp_display->connector = msm_dp_drm_connector_init(msm_dp_display, encoder);
-	if (IS_ERR(msm_dp_display->connector)) {
-		ret = PTR_ERR(msm_dp_display->connector);
+	connector = msm_dp_drm_connector_init(msm_dp_display, encoder);
+	if (IS_ERR(connector)) {
+		ret = PTR_ERR(connector);
 		DRM_DEV_ERROR(dev->dev,
 			"failed to create dp connector: %d\n", ret);
-		msm_dp_display->connector = NULL;
 		return ret;
 	}
 
@@ -1454,14 +1487,17 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 {
 	struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(drm_bridge);
 	struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
+	struct drm_connector *connector;
 	int rc = 0;
 	struct msm_dp_display_private *msm_dp_display;
 	bool force_link_train = false;
 
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 
+	connector = drm_atomic_get_new_connector_for_encoder(state, drm_bridge->encoder);
+
 	if (dp->is_edp)
-		msm_dp_hpd_plug_handle(msm_dp_display);
+		msm_dp_hpd_plug_handle(msm_dp_display, connector);
 
 	if (pm_runtime_resume_and_get(&dp->pdev->dev)) {
 		DRM_ERROR("failed to pm_runtime_resume\n");
@@ -1485,10 +1521,10 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
 
 	msm_dp_display_enable(msm_dp_display, force_link_train);
 
-	rc = msm_dp_display_post_enable(dp);
+	rc = msm_dp_display_post_enable(dp, connector);
 	if (rc) {
 		DRM_ERROR("DP display post enable failed, rc=%d\n", rc);
-		msm_dp_display_disable(msm_dp_display);
+		msm_dp_display_disable(msm_dp_display, connector);
 	}
 
 	drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
@@ -1512,13 +1548,17 @@ void msm_dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
 	struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(drm_bridge);
 	struct msm_dp *dp = msm_dp_bridge->msm_dp_display;
 	struct msm_dp_display_private *msm_dp_display;
+	struct drm_connector *connector;
 
 	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
 
+	connector = drm_atomic_get_old_connector_for_encoder(state,
+							     drm_bridge->encoder);
+
 	if (dp->is_edp)
-		msm_dp_hpd_unplug_handle(msm_dp_display);
+		msm_dp_hpd_unplug_handle(msm_dp_display, connector);
 
-	msm_dp_display_disable(msm_dp_display);
+	msm_dp_display_disable(msm_dp_display, connector);
 
 	drm_dbg_dp(dp->drm_dev, "type=%d Done\n", dp->connector_type);
 
@@ -1581,15 +1621,15 @@ void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
 
 	if (status == connector_status_connected) {
 		if (hpd_link_status == ISR_HPD_REPLUG_COUNT) {
-			msm_dp_hpd_unplug_handle(dp);
-			msm_dp_hpd_plug_handle(dp);
+			msm_dp_hpd_unplug_handle(dp, connector);
+			msm_dp_hpd_plug_handle(dp, connector);
 		} else if (hpd_link_status == ISR_IRQ_HPD_PULSE_COUNT) {
-			msm_dp_irq_hpd_handle(dp);
+			msm_dp_irq_hpd_handle(dp, connector);
 		} else {
-			msm_dp_hpd_plug_handle(dp);
+			msm_dp_hpd_plug_handle(dp, connector);
 		}
 	} else {
-		msm_dp_hpd_unplug_handle(dp);
+		msm_dp_hpd_unplug_handle(dp, connector);
 	}
 
 	pm_runtime_put_sync(&msm_dp_display->pdev->dev);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index f1523dbe33d5..a93904ea33c7 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -16,7 +16,6 @@ struct msm_dp {
 
 	struct drm_device *drm_dev;
 	struct platform_device *pdev;
-	struct drm_connector *connector;
 	struct drm_bridge *next_bridge;
 	struct drm_bridge *bridge;
 	bool audio_enabled;

-- 
2.47.3

Re: [PATCH 22/24] drm/msm/dp: stop storing the connector in struct msm_dp

[sashiko-bot]

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

New issues:
- [Medium] msm_dp_display_get_connector() returns a connector pointer without holding a reference, violating the DRM iterator API.

Pre-existing issues:
- [High] Missing assignment of `debug->connector` leads to a guaranteed NULL pointer dereference when reading debugfs files.
--

--- Patch [22]: [PATCH 22/24] drm/msm/dp: stop storing the connector in struct msm_dp ---
commit 6517fb125f3c91df434e357450ad362fc4f2fec3
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dp: stop storing the connector in struct msm_dp

This commit removes the cached drm_connector from struct msm_dp. Instead of
storing the connector, consumers are updated to obtain it from their calling
context, the atomic state, or by looking it up from the encoder.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -1329,6 +1341,25 @@ bool msm_dp_wide_bus_available(const struct msm_dp *msm_dp_display)
>  	return dp->wide_bus_supported;
>  }
>  
> +static struct drm_connector *
> +msm_dp_display_get_connector(struct msm_dp *msm_dp_display)
> +{
> +	struct drm_encoder *encoder = msm_dp_display->bridge->encoder;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *connector, *found = NULL;
> +
> +	drm_connector_list_iter_begin(msm_dp_display->drm_dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		if (drm_connector_has_possible_encoder(connector, encoder)) {
> +			found = connector;
> +			break;
> +		}
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	return found;
> +}

[Severity: Medium]
When the loop breaks, drm_connector_list_iter_end() automatically drops the
reference on the current iterator connector. Since drm_connector_get() is not
called on found before returning, does this return a raw pointer without an
elevated reference count?

> +
>  void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *root, bool is_edp)
>  {
>  	struct msm_dp_display_private *dp;
> @@ -1338,7 +1369,9 @@ void msm_dp_display_debugfs_init(struct msm_dp *msm_dp_display, struct dentry *r
>  	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  	dev = &dp->msm_dp_display.pdev->dev;
>  
> -	rc = msm_dp_debug_init(dev, dp->panel, dp->link, dp->msm_dp_display.connector, root, is_edp);
> +	rc = msm_dp_debug_init(dev, dp->panel, dp->link,
> +			       msm_dp_display_get_connector(msm_dp_display),
> +			       root, is_edp);

[Severity: High]
This is a pre-existing issue, but I noticed that msm_dp_debug_init() in
dp_debug.c accepts the connector argument but never assigns it to the
allocated debug structure (missing debug->connector = connector).

Later, when a user reads from debugfs nodes like dp_test_active, the show
handlers dereference debug->connector. Since it remains NULL from the
devm_kzalloc() allocation, will reading these debugfs files cause a NULL
pointer dereference?

>  	if (rc)
>  		DRM_ERROR("failed to initialize debug, rc = %d\n", rc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=22

[PATCH 23/24] drm/msm: create the bridge connectors from common code

[Dmitry Baryshkov]

Each display sub-block (DSI, DP, HDMI) and the MDP4 LVDS path open-coded
the same drm_bridge_connector_init() call in its own modeset path,
scattering connector creation across the backends and duplicating the
boilerplate.

Now that every sub-block attaches its bridge chain to the encoder created
by the backend, and no sub-block keeps its own reference to the resulting
connector, the connectors can be built entirely from common code. Have
each ->modeset_init() only set up the bridge chain, and add a new
msm_kms_init_connectors() that walks every encoder once the bridges are
in place and creates a bridge connector for each. Encoders without a
bridge chain - such as the virtual writeback encoder, which brings its
own drm_writeback_connector - are skipped.

The only sub-block specific step left is attaching the DP subconnector
property, which is keyed off the connector type and so is applied
generically in the loop; no per-sub-block callback is needed.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  8 ------
 drivers/gpu/drm/msm/dp/dp_display.c      |  9 ------
 drivers/gpu/drm/msm/dp/dp_drm.c          | 17 -----------
 drivers/gpu/drm/msm/dp/dp_drm.h          |  2 --
 drivers/gpu/drm/msm/dsi/dsi.c            |  6 ++--
 drivers/gpu/drm/msm/dsi/dsi.h            |  4 +--
 drivers/gpu/drm/msm/dsi/dsi_manager.c    | 13 ++-------
 drivers/gpu/drm/msm/hdmi/hdmi.c          | 22 ++-------------
 drivers/gpu/drm/msm/msm_kms.c            | 48 ++++++++++++++++++++++++++++----
 9 files changed, 52 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index cd566a287f6f..93f1f0e81ac4 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -7,7 +7,6 @@
 #include <linux/delay.h>
 
 #include <drm/drm_bridge.h>
-#include <drm/drm_bridge_connector.h>
 #include <drm/drm_vblank.h>
 
 #include "msm_drv.h"
@@ -191,7 +190,6 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms,
 	struct drm_device *dev = mdp4_kms->dev;
 	struct msm_drm_private *priv = dev->dev_private;
 	struct drm_encoder *encoder;
-	struct drm_connector *connector;
 	struct drm_bridge *next_bridge;
 	int dsi_id;
 	int ret;
@@ -226,12 +224,6 @@ static int mdp4_modeset_init_intf(struct mdp4_kms *mdp4_kms,
 			return ret;
 		}
 
-		connector = drm_bridge_connector_init(dev, encoder);
-		if (IS_ERR(connector)) {
-			DRM_DEV_ERROR(dev->dev, "failed to initialize LVDS connector\n");
-			return PTR_ERR(connector);
-		}
-
 		break;
 	case DRM_MODE_ENCODER_TMDS:
 		encoder = mdp4_dtv_encoder_init(dev);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 44503789d454..c03878183b9a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1380,7 +1380,6 @@ static int msm_dp_modeset_init(struct msm_display *display,
 			       struct drm_device *dev, struct drm_encoder *encoder)
 {
 	struct msm_dp *msm_dp_display = container_of(display, struct msm_dp, display);
-	struct drm_connector *connector;
 	int ret;
 
 	msm_dp_display->drm_dev = dev;
@@ -1392,14 +1391,6 @@ static int msm_dp_modeset_init(struct msm_display *display,
 		return ret;
 	}
 
-	connector = msm_dp_drm_connector_init(msm_dp_display, encoder);
-	if (IS_ERR(connector)) {
-		ret = PTR_ERR(connector);
-		DRM_DEV_ERROR(dev->dev,
-			"failed to create dp connector: %d\n", ret);
-		return ret;
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 00653b0364dc..01f8320f9c32 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -7,7 +7,6 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_bridge.h>
-#include <drm/drm_bridge_connector.h>
 #include <drm/drm_crtc.h>
 
 #include "msm_drv.h"
@@ -312,19 +311,3 @@ int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
 
 	return 0;
 }
-
-/* connector initialization */
-struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
-					    struct drm_encoder *encoder)
-{
-	struct drm_connector *connector = NULL;
-
-	connector = drm_bridge_connector_init(msm_dp_display->drm_dev, encoder);
-	if (IS_ERR(connector))
-		return connector;
-
-	if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
-		drm_connector_attach_dp_subconnector_property(connector);
-
-	return connector;
-}
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index 4c6b14dd52ac..4d642de4a50b 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -19,8 +19,6 @@ struct msm_dp_bridge {
 
 #define to_dp_bridge(x)     container_of((x), struct msm_dp_bridge, bridge)
 
-struct drm_connector *msm_dp_drm_connector_init(struct msm_dp *msm_dp_display,
-					    struct drm_encoder *encoder);
 int msm_dp_bridge_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
 		   struct drm_encoder *encoder);
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 3bcea5f5b5a3..19a3ff0d27ba 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -283,15 +283,15 @@ static int msm_dsi_modeset_init(struct msm_display *display,
 	    !msm_dsi_is_master_dsi(msm_dsi)) {
 		/*
 		 * Do not return an eror here,
-		 * Just skip creating encoder/connector for the slave-DSI.
+		 * Just skip creating the bridge for the slave-DSI.
 		 */
 		return 0;
 	}
 
-	ret = msm_dsi_manager_connector_init(msm_dsi, encoder);
+	ret = msm_dsi_manager_bridge_init(msm_dsi, encoder);
 	if (ret) {
 		DRM_DEV_ERROR(dev->dev,
-			"failed to create dsi connector: %d\n", ret);
+			"failed to create dsi bridge: %d\n", ret);
 		return ret;
 	}
 
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index d594a7399f54..87a7be955c5f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -52,8 +52,8 @@ struct msm_dsi {
 /* dsi manager */
 bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
 bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
-int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
-				   struct drm_encoder *encoder);
+int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi,
+				struct drm_encoder *encoder);
 int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg);
 bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len);
 int msm_dsi_manager_register(struct msm_dsi *msm_dsi);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 2a5326f1fa8b..9edace2b396d 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -3,7 +3,6 @@
  * Copyright (c) 2015, The Linux Foundation. All rights reserved.
  */
 
-#include "drm/drm_bridge_connector.h"
 
 #include "msm_kms.h"
 #include "dsi.h"
@@ -458,13 +457,11 @@ static const struct drm_bridge_funcs dsi_mgr_bridge_funcs = {
 };
 
 /* initialize bridge */
-int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
-				   struct drm_encoder *encoder)
+int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi,
+				struct drm_encoder *encoder)
 {
-	struct drm_device *dev = msm_dsi->dev;
 	struct drm_bridge *bridge;
 	struct dsi_bridge *dsi_bridge;
-	struct drm_connector *connector;
 	int ret;
 
 	dsi_bridge = devm_drm_bridge_alloc(msm_dsi->dev->dev, struct dsi_bridge, base,
@@ -484,12 +481,6 @@ int msm_dsi_manager_connector_init(struct msm_dsi *msm_dsi,
 	if (ret)
 		return ret;
 
-	connector = drm_bridge_connector_init(dev, encoder);
-	if (IS_ERR(connector)) {
-		DRM_ERROR("Unable to create bridge connector\n");
-		return PTR_ERR(connector);
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 62961dddd8f5..33474e90af53 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -11,7 +11,6 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 
-#include <drm/drm_bridge_connector.h>
 #include <drm/drm_of.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 
@@ -164,7 +163,6 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
 		struct drm_device *dev, struct drm_encoder *encoder)
 {
 	struct hdmi *hdmi = container_of(display, struct hdmi, display);
-	struct drm_connector *connector = NULL;
 	int ret;
 
 	hdmi->dev = dev;
@@ -173,7 +171,7 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
 	ret = msm_hdmi_bridge_init(hdmi);
 	if (ret) {
 		DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: %d\n", ret);
-		goto fail;
+		return ret;
 	}
 
 	if (hdmi->next_bridge) {
@@ -181,34 +179,20 @@ static int msm_hdmi_modeset_init(struct msm_display *display,
 					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 		if (ret) {
 			DRM_DEV_ERROR(dev->dev, "failed to attach next HDMI bridge: %d\n", ret);
-			goto fail;
+			return ret;
 		}
 	}
 
-	connector = drm_bridge_connector_init(hdmi->dev, encoder);
-	if (IS_ERR(connector)) {
-		ret = PTR_ERR(connector);
-		DRM_DEV_ERROR(dev->dev, "failed to create HDMI connector: %d\n", ret);
-		connector = NULL;
-		goto fail;
-	}
-
 	ret = devm_request_irq(dev->dev, hdmi->irq,
 			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
 			"hdmi_isr", hdmi);
 	if (ret < 0) {
 		DRM_DEV_ERROR(dev->dev, "failed to request IRQ%u: %d\n",
 				hdmi->irq, ret);
-		goto fail;
+		return ret;
 	}
 
 	return 0;
-
-fail:
-	if (connector)
-		connector->funcs->destroy(connector);
-
-	return ret;
 }
 
 static void msm_hdmi_snapshot(struct msm_display *display,
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index 1a081e15a0a8..71af3780b7b1 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -10,7 +10,10 @@
 #include <linux/sched/mm.h>
 #include <uapi/linux/sched/types.h>
 
+#include <drm/drm_bridge_connector.h>
+#include <drm/drm_connector.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_mode_config.h>
 #include <drm/drm_vblank.h>
 #include <drm/clients/drm_client_setup.h>
@@ -271,15 +274,15 @@ static int msm_display_modeset_init(struct msm_display *display,
 }
 
 /*
- * Set up the bridges and connectors for the display sub-blocks, dispatching
- * through the common display interface using the encoders the backend created
- * in ->kms_init().
+ * Set up the bridge chains for the display sub-blocks, dispatching through the
+ * common display interface using the encoders the backend created in
+ * ->kms_init().
  *
- * The slave link of a bonded DSI pair shares its master's encoder and creates
- * no connector of its own; its ->modeset_init() handles that internally, so it
+ * The slave link of a bonded DSI pair shares its master's encoder and sets up
+ * no bridge of its own; its ->modeset_init() handles that internally, so it
  * needs no special-casing here.
  */
-static int msm_kms_init_connectors(struct drm_device *ddev)
+static int msm_kms_modeset_init(struct drm_device *ddev)
 {
 	struct msm_drm_private *priv = ddev->dev_private;
 	struct msm_kms *kms = priv->kms;
@@ -310,6 +313,35 @@ static int msm_kms_init_connectors(struct drm_device *ddev)
 	return ret;
 }
 
+/* Build a bridge connector for each encoder. */
+static int msm_kms_init_connectors(struct drm_device *ddev)
+{
+	struct drm_encoder *encoder;
+
+	drm_for_each_encoder(encoder, ddev) {
+		struct drm_connector *connector;
+		int ret;
+
+		/* the virtual writeback encoder brings its own connector */
+		if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
+			continue;
+
+		connector = drm_bridge_connector_init(ddev, encoder);
+		if (IS_ERR(connector)) {
+			DRM_DEV_ERROR(ddev->dev,
+				      "failed to init connector for %s: %ld\n",
+				      encoder->name, PTR_ERR(connector));
+			return PTR_ERR(connector);
+		}
+
+		/* The DP subconnector property applies to DisplayPort only. */
+		if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+			drm_connector_attach_dp_subconnector_property(connector);
+	}
+
+	return 0;
+}
+
 int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(dev);
@@ -353,6 +385,10 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
 	 * ->hw_init(); now set up the bridges and connectors for them from
 	 * common code.
 	 */
+	ret = msm_kms_modeset_init(ddev);
+	if (ret)
+		goto err_msm_uninit;
+
 	ret = msm_kms_init_connectors(ddev);
 	if (ret)
 		goto err_msm_uninit;

-- 
2.47.3

Re: [PATCH 23/24] drm/msm: create the bridge connectors from common code

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Centralized `msm_kms_init_connectors` loop unconditionally calls `drm_bridge_connector_init` for all non-virtual encoders, failing the driver probe with `-EINVAL` if any encoder lacks an attached bridge.
--

--- Patch [23]: [PATCH 23/24] drm/msm: create the bridge connectors from common code ---
commit 04453784aafc591b434b0736383d9d31623d416b
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
drm/msm: create the bridge connectors from common code

This patch centralizes bridge connector creation in the msm backend by
having each sub-block only set up its bridge chain. A new function,
msm_kms_init_connectors(), then walks through all encoders to create the
connectors using common code.

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index 1a081e15a0a89..71af3780b7b13 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
[ ... ]
> @@ -310,6 +313,35 @@ static int msm_kms_init_connectors(struct drm_device *ddev)
>  	return ret;
>  }
>  
> +/* Build a bridge connector for each encoder. */
> +static int msm_kms_init_connectors(struct drm_device *ddev)
> +{
> +	struct drm_encoder *encoder;
> +
> +	drm_for_each_encoder(encoder, ddev) {
> +		struct drm_connector *connector;
> +		int ret;
> +
> +		/* the virtual writeback encoder brings its own connector */
> +		if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
> +			continue;
> +
> +		connector = drm_bridge_connector_init(ddev, encoder);
> +		if (IS_ERR(connector)) {
> +			DRM_DEV_ERROR(ddev->dev,
> +				      "failed to init connector for %s: %ld\n",
> +				      encoder->name, PTR_ERR(connector));
> +			return PTR_ERR(connector);
> +		}

[Severity: High]
Will this fail the entire driver probe on hardware where some encoders are
left without an attached bridge?

For example, on MDP4 hardware, a TMDS encoder is registered unconditionally
in mdp4_modeset_init_intf(), but it will have an empty bridge chain if HDMI
is not present. Similarly, for bonded DSI, msm_dsi_modeset_init() skips
bridge creation for the slave DSI, leaving its encoder without a bridge.

When drm_bridge_connector_init() processes an encoder with an empty bridge
chain, it returns ERR_PTR(-EINVAL) because it cannot determine the connector
type. Since this loop treats any error as fatal, won't this break
display initialization on these configurations?

> +
> +		/* The DP subconnector property applies to DisplayPort only. */
> +		if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> +			drm_connector_attach_dp_subconnector_property(connector);
> +	}
> +
> +	return 0;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=23

[PATCH 24/24] drm/bridge-connector: attach the DP subconnector property

[Dmitry Baryshkov]

DisplayPort connectors can expose the subconnector property, describing
the attached dongle. While i915 and amdgpu drivers already provide this
property, bridge-based DisplayPort connectors don't expose it.

Attach the DP subconnector property from drm_bridge_connector_init()
whenever the resolved connector type is DisplayPort, and drop the
open-coded attachment from the msm code, pushing other DP drivers
towards adding support for the property.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/display/drm_bridge_connector.c | 4 ++++
 drivers/gpu/drm/msm/msm_kms.c                  | 5 -----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
index 8b54069fa53a..f83d5f52ab14 100644
--- a/drivers/gpu/drm/display/drm_bridge_connector.c
+++ b/drivers/gpu/drm/display/drm_bridge_connector.c
@@ -1089,6 +1089,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
 	    IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
 		drm_connector_attach_content_protection_property(connector, true);
 
+	/* The DP subconnector property applies to DisplayPort only. */
+	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+		drm_connector_attach_dp_subconnector_property(connector);
+
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index 71af3780b7b1..cf602be09ff8 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -320,7 +320,6 @@ static int msm_kms_init_connectors(struct drm_device *ddev)
 
 	drm_for_each_encoder(encoder, ddev) {
 		struct drm_connector *connector;
-		int ret;
 
 		/* the virtual writeback encoder brings its own connector */
 		if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
@@ -333,10 +332,6 @@ static int msm_kms_init_connectors(struct drm_device *ddev)
 				      encoder->name, PTR_ERR(connector));
 			return PTR_ERR(connector);
 		}
-
-		/* The DP subconnector property applies to DisplayPort only. */
-		if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
-			drm_connector_attach_dp_subconnector_property(connector);
 	}
 
 	return 0;

-- 
2.47.3

Re: [PATCH 24/24] drm/bridge-connector: attach the DP subconnector property

[sashiko-bot]

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

Pre-existing issues:
- [High] Unchecked return value from drm_property_create_enum() causes a NULL pointer dereference inside drm_object_attach_property().
--

--- Patch [24]: [PATCH 24/24] drm/bridge-connector: attach the DP subconnector property ---
commit 3033458679cc26b1f68f203c9dbef371dae0aec7
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Subject: drm/bridge-connector: attach the DP subconnector property

This patch attaches the DP subconnector property from
drm_bridge_connector_init() for all DisplayPort connectors. It also removes
the open-coded attachment from the MSM driver to push other DP drivers towards
adding support for the property.

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 8b54069fa53a4..f83d5f52ab146 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -1089,6 +1089,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  	    IS_ENABLED(CONFIG_DRM_DISPLAY_HDCP_HELPER))
>  		drm_connector_attach_content_protection_property(connector, true);
>  
> +	/* The DP subconnector property applies to DisplayPort only. */
> +	if (connector_type == DRM_MODE_CONNECTOR_DisplayPort)
> +		drm_connector_attach_dp_subconnector_property(connector);

[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
if drm_property_create_enum() fails under memory pressure?

By adding this call here, the bug is now exposed to all DisplayPort bridge
connectors. In drm_connector_attach_dp_subconnector_property():

	if (!mode_config->dp_subconnector_property)
		mode_config->dp_subconnector_property =
			drm_property_create_enum(connector->dev, ...);

	drm_object_attach_property(&connector->base,
				   mode_config->dp_subconnector_property,
				   DRM_MODE_SUBCONNECTOR_Unknown);

If drm_property_create_enum() returns NULL, it passes NULL to
drm_object_attach_property().

Inside drm_object_attach_property(), the NULL property pointer is immediately
dereferenced:

void drm_object_attach_property(struct drm_mode_object *obj,
				struct drm_property *property,
				uint64_t init_val)
{
	int count = obj->properties->count;
	struct drm_device *dev = property->dev;

Could an error check be added to
drm_connector_attach_dp_subconnector_property() to handle the allocation
failure instead of dereferencing a NULL pointer?

> +
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret)
>  		return ERR_PTR(ret);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-drm-msm-display-interface-v1-0-368c10fe62fd@oss.qualcomm.com?part=24