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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jan 06, 2023 at 06:53:49PM +0100, Daniel Vetter wrote:
> On Thu, Jan 05, 2023 at 05:40:17PM -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 here.
> > 
> > Add a new exception, such that we allow CRTCs to be "disabled" (with
> > self-refresh active) with vblank interrupts still enabled.
> > 
> > Cc: <stable@vger.kernel.org> # dependency for subsequent patch
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > 
> >  drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index d579fd8f7cb8..7b5eddadebd5 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1207,6 +1207,12 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> >  
> >  		if (!drm_dev_has_vblank(dev))
> >  			continue;
> > +		/*
> > +		 * Self-refresh is not a true "disable"; let vblank remain
> > +		 * enabled.
> > +		 */
> > +		if (new_crtc_state->self_refresh_active)
> > +			continue;
> 
> This very fishy, because we check in crtc_needs_disable whether this
> output should stay on due to self-refresh. Which means you should never
> end up in here.
> 
> And yes vblank better work in self refresh :-) If it doesn't, then you
> need to fake it with a timer, that's at least what i915 has done for
> transparent self-refresh.
> 
> We might need a few more helpers. Also, probably more igt, or is this
> something igt testing has uncovered? If so, please cite the igt testcase
> which hits this.

Ok I think I was a bit slow here, and it makes sense. Except this now
means we loose this check, and I'm also not sure whether we really want
drivers to implement this all.

What I think we want here is a bit more:
- for the self-refresh case check that the vblank all still works

- check that drivers which use self_refresh are not using
  drm_atomic_helper_wait_for_vblanks(), because that would defeat the
  point

- have a drm_crtc_vblank_off/on which take the crtc state, so they can
  look at the self-refresh state

- fake vblanks with hrtimer, because on most hw when you turn off the crtc
  the vblanks are also turned off, and so your compositor would still
  hang. The vblank machinery already has all the code to make this happen
  (and if it's not all, then i915 psr code should have it).

- I think kunit tests for this all would be really good, it's a rather
  complex state machinery between modesets and vblank functionality. You
  can speed up the kunit tests with some really high refresh rate, which
  isn't possible on real hw.

I'm also wondering why we've had this code for years and only hit issues
now?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch