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

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


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>

--
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

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, &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@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