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.
== Some alternatives discussed: ==
It's likely that on many display controllers, vblank interrupts will
turn off when the CRTC is disabled, and so in some cases, self-refresh
may not support vblank. To support such cases, we might consider
additions to the generic helpers such that we fire vblank events based
on a timer.
However, there is currently only one driver using the common
self-refresh helpers (i.e., rockchip), and at least as of commit
bed030a49f3e ("drm/rockchip: Don't fully disable vop on self refresh"),
the CRTC hardware is powered enough to continue to generate vblank
interrupts.
So we chose the simpler option of leaving vblank interrupts enabled. We
can reevaluate this decision and perhaps augment the helpers if/when we
gain a second driver that has different requirements.
v3:
* include discussion summary
v2:
* add 'ret != 0' warning case for self-refresh
* describe failing test case and relation to drm/rockchip patch better
Cc: <[email protected]> # dependency for "drm/rockchip: vop: Leave
# vblank enabled in self-refresh"
Signed-off-by: Brian Norris <[email protected]>
---
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);
}
--
2.39.0.314.g84b9a713c41-goog
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. The bug has been around a while (longer than KernelCI
noticed), but was only exposed once self-refresh was bugfixed more
recently, and so KernelCI could properly test it. Some other notes in:
https://lore.kernel.org/dri-devel/[email protected]/
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
== 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.
v3:
* no update
v2:
* skip unnecessary lock/unlock
Fixes: 6c836d965bad ("drm/rockchip: Use the helpers for PSR")
Cc: <[email protected]>
Link: https://lore.kernel.org/dri-devel/[email protected]/
Reported-by: "kernelci.org bot" <[email protected]>
Signed-off-by: Brian Norris <[email protected]>
---
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);
--
2.39.0.314.g84b9a713c41-goog
On Mon, Jan 09, 2023 at 05:18:16PM -0800, Brian Norris wrote:
> 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.
>
> == Some alternatives discussed: ==
>
> It's likely that on many display controllers, vblank interrupts will
> turn off when the CRTC is disabled, and so in some cases, self-refresh
> may not support vblank. To support such cases, we might consider
> additions to the generic helpers such that we fire vblank events based
> on a timer.
>
> However, there is currently only one driver using the common
> self-refresh helpers (i.e., rockchip), and at least as of commit
> bed030a49f3e ("drm/rockchip: Don't fully disable vop on self refresh"),
> the CRTC hardware is powered enough to continue to generate vblank
> interrupts.
>
> So we chose the simpler option of leaving vblank interrupts enabled. We
> can reevaluate this decision and perhaps augment the helpers if/when we
> gain a second driver that has different requirements.
>
> v3:
> * include discussion summary
>
> v2:
> * add 'ret != 0' warning case for self-refresh
> * describe failing test case and relation to drm/rockchip patch better
>
> Cc: <[email protected]> # dependency for "drm/rockchip: vop: Leave
> # vblank enabled in self-refresh"
> Signed-off-by: Brian Norris <[email protected]>
Pushed both patches to drm-misc-next, thanks Brian
> ---
> 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);
> }
> --
> 2.39.0.314.g84b9a713c41-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS