Re: [PATCH v7 15/35] drm/msm/dp: factor out _helper variants of bridge ops accepting a panel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] msm_dp_display_post_enable() hardcodes root dp->panel, breaking stream-specific PSR config
--

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

drm/msm/dp: factor out _helper variants of bridge ops accepting a panel

The atomic bridge callbacks (set_mode / enable / disable /
post_disable) on dp_display currently hard-code dp->panel. This
commit introduces helper variants that take a struct msm_dp_panel
pointer to allow the upcoming DP MST encoder to drive link-level
prepare and unprepare uniformly.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> +void msm_dp_display_enable_helper(struct msm_dp *msm_dp_display,
> +				  struct msm_dp_panel *msm_dp_panel)
> +{
> +	int rc = 0;
> +	struct msm_dp_display_private *dp;
> +
> +	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
> +
> +	rc = msm_dp_display_enable(dp, msm_dp_panel);
> +	if (rc)
> +		DRM_ERROR("DP display enable failed, rc=%d\n", rc);
> +
> +	rc = msm_dp_display_post_enable(msm_dp_display);

[Severity: Medium]
Does this call correctly configure PSR for MST streams?

This helper correctly accepts a stream-specific msm_dp_panel, but
msm_dp_display_post_enable() was not refactored to take a panel argument.

Looking at msm_dp_display_post_enable():

static int msm_dp_display_post_enable(struct msm_dp *msm_dp_display)
{
        ...
        if (msm_dp_display->psr_supported)
                msm_dp_ctrl_config_psr(dp->ctrl, dp->panel);
        ...
}

It still hardcodes the root dp->panel instead of the active stream's panel.
Could this misconfiguration lead to missing or incorrect PSR setup on MST
streams?

> +	if (rc) {
> +		DRM_ERROR("DP display post enable failed, rc=%d\n", rc);
> +		msm_dp_display_disable(dp, msm_dp_panel);
> +	}
> +
> +	drm_dbg_dp(msm_dp_display->drm_dev, "type=%d Done\n", msm_dp_display->connector_type);
> +}
[ ... ]
> +void msm_dp_display_disable_helper(struct msm_dp *msm_dp_display,
> +				   struct msm_dp_panel *msm_dp_panel)
> +{
> +	struct msm_dp_display_private *dp;
> +
> +	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  
>  	/*
>  	 * If .atomic_enable() bailed out - link training failure is the common
> @@ -1521,17 +1542,28 @@ void msm_dp_display_atomic_disable(struct msm_dp *dp)
>  	 * TrustZone-level SOCCP/ADSP force-stop and a silent SoC reset.
>  	 * There is nothing to push idle, so leave it alone.
>  	 */
> -	if (!dp->power_on)
> +	if (!msm_dp_display->active_stream_cnt)
>  		return;

[Severity: High]
This is a pre-existing issue, but could this safety guard be bypassed if a
stream fails to enable?

Looking at msm_dp_display_enable(), active_stream_cnt is incremented
unconditionally even if msm_dp_ctrl_on_stream() fails:

static int msm_dp_display_enable(struct msm_dp_display_private *dp,
                                 struct msm_dp_panel *msm_dp_panel)
{
        rc = msm_dp_ctrl_on_stream(dp->ctrl, msm_dp_panel, ...);
        msm_dp_display->active_stream_cnt++;
        return rc;
}

If msm_dp_ctrl_on_stream() returns an error during standard atomic commit paths
(e.g., due to a transient hardware failure like pixel clock enablement),
active_stream_cnt is artificially greater than zero. Later, when the stream is
disabled, this check evaluates to true.

Pushing idle patterns to a controller that failed to enable will time out,
which as documented here can cause a TrustZone-level SOCCP/ADSP force-stop and
a silent SoC reset.

Can we avoid incrementing active_stream_cnt when msm_dp_ctrl_on_stream()
fails?

> -	msm_dp_ctrl_push_vcpf(msm_dp_display->ctrl, msm_dp_display->panel);
> -	msm_dp_ctrl_mst_timeslot_setup(msm_dp_display->ctrl);
> -	msm_dp_ctrl_mst_send_act(msm_dp_display->ctrl, msm_dp_display->panel);
> +	msm_dp_ctrl_push_vcpf(dp->ctrl, msm_dp_panel);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=15