Re: [PATCH v7 06/15] 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: missing dc_drv_set_crtc_disable() in dc_crtc_disable_at_boot()
--

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

drm/imx: dc-crtc: Disable at boot

This commit adds a helper dc_crtc_disable_at_boot() and calls it during
dc_drm_bind() before drm_dev_register() to disable any running display
controller pipes and balance clock enable counts after an unplug and rebind
cycle.

> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index 0d64186a99018..c7dbe5f500b4a 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
> @@ -287,12 +288,24 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_commit *state)

[ ... ]

> @@ -318,8 +327,9 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
>  	/* request display engine power-off when CRTC is disabled */
>  	pm_runtime_put(dc_crtc->de->dev);
>  
> -	drm_dev_exit(idx);
> +	dc_drv_set_crtc_disable(dc_drm, drm_crtc_index(crtc));
>  
> +	drm_dev_exit(idx);
>  out:
>  	drm_crtc_vblank_off(crtc);
>  
> @@ -331,6 +341,43 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  }
>  
> +void 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);
> +		return;
> +	}
> +
> +	if (!dc_fg_wait_for_frame_index_moving(dc_crtc->fg)) {
> +		dc_crtc_dbg(crtc, "FrameGen frame index isn't moving\n");
> +		goto out;
> +	}
> +
> +	dc_crtc_dbg(crtc, "disabling FrameGen at boot\n");
> +	__dc_crtc_disable_fg(crtc);
> +
> +	if (!dc_drv_crtc_was_enabled(dc_drm, drm_crtc_index(crtc)))
> +		goto out;
> +
> +	dc_crtc_dbg(crtc, "disabling clocks at boot\n");
> +
> +	dc_fg_disable_clock(dc_crtc->fg);
> +
> +	if (!dc_drm->pe_clk_axi_disabled) {
> +		clk_disable_unprepare(dc_drm->pe->clk_axi);
> +		dc_drm->pe_clk_axi_disabled = true;
> +	}

[Severity: Medium]
Should dc_crtc_disable_at_boot() also call dc_drv_set_crtc_disable() after
disabling the clocks here?

When a pipe is enabled via dc_crtc_atomic_enable(), dc_drv_set_crtc_enable()
sets the corresponding bit in priv->gp_regmap:

drivers/gpu/drm/imx/dc/dc-drv.c:dc_drv_set_crtc_enable() {
	...
	regmap_set_bits(priv->gp_regmap, 0, CRTC_ENABLED(crtc_index));
}

If the device is then unplugged via dc_drm_unbind() -> drm_dev_unplug(),
dc_crtc_atomic_disable() returns early at drm_dev_enter() without calling
dc_drv_set_crtc_disable().

On the next bind in dc_drm_bind() -> dc_crtc_disable_at_boot(), the helper
stops the frame generator, sees dc_drv_crtc_was_enabled() return true, and
disables dc_crtc->fg->clk_disp and dc_drm->pe->clk_axi, dropping their clock
enable and prepare counts back to zero. However, because it does not call
dc_drv_set_crtc_disable(dc_drm, drm_crtc_index(crtc)), the enable bit in
priv->gp_regmap remains set.

If dc_crtc_disable_at_boot() runs again while the frame generator is active
without an intervening atomic enable and disable cycle (for instance, across
a warm reboot where the general-purpose register stays set and the
bootloader enables the display, or if __dc_crtc_disable_fg() times out and
bind is retried), wouldn't dc_drv_crtc_was_enabled() still see the stale bit
and call dc_fg_disable_clock() and clk_disable_unprepare() on clocks that
are already disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-imx8-dc-prefetch-v7-0-a0b50cc4fc65@nxp.com?part=6