Subject: kern/36239: agp_i810.c aperture size detection doesn't work
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: None <blair.sadewitz@gmail.com>
List: netbsd-bugs
Date: 04/28/2007 19:20:01
>Number: 36239
>Category: kern
>Synopsis: agp_i810.c aperture size detection doesn't work
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: kern-bug-people
>State: open
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Sat Apr 28 19:20:00 +0000 2007
>Originator: Blair Sadewitz
>Release: 4.99.18 -current
>Organization:
>Environment:
NetBSD syncope.no-ip.org 4.99.18 NetBSD 4.99.18 (SYNCOPE) #13: Fri Apr 27 10:40:51 EDT 2007 blair@syncope.no-ip.org:/usr/src/sys/arch/i386/compile/SYNCOPE i386
>Description:
The current code in agp_i810.c tries to detect the aperture by reading a PCI BAR. However, the aperture size isn't actually present there.
Conveniently, the GTT size is set by the chipset as aperture size / 1024.
The attached patch figures the aperture size using this method instead.
Also, I merged the stolen memory select...case statements for the I855 and I915 because they're the same. The I915 can have up to 64MB of stolen memory, so I added cases for 48MB and 64MB as well.
Additionally, I think the #if 0'd release of allocated memory for DRM should instead be determined by whether or not DRM AGP support is compiled in. Perhaps it could instead be whether or not the I915 driver is compiled in; that's up to you guys. If this call is excluded, however, attempting to use DRM on the i915 will result in a hard lock of the machine upon loading the X server.
I have tested this patch for both aperture size detection and DRM. My adapter, though, never seems to use more than 8MB of stolen memory, so I can't test that. That code's a no-brainer, though, so it should work.
I got the register values from FreeBSD.
The new Xorg intel driver also tries to do a 10-page allocation
for triple buffering and some other things. This fails with the current code, so I made a provision for that where previously one- or four-page allocations were allowed. Maybe this code should be refined? I'm not sure.
>How-To-Repeat:
Use a non-256MB aperture size on an i915, or try to use DRM. I marked this bug as 'serious' because X will not work with DRM the way agp_i810.c is now.
>Fix:
Index: agpreg.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/agpreg.h,v
retrieving revision 1.11
diff -u -r1.11 agpreg.h
--- agpreg.h 27 Mar 2007 00:34:16 -0000 1.11
+++ agpreg.h 28 Apr 2007 18:45:42 -0000
@@ -221,9 +221,8 @@
#define AGP_I915_GTTADR 0x1c
#define AGP_I915_GCC1 0x52
#define AGP_I915_GCC1_GMS 0x70
-#define AGP_I915_GCC1_GMS_STOLEN_0M 0x00
-#define AGP_I915_GCC1_GMS_STOLEN_1M 0x10
-#define AGP_I915_GCC1_GMS_STOLEN_8M 0x30
+#define AGP_I915_GCC1_GMS_STOLEN_48M 0x60
+#define AGP_I915_GCC1_GMS_STOLEN_64M 0x70
#define AGP_I915_MSAC 0x62
#define AGP_I915_MSAC_APER_128M 0x02
Index: agp_i810.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/agp_i810.c,v
retrieving revision 1.40
diff -u -r1.40 agp_i810.c
--- agp_i810.c 24 Mar 2007 23:02:24 -0000 1.40
+++ agp_i810.c 28 Apr 2007 18:45:42 -0000
@@ -87,6 +87,7 @@
bus_space_handle_t bsh; /* register bus_space handle */
bus_space_tag_t gtt_bst; /* GTT bus_space tag */
bus_space_handle_t gtt_bsh; /* GTT bus_space handle */
+ bus_size_t gtt_size; /* GTT size */
struct pci_attach_args vga_pa;
void *sc_powerhook;
@@ -228,7 +229,7 @@
}
error = pci_mapreg_map(&isc->vga_pa, AGP_I915_GTTADR,
PCI_MAPREG_TYPE_MEM, 0, &isc->gtt_bst, &isc->gtt_bsh,
- NULL, NULL);
+ NULL, &isc->gtt_size);
if (error != 0) {
aprint_error(": can't map gttadr registers\n");
/* XXX we should release mmadr here */
@@ -319,8 +320,10 @@
WRITE4(AGP_I810_PGTBL_CTL, pgtblctl);
gatt->ag_physical = pgtblctl & ~1;
- } else if (isc->chiptype == CHIP_I855) {
- /* The 855GM automatically initializes the 128k gatt on boot. */
+ } else if ((isc->chiptype == CHIP_I855) || (isc->chiptype == CHIP_I915))
+ {
+ /* The 855GM automatically initializes the 128k gatt on boot,
+ while the 915GM does so with a 256k gatt */
pcireg_t reg;
u_int32_t pgtblctl;
u_int16_t gcc1;
@@ -343,44 +346,16 @@
case AGP_I855_GCC1_GMS_STOLEN_32M:
isc->stolen = (32768 - 132) * 1024 / 4096;
break;
- default:
- isc->stolen = 0;
- aprint_error(
- ": unknown memory configuration, disabling\n");
- agp_generic_detach(sc);
- return EINVAL;
- }
- if (isc->stolen > 0) {
- aprint_error(": detected %dk stolen memory\n%s",
- isc->stolen * 4, sc->as_dev.dv_xname);
- }
-
- /* GATT address is already in there, make sure it's enabled */
- pgtblctl = READ4(AGP_I810_PGTBL_CTL);
- pgtblctl |= 1;
- WRITE4(AGP_I810_PGTBL_CTL, pgtblctl);
-
- gatt->ag_physical = pgtblctl & ~1;
- } else { /* CHIP_I915 */
- /* The 915G automatically initializes the 256k gatt on boot. */
- pcireg_t reg;
- u_int32_t pgtblctl;
- u_int16_t gcc1;
-
- reg = pci_conf_read(sc->as_pc, sc->as_tag, AGP_I915_GCC1);
- gcc1 = (u_int16_t)(reg >> 16);
- switch (gcc1 & AGP_I915_GCC1_GMS) {
- case AGP_I915_GCC1_GMS_STOLEN_0M:
- isc->stolen = 0;
+ case AGP_I915_GCC1_GMS_STOLEN_48M:
+ isc->stolen = (49152 - 132) * 1024 / 4096;
break;
- case AGP_I915_GCC1_GMS_STOLEN_1M:
- isc->stolen = (1024 - 260) * 1024 / 4096;
- break;
- case AGP_I915_GCC1_GMS_STOLEN_8M:
- isc->stolen = (8192 - 260) * 1024 / 4096;
+ case AGP_I915_GCC1_GMS_STOLEN_64M:
+ isc->stolen = (65536 - 132) * 1024 / 4096;
break;
default:
- isc->stolen = 0;
+ /* XXX I removed the I855 stolen 0 value in agpreg.h
+ because it is the default case. */
+ isc->stolen = 0;
aprint_error(
": unknown memory configuration, disabling\n");
agp_generic_detach(sc);
@@ -397,7 +372,7 @@
WRITE4(AGP_I810_PGTBL_CTL, pgtblctl);
gatt->ag_physical = pgtblctl & ~1;
- }
+ }
/*
* Make sure the chipset can see everything.
@@ -410,7 +385,12 @@
printf("%s: WARNING: unable to establish PCI power hook\n",
sc->as_dev.dv_xname);
-#if 0
+#if 1
+ /* XXX This should be something like:
+ * #if NDRMBASE > 0 (where NDRMBASE is defined by the presence of
+ * drm_agpsupport.c) Does that sound OK?
+ */
+
/*
* another device (drm) may need access to this region
* we do not need it anymore
@@ -482,15 +462,10 @@
return 128 * 1024 * 1024;
} else if (isc->chiptype == CHIP_I855) {
return 128 * 10+ } else { /* CHIP_I915: Aperture size is 1024 * GTT size as per
+ Eric Anholt */
+ return isc->gtt_size * 1024;
- reg = pci_conf_read(sc->as_pc, sc->as_tag, AGP_I915_MSAC);
- msac = (u_int16_t)(reg >> 16);
- if (msac & AGP_I915_MSAC_APER_128M)
- return 128 * 1024 * 1024;
- else
- return 256 * 1024 * 1024;
}
}
@@ -648,7 +623,7 @@
24 * 1024;
- } else { /* CHIP_I915 */
- u_int16_t msac;
Index: agpreg.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/agpreg.h,v
retrieving revision 1.11
diff -u -r1.11 agpreg.h
--- agpreg.h 27 Mar 2007 00:34:16 -0000 1.11
+++ agpreg.h 28 Apr 2007 18:45:42 -0000
@@ -221,9 +221,8 @@
#define AGP_I915_GTTADR 0x1c
#define AGP_I915_GCC1 0x52
#define AGP_I915_GCC1_GMS 0x70
-#define AGP_I915_GCC1_GMS_STOLEN_0M 0x00
-#define AGP_I915_GCC1_GMS_STOLEN_1M 0x10
-#define AGP_I915_GCC1_GMS_STOLEN_8M 0x30
+#define AGP_I915_GCC1_GMS_STOLEN_48M 0x60
+#define AGP_I915_GCC1_GMS_STOLEN_64M 0x70
#define AGP_I915_MSAC 0x62
#define AGP_I915_MSAC_APER_128M 0x02
Index: agp_i810.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/agp_i810.c,v
retrieving revision 1.40
diff -u -r1.40 agp_i810.c
--- agp_i810.c 24 Mar 2007 23:02:24 -0000 1.40
+++ agp_i810.c 28 Apr 2007 18:45:42 -0000
@@ -87,6 +87,7 @@
bus_space_handle_t bsh; /* register bus_space handle */
bus_space_tag_t gtt_bst; /* GTT bus_space tag */
bus_space_handle_t gtt_bsh; /* GTT bus_space handle */
+ bus_size_t gtt_size; /* GTT size */
struct pci_attach_args vga_pa;
void *sc_powerhook;
@@ -228,7 +229,7 @@
}
error = pci_mapreg_map(&isc->vga_pa, AGP_I915_GTTADR,
PCI_MAPREG_TYPE_MEM, 0, &isc->gtt_bst, &isc->gtt_bsh,
- NULL, NULL);
+ NULL, &isc->gtt_size);
if (error != 0) {
aprint_error(": can't map gttadr registers\n");
/* XXX we should release mmadr here */
@@ -319,8 +320,10 @@
WRITE4(AGP_I810_PGTBL_CTL, pgtblctl);
gatt->ag_physical = pgtblctl & ~1;
- } else if (isc->chiptype == CHIP_I855) {
- /* The 855GM automatically initializes the 128k gatt on boot. */
+ } else if ((isc->chiptype == CHIP_I855) || (isc->chiptype == CHIP_I915))
+ {
+ /* The 855GM automatically initializes the 128k gatt on boot,
+ while the 915GM does so with a 256k gatt */
pcireg_t reg;
u_int32_t pgtblctl;
u_int16_t gcc1;
@@ -343,44 +346,16 @@
case AGP_I855_GCC1_GMS_STOLEN_32M:
isc->stolen = (32768 - 132) * 1024 / 4096;
break;
- default:
- isc->stolen = 0;
- aprint_error(
- ": unknown memory configuration, disabling\n");
- agp_generic_detach(sc);
- return EINVAL;
- }
- if (isc->stolen > 0) {
- aprint_error(": detected %dk stolen memory\n%s",
- isc->stolen * 4, sc->as_dev.dv_xname);
- }
-
- /* GATT address is already in there, make sure it's enabled */
- pgtblctl = READ4(AGP_I810_PGTBL_CTL);
- pgtblctl |= 1;
- WRITE4(AGP_I810_PGTBL_CTL, pgtblctl);
-
- gatt->ag_physical = pgtblctl & ~1;
- } else { /* CHIP_I915 */
- /* The 915G automatically initializes the 256k gatt on boot. */
- pcireg_t reg;
- u_int32_t pgtblctl;
- u_int16_t gcc1;
-
- reg = pci_conf_read(sc->as_pc, sc->as_tag, AGP_I915_GCC1);
- gcc1 = (u_int16_t)(reg >> 16);
- switch (gcc1 & AGP_I915_GCC1_GMS) {
- case AGP_I915_GCC1_GMS_STOLEN_0M:
- isc->stolen = 0;
+ case AGP_I915_GCC1_GMS_STOLEN_48M:
+ isc->stolen = (49152 - 132) * 1024 / 4096;
break;
- case AGP_I915_GCC1_GMS_STOLEN_1M:
- isc->stolen = (1024 - 260) * 1024 / 4096;
- break;
- case AGP_I915_GCC1_GMS_STOLEN_8M:
- isc->stolen = (8192 - 260) * 1024 / 4096;
+ case AGP_I915_GCC1_GMS_STOLEN_64M:
+ isc->stolen = (65536 - 132) * 1024 / 4096;
break;
default:
- isc->stolen = 0;
+ /* XXX I removed the I855 stolen 0 value in agpreg.h
+ because it is the default case. */
+ isc->stolen = 0;
aprint_error(
": unknown memory configuration, disabling\n");
agp_generic_detach(sc);
@@ -397,7 +372,7 @@
WRITE4(AGP_I810_PGTBL_CTL, pgtblctl);
gatt->ag_physical = pgtblctl & ~1;
- }
+ }
gatt->ag_physical = pgtblctl & ~1;
- }
+ }
/*
* Make sure the chipset can see everything.
@@ -410,7 +385,12 @@
printf("%s: WARNING: unable to establish PCI power hook\n",
sc->as_dev.dv_xname);
-#if 0
+#if 1
+ /* XXX This should be something like:
+ * #if NDRMBASE > 0 (where NDRMBASE is defined by the presence of
+ * drm_agpsupport.c) Does that sound OK?
+ */
+
/*
* another device (drm) may need access to this region
* we do not need it anymore
@@ -482,15 +462,10 @@
return 128 * 1024 * 1024;
} else if (isc->chiptype == CHIP_I855) {
return 128 * 1024 * 1024;
- } else { /* CHIP_I915 */
- u_int16_t msac;
+ } else { /* CHIP_I915: Aperture size is 1024 * GTT size as per
+ Eric Anholt */
+ return isc->gtt_size * 1024;
- reg = pci_conf_read(sc->as_pc, sc->as_tag, AGP_I915_MSAC);
return 128 * 1024 * 1024;
- } else { /* CHIP_I915 */
- u_int16_t msac;
+ } else { /* CHIP_I915: Aperture size is 1024 * GTT size as per
+ Eric Anholt */
+ return isc->gtt_size * 1024;
- reg = pci_conf_read(sc->as_pc, sc->as_tag, AGP_I915_MSAC);
- msac = (u_int16_t)(reg >> 16);
- if (msac & AGP_I915_MSAC_APER_128M)
- return 128 * 1024 * 1024;
- else
- return 256 * 1024 * 1024;
}
}
@@ -648,7 +623,7 @@
/*
* Bogus mapping for the hardware cursor.
*/
- if (size != AGP_PAGE_SIZE && size != 4 * AGP_PAGE_SIZE)
+ if (size != AGP_PAGE_SIZE && size != 4 * AGP_PAGE_SIZE && size !
= 10 * AGP_PAGE_SIZE)
return 0;
}