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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4234 bytes --]

Hi,

On Fri, Sep 04, 2026 at 03:54:54PM +0800, Liu Ying wrote:
> CRTC(s) could still be running after the DRM device is unplugged by
> calling drm_dev_unplug(), because the CRTC disablement logic is
> protected and bypassed by the drm_dev_enter()/drm_dev_exit() pair.
> Hence, Pixel Engine's AXI clock use count(managed by Pixel Engine
> driver's runtime PM) and pixel clock use count could be inbalanced
> after removing and re-installing the driver module.  To fix this,
> add a helper dc_crtc_disable_at_boot() and call it to properly
> disable all CRTCs before advertising DRM device to user-space by
> calling drm_dev_register().
> 
> Fixes: 711a3b878366 ("drm/imx: Add i.MX8qxp Display Controller KMS")
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>

Just as a heads-up, the readout series I'm working on is probably going
to help you deal with it:
https://lore.kernel.org/r/20260629-drm-state-readout-v4-0-5966657980ed@kernel.org

> ---
> v6:
> - Rebase onto the latest drm-misc-next and resolve conflicts.
> 
> v3:
> - Collect Frank's R-b tag.
> ---
>  drivers/gpu/drm/imx/dc/dc-crtc.c | 55 +++++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/imx/dc/dc-drv.c  |  5 ++++
>  drivers/gpu/drm/imx/dc/dc-drv.h  |  3 +++
>  3 files changed, 54 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> index 0d64186a9901..1a537cf88daf 100644
> --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
> @@ -293,23 +293,29 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
>  	dc_crtc_queue_state_event(new_crtc_state);
>  }
>  
> -static void
> -dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
> +static inline void __dc_crtc_disable_fg(struct drm_crtc *crtc)
>  {
> -	struct drm_crtc_state *new_crtc_state =
> -				drm_atomic_get_new_crtc_state(state, crtc);
> -	struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
>  	struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> -	int idx;
> -
> -	if (!drm_dev_enter(crtc->dev, &idx))
> -		goto out;
>  
>  	enable_irq(dc_crtc->irq_dec_seqcomplete);
>  	dc_fg_disable(dc_crtc->fg);
>  	DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(dec_seqcomplete_done);
>  	disable_irq(dc_crtc->irq_dec_seqcomplete);
> +}
>  
> +static void
> +dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
> +{
> +	struct drm_crtc_state *new_crtc_state =
> +				drm_atomic_get_new_crtc_state(state, crtc);
> +	struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
> +	struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> +	int idx;
> +
> +	if (!drm_dev_enter(crtc->dev, &idx))
> +		goto out;
> +
> +	__dc_crtc_disable_fg(crtc);
>  	dc_fg_disable_clock(dc_crtc->fg);
>  
>  	/* request pixel engine power-off as plane is off too */
> @@ -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)
> +{
> +	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 at boot\n");
> +	__dc_crtc_disable_fg(crtc);
> +	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;
> +	}
> +
> +out:
> +	pm_runtime_put(dc_crtc->de->dev);

I'm not sure you actually need the pe_clk_axi_disabled flag. If you make
it handled by runtime_pm, then you can also use pm_runtime_set_active to
flag the device as active already when you probe, and the runtime_pm
will know when to enable / disable things properly.
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]