linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE
@ 2013-02-04  7:10 Zhang Rui
  2013-02-04  7:10 ` [PATCH V2 2/3] ACPI: enable ACPI SCI during suspend Zhang Rui
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Zhang Rui @ 2013-02-04  7:10 UTC (permalink / raw)
  To: linux-kernel, linux-pm, linux-acpi, intel-gfx; +Cc: rjw, lenb, Zhang Rui

PM_SUSPEND_FREEZE state is a general state that
does not need any platform specific support, it equals
frozen processes + suspended devices + idle processors.

Compared with PM_SUSPEND_MEMORY,
PM_SUSPEND_FREEZE saves less power
because the system is still in a running state.
PM_SUSPEND_FREEZE has less resume latency because it does not
touch BIOS, and the processors are in idle state.

Compared with RTPM/idle,
PM_SUSPEND_FREEZE saves more power as
1. the processor has longer sleep time because processes are frozen.
   The deeper c-state the processor supports, more power saving we can get.
2. PM_SUSPEND_FREEZE uses system suspend code path, thus we can get
   more power saving from the devices that does not have good RTPM support.

This state is useful for
1) platforms that do not have STR, or have a broken STR.
2) platforms that have an extremely low power idle state,
   which can be used to replace STR.

The following describes how PM_SUSPEND_FREEZE state works.
1. echo freeze > /sys/power/state
2. the processes are frozen.
3. all the devices are suspended.
4. all the processors are blocked by a wait queue
5. all the processors idles and enters (Deep) c-state.
6. an interrupt fires.
7. a processor is woken up and handles the irq.
8. if it is a general event,
   a) the irq handler runs and quites.
   b) goto step 4.
9. if it is a real wake event, say, power button pressing, keyboard touch, mouse moving,
   a) the irq handler runs and activate the wakeup source
   b) wakeup_source_activate() notifies the wait queue.
   c) system starts resuming from PM_SUSPEND_FREEZE
10. all the devices are resumed.
11. all the processes are unfrozen.
12. system is back to working.

Known Issue:
The wakeup of this new PM_SUSPEND_FREEZE state may behave differently
from the previous suspend state.
Take ACPI platform for example, there are some GPEs that only enabled
when the system is in sleep state, to wake the system backk from S3/S4.
But we are not touching these GPEs during transition to PM_SUSPEND_FREEZE.
This means we may lose some wake event.
But on the other hand, as we do not disable all the Interrupts during
PM_SUSPEND_FREEZE, we may get some extra "wakeup" Interrupts, that are
not available for S3/S4.

The patches has been tested on an old Sony laptop, and here are the results:

Average Power:
1. RPTM/idle for half an hour:
   14.8W, 12.6W, 14.1W, 12.5W, 14.4W, 13.2W, 12.9W
2. Freeze for half an hour:
   11W, 10.4W, 9.4W, 11.3W 10.5W
3. RTPM/idle for three hours:
   11.6W
4. Freeze for three hours:
   10W
5. Suspend to Memory:
   0.5~0.9W

Average Resume Latency:
1. RTPM/idle with a black screen: (From pressing keyboard to screen back)
   Less than 0.2s
2. Freeze: (From pressing power button to screen back)
   2.50s
3. Suspend to Memory: (From pressing power button to screen back)
   4.33s

>From the results, we can see that all the platforms should benefit from
this patch, even if it does not have Low Power S0.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/base/power/wakeup.c |    6 ++++
 include/linux/suspend.h     |    5 +++-
 kernel/power/main.c         |    2 +-
 kernel/power/suspend.c      |   69 +++++++++++++++++++++++++++++++++++--------
 4 files changed, 67 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index e6ee5e8..79715e7 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -382,6 +382,12 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 {
 	unsigned int cec;
 
+	/*
+	 * active wakeup source should bring the system
+	 * out of PM_SUSPEND_FREEZE state
+	 */
+	freeze_wake();
+
 	ws->active = true;
 	ws->active_count++;
 	ws->last_time = ktime_get();
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0c808d7..7420ab5 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -34,8 +34,10 @@ static inline void pm_restore_console(void)
 typedef int __bitwise suspend_state_t;
 
 #define PM_SUSPEND_ON		((__force suspend_state_t) 0)
-#define PM_SUSPEND_STANDBY	((__force suspend_state_t) 1)
+#define PM_SUSPEND_FREEZE	((__force suspend_state_t) 1)
+#define PM_SUSPEND_STANDBY	((__force suspend_state_t) 2)
 #define PM_SUSPEND_MEM		((__force suspend_state_t) 3)
+#define PM_SUSPEND_MIN		PM_SUSPEND_FREEZE
 #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
 
 enum suspend_stat_step {
@@ -192,6 +194,7 @@ struct platform_suspend_ops {
  */
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
+extern void freeze_wake(void);
 
 /**
  * arch_suspend_disable_irqs - disable IRQs for suspend
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 1c16f91..b1c26a9 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -313,7 +313,7 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
 static suspend_state_t decode_state(const char *buf, size_t n)
 {
 #ifdef CONFIG_SUSPEND
-	suspend_state_t state = PM_SUSPEND_STANDBY;
+	suspend_state_t state = PM_SUSPEND_MIN;
 	const char * const *s;
 #endif
 	char *p;
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index c8b7446..d4feda0 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -30,12 +30,38 @@
 #include "power.h"
 
 const char *const pm_states[PM_SUSPEND_MAX] = {
+	[PM_SUSPEND_FREEZE]	= "freeze",
 	[PM_SUSPEND_STANDBY]	= "standby",
 	[PM_SUSPEND_MEM]	= "mem",
 };
 
 static const struct platform_suspend_ops *suspend_ops;
 
+static bool need_suspend_ops(suspend_state_t state)
+{
+	return !!(state > PM_SUSPEND_FREEZE);
+}
+
+static DECLARE_WAIT_QUEUE_HEAD(suspend_freeze_wait_head);
+static bool suspend_freeze_wake;
+
+static void freeze_begin(void)
+{
+	suspend_freeze_wake = false;
+}
+
+static void freeze_enter(void)
+{
+	wait_event(suspend_freeze_wait_head, suspend_freeze_wake);
+}
+
+void freeze_wake(void)
+{
+	suspend_freeze_wake = true;
+	wake_up(&suspend_freeze_wait_head);
+}
+EXPORT_SYMBOL_GPL(freeze_wake);
+
 /**
  * suspend_set_ops - Set the global suspend method table.
  * @ops: Suspend operations to use.
@@ -50,8 +76,11 @@ EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 bool valid_state(suspend_state_t state)
 {
+	if (state == PM_SUSPEND_FREEZE)
+		return true;
 	/*
-	 * All states need lowlevel support and need to be valid to the lowlevel
+	 * PM_SUSPEND_STANDBY and PM_SUSPEND_MEMORY states need lowlevel
+	 * support and need to be valid to the lowlevel
 	 * implementation, no valid callback implies that none are valid.
 	 */
 	return suspend_ops && suspend_ops->valid && suspend_ops->valid(state);
@@ -89,11 +118,11 @@ static int suspend_test(int level)
  * hibernation).  Run suspend notifiers, allocate the "suspend" console and
  * freeze processes.
  */
-static int suspend_prepare(void)
+static int suspend_prepare(suspend_state_t state)
 {
 	int error;
 
-	if (!suspend_ops || !suspend_ops->enter)
+	if (need_suspend_ops(state) && (!suspend_ops || !suspend_ops->enter))
 		return -EPERM;
 
 	pm_prepare_console();
@@ -137,7 +166,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 {
 	int error;
 
-	if (suspend_ops->prepare) {
+	if (need_suspend_ops(state) && suspend_ops->prepare) {
 		error = suspend_ops->prepare();
 		if (error)
 			goto Platform_finish;
@@ -149,12 +178,23 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 		goto Platform_finish;
 	}
 
-	if (suspend_ops->prepare_late) {
+	if (need_suspend_ops(state) && suspend_ops->prepare_late) {
 		error = suspend_ops->prepare_late();
 		if (error)
 			goto Platform_wake;
 	}
 
+	/*
+	 * PM_SUSPEND_FREEZE equals
+	 * frozen processes + suspended devices + idle processors.
+	 * Thus we should invoke freeze_enter() soon after
+	 * all the devices are suspended.
+	 */
+	if (state == PM_SUSPEND_FREEZE) {
+		freeze_enter();
+		goto Platform_wake;
+	}
+
 	if (suspend_test(TEST_PLATFORM))
 		goto Platform_wake;
 
@@ -182,13 +222,13 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 	enable_nonboot_cpus();
 
  Platform_wake:
-	if (suspend_ops->wake)
+	if (need_suspend_ops(state) && suspend_ops->wake)
 		suspend_ops->wake();
 
 	dpm_resume_start(PMSG_RESUME);
 
  Platform_finish:
-	if (suspend_ops->finish)
+	if (need_suspend_ops(state) && suspend_ops->finish)
 		suspend_ops->finish();
 
 	return error;
@@ -203,11 +243,11 @@ int suspend_devices_and_enter(suspend_state_t state)
 	int error;
 	bool wakeup = false;
 
-	if (!suspend_ops)
+	if (need_suspend_ops(state) && !suspend_ops)
 		return -ENOSYS;
 
 	trace_machine_suspend(state);
-	if (suspend_ops->begin) {
+	if (need_suspend_ops(state) && suspend_ops->begin) {
 		error = suspend_ops->begin(state);
 		if (error)
 			goto Close;
@@ -226,7 +266,7 @@ int suspend_devices_and_enter(suspend_state_t state)
 
 	do {
 		error = suspend_enter(state, &wakeup);
-	} while (!error && !wakeup
+	} while (!error && !wakeup && need_suspend_ops(state)
 		&& suspend_ops->suspend_again && suspend_ops->suspend_again());
 
  Resume_devices:
@@ -236,13 +276,13 @@ int suspend_devices_and_enter(suspend_state_t state)
 	ftrace_start();
 	resume_console();
  Close:
-	if (suspend_ops->end)
+	if (need_suspend_ops(state) && suspend_ops->end)
 		suspend_ops->end();
 	trace_machine_suspend(PWR_EVENT_EXIT);
 	return error;
 
  Recover_platform:
-	if (suspend_ops->recover)
+	if (need_suspend_ops(state) && suspend_ops->recover)
 		suspend_ops->recover();
 	goto Resume_devices;
 }
@@ -278,12 +318,15 @@ static int enter_state(suspend_state_t state)
 	if (!mutex_trylock(&pm_mutex))
 		return -EBUSY;
 
+	if (state == PM_SUSPEND_FREEZE)
+		freeze_begin();
+
 	printk(KERN_INFO "PM: Syncing filesystems ... ");
 	sys_sync();
 	printk("done.\n");
 
 	pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]);
-	error = suspend_prepare();
+	error = suspend_prepare(state);
 	if (error)
 		goto Unlock;
 
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V2 2/3] ACPI: enable ACPI SCI during suspend
  2013-02-04  7:10 [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui
@ 2013-02-04  7:10 ` Zhang Rui
  2013-02-04  7:10 ` [PATCH V2 3/3] i915: ignore lid open event when resuming Zhang Rui
  2013-02-06  1:11 ` [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui
  2 siblings, 0 replies; 11+ messages in thread
From: Zhang Rui @ 2013-02-04  7:10 UTC (permalink / raw)
  To: linux-kernel, linux-pm, linux-acpi, intel-gfx; +Cc: rjw, lenb, Zhang Rui

Enable ACPI SCI during suspend so that SCI can be used
as wake events for PM_SUSPEND_FREEZE.

For S3/S4 transition,
We disable all GPEs in suspend_ops->prepare_late() to
fix a problem that GPEs may trigger SCI  before
arch_suspend_disable_irqs() is run.
So it is safe to leave the SCI enabled until
arch_suspend_irq_disable() is run.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/osl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3ff2678..3adeb10 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -787,7 +787,7 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
 
 	acpi_irq_handler = handler;
 	acpi_irq_context = context;
-	if (request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)) {
+	if (request_irq(irq, acpi_irq, IRQF_SHARED | IRQF_NO_SUSPEND, "acpi", acpi_irq)) {
 		printk(KERN_ERR PREFIX "SCI (IRQ%d) allocation failed\n", irq);
 		acpi_irq_handler = NULL;
 		return AE_NOT_ACQUIRED;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH V2 3/3] i915: ignore lid open event when resuming
  2013-02-04  7:10 [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui
  2013-02-04  7:10 ` [PATCH V2 2/3] ACPI: enable ACPI SCI during suspend Zhang Rui
@ 2013-02-04  7:10 ` Zhang Rui
  2013-02-04 15:25   ` [Intel-gfx] " Daniel Vetter
  2013-02-06  1:11 ` [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui
  2 siblings, 1 reply; 11+ messages in thread
From: Zhang Rui @ 2013-02-04  7:10 UTC (permalink / raw)
  To: linux-kernel, linux-pm, linux-acpi, intel-gfx; +Cc: rjw, lenb, Zhang Rui

i915 driver needs to do modeset when
1. system resumes from sleep
2. lid is opened

In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
thus it is the i915_resume code does the modeset rather than intel_lid_notify().

But in PM_SUSPEND_FREEZE state, this will be broken because
system is still responsive to the lid events.
1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
2. When we reopen the lid, intel_lid_notify() will do a modeset,
   before the system is resumed.
here is the error log,

[92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
[92146.548076] Hardware name: VGN-Z540N
[92146.548078] pipe_off wait timed out
[92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
[92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
[92146.548175] Call Trace:
[92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
[92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
[92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
[92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
[92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
[92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
[92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
[92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
[92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
[92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
[92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
[92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
[92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
[92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
[92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
[92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
[92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
[92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
[92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
[92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
[92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
[92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
[92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
[92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
[92146.548640]  [<c1056e84>] kthread+0x94/0xa0
[92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
[92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
[92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0

three different modeset flags are introduced in this patch
MODESET_ON_LID: do modeset on next lid open event
MODESET_DONE:  modeset already done
MODESET_ON_RESUME:  do modeset when system is resumed

In this way,
1. when lid is closed, MODESET_ON_LID is set so that
   we'll do modeset on next lid open event.
2. when lid is opened, MODESET_DONE is set
   so that duplicate lid open events will be ignored.
3. when system suspends, MODESET_ON_RESUME is set.
   In this case, we will not do modeset on any lid events.

Plus, locking mechanism is also introduced to avoid racing.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c   |    1 +
 drivers/gpu/drm/i915/i915_drv.c   |   14 +++++++++-----
 drivers/gpu/drm/i915/i915_drv.h   |   11 +++++++++--
 drivers/gpu/drm/i915/intel_lvds.c |   33 ++++++++++++++++++++-------------
 4 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 99daa89..c7cb546 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->dpio_lock);
 
 	mutex_init(&dev_priv->rps.hw_lock);
+	mutex_init(&dev_priv->modeset_lock);
 
 	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
 		dev_priv->num_pipe = 3;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1172658..bd7ab5b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/* ignore lid events during suspend */
+	mutex_lock(&dev_priv->modeset_lock);
+	dev_priv->modeset = MODESET_ON_RESUME;
+	mutex_unlock(&dev_priv->modeset_lock);
+
+
 	drm_kms_helper_poll_disable(dev);
 
 	pci_save_state(dev->pdev);
@@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	intel_opregion_fini(dev);
 
-	/* Modeset on resume, not lid events */
-	dev_priv->modeset_on_lid = 0;
-
 	console_lock();
 	intel_fbdev_set_suspend(dev, 1);
 	console_unlock();
@@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
 	intel_opregion_init(dev);
 
-	dev_priv->modeset_on_lid = 0;
-
 	/*
 	 * The console lock can be pretty contented on resume due
 	 * to all the printk activity.  Try to keep it out of the hot
@@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		schedule_work(&dev_priv->console_resume_work);
 	}
 
+	mutex_lock(&dev_priv->modeset_lock);
+	dev_priv->modeset = MODESET_DONE;
+	mutex_unlock(&dev_priv->modeset_lock);
 	return error;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 12ab3bd..1a57ae8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -620,6 +620,12 @@ struct intel_l3_parity {
 	struct work_struct error_work;
 };
 
+enum modeset_flag {
+	MODESET_ON_LID_OPEN,
+	MODESET_DONE,
+	MODESET_ON_RESUME,
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 
@@ -750,9 +756,10 @@ typedef struct drm_i915_private {
 
 	unsigned long quirks;
 
-	/* Register state */
-	bool modeset_on_lid;
+	enum modeset_flag modeset; 
+	struct mutex modeset_lock;
 
+	/* Register state */
 	struct {
 		/** Bridge to intel-gtt-ko */
 		struct intel_gtt *gtt;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 17aee74..4eb966a 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
 };
 
 /*
- * Lid events. Note the use of 'modeset_on_lid':
- *  - we set it on lid close, and reset it on open
+ * Lid events. Note the use of 'modeset':
+ *  - we set it to MODESET_ON_LID on lid close,
+ *    and set it to MODESET_DONE on open
  *  - we use it as a "only once" bit (ie we ignore
- *    duplicate events where it was already properly
- *    set/reset)
- *  - the suspend/resume paths will also set it to
- *    zero, since they restore the mode ("lid open").
+ *    duplicate events where it was already properly set)
+ *  - the suspend/resume paths will set it to
+ *    MODESET_ON_RESUME and ignore the lid open event,
+ *    because they restore the mode ("lid open").
  */
 static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 			    void *unused)
@@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
 		return NOTIFY_OK;
 
+	mutex_lock(&dev_priv->modeset_lock);
+	if (dev_priv->modeset == MODESET_ON_RESUME)
+		goto exit;
 	/*
 	 * check and update the status of LVDS connector after receiving
 	 * the LID nofication event.
@@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 
 	/* Don't force modeset on machines where it causes a GPU lockup */
 	if (dmi_check_system(intel_no_modeset_on_lid))
-		return NOTIFY_OK;
+		goto exit;
 	if (!acpi_lid_open()) {
-		dev_priv->modeset_on_lid = 1;
-		return NOTIFY_OK;
+		/* do modeset on next lid open event */
+		dev_priv->modeset = MODESET_ON_LID_OPEN;
+		goto exit;
 	}
 
-	if (!dev_priv->modeset_on_lid)
-		return NOTIFY_OK;
-
-	dev_priv->modeset_on_lid = 0;
+	if (dev_priv->modeset == MODESET_DONE)
+		goto exit;
 
 	mutex_lock(&dev->mode_config.mutex);
 	intel_modeset_setup_hw_state(dev, true);
 	mutex_unlock(&dev->mode_config.mutex);
 
+	dev_priv->modeset = MODESET_DONE;
+
+exit:
+	mutex_unlock(&dev_priv->modeset_lock);
 	return NOTIFY_OK;
 }
 
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
  2013-02-04  7:10 ` [PATCH V2 3/3] i915: ignore lid open event when resuming Zhang Rui
@ 2013-02-04 15:25   ` Daniel Vetter
  2013-02-04 23:58     ` Zhang Rui
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2013-02-04 15:25 UTC (permalink / raw)
  To: Zhang Rui; +Cc: linux-kernel, linux-pm, linux-acpi, intel-gfx, rjw, lenb

On Mon, Feb 04, 2013 at 03:10:11PM +0800, Zhang Rui wrote:
> i915 driver needs to do modeset when
> 1. system resumes from sleep
> 2. lid is opened
> 
> In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
> thus it is the i915_resume code does the modeset rather than intel_lid_notify().
> 
> But in PM_SUSPEND_FREEZE state, this will be broken because
> system is still responsive to the lid events.
> 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
> 2. When we reopen the lid, intel_lid_notify() will do a modeset,
>    before the system is resumed.
> here is the error log,
> 
> [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
> [92146.548076] Hardware name: VGN-Z540N
> [92146.548078] pipe_off wait timed out
> [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
> [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
> [92146.548175] Call Trace:
> [92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
> [92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> [92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> [92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
> [92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
> [92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
> [92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
> [92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
> [92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
> [92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
> [92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
> [92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
> [92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
> [92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
> [92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
> [92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
> [92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
> [92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
> [92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
> [92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
> [92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
> [92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
> [92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
> [92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
> [92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
> [92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
> [92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
> [92146.548640]  [<c1056e84>] kthread+0x94/0xa0
> [92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
> [92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
> [92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0
> 
> three different modeset flags are introduced in this patch
> MODESET_ON_LID: do modeset on next lid open event
> MODESET_DONE:  modeset already done
> MODESET_ON_RESUME:  do modeset when system is resumed
> 
> In this way,
> 1. when lid is closed, MODESET_ON_LID is set so that
>    we'll do modeset on next lid open event.
> 2. when lid is opened, MODESET_DONE is set
>    so that duplicate lid open events will be ignored.
> 3. when system suspends, MODESET_ON_RESUME is set.
>    In this case, we will not do modeset on any lid events.
> 
> Plus, locking mechanism is also introduced to avoid racing.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>

Looks nice, two tiny bikesheds below.

> ---
>  drivers/gpu/drm/i915/i915_dma.c   |    1 +
>  drivers/gpu/drm/i915/i915_drv.c   |   14 +++++++++-----
>  drivers/gpu/drm/i915/i915_drv.h   |   11 +++++++++--
>  drivers/gpu/drm/i915/intel_lvds.c |   33 ++++++++++++++++++++-------------
>  4 files changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 99daa89..c7cb546 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	spin_lock_init(&dev_priv->dpio_lock);
>  
>  	mutex_init(&dev_priv->rps.hw_lock);
> +	mutex_init(&dev_priv->modeset_lock);
>  
>  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
>  		dev_priv->num_pipe = 3;
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1172658..bd7ab5b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	/* ignore lid events during suspend */
> +	mutex_lock(&dev_priv->modeset_lock);
> +	dev_priv->modeset = MODESET_ON_RESUME;

I think calling this MODESET_SUSPENDED would make it clearer that we
have disable the modeset (and so lid restore) handling.

> +	mutex_unlock(&dev_priv->modeset_lock);
> +
> +
>  	drm_kms_helper_poll_disable(dev);
>  
>  	pci_save_state(dev->pdev);
> @@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	intel_opregion_fini(dev);
>  
> -	/* Modeset on resume, not lid events */
> -	dev_priv->modeset_on_lid = 0;
> -
>  	console_lock();
>  	intel_fbdev_set_suspend(dev, 1);
>  	console_unlock();
> @@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
>  
>  	intel_opregion_init(dev);
>  
> -	dev_priv->modeset_on_lid = 0;
> -
>  	/*
>  	 * The console lock can be pretty contented on resume due
>  	 * to all the printk activity.  Try to keep it out of the hot
> @@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
>  		schedule_work(&dev_priv->console_resume_work);
>  	}
>  
> +	mutex_lock(&dev_priv->modeset_lock);
> +	dev_priv->modeset = MODESET_DONE;
> +	mutex_unlock(&dev_priv->modeset_lock);
>  	return error;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 12ab3bd..1a57ae8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -620,6 +620,12 @@ struct intel_l3_parity {
>  	struct work_struct error_work;
>  };
>  
> +enum modeset_flag {
> +	MODESET_ON_LID_OPEN,
> +	MODESET_DONE,
> +	MODESET_ON_RESUME,
> +};
> +
>  typedef struct drm_i915_private {
>  	struct drm_device *dev;
>  
> @@ -750,9 +756,10 @@ typedef struct drm_i915_private {
>  
>  	unsigned long quirks;
>  
> -	/* Register state */
> -	bool modeset_on_lid;
> +	enum modeset_flag modeset; 
> +	struct mutex modeset_lock;

Using modeset/modeset_lock is imo a bit too generic, since this is all
about restoring the modeset configuration on lid open. What about

-	bool modeset_on_lid;
+	enum modeset_restore modeset_restore; 
+	struct mutex modeset_restore_lock;

instead?

Yours, Daniel

>  
> +	/* Register state */
>  	struct {
>  		/** Bridge to intel-gtt-ko */
>  		struct intel_gtt *gtt;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 17aee74..4eb966a 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
>  };
>  
>  /*
> - * Lid events. Note the use of 'modeset_on_lid':
> - *  - we set it on lid close, and reset it on open
> + * Lid events. Note the use of 'modeset':
> + *  - we set it to MODESET_ON_LID on lid close,
> + *    and set it to MODESET_DONE on open
>   *  - we use it as a "only once" bit (ie we ignore
> - *    duplicate events where it was already properly
> - *    set/reset)
> - *  - the suspend/resume paths will also set it to
> - *    zero, since they restore the mode ("lid open").
> + *    duplicate events where it was already properly set)
> + *  - the suspend/resume paths will set it to
> + *    MODESET_ON_RESUME and ignore the lid open event,
> + *    because they restore the mode ("lid open").
>   */
>  static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
>  			    void *unused)
> @@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
>  	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
>  		return NOTIFY_OK;
>  
> +	mutex_lock(&dev_priv->modeset_lock);
> +	if (dev_priv->modeset == MODESET_ON_RESUME)
> +		goto exit;
>  	/*
>  	 * check and update the status of LVDS connector after receiving
>  	 * the LID nofication event.
> @@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
>  
>  	/* Don't force modeset on machines where it causes a GPU lockup */
>  	if (dmi_check_system(intel_no_modeset_on_lid))
> -		return NOTIFY_OK;
> +		goto exit;
>  	if (!acpi_lid_open()) {
> -		dev_priv->modeset_on_lid = 1;
> -		return NOTIFY_OK;
> +		/* do modeset on next lid open event */
> +		dev_priv->modeset = MODESET_ON_LID_OPEN;
> +		goto exit;
>  	}
>  
> -	if (!dev_priv->modeset_on_lid)
> -		return NOTIFY_OK;
> -
> -	dev_priv->modeset_on_lid = 0;
> +	if (dev_priv->modeset == MODESET_DONE)
> +		goto exit;
>  
>  	mutex_lock(&dev->mode_config.mutex);
>  	intel_modeset_setup_hw_state(dev, true);
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> +	dev_priv->modeset = MODESET_DONE;
> +
> +exit:
> +	mutex_unlock(&dev_priv->modeset_lock);
>  	return NOTIFY_OK;
>  }
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
  2013-02-04 15:25   ` [Intel-gfx] " Daniel Vetter
@ 2013-02-04 23:58     ` Zhang Rui
  2013-02-05  7:41       ` Zhang Rui
  0 siblings, 1 reply; 11+ messages in thread
From: Zhang Rui @ 2013-02-04 23:58 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: linux-kernel, linux-pm, linux-acpi, intel-gfx, rjw, lenb

On Mon, 2013-02-04 at 16:25 +0100, Daniel Vetter wrote:
> On Mon, Feb 04, 2013 at 03:10:11PM +0800, Zhang Rui wrote:
> > i915 driver needs to do modeset when
> > 1. system resumes from sleep
> > 2. lid is opened
> > 
> > In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
> > thus it is the i915_resume code does the modeset rather than intel_lid_notify().
> > 
> > But in PM_SUSPEND_FREEZE state, this will be broken because
> > system is still responsive to the lid events.
> > 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
> > 2. When we reopen the lid, intel_lid_notify() will do a modeset,
> >    before the system is resumed.
> > here is the error log,
> > 
> > [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
> > [92146.548076] Hardware name: VGN-Z540N
> > [92146.548078] pipe_off wait timed out
> > [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
> > [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
> > [92146.548175] Call Trace:
> > [92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
> > [92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
> > [92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
> > [92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
> > [92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
> > [92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
> > [92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
> > [92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
> > [92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
> > [92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
> > [92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
> > [92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
> > [92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
> > [92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
> > [92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
> > [92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
> > [92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
> > [92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
> > [92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
> > [92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
> > [92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
> > [92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
> > [92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
> > [92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
> > [92146.548640]  [<c1056e84>] kthread+0x94/0xa0
> > [92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
> > [92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
> > [92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0
> > 
> > three different modeset flags are introduced in this patch
> > MODESET_ON_LID: do modeset on next lid open event
> > MODESET_DONE:  modeset already done
> > MODESET_ON_RESUME:  do modeset when system is resumed
> > 
> > In this way,
> > 1. when lid is closed, MODESET_ON_LID is set so that
> >    we'll do modeset on next lid open event.
> > 2. when lid is opened, MODESET_DONE is set
> >    so that duplicate lid open events will be ignored.
> > 3. when system suspends, MODESET_ON_RESUME is set.
> >    In this case, we will not do modeset on any lid events.
> > 
> > Plus, locking mechanism is also introduced to avoid racing.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> 
> Looks nice, two tiny bikesheds below.
> 
Great, thanks for reviewing. Refreshed patch attached.

>From 0d597b4859535c79ed545160cf4af9e6b5970e3c Mon Sep 17 00:00:00 2001
From: Zhang Rui <rui.zhang@intel.com>
Date: Thu, 24 Jan 2013 13:27:22 +0800
Subject: [PATCH V3 3/3] i915: ignore lid open event when resuming

i915 driver needs to do modeset when
1. system resumes from sleep
2. lid is opened

In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
thus it is the i915_resume code does the modeset rather than intel_lid_notify().

But in PM_SUSPEND_FREEZE state, this will be broken because
system is still responsive to the lid events.
1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
2. When we reopen the lid, intel_lid_notify() will do a modeset,
   before the system is resumed.
here is the error log,

[92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
[92146.548076] Hardware name: VGN-Z540N
[92146.548078] pipe_off wait timed out
[92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
[92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
[92146.548175] Call Trace:
[92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
[92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
[92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
[92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
[92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
[92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
[92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
[92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
[92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
[92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
[92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
[92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
[92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
[92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
[92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
[92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
[92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
[92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
[92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
[92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
[92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
[92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
[92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
[92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
[92146.548640]  [<c1056e84>] kthread+0x94/0xa0
[92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
[92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
[92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0

three different modeset flags are introduced in this patch
MODESET_ON_LID: do modeset on next lid open event
MODESET_DONE:  modeset already done
MODESET_ON_RESUME:  do modeset when system is resumed

In this way,
1. when lid is closed, MODESET_ON_LID is set so that
   we'll do modeset on next lid open event.
2. when lid is opened, MODESET_DONE is set
   so that duplicate lid open events will be ignored.
3. when system suspends, MODESET_ON_RESUME is set.
   In this case, we will not do modeset on any lid events.

Plus, locking mechanism is also introduced to avoid racing.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c   |    1 +
 drivers/gpu/drm/i915/i915_drv.c   |   14 +++++++++-----
 drivers/gpu/drm/i915/i915_drv.h   |   11 +++++++++--
 drivers/gpu/drm/i915/intel_lvds.c |   33 ++++++++++++++++++++-------------
 4 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 99daa89..a5115d8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->dpio_lock);
 
 	mutex_init(&dev_priv->rps.hw_lock);
+	mutex_init(&dev_priv->modeset_restore_lock);
 
 	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
 		dev_priv->num_pipe = 3;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1172658..68c6048 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/* ignore lid events during suspend */
+	mutex_lock(&dev_priv->modeset_restore_lock);
+	dev_priv->modeset_restore = MODESET_SUSPENDED;
+	mutex_unlock(&dev_priv->modeset_restore_lock);
+
+
 	drm_kms_helper_poll_disable(dev);
 
 	pci_save_state(dev->pdev);
@@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	intel_opregion_fini(dev);
 
-	/* Modeset on resume, not lid events */
-	dev_priv->modeset_on_lid = 0;
-
 	console_lock();
 	intel_fbdev_set_suspend(dev, 1);
 	console_unlock();
@@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
 	intel_opregion_init(dev);
 
-	dev_priv->modeset_on_lid = 0;
-
 	/*
 	 * The console lock can be pretty contented on resume due
 	 * to all the printk activity.  Try to keep it out of the hot
@@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		schedule_work(&dev_priv->console_resume_work);
 	}
 
+	mutex_lock(&dev_priv->modeset_restore_lock);
+	dev_priv->modeset_restore = MODESET_DONE;
+	mutex_unlock(&dev_priv->modeset_restore_lock);
 	return error;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 12ab3bd..0fddad01 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -620,6 +620,12 @@ struct intel_l3_parity {
 	struct work_struct error_work;
 };
 
+enum modeset_restore {
+	MODESET_ON_LID_OPEN,
+	MODESET_DONE,
+	MODESET_SUSPENDED,
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 
@@ -750,9 +756,10 @@ typedef struct drm_i915_private {
 
 	unsigned long quirks;
 
-	/* Register state */
-	bool modeset_on_lid;
+	enum modeset_restore modeset_restore; 
+	struct mutex modeset_restore_lock;
 
+	/* Register state */
 	struct {
 		/** Bridge to intel-gtt-ko */
 		struct intel_gtt *gtt;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 17aee74..2f55e9c 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
 };
 
 /*
- * Lid events. Note the use of 'modeset_on_lid':
- *  - we set it on lid close, and reset it on open
+ * Lid events. Note the use of 'modeset':
+ *  - we set it to MODESET_ON_LID on lid close,
+ *    and set it to MODESET_DONE on open
  *  - we use it as a "only once" bit (ie we ignore
- *    duplicate events where it was already properly
- *    set/reset)
- *  - the suspend/resume paths will also set it to
- *    zero, since they restore the mode ("lid open").
+ *    duplicate events where it was already properly set)
+ *  - the suspend/resume paths will set it to
+ *    MODESET_ON_RESUME and ignore the lid open event,
+ *    because they restore the mode ("lid open").
  */
 static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 			    void *unused)
@@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
 		return NOTIFY_OK;
 
+	mutex_lock(&dev_priv->modeset_restore_lock);
+	if (dev_priv->modeset_restore == MODESET_SUSPENDED)
+		goto exit;
 	/*
 	 * check and update the status of LVDS connector after receiving
 	 * the LID nofication event.
@@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 
 	/* Don't force modeset on machines where it causes a GPU lockup */
 	if (dmi_check_system(intel_no_modeset_on_lid))
-		return NOTIFY_OK;
+		goto exit;
 	if (!acpi_lid_open()) {
-		dev_priv->modeset_on_lid = 1;
-		return NOTIFY_OK;
+		/* do modeset on next lid open event */
+		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
+		goto exit;
 	}
 
-	if (!dev_priv->modeset_on_lid)
-		return NOTIFY_OK;
-
-	dev_priv->modeset_on_lid = 0;
+	if (dev_priv->modeset_restore == MODESET_DONE)
+		goto exit;
 
 	mutex_lock(&dev->mode_config.mutex);
 	intel_modeset_setup_hw_state(dev, true);
 	mutex_unlock(&dev->mode_config.mutex);
 
+	dev_priv->modeset_restore = MODESET_DONE;
+
+exit:
+	mutex_unlock(&dev_priv->modeset_restore_lock);
 	return NOTIFY_OK;
 }
 
-- 
1.7.9.5




^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
  2013-02-04 23:58     ` Zhang Rui
@ 2013-02-05  7:41       ` Zhang Rui
  2013-02-05 10:07         ` Daniel Vetter
  0 siblings, 1 reply; 11+ messages in thread
From: Zhang Rui @ 2013-02-05  7:41 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: linux-kernel, linux-pm, linux-acpi, intel-gfx, rjw, lenb

On Tue, 2013-02-05 at 07:58 +0800, Zhang Rui wrote:
> On Mon, 2013-02-04 at 16:25 +0100, Daniel Vetter wrote:
> > On Mon, Feb 04, 2013 at 03:10:11PM +0800, Zhang Rui wrote:
> > > i915 driver needs to do modeset when
> > > 1. system resumes from sleep
> > > 2. lid is opened
> > > 
> > > In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
> > > thus it is the i915_resume code does the modeset rather than intel_lid_notify().
> > > 
> > > But in PM_SUSPEND_FREEZE state, this will be broken because
> > > system is still responsive to the lid events.
> > > 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
> > > 2. When we reopen the lid, intel_lid_notify() will do a modeset,
> > >    before the system is resumed.
> > > here is the error log,
> > > 
> > > [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
> > > [92146.548076] Hardware name: VGN-Z540N
> > > [92146.548078] pipe_off wait timed out
> > > [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
> > > [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
> > > [92146.548175] Call Trace:
> > > [92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
> > > [92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > > [92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > > [92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
> > > [92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
> > > [92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
> > > [92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
> > > [92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
> > > [92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
> > > [92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
> > > [92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
> > > [92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
> > > [92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
> > > [92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
> > > [92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
> > > [92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
> > > [92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
> > > [92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
> > > [92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
> > > [92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
> > > [92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
> > > [92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
> > > [92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
> > > [92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
> > > [92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
> > > [92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
> > > [92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
> > > [92146.548640]  [<c1056e84>] kthread+0x94/0xa0
> > > [92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
> > > [92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
> > > [92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0
> > > 
> > > three different modeset flags are introduced in this patch
> > > MODESET_ON_LID: do modeset on next lid open event
> > > MODESET_DONE:  modeset already done
> > > MODESET_ON_RESUME:  do modeset when system is resumed
> > > 
> > > In this way,
> > > 1. when lid is closed, MODESET_ON_LID is set so that
> > >    we'll do modeset on next lid open event.
> > > 2. when lid is opened, MODESET_DONE is set
> > >    so that duplicate lid open events will be ignored.
> > > 3. when system suspends, MODESET_ON_RESUME is set.
> > >    In this case, we will not do modeset on any lid events.
> > > 
> > > Plus, locking mechanism is also introduced to avoid racing.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > 
> > Looks nice, two tiny bikesheds below.
> > 
> Great, thanks for reviewing. Refreshed patch attached.

oops, forgot to update the changelog and comments.
refreshed patch attached.

>From b584fcebb6d715a317f192c88606b875ee88ce78 Mon Sep 17 00:00:00 2001
From: Zhang Rui <rui.zhang@intel.com>
Date: Thu, 24 Jan 2013 13:27:22 +0800
Subject: [PATCH V3 3/3] i915: ignore lid open event when resuming

i915 driver needs to do modeset when
1. system resumes from sleep
2. lid is opened

In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
thus it is the i915_resume code does the modeset rather than intel_lid_notify().

But in PM_SUSPEND_FREEZE state, this will be broken because
system is still responsive to the lid events.
1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
2. When we reopen the lid, intel_lid_notify() will do a modeset,
   before the system is resumed.
here is the error log,

[92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
[92146.548076] Hardware name: VGN-Z540N
[92146.548078] pipe_off wait timed out
[92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
[92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
[92146.548175] Call Trace:
[92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
[92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
[92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
[92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
[92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
[92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
[92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
[92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
[92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
[92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
[92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
[92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
[92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
[92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
[92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
[92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
[92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
[92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
[92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
[92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
[92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
[92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
[92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
[92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
[92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
[92146.548640]  [<c1056e84>] kthread+0x94/0xa0
[92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
[92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
[92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0

three different modeset flags are introduced in this patch
MODESET_ON_LID_OPEN: do modeset on next lid open event
MODESET_DONE:  modeset already done
MODESET_SUSPENDED:  suspended, only do modeset when system is resumed

In this way,
1. when lid is closed, MODESET_ON_LID_OPEN is set so that
   we'll do modeset on next lid open event.
2. when lid is opened, MODESET_DONE is set
   so that duplicate lid open events will be ignored.
3. when system suspends, MODESET_SUSPENDED is set.
   In this case, we will not do modeset on any lid events.

Plus, locking mechanism is also introduced to avoid racing.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c   |    1 +
 drivers/gpu/drm/i915/i915_drv.c   |   14 +++++++++-----
 drivers/gpu/drm/i915/i915_drv.h   |   11 +++++++++--
 drivers/gpu/drm/i915/intel_lvds.c |   33 ++++++++++++++++++++-------------
 4 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 99daa89..a5115d8 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	spin_lock_init(&dev_priv->dpio_lock);
 
 	mutex_init(&dev_priv->rps.hw_lock);
+	mutex_init(&dev_priv->modeset_restore_lock);
 
 	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
 		dev_priv->num_pipe = 3;
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1172658..68c6048 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/* ignore lid events during suspend */
+	mutex_lock(&dev_priv->modeset_restore_lock);
+	dev_priv->modeset_restore = MODESET_SUSPENDED;
+	mutex_unlock(&dev_priv->modeset_restore_lock);
+
+
 	drm_kms_helper_poll_disable(dev);
 
 	pci_save_state(dev->pdev);
@@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 
 	intel_opregion_fini(dev);
 
-	/* Modeset on resume, not lid events */
-	dev_priv->modeset_on_lid = 0;
-
 	console_lock();
 	intel_fbdev_set_suspend(dev, 1);
 	console_unlock();
@@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
 
 	intel_opregion_init(dev);
 
-	dev_priv->modeset_on_lid = 0;
-
 	/*
 	 * The console lock can be pretty contented on resume due
 	 * to all the printk activity.  Try to keep it out of the hot
@@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		schedule_work(&dev_priv->console_resume_work);
 	}
 
+	mutex_lock(&dev_priv->modeset_restore_lock);
+	dev_priv->modeset_restore = MODESET_DONE;
+	mutex_unlock(&dev_priv->modeset_restore_lock);
 	return error;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 12ab3bd..0fddad01 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -620,6 +620,12 @@ struct intel_l3_parity {
 	struct work_struct error_work;
 };
 
+enum modeset_restore {
+	MODESET_ON_LID_OPEN,
+	MODESET_DONE,
+	MODESET_SUSPENDED,
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 
@@ -750,9 +756,10 @@ typedef struct drm_i915_private {
 
 	unsigned long quirks;
 
-	/* Register state */
-	bool modeset_on_lid;
+	enum modeset_restore modeset_restore; 
+	struct mutex modeset_restore_lock;
 
+	/* Register state */
 	struct {
 		/** Bridge to intel-gtt-ko */
 		struct intel_gtt *gtt;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 17aee74..ff89d58 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
 };
 
 /*
- * Lid events. Note the use of 'modeset_on_lid':
- *  - we set it on lid close, and reset it on open
+ * Lid events. Note the use of 'modeset':
+ *  - we set it to MODESET_ON_LID_OPEN on lid close,
+ *    and set it to MODESET_DONE on open
  *  - we use it as a "only once" bit (ie we ignore
- *    duplicate events where it was already properly
- *    set/reset)
- *  - the suspend/resume paths will also set it to
- *    zero, since they restore the mode ("lid open").
+ *    duplicate events where it was already properly set)
+ *  - the suspend/resume paths will set it to
+ *    MODESET_SUSPENDED and ignore the lid open event,
+ *    because they restore the mode ("lid open").
  */
 static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 			    void *unused)
@@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
 		return NOTIFY_OK;
 
+	mutex_lock(&dev_priv->modeset_restore_lock);
+	if (dev_priv->modeset_restore == MODESET_SUSPENDED)
+		goto exit;
 	/*
 	 * check and update the status of LVDS connector after receiving
 	 * the LID nofication event.
@@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 
 	/* Don't force modeset on machines where it causes a GPU lockup */
 	if (dmi_check_system(intel_no_modeset_on_lid))
-		return NOTIFY_OK;
+		goto exit;
 	if (!acpi_lid_open()) {
-		dev_priv->modeset_on_lid = 1;
-		return NOTIFY_OK;
+		/* do modeset on next lid open event */
+		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
+		goto exit;
 	}
 
-	if (!dev_priv->modeset_on_lid)
-		return NOTIFY_OK;
-
-	dev_priv->modeset_on_lid = 0;
+	if (dev_priv->modeset_restore == MODESET_DONE)
+		goto exit;
 
 	mutex_lock(&dev->mode_config.mutex);
 	intel_modeset_setup_hw_state(dev, true);
 	mutex_unlock(&dev->mode_config.mutex);
 
+	dev_priv->modeset_restore = MODESET_DONE;
+
+exit:
+	mutex_unlock(&dev_priv->modeset_restore_lock);
 	return NOTIFY_OK;
 }
 
-- 
1.7.9.5




^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
  2013-02-05  7:41       ` Zhang Rui
@ 2013-02-05 10:07         ` Daniel Vetter
  2013-02-05 10:14           ` Rafael J. Wysocki
  2013-02-05 12:34           ` Zhang Rui
  0 siblings, 2 replies; 11+ messages in thread
From: Daniel Vetter @ 2013-02-05 10:07 UTC (permalink / raw)
  To: Zhang Rui
  Cc: Daniel Vetter, linux-kernel, linux-pm, linux-acpi, intel-gfx, rjw, lenb

On Tue, Feb 05, 2013 at 03:41:53PM +0800, Zhang Rui wrote:
> oops, forgot to update the changelog and comments.
> refreshed patch attached.
> 
> From b584fcebb6d715a317f192c88606b875ee88ce78 Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang@intel.com>
> Date: Thu, 24 Jan 2013 13:27:22 +0800
> Subject: [PATCH V3 3/3] i915: ignore lid open event when resuming
> 
> i915 driver needs to do modeset when
> 1. system resumes from sleep
> 2. lid is opened

Patch applied, thanks. There's been a bit of a merge conflict and one tiny
checkpatch error, both fixed while applying. I plan to push this patch to
drm-next for 3.9.
-Daniel

> 
> In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
> thus it is the i915_resume code does the modeset rather than intel_lid_notify().
> 
> But in PM_SUSPEND_FREEZE state, this will be broken because
> system is still responsive to the lid events.
> 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
> 2. When we reopen the lid, intel_lid_notify() will do a modeset,
>    before the system is resumed.
> here is the error log,
> 
> [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
> [92146.548076] Hardware name: VGN-Z540N
> [92146.548078] pipe_off wait timed out
> [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
> [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
> [92146.548175] Call Trace:
> [92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
> [92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> [92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> [92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
> [92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
> [92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
> [92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
> [92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
> [92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
> [92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
> [92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
> [92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
> [92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
> [92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
> [92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
> [92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
> [92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
> [92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
> [92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
> [92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
> [92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
> [92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
> [92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
> [92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
> [92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
> [92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
> [92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
> [92146.548640]  [<c1056e84>] kthread+0x94/0xa0
> [92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
> [92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
> [92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0
> 
> three different modeset flags are introduced in this patch
> MODESET_ON_LID_OPEN: do modeset on next lid open event
> MODESET_DONE:  modeset already done
> MODESET_SUSPENDED:  suspended, only do modeset when system is resumed
> 
> In this way,
> 1. when lid is closed, MODESET_ON_LID_OPEN is set so that
>    we'll do modeset on next lid open event.
> 2. when lid is opened, MODESET_DONE is set
>    so that duplicate lid open events will be ignored.
> 3. when system suspends, MODESET_SUSPENDED is set.
>    In this case, we will not do modeset on any lid events.
> 
> Plus, locking mechanism is also introduced to avoid racing.
> 
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c   |    1 +
>  drivers/gpu/drm/i915/i915_drv.c   |   14 +++++++++-----
>  drivers/gpu/drm/i915/i915_drv.h   |   11 +++++++++--
>  drivers/gpu/drm/i915/intel_lvds.c |   33 ++++++++++++++++++++-------------
>  4 files changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 99daa89..a5115d8 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	spin_lock_init(&dev_priv->dpio_lock);
>  
>  	mutex_init(&dev_priv->rps.hw_lock);
> +	mutex_init(&dev_priv->modeset_restore_lock);
>  
>  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
>  		dev_priv->num_pipe = 3;
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1172658..68c6048 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	/* ignore lid events during suspend */
> +	mutex_lock(&dev_priv->modeset_restore_lock);
> +	dev_priv->modeset_restore = MODESET_SUSPENDED;
> +	mutex_unlock(&dev_priv->modeset_restore_lock);
> +
> +
>  	drm_kms_helper_poll_disable(dev);
>  
>  	pci_save_state(dev->pdev);
> @@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>  
>  	intel_opregion_fini(dev);
>  
> -	/* Modeset on resume, not lid events */
> -	dev_priv->modeset_on_lid = 0;
> -
>  	console_lock();
>  	intel_fbdev_set_suspend(dev, 1);
>  	console_unlock();
> @@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
>  
>  	intel_opregion_init(dev);
>  
> -	dev_priv->modeset_on_lid = 0;
> -
>  	/*
>  	 * The console lock can be pretty contented on resume due
>  	 * to all the printk activity.  Try to keep it out of the hot
> @@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
>  		schedule_work(&dev_priv->console_resume_work);
>  	}
>  
> +	mutex_lock(&dev_priv->modeset_restore_lock);
> +	dev_priv->modeset_restore = MODESET_DONE;
> +	mutex_unlock(&dev_priv->modeset_restore_lock);
>  	return error;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 12ab3bd..0fddad01 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -620,6 +620,12 @@ struct intel_l3_parity {
>  	struct work_struct error_work;
>  };
>  
> +enum modeset_restore {
> +	MODESET_ON_LID_OPEN,
> +	MODESET_DONE,
> +	MODESET_SUSPENDED,
> +};
> +
>  typedef struct drm_i915_private {
>  	struct drm_device *dev;
>  
> @@ -750,9 +756,10 @@ typedef struct drm_i915_private {
>  
>  	unsigned long quirks;
>  
> -	/* Register state */
> -	bool modeset_on_lid;
> +	enum modeset_restore modeset_restore; 
> +	struct mutex modeset_restore_lock;
>  
> +	/* Register state */
>  	struct {
>  		/** Bridge to intel-gtt-ko */
>  		struct intel_gtt *gtt;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 17aee74..ff89d58 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
>  };
>  
>  /*
> - * Lid events. Note the use of 'modeset_on_lid':
> - *  - we set it on lid close, and reset it on open
> + * Lid events. Note the use of 'modeset':
> + *  - we set it to MODESET_ON_LID_OPEN on lid close,
> + *    and set it to MODESET_DONE on open
>   *  - we use it as a "only once" bit (ie we ignore
> - *    duplicate events where it was already properly
> - *    set/reset)
> - *  - the suspend/resume paths will also set it to
> - *    zero, since they restore the mode ("lid open").
> + *    duplicate events where it was already properly set)
> + *  - the suspend/resume paths will set it to
> + *    MODESET_SUSPENDED and ignore the lid open event,
> + *    because they restore the mode ("lid open").
>   */
>  static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
>  			    void *unused)
> @@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
>  	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
>  		return NOTIFY_OK;
>  
> +	mutex_lock(&dev_priv->modeset_restore_lock);
> +	if (dev_priv->modeset_restore == MODESET_SUSPENDED)
> +		goto exit;
>  	/*
>  	 * check and update the status of LVDS connector after receiving
>  	 * the LID nofication event.
> @@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
>  
>  	/* Don't force modeset on machines where it causes a GPU lockup */
>  	if (dmi_check_system(intel_no_modeset_on_lid))
> -		return NOTIFY_OK;
> +		goto exit;
>  	if (!acpi_lid_open()) {
> -		dev_priv->modeset_on_lid = 1;
> -		return NOTIFY_OK;
> +		/* do modeset on next lid open event */
> +		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> +		goto exit;
>  	}
>  
> -	if (!dev_priv->modeset_on_lid)
> -		return NOTIFY_OK;
> -
> -	dev_priv->modeset_on_lid = 0;
> +	if (dev_priv->modeset_restore == MODESET_DONE)
> +		goto exit;
>  
>  	mutex_lock(&dev->mode_config.mutex);
>  	intel_modeset_setup_hw_state(dev, true);
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> +	dev_priv->modeset_restore = MODESET_DONE;
> +
> +exit:
> +	mutex_unlock(&dev_priv->modeset_restore_lock);
>  	return NOTIFY_OK;
>  }
>  
> -- 
> 1.7.9.5
> 
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
  2013-02-05 10:07         ` Daniel Vetter
@ 2013-02-05 10:14           ` Rafael J. Wysocki
  2013-02-05 12:35             ` Zhang Rui
  2013-02-05 12:34           ` Zhang Rui
  1 sibling, 1 reply; 11+ messages in thread
From: Rafael J. Wysocki @ 2013-02-05 10:14 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Zhang Rui, linux-kernel, linux-pm, linux-acpi, intel-gfx, lenb

On Tuesday, February 05, 2013 11:07:11 AM Daniel Vetter wrote:
> On Tue, Feb 05, 2013 at 03:41:53PM +0800, Zhang Rui wrote:
> > oops, forgot to update the changelog and comments.
> > refreshed patch attached.
> > 
> > From b584fcebb6d715a317f192c88606b875ee88ce78 Mon Sep 17 00:00:00 2001
> > From: Zhang Rui <rui.zhang@intel.com>
> > Date: Thu, 24 Jan 2013 13:27:22 +0800
> > Subject: [PATCH V3 3/3] i915: ignore lid open event when resuming
> > 
> > i915 driver needs to do modeset when
> > 1. system resumes from sleep
> > 2. lid is opened
> 
> Patch applied, thanks. There's been a bit of a merge conflict and one tiny
> checkpatch error, both fixed while applying. I plan to push this patch to
> drm-next for 3.9.

Thanks Daniel!

I will take the other patches from the series, then.

Rafael


> > In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
> > thus it is the i915_resume code does the modeset rather than intel_lid_notify().
> > 
> > But in PM_SUSPEND_FREEZE state, this will be broken because
> > system is still responsive to the lid events.
> > 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
> > 2. When we reopen the lid, intel_lid_notify() will do a modeset,
> >    before the system is resumed.
> > here is the error log,
> > 
> > [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
> > [92146.548076] Hardware name: VGN-Z540N
> > [92146.548078] pipe_off wait timed out
> > [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
> > [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
> > [92146.548175] Call Trace:
> > [92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
> > [92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
> > [92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
> > [92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
> > [92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
> > [92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
> > [92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
> > [92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
> > [92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
> > [92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
> > [92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
> > [92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
> > [92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
> > [92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
> > [92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
> > [92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
> > [92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
> > [92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
> > [92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
> > [92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
> > [92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
> > [92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
> > [92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
> > [92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
> > [92146.548640]  [<c1056e84>] kthread+0x94/0xa0
> > [92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
> > [92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
> > [92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0
> > 
> > three different modeset flags are introduced in this patch
> > MODESET_ON_LID_OPEN: do modeset on next lid open event
> > MODESET_DONE:  modeset already done
> > MODESET_SUSPENDED:  suspended, only do modeset when system is resumed
> > 
> > In this way,
> > 1. when lid is closed, MODESET_ON_LID_OPEN is set so that
> >    we'll do modeset on next lid open event.
> > 2. when lid is opened, MODESET_DONE is set
> >    so that duplicate lid open events will be ignored.
> > 3. when system suspends, MODESET_SUSPENDED is set.
> >    In this case, we will not do modeset on any lid events.
> > 
> > Plus, locking mechanism is also introduced to avoid racing.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c   |    1 +
> >  drivers/gpu/drm/i915/i915_drv.c   |   14 +++++++++-----
> >  drivers/gpu/drm/i915/i915_drv.h   |   11 +++++++++--
> >  drivers/gpu/drm/i915/intel_lvds.c |   33 ++++++++++++++++++++-------------
> >  4 files changed, 39 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 99daa89..a5115d8 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	spin_lock_init(&dev_priv->dpio_lock);
> >  
> >  	mutex_init(&dev_priv->rps.hw_lock);
> > +	mutex_init(&dev_priv->modeset_restore_lock);
> >  
> >  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
> >  		dev_priv->num_pipe = 3;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1172658..68c6048 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	/* ignore lid events during suspend */
> > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > +	dev_priv->modeset_restore = MODESET_SUSPENDED;
> > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> > +
> > +
> >  	drm_kms_helper_poll_disable(dev);
> >  
> >  	pci_save_state(dev->pdev);
> > @@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  
> >  	intel_opregion_fini(dev);
> >  
> > -	/* Modeset on resume, not lid events */
> > -	dev_priv->modeset_on_lid = 0;
> > -
> >  	console_lock();
> >  	intel_fbdev_set_suspend(dev, 1);
> >  	console_unlock();
> > @@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
> >  
> >  	intel_opregion_init(dev);
> >  
> > -	dev_priv->modeset_on_lid = 0;
> > -
> >  	/*
> >  	 * The console lock can be pretty contented on resume due
> >  	 * to all the printk activity.  Try to keep it out of the hot
> > @@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
> >  		schedule_work(&dev_priv->console_resume_work);
> >  	}
> >  
> > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > +	dev_priv->modeset_restore = MODESET_DONE;
> > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> >  	return error;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 12ab3bd..0fddad01 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -620,6 +620,12 @@ struct intel_l3_parity {
> >  	struct work_struct error_work;
> >  };
> >  
> > +enum modeset_restore {
> > +	MODESET_ON_LID_OPEN,
> > +	MODESET_DONE,
> > +	MODESET_SUSPENDED,
> > +};
> > +
> >  typedef struct drm_i915_private {
> >  	struct drm_device *dev;
> >  
> > @@ -750,9 +756,10 @@ typedef struct drm_i915_private {
> >  
> >  	unsigned long quirks;
> >  
> > -	/* Register state */
> > -	bool modeset_on_lid;
> > +	enum modeset_restore modeset_restore; 
> > +	struct mutex modeset_restore_lock;
> >  
> > +	/* Register state */
> >  	struct {
> >  		/** Bridge to intel-gtt-ko */
> >  		struct intel_gtt *gtt;
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 17aee74..ff89d58 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
> >  };
> >  
> >  /*
> > - * Lid events. Note the use of 'modeset_on_lid':
> > - *  - we set it on lid close, and reset it on open
> > + * Lid events. Note the use of 'modeset':
> > + *  - we set it to MODESET_ON_LID_OPEN on lid close,
> > + *    and set it to MODESET_DONE on open
> >   *  - we use it as a "only once" bit (ie we ignore
> > - *    duplicate events where it was already properly
> > - *    set/reset)
> > - *  - the suspend/resume paths will also set it to
> > - *    zero, since they restore the mode ("lid open").
> > + *    duplicate events where it was already properly set)
> > + *  - the suspend/resume paths will set it to
> > + *    MODESET_SUSPENDED and ignore the lid open event,
> > + *    because they restore the mode ("lid open").
> >   */
> >  static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >  			    void *unused)
> > @@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >  	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
> >  		return NOTIFY_OK;
> >  
> > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > +	if (dev_priv->modeset_restore == MODESET_SUSPENDED)
> > +		goto exit;
> >  	/*
> >  	 * check and update the status of LVDS connector after receiving
> >  	 * the LID nofication event.
> > @@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >  
> >  	/* Don't force modeset on machines where it causes a GPU lockup */
> >  	if (dmi_check_system(intel_no_modeset_on_lid))
> > -		return NOTIFY_OK;
> > +		goto exit;
> >  	if (!acpi_lid_open()) {
> > -		dev_priv->modeset_on_lid = 1;
> > -		return NOTIFY_OK;
> > +		/* do modeset on next lid open event */
> > +		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> > +		goto exit;
> >  	}
> >  
> > -	if (!dev_priv->modeset_on_lid)
> > -		return NOTIFY_OK;
> > -
> > -	dev_priv->modeset_on_lid = 0;
> > +	if (dev_priv->modeset_restore == MODESET_DONE)
> > +		goto exit;
> >  
> >  	mutex_lock(&dev->mode_config.mutex);
> >  	intel_modeset_setup_hw_state(dev, true);
> >  	mutex_unlock(&dev->mode_config.mutex);
> >  
> > +	dev_priv->modeset_restore = MODESET_DONE;
> > +
> > +exit:
> > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> >  	return NOTIFY_OK;
> >  }
> >  
> 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
  2013-02-05 10:07         ` Daniel Vetter
  2013-02-05 10:14           ` Rafael J. Wysocki
@ 2013-02-05 12:34           ` Zhang Rui
  1 sibling, 0 replies; 11+ messages in thread
From: Zhang Rui @ 2013-02-05 12:34 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: linux-kernel, linux-pm, linux-acpi, intel-gfx, rjw, lenb

On Tue, 2013-02-05 at 11:07 +0100, Daniel Vetter wrote:
> On Tue, Feb 05, 2013 at 03:41:53PM +0800, Zhang Rui wrote:
> > oops, forgot to update the changelog and comments.
> > refreshed patch attached.
> > 
> > From b584fcebb6d715a317f192c88606b875ee88ce78 Mon Sep 17 00:00:00 2001
> > From: Zhang Rui <rui.zhang@intel.com>
> > Date: Thu, 24 Jan 2013 13:27:22 +0800
> > Subject: [PATCH V3 3/3] i915: ignore lid open event when resuming
> > 
> > i915 driver needs to do modeset when
> > 1. system resumes from sleep
> > 2. lid is opened
> 
> Patch applied, thanks. There's been a bit of a merge conflict and one tiny
> checkpatch error, both fixed while applying. I plan to push this patch to
> drm-next for 3.9.
> -Daniel
> 

great, thanks!

-rui
> > 
> > In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
> > thus it is the i915_resume code does the modeset rather than intel_lid_notify().
> > 
> > But in PM_SUSPEND_FREEZE state, this will be broken because
> > system is still responsive to the lid events.
> > 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
> > 2. When we reopen the lid, intel_lid_notify() will do a modeset,
> >    before the system is resumed.
> > here is the error log,
> > 
> > [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
> > [92146.548076] Hardware name: VGN-Z540N
> > [92146.548078] pipe_off wait timed out
> > [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
> > [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
> > [92146.548175] Call Trace:
> > [92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
> > [92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
> > [92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
> > [92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
> > [92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
> > [92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
> > [92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
> > [92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
> > [92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
> > [92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
> > [92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
> > [92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
> > [92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
> > [92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
> > [92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
> > [92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
> > [92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
> > [92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
> > [92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
> > [92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
> > [92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
> > [92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
> > [92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
> > [92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
> > [92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
> > [92146.548640]  [<c1056e84>] kthread+0x94/0xa0
> > [92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
> > [92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
> > [92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0
> > 
> > three different modeset flags are introduced in this patch
> > MODESET_ON_LID_OPEN: do modeset on next lid open event
> > MODESET_DONE:  modeset already done
> > MODESET_SUSPENDED:  suspended, only do modeset when system is resumed
> > 
> > In this way,
> > 1. when lid is closed, MODESET_ON_LID_OPEN is set so that
> >    we'll do modeset on next lid open event.
> > 2. when lid is opened, MODESET_DONE is set
> >    so that duplicate lid open events will be ignored.
> > 3. when system suspends, MODESET_SUSPENDED is set.
> >    In this case, we will not do modeset on any lid events.
> > 
> > Plus, locking mechanism is also introduced to avoid racing.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c   |    1 +
> >  drivers/gpu/drm/i915/i915_drv.c   |   14 +++++++++-----
> >  drivers/gpu/drm/i915/i915_drv.h   |   11 +++++++++--
> >  drivers/gpu/drm/i915/intel_lvds.c |   33 ++++++++++++++++++++-------------
> >  4 files changed, 39 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 99daa89..a5115d8 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	spin_lock_init(&dev_priv->dpio_lock);
> >  
> >  	mutex_init(&dev_priv->rps.hw_lock);
> > +	mutex_init(&dev_priv->modeset_restore_lock);
> >  
> >  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
> >  		dev_priv->num_pipe = 3;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1172658..68c6048 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	/* ignore lid events during suspend */
> > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > +	dev_priv->modeset_restore = MODESET_SUSPENDED;
> > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> > +
> > +
> >  	drm_kms_helper_poll_disable(dev);
> >  
> >  	pci_save_state(dev->pdev);
> > @@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
> >  
> >  	intel_opregion_fini(dev);
> >  
> > -	/* Modeset on resume, not lid events */
> > -	dev_priv->modeset_on_lid = 0;
> > -
> >  	console_lock();
> >  	intel_fbdev_set_suspend(dev, 1);
> >  	console_unlock();
> > @@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
> >  
> >  	intel_opregion_init(dev);
> >  
> > -	dev_priv->modeset_on_lid = 0;
> > -
> >  	/*
> >  	 * The console lock can be pretty contented on resume due
> >  	 * to all the printk activity.  Try to keep it out of the hot
> > @@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
> >  		schedule_work(&dev_priv->console_resume_work);
> >  	}
> >  
> > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > +	dev_priv->modeset_restore = MODESET_DONE;
> > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> >  	return error;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 12ab3bd..0fddad01 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -620,6 +620,12 @@ struct intel_l3_parity {
> >  	struct work_struct error_work;
> >  };
> >  
> > +enum modeset_restore {
> > +	MODESET_ON_LID_OPEN,
> > +	MODESET_DONE,
> > +	MODESET_SUSPENDED,
> > +};
> > +
> >  typedef struct drm_i915_private {
> >  	struct drm_device *dev;
> >  
> > @@ -750,9 +756,10 @@ typedef struct drm_i915_private {
> >  
> >  	unsigned long quirks;
> >  
> > -	/* Register state */
> > -	bool modeset_on_lid;
> > +	enum modeset_restore modeset_restore; 
> > +	struct mutex modeset_restore_lock;
> >  
> > +	/* Register state */
> >  	struct {
> >  		/** Bridge to intel-gtt-ko */
> >  		struct intel_gtt *gtt;
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 17aee74..ff89d58 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
> >  };
> >  
> >  /*
> > - * Lid events. Note the use of 'modeset_on_lid':
> > - *  - we set it on lid close, and reset it on open
> > + * Lid events. Note the use of 'modeset':
> > + *  - we set it to MODESET_ON_LID_OPEN on lid close,
> > + *    and set it to MODESET_DONE on open
> >   *  - we use it as a "only once" bit (ie we ignore
> > - *    duplicate events where it was already properly
> > - *    set/reset)
> > - *  - the suspend/resume paths will also set it to
> > - *    zero, since they restore the mode ("lid open").
> > + *    duplicate events where it was already properly set)
> > + *  - the suspend/resume paths will set it to
> > + *    MODESET_SUSPENDED and ignore the lid open event,
> > + *    because they restore the mode ("lid open").
> >   */
> >  static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >  			    void *unused)
> > @@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >  	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
> >  		return NOTIFY_OK;
> >  
> > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > +	if (dev_priv->modeset_restore == MODESET_SUSPENDED)
> > +		goto exit;
> >  	/*
> >  	 * check and update the status of LVDS connector after receiving
> >  	 * the LID nofication event.
> > @@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> >  
> >  	/* Don't force modeset on machines where it causes a GPU lockup */
> >  	if (dmi_check_system(intel_no_modeset_on_lid))
> > -		return NOTIFY_OK;
> > +		goto exit;
> >  	if (!acpi_lid_open()) {
> > -		dev_priv->modeset_on_lid = 1;
> > -		return NOTIFY_OK;
> > +		/* do modeset on next lid open event */
> > +		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> > +		goto exit;
> >  	}
> >  
> > -	if (!dev_priv->modeset_on_lid)
> > -		return NOTIFY_OK;
> > -
> > -	dev_priv->modeset_on_lid = 0;
> > +	if (dev_priv->modeset_restore == MODESET_DONE)
> > +		goto exit;
> >  
> >  	mutex_lock(&dev->mode_config.mutex);
> >  	intel_modeset_setup_hw_state(dev, true);
> >  	mutex_unlock(&dev->mode_config.mutex);
> >  
> > +	dev_priv->modeset_restore = MODESET_DONE;
> > +
> > +exit:
> > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> >  	return NOTIFY_OK;
> >  }
> >  
> > -- 
> > 1.7.9.5
> > 
> > 
> > 
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Intel-gfx] [PATCH V2 3/3] i915: ignore lid open event when resuming
  2013-02-05 10:14           ` Rafael J. Wysocki
@ 2013-02-05 12:35             ` Zhang Rui
  0 siblings, 0 replies; 11+ messages in thread
From: Zhang Rui @ 2013-02-05 12:35 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Daniel Vetter, linux-kernel, linux-pm, linux-acpi, intel-gfx, lenb

On Tue, 2013-02-05 at 11:14 +0100, Rafael J. Wysocki wrote:
> On Tuesday, February 05, 2013 11:07:11 AM Daniel Vetter wrote:
> > On Tue, Feb 05, 2013 at 03:41:53PM +0800, Zhang Rui wrote:
> > > oops, forgot to update the changelog and comments.
> > > refreshed patch attached.
> > > 
> > > From b584fcebb6d715a317f192c88606b875ee88ce78 Mon Sep 17 00:00:00 2001
> > > From: Zhang Rui <rui.zhang@intel.com>
> > > Date: Thu, 24 Jan 2013 13:27:22 +0800
> > > Subject: [PATCH V3 3/3] i915: ignore lid open event when resuming
> > > 
> > > i915 driver needs to do modeset when
> > > 1. system resumes from sleep
> > > 2. lid is opened
> > 
> > Patch applied, thanks. There's been a bit of a merge conflict and one tiny
> > checkpatch error, both fixed while applying. I plan to push this patch to
> > drm-next for 3.9.
> 
> Thanks Daniel!
> 
> I will take the other patches from the series, then.
> 
great, thank you, Rafael.

-rui
> Rafael
> 
> 
> > > In PM_SUSPEND_MEM state, all the GPEs are cleared when system resumes,
> > > thus it is the i915_resume code does the modeset rather than intel_lid_notify().
> > > 
> > > But in PM_SUSPEND_FREEZE state, this will be broken because
> > > system is still responsive to the lid events.
> > > 1. When we close the lid in Freeze state, intel_lid_notify() sets modeset_on_lid.
> > > 2. When we reopen the lid, intel_lid_notify() will do a modeset,
> > >    before the system is resumed.
> > > here is the error log,
> > > 
> > > [92146.548074] WARNING: at drivers/gpu/drm/i915/intel_display.c:1028 intel_wait_for_pipe_off+0x184/0x190 [i915]()
> > > [92146.548076] Hardware name: VGN-Z540N
> > > [92146.548078] pipe_off wait timed out
> > > [92146.548167] Modules linked in: hid_generic usbhid hid snd_hda_codec_realtek snd_hda_intel snd_hda_codec parport_pc snd_hwdep ppdev snd_pcm_oss i915 snd_mixer_oss snd_pcm arc4 iwldvm snd_seq_dummy mac80211 snd_seq_oss snd_seq_midi fbcon tileblit font bitblit softcursor drm_kms_helper snd_rawmidi snd_seq_midi_event coretemp drm snd_seq kvm btusb bluetooth snd_timer iwlwifi pcmcia tpm_infineon i2c_algo_bit joydev snd_seq_device intel_agp cfg80211 snd intel_gtt yenta_socket pcmcia_rsrc sony_laptop agpgart microcode psmouse tpm_tis serio_raw mxm_wmi soundcore snd_page_alloc tpm acpi_cpufreq lpc_ich pcmcia_core tpm_bios mperf processor lp parport firewire_ohci firewire_core crc_itu_t sdhci_pci sdhci thermal e1000e
> > > [92146.548173] Pid: 4304, comm: kworker/0:0 Tainted: G        W    3.8.0-rc3-s0i3-v3-test+ #9
> > > [92146.548175] Call Trace:
> > > [92146.548189]  [<c10378e2>] warn_slowpath_common+0x72/0xa0
> > > [92146.548227]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > > [92146.548263]  [<f86398b4>] ? intel_wait_for_pipe_off+0x184/0x190 [i915]
> > > [92146.548270]  [<c10379b3>] warn_slowpath_fmt+0x33/0x40
> > > [92146.548307]  [<f86398b4>] intel_wait_for_pipe_off+0x184/0x190 [i915]
> > > [92146.548344]  [<f86399c2>] intel_disable_pipe+0x102/0x190 [i915]
> > > [92146.548380]  [<f8639ea4>] ? intel_disable_plane+0x64/0x80 [i915]
> > > [92146.548417]  [<f8639f7c>] i9xx_crtc_disable+0xbc/0x150 [i915]
> > > [92146.548456]  [<f863ebee>] intel_crtc_update_dpms+0x5e/0x90 [i915]
> > > [92146.548493]  [<f86437cf>] intel_modeset_setup_hw_state+0x42f/0x8f0 [i915]
> > > [92146.548535]  [<f8645b0b>] intel_lid_notify+0x9b/0xc0 [i915]
> > > [92146.548543]  [<c15610d3>] notifier_call_chain+0x43/0x60
> > > [92146.548550]  [<c105d1e1>] __blocking_notifier_call_chain+0x41/0x80
> > > [92146.548556]  [<c105d23f>] blocking_notifier_call_chain+0x1f/0x30
> > > [92146.548563]  [<c131a684>] acpi_lid_send_state+0x78/0xa4
> > > [92146.548569]  [<c131aa9e>] acpi_button_notify+0x3b/0xf1
> > > [92146.548577]  [<c12df56a>] ? acpi_os_execute+0x17/0x19
> > > [92146.548582]  [<c12e591a>] ? acpi_ec_sync_query+0xa5/0xbc
> > > [92146.548589]  [<c12e2b82>] acpi_device_notify+0x16/0x18
> > > [92146.548595]  [<c12f4904>] acpi_ev_notify_dispatch+0x38/0x4f
> > > [92146.548600]  [<c12df0e8>] acpi_os_execute_deferred+0x20/0x2b
> > > [92146.548607]  [<c1051208>] process_one_work+0x128/0x3f0
> > > [92146.548613]  [<c1564f73>] ? common_interrupt+0x33/0x38
> > > [92146.548618]  [<c104f8c0>] ? wake_up_worker+0x30/0x30
> > > [92146.548624]  [<c12df0c8>] ? acpi_os_wait_events_complete+0x1e/0x1e
> > > [92146.548629]  [<c10524f9>] worker_thread+0x119/0x3b0
> > > [92146.548634]  [<c10523e0>] ? manage_workers+0x240/0x240
> > > [92146.548640]  [<c1056e84>] kthread+0x94/0xa0
> > > [92146.548647]  [<c1060000>] ? ftrace_raw_output_sched_stat_runtime+0x70/0xf0
> > > [92146.548652]  [<c15649b7>] ret_from_kernel_thread+0x1b/0x28
> > > [92146.548658]  [<c1056df0>] ? kthread_create_on_node+0xc0/0xc0
> > > 
> > > three different modeset flags are introduced in this patch
> > > MODESET_ON_LID_OPEN: do modeset on next lid open event
> > > MODESET_DONE:  modeset already done
> > > MODESET_SUSPENDED:  suspended, only do modeset when system is resumed
> > > 
> > > In this way,
> > > 1. when lid is closed, MODESET_ON_LID_OPEN is set so that
> > >    we'll do modeset on next lid open event.
> > > 2. when lid is opened, MODESET_DONE is set
> > >    so that duplicate lid open events will be ignored.
> > > 3. when system suspends, MODESET_SUSPENDED is set.
> > >    In this case, we will not do modeset on any lid events.
> > > 
> > > Plus, locking mechanism is also introduced to avoid racing.
> > > 
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c   |    1 +
> > >  drivers/gpu/drm/i915/i915_drv.c   |   14 +++++++++-----
> > >  drivers/gpu/drm/i915/i915_drv.h   |   11 +++++++++--
> > >  drivers/gpu/drm/i915/intel_lvds.c |   33 ++++++++++++++++++++-------------
> > >  4 files changed, 39 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 99daa89..a5115d8 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1585,6 +1585,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > >  	spin_lock_init(&dev_priv->dpio_lock);
> > >  
> > >  	mutex_init(&dev_priv->rps.hw_lock);
> > > +	mutex_init(&dev_priv->modeset_restore_lock);
> > >  
> > >  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
> > >  		dev_priv->num_pipe = 3;
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 1172658..68c6048 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -468,6 +468,12 @@ static int i915_drm_freeze(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  
> > > +	/* ignore lid events during suspend */
> > > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > > +	dev_priv->modeset_restore = MODESET_SUSPENDED;
> > > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> > > +
> > > +
> > >  	drm_kms_helper_poll_disable(dev);
> > >  
> > >  	pci_save_state(dev->pdev);
> > > @@ -492,9 +498,6 @@ static int i915_drm_freeze(struct drm_device *dev)
> > >  
> > >  	intel_opregion_fini(dev);
> > >  
> > > -	/* Modeset on resume, not lid events */
> > > -	dev_priv->modeset_on_lid = 0;
> > > -
> > >  	console_lock();
> > >  	intel_fbdev_set_suspend(dev, 1);
> > >  	console_unlock();
> > > @@ -569,8 +572,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
> > >  
> > >  	intel_opregion_init(dev);
> > >  
> > > -	dev_priv->modeset_on_lid = 0;
> > > -
> > >  	/*
> > >  	 * The console lock can be pretty contented on resume due
> > >  	 * to all the printk activity.  Try to keep it out of the hot
> > > @@ -583,6 +584,9 @@ static int __i915_drm_thaw(struct drm_device *dev)
> > >  		schedule_work(&dev_priv->console_resume_work);
> > >  	}
> > >  
> > > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > > +	dev_priv->modeset_restore = MODESET_DONE;
> > > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> > >  	return error;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 12ab3bd..0fddad01 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -620,6 +620,12 @@ struct intel_l3_parity {
> > >  	struct work_struct error_work;
> > >  };
> > >  
> > > +enum modeset_restore {
> > > +	MODESET_ON_LID_OPEN,
> > > +	MODESET_DONE,
> > > +	MODESET_SUSPENDED,
> > > +};
> > > +
> > >  typedef struct drm_i915_private {
> > >  	struct drm_device *dev;
> > >  
> > > @@ -750,9 +756,10 @@ typedef struct drm_i915_private {
> > >  
> > >  	unsigned long quirks;
> > >  
> > > -	/* Register state */
> > > -	bool modeset_on_lid;
> > > +	enum modeset_restore modeset_restore; 
> > > +	struct mutex modeset_restore_lock;
> > >  
> > > +	/* Register state */
> > >  	struct {
> > >  		/** Bridge to intel-gtt-ko */
> > >  		struct intel_gtt *gtt;
> > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > > index 17aee74..ff89d58 100644
> > > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > > @@ -492,13 +492,14 @@ static const struct dmi_system_id intel_no_modeset_on_lid[] = {
> > >  };
> > >  
> > >  /*
> > > - * Lid events. Note the use of 'modeset_on_lid':
> > > - *  - we set it on lid close, and reset it on open
> > > + * Lid events. Note the use of 'modeset':
> > > + *  - we set it to MODESET_ON_LID_OPEN on lid close,
> > > + *    and set it to MODESET_DONE on open
> > >   *  - we use it as a "only once" bit (ie we ignore
> > > - *    duplicate events where it was already properly
> > > - *    set/reset)
> > > - *  - the suspend/resume paths will also set it to
> > > - *    zero, since they restore the mode ("lid open").
> > > + *    duplicate events where it was already properly set)
> > > + *  - the suspend/resume paths will set it to
> > > + *    MODESET_SUSPENDED and ignore the lid open event,
> > > + *    because they restore the mode ("lid open").
> > >   */
> > >  static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> > >  			    void *unused)
> > > @@ -512,6 +513,9 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> > >  	if (dev->switch_power_state != DRM_SWITCH_POWER_ON)
> > >  		return NOTIFY_OK;
> > >  
> > > +	mutex_lock(&dev_priv->modeset_restore_lock);
> > > +	if (dev_priv->modeset_restore == MODESET_SUSPENDED)
> > > +		goto exit;
> > >  	/*
> > >  	 * check and update the status of LVDS connector after receiving
> > >  	 * the LID nofication event.
> > > @@ -520,21 +524,24 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> > >  
> > >  	/* Don't force modeset on machines where it causes a GPU lockup */
> > >  	if (dmi_check_system(intel_no_modeset_on_lid))
> > > -		return NOTIFY_OK;
> > > +		goto exit;
> > >  	if (!acpi_lid_open()) {
> > > -		dev_priv->modeset_on_lid = 1;
> > > -		return NOTIFY_OK;
> > > +		/* do modeset on next lid open event */
> > > +		dev_priv->modeset_restore = MODESET_ON_LID_OPEN;
> > > +		goto exit;
> > >  	}
> > >  
> > > -	if (!dev_priv->modeset_on_lid)
> > > -		return NOTIFY_OK;
> > > -
> > > -	dev_priv->modeset_on_lid = 0;
> > > +	if (dev_priv->modeset_restore == MODESET_DONE)
> > > +		goto exit;
> > >  
> > >  	mutex_lock(&dev->mode_config.mutex);
> > >  	intel_modeset_setup_hw_state(dev, true);
> > >  	mutex_unlock(&dev->mode_config.mutex);
> > >  
> > > +	dev_priv->modeset_restore = MODESET_DONE;
> > > +
> > > +exit:
> > > +	mutex_unlock(&dev_priv->modeset_restore_lock);
> > >  	return NOTIFY_OK;
> > >  }
> > >  
> > 
> > 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE
  2013-02-04  7:10 [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui
  2013-02-04  7:10 ` [PATCH V2 2/3] ACPI: enable ACPI SCI during suspend Zhang Rui
  2013-02-04  7:10 ` [PATCH V2 3/3] i915: ignore lid open event when resuming Zhang Rui
@ 2013-02-06  1:11 ` Zhang Rui
  2 siblings, 0 replies; 11+ messages in thread
From: Zhang Rui @ 2013-02-06  1:11 UTC (permalink / raw)
  To: linux-kernel; +Cc: linux-pm, linux-acpi, intel-gfx, rjw, lenb

On Mon, 2013-02-04 at 15:10 +0800, Zhang Rui wrote:
> PM_SUSPEND_FREEZE state is a general state that
> does not need any platform specific support, it equals
> frozen processes + suspended devices + idle processors.
> 
> Compared with PM_SUSPEND_MEMORY,
> PM_SUSPEND_FREEZE saves less power
> because the system is still in a running state.
> PM_SUSPEND_FREEZE has less resume latency because it does not
> touch BIOS, and the processors are in idle state.
> 
> Compared with RTPM/idle,
> PM_SUSPEND_FREEZE saves more power as
> 1. the processor has longer sleep time because processes are frozen.
>    The deeper c-state the processor supports, more power saving we can get.
> 2. PM_SUSPEND_FREEZE uses system suspend code path, thus we can get
>    more power saving from the devices that does not have good RTPM support.
> 
> This state is useful for
> 1) platforms that do not have STR, or have a broken STR.
> 2) platforms that have an extremely low power idle state,
>    which can be used to replace STR.
> 
> The following describes how PM_SUSPEND_FREEZE state works.
> 1. echo freeze > /sys/power/state
> 2. the processes are frozen.
> 3. all the devices are suspended.
> 4. all the processors are blocked by a wait queue
> 5. all the processors idles and enters (Deep) c-state.
> 6. an interrupt fires.
> 7. a processor is woken up and handles the irq.
> 8. if it is a general event,
>    a) the irq handler runs and quites.
>    b) goto step 4.
> 9. if it is a real wake event, say, power button pressing, keyboard touch, mouse moving,
>    a) the irq handler runs and activate the wakeup source
>    b) wakeup_source_activate() notifies the wait queue.
>    c) system starts resuming from PM_SUSPEND_FREEZE
> 10. all the devices are resumed.
> 11. all the processes are unfrozen.
> 12. system is back to working.
> 
> Known Issue:
> The wakeup of this new PM_SUSPEND_FREEZE state may behave differently
> from the previous suspend state.
> Take ACPI platform for example, there are some GPEs that only enabled
> when the system is in sleep state, to wake the system backk from S3/S4.
> But we are not touching these GPEs during transition to PM_SUSPEND_FREEZE.
> This means we may lose some wake event.
> But on the other hand, as we do not disable all the Interrupts during
> PM_SUSPEND_FREEZE, we may get some extra "wakeup" Interrupts, that are
> not available for S3/S4.
> 
> The patches has been tested on an old Sony laptop, and here are the results:
> 
> Average Power:
> 1. RPTM/idle for half an hour:
>    14.8W, 12.6W, 14.1W, 12.5W, 14.4W, 13.2W, 12.9W
> 2. Freeze for half an hour:
>    11W, 10.4W, 9.4W, 11.3W 10.5W
> 3. RTPM/idle for three hours:
>    11.6W
> 4. Freeze for three hours:
>    10W
> 5. Suspend to Memory:
>    0.5~0.9W
> 
> Average Resume Latency:
> 1. RTPM/idle with a black screen: (From pressing keyboard to screen back)
>    Less than 0.2s
> 2. Freeze: (From pressing power button to screen back)
>    2.50s
> 3. Suspend to Memory: (From pressing power button to screen back)
>    4.33s
> 
> From the results, we can see that all the platforms should benefit from
> this patch, even if it does not have Low Power S0.
> 
fixed a build error when CONFIG_SUSPEND not set.
refreshed patch attached.

>From e784933534b8fc00b4e7b52f3c34ea9e614e513e Mon Sep 17 00:00:00 2001
From: Zhang Rui <rui.zhang@intel.com>
Date: Mon, 14 Jan 2013 11:12:53 +0800
Subject: [PATCH 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE

PM_SUSPEND_FREEZE state is a general state that
does not need any platform specific support, it equals
frozen processes + suspended devices + idle processors.

Compared with PM_SUSPEND_MEMORY,
PM_SUSPEND_FREEZE saves less power
because the system is still in a running state.
PM_SUSPEND_FREEZE has less resume latency because it does not
touch BIOS, and the processors are in idle state.

Compared with RTPM/idle,
PM_SUSPEND_FREEZE saves more power as
1. the processor has longer sleep time because processes are frozen.
   The deeper c-state the processor supports, more power saving we can get.
2. PM_SUSPEND_FREEZE uses system suspend code path, thus we can get
   more power saving from the devices that does not have good RTPM support.

This state is useful for
1) platforms that do not have STR, or have a broken STR.
2) platforms that have an extremely low power idle state,
   which can be used to replace STR.

The following describes how PM_SUSPEND_FREEZE state works.
1. echo freeze > /sys/power/state
2. the processes are frozen.
3. all the devices are suspended.
4. all the processors are blocked by a wait queue
5. all the processors idles and enters (Deep) c-state.
6. an interrupt fires.
7. a processor is woken up and handles the irq.
8. if it is a general event,
   a) the irq handler runs and quites.
   b) goto step 4.
9. if it is a real wake event, say, power button pressing, keyboard touch, mouse moving,
   a) the irq handler runs and activate the wakeup source
   b) wakeup_source_activate() notifies the wait queue.
   c) system starts resuming from PM_SUSPEND_FREEZE
10. all the devices are resumed.
11. all the processes are unfrozen.
12. system is back to working.

Known Issue:
The wakeup of this new PM_SUSPEND_FREEZE state may behave differently
from the previous suspend state.
Take ACPI platform for example, there are some GPEs that only enabled
when the system is in sleep state, to wake the system backk from S3/S4.
But we are not touching these GPEs during transition to PM_SUSPEND_FREEZE.
This means we may lose some wake event.
But on the other hand, as we do not disable all the Interrupts during
PM_SUSPEND_FREEZE, we may get some extra "wakeup" Interrupts, that are
not available for S3/S4.

The patches has been tested on an old Sony laptop, and here are the results:

Average Power:
1. RPTM/idle for half an hour:
   14.8W, 12.6W, 14.1W, 12.5W, 14.4W, 13.2W, 12.9W
2. Freeze for half an hour:
   11W, 10.4W, 9.4W, 11.3W 10.5W
3. RTPM/idle for three hours:
   11.6W
4. Freeze for three hours:
   10W
5. Suspend to Memory:
   0.5~0.9W

Average Resume Latency:
1. RTPM/idle with a black screen: (From pressing keyboard to screen back)
   Less than 0.2s
2. Freeze: (From pressing power button to screen back)
   2.50s
3. Suspend to Memory: (From pressing power button to screen back)
   4.33s

>From the results, we can see that all the platforms should benefit from
this patch, even if it does not have Low Power S0.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/base/power/wakeup.c |    6 ++++
 include/linux/suspend.h     |    6 +++-
 kernel/power/main.c         |    2 +-
 kernel/power/suspend.c      |   69 +++++++++++++++++++++++++++++++++++--------
 4 files changed, 68 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index e6ee5e8..79715e7 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -382,6 +382,12 @@ static void wakeup_source_activate(struct wakeup_source *ws)
 {
 	unsigned int cec;
 
+	/*
+	 * active wakeup source should bring the system
+	 * out of PM_SUSPEND_FREEZE state
+	 */
+	freeze_wake();
+
 	ws->active = true;
 	ws->active_count++;
 	ws->last_time = ktime_get();
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0c808d7..d4e3f16 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -34,8 +34,10 @@ static inline void pm_restore_console(void)
 typedef int __bitwise suspend_state_t;
 
 #define PM_SUSPEND_ON		((__force suspend_state_t) 0)
-#define PM_SUSPEND_STANDBY	((__force suspend_state_t) 1)
+#define PM_SUSPEND_FREEZE	((__force suspend_state_t) 1)
+#define PM_SUSPEND_STANDBY	((__force suspend_state_t) 2)
 #define PM_SUSPEND_MEM		((__force suspend_state_t) 3)
+#define PM_SUSPEND_MIN		PM_SUSPEND_FREEZE
 #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
 
 enum suspend_stat_step {
@@ -192,6 +194,7 @@ struct platform_suspend_ops {
  */
 extern void suspend_set_ops(const struct platform_suspend_ops *ops);
 extern int suspend_valid_only_mem(suspend_state_t state);
+extern void freeze_wake(void);
 
 /**
  * arch_suspend_disable_irqs - disable IRQs for suspend
@@ -217,6 +220,7 @@ extern int pm_suspend(suspend_state_t state);
 
 static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {}
 static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; }
+static inline void freeze_wake(void) {}
 #endif /* !CONFIG_SUSPEND */
 
 /* struct pbe is used for creating lists of pages that should be restored
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 1c16f91..b1c26a9 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -313,7 +313,7 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
 static suspend_state_t decode_state(const char *buf, size_t n)
 {
 #ifdef CONFIG_SUSPEND
-	suspend_state_t state = PM_SUSPEND_STANDBY;
+	suspend_state_t state = PM_SUSPEND_MIN;
 	const char * const *s;
 #endif
 	char *p;
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index c8b7446..d4feda0 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -30,12 +30,38 @@
 #include "power.h"
 
 const char *const pm_states[PM_SUSPEND_MAX] = {
+	[PM_SUSPEND_FREEZE]	= "freeze",
 	[PM_SUSPEND_STANDBY]	= "standby",
 	[PM_SUSPEND_MEM]	= "mem",
 };
 
 static const struct platform_suspend_ops *suspend_ops;
 
+static bool need_suspend_ops(suspend_state_t state)
+{
+	return !!(state > PM_SUSPEND_FREEZE);
+}
+
+static DECLARE_WAIT_QUEUE_HEAD(suspend_freeze_wait_head);
+static bool suspend_freeze_wake;
+
+static void freeze_begin(void)
+{
+	suspend_freeze_wake = false;
+}
+
+static void freeze_enter(void)
+{
+	wait_event(suspend_freeze_wait_head, suspend_freeze_wake);
+}
+
+void freeze_wake(void)
+{
+	suspend_freeze_wake = true;
+	wake_up(&suspend_freeze_wait_head);
+}
+EXPORT_SYMBOL_GPL(freeze_wake);
+
 /**
  * suspend_set_ops - Set the global suspend method table.
  * @ops: Suspend operations to use.
@@ -50,8 +76,11 @@ EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 bool valid_state(suspend_state_t state)
 {
+	if (state == PM_SUSPEND_FREEZE)
+		return true;
 	/*
-	 * All states need lowlevel support and need to be valid to the lowlevel
+	 * PM_SUSPEND_STANDBY and PM_SUSPEND_MEMORY states need lowlevel
+	 * support and need to be valid to the lowlevel
 	 * implementation, no valid callback implies that none are valid.
 	 */
 	return suspend_ops && suspend_ops->valid && suspend_ops->valid(state);
@@ -89,11 +118,11 @@ static int suspend_test(int level)
  * hibernation).  Run suspend notifiers, allocate the "suspend" console and
  * freeze processes.
  */
-static int suspend_prepare(void)
+static int suspend_prepare(suspend_state_t state)
 {
 	int error;
 
-	if (!suspend_ops || !suspend_ops->enter)
+	if (need_suspend_ops(state) && (!suspend_ops || !suspend_ops->enter))
 		return -EPERM;
 
 	pm_prepare_console();
@@ -137,7 +166,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 {
 	int error;
 
-	if (suspend_ops->prepare) {
+	if (need_suspend_ops(state) && suspend_ops->prepare) {
 		error = suspend_ops->prepare();
 		if (error)
 			goto Platform_finish;
@@ -149,12 +178,23 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 		goto Platform_finish;
 	}
 
-	if (suspend_ops->prepare_late) {
+	if (need_suspend_ops(state) && suspend_ops->prepare_late) {
 		error = suspend_ops->prepare_late();
 		if (error)
 			goto Platform_wake;
 	}
 
+	/*
+	 * PM_SUSPEND_FREEZE equals
+	 * frozen processes + suspended devices + idle processors.
+	 * Thus we should invoke freeze_enter() soon after
+	 * all the devices are suspended.
+	 */
+	if (state == PM_SUSPEND_FREEZE) {
+		freeze_enter();
+		goto Platform_wake;
+	}
+
 	if (suspend_test(TEST_PLATFORM))
 		goto Platform_wake;
 
@@ -182,13 +222,13 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 	enable_nonboot_cpus();
 
  Platform_wake:
-	if (suspend_ops->wake)
+	if (need_suspend_ops(state) && suspend_ops->wake)
 		suspend_ops->wake();
 
 	dpm_resume_start(PMSG_RESUME);
 
  Platform_finish:
-	if (suspend_ops->finish)
+	if (need_suspend_ops(state) && suspend_ops->finish)
 		suspend_ops->finish();
 
 	return error;
@@ -203,11 +243,11 @@ int suspend_devices_and_enter(suspend_state_t state)
 	int error;
 	bool wakeup = false;
 
-	if (!suspend_ops)
+	if (need_suspend_ops(state) && !suspend_ops)
 		return -ENOSYS;
 
 	trace_machine_suspend(state);
-	if (suspend_ops->begin) {
+	if (need_suspend_ops(state) && suspend_ops->begin) {
 		error = suspend_ops->begin(state);
 		if (error)
 			goto Close;
@@ -226,7 +266,7 @@ int suspend_devices_and_enter(suspend_state_t state)
 
 	do {
 		error = suspend_enter(state, &wakeup);
-	} while (!error && !wakeup
+	} while (!error && !wakeup && need_suspend_ops(state)
 		&& suspend_ops->suspend_again && suspend_ops->suspend_again());
 
  Resume_devices:
@@ -236,13 +276,13 @@ int suspend_devices_and_enter(suspend_state_t state)
 	ftrace_start();
 	resume_console();
  Close:
-	if (suspend_ops->end)
+	if (need_suspend_ops(state) && suspend_ops->end)
 		suspend_ops->end();
 	trace_machine_suspend(PWR_EVENT_EXIT);
 	return error;
 
  Recover_platform:
-	if (suspend_ops->recover)
+	if (need_suspend_ops(state) && suspend_ops->recover)
 		suspend_ops->recover();
 	goto Resume_devices;
 }
@@ -278,12 +318,15 @@ static int enter_state(suspend_state_t state)
 	if (!mutex_trylock(&pm_mutex))
 		return -EBUSY;
 
+	if (state == PM_SUSPEND_FREEZE)
+		freeze_begin();
+
 	printk(KERN_INFO "PM: Syncing filesystems ... ");
 	sys_sync();
 	printk("done.\n");
 
 	pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]);
-	error = suspend_prepare();
+	error = suspend_prepare(state);
 	if (error)
 		goto Unlock;
 
-- 
1.7.9.5






^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-02-06  1:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-04  7:10 [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui
2013-02-04  7:10 ` [PATCH V2 2/3] ACPI: enable ACPI SCI during suspend Zhang Rui
2013-02-04  7:10 ` [PATCH V2 3/3] i915: ignore lid open event when resuming Zhang Rui
2013-02-04 15:25   ` [Intel-gfx] " Daniel Vetter
2013-02-04 23:58     ` Zhang Rui
2013-02-05  7:41       ` Zhang Rui
2013-02-05 10:07         ` Daniel Vetter
2013-02-05 10:14           ` Rafael J. Wysocki
2013-02-05 12:35             ` Zhang Rui
2013-02-05 12:34           ` Zhang Rui
2013-02-06  1:11 ` [PATCH V2 1/3] PM: Introduce suspend state PM_SUSPEND_FREEZE Zhang Rui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).