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 <[email protected]>
>> ---
>> 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
