Re: [PATCH v8 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: premature clk_axi disable in dc_crtc_disable_at_boot()
--

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

drm/imx: dc-crtc: Disable at boot

This commit adds dc_crtc_disable_at_boot() and calls it for each CRTC in
dc_drm_bind() prior to drm_dev_register() to ensure CRTCs and their
associated clocks left running across an unplug are disabled at boot.

> 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]
In dc_crtc_disable_at_boot() in drivers/gpu/drm/imx/dc/dc-crtc.c, can jumping
to out1 when pm_runtime_resume_and_get() fails on CRTC1 prematurely disable
dc_drm->pe->clk_axi?

When dc_drm_bind() iterates over CRTCs with drm_for_each_crtc():

1. If CRTC0 was left enabled before unplug, dc_crtc_disable_at_boot() disables
   CRTC0's FrameGen and pixel clock, sets dc_drm->pe_clk_axi_needs_disable to
   true, and clears CRTC_ENABLED(0).

2. Next, dc_crtc_disable_at_boot() is called for CRTC1 (drm_crtc_index(crtc) == 1).
   If pm_runtime_resume_and_get(dc_crtc->de->dev) fails, it jumps to out1
   before stopping CRTC1's FrameGen or clearing CRTC_ENABLED(1).

3. At out1, both dc_drm->pe_clk_axi_needs_disable and drm_crtc_index(crtc) == 1
   are true, so clk_disable_unprepare(dc_drm->pe->clk_axi) is called even
   though CRTC1 failed to disable.

If CRTC1 was also active, this cuts the Pixel Engine AXI clock while CRTC1 is
still scanning out through the Pixel Engine, and leaves CRTC_ENABLED(1) set so
that a subsequent bind attempt would disable dc_drm->pe->clk_axi a second time.

Should the clk_disable_unprepare(dc_drm->pe->clk_axi) call happen before out1
(under out2), or only when ret == 0?

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