[PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

If a non-KMS graphics driver is used, the kernel carries out a VT
switch during suspend/resume to avoid possible problems with freezing
X at a wrong time and to cause X to repaint everything after resume.

This is not necessary any more if the KMS is used, so skip the kernel
VT switch during suspend/resume for i915 in the KMS mode.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---

Hi,

I've been testing this patch for over a week and haven't seen a single problem
related to it during this time.

Are there any objections to it?

Rafael

---
 drivers/gpu/drm/i915/i915_drv.c |    4 ++++
 include/linux/suspend.h         |    9 +++++++++
 kernel/power/suspend.c          |   16 +++++++++++++---
 3 files changed, 26 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/suspend.h
===================================================================
--- linux-2.6.orig/include/linux/suspend.h
+++ linux-2.6/include/linux/suspend.h
@@ -116,6 +116,13 @@ struct platform_suspend_ops {
 };
 
 #ifdef CONFIG_SUSPEND
+extern bool vt_switch_enabled;
+
+static inline void disable_suspend_vt_switch(void)
+{
+	vt_switch_enabled = false;
+}
+
 /**
  * suspend_set_ops - set platform dependent suspend operations
  * @ops: The new suspend operations to set.
@@ -143,6 +150,8 @@ extern void arch_suspend_enable_irqs(voi
 
 extern int pm_suspend(suspend_state_t state);
 #else /* !CONFIG_SUSPEND */
+static inline void disable_suspend_vt_switch(void) {}
+
 #define suspend_valid_only_mem	NULL
 
 static inline void suspend_set_ops(struct platform_suspend_ops *ops) {}
Index: linux-2.6/kernel/power/suspend.c
===================================================================
--- linux-2.6.orig/kernel/power/suspend.c
+++ linux-2.6/kernel/power/suspend.c
@@ -18,6 +18,13 @@
 
 #include "power.h"
 
+/*
+ * If set, switch to a new VT before suspend and switch back to the original
+ * one during resume.
+ */
+bool vt_switch_enabled = true;
+EXPORT_SYMBOL_GPL(vt_switch_enabled);
+
 const char *const pm_states[PM_SUSPEND_MAX] = {
 	[PM_SUSPEND_STANDBY]	= "standby",
 	[PM_SUSPEND_MEM]	= "mem",
@@ -82,7 +89,8 @@ static int suspend_prepare(void)
 	if (!suspend_ops || !suspend_ops->enter)
 		return -EPERM;
 
-	pm_prepare_console();
+	if (vt_switch_enabled)
+		pm_prepare_console();
 
 	error = pm_notifier_call_chain(PM_SUSPEND_PREPARE);
 	if (error)
@@ -100,7 +108,8 @@ static int suspend_prepare(void)
 	usermodehelper_enable();
  Finish:
 	pm_notifier_call_chain(PM_POST_SUSPEND);
-	pm_restore_console();
+	if (vt_switch_enabled)
+		pm_restore_console();
 	return error;
 }
 
@@ -238,7 +247,8 @@ static void suspend_finish(void)
 	suspend_thaw_processes();
 	usermodehelper_enable();
 	pm_notifier_call_chain(PM_POST_SUSPEND);
-	pm_restore_console();
+	if (vt_switch_enabled)
+		pm_restore_console();
 }
 
 /**
Index: linux-2.6/drivers/gpu/drm/i915/i915_drv.c
===================================================================
--- linux-2.6.orig/drivers/gpu/drm/i915/i915_drv.c
+++ linux-2.6/drivers/gpu/drm/i915/i915_drv.c
@@ -28,6 +28,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/suspend.h>
 #include "drmP.h"
 #include "drm.h"
 #include "i915_drm.h"
@@ -549,6 +550,9 @@ static int __init i915_init(void)
 		driver.driver_features &= ~DRIVER_MODESET;
 #endif
 
+	if (driver.driver_features & DRIVER_MODESET)
+		disable_suspend_vt_switch();
+
 	return drm_init(&driver);
 }
 

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Alan Cox]

> I've been testing this patch for over a week and haven't seen a single problem
> related to it during this time.
> 
> Are there any objections to it?

Usual question 8) - explain the locking. What happens if we suspend as
kms is initialising/being removed.

Also what happens if you have KMS and non KMS consoles both active
through the frame buffer off different cards ?

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Rafael J. Wysocki]

On Sunday 24 January 2010, Alan Cox wrote:
> > I've been testing this patch for over a week and haven't seen a single problem
> > related to it during this time.
> > 
> > Are there any objections to it?
> 
> Usual question 8) - explain the locking. What happens if we suspend as
> kms is initialising/being removed.

Nothing extremely interesting AFAICS.  There's no X yet or it's already exited,
so we don't need a VT switch anyway.

> Also what happens if you have KMS and non KMS consoles both active
> through the frame buffer off different cards ?

That's more interesting.  In fact I didn't take that into consideration. :-)

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

On Sun, 24 Jan 2010 00:55:59 +0100, "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> If a non-KMS graphics driver is used, the kernel carries out a VT
> switch during suspend/resume to avoid possible problems with freezing
> X at a wrong time and to cause X to repaint everything after resume.
> 
> This is not necessary any more if the KMS is used, so skip the kernel
> VT switch during suspend/resume for i915 in the KMS mode.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> 
> Hi,
> 
> I've been testing this patch for over a week and haven't seen a single problem
> related to it during this time.
> 
> Are there any objections to it?
> 
> Rafael

Love it!

Acked-by: Eric Anholt <eric@anholt.net>

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Jesse Barnes]

On Sun, 24 Jan 2010 00:55:59 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> If a non-KMS graphics driver is used, the kernel carries out a VT
> switch during suspend/resume to avoid possible problems with freezing
> X at a wrong time and to cause X to repaint everything after resume.
> 
> This is not necessary any more if the KMS is used, so skip the kernel
> VT switch during suspend/resume for i915 in the KMS mode.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> 
> Hi,
> 
> I've been testing this patch for over a week and haven't seen a
> single problem related to it during this time.
> 
> Are there any objections to it?

This probably belongs in the core DRM KMS code instead of the driver.
I can imagine that there may be some X driver code that still needs to
be executed on suspend/resume for some drivers, so this may introduce
bugs, but it's definitely the right way to go.

Dave?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Alan Cox]

> This probably belongs in the core DRM KMS code instead of the driver.
> I can imagine that there may be some X driver code that still needs to
> be executed on suspend/resume for some drivers, so this may introduce
> bugs, but it's definitely the right way to go.

You can have a mix of KMS and non KMS consoles active on different cards.
So I don't think that is the case.

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Jesse Barnes]

On Mon, 25 Jan 2010 18:43:50 +0000
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > This probably belongs in the core DRM KMS code instead of the
> > driver. I can imagine that there may be some X driver code that
> > still needs to be executed on suspend/resume for some drivers, so
> > this may introduce bugs, but it's definitely the right way to go.
> 
> You can have a mix of KMS and non KMS consoles active on different
> cards. So I don't think that is the case.

I pity users with that configuration.

But in that case we should be able to disable the VT switch disable
path; we just have to check each driver as it's loaded.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Rafael J. Wysocki]

On Monday 25 January 2010, Jesse Barnes wrote:
> On Mon, 25 Jan 2010 18:43:50 +0000
> Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> > > This probably belongs in the core DRM KMS code instead of the
> > > driver. I can imagine that there may be some X driver code that
> > > still needs to be executed on suspend/resume for some drivers, so
> > > this may introduce bugs, but it's definitely the right way to go.
> > 
> > You can have a mix of KMS and non KMS consoles active on different
> > cards. So I don't think that is the case.
> 
> I pity users with that configuration.
> 
> But in that case we should be able to disable the VT switch disable
> path; we just have to check each driver as it's loaded.

OK, what the right sequence of checks would be in that case and where to place
them?

Rafael

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Alan Cox]

> > But in that case we should be able to disable the VT switch disable
> > path; we just have to check each driver as it's loaded.
> 
> OK, what the right sequence of checks would be in that case and where to place
> them?

Why are we even driving a vt switch direct from the suspend/resume
logic ? The problem starts there. If it was being handled off the device
suspend/resume method then there wouldn't be a mess to start with ?

Start at the beginning

- Why do we switch to arbitarily chosen 'last vt'
- Why isn't vt related suspend/resume handled by the device

Alan

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Rafael J. Wysocki]

On Monday 25 January 2010, Alan Cox wrote:
> > > But in that case we should be able to disable the VT switch disable
> > > path; we just have to check each driver as it's loaded.
> > 
> > OK, what the right sequence of checks would be in that case and where to place
> > them?
> 
> Why are we even driving a vt switch direct from the suspend/resume
> logic ? The problem starts there. If it was being handled off the device
> suspend/resume method then there wouldn't be a mess to start with ?
> 
> Start at the beginning
> 
> - Why do we switch to arbitarily chosen 'last vt'
> - Why isn't vt related suspend/resume handled by the device

Well, that was added long ago as a workaround for some problems people
reported (presumably).  I've never looked at that before, so I can't really
tell why someone did it this particular way.

Obviously I'd like to clean it up, though.

So, what device should handle this in your opinion?

Rafael

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Alan Cox]

> Obviously I'd like to clean it up, though.
> 
> So, what device should handle this in your opinion?

My guess would be the relevant device driver for the hardware layer

So fbcon would probably not switch because it can put video modes back,
KMS obvious likewise. The text console might do something as part of its
suspend/resume path IFF the vt in question is in graphics mode.

Alan

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Pavel Machek]

On Mon 2010-01-25 22:54:37, Rafael J. Wysocki wrote:
> On Monday 25 January 2010, Alan Cox wrote:
> > > > But in that case we should be able to disable the VT switch disable
> > > > path; we just have to check each driver as it's loaded.
> > > 
> > > OK, what the right sequence of checks would be in that case and where to place
> > > them?
> > 
> > Why are we even driving a vt switch direct from the suspend/resume
> > logic ? The problem starts there. If it was being handled off the device
> > suspend/resume method then there wouldn't be a mess to start with ?
> > 
> > Start at the beginning
> > 
> > - Why do we switch to arbitarily chosen 'last vt'
> > - Why isn't vt related suspend/resume handled by the device
> 
> Well, that was added long ago as a workaround for some problems people
> reported (presumably).  I've never looked at that before, so I can't really
> tell why someone did it this particular way.

As X drives hardware, it is/was neccessary to get control out of X and
console switch was convenient.

Note that it needs to happen with userland still active -- before
freezer.

And yes, it should be per-driver these days.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Rafael J. Wysocki]

On Tuesday 26 January 2010, Pavel Machek wrote:
> On Mon 2010-01-25 22:54:37, Rafael J. Wysocki wrote:
> > On Monday 25 January 2010, Alan Cox wrote:
> > > > > But in that case we should be able to disable the VT switch disable
> > > > > path; we just have to check each driver as it's loaded.
> > > > 
> > > > OK, what the right sequence of checks would be in that case and where to place
> > > > them?
> > > 
> > > Why are we even driving a vt switch direct from the suspend/resume
> > > logic ? The problem starts there. If it was being handled off the device
> > > suspend/resume method then there wouldn't be a mess to start with ?
> > > 
> > > Start at the beginning
> > > 
> > > - Why do we switch to arbitarily chosen 'last vt'
> > > - Why isn't vt related suspend/resume handled by the device
> > 
> > Well, that was added long ago as a workaround for some problems people
> > reported (presumably).  I've never looked at that before, so I can't really
> > tell why someone did it this particular way.
> 
> As X drives hardware, it is/was neccessary to get control out of X and
> console switch was convenient.
> 
> Note that it needs to happen with userland still active -- before
> freezer.

Well, that's a bit cumbersome.

> And yes, it should be per-driver these days.

That would have to be done using suspend notifiers and should depend on what
driver actually controls the screen at the moment.  And I guess the only case
in which we actually _need_ to do the kernel VT switch is when the hardware
is controlled by X and without KMS.

Is there a simple way to determine if that's the case?

Rafael

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Pavel Machek]

On Tue 2010-01-26 19:47:45, Rafael J. Wysocki wrote:
> On Tuesday 26 January 2010, Pavel Machek wrote:
> > On Mon 2010-01-25 22:54:37, Rafael J. Wysocki wrote:
> > > On Monday 25 January 2010, Alan Cox wrote:
> > > > > > But in that case we should be able to disable the VT switch disable
> > > > > > path; we just have to check each driver as it's loaded.
> > > > > 
> > > > > OK, what the right sequence of checks would be in that case and where to place
> > > > > them?
> > > > 
> > > > Why are we even driving a vt switch direct from the suspend/resume
> > > > logic ? The problem starts there. If it was being handled off the device
> > > > suspend/resume method then there wouldn't be a mess to start with ?
> > > > 
> > > > Start at the beginning
> > > > 
> > > > - Why do we switch to arbitarily chosen 'last vt'
> > > > - Why isn't vt related suspend/resume handled by the device
> > > 
> > > Well, that was added long ago as a workaround for some problems people
> > > reported (presumably).  I've never looked at that before, so I can't really
> > > tell why someone did it this particular way.
> > 
> > As X drives hardware, it is/was neccessary to get control out of X and
> > console switch was convenient.
> > 
> > Note that it needs to happen with userland still active -- before
> > freezer.
> 
> Well, that's a bit cumbersome.

(which was one of reasons it got special casing).

> > And yes, it should be per-driver these days.
> 
> That would have to be done using suspend notifiers and should depend on what
> driver actually controls the screen at the moment.  And I guess the only case
> in which we actually _need_ to do the kernel VT switch is when the hardware
> is controlled by X and without KMS.

We need vt switch when display is controlled by userland app directly
accessing hw. It may or may not be X (svgalib anyone?,
gtk-on-framebuffer? qtopia?).

Ideally, userspace should explicitely tell us. KD_KERNEL_GRAPHICS
console mode?

Plus the switch is needed for any graphics app using fbcon -- I do not
think we actually save the framebuffer over suspend. (This one should
probably be fixed).
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[H. Peter Anvin]

On 01/26/2010 06:58 AM, Pavel Machek wrote:
>>
>> That would have to be done using suspend notifiers and should depend on what
>> driver actually controls the screen at the moment.  And I guess the only case
>> in which we actually _need_ to do the kernel VT switch is when the hardware
>> is controlled by X and without KMS.
> 
> We need vt switch when display is controlled by userland app directly
> accessing hw. It may or may not be X (svgalib anyone?,
> gtk-on-framebuffer? qtopia?).
> 
> Ideally, userspace should explicitely tell us. KD_KERNEL_GRAPHICS
> console mode?
> 

It seems that the kernel would already know if it's in control of the
mode switch, no?  If userspace ever takes control and it doesn't already
notify the kernel that it is taking over, we would seem to have a much
bigger problem...

> Plus the switch is needed for any graphics app using fbcon -- I do not
> think we actually save the framebuffer over suspend. (This one should
> probably be fixed).

Quite.

	-hpa

Re: [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Pavel Machek]

On Tue 2010-01-26 13:46:01, H. Peter Anvin wrote:
> On 01/26/2010 06:58 AM, Pavel Machek wrote:
> >>
> >> That would have to be done using suspend notifiers and should depend on what
> >> driver actually controls the screen at the moment.  And I guess the only case
> >> in which we actually _need_ to do the kernel VT switch is when the hardware
> >> is controlled by X and without KMS.
> > 
> > We need vt switch when display is controlled by userland app directly
> > accessing hw. It may or may not be X (svgalib anyone?,
> > gtk-on-framebuffer? qtopia?).
> > 
> > Ideally, userspace should explicitely tell us. KD_KERNEL_GRAPHICS
> > console mode?
> 
> It seems that the kernel would already know if it's in control of the
> mode switch, no? 

No, I do not think so. IIRC KD_GRAPHICS means "console is under
userland control"; X will use it even if it does not directly talk to
the hardware.

IOW kernel knows if userland *may* be in control of graphics hardware.

(And yes, not switching consoles when console is in KD_TEXT should be
easy and obvious optimalization).

Currently we have 

#define         KD_TEXT         0x00
#define         KD_GRAPHICS     0x01
#define         KD_TEXT0        0x02    /* obsolete */
#define         KD_TEXT1        0x03    /* obsolete */

