[RFC] Suspend/resume without VT switches

[RFC] Suspend/resume without VT switches

[Jesse Barnes]

I've lightly tested this with X and it definitely makes my
suspend/resume sequence a bit prettier.  It should speed things up
trivally as well, but most of those gains come from other changes to the
i915 driver (posted earlier to intel-gfx).

Any thoughts?  I suspect we'll have to be more defensive about the
resume configuration in case the BIOS did something weird, but overall I
think we should be able to do this one way or another.

Thanks,
Jesse

[PATCH 1/2] PM: make VT switching to the suspend console optional

[Jesse Barnes]

KMS drivers can potentially restore the display configuration without
userspace help.  Such drivers can set a new global, pm_vt_switch, to
false if they support this feature.  In that case, the PM layer won't VT
switch to the suspend console at suspend time and then back to the
original VT on resume, but rather leave things alone for a nicer looking
suspend and resume sequence.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 include/linux/pm.h     |    3 +++
 kernel/power/console.c |    7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 007e687..d8e7efb 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -35,6 +35,9 @@ extern void (*pm_idle)(void);
 extern void (*pm_power_off)(void);
 extern void (*pm_power_off_prepare)(void);
 
+/* VT switch to the suspend console or not */
+extern bool pm_vt_switch; /* defaults to true in console.c */
+
 /*
  * Device power management
  */
diff --git a/kernel/power/console.c b/kernel/power/console.c
index b1dc456..65376b4 100644
--- a/kernel/power/console.c
+++ b/kernel/power/console.c
@@ -13,9 +13,13 @@
 #define SUSPEND_CONSOLE	(MAX_NR_CONSOLES-1)
 
 static int orig_fgconsole, orig_kmsg;
