Re: [PATCH] drm/i915: no longer call drm_helper_resume_force_mode

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 5 Sep 2012 13:04:54 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Wed, 5 Sep 2012 21:56:08 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > On Wed, Sep 5, 2012 at 8:31 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > On Wed, 29 Aug 2012 23:13:29 +0200
> > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > >
> > >> Since this only calls crtc helper functions, of which a shocking
> > >> amount are NULL.
> > >>
> > >> Now the curious thing is how the new modeset code worked with this
> > >> function call still present:
> > >>
> > >> Thanks to the hw state readout and the suspend fixes to properly
> > >> quiescent the register state, nothing is actually enabled at resume
> > >> (if the bios doesn't set up anything). Which means resume_force_mode
> > >> doesn't actually do anything and hence nothing blows up at resume
> > >> time.
> > >>
> > >> The other reason things do work is that the fbcon layer has it's own
> > >> resume notifier callback, which restores the mode. And thanks to the
> > >> force vt switch at suspend/resume, that then forces X to restore it's
> > >> own mode.
> > >>
> > >> Hence everything still worked (as long as the bios doesn't enable
> > >> anything). And we can just kill the call to resume_force_mode.
> > >>
> > >> The upside of both this patch and the preceeding patch to quiescent
> > >> the modeset state is that our resume path is much simpler:
> > >> - We now longer restore bogus register values (which most often would
> > >>   enable the backlight a bit and a few ports), causing flickering.
> > >> - We now longer call resume_force_mode to restore a mode that the
> > >>   fbcon layer would overwrite right away anyway.
> > >>
> > >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >> ---
> > >>  drivers/gpu/drm/i915/i915_drv.c | 5 -----
> > >>  1 file changed, 5 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > >> index fe7512a..cd6697c 100644
> > >> --- a/drivers/gpu/drm/i915/i915_drv.c
> > >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> > >> @@ -549,11 +549,6 @@ static int i915_drm_thaw(struct drm_device *dev)
> > >>               intel_modeset_setup_hw_state(dev);
> > >>               drm_mode_config_reset(dev);
> > >>               drm_irq_install(dev);
> > >> -
> > >> -             /* Resume the modeset for every activated CRTC */
> > >> -             mutex_lock(&dev->mode_config.mutex);
> > >> -             drm_helper_resume_force_mode(dev);
> > >> -             mutex_unlock(&dev->mode_config.mutex);
> > >>       }
> > >>
> > >>       intel_opregion_init(dev);
> > >
> > > Wouldn't the fb layer's modeset end up being a no-op if the suspended
> > > mode was the same as the fb mode (often the case)?  Or at the very
> > > least just a flip rather than a full mode set.
> > 
> > I guess most of the flicker was because the register restoring
> > restored a bunch of crap (since the old modeset state wasn't properly
> > cleared before suspending).
> > 
> > > Though we do need to deal with non-fb, non-X resumes as well.  kmscon
> > > and wayland will expect to be restored at resume time even if CONFIG_VT
> > > and the fb layer aren't compiled into the kernel.
> > 
> > Tbh I was rather surprised that when I've noticed this little issue
> > here the restore still worked - until I've noticed by looking at the
> > logs that both the fbcon and the X server restore their modes.
> > 
> > I'm not sure what exactly we should do here, since even with the
> > current code the concept of a controlling node isn't really defined in
> > the kms interface (fbcon uses a bunch of funky checks to ensure it
> > doesn't clobber the output state of someone else). But for now (with
> > fbcon pretty much being non-optional) things keep on working, and
> > afaict actually work a bit better overall.
> 
> It's probably ok for now, but at some point we'll want some code that
> restores the suspend mode if fbcon isn't enabled...
> 

So acked/reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

But note that if someone complains we'll need to add this back...

-- 
Jesse Barnes, Intel Open Source Technology Center