Re: [PATCH v8 06/15] drm/imx: dc-crtc: Disable at boot

[Liu Ying]

On Fri, Oct 30, 2026 at 03:05:14PM +0200, Laurentiu Palcu wrote:
> On Fri, Oct 30, 2026 at 05:54:30PM +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@xxxxxxx>
> > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > ---
> > v8:
> > - Call dc_drv_set_crtc_disable() in dc_crtc_disable_at_boot() after CRTC
> >   is disabled.  (Sashiko AI bot)
> > - Make dc_crtc_disable_at_boot() return an integer instead of void.
> >   (Laurentiu)
> > - Postpone Pixel Engine AXI clock disablement at boot to the time point
> >   where CRTC1(with index == 1) is being disabled at boot.
> > 
> > v7:
> > - Check CRTC status to ensure that clocks were enabled by a previous boot
> >   before disabling them in dc_crtc_disable_at_boot().  (Sashiko AI bot)
> > - Modify debug message in dc_crtc_disable_at_boot().
> > 
> > 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 | 72 ++++++++++++++++++++++++++++++++++------
> >  drivers/gpu/drm/imx/dc/dc-drv.c  | 11 ++++++
> >  drivers/gpu/drm/imx/dc/dc-drv.h  |  3 ++
> >  3 files changed, 76 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
> > index 0d64186a9901..5bc952d6883e 100644
> > --- a/drivers/gpu/drm/imx/dc/dc-crtc.c
> > +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c
> > @@ -227,6 +227,7 @@ dc_crtc_atomic_enable(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 drm_display_mode *adj = &new_crtc_state->adjusted_mode;
> > +	struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev);
> >  	struct dc_crtc *dc_crtc = to_dc_crtc(crtc);
> >  	enum dc_link_id cf_link;
> >  	int idx, ret;
> > @@ -287,29 +288,37 @@ dc_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_atomic_commit *state)
> >  
> >  	DC_CRTC_CHECK_FRAMEGEN_FIFO(dc_crtc->fg);
> >  
> > +	dc_drv_set_crtc_enable(dc_drm, drm_crtc_index(crtc));
> > +
> >  	drm_dev_exit(idx);
> >  
> >  out:
> >  	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 */
> > @@ -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,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");
> 
> Shouldn't you set 'ret' to something here? Otherwise you'd return the previous
> ret value, which would be 0.

We take FrameGen frame index not moving as a normal case, which means
the CRTC was not enabled before this boot, so returning 0 is fine.

> 
> -- 
> Thanks,
> Laurentiu

-- 
Regards,
Liu Ying