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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Memory leak of `msm_dp_panel` structures during DRM aggregate device bind/unbind.

Pre-existing issues:
- [High] Acquiring a mutex within encoder atomic enable/disable callbacks, which run in atomic context.
--

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

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

Unlike SST, where a DP controller is represented by a single encoder,
MST requires an encoder instance for each stream.

Create one DRM encoder per MST stream, associate it with a stream ID,
and initialize it with MST-specific encoder helper callbacks.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 004d5fc460a87..533109b4365c3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2744,6 +2744,24 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
>  	.atomic_enable = dpu_encoder_virt_atomic_enable,
>  };
>  
> +static void dpu_encoder_mst_atomic_enable(struct drm_encoder *enc,
> +				      struct drm_atomic_commit *state)
> +{
> +	dpu_encoder_virt_atomic_enable(enc, state);

[Severity: High]
This is a pre-existing issue, but does calling
dpu_encoder_virt_atomic_enable() here violate atomic context rules?

According to subsystem guidelines, atomic_enable callbacks run in an atomic
context where sleeping is forbidden. However,
dpu_encoder_virt_atomic_enable() acquires a sleepable mutex:

static void dpu_encoder_virt_atomic_enable(...)
{
    ...
    mutex_lock(&dpu_enc->enc_lock);
    ...
}

Could this lead to kernel warnings, system instability, or potential deadlocks?

> +}
> +
> +static void dpu_encoder_mst_atomic_disable(struct drm_encoder *enc,
> +				       struct drm_atomic_commit *state)
> +{
> +	dpu_encoder_virt_atomic_disable(enc, state);
> +}
> +

[ ... ]

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 66cabac0a7264..d6182cec06d58 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;

[Severity: High]
If this loop fails on stream_id > 0, do the panels allocated in previous
iterations leak?

Additionally, because the panel allocation in msm_dp_mst_attach_encoder() is
tied to the independent DP device's lifetime, it appears these panels will
also leak on aggregate unbind, as well as on KMS initialization failures
causing probe deferral.

> +			}
> +		}
>  	}
>  
>  	return 0;

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> index 94a717ba7c827..93536b6936cbd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> @@ -145,6 +145,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) {

[Severity: High]
Does this allocation leak during DRM aggregate device unbind?

msm_dp_display_get_panel() uses devm_kzalloc tied to the DP component
device (pdev->dev). Because the DP component device outlives the DRM bind
cycle, its devm allocations are not freed on DRM unbind.

Could repeatedly binding and unbinding the DRM aggregate device (e.g., via
sysfs) permanently leak this msm_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;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-msm-dp-mst-v6-0-c91d35d6fb9e@oss.qualcomm.com?part=22