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

[Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>]

On Mon, Nov 02, 2026 at 10:11:22AM +0800, Liu Ying wrote:
> 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@nxp.com>
> > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > ---
> > > 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.

Ok

Reviewed-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>

-- 
Thanks,
Laurentiu