Re: [PATCH v7 06/15] drm/msm/dp: split link setup from source params

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Link retraining erases active stream configurations due to overwriting shared register
--

commit c62d46d098f37a3918af36fbf87cf140b8d49f60
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: split link setup from source params

msm_dp_ctrl_configure_source_params() should only handle stream-related
configuration. Move the link setup out of it so MST can program link and
stream settings separately.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 71d45b2c4daf8..1c2eccec6ec6d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c

[ ... ]

> @@ -2551,6 +2547,10 @@ int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, bool force_link_train
>  	 */
>  	reinit_completion(&ctrl->video_comp);
>  
> +	msm_dp_ctrl_lane_mapping(ctrl);
> +	msm_dp_setup_peripheral_flush(ctrl);
> +	msm_dp_ctrl_config_ctrl_link(ctrl);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how link setup
and stream setup share registers, can msm_dp_ctrl_config_ctrl_link() wipe
out active stream configurations?

In msm_dp_ctrl_config_ctrl_link(), the REG_DP_CONFIGURATION_CTRL register
state is built from 0 rather than reading the existing state:

u32 config = 0;
const u8 *dpcd = panel->dpcd;

/* Default-> LSCLK DIV: 1/4 LCLK  */
config |= (2 << DP_CONFIGURATION_CTRL_LSCLK_DIV_SHIFT);
...
msm_dp_write_link(ctrl, REG_DP_CONFIGURATION_CTRL, config);

When msm_dp_ctrl_link_maintenance() triggers link retraining due to a sink
request, it eventually calls msm_dp_ctrl_config_ctrl_link(). Because this
function overwrites the shared REG_DP_CONFIGURATION_CTRL register without
a read-modify-write, does it erase all stream configurations (like
DP_CONFIGURATION_CTRL_RGB_YUV or DP_CONFIGURATION_CTRL_SEND_VSC)
previously set by msm_dp_ctrl_config_ctrl_streams()?

If the maintenance routine then resumes sending video, this might leave the
hardware outputting an incorrect stream format. Would it be safer to use
msm_dp_read_link() to preserve the existing register state?

> +
>  	msm_dp_ctrl_configure_source_params(ctrl);
>  
>  	msm_dp_ctrl_config_msa(ctrl,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=6