I guess KD_KERNEL_GRAPHICS (or KD_INDIRECT_GRAPHICS or
KD_GRAPHICS_BUT_KERNEL_CAN_DO_CONSOLE_SWITCHING or something like
that) would be needed so that userland can indicate that no, cursor is
no longer welcome on the screen but no, it is not accessing hw
directly.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Stefan Seyfried]

Hi,

On Tue, 26 Jan 2010 15:58:43 +0100 Pavel Machek <pavel@ucw.cz> wrote:
> We need vt switch when display is controlled by userland app directly
> accessing hw. It may or may not be X (svgalib anyone?,
> gtk-on-framebuffer? qtopia?).

anything-on-framebuffer should not be different from plain framebuffer
console, or am I missing something?

> Ideally, userspace should explicitely tell us. KD_KERNEL_GRAPHICS
> console mode?
> 
> Plus the switch is needed for any graphics app using fbcon -- I do not
> think we actually save the framebuffer over suspend. (This one should
> probably be fixed).

Framebuffer should be easy to fix - it works pretty well already
because apparently the fbcon code needs to "shadow buffer" all VT
"windows" anyway - so maybe it's just the issue of doing an additional
"redraw()" somewhere appropriate.

The VGA consoles loses their content, because AFAICT they are in the
graphics card memory, which is not saved and restored.

	seife
