Re: [PATCH 05/14] drm/imx: dc-crtc: Disable at boot

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

On 09/19/2025, Frank Li wrote:
> On Fri, Jul 04, 2025 at 05:03:52PM +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")
>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>> ---
>>  drivers/gpu/drm/imx/dc/dc-crtc.c | 50 ++++++++++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/imx/dc/dc-drv.c  |  5 ++++
>>  drivers/gpu/drm/imx/dc/dc-drv.h  |  3 +++
>>  3 files changed, 53 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c
>> index 31d3a982deaf7a0390937285c9d5d00100323181..45a87df1ad6a8bd768aa5ed38d6f03f14052b3d7 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_state *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);
>> +	disable_irq(dc_crtc->irq_dec_seqcomplete);
>> +}
>> +
>>  static void
>>  dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
>>  {
>> @@ -305,11 +315,7 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *state)
>>  	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);
>> -
>> +	__dc_crtc_disable_fg(crtc);
>>  	dc_fg_disable_clock(dc_crtc->fg);
>>
>>  	/* request pixel engine power-off as plane is off too */
>> @@ -337,6 +343,40 @@ dc_crtc_atomic_disable(struct drm_crtc *crtc, struct drm_atomic_state *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;
>> +	}
> 
> look like dc_crtc_disable_at_boot() call by dc_drm_bind(), does it call
> twice without call unbind()? Most like other place paired function have
> not correct.

It could be called multiple times by bind() without calling unbind.  But
it's fine and by design.  As this function checks FrameGen frame index
moving before disabling CRTC(no moving with a disabled CRTC), CRTC is
actually disabled at most for one time no matter how many times bind()
is called without unbind().

> 
> Frank
> 
>> +
>> +out:
>> +	ret = pm_runtime_put(dc_crtc->de->dev);
>> +	if (ret < 0)
>> +		dc_crtc_err(crtc, "failed to put DC display engine RPM: %d\n",
>> +			    ret);
>> +}
>> +
>>  static bool dc_crtc_get_scanout_position(struct drm_crtc *crtc,
>>  					 bool in_vblank_irq,
>>  					 int *vpos, int *hpos,
>> diff --git a/drivers/gpu/drm/imx/dc/dc-drv.c b/drivers/gpu/drm/imx/dc/dc-drv.c
>> index 04f021d2d6cfc93972aa8d9073be24d347152602..f93766b6bfbfae8510db05278d104820ca0719c4 100644
>> --- a/drivers/gpu/drm/imx/dc/dc-drv.c
>> +++ b/drivers/gpu/drm/imx/dc/dc-drv.c
>> @@ -17,6 +17,7 @@
>>
>>  #include <drm/clients/drm_client_setup.h>
>>  #include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc.h>
>>  #include <drm/drm_drv.h>
>>  #include <drm/drm_fbdev_dma.h>
>>  #include <drm/drm_fourcc.h>
>> @@ -96,6 +97,7 @@ static int dc_drm_bind(struct device *dev)
>>  	struct dc_priv *priv = dev_get_drvdata(dev);
>>  	struct dc_drm_device *dc_drm;
>>  	struct drm_device *drm;
>> +	struct drm_crtc *crtc;
>>  	int ret;
>>
>>  	dc_drm = devm_drm_dev_alloc(dev, &dc_drm_driver, struct dc_drm_device,
>> @@ -118,6 +120,9 @@ static int dc_drm_bind(struct device *dev)
>>  	if (ret)
>>  		return ret;
>>
>> +	drm_for_each_crtc(crtc, drm)
>> +		dc_crtc_disable_at_boot(crtc);
>> +
>>  	ret = drm_dev_register(drm, 0);
>>  	if (ret) {
>>  		dev_err(dev, "failed to register drm device: %d\n", ret);
>> diff --git a/drivers/gpu/drm/imx/dc/dc-drv.h b/drivers/gpu/drm/imx/dc/dc-drv.h
>> index eb61b8c7626933adc7688f046139e2167665dad1..68e99ba7cedbca1b8bdc0d8ced7a610a1056bfc7 100644
>> --- a/drivers/gpu/drm/imx/dc/dc-drv.h
>> +++ b/drivers/gpu/drm/imx/dc/dc-drv.h
>> @@ -50,6 +50,8 @@ struct dc_drm_device {
>>  	struct dc_pe *pe;
>>  	/** @tc: tcon list */
>>  	struct dc_tc *tc[DC_DISPLAYS];
>> +	/** @pe_clk_axi_disabled: disablement flag at boot */
>> +	bool pe_clk_axi_disabled;
>>  };
>>
>>  struct dc_subdev_info {
>> @@ -96,6 +98,7 @@ static inline int dc_subdev_get_id(const struct dc_subdev_info *info,
>>  	return -EINVAL;
>>  }
>>
>> +void dc_crtc_disable_at_boot(struct drm_crtc *crtc);
>>  void dc_de_post_bind(struct dc_drm_device *dc_drm);
>>  void dc_pe_post_bind(struct dc_drm_device *dc_drm);
>>
>>
>> --
>> 2.34.1
>>


-- 
Regards,
Liu Ying