Re: [PATCH v8 16/29] drm/msm/dpu: add per-stream MST encoders

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/msm/dpu: KMS init failure on SC8180X in
  _dpu_kms_initialize_displayport()
- [Medium] drm/msm/dp: devres leak of msm_dp_panel in
  msm_dp_mst_attach_encoder()
--

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

drm/msm/dpu: add per-stream MST encoders

This commit initializes one DRM encoder per DisplayPort MST stream with
MST-specific helper callbacks and attaches each encoder and stream panel to
the DP MST state.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 3c3203ca32c76..92691d39dcb24 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -688,6 +688,21 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>  				return rc;
>  			}
>  		}
> +
> +		for (int stream_id = 0; stream_cnt > 1 && stream_id < stream_cnt; stream_id++) {
> +			info.stream_id = stream_id;
> +			encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DPMST, &info);
> +			if (IS_ERR(encoder)) {
> +				DPU_ERROR("encoder init failed for dp mst display\n");
> +				return PTR_ERR(encoder);
> +			}
> +
> +			rc = msm_dp_mst_attach_encoder(priv->kms->dp[i], stream_id, encoder);
> +			if (rc) {
> +				DPU_ERROR("dp_mst attach_encoder failed, rc = %d\n", rc);
> +				return rc;
> +			}
> +		}
>  	}
>  
>  	return 0;

[Severity: High]
Does this break DPU KMS initialization on SC8180X?

On SC8180X, arch/arm64/boot/dts/qcom/sc8180x.dtsi defines both stream_pixel
and stream_1_pixel on mdss_dp0 and mdss_dp1, so msm_dp_get_mst_max_stream()
returns stream_cnt = 2.  However, sc8180x_intf[] only defines one INTF_DP
entry for MSM_DP_CONTROLLER_0 and one for MSM_DP_CONTROLLER_1, while the
shared MST interface has a dummy controller_id:

drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h:
	/* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
	{
		.name = "intf_3", .id = INTF_3,
		.base = 0x6b800, .len = 0x280,
		.type = INTF_DP,
		.controller_id = 999,

When stream_id == 1, dpu_encoder_init() -> dpu_encoder_setup_display() ->
dpu_encoder_get_intf() cannot find a second matching INTF_DP entry and
returns NULL, causing dpu_encoder_init() to return ERR_PTR(-ENOMEM) and
aborting _dpu_kms_initialize_displayport() and all display initialization.

> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> index a2900ff3b0ab3..ef5fb70863b81 100644
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> @@ -147,6 +147,26 @@ static int msm_dp_mst_encoder_stream_id(struct msm_dp_mst *mst,
>  	return -1;
>  }
>  
> +int msm_dp_mst_attach_encoder(struct msm_dp *dp_display, unsigned int stream_id,
> +			      struct drm_encoder *encoder)
> +{
> +	struct msm_dp_mst *mst = dp_display->msm_dp_mst;
> +	struct msm_dp_panel *dp_panel;
> +
> +	dp_panel = msm_dp_display_get_panel(dp_display, stream_id);
> +	if (!dp_panel) {
> +		drm_err(dp_display->drm_dev,
> +			"[MST] failed to allocate panel for stream %d\n", stream_id);
> +		return -ENOMEM;
> +	}
> +
> +	mst->mst_encoders[stream_id].enc = encoder;
> +	mst->mst_encoders[stream_id].stream_id = stream_id;
> +	mst->mst_encoders[stream_id].dp_panel = dp_panel;
> +
> +	return 0;
> +}

[Severity: Medium]
Does calling msm_dp_display_get_panel() here leak struct
msm_dp_panel_private across DRM component master unbind/rebind cycles or KMS
init failure?

msm_dp_mst_attach_encoder() runs during DRM master bind in
_dpu_kms_initialize_displayport(), but msm_dp_display_get_panel() ->
msm_dp_panel_get() allocates the panel with devm_kzalloc() on the persistent
subordinate DP platform device (&dp->msm_dp_display.pdev->dev).  When the
DRM master unbinds or msm_drm_init() fails later, dpu_kms_destroy() ->
msm_dp_mst_unregister() -> msm_dp_mst_mgr_destroy() frees mst and drops
mst->mst_encoders[stream_id].dp_panel, leaving the devres allocation behind
on the DP device and allocating a new panel on the next bind.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-msm-dp-mst-v8-0-d040fb33db5f@oss.qualcomm.com?part=16