On Sat, Dec 21, 2013 at 11:50 AM, Bjorn Helgaas <bhelg...@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 <bhelg...@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 <bhelg...@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 <bhelg...@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 <bhelg...@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-ying...@kernel.org
> Based-on-patch-by: Yinghai Lu <ying...@kernel.org>
> Signed-off-by: Bjorn Helgaas <bhelg...@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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
