Re: [PATCH v2 10/10] intel-gtt: Read 64bit for gmar_bus_addr

From: Bjorn Helgaas
Date: Sat Dec 21 2013 - 13:51:22 EST

[+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@xxxxxxxxxx> 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:


commit 8ba262d78f9d218672486c62ba6a1c7a073bd272
Author: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
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@xxxxxxxxxx>

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, &gtt_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@xxxxxxxxxx>
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@xxxxxxxxxx
Based-on-patch-by: Yinghai Lu <yinghai@xxxxxxxxxx>
Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/