Hi!
> > > > > Especially the PCI video_state trick finally got me a working resume
> > > > > on
> > > > > 2.6.19-ck2 r128 Rage Mobility M4 AGP *WITH*(!) fully enabled and
> > > > > working
> > > > > (and keeping working!) DRI (3D).
> > > >
> > > > Can we get whitelist entry for suspend.sf.net? s2ram from there can do
> > > > all the tricks you described, one letter per trick :-). We even got
> > > > PCI saving lately.
> > >
> > > Whitelist? Let me blacklist it all the way to Timbuktu instead!
> >
> > > I've been doing more testing, and X never managed to come back to working
> > > state without some of my couple intel-agp changes:
> > > - a proper suspend method, doing a proper pci_save_state()
> > > or improved equivalent
> > > - a missing resume check for my i815 chipset
> > > - global cache flush in _configure
> > > - restoring AGP bridge PCI config space
> >
> > Can you post a patch?
>
> Took way longer than I'd have wanted it to (nice daughter and stuff ;),
> but here it is.
No problem.
Ok, I guess we'll need a real commit log.
> #define INTEL_I820_RDCR 0x51
> @@ -664,7 +671,7 @@
> if ((pg_start + mem->page_count) > num_entries)
> goto out_err;
>
> - /* The i830 can't check the GTT for entries since its read only,
> + /* The i830 can't check the GTT for entries since it's read-only,
> * depend on the caller to make the correct offset decisions.
> */
>
> @@ -787,7 +794,7 @@
> if ((pg_start + mem->page_count) > num_entries)
> goto out_err;
>
> - /* The i915 can't check the GTT for entries since its read only,
> + /* The i915 can't check the GTT for entries since it's read-only,
> * depend on the caller to make the correct offset decisions.
> */
>
You should not do cleanups at the same time as code changes.
> @@ -1103,8 +1110,8 @@
> /* attbase - aperture base */
> /* the Intel 815 chipset spec. says that bits 29-31 in the
> * ATTBASE register are reserved -> try not to write them */
> - if (agp_bridge->gatt_bus_addr & INTEL_815_ATTBASE_MASK) {
> - printk (KERN_EMERG PFX "gatt bus addr too high");
> + if (agp_bridge->gatt_bus_addr & I815_ATTBASE_MASK) {
> + printk (KERN_EMERG PFX "gatt bus addr too high\n");
> return -EINVAL;
> }
>
> @@ -1119,7 +1126,7 @@
> agp_bridge->gart_bus_addr = (temp & PCI_BASE_ADDRESS_MEM_MASK);
>
> pci_read_config_dword(agp_bridge->dev, INTEL_ATTBASE, &addr);
> - addr &= INTEL_815_ATTBASE_MASK;
> + addr &= I815_ATTBASE_MASK;
> addr |= agp_bridge->gatt_bus_addr;
> pci_write_config_dword(agp_bridge->dev, INTEL_ATTBASE, addr);
And I guess macro search&replace counts as cleanup, too. It would be
nice to submit them separately and first.
> @@ -1355,7 +1362,7 @@
> pci_write_config_dword(agp_bridge->dev, INTEL_ATTBASE,
> agp_bridge->gatt_bus_addr);
>
> /* agpctrl */
> - pci_write_config_dword(agp_bridge->dev, INTEL_AGPCTRL, 0x0000);
> + pci_write_config_dword(agp_bridge->dev, INTEL_AGPCTRL, 0x00000000);
>
Huh?
> #ifdef CONFIG_PM
> +
I'd kill this empty line.
> +static int agp_i815_remember_state(struct pci_dev *pdev, int restore)
> +{
> + typedef struct {
> + int reg;
> + u32 value;
> + } saved_regs;
> +
> + /* Dell Inspiron 8000 BIOS rev. A23 needs the following registers saved
> + * to be able to successfully restore X11 when AGP 3D is enabled
> + * (register values given are after resume vs. before suspend):
> + *
> + * I815_GMCHCFG (0x50; we need to set bit 1 (Aperture Access Global
> Enable) of I815_APCONT (0x51),
> + * thus use I815_GMCHCFG (0x50) as 32bit base register);
> 0x4fdd0040 instead of 0x4fdd0240
> + * ??? (0x80); 0x07ce1cde instead of 0x07cb94de
> + * I815_SM_RCOMP (0x98); 0x80648064 instead of 0x80548054
> + * I815_SM (0x9c); 0x00c48405 instead of 0x04848405
> + * I815_AGPCMD (0xa8); 0x00000000 instead of 0x00000304
> + * INTEL_AGPCTRL (0xb0); 0x00000000 instead of 0x00000080
> + * INTEL_APSIZE (0xb4);
> + * INTEL_ATTBASE (0xb8); 0x00000000 instead of 0x024b0000
> + * I815_ERRSTS?? (0xc8; undocumented for i815, see above); 0x00000000
> instead of 0x00000800
> + * ??? (0xe8); 0x1c700000 instead of 0x18500000
> + *
> + * Other machines/chipsets/BIOS versions may require
> + * a different set of registers to be properly saved.
> + */
> + static saved_regs i815_saved_regs[] = {
> + { I815_UNKNOWN_80, 0 },
> + { I815_GMCHCFG, 0 },
> + { I815_SM_RCOMP, 0 },
> + { I815_SM, 0 },
> + { I815_AGPCMD, 0 },
> + { INTEL_AGPCTRL, 0 },
> + { INTEL_APSIZE, 0 },
> + { INTEL_ATTBASE, 0 },
> + { I815_ERRSTS, 0 },
> + { I815_UNKNOWN_E8, 0 },
> + { 0, 0 }, /* DELIMITER */
> + };
> + saved_regs *p;
> +
> + if (restore) {
> + u32 val;
> + for (p = i815_saved_regs; (p->reg != 0); p++)
> + {
This goes to previous line. ";p->reg ;" is enough.
> + pci_read_config_dword(pdev, p->reg, &val);
> + if (val != p->value) {
> + printk(KERN_DEBUG "AGP: Writing back config
> space on "
> + "device %s at offset %x (was %x,
> writing %x)\n",
> + pci_name(pdev), p->reg,
> + val, p->value);
> + pci_write_config_dword(pdev, p->reg,
> + p->value);
> + }
> + }
> + } else {
> + for (p = i815_saved_regs; (p->reg != 0); p++)
Same here.
> + pci_read_config_dword(pdev, p->reg, &p->value);
> + }
> + return 1;
> +}
Should this betwo functions, one for save, one for restore, with "to
save" array being global?
> +/*
> + * set DEBUG_AGP_PM to 1 if your AGP chipset has issues resuming
> + * (machine lockups due to non-restored hardware registered values),
> + * then figure out from the log which registers have to be restored manually,
> + * then add specific support for your chipset, similar to what already exists
Can we get debugging stuff separately?
> @@ -2106,6 +2282,12 @@
> {
> if (agp_off)
> return -EINVAL;
> + /* let people know that this is an important place to investigate at in
> case of resume lockups */
> + printk(KERN_INFO PFX
> + "suspend/resume of intel-agp.ko is NOT always stable for all
> Intel AGP\n"
> + "chipset/BIOS combos, may cause resume lockups when DRI (3D
> accel) active,\n"
> + "in AGP (non-PCI) mode! (see DEBUG_AGP_PM in intel-agp.c source
> to investigate)\n"
> + );
I'm not sure if we want such big/ugly warnings. Can you get it to one
line, at least?
Otherwise it looks ok.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
