diff mbox

drm/i915: no longer call drm_helper_resume_force_mode

Message ID 1346274809-31155-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Aug. 29, 2012, 9:13 p.m. UTC
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(-)

Comments

Jesse Barnes Sept. 5, 2012, 6:31 p.m. UTC | #1
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.

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.
Daniel Vetter Sept. 5, 2012, 7:56 p.m. UTC | #2
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.
-Daniel
Jesse Barnes Sept. 5, 2012, 8:04 p.m. UTC | #3
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...
Jesse Barnes Sept. 6, 2012, 8:47 p.m. UTC | #4
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...
diff mbox

Patch

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);