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: <stable@xxxxxxxxxxxxxxx> # dependency for "drm/rockchip: vop: Leave
> # vblank enabled in self-refresh"
> Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
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
