Re: [PATCH] drm/msm/dsi: don't re-lock the PHY PLL on every DSI command

Re: [PATCH] drm/msm/dsi: don't re-lock the PHY PLL on every DSI command

[Dmitry Baryshkov]

On Mon, Jul 06, 2026 at 11:07:53AM -0700, Kavan Smith wrote:
> msm_dsi_host_xfer_prepare() runs for every DSI command, including runtime
> DCS writes such as backlight (MIPI DCS 0x51), and unconditionally calls
> link_clk_set_rate() before enabling the link clocks. On MSM8916 (DSI 6G
> v1.3.1) the requested byte-clock rate never exactly equals the DSI PHY
> PLL's achievable rate (e.g. 56250000 Hz requested vs 56246337 Hz from the
> PLL), so the clk framework treats every call as a rate change and re-locks
> the PLL.
> 
> During video-mode operation the byte, pixel and byte-intf clocks are
> already running and feeding continuous scanout. Re-locking the PLL glitches
> that live clock. On a video-mode panel with no internal timing generator
> (e.g. samsung,s6d7aa0 / lsl080al03 on the Samsung Galaxy Tab A 8.0, which
> regenerates its H/V timing directly from the DSI clock lane) the glitch
> makes the panel lose pixel lock, producing ~1 second of displaced/wrapped
> scanout on every DCS command. No FIFO underrun or dsi_err_worker error
> fires; it is a silent clock-domain glitch. Stock (downstream MDSS) firmware
> sends the same 0x51 with CMD_CLK_CTRL, which only refcount-enables the
> clocks and never re-runs clk_set_rate, and does not glitch.
> 
> The link rate is already programmed at power-on by msm_dsi_host_power_on(),
> so skip the redundant re-set once the link is up; init-time transfers
> (before power_on) still set the rate.

THe problem is that by definition the transfer callbacks can be sent at
any time, see the comment at mipi_dsi_host_ops. I think the proper fix
would be to call clk_round_rate() beforehand, making the CCF skip all
further rate sets.


> 
> Link: https://lists.freedesktop.org/archives/dri-devel/2018-April/172218.html

This should be Reported-by / Closes. Also probably add Fixes and cc:stable.

> Signed-off-by: Kavan Smith <kavansmith82@gmail.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH] drm/msm/dsi: don't re-lock the PHY PLL on every DSI command

[sashiko-bot]

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

New issues:
- [High] The conditionally skipped `link_clk_set_rate()` is actually dead code, introducing a regression that permanently drops the OPP performance state vote.

Pre-existing issues:
- [High] Missing return value check for `pm_runtime_get_sync()` leads to unclocked/unpowered register access.
- [High] Missing error check for `link_clk_enable()` in `msm_dsi_host_xfer_prepare` leads to unclocked register access and CCF clock refcount underflow.
- [Medium] Runtime PM usage counter leak in `msm_dsi_host_power_on()` error path.
--

commit 215b342ad464ceca1a10f5f0b7106969f6672459
Author: Kavan Smith <kavansmith82@gmail.com>
Subject: drm/msm/dsi: don't re-lock the PHY PLL on every DSI command

This commit attempts to prevent re-locking the PHY PLL on every DSI command
by skipping the link clock rate update if the link is already up. It does
this by checking if the power is already on during the transfer preparation.

Link: https://lists.freedesktop.org/archives/dri-devel/2018-April/172218.html

> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 982abaaac00d5..a98fc89596ca2 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -2170,7 +2170,17 @@ int msm_dsi_host_xfer_prepare(struct mipi_dsi_host *host,
>  	 * mdp clock need to be enabled to receive dsi interrupt
>  	 */
>  	pm_runtime_get_sync(&msm_host->pdev->dev);

[Severity: High]
This is a pre-existing issue, but does this code need to check the return
value of pm_runtime_get_sync()?

If it fails to resume the device's PM domain, the device remains unpowered or
unclocked, and the subsequent register accesses (like dsi_write on
REG_DSI_CTRL) might trigger a synchronous external abort.

Looking at msm_dsi_host_power_on(), it also ignores the return value of
pm_runtime_get_sync() before accessing hardware.

[Severity: Medium]
Also in msm_dsi_host_power_on(), this is a pre-existing issue, but does the
error path permanently leak the runtime PM usage count?

If an initialization step fails, the code jumps to fail_disable_reg:

msm_dsi_host_power_on() {
    ...
    pm_runtime_get_sync(&msm_host->pdev->dev);
    ...
    if (ret)
        goto fail_disable_reg;
    ...
fail_disable_clk:
    pm_runtime_put(&msm_host->pdev->dev);
fail_disable_reg:
    ...
}

Since this bypasses the fail_disable_clk label which contains the
pm_runtime_put() call, this seems to prevent the device from ever suspending.

> -	cfg_hnd->ops->link_clk_set_rate(msm_host);
> +	/*
> +	 * Don't re-set the link clock rate when the link is already up. The
> +	 * requested byte-clock rate rarely equals the DSI PHY PLL's achievable
> +	 * rate, so clk_set_rate() re-locks the PLL on every command; for a
> +	 * video-mode panel with no internal timing generator that clock glitch
> +	 * makes the panel lose pixel lock mid-scanout (~1s of displaced image on
> +	 * each DCS write, e.g. every backlight update). The rate is already set
> +	 * at power-on.
> +	 */
> +	if (!msm_host->power_on)
> +		cfg_hnd->ops->link_clk_set_rate(msm_host);

[Severity: High]
Can the !msm_host->power_on condition ever be true?

msm_dsi_host_xfer_prepare() is exclusively reachable via dsi_host_transfer(),
which explicitly checks !msm_host->power_on and returns early:

drivers/gpu/drm/msm/dsi/dsi_host.c:dsi_host_transfer() {
    ...
	if (!msg || !msm_host->power_on)
		return -EINVAL;
    ...
}

Because msm_host->power_on is always true here, the newly added condition is
permanently false, meaning link_clk_set_rate() is never called during DCS
transfers.

Since msm_dsi_host_xfer_restore() unconditionally calls link_clk_disable()
(which explicitly zeroes the OPP performance state vote via
dev_pm_opp_set_rate), won't this permanently drop the OPP performance state
vote on all subsequent commands? The hardware would be left operating at high
clock frequencies with a 0 voltage vote.

>  	cfg_hnd->ops->link_clk_enable(msm_host);

[Severity: High]
This is a pre-existing issue, but does this code need to check the return
value of link_clk_enable()?

If enabling the link clocks fails (e.g., clk_prepare_enable() fails internally),
the clocks remain off and the reference count is not incremented. The driver
would then proceed to access hardware registers unclocked, triggering a
synchronous external abort.

Furthermore, msm_dsi_host_xfer_restore() unconditionally calls
link_clk_disable(). This decrements a clock reference count that was never
successfully incremented, which causes a CCF refcount underflow WARN.

>  
>  	/* TODO: vote for bus bandwidth */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706180753.408753-1-kavansmith82@gmail.com?part=1