The self-refresh helper framework overloads "disable" to sometimes mean "go into self-refresh mode," and this mode activates automatically (e.g., after some period of unchanging display output). In such cases, the display pipe is still considered "on", and user-space is not aware that we went into self-refresh mode. Thus, users may expect that vblank-related features (such as DRM_IOCTL_WAIT_VBLANK) still work properly.
However, we trigger the WARN_ONCE() here if a CRTC driver tries to leave vblank enabled.
Add a different expectation: that CRTCs *should* leave vblank enabled when going into self-refresh.
This patch is preparation for another patch -- "drm/rockchip: vop: Leave vblank enabled in self-refresh" -- which resolves conflicts between the above self-refresh behavior and the API tests in IGT's kms_vblank test module.
v2: * add 'ret != 0' warning case for self-refresh * describe failing test case and relation to drm/rockchip patch better
Cc: stable@vger.kernel.org # dependency for "drm/rockchip: vop: Leave # vblank enabled in self-refresh" Signed-off-by: Brian Norris briannorris@chromium.org --- drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d579fd8f7cb8..a22485e3e924 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1209,7 +1209,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) continue;
ret = drm_crtc_vblank_get(crtc); - WARN_ONCE(ret != -EINVAL, "driver forgot to call drm_crtc_vblank_off()\n"); + /* + * Self-refresh is not a true "disable"; ensure vblank remains + * enabled. + */ + if (new_crtc_state->self_refresh_active) + WARN_ONCE(ret != 0, + "driver disabled vblank in self-refresh\n"); + else + WARN_ONCE(ret != -EINVAL, + "driver forgot to call drm_crtc_vblank_off()\n"); if (ret == 0) drm_crtc_vblank_put(crtc); }
If we disable vblank when entering self-refresh, vblank APIs (like DRM_IOCTL_WAIT_VBLANK) no longer work. But user space is not aware when we enter self-refresh, so this appears to be an API violation -- that DRM_IOCTL_WAIT_VBLANK fails with EINVAL whenever the display is idle and enters self-refresh.
The downstream driver used by many of these systems never used to disable vblank for PSR, and in fact, even upstream, we didn't do that until radically redesigning the state machine in commit 6c836d965bad ("drm/rockchip: Use the helpers for PSR").
Thus, it seems like a reasonable API fix to simply restore that behavior, and leave vblank enabled.
Note that this appears to potentially unbalance the drm_crtc_vblank_{off,on}() calls in some cases, but: (a) drm_crtc_vblank_on() documents this as OK and (b) if I do the naive balancing, I find state machine issues such that we're not in sync properly; so it's easier to take advantage of (a).
This issue was exposed by IGT's kms_vblank tests, and reported by KernelCI.
Backporting notes: Marking as 'Fixes' commit 6c836d965bad ("drm/rockchip: Use the helpers for PSR"), but it probably depends on commit bed030a49f3e ("drm/rockchip: Don't fully disable vop on self refresh") as well.
We also need the previous patch ("drm/atomic: Allow vblank-enabled + self-refresh "disable""), of course.
v2: * skip unnecessary lock/unlock
Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/dri-devel/Y5itf0+yNIQa6fU4@sirena.org.uk/ Reported-by: "kernelci.org bot" bot@kernelci.org Signed-off-by: Brian Norris briannorris@chromium.org --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index fa1f4ee6d195..9fea03121247 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -717,13 +717,13 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc, if (crtc->state->self_refresh_active) rockchip_drm_set_win_enabled(crtc, false);
+ if (crtc->state->self_refresh_active) + goto out; + mutex_lock(&vop->vop_lock);
drm_crtc_vblank_off(crtc);
- if (crtc->state->self_refresh_active) - goto out; - /* * Vop standby will take effect at end of current frame, * if dsp hold valid irq happen, it means standby complete. @@ -757,9 +757,9 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc, vop_core_clks_disable(vop); pm_runtime_put(vop->dev);
-out: mutex_unlock(&vop->vop_lock);
+out: if (crtc->state->event && !crtc->state->active) { spin_lock_irq(&crtc->dev->event_lock); drm_crtc_send_vblank_event(crtc, crtc->state->event);
On Fri, Jan 6, 2023 at 5:23 PM Brian Norris briannorris@chromium.org wrote:
v2:
- add 'ret != 0' warning case for self-refresh
- describe failing test case and relation to drm/rockchip patch better
Ugh, there's always something you remember right after you hit send: I forgot to better summarize some of the other discussion from v1, and alternatives we didn't entertain. I'll write that up now (not sure whether in patch 1 or 2) and plan on sending a v3 for next week, in case there are any other comments I should address at the same time.
Sorry for the noise, Brian
On Fri, Jan 06, 2023 at 05:27:33PM -0800, Brian Norris wrote:
On Fri, Jan 6, 2023 at 5:23 PM Brian Norris briannorris@chromium.org wrote:
v2:
- add 'ret != 0' warning case for self-refresh
- describe failing test case and relation to drm/rockchip patch better
Ugh, there's always something you remember right after you hit send: I forgot to better summarize some of the other discussion from v1, and alternatives we didn't entertain. I'll write that up now (not sure whether in patch 1 or 2) and plan on sending a v3 for next week, in case there are any other comments I should address at the same time.
For me it needs to be in the helper patch, since anyone not doing rockchip work will otherwise never find it. But you can also duplicate the discussion summary into the 2nd patch, doesn't really hurt. -Daniel
linux-stable-mirror@lists.linaro.org
-
Brian Norris -
Daniel Vetter