Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/imx: dc-crtc: premature clk_axi disable in dc_crtc_disable_at_boot() --
commit e47c892a97b7a909d6fe7c57ba9a8454d386aea8 Author: Liu Ying <[email protected]> drm/imx: dc-crtc: Disable at boot This commit adds dc_crtc_disable_at_boot() and calls it for each CRTC in dc_drm_bind() prior to drm_dev_register() to ensure CRTCs and their associated clocks left running across an unplug are disabled at boot. > diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c > b/drivers/gpu/drm/imx/dc/dc-crtc.c > index 0d64186a99018..5bc952d6883e1 100644 > --- a/drivers/gpu/drm/imx/dc/dc-crtc.c > +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c [ ... ] > @@ -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"); > + goto out2; > + } > + > + dc_crtc_dbg(crtc, "disabling FrameGen at boot\n"); > + __dc_crtc_disable_fg(crtc); > + > + if (!dc_drv_crtc_is_enabled(dc_drm, drm_crtc_index(crtc))) > + goto out2; > + > + dc_crtc_dbg(crtc, "disabling clocks at boot\n"); > + > + dc_fg_disable_clock(dc_crtc->fg); > + > + dc_drm->pe_clk_axi_needs_disable = true; > + > + dc_drv_set_crtc_disable(dc_drm, drm_crtc_index(crtc)); > + > +out2: > + pm_runtime_put(dc_crtc->de->dev); > +out1: > + /* Let CRTC(with index == 1) disablement path disable AXI clock. */ > + if (dc_drm->pe_clk_axi_needs_disable && drm_crtc_index(crtc) == 1) > + clk_disable_unprepare(dc_drm->pe->clk_axi); > + > + return ret; > +} [Severity: Medium] In dc_crtc_disable_at_boot() in drivers/gpu/drm/imx/dc/dc-crtc.c, can jumping to out1 when pm_runtime_resume_and_get() fails on CRTC1 prematurely disable dc_drm->pe->clk_axi? When dc_drm_bind() iterates over CRTCs with drm_for_each_crtc(): 1. If CRTC0 was left enabled before unplug, dc_crtc_disable_at_boot() disables CRTC0's FrameGen and pixel clock, sets dc_drm->pe_clk_axi_needs_disable to true, and clears CRTC_ENABLED(0). 2. Next, dc_crtc_disable_at_boot() is called for CRTC1 (drm_crtc_index(crtc) == 1). If pm_runtime_resume_and_get(dc_crtc->de->dev) fails, it jumps to out1 before stopping CRTC1's FrameGen or clearing CRTC_ENABLED(1). 3. At out1, both dc_drm->pe_clk_axi_needs_disable and drm_crtc_index(crtc) == 1 are true, so clk_disable_unprepare(dc_drm->pe->clk_axi) is called even though CRTC1 failed to disable. If CRTC1 was also active, this cuts the Pixel Engine AXI clock while CRTC1 is still scanning out through the Pixel Engine, and leaves CRTC_ENABLED(1) set so that a subsequent bind attempt would disable dc_drm->pe->clk_axi a second time. Should the clk_disable_unprepare(dc_drm->pe->clk_axi) call happen before out1 (under out2), or only when ret == 0? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
