Re: [RFC] Make AGP work with IOMMU

Based on some patches from Zhenyu that I found at
http://people.freedesktop.org/~zhen/agp-mm-*, this set of patches stops
the Intel graphics drivers from being utterly broken when the IOMMU is
enabled...

       http://git.infradead.org/users/dwmw2/iommu-agp.git

I'm still seeing write faults to seemingly random addresses at startup
when I first enable the IOMMU (and before the gfx driver actually
starts). Those happen even when I don't use any graphics drivers at all,
though.

And I'm slightly confused about the way we use the scratch page without
mask_memory().

Looks good, some comments below:

commit d8450c7be46afd0b51d2dc171c9d2eb60366ce05
Author: Zhenyu Wang <zhenyu.z.wang@xxxxxxxxx>
Date:   Mon Jul 27 12:59:57 2009 +0100

   intel_agp: Use PCI DMA API correctly on chipsets new enough to have IOMMU

   When graphics dma remapping engine is active, we must fill
   gart table with dma address from dmar engine, as now graphics
   device access to graphics memory must go through dma remapping
   table to get real physical address.

   Add this support to all drivers which use intel_i915_insert_entries()

   Signed-off-by: Zhenyu Wang <zhenyu.z.wang@xxxxxxxxx>
   Signed-off-by: David Woodhouse <David.Woodhouse@xxxxxxxxx>

commit ca8cdb1f983c508f586b90870703e85aab87659a
Author: Zhenyu Wang <zhenyu.z.wang@xxxxxxxxx>
Date:   Thu Jul 23 17:25:49 2009 +0100

   agp: Add generic support for graphics dma remapping

   New driver hooks for support graphics memory dma remapping
   are introduced in this patch. It makes generic code can
   tell if current device needs dma remapping, then call driver
   provided interfaces for mapping and unmapping. Change has
   also been made to handle scratch_page in remapping case.

   Signed-off-by: Zhenyu Wang <zhenyu.z.wang@xxxxxxxxx>
   Signed-off-by: David Woodhouse <David.Woodhouse@xxxxxxxxx>

commit 42a509b9cd513ddd536c38cc302c7457a3c811d7
Author: David Woodhouse <David.Woodhouse@xxxxxxxxx>
Date:   Mon Jul 27 10:27:29 2009 +0100

   agp: Switch mask_memory() method to take address argument again, not page

   In commit 07613ba2 ("agp: switch AGP to use page array instead of
   unsigned long array") we switched the mask_memory() method to take a
   'struct page *' instead of an address. This is painful, because in some
   cases it has to be an IOMMU-mapped virtual bus address (in fact,
   shouldn't it _always_ be a dma_addr_t returned from pci_map_xxx(), and
   we just happen to get lucky most of the time?)

Yup pretty much we always got lucky, its not like AGP and IOMMU systems
are a huge item, its really only Intel IGPs which use the AGP
subsystem these days.




 struct agp_bridge_data *agp_generic_find_bridge(struct pci_dev *pdev);
diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
index ba9bde7..542a878 100644
--- a/drivers/char/agp/amd-k7-agp.c
+++ b/drivers/char/agp/amd-k7-agp.c
@@ -325,7 +325,9 @@ static int amd_insert_memory(struct agp_memory *mem, off_t pg_start, int type)
               addr = (j * PAGE_SIZE) + agp_bridge->gart_bus_addr;
               cur_gatt = GET_GATT(addr);
               writel(agp_generic_mask_memory(agp_bridge,
-                       mem->pages[i], mem->type), cur_gatt+GET_GATT_OFF(addr));
+                                              phys_to_gart(page_to_phys(mem->pages[i])),

don't suppose we want page_to_gart or is the double function nicer?

diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c
index cfa5a64..19ac366 100644
--- a/drivers/char/agp/backend.c
+++ b/drivers/char/agp/backend.c
@@ -150,8 +150,17 @@ static int agp_backend_initialize(struct agp_bridge_data *bridge)
               }

               bridge->scratch_page_real = phys_to_gart(page_to_phys(page));
-               bridge->scratch_page =
-                   bridge->driver->mask_memory(bridge, page, 0);
+               bridge->scratch_page = bridge->driver->mask_memory(bridge,
+                                          phys_to_gart(page_to_phys(page)), 0);
+
+               if (bridge->driver->agp_map_page &&
+                   bridge->driver->agp_map_page(phys_to_virt(page_to_phys(page)),

and maybe page_to_virt.


+#ifdef USE_PCI_DMA_API
+static int intel_agp_map_page(void *addr, dma_addr_t *ret)
+{
+       *ret = pci_map_single(intel_private.pcidev, addr,
+                             PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
+       if (pci_dma_mapping_error(intel_private.pcidev, *ret))
+               return -EINVAL;
+       return 0;
+}
+
+static void intel_agp_unmap_page(void *addr, dma_addr_t dma)
+{
+       pci_unmap_single(intel_private.pcidev, dma,
+                        PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
+}
+
+static int intel_agp_map_memory(struct agp_memory *mem)
+{
+       struct scatterlist *sg;
+       int i;
+
+       DBG("try mapping %lu pages\n", (unsigned long)mem->page_count);
+
+       if ((mem->page_count * sizeof(*mem->sg_list)) < 2*PAGE_SIZE)
+               mem->sg_list = kcalloc(mem->page_count, sizeof(*mem->sg_list),
+                                      GFP_KERNEL);
+
+       if (mem->sg_list == NULL) {
+               mem->sg_list = vmalloc(mem->page_count * sizeof(*mem->sg_list));
+               mem->sg_vmalloc_flag = 1;

Can we drop vmalloc_flag and use is_vmalloc_addr on the free function?

(aside: yet another place that wants a kmalloc/vmalloc allocator. I suspect
vmalloc here to be slow but I suppose there isn't much we can do.)

+       }
+
+       if (!mem->sg_list) {
+               mem->sg_vmalloc_flag = 0;
+               return -ENOMEM;
+       }
+       sg_init_table(mem->sg_list, mem->page_count);
+
+       sg = mem->sg_list;
+       for (i = 0 ; i < mem->page_count; i++, sg = sg_next(sg))
+               sg_set_page(sg, mem->pages[i], PAGE_SIZE, 0);
+
+       mem->num_sg = pci_map_sg(intel_private.pcidev, mem->sg_list,
+                                mem->page_count, PCI_DMA_BIDIRECTIONAL);
+       if (!mem->num_sg) {
+               if (mem->sg_vmalloc_flag)
+                       vfree(mem->sg_list);
+               else
+                       kfree(mem->sg_list);
+               mem->sg_list = NULL;
+               mem->sg_vmalloc_flag = 0;

some common cleanup function?

+               return -ENOMEM;
+       }
+       return 0;
+}
+
+static void intel_agp_unmap_memory(struct agp_memory *mem)
+{
+       DBG("try unmapping %lu pages\n", (unsigned long)mem->page_count);
+
+       pci_unmap_sg(intel_private.pcidev, mem->sg_list,
+                    mem->page_count, PCI_DMA_BIDIRECTIONAL);
+       if (mem->sg_vmalloc_flag)
+               vfree(mem->sg_list);
+       else
+               kfree(mem->sg_list);
+       mem->sg_vmalloc_flag = 0;
+       mem->sg_list = NULL;

since we reproduce it here.

Dave.
--
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/

Relevant Pages
Loading