Re: [PATCH v2 1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable"

[Daniel Vetter]

On Fri, Jan 06, 2023 at 05:27:33PM -0800, Brian Norris wrote:
> On Fri, Jan 6, 2023 at 5:23 PM Brian Norris  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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable"

[Brian Norris]

On Fri, Jan 6, 2023 at 5:23 PM Brian Norris  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

[PATCH v2 1/2] drm/atomic: Allow vblank-enabled + self-refresh "disable"

[Brian Norris]

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