Re: [PATCH v9 07/16] drm/imx: dc-crtc: Disable at boot

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/imx: dc-crtc: premature clk_axi disable in dc_crtc_disable_at_boot()
--

commit 8bca21cfc221adc6d10d2019ff78d5b2dc5c6e91
Author: Liu Ying <victor.liu@nxp.com>

drm/imx: dc-crtc: Disable at boot

This commit adds a dc_crtc_disable_at_boot() helper and calls it for each
CRTC during driver bind to disable any running CRTCs and balance display
and AXI clock usage counts before registering the DRM device.

> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index 0d64186a99018..5bc952d6883e1 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c

[ ... ]

> @@ -331,6 +341,48 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  }
>  
> +int dc_crtc_disable_at_boot(struct drm_crtc *crtc)
> +{
> +	struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
> +	struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dc_crtc->de->dev);
> +	if (ret < 0) {
> +		dc_crtc_err(crtc, "failed to get DC display engine RPM: %d\n",
> +			    ret);
> +		goto out1;
> +	}
> +
> +	if (!dc_fg_wait_for_frame_index_moving(dc_crtc->fg)) {
> +		dc_crtc_dbg(crtc, "FrameGen frame index isn't moving\n");
> +		goto out2;
> +	}
> +
> +	dc_crtc_dbg(crtc, "disabling FrameGen at boot\n");
> +	__dc_crtc_disable_fg(crtc);
> +
> +	if (!dc_drv_crtc_is_enabled(dc_drm, drm_crtc_index(crtc)))
> +		goto out2;
> +
> +	dc_crtc_dbg(crtc, "disabling clocks at boot\n");
> +
> +	dc_fg_disable_clock(dc_crtc->fg);
> +
> +	dc_drm->pe_clk_axi_needs_disable = true;
> +
> +	dc_drv_set_crtc_disable(dc_drm, drm_crtc_index(crtc));
> +
> +out2:
> +	pm_runtime_put(dc_crtc->de->dev);
> +out1:
> +	/* Let CRTC(with index == 1) disablement path disable AXI clock. */
> +	if (dc_drm->pe_clk_axi_needs_disable && drm_crtc_index(crtc) == 1)
> +		clk_disable_unprepare(dc_drm->pe->clk_axi);
> +
> +	return ret;
> +}

[Severity: Medium]
As reported by Sashiko on v8 [1], can dc_crtc_disable_at_boot() in
drivers/gpu/drm/imx/dc/dc-crtc.c prematurely disable dc_drm->pe->clk_axi
when disabling CRTC 1 fails?

In dc_drm_bind(), drm_for_each_crtc() invokes dc_crtc_disable_at_boot()
for CRTC 0 and then CRTC 1. When CRTC 0 was active at boot, its call sets
dc_drm->pe_clk_axi_needs_disable = true and clears CRTC_ENABLED(0).

When dc_crtc_disable_at_boot() then runs for CRTC 1, if
pm_runtime_resume_and_get(dc_crtc->de->dev) fails and jumps to out1,
CRTC 1's FrameGen has not been stopped and CRTC_ENABLED(1) has not been
cleared. However, at out1, dc_drm->pe_clk_axi_needs_disable is already
true from CRTC 0 and drm_crtc_index(crtc) == 1, so
clk_disable_unprepare(dc_drm->pe->clk_axi) is called unconditionally.

If CRTC 1 was also active at boot, wouldn't this gate the Pixel Engine
AXI clock while CRTC 1 is still scanning out from the Pixel Engine, and
leave CRTC_ENABLED(1) set in the general purpose register so that a
subsequent bind retry disables dc_drm->pe->clk_axi a second time?

[1] https://lore.kernel.org/r/20261030-imx8-dc-prefetch-v8-6-c4f81afd4533@nxp.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-imx8-dc-prefetch-v9-0-3b985e7636e2@nxp.com?part=7