Message ID | 20131221185045.GA22728@google.com (mailing list archive) |
---|---|
State | New, archived |
Delegated to: | Bjorn Helgaas |
Headers | show |
On Sat, Dec 21, 2013 at 11:50 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > [+cc Daniel] > > On Fri, Dec 20, 2013 at 05:19:38PM -0800, Yinghai Lu wrote: >> On Fri, Dec 20, 2013 at 4:27 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >> > Why are we reading these BARs directly anyway? These look like >> > standard PCI BARs (I810_GMADDR == 0x10 and I915_GMADDR == 0x18), so >> > the PCI core should already be reading them correctly, shouldn't it? >> > Can't we just use pcibios_resource_to_bus(pci_resource_start())? >> > >> > It looks like i810_setup(), i830_setup(), and i9xx_setup() have the >> > same problem and should also be using pci_resource_start() or >> > something similar. >> >> Agreed. >> >> should be sth like: >> >> >> Index: linux-2.6/drivers/char/agp/intel-gtt.c >> =================================================================== >> --- linux-2.6.orig/drivers/char/agp/intel-gtt.c >> +++ linux-2.6/drivers/char/agp/intel-gtt.c >> @@ -608,9 +608,9 @@ static bool intel_gtt_can_wc(void) >> >> static int intel_gtt_init(void) >> { >> - u32 gma_addr; >> + struct pci_bus_region r; >> u32 gtt_map_size; >> - int ret; >> + int ret, idx; >> >> ret = intel_private.driver->setup(); >> if (ret != 0) >> @@ -660,13 +660,14 @@ static int intel_gtt_init(void) >> } >> >> if (INTEL_GTT_GEN <= 2) >> - pci_read_config_dword(intel_private.pcidev, I810_GMADDR, >> - &gma_addr); >> + idx = 0; /* I810_GMADDR */ >> else >> - pci_read_config_dword(intel_private.pcidev, I915_GMADDR, >> - &gma_addr); >> + idx = 2; /* I915_GMADDR */ >> >> - intel_private.gma_bus_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK); >> + pcibios_resource_to_bus(intel_private.pcidev->bus, &r, >> + &intel_private.pcidev->resource[idx]); >> + >> + intel_private.gma_bus_addr = r.start; >> >> return 0; >> } > > I think it's even worse than we first thought. Not only does the current > code fail on 64-bit BARs, but it also ignores the CPU/bus address > difference, and I think we want the CPU address. So I think we need > something like these: Please ignore the following patches. After looking again, I think "gma_bus_addr" really is a bus addresses after all, and something like Yinghai's patch is more appropriate. But I think there are other places with similar issues, so I'm working on a more extensive set of patches. Bjorn > commit 8ba262d78f9d218672486c62ba6a1c7a073bd272 > Author: Bjorn Helgaas <bhelgaas@google.com> > Date: Sat Dec 21 10:49:58 2013 -0700 > > agp/intel: Rename "*_bus_addr" to "*_phys_addr" > > We're dealing with CPU physical addresses here, which may be different from > bus addresses, so rename "gtt_bus_addr" to "gtt_phys_addr" and > "gma_bus_addr" to "gma_phys_addr" to avoid confusion. > > No functional change. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c > index b8e2014cb9cb..dca04a6aa7f8 100644 > --- a/drivers/char/agp/intel-gtt.c > +++ b/drivers/char/agp/intel-gtt.c > @@ -64,7 +64,7 @@ static struct _intel_private { > struct pci_dev *pcidev; /* device one */ > struct pci_dev *bridge_dev; > u8 __iomem *registers; > - phys_addr_t gtt_bus_addr; > + phys_addr_t gtt_phys_addr; > u32 PGETBL_save; > u32 __iomem *gtt; /* I915G */ > bool clear_fake_agp; /* on first access via agp, fill with scratch */ > @@ -78,7 +78,7 @@ static struct _intel_private { > int refcount; > /* Whether i915 needs to use the dmar apis or not. */ > unsigned int needs_dmar : 1; > - phys_addr_t gma_bus_addr; > + phys_addr_t gma_phys_addr; > /* Size of memory reserved for graphics by the BIOS */ > unsigned int stolen_size; > /* Total number of gtt entries. */ > @@ -191,7 +191,7 @@ static int i810_setup(void) > writel(virt_to_phys(gtt_table) | I810_PGETBL_ENABLED, > intel_private.registers+I810_PGETBL_CTL); > > - intel_private.gtt_bus_addr = reg_addr + I810_PTE_BASE; > + intel_private.gtt_phys_addr = reg_addr + I810_PTE_BASE; > > if ((readl(intel_private.registers+I810_DRAM_CTL) > & I810_DRAM_ROW_0) == I810_DRAM_ROW_0_SDRAM) { > @@ -636,10 +636,10 @@ static int intel_gtt_init(void) > > intel_private.gtt = NULL; > if (intel_gtt_can_wc()) > - intel_private.gtt = ioremap_wc(intel_private.gtt_bus_addr, > + intel_private.gtt = ioremap_wc(intel_private.gtt_phys_addr, > gtt_map_size); > if (intel_private.gtt == NULL) > - intel_private.gtt = ioremap(intel_private.gtt_bus_addr, > + intel_private.gtt = ioremap(intel_private.gtt_phys_addr, > gtt_map_size); > if (intel_private.gtt == NULL) { > intel_private.driver->cleanup(); > @@ -666,7 +666,7 @@ static int intel_gtt_init(void) > pci_read_config_dword(intel_private.pcidev, I915_GMADDR, > &gma_addr); > > - intel_private.gma_bus_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK); > + intel_private.gma_phys_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK); > > return 0; > } > @@ -796,7 +796,7 @@ static int i830_setup(void) > if (!intel_private.registers) > return -ENOMEM; > > - intel_private.gtt_bus_addr = reg_addr + I810_PTE_BASE; > + intel_private.gtt_phys_addr = reg_addr + I810_PTE_BASE; > > return 0; > } > @@ -821,7 +821,7 @@ static int intel_fake_agp_configure(void) > return -EIO; > > intel_private.clear_fake_agp = true; > - agp_bridge->gart_bus_addr = intel_private.gma_bus_addr; > + agp_bridge->gart_bus_addr = intel_private.gma_phys_addr; > > return 0; > } > @@ -1123,13 +1123,13 @@ static int i9xx_setup(void) > case 3: > pci_read_config_dword(intel_private.pcidev, > I915_PTEADDR, >t_addr); > - intel_private.gtt_bus_addr = gtt_addr; > + intel_private.gtt_phys_addr = gtt_addr; > break; > case 5: > - intel_private.gtt_bus_addr = reg_addr + MB(2); > + intel_private.gtt_phys_addr = reg_addr + MB(2); > break; > default: > - intel_private.gtt_bus_addr = reg_addr + KB(512); > + intel_private.gtt_phys_addr = reg_addr + KB(512); > break; > } > > @@ -1409,7 +1409,7 @@ void intel_gtt_get(size_t *gtt_total, size_t *stolen_size, > { > *gtt_total = intel_private.gtt_total_entries << PAGE_SHIFT; > *stolen_size = intel_private.stolen_size; > - *mappable_base = intel_private.gma_bus_addr; > + *mappable_base = intel_private.gma_phys_addr; > *mappable_end = intel_private.gtt_mappable_entries << PAGE_SHIFT; > } > EXPORT_SYMBOL(intel_gtt_get); > > commit 31349de4ce32a0c2c2d14df35717544e94d56066 > Author: Bjorn Helgaas <bhelgaas@google.com> > Date: Sat Dec 21 10:09:19 2013 -0700 > > intel-gtt: Use CPU address (not BAR value) for GART > > There were two problems here: > > 1) The GMADR can be either a 32-bit or a 64-bit BAR, but we only read the > low 32 bits, so this failed if GMADR was above 4GB. > > 2) The value read from the BAR is a bus address, not a CPU physical > address, and these may be different. > > Use pci_resource_start() instead of reading the BAR directly to remove the > BAR size and bus/CPU address issue, as gen8_gmch_probe() already does. > > Reference: http://lkml.kernel.org/r/1385429290-25397-11-git-send-email-yinghai@kernel.org > Based-on-patch-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c > index dca04a6aa7f8..35169ff0ffe1 100644 > --- a/drivers/char/agp/intel-gtt.c > +++ b/drivers/char/agp/intel-gtt.c > @@ -610,7 +610,7 @@ static int intel_gtt_init(void) > { > u32 gma_addr; > u32 gtt_map_size; > - int ret; > + int ret, bar; > > ret = intel_private.driver->setup(); > if (ret != 0) > @@ -660,14 +660,12 @@ static int intel_gtt_init(void) > } > > if (INTEL_GTT_GEN <= 2) > - pci_read_config_dword(intel_private.pcidev, I810_GMADDR, > - &gma_addr); > + bar = 0; /* I810_GMADDR */ > else > - pci_read_config_dword(intel_private.pcidev, I915_GMADDR, > - &gma_addr); > - > - intel_private.gma_phys_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK); > + bar = 2; /* I915_GMADDR */ > > + intel_private.gma_phys_addr = pci_resource_start(intel_private.pcidev, > + bar); > return 0; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c index b8e2014cb9cb..dca04a6aa7f8 100644 --- a/drivers/char/agp/intel-gtt.c +++ b/drivers/char/agp/intel-gtt.c @@ -64,7 +64,7 @@ static struct _intel_private { struct pci_dev *pcidev; /* device one */ struct pci_dev *bridge_dev; u8 __iomem *registers; - phys_addr_t gtt_bus_addr; + phys_addr_t gtt_phys_addr; u32 PGETBL_save; u32 __iomem *gtt; /* I915G */ bool clear_fake_agp; /* on first access via agp, fill with scratch */ @@ -78,7 +78,7 @@ static struct _intel_private { int refcount; /* Whether i915 needs to use the dmar apis or not. */ unsigned int needs_dmar : 1; - phys_addr_t gma_bus_addr; + phys_addr_t gma_phys_addr; /* Size of memory reserved for graphics by the BIOS */ unsigned int stolen_size; /* Total number of gtt entries. */ @@ -191,7 +191,7 @@ static int i810_setup(void) writel(virt_to_phys(gtt_table) | I810_PGETBL_ENABLED, intel_private.registers+I810_PGETBL_CTL); - intel_private.gtt_bus_addr = reg_addr + I810_PTE_BASE; + intel_private.gtt_phys_addr = reg_addr + I810_PTE_BASE; if ((readl(intel_private.registers+I810_DRAM_CTL) & I810_DRAM_ROW_0) == I810_DRAM_ROW_0_SDRAM) { @@ -636,10 +636,10 @@ static int intel_gtt_init(void) intel_private.gtt = NULL; if (intel_gtt_can_wc()) - intel_private.gtt = ioremap_wc(intel_private.gtt_bus_addr, + intel_private.gtt = ioremap_wc(intel_private.gtt_phys_addr, gtt_map_size); if (intel_private.gtt == NULL) - intel_private.gtt = ioremap(intel_private.gtt_bus_addr, + intel_private.gtt = ioremap(intel_private.gtt_phys_addr, gtt_map_size); if (intel_private.gtt == NULL) { intel_private.driver->cleanup(); @@ -666,7 +666,7 @@ static int intel_gtt_init(void) pci_read_config_dword(intel_private.pcidev, I915_GMADDR, &gma_addr); - intel_private.gma_bus_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK); + intel_private.gma_phys_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK); return 0; } @@ -796,7 +796,7 @@ static int i830_setup(void) if (!intel_private.registers) return -ENOMEM; - intel_private.gtt_bus_addr = reg_addr + I810_PTE_BASE; + intel_private.gtt_phys_addr = reg_addr + I810_PTE_BASE; return 0; } @@ -821,7 +821,7 @@ static int intel_fake_agp_configure(void) return -EIO; intel_private.clear_fake_agp = true; - agp_bridge->gart_bus_addr = intel_private.gma_bus_addr; + agp_bridge->gart_bus_addr = intel_private.gma_phys_addr; return 0; } @@ -1123,13 +1123,13 @@ static int i9xx_setup(void) case 3: pci_read_config_dword(intel_private.pcidev, I915_PTEADDR, >t_addr); - intel_private.gtt_bus_addr = gtt_addr; + intel_private.gtt_phys_addr = gtt_addr; break; case 5: - intel_private.gtt_bus_addr = reg_addr + MB(2); + intel_private.gtt_phys_addr = reg_addr + MB(2); break; default: - intel_private.gtt_bus_addr = reg_addr + KB(512); + intel_private.gtt_phys_addr = reg_addr + KB(512); break; } @@ -1409,7 +1409,7 @@ void intel_gtt_get(size_t *gtt_total, size_t *stolen_size, { *gtt_total = intel_private.gtt_total_entries << PAGE_SHIFT; *stolen_size = intel_private.stolen_size; - *mappable_base = intel_private.gma_bus_addr; + *mappable_base = intel_private.gma_phys_addr; *mappable_end = intel_private.gtt_mappable_entries << PAGE_SHIFT; } EXPORT_SYMBOL(intel_gtt_get); commit 31349de4ce32a0c2c2d14df35717544e94d56066 Author: Bjorn Helgaas <bhelgaas@google.com> Date: Sat Dec 21 10:09:19 2013 -0700 intel-gtt: Use CPU address (not BAR value) for GART There were two problems here: 1) The GMADR can be either a 32-bit or a 64-bit BAR, but we only read the low 32 bits, so this failed if GMADR was above 4GB. 2) The value read from the BAR is a bus address, not a CPU physical address, and these may be different. Use pci_resource_start() instead of reading the BAR directly to remove the BAR size and bus/CPU address issue, as gen8_gmch_probe() already does. Reference: http://lkml.kernel.org/r/1385429290-25397-11-git-send-email-yinghai@kernel.org Based-on-patch-by: Yinghai Lu <yinghai@kernel.org> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c index dca04a6aa7f8..35169ff0ffe1 100644 --- a/drivers/char/agp/intel-gtt.c +++ b/drivers/char/agp/intel-gtt.c @@ -610,7 +610,7 @@ static int intel_gtt_init(void) { u32 gma_addr; u32 gtt_map_size; - int ret; + int ret, bar; ret = intel_private.driver->setup(); if (ret != 0) @@ -660,14 +660,12 @@ static int intel_gtt_init(void) } if (INTEL_GTT_GEN <= 2) - pci_read_config_dword(intel_private.pcidev, I810_GMADDR, - &gma_addr); + bar = 0; /* I810_GMADDR */ else - pci_read_config_dword(intel_private.pcidev, I915_GMADDR, - &gma_addr); - - intel_private.gma_phys_addr = (gma_addr & PCI_BASE_ADDRESS_MEM_MASK); + bar = 2; /* I915_GMADDR */ + intel_private.gma_phys_addr = pci_resource_start(intel_private.pcidev, + bar); return 0; }