+bool pm_vt_switch = true;
 
 int pm_prepare_console(void)
 {
+	if (!pm_vt_switch)
+		return 0;
+
 	orig_fgconsole = vt_move_to_console(SUSPEND_CONSOLE, 1);
 	if (orig_fgconsole < 0)
 		return 1;
@@ -26,6 +30,9 @@ int pm_prepare_console(void)
 
 void pm_restore_console(void)
 {
+	if (!pm_vt_switch)
+		return;
+
 	if (orig_fgconsole >= 0) {
 		vt_move_to_console(orig_fgconsole, 0);
 		vt_kmsg_redirect(orig_kmsg);
-- 
1.7.9.5

Re: [PATCH 1/2] PM: make VT switching to the suspend console optional

[Alan Cox]

On Fri,  2 Nov 2012 14:43:40 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> KMS drivers can potentially restore the display configuration without
> userspace help.  Such drivers can set a new global, pm_vt_switch, to
> false if they support this feature.  In that case, the PM layer won't VT
> switch to the suspend console at suspend time and then back to the
> original VT on resume, but rather leave things alone for a nicer looking
> suspend and resume sequence.

What if you are multi-head ? What are the locking rules for a suspend/kms
module unload race, what happens when you load/unload and hand over
multiple frame buffers ? What if you have vts split across two adapters ?

Put me down as 100% in favour of the feature but we need to be a bit more
careful about the implementation. The logic probably needs to be in the
vt layer.

I suspect we actually need a per vt flag for this, or a flag on the
underlying object below the vt somewhere.

So NAK for the implementation ACK for the idea.

Alan

Re: [PATCH 1/2] PM: make VT switching to the suspend console optional

[Jesse Barnes]

On 11/2/2012 4:43 PM, Alan Cox wrote:
> On Fri,  2 Nov 2012 14:43:40 -0700
> Jesse Barnes<jbarnes@virtuousgeek.org>  wrote:
>
>> KMS drivers can potentially restore the display configuration without
>> userspace help.  Such drivers can set a new global, pm_vt_switch, to
>> false if they support this feature.  In that case, the PM layer won't VT
>> switch to the suspend console at suspend time and then back to the
>> original VT on resume, but rather leave things alone for a nicer looking
>> suspend and resume sequence.
>
> What if you are multi-head ? What are the locking rules for a suspend/kms
> module unload race, what happens when you load/unload and hand over
> multiple frame buffers ? What if you have vts split across two adapters ?
>
> Put me down as 100% in favour of the feature but we need to be a bit more
> careful about the implementation. The logic probably needs to be in the
> vt layer.
>
> I suspect we actually need a per vt flag for this, or a flag on the
> underlying object below the vt somewhere.
>
> So NAK for the implementation ACK for the idea.

Yeah good points, I didn't consider multi-head/VT split configurations 
at all obviously.  We can probably stuff something into the VT layer for 
that, but how would I even configure a VT split across two adapters 
today?  For vgacon we just route VGA to a single adapter, but I'm not 
sure how that works for fbcon.  Does it properly support multihead?  I 
thought not...

Dunno about suspend vs unload, how do we deal that in other drivers like 
the disk driver for suspend for example?  Overall that case seems pretty 
esoteric...

What do you mean about hand over to multiple frame buffers?

Jesse

Re: [PATCH 1/2] PM: make VT switching to the suspend console optional

[Alan Cox]

> that, but how would I even configure a VT split across two adapters 
> today?  For vgacon we just route VGA to a single adapter, but I'm not 

con2fb /dev/fb1 /dev/tty1

> Dunno about suspend vs unload, how do we deal that in other drivers like 
> the disk driver for suspend for example?  Overall that case seems pretty 
> esoteric...
> 
> What do you mean about hand over to multiple frame buffers?

You have a global but I can insmod i915 move the consoles off it and
unload it (at least in theory - last time I tried it crashed at
least on gma500 which I need to fix 8))

So you've got a global you can't just set back but need to adjust on
unload.

And you've got races like suspend as we are changing framebuffer which
your code doesn't consider as you have no locking.

If we push the logic into the vt layer we can pretty easily dump it under
the vt locks. It's not the whole story as there are all sorts of things
it doesn't handle but it does mean we can handle the case of

"if we are switching from a vt which is on a device that doesn't need it
for suspend then do nothing"

properly, and we can make any future features work right


I think all we need is consw to have a con_sw_suspend/con_sw_resume
method and the framebuffer layer to let kms get at it.

Alan

Re: [PATCH 1/2] PM: make VT switching to the suspend console optional

[Jesse Barnes]

On 11/2/2012 5:42 PM, Alan Cox wrote:
>> that, but how would I even configure a VT split across two adapters
>> today?  For vgacon we just route VGA to a single adapter, but I'm not
>
> con2fb /dev/fb1 /dev/tty1
>
>> Dunno about suspend vs unload, how do we deal that in other drivers like
>> the disk driver for suspend for example?  Overall that case seems pretty
>> esoteric...
>>
>> What do you mean about hand over to multiple frame buffers?
>
> You have a global but I can insmod i915 move the consoles off it and
> unload it (at least in theory - last time I tried it crashed at
> least on gma500 which I need to fix 8))

i915 doesn't crash in that case, but you definitely don't get the 
console back as we don't restore all the VGA state.  Easy enough to 
restore the global at least though.

>
> So you've got a global you can't just set back but need to adjust on
> unload.
>
> And you've got races like suspend as we are changing framebuffer which
> your code doesn't consider as you have no locking.
>
> If we push the logic into the vt layer we can pretty easily dump it under
> the vt locks. It's not the whole story as there are all sorts of things
> it doesn't handle but it does mean we can handle the case of
>
> "if we are switching from a vt which is on a device that doesn't need it
> for suspend then do nothing"
>
> properly, and we can make any future features work right
>
>
> I think all we need is consw to have a con_sw_suspend/con_sw_resume
> method and the framebuffer layer to let kms get at it.

yay console layer... ok I'll check it out.

Overall it's probably worth some grotting around in the console layer. 
Avoiding a VT switch makes suspend/resume a lot nicer looking.  No more 
blinking cursor in the corner and ugly flickering.

Jesse

[PATCH 2/2] drm/i915: support resume without VT switch

[Jesse Barnes]

Add support for resuming the suspend configuration at resume time to
avoid slow and ugly VT switches during suspend and resume.  Also emit a
hotplug event at resume time to make sure any potential configuration
changes (monitors coming and going, dock events) are handled properly.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_dma.c      |    3 +++
 drivers/gpu/drm/i915/i915_drv.c      |   28 +++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |   25 +++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 622f910..0bc63d2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1348,6 +1348,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* We're off and running w/KMS */
 	dev_priv->mm.suspended = 0;
 
+	/* This driver doesn't need a VT switch to restore the mode */
+	pm_vt_switch = false;
+
 	return 0;
 
 cleanup_irq:
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 26a0165..e7711ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -472,8 +472,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 		cancel_delayed_work_sync(&dev_priv->gen6_power_work);
 
-		intel_modeset_disable(dev);
-
 		drm_irq_uninstall(dev);
 	}
 
