Re: [PATCH v9 08/14] drm/msm/dp: break up dp_display_enable into two parts

[sashiko-bot@kernel.org]

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

New issues:
- [High] Silent inversion of hardware initialization sequence (pixel clock and link retraining)
- [Medium] API split fails to support inserting MST payload programming as claimed

Pre-existing issues:
- [High] Unpowered hardware register access causes kernel panic on error path
--

--- Patch [8]: [PATCH v9 08/14] drm/msm/dp: break up dp_display_enable into two parts ---
commit 722ccac6521827b0527a34404c0521cf56234857
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: break up dp_display_enable into two parts

Split dp_display_enable into prepare and enable APIs. This aims to
support MST bridges_enable inserting MST payload functions between
enabling stream clocks and programming registers.

> dp_display_enable() currently re-trains the link if needed and then
> enables the pixel clock, programs the controller to start sending the
> pixel stream. Split these two parts into prepare/enable APIs, to support
> MST bridges_enable insert the MST payloads funcs between enable
> stream_clks and program register.

[Severity: Medium]
Does this split actually allow inserting MST payload functions between
enabling stream clocks and programming the registers?

Looking at the implementation of msm_dp_ctrl_on_stream() below, the call to
msm_dp_ctrl_on_pixel_clk() and the subsequent register programming are still
tightly coupled in the same function. It doesn't seem possible for an
external caller to inject payload programming between them.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 068d4f8b673f6..43094dda34349 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c

[ ... ]

> -int msm_dp_ctrl_on_stream(struct msm_dp_ctrl *msm_dp_ctrl, bool force_link_train)
> +int msm_dp_ctrl_prepare_stream_on(struct msm_dp_ctrl *msm_dp_ctrl, bool force_link_train)
>  {
>  	int ret = 0;

[ ... ]

> -	ret = msm_dp_ctrl_on_pixel_clk(ctrl, pixel_rate);
> -	if (ret)
> -		return ret;
> -
>  	if (force_link_train || !msm_dp_ctrl_channel_eq_ok(ctrl))
>  		msm_dp_ctrl_link_retrain(ctrl);

[Severity: High]
Does this change invert the hardware initialization sequence?

Before this patch, msm_dp_ctrl_on_stream() called msm_dp_ctrl_on_pixel_clk()
to enable the pixel clock before performing the link retraining.

With the new split, msm_dp_ctrl_prepare_stream_on() performs link retraining,
but the pixel clock isn't enabled until msm_dp_ctrl_on_stream() is called
later. Could this cause link training failures or black screens by retraining
before the pixel clock is ready?

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 2a84a6ffedbb5..d094d484474cd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

> -static int msm_dp_display_enable(struct msm_dp_display_private *dp, bool force_link_train)
> +static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
> +{
> +	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
> +	int rc = 0;
> +	bool force_link_train = false;
> +
> +	drm_dbg_dp(dp->drm_dev, "sink_count=%d\n", dp->link->sink_count);
> +
> +	if (msm_dp_display->is_edp)
> +		msm_dp_hpd_plug_handle(dp);
> +
> +	rc = pm_runtime_resume_and_get(&msm_dp_display->pdev->dev);
> +	if (rc) {
> +		DRM_ERROR("failed to pm_runtime_resume\n");
> +		return rc;
> +	}

[Severity: High]
This is a pre-existing issue, but if pm_runtime_resume_and_get() fails here,
the device is left unpowered while the DRM state considers it enabled.

Since msm_dp_bridge_atomic_enable() returns void and cannot abort the overall
atomic commit if this fails, msm_dp_bridge_atomic_disable() will
unconditionally be called later during teardown.

That disable path eventually calls msm_dp_ctrl_push_idle() which accesses
unclocked and unpowered DP hardware registers, leading to a synchronous
external abort (kernel panic).

Should there be a way to prevent hardware access during disable if the
device never successfully resumed?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-dp_mstclean-v9-0-f7779fce10f4@oss.qualcomm.com?part=8