Re: [PATCH v6 04/13] drm/imx: dc-crtc: Disable at boot

[sashiko-bot@kernel.org]

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

New issues:
- [High] Manually disabling the Pixel Engine AXI clock in dc_crtc_disable_at_boot() bypasses Runtime PM, causing hardware hangs on the next CRTC enable due to mismatched states.
- [Medium] Unconditional clk_disable_unprepare() causes CCF underflow warnings on clean boot.

Pre-existing issues:
- [High] __dc_crtc_disable_fg waits for a completion without calling reinit_completion() first.
--

commit 9e8abe6497b6dd773b877a4459824b20ea22a927
Author: Liu Ying <victor.liu@nxp.com>
Subject: drm/imx: dc-crtc: Disable at boot

This commit adds a helper dc_crtc_disable_at_boot() which is called before 
advertising the DRM device to user-space. This addresses an imbalance in 
the Pixel Engine AXI clock and pixel clock use counts caused when CRTCs 
continue running after the DRM device is unplugged.

> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index 764510ff90a4a..71694f1244e17 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c

[ ... ]

> @@ -293,6 +293,16 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
>  	dc_crtc_queue_state_event(new_crtc_state);
>  }
>  
> +static inline void __dc_crtc_disable_fg(struct drm_crtc *crtc)
> +{
> +	struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> +
> +	enable_irq(dc_crtc->irq_dec_seqcomplete);
> +	dc_fg_disable(dc_crtc->fg);
> +	DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);

[Severity: High]
This is a pre-existing issue, but does this wait for completion need a call
to reinit_completion() first?

If a previous wait timed out but the hardware eventually signaled the
completion, the done counter remains greater than 0. Subsequent calls to
wait_for_completion_timeout() will return immediately instead of waiting
for the hardware, which could lead to premature disablement of hardware
clocks while the hardware is still processing the sequence.

> +	disable_irq(dc_crtc->irq_dec_seqcomplete);
> +}
> +

[ ... ]

> @@ -331,6 +337,37 @@ 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)
> +{

[ ... ]

> +	dc_crtc_dbg(crtc, "disabling at boot\n");
> +	__dc_crtc_disable_fg(crtc);
> +	dc_fg_disable_clock(dc_crtc->fg);

[Severity: Medium]
Does this disable the frame generator clock without prior enablement?

On a clean boot where the bootloader left the display running, the code
detects the moving frame index and attempts to disable the clock. Since the
driver hasn't prepared/enabled it yet, the CCF enable_count is 0. This
will trigger a CCF underflow WARN_ON and abort the disablement.

> +
> +	if (!dc_drm->pe_clk_axi_disabled) {
> +		clk_disable_unprepare(dc_drm->pe->clk_axi);

[Severity: High]
Can manually disabling this clock bypass Runtime PM and cause hardware hangs
on the next CRTC enable?

If the DRM module is unloaded while active, the pe->dev usage count is
leaked and it remains in the RPM_ACTIVE state. When the module is reloaded,
this code physically disables the clock.

When the CRTC is next enabled, pm_runtime_resume_and_get() will return
immediately because the device is still RPM_ACTIVE. The clk_prepare_enable()
sequence will be skipped, and subsequent register accesses could hang the
system.

[Severity: Medium]
Similarly to the frame generator clock above, could this also trigger a CCF
underflow warning on a clean boot?

Since the AXI clock wasn't enabled by the driver yet, calling
clk_disable_unprepare() when the CCF enable_count is 0 will trigger a
WARN_ON and the clock won't actually be disabled.

> +		dc_drm->pe_clk_axi_disabled = true;
> +	}
> +
> +out:
> +	pm_runtime_put(dc_crtc->de->dev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-imx8-dc-prefetch-v6-0-26643e3f6cb7@nxp.com?part=4