Re: [PATCH v8 07/15] drm/msm/dp: move the pixel clock control to its own API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Spurious WARN_ON_ONCE triggered on display enable retry due to unhandled clock state on error path.
- [Low] Incomplete refactoring: msm_dp_ctrl_off_link_stream() still uses open-coded pixel clock disablement.
--

--- Patch [7]: [PATCH v8 07/15] drm/msm/dp: move the pixel clock control to its own API ---
commit 56bf43c0164f110e1bba46e5b2fdcdcbaa97bf82
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: move the pixel clock control to its own API

Enable/Disable of DP pixel clock happens in multiple code paths
leading to code duplication. Move it into individual helpers so that
the helpers can be called wherever necessary.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 711579de899d8..a79d9bb93cd06 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c

[ ... ]

> +static int msm_dp_ctrl_on_pixel_clk(struct msm_dp_ctrl_private *ctrl, unsigned long pixel_rate)
> +{
> +	int ret;
> +
> +	ret = clk_set_rate(ctrl->pixel_clk, pixel_rate * 1000);
> +	if (ret) {
> +		DRM_ERROR("Failed to set pixel clock rate. ret=%d\n", ret);
> +		return ret;
> +	}
> +
> +	if (WARN_ON_ONCE(ctrl->stream_clks_on))
> +		return 0;

[Severity: Medium]
Can this WARN_ON_ONCE be triggered by a recoverable hardware timeout?

If msm_dp_ctrl_wait4video_ready() fails in msm_dp_ctrl_on_stream(), it returns
directly without disabling the pixel clock, leaving ctrl->stream_clks_on set
to true.

Since the display enable failed, msm_dp_display->power_on remains false.
A subsequent cleanup call to msm_dp_display_disable() returns immediately
because power_on is false, which prevents it from cleaning up the clock state.

When the display is retried, msm_dp_ctrl_on_pixel_clk() will be called again
and hit this new warning.

> +
> +	ret = clk_prepare_enable(ctrl->pixel_clk);
> +	if (ret) {
> +		DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
> +		return ret;
> +	}
> +	ctrl->stream_clks_on = true;
> +
> +	return ret;
> +}
> +
> +static void msm_dp_ctrl_off_pixel_clk(struct msm_dp_ctrl *msm_dp_ctrl)
> +{
> +	struct msm_dp_ctrl_private *ctrl;
> +
> +	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
> +
> +	if (ctrl->stream_clks_on) {
> +		clk_disable_unprepare(ctrl->pixel_clk);
> +		ctrl->stream_clks_on = false;
> +	}
> +}

[Severity: Low]
This isn't a bug, but could this new helper also be used in
msm_dp_ctrl_off_link_stream() to replace the open-coded check?

It looks like msm_dp_ctrl_off_link_stream() still retains the duplicated pixel
clock disablement logic:

if (ctrl->stream_clks_on) {
        clk_disable_unprepare(ctrl->pixel_clk);
        ctrl->stream_clks_on = false;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-dp_mstclean-v8-0-bea261bf4e7d@oss.qualcomm.com?part=7