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

[Sean Paul]

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