Dave,
It looks that config space save/restore for intel-agp still has problem
that might affect some chip models. Andreas Mohr's work on his i815 suspend/resume
support showed that we need to save extra bits in config space on this old chip type.
His patch is in -mm tree, http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.22-rc4/2.6.22-rc4-mm2/broken-out/working-3d-dri-intel-agpko-resume-for-i815-chip.patch
And recently James Bottomley also reported that save/restore whole 256 bytes config
space for gfx device can fix his s3 issue on Fujitsu P7120 i915.
http://lists.freedesktop.org/archives/xorg/2007-June/025346.html
So here's my suggested patch for save whole 256 bytes config space for intel-agp,
which could fix these issues. I tested it on my 965GM, that s3 works fine with X.
This patch bases on latest kernel git and my intel-agp patch set on agpgart.git tip.
http://git.kernel.org/?p=linux/kernel/git/davej/agpgart.git;a=summary
Signed-off-by: Wang Zhenyu <[email protected]>
---
drivers/char/agp/intel-agp.c | 44 +++++++++++++++++++++++++++++++++++++++++-
1 files changed, 43 insertions(+), 1 deletions(-)
diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index d383168..bc18241 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -110,6 +110,7 @@ static struct _intel_private {
* popup and for the GTT.
*/
int gtt_entries; /* i830+ */
+ u32 extra_saved_config[48]; /* suspend/resume */
} intel_private;
static int intel_i810_fetch_size(void)
@@ -1974,9 +1975,33 @@ static void __devexit agp_intel_remove(struct pci_dev *pdev)
}
#ifdef CONFIG_PM
+static int agp_intel_suspend (struct pci_dev *pdev, pm_message_t state)
+{
+ int i;
+
+ pci_save_state(pdev);
+
+ if (intel_private.pcidev) {
+ pci_save_state(intel_private.pcidev);
+
+ for (i = 0; i < 48; i++)
+ pci_read_config_dword(intel_private.pcidev, i*4+64,
+ &intel_private.extra_saved_config[i]);
+
+ pci_set_power_state(intel_private.pcidev,
+ pci_choose_state(intel_private.pcidev, state));
+ }
+
+ pci_set_power_state(pdev, pci_choose_state(pdev, state));
+
+ return 0;
+}
+
static int agp_intel_resume(struct pci_dev *pdev)
{
struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
+ int i = 0;
+ u32 val;
pci_restore_state(pdev);
@@ -1984,8 +2009,24 @@ static int agp_intel_resume(struct pci_dev *pdev)
* 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)
+ if (intel_private.pcidev) {
+ pci_set_power_state(intel_private.pcidev, PCI_D0);
pci_restore_state(intel_private.pcidev);
+ for (i = 0; i < 48; i++) {
+ pci_read_config_dword(intel_private.pcidev, i*4+64,
+ &val);
+ if (val != intel_private.extra_saved_config[i]) {
+ printk(KERN_DEBUG "intel-agp: Writing back"
+ "config space at offset %x"
+ " (was %x, writing %x)\n",
+ i*4+64, val,
+ intel_private.extra_saved_config[i]);
+ pci_write_config_dword(intel_private.pcidev,
+ i*4+64,
+ intel_private.extra_saved_config[i]);
+ }
+ }
+ }
if (bridge->driver == &intel_generic_driver)
intel_configure();
@@ -2062,6 +2103,7 @@ static struct pci_driver agp_intel_pci_driver = {
.probe = agp_intel_probe,
.remove = __devexit_p(agp_intel_remove),
#ifdef CONFIG_PM
+ .suspend = agp_intel_suspend,
.resume = agp_intel_resume,
#endif
};
--
1.4.4.4
On Tue, Jun 12, 2007 at 09:54:59AM +0800, Wang Zhenyu wrote:
> It looks that config space save/restore for intel-agp still has problem
> that might affect some chip models. Andreas Mohr's work on his i815 suspend/resume
> support showed that we need to save extra bits in config space on this old chip type.
> His patch is in -mm tree, http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.22-rc4/2.6.22-rc4-mm2/broken-out/working-3d-dri-intel-agpko-resume-for-i815-chip.patch
>
> And recently James Bottomley also reported that save/restore whole 256 bytes config
> space for gfx device can fix his s3 issue on Fujitsu P7120 i915.
> http://lists.freedesktop.org/archives/xorg/2007-June/025346.html
>
> So here's my suggested patch for save whole 256 bytes config space for intel-agp,
> which could fix these issues. I tested it on my 965GM, that s3 works fine with X.
I'd feel much safer if we only did this on chipsets where we know we
have to do it. Doing this for *every* Intel chipset ever made _will_
bite us. There are some early chipsets (440BX era iirc) that would just
hang the box when you tried to read from various write-only registers.
But even on the chipsets where we _do_ need to save/restore something in
the upper part of the config space, surely it'd be a lot safer
to just save/restore what we need to rather than risk all sorts
of mayhem by writing to bits that may trigger hardware events.
Dave
--
http://www.codemonkey.org.uk
On 2007.06.11 22:23:21 +0000, Dave Jones wrote:
>
> I'd feel much safer if we only did this on chipsets where we know we
> have to do it. Doing this for *every* Intel chipset ever made _will_
> bite us. There are some early chipsets (440BX era iirc) that would just
> hang the box when you tried to read from various write-only registers.
>
> But even on the chipsets where we _do_ need to save/restore something in
> the upper part of the config space, surely it'd be a lot safer
> to just save/restore what we need to rather than risk all sorts
> of mayhem by writing to bits that may trigger hardware events.
>
I understand. Before James reported his problem on i915, I have thought
the basic restore on that chip should already be enough, but he proved I was
wrong and I'm not sure if this also happens on other i915 board with different
BIOS.
And with my patch it has already removed the restore cases for 440BX like type,
coz it's gmch_chip_id == 0 and intel_private.pcidev is NULL, so it won't save
extra space on those chips.
On Tue, Jun 12, 2007 at 11:34:25AM +0800, Wang Zhenyu wrote:
> I understand. Before James reported his problem on i915, I have thought
> the basic restore on that chip should already be enough, but he proved I was
> wrong and I'm not sure if this also happens on other i915 board with different
> BIOS.
>
> And with my patch it has already removed the restore cases for 440BX like type,
> coz it's gmch_chip_id == 0 and intel_private.pcidev is NULL, so it won't save
> extra space on those chips.
The 440BX was one example, for all we know there are similar ordering
issues with other chipsets. We hit this problem with the code that
restores the first 64 bytes first of all. Then we found out we had
to restore them in reverse order to be safe. We were able to do
this generically, because those bytes are standardised across devices.
The upper config space isn't standardised, so we have to obey the
per-device rules as to what order we read/write things.
Writing back an "enable" bit somewhere before we've written back
addresses in later registers for example may result in really
bizarre things happening. These are the kind of bugs that aren't
obvious, and turn out to be "that weird reboot that happens
every 3rd tuesday" six months after we've merged the changes
and everyones forgotten all about the potential problems.
The AGP code has had more than its fair share of really nasty
bugs like this to track down, so I'm strongly opposed to introducing
hacks that may trip us up later.
Whilst I'm not a huge fan of the 815 patch in -mm as it stands,
I think it's a better direction to go in to have per-chipset
save/restore routines.
Dave
--
http://www.codemonkey.org.uk
On Tue, Jun 12, 2007 at 09:54:59AM +0800, Wang Zhenyu wrote:
> + for (i = 0; i < 48; i++) {
You seem to be writing the base address after the aperture size? That
won't work. As Dave says, there are ordering contraints.
--
Matthew Garrett | [email protected]
On Monday, June 11, 2007 8:59:14 Dave Jones wrote:
> On Tue, Jun 12, 2007 at 11:34:25AM +0800, Wang Zhenyu wrote:
> > I understand. Before James reported his problem on i915, I have
> > thought the basic restore on that chip should already be enough,
> > but he proved I was wrong and I'm not sure if this also happens on
> > other i915 board with different BIOS.
> >
> > And with my patch it has already removed the restore cases for
> > 440BX like type, coz it's gmch_chip_id == 0 and
> > intel_private.pcidev is NULL, so it won't save extra space on
> > those chips.
>
> The 440BX was one example, for all we know there are similar ordering
> issues with other chipsets. We hit this problem with the code that
> restores the first 64 bytes first of all. Then we found out we had
> to restore them in reverse order to be safe. We were able to do
> this generically, because those bytes are standardised across
> devices.
>
> The upper config space isn't standardised, so we have to obey the
> per-device rules as to what order we read/write things.
> Writing back an "enable" bit somewhere before we've written back
> addresses in later registers for example may result in really
> bizarre things happening. These are the kind of bugs that aren't
> obvious, and turn out to be "that weird reboot that happens
> every 3rd tuesday" six months after we've merged the changes
> and everyones forgotten all about the potential problems.
>
> The AGP code has had more than its fair share of really nasty
> bugs like this to track down, so I'm strongly opposed to introducing
> hacks that may trip us up later.
>
> Whilst I'm not a huge fan of the 815 patch in -mm as it stands,
> I think it's a better direction to go in to have per-chipset
> save/restore routines.
I agree. I think saving the registers explicitly rather than as a block
is the best long term direction. That way we can carefully evaluate
which ones actually need to be saved/restored, and it what order.
Doing it as a block hides that and makes it seem chipset independent
when it really might not be...
Jesse