From: Dave Jones <davej@redhat.com> Subject: [rhel5] AGP corruption fixes. Date: Wed, 22 Nov 2006 20:50:26 -0500 Bugzilla: 218107 Message-Id: <20061123015026.GA29538@redhat.com> Changelog: AGP corruption fixes. The i965 AGP support in RHEL5 has a really nasty data corruption problem that is solved by the two commits below. It only shows up on systems with >4GB of ram, or systems with 4GB + a memory hole that supports hoisting the remainder above the 4GB barrier. The long and short of it is that the AGP code can't deal with addresses this high, and strange things happen when it 'accidentally' occurs. (Linus found this completely by accident by noticing that a loop bzero'ing malloc'd memory started scribbling over his screen). The fix below does partially cripple some GART implementations (namely AMD64), which *could* support having a GATT above the 4GB mark -- now it's always forced to below 4GB. It's unlikely that this will ever be noticed as a problem, but will likely get a follow-on improvement upstream post 2.6.19. If we ever backport 965 support to RHEL4, we'll need the same change there. Horrible, horrible bug. Dave commit 7d915a38985d2826acbdc9dc9cca8a93e23e5278 tree 6954d28e1e835a9043c1f201c0791c390e528e7f parent b42172fc7b569a0ef2b0fa38d71382969074c0e2 author Linus Torvalds <torvalds@woody.osdl.org> 1164217074 -0800 committer Linus Torvalds <torvalds@woody.osdl.org> 1164217074 -0800 [AGP] Fix intel 965 AGP memory mapping function This introduces a i965-specific "mask_memory()" function that knows about the extended physical addresses that the i965 supports. This allows us to correctly map in physical memory in the >4GB range into the GTT. Also simplify/clean-up the i965 case for the aperture sizing by just returning the fixed 512kB size from "fetch_size()". We don't really care that not all of the aperture may be visible - the only thing that cares about the aperture size is the Intel "stolen memory" calculation, which depends on the fixed size. Cc: Keith Packard <keithp@keithp.com> Cc: Eric Anholt <eric@anholt.net> Cc: Dave Jones <davej@redhat.com> Signed-off-by: Linus Torvalds <torvalds@osdl.org> drivers/char/agp/intel-agp.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c index d1ede7d..aceece7 100644 --- a/drivers/char/agp/intel-agp.c +++ b/drivers/char/agp/intel-agp.c @@ -387,11 +387,7 @@ static void intel_i830_init_gtt_entries( /* We obtain the size of the GTT, which is also stored (for some * reason) at the top of stolen memory. Then we add 4KB to that * for the video BIOS popup, which is also stored in there. */ - - if (IS_I965) - size = 512 + 4; - else - size = agp_bridge->driver->fetch_size() + 4; + size = agp_bridge->driver->fetch_size() + 4; if (agp_bridge->dev->device == PCI_DEVICE_ID_INTEL_82830_HB || agp_bridge->dev->device == PCI_DEVICE_ID_INTEL_82845G_HB) { @@ -805,6 +801,26 @@ static int intel_i915_create_gatt_table( return 0; } + +/* + * The i965 supports 36-bit physical addresses, but to keep + * the format of the GTT the same, the bits that don't fit + * in a 32-bit word are shifted down to bits 4..7. + * + * Gcc is smart enough to notice that "(addr >> 28) & 0xf0" + * is always zero on 32-bit architectures, so no need to make + * this conditional. + */ +static unsigned long intel_i965_mask_memory(struct agp_bridge_data *bridge, + unsigned long addr, int type) +{ + /* Shift high bits down */ + addr |= (addr >> 28) & 0xf0; + + /* Type checking must be done elsewhere */ + return addr | bridge->driver->masks[type].mask; +} + static int intel_i965_fetch_size(void) { struct aper_size_info_fixed *values; @@ -832,7 +848,8 @@ static int intel_i965_fetch_size(void) agp_bridge->previous_size = agp_bridge->current_size = (void *)(values + offset); - return values[offset].size; + /* The i965 GTT is always sized as if it had a 512kB aperture size */ + return 512; } /* The intel i965 automatically initializes the agp aperture during POST. @@ -1584,7 +1601,7 @@ static struct agp_bridge_driver intel_i9 .fetch_size = intel_i965_fetch_size, .cleanup = intel_i915_cleanup, .tlb_flush = intel_i810_tlbflush, - .mask_memory = intel_i810_mask_memory, + .mask_memory = intel_i965_mask_memory, .masks = intel_i810_masks, .agp_enable = intel_i810_agp_enable, .cache_flush = global_cache_flush, commit 66c669baa7d70b8d135da67f36c8dba12cea71b8 tree 8839924101a47a08d9da70f9bfcd8abb4023e466 parent 7d915a38985d2826acbdc9dc9cca8a93e23e5278 author Linus Torvalds <torvalds@woody.osdl.org> 1164236129 -0800 committer Linus Torvalds <torvalds@woody.osdl.org> 1164236129 -0800 [AGP] Allocate AGP pages with GFP_DMA32 by default Not all graphic page remappers support physical addresses over the 4GB mark for remapping, so while some do (the AMD64 GART always did, and I just fixed the i965 to do so properly), we're safest off just forcing GFP_DMA32 allocations to make sure graphics pages get allocated in the low 32-bit address space by default. AGP sub-drivers that really care, and can do better, could just choose to implement their own allocator (or we could add another "64-bit safe" default allocator for their use), but quite frankly, you're not likely to care in practice. So for now, this trivial change means that we won't be allocating pages that we can't map correctly by mistake on x86-64. [ On traditional 32-bit x86, this could never happen, because GFP_KERNEL would never allocate any highmem memory anyway ] Acked-by: Andi Kleen <ak@suse.de> Acked-by: Dave Jones <davej@redhat.com> Cc: Eric Anholt <eric@anholt.net> Cc: Keith Packard <keithp@keithp.com> Signed-off-by: Linus Torvalds <torvalds@osdl.org> drivers/char/agp/generic.c | 2 +- drivers/char/agp/intel-agp.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c index c392001..5ff457b 100644 --- a/drivers/char/agp/generic.c +++ b/drivers/char/agp/generic.c @@ -1054,7 +1054,7 @@ void *agp_generic_alloc_page(struct agp_ { struct page * page; - page = alloc_page(GFP_KERNEL); + page = alloc_page(GFP_KERNEL | GFP_DMA32); if (page == NULL) return NULL; diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c index aceece7..555b3a8 100644 --- a/drivers/char/agp/intel-agp.c +++ b/drivers/char/agp/intel-agp.c @@ -169,7 +169,7 @@ static void *i8xx_alloc_pages(void) { struct page * page; - page = alloc_pages(GFP_KERNEL, 2); + page = alloc_pages(GFP_KERNEL | GFP_DMA32, 2); if (page == NULL) return NULL; -- http://www.codemonkey.org.uk