Re: [PATCH v8 11/29] drm/msm/dp: add msm_dp_display_get_panel() to initialize DP panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/msm/dp: uninitialized hw_revision in
  msm_dp_display_get_panel()
- [Medium] drm/msm/dp: devm_kzalloc leak and panel state desync in
  msm_dp_display_get_panel()
--

commit 925ddc6c6313feca1fc26113de74e6f96e91e020
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: add msm_dp_display_get_panel() to initialize DP panel

This commit adds msm_dp_display_get_panel() and msm_dp_ctrl_get_stream_cnt()
so the DP MST module can initialize and obtain a struct msm_dp_panel for a
given stream ID.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 6a522cc71a2ac..fa027659a079f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -536,6 +536,33 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
>  	return rc;
>  }
>  
> +struct msm_dp_panel *msm_dp_display_get_panel(struct msm_dp *msm_dp_display,
> +					      enum msm_dp_stream_id stream_id)
> +{
> +	struct msm_dp_display_private *dp;
> +	struct msm_dp_panel *dp_panel;
> +
> +	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
> +
> +	if (stream_id >= msm_dp_ctrl_get_stream_cnt(dp->ctrl)) {
> +		DRM_ERROR("invalid stream_id %d\n", stream_id);
> +		return NULL;
> +	}
> +
> +	dp_panel = msm_dp_panel_get(&dp->msm_dp_display.pdev->dev, dp->aux, dp->link,
> +				    dp->link_base, dp->mst2link_base, dp->mst3link_base,
> +				    dp->pixel_base[stream_id]);

[Severity: Medium]
Does calling msm_dp_panel_get() here leak struct msm_dp_panel_private across
KMS component unbind and rebind cycles?

msm_dp_panel_get() allocates the panel with devm_kzalloc() on the DP platform
device (&dp->msm_dp_display.pdev->dev), while MST initialization calls
msm_dp_display_get_panel() during KMS component bind and has no corresponding
release on KMS unbind. Rebinding the KMS component while the DP platform
device stays bound allocates another set of panels on the platform device.

Allocating separate panel instances (including a second panel for
DP_STREAM_0 alongside dp->panel) also leaves dp->panel state such as
msm_dp_mode.out_fmt_is_yuv_420 out of sync when queried through
msm_dp_wide_bus_available().

> +
> +	if (IS_ERR(dp_panel)) {
> +		DRM_ERROR("failed to initialize panel\n");
> +		return NULL;
> +	}
> +
> +	dp_panel->stream_id = stream_id;
> +
> +	return dp_panel;
> +}

[Severity: Medium]
Does this leave dp_panel->hw_revision uninitialized at 0 for panels returned
by msm_dp_display_get_panel()?

msm_dp_ctrl_reset() only populates panel->hw_revision on the SST dp->panel
instance, after msm_dp_display_get_panel() has already allocated separate
per-stream panels:

drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_reset() {
    ...
	if (!ctrl->hw_revision)
		ctrl->hw_revision = msm_dp_read_ahb(ctrl, REG_DP_HW_VERSION);
	panel->hw_revision = ctrl->hw_revision;
    ...
}

When VSC SDP is enabled or disabled on an MST stream (for example, for YUV420
modes), msm_dp_panel_update_sdp() checks whether hw_revision is between
DP_HW_VERSION_1_0 and DP_HW_VERSION_1_2 before writing MMSS_DP_SDP_CFG3, so
that SDP flush write is skipped on DP 1.0 and 1.1 controllers. Even after
commit "drm/msm/dp: move link capabilities to dp_link" moves link
capabilities to struct msm_dp_link, hw_revision remains in struct
msm_dp_panel.

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