Re: Suspend problems in 2.6.31-rc6

[Zhenyu Wang <zhenyuw@linux.intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3151 bytes --]

On 2009.08.21 19:01:06 +0200, Rafael J. Wysocki wrote:
> Yes.  Moreover, it doesn't need to be restored at that point, because it has
> already been restored earlier by the PCI bus type code.

true.

> 
> So, Zhenyu, it is completely unnecessary to call pci_restore_state() from
> a driver's ->resume() routine, because the PCI bus type driver has
> already called it in its ->resume_noirq().  And please note that
> pci_pm_default_resume_noirq() clears state_saved, which is why the Alek's
> patch helps.
> 
> Without the Alek's patch on the Alan's system the config registers of both
> PCI devices are restored twice, first by the PCI bus type at the "noirq" stage
> and next by the driver.  The Alek's patch turns the second restorations into
> NOPs, which is fine, because they are unnecessary in general and they break
> things on the Alan's system.
> 

yeah, Alex's patch is ok in general, and Alan's problem seems specific on 845G.
So the origin restore order issue for us seems gone with pci layer early resume,
we could remove the restore in intel_agp and drm/i915, although Alex's patch makes
them noop.

How about this one? Alan, could you help to test this with drm/i915 module loaded?

thanks.

From 7ab123a67e3ab3f31051d95e74362ae711e0657e Mon Sep 17 00:00:00 2001
From: Zhenyu Wang <zhenyuw@linux.intel.com>
Date: Mon, 24 Aug 2009 10:29:07 +0800
Subject: [PATCH] agp/intel: remove restore in resume

As earlier pci driver resume has already restored space for
host bridge and graphics device, don't need to restore it
again which might cause problem on some chips, e.g 845G tested
by Alan.

Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
---
 drivers/char/agp/intel-agp.c    |    9 ---------
 drivers/gpu/drm/i915/i915_drv.c |    1 -
 2 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index 8c9d50d..ed43ab2 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -2308,15 +2308,6 @@ static int agp_intel_resume(struct pci_dev *pdev)
 	struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
 	int ret_val;
 
-	pci_restore_state(pdev);
-
-	/* We should restore our graphics device's config space,
-	 * as host bridge (00:00) resumes before graphics device (02:00),
-	 * then our access to its pci space can work right.
-	 */
-	if (intel_private.pcidev)
-		pci_restore_state(intel_private.pcidev);
-
 	if (bridge->driver == &intel_generic_driver)
 		intel_configure();
 	else if (bridge->driver == &intel_850_driver)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index fc4b68a..645f298 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -95,7 +95,6 @@ static int i915_resume(struct drm_device *dev)
 	int ret = 0;
 
 	pci_set_power_state(dev->pdev, PCI_D0);
-	pci_restore_state(dev->pdev);
 	if (pci_enable_device(dev->pdev))
 		return -1;
 	pci_set_master(dev->pdev);
-- 
1.5.4

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]