Re: [PATCH v8 10/15] drm/msm/dp: allow dp_ctrl stream APIs to use any panel passed to it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Secondary stream panels fail to have their `hw_revision` initialized, silently breaking controller-specific hardware workarounds in MST setups.
- [High] Stream-specific panels are incorrectly passed to mainlink APIs, corrupting shared link state and misconfiguring the link based on leaf monitor capabilities instead of the MST hub.
--

--- Patch [10]: [PATCH v8 10/15] drm/msm/dp: allow dp_ctrl stream APIs to use any panel passed to it ---
commit 07d3f4b80ae5875a958a8aceb22c9ffab07db108
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: allow dp_ctrl stream APIs to use any panel passed to it

With MST, multiple sinks share a single DP controller, so a cached
panel in msm_dp_ctrl_private can no longer represent the per-stream
sink. Drop the cache and pass panel explicitly to all stream-related
dp_ctrl APIs.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index ae79cbec26d19..207643fa73063 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -219,7 +219,7 @@ void msm_dp_ctrl_reset(struct msm_dp_ctrl *msm_dp_ctrl)
>  
>  	if (!ctrl->hw_revision) {
>  		ctrl->hw_revision = msm_dp_read_ahb(ctrl, REG_DP_HW_VERSION);
> -		ctrl->panel->hw_revision = ctrl->hw_revision;
> +		panel->hw_revision = ctrl->hw_revision;
>  	}
>  }

[Severity: High]
Does this leave the hw_revision uninitialized for secondary stream panels in an
MST setup?

Since ctrl->hw_revision is populated during the host initialization for the
primary panel, any subsequent stream panels passed to this function will bypass
the assignment. Could this silently break hardware workarounds that depend on
panel->hw_revision being set correctly?

[ ... ]

> @@ -414,10 +414,11 @@ static void msm_dp_ctrl_config_ctrl_streams(struct msm_dp_ctrl_private *ctrl,
>  	msm_dp_write_link(ctrl, REG_DP_CONFIGURATION_CTRL, config);
>  }
>  
> -static void msm_dp_ctrl_config_ctrl_link(struct msm_dp_ctrl_private *ctrl)
> +static void msm_dp_ctrl_config_ctrl_link(struct msm_dp_ctrl_private *ctrl,
> +					 struct msm_dp_panel *panel)
>  {
>  	u32 config = 0;
> -	const u8 *dpcd = ctrl->panel->dpcd;
> +	const u8 *dpcd = panel->dpcd;
>  
>  	/* Default-> LSCLK DIV: 1/4 LCLK  */
>  	config |= (2 << DP_CONFIGURATION_CTRL_LSCLK_DIV_SHIFT);

[Severity: High]
Can the direct write to REG_DP_CONFIGURATION_CTRL at the end of this function
erase stream-specific configurations established by previously enabled streams?

Since msm_dp_ctrl_config_ctrl_link() is called on every stream enable from
msm_dp_ctrl_on_stream(), it seems this might overwrite the shared register and
corrupt the state of active streams.

[ ... ]

> @@ -1810,11 +1821,12 @@ static void msm_dp_ctrl_link_clk_disable(struct msm_dp_ctrl *msm_dp_ctrl)
>  		   str_on_off(ctrl->core_clks_on));
>  }
>  
> -static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private *ctrl)
> +static int msm_dp_ctrl_enable_mainlink_clocks(struct msm_dp_ctrl_private *ctrl,
> +					      struct msm_dp_panel *panel)
>  {
>  	int ret = 0;
>  	struct phy *phy = ctrl->phy;
> -	const u8 *dpcd = ctrl->panel->dpcd;
> +	const u8 *dpcd = panel->dpcd;
>  
>  	ctrl->phy_opts.dp.lanes = ctrl->link->link_params.num_lanes;
>  	ctrl->phy_opts.dp.link_rate = ctrl->link->link_params.rate / 100;

[Severity: High]
Is it correct to use the stream-specific (leaf) DPCD to configure shared
mainlink properties here and in msm_dp_ctrl_link_train()?

This appears to misconfigure the link based on leaf monitor capabilities instead
of the MST hub. If a leaf monitor doesn't support features like enhanced
framing or downspread, but the hub does, could this break the link to the hub?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-dp_mstclean-v8-0-bea261bf4e7d@oss.qualcomm.com?part=10