-- 
Stefan Seyfried

"Any ideas, John?"
"Well, surrounding them's out."

Re: [linux-pm] [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Pavel Machek]

On Thu 2010-01-28 00:07:51, Stefan Seyfried wrote:
> Hi,
> 
> On Tue, 26 Jan 2010 15:58:43 +0100 Pavel Machek <pavel@ucw.cz> wrote:
> > We need vt switch when display is controlled by userland app directly
> > accessing hw. It may or may not be X (svgalib anyone?,
> > gtk-on-framebuffer? qtopia?).
> 
> anything-on-framebuffer should not be different from plain framebuffer
> console, or am I missing something?

It is. At least svgalib accesses hw directly. It probably can run even
on framebuffer.

> > Ideally, userspace should explicitely tell us. KD_KERNEL_GRAPHICS
> > console mode?
> > 
> > Plus the switch is needed for any graphics app using fbcon -- I do not
> > think we actually save the framebuffer over suspend. (This one should
> > probably be fixed).
> 
> Framebuffer should be easy to fix - it works pretty well already
> because apparently the fbcon code needs to "shadow buffer" all VT
> "windows" anyway - so maybe it's just the issue of doing an additional
> "redraw()" somewhere appropriate.

Well, for now the "shadow buffer" contains only text, not graphics
images. So you'd need to enlarge it a lot. Doable but more than one liner.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [PATCH] PM / i915: Skip kernel VT switch during suspend/resume if KMS is used

[Stefan Seyfried]

On Sun, 31 Jan 2010 09:54:19 +0100
Pavel Machek <pavel@ucw.cz> wrote:

> On Thu 2010-01-28 00:07:51, Stefan Seyfried wrote:
> > Hi,
> > 
> > On Tue, 26 Jan 2010 15:58:43 +0100 Pavel Machek <pavel@ucw.cz> wrote:
> > > We need vt switch when display is controlled by userland app directly
> > > accessing hw. It may or may not be X (svgalib anyone?,
> > > gtk-on-framebuffer? qtopia?).
> > 
> > anything-on-framebuffer should not be different from plain framebuffer
> > console, or am I missing something?
> 
> It is. At least svgalib accesses hw directly. It probably can run even
> on framebuffer.

If it accesses hw directly, it's not really "anything-on-framebuffer"
anymore, is it? The framebuffer device is there to abstract the
hardware.
 
> Well, for now the "shadow buffer" contains only text, not graphics
> images. So you'd need to enlarge it a lot. Doable but more than one liner.

Yes, noticed this today. I was using bootsplash-patched kernels, which
are obviously different in this aspect, so that shifted my view on
reality ;)
-- 
Stefan Seyfried

"Any ideas, John?"
"Well, surrounding them's out."