Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/msm/dpu: missing drm_crtc_vblank_off() in dpu_crtc_disable() from PSR - [High] drm/msm/dpu: keeping vblank on in dpu_crtc_disable() causes unclocked MMIO --
commit 520cd497636d4c5f54127317e7ab1594f34dcf4a Author: Ramón Jesús Miranda <[email protected]> drm/msm/dpu: keep vblank enabled when entering self refresh This commit skips calling drm_crtc_vblank_off() in dpu_crtc_disable() when entering self-refresh so that vblank remains enabled and avoids the warning in drm_atomic_helper_commit_crtc_disable(). > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 77a2d1f6700d0..eedcd21b936c2 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1212,8 +1212,13 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > return; > } > > - /* Disable/save vblank irq handling */ > - drm_crtc_vblank_off(crtc); > + /* > + * Disable/save vblank irq handling, unless we are entering self > + * refresh: the DRM core expects vblank to stay enabled then and > + * warns ("driver disabled vblank in self-refresh") otherwise. > + */ > + if (!crtc->state->self_refresh_active) > + drm_crtc_vblank_off(crtc); [Severity: High] What happens if the CRTC is later disabled while it is in self-refresh mode (for example via DPMS off, blanking, modeset, or system suspend, where old_crtc_state->self_refresh_active is true and new_crtc_state->self_refresh_active is false)? Earlier in dpu_crtc_disable(), the old_crtc_state->self_refresh_active branch detaches the encoder and returns early: drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:dpu_crtc_disable() if (old_crtc_state->self_refresh_active) { drm_for_each_encoder_mask(encoder, crtc->dev, old_crtc_state->encoder_mask) { dpu_encoder_assign_crtc(encoder, NULL); } return; } Because drm_crtc_vblank_off(crtc) was skipped when entering self-refresh and the subsequent disable returns early without calling drm_crtc_vblank_off(crtc) either, won't drm_atomic_helper_commit_crtc_disable() see drm_crtc_vblank_get(crtc) return 0 instead of -EINVAL and trigger: WARN_ONCE(ret != -EINVAL, "driver forgot to call drm_crtc_vblank_off()\n"); while leaving vblank enabled on the disabled CRTC after dpu_enc->crtc has been cleared to NULL? With the encoder detached, subsequent calls to dpu_crtc_get_vblank_counter() or dpu_crtc_get_scanout_position() will log "no encoder found for crtc", and dpu_encoder_toggle_vblank_for_crtc() will no longer match dpu_enc->crtc == crtc to disable the vblank IRQ or drop phys_enc->vblank_refcount. [Severity: High] Also, is it safe to leave DRM vblank enabled across self-refresh entry while the DPU encoder, CRTC, and clocks are powered down? When entering self-refresh, dpu_encoder_virt_atomic_disable() stops the timing engine, disables encoder IRQs, and drops the encoder runtime PM reference via _dpu_encoder_resource_disable(), and dpu_crtc_disable() drops the CRTC runtime PM reference via pm_runtime_put_sync(crtc->dev->dev). Because dpu_kms sets dev->vblank_disable_immediate = true, vblank->enabled is false and vblank->refcount is 0 when the idle timer enters self-refresh. With drm_crtc_vblank_off(crtc) skipped here, drm_atomic_helper_commit_crtc_disable() immediately calls: drm_crtc_vblank_get(crtc) -> drm_vblank_enable() -> msm_crtc_enable_vblank() -> vblank_ctrl_queue_work(priv, crtc, true) drm_crtc_vblank_put(crtc) Since disable_immediate is set, drm_crtc_vblank_put() leaves vblank->enabled as true (waiting for a hardware vblank interrupt that cannot fire while the timing generator is stopped), and vblank_ctrl_worker runs asynchronously on priv->kms->wq without holding a runtime PM reference: vblank_ctrl_worker() -> dpu_crtc_vblank(crtc, true) -> dpu_encoder_toggle_vblank_for_crtc() -> dpu_encoder_phys_vid_control_vblank_irq() -> dpu_core_irq_register_callback() -> dpu_hw_intr_enable_irq_locked() Once msm_atomic_commit_tail() finishes and drops the last runtime PM reference in dpu_kms_disable_commit(), dpu_runtime_suspend() disables the DPU clocks. Can vblank_ctrl_worker (or userspace calling DRM_IOCTL_WAIT_VBLANK or DRM_IOCTL_CRTC_GET_SEQUENCE while in PSR) then perform unclocked MMIO writes in dpu_hw_intr_enable_irq_locked() and unclocked MMIO reads of INTF_LINE_COUNT in dpu_crtc_get_scanout_position() -> dpu_hw_intf_get_line_count(), while also incrementing phys_enc->vblank_refcount on the disabled encoder? > > drm_for_each_encoder_mask(encoder, crtc->dev, > old_crtc_state->encoder_mask) { [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