@@ -533,6 +531,24 @@ void intel_console_resume(struct work_struct *work)
 	console_unlock();
 }
 
+static void intel_resume_hotplug(struct drm_device *dev)
+{
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *encoder;
+
+	mutex_lock(&mode_config->mutex);
+	DRM_DEBUG_KMS("running encoder hotplug functions\n");
+
+	list_for_each_entry(encoder, &mode_config->encoder_list, base.head)
+		if (encoder->hot_plug)
+			encoder->hot_plug(encoder);
+
+	mutex_unlock(&mode_config->mutex);
+
+	/* Just fire off a uevent and let userspace tell us what to do */
+	drm_helper_hpd_irq_event(dev);
+}
+
 static int __i915_drm_thaw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -553,8 +569,14 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 
 		intel_modeset_init_hw(dev);
-		intel_modeset_setup_hw_state(dev);
 		drm_irq_install(dev);
+
+		/* Resume the modeset for every activated CRTC */
+		mutex_lock(&dev->mode_config.mutex);
+		intel_resume_force_mode(dev);
+		mutex_unlock(&dev->mode_config.mutex);
+
+		intel_resume_hotplug(dev);
 	}
 
 	intel_opregion_init(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index df65e48..3dc4e98 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1627,6 +1627,7 @@ extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void intel_detect_pch(struct drm_device *dev);
 extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
 extern int intel_enable_rc6(const struct drm_device *dev);
+extern int intel_resume_force_mode(struct drm_device *dev);
 
 extern bool i915_semaphore_is_enabled(struct drm_device *dev);
 int i915_reg_read_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4eb84ad..bcf7872 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8996,6 +8996,31 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	drm_mode_config_cleanup(dev);
 }
 
+int intel_resume_force_mode(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
+	struct drm_encoder_helper_funcs *encoder_funcs;
+	struct drm_crtc_helper_funcs *crtc_funcs;
+	int ret;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+
+		if (!crtc->enabled) {
+			DRM_ERROR("skipping disabled crtc\n");
+			continue;
+		}
+
+		ret = intel_set_mode(crtc, &crtc->mode, crtc->x, crtc->y,
+				     crtc->fb);
+
+		if (ret == false)
+			DRM_ERROR("failed to set mode on crtc %p\n", crtc);
+	}
+
+	return 0;
+}
+
 /*
  * Return which encoder is currently attached for connector.
  */
-- 
1.7.9.5

Re: [RFC] Suspend/resume without VT switches

[Rafael J. Wysocki]

On Friday, November 02, 2012 02:43:39 PM Jesse Barnes wrote:
> I've lightly tested this with X and it definitely makes my
> suspend/resume sequence a bit prettier.  It should speed things up
> trivally as well, but most of those gains come from other changes to the
> i915 driver (posted earlier to intel-gfx).
> 
> Any thoughts?

I like the idea.

