Re: [PATCH] drm/msm/dpu: keep vblank enabled when entering self refresh

Re: [PATCH] drm/msm/dpu: keep vblank enabled when entering self refresh

[Ramón Jesús Miranda]

Thanks for the review. Both points look right to me: the self refresh
exit path in dpu_crtc_disable() returns early without
drm_crtc_vblank_off(), and with vblank_disable_immediate the vblank
worker can end up touching DPU interrupt registers after the last
runtime PM reference is gone.

Please consider this patch withdrawn. I will look at keeping the vblank
IRQ path consistent with the self refresh entry/exit (and the runtime PM
reference) and send a v2 if I get it right.

Thanks,
Ramón

[PATCH] drm/msm/dpu: keep vblank enabled when entering self refresh

[Ramón Jesús Miranda]

When the CRTC is disabled to enter self refresh, dpu_crtc_disable() only
checks old_crtc_state->self_refresh_active, which covers leaving self
refresh, and calls drm_crtc_vblank_off() otherwise. The atomic helper
then expects vblank to remain enabled for a self refresh "disable" and
warns:

  driver disabled vblank in self-refresh
  WARNING: drivers/gpu/drm/drm_atomic_helper.c:1296 at
  drm_atomic_helper_commit_crtc_disable+0x1c0/0x1f8 [drm_kms_helper]

Skip drm_crtc_vblank_off() when the new state enters self refresh, as
other drivers supporting self refresh do. drm_crtc_vblank_on() in the
enable path is safe either way, it only drops the reference taken by
drm_crtc_vblank_off().

Seen at every boot on a Microsoft Surface Pro 11 (X1E80100, Samsung OLED
eDP panel) with eDP PSR enabled; with this change the warning is gone and
self refresh keeps working.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Ramón Jesús Miranda <ramonjesusmiranda@gmail.com>
---
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1212,8 +1212,13 @@
 		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);
 
 	drm_for_each_encoder_mask(encoder, crtc->dev,
 				  old_crtc_state->encoder_mask) {