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

[Liu Ying <victor.liu@nxp.com>]

On Fri, Sep 04, 2026 at 10:27:41AM +0200, Maxime Ripard wrote:
> Hi,

Hi,

Sorry for late reply.

> 
> 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

Good to know it.  Maybe, imx8-dc can use it in the future.

> 
> > ---
> > 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.

Because imx8-dc supports two CRTCs and dc_drm_bind() calls this function
to disable both the CRTCs, pe_clk_axi_disabled flag is just used to avoid
disabling clk_axi twice.  clk_axi is managed by runtime PM, so it can only
be enabled once.

> Maxime

-- 
Regards,
Liu Ying