> I suspect we'll have to be more defensive about the
> resume configuration in case the BIOS did something weird, but overall I
> think we should be able to do this one way or another.

Perhaps patch [1/2] should be [2/2] and vice versa? :-)

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC] Suspend/resume without VT switches

[Jesse Barnes]

On Fri, 02 Nov 2012 22:51:07 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Friday, November 02, 2012 02:43:39 PM Jesse Barnes wrote:
> > I've lightly tested this with X and it definitely makes my
> > suspend/resume sequence a bit prettier.  It should speed things up
> > trivally as well, but most of those gains come from other changes to the
> > i915 driver (posted earlier to intel-gfx).
> > 
> > Any thoughts?
> 
> I like the idea.
> 
> > I suspect we'll have to be more defensive about the
> > resume configuration in case the BIOS did something weird, but overall I
> > think we should be able to do this one way or another.
> 
> Perhaps patch [1/2] should be [2/2] and vice versa? :-)

But then it wouldn't compile?  I added the variable first, defaulting
to the current behavior, then made i915 support it and set the variable
to false there...  At least, that's what I intended to do.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [RFC] Suspend/resume without VT switches

[Rafael J. Wysocki]

On Friday, November 02, 2012 04:29:37 PM Jesse Barnes wrote:
> On Fri, 02 Nov 2012 22:51:07 +0100
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Friday, November 02, 2012 02:43:39 PM Jesse Barnes wrote:
> > > I've lightly tested this with X and it definitely makes my
> > > suspend/resume sequence a bit prettier.  It should speed things up
> > > trivally as well, but most of those gains come from other changes to the
> > > i915 driver (posted earlier to intel-gfx).
> > > 
> > > Any thoughts?
> > 
> > I like the idea.
> > 
> > > I suspect we'll have to be more defensive about the
> > > resume configuration in case the BIOS did something weird, but overall I
> > > think we should be able to do this one way or another.
> > 
> > Perhaps patch [1/2] should be [2/2] and vice versa? :-)
> 
> But then it wouldn't compile?  I added the variable first, defaulting
> to the current behavior, then made i915 support it and set the variable
> to false there...  At least, that's what I intended to do.

OK

So what happens if there are multiple graphics adapters in the system?
Including such that aren't handled by i915?  pm_vt_switch is global, so isn't
there any problem with that?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC] Suspend/resume without VT switches

[Jesse Barnes]

On 11/2/2012 4:38 PM, Rafael J. Wysocki wrote:
> On Friday, November 02, 2012 04:29:37 PM Jesse Barnes wrote:
>> On Fri, 02 Nov 2012 22:51:07 +0100
>> "Rafael J. Wysocki"<rjw@sisk.pl>  wrote:
>>
>>> On Friday, November 02, 2012 02:43:39 PM Jesse Barnes wrote:
>>>> I've lightly tested this with X and it definitely makes my
>>>> suspend/resume sequence a bit prettier.  It should speed things up
>>>> trivally as well, but most of those gains come from other changes to the
>>>> i915 driver (posted earlier to intel-gfx).
>>>>
>>>> Any thoughts?
>>>
>>> I like the idea.
>>>
>>>> I suspect we'll have to be more defensive about the
>>>> resume configuration in case the BIOS did something weird, but overall I
>>>> think we should be able to do this one way or another.
>>>
>>> Perhaps patch [1/2] should be [2/2] and vice versa? :-)
>>
>> But then it wouldn't compile?  I added the variable first, defaulting
>> to the current behavior, then made i915 support it and set the variable
>> to false there...  At least, that's what I intended to do.
>
> OK
>
> So what happens if there are multiple graphics adapters in the system?
> Including such that aren't handled by i915?  pm_vt_switch is global, so isn't
> there any problem with that?

Yeah I guess it depends on configuration.  Maybe only the driver that's 
currently driving the VT should be able to set this, then clear it if 
it's no longer in charge?  Not sure how to do that yet (ugg means I get 
to dig into the console layer again...)

Thanks,
Jesse