[RFC] Make AGP work with IOMMU

[RFC] Make AGP work with IOMMU

[David Woodhouse]

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().

commit d8450c7be46afd0b51d2dc171c9d2eb60366ce05
Author: Zhenyu Wang <zhenyu.z.wang@intel.com>
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@intel.com>
    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

commit ca8cdb1f983c508f586b90870703e85aab87659a
Author: Zhenyu Wang <zhenyu.z.wang@intel.com>
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@intel.com>
    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

commit 42a509b9cd513ddd536c38cc302c7457a3c811d7
Author: David Woodhouse <David.Woodhouse@intel.com>
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?)
    
    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

diff --git a/drivers/char/agp/agp.h b/drivers/char/agp/agp.h
index 178e2e9..17e6d0d 100644
--- a/drivers/char/agp/agp.h
+++ b/drivers/char/agp/agp.h
@@ -107,7 +107,7 @@ struct agp_bridge_driver {
 	void (*agp_enable)(struct agp_bridge_data *, u32);
 	void (*cleanup)(void);
 	void (*tlb_flush)(struct agp_memory *);
-	unsigned long (*mask_memory)(struct agp_bridge_data *, struct page *, int);
+	unsigned long (*mask_memory)(struct agp_bridge_data *, dma_addr_t, int);
 	void (*cache_flush)(void);
 	int (*create_gatt_table)(struct agp_bridge_data *);
 	int (*free_gatt_table)(struct agp_bridge_data *);
@@ -121,6 +121,11 @@ struct agp_bridge_driver {
 	void (*agp_destroy_pages)(struct agp_memory *);
 	int (*agp_type_to_mask_type) (struct agp_bridge_data *, int);
 	void (*chipset_flush)(struct agp_bridge_data *);
+
+	int (*agp_map_page)(void *addr, dma_addr_t *ret);
+	void (*agp_unmap_page)(void *addr, dma_addr_t dma);
+	int (*agp_map_memory)(struct agp_memory *mem);
+	void (*agp_unmap_memory)(struct agp_memory *mem);
 };
 
 struct agp_bridge_data {
@@ -135,6 +140,7 @@ struct agp_bridge_data {
 	u32 *gatt_table_real;
 	unsigned long scratch_page;
 	unsigned long scratch_page_real;
+	dma_addr_t scratch_page_dma;
 	unsigned long gart_bus_addr;
 	unsigned long gatt_bus_addr;
 	u32 mode;
@@ -291,7 +297,7 @@ int agp_3_5_enable(struct agp_bridge_data *bridge);
 void global_cache_flush(void);
 void get_agp_version(struct agp_bridge_data *bridge);
 unsigned long agp_generic_mask_memory(struct agp_bridge_data *bridge,
-				      struct page *page, int type);
+				      dma_addr_t phys, int type);
 int agp_generic_type_to_mask_type(struct agp_bridge_data *bridge,
 				  int type);
 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])),
+					       mem->type),
+		       cur_gatt+GET_GATT_OFF(addr));
 		readl(cur_gatt+GET_GATT_OFF(addr));	/* PCI Posting. */
 	}
 	amd_irongate_tlbflush(mem);
diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index 3bf5dda..e85a5b3 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -79,7 +79,8 @@ static int amd64_insert_memory(struct agp_memory *mem, off_t pg_start, int type)
 
 	for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
 		tmp = agp_bridge->driver->mask_memory(agp_bridge,
-			mem->pages[i], mask_type);
+						      phys_to_gart(page_to_phys(mem->pages[i])),
+						      mask_type);
 
 		BUG_ON(tmp & 0xffffff0000000ffcULL);
 		pte = (tmp & 0x000000ff00000000ULL) >> 28;
diff --git a/drivers/char/agp/ati-agp.c b/drivers/char/agp/ati-agp.c
index 33656e1..59ebd60 100644
--- a/drivers/char/agp/ati-agp.c
+++ b/drivers/char/agp/ati-agp.c
@@ -302,7 +302,8 @@ static int ati_insert_memory(struct agp_memory * mem,
 		addr = (j * PAGE_SIZE) + agp_bridge->gart_bus_addr;
 		cur_gatt = GET_GATT(addr);
 		writel(agp_bridge->driver->mask_memory(agp_bridge,	
-						       mem->pages[i], mem->type),
+						       phys_to_gart(page_to_phys(mem->pages[i])),
+						       mem->type),
 		       cur_gatt+GET_GATT_OFF(addr));
 	}
 	readl(GET_GATT(agp_bridge->gart_bus_addr)); /* PCI posting */
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)),
+						&bridge->scratch_page_dma)) {
+			dev_err(&bridge->dev->dev,
+				"unable to dma-map scratch page\n");
+			rc = -ENOMEM;
+			goto err_out_nounmap;
+		}
 	}
 
 	size_value = bridge->driver->fetch_size();
@@ -191,6 +200,13 @@ static int agp_backend_initialize(struct agp_bridge_data *bridge)
 	return 0;
 
 err_out:
+	if (bridge->driver->needs_scratch_page &&
+	    bridge->driver->agp_unmap_page) {
+		void *va = gart_to_virt(bridge->scratch_page_real);
+
+		bridge->driver->agp_unmap_page(va, bridge->scratch_page_dma);
+	}
+err_out_nounmap:
 	if (bridge->driver->needs_scratch_page) {
 		void *va = gart_to_virt(bridge->scratch_page_real);
 
@@ -221,6 +237,10 @@ static void agp_backend_cleanup(struct agp_bridge_data *bridge)
 	    bridge->driver->needs_scratch_page) {
 		void *va = gart_to_virt(bridge->scratch_page_real);
 
+		if (bridge->driver->agp_unmap_page)
+			bridge->driver->agp_unmap_page(va,
+					       bridge->scratch_page_dma);
+
 		bridge->driver->agp_destroy_page(va, AGP_PAGE_DESTROY_UNMAP);
 		bridge->driver->agp_destroy_page(va, AGP_PAGE_DESTROY_FREE);
 	}
diff --git a/drivers/char/agp/generic.c b/drivers/char/agp/generic.c
index 1e8b461..28f0208 100644
--- a/drivers/char/agp/generic.c
+++ b/drivers/char/agp/generic.c
@@ -437,6 +437,12 @@ int agp_bind_memory(struct agp_memory *curr, off_t pg_start)
 		curr->bridge->driver->cache_flush();
 		curr->is_flushed = true;
 	}
+
+	if (curr->bridge->driver->agp_map_memory) {
+		ret_val = curr->bridge->driver->agp_map_memory(curr);
+		if (ret_val)
+			return ret_val;
+	}
 	ret_val = curr->bridge->driver->insert_memory(curr, pg_start, curr->type);
 
 	if (ret_val != 0)
@@ -478,6 +484,9 @@ int agp_unbind_memory(struct agp_memory *curr)
 	if (ret_val != 0)
 		return ret_val;
 
+	if (curr->bridge->driver->agp_unmap_memory)
+		curr->bridge->driver->agp_unmap_memory(curr);
+
 	curr->is_bound = false;
 	curr->pg_start = 0;
 	spin_lock(&curr->bridge->mapped_lock);
@@ -1132,7 +1141,9 @@ int agp_generic_insert_memory(struct agp_memory * mem, off_t pg_start, int type)
 	}
 
 	for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
-		writel(bridge->driver->mask_memory(bridge, mem->pages[i], mask_type),
+		writel(bridge->driver->mask_memory(bridge,
+						   phys_to_gart(page_to_phys(mem->pages[i])),
+						   mask_type),
 		       bridge->gatt_table+j);
 	}
 	readl(bridge->gatt_table+j-1);	/* PCI Posting. */
@@ -1347,9 +1358,8 @@ void global_cache_flush(void)
 EXPORT_SYMBOL(global_cache_flush);
 
 unsigned long agp_generic_mask_memory(struct agp_bridge_data *bridge,
-				      struct page *page, int type)
+				      dma_addr_t addr, int type)
 {
-	unsigned long addr = phys_to_gart(page_to_phys(page));
 	/* memory type is ignored in the generic routine */
 	if (bridge->driver->masks)
 		return addr | bridge->driver->masks[0].mask;
diff --git a/drivers/char/agp/hp-agp.c b/drivers/char/agp/hp-agp.c
index 8f3d4c1..64dbf4b 100644
--- a/drivers/char/agp/hp-agp.c
+++ b/drivers/char/agp/hp-agp.c
@@ -394,10 +394,8 @@ hp_zx1_remove_memory (struct agp_memory *mem, off_t pg_start, int type)
 }
 
 static unsigned long
-hp_zx1_mask_memory (struct agp_bridge_data *bridge,
-		    struct page *page, int type)
+hp_zx1_mask_memory (struct agp_bridge_data *bridge, dma_addr_t addr, int type)
 {
-	unsigned long addr = phys_to_gart(page_to_phys(page));
 	return HP_ZX1_PDIR_VALID_BIT | addr;
 }
 
diff --git a/drivers/char/agp/i460-agp.c b/drivers/char/agp/i460-agp.c
index 60cc35b..54191f8 100644
--- a/drivers/char/agp/i460-agp.c
+++ b/drivers/char/agp/i460-agp.c
@@ -61,7 +61,7 @@
 #define WR_FLUSH_GATT(index)	RD_GATT(index)
 
 static unsigned long i460_mask_memory (struct agp_bridge_data *bridge,
-				       unsigned long addr, int type);
+				       dma_addr_t addr, int type);
 
 static struct {
 	void *gatt;				/* ioremap'd GATT area */
@@ -546,20 +546,13 @@ static void i460_destroy_page (struct page *page, int flags)
 #endif /* I460_LARGE_IO_PAGES */
 
 static unsigned long i460_mask_memory (struct agp_bridge_data *bridge,
-				       unsigned long addr, int type)
+				       dma_addr_t addr, int type)
 {
 	/* Make sure the returned address is a valid GATT entry */
 	return bridge->driver->masks[0].mask
 		| (((addr & ~((1 << I460_IO_PAGE_SHIFT) - 1)) & 0xfffff000) >> 12);
 }
 
-static unsigned long i460_page_mask_memory(struct agp_bridge_data *bridge,
-					   struct page *page, int type)
-{
-	unsigned long addr = phys_to_gart(page_to_phys(page));
-	return i460_mask_memory(bridge, addr, type);
-}
-
 const struct agp_bridge_driver intel_i460_driver = {
 	.owner			= THIS_MODULE,
 	.aperture_sizes		= i460_sizes,
@@ -569,7 +562,7 @@ const struct agp_bridge_driver intel_i460_driver = {
 	.fetch_size		= i460_fetch_size,
 	.cleanup		= i460_cleanup,
 	.tlb_flush		= i460_tlb_flush,
-	.mask_memory		= i460_page_mask_memory,
+	.mask_memory		= i460_mask_memory,
 	.masks			= i460_masks,
 	.agp_enable		= agp_generic_enable,
 	.cache_flush		= global_cache_flush,
diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index 8c9d50d..20fe82b 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -10,6 +10,16 @@
 #include <linux/agp_backend.h>
 #include "agp.h"
 
+/*
+ * If we have Intel graphics, we're not going to have anything other than
+ * an Intel IOMMU. So make the correct use of the PCI DMA API contingent
+ * on the Intel IOMMU support (CONFIG_DMAR).
+ * Only newer chipsets need to bother with this, of course.
+ */
+#ifdef CONFIG_DMAR
+#define USE_PCI_DMA_API 1
+#endif
+
 #define PCI_DEVICE_ID_INTEL_E7221_HB	0x2588
 #define PCI_DEVICE_ID_INTEL_E7221_IG	0x258a
 #define PCI_DEVICE_ID_INTEL_82946GZ_HB      0x2970
@@ -170,6 +180,131 @@ static struct _intel_private {
 	int resource_valid;
 } intel_private;
 
+#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;
+	}
+
+	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;
+		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;
+	mem->num_sg = 0;
+}
+
+static void intel_agp_insert_sg_entries(struct agp_memory *mem,
+					off_t pg_start, int mask_type)
+{
+	struct scatterlist *sg;
+	int i, j;
+
+	j = pg_start;
+
+	WARN_ON(!mem->num_sg);
+
+	if (mem->num_sg == mem->page_count) {
+		for_each_sg(mem->sg_list, sg, mem->page_count, i) {
+			writel(agp_bridge->driver->mask_memory(agp_bridge,
+					sg_dma_address(sg), mask_type),
+					intel_private.gtt+j);
+			j++;
+		}
+	} else {
+		/* sg may merge pages, but we have to seperate
+		 * per-page addr for GTT */
+		unsigned int len, m;
+
+		for_each_sg(mem->sg_list, sg, mem->num_sg, i) {
+			len = sg_dma_len(sg) / PAGE_SIZE;
+			for (m = 0; m < len; m++) {
+				writel(agp_bridge->driver->mask_memory(agp_bridge,
+								       sg_dma_address(sg) + m * PAGE_SIZE,
+								       mask_type),
+				       intel_private.gtt+j);
+				j++;
+			}
+		}
+	}
+	readl(intel_private.gtt+j-1);
+}
+
+#else
+
+static void intel_agp_insert_sg_entries(struct agp_memory *mem,
+					off_t pg_start, int mask_type)
+{
+	int i, j;
+
+	for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
+		writel(agp_bridge->driver->mask_memory(agp_bridge,
+			       phys_to_gart(page_to_phys(mem->pages[i])), mask_type),
+		       intel_private.gtt+j);
+	}
+
+	readl(intel_private.gtt+j-1);
+}
+
+#endif
+
 static int intel_i810_fetch_size(void)
 {
 	u32 smram_miscc;
@@ -343,7 +478,7 @@ static int intel_i810_insert_entries(struct agp_memory *mem, off_t pg_start,
 			global_cache_flush();
 		for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
 			writel(agp_bridge->driver->mask_memory(agp_bridge,
-							       mem->pages[i],
+							       phys_to_gart(page_to_phys(mem->pages[i])),
 							       mask_type),
 			       intel_private.registers+I810_PTE_BASE+(j*4));
 		}
@@ -461,9 +596,8 @@ static void intel_i810_free_by_type(struct agp_memory *curr)
 }
 
 static unsigned long intel_i810_mask_memory(struct agp_bridge_data *bridge,
-					    struct page *page, int type)
+					    dma_addr_t addr, int type)
 {
-	unsigned long addr = phys_to_gart(page_to_phys(page));
 	/* Type checking must be done elsewhere */
 	return addr | bridge->driver->masks[type].mask;
 }
@@ -851,7 +985,7 @@ static int intel_i830_insert_entries(struct agp_memory *mem, off_t pg_start,
 
 	for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
 		writel(agp_bridge->driver->mask_memory(agp_bridge,
-						       mem->pages[i], mask_type),
+			       phys_to_gart(page_to_phys(mem->pages[i])), mask_type),
 		       intel_private.registers+I810_PTE_BASE+(j*4));
 	}
 	readl(intel_private.registers+I810_PTE_BASE+((j-1)*4));
@@ -1004,9 +1138,13 @@ static int intel_i915_configure(void)
 	writel(agp_bridge->gatt_bus_addr|I810_PGETBL_ENABLED, intel_private.registers+I810_PGETBL_CTL);
 	readl(intel_private.registers+I810_PGETBL_CTL);	/* PCI Posting. */
 
+#ifndef USE_PCI_DMA_API
+	agp_bridge->scratch_page_dma = agp_bridge->scratch_page;
+#endif
+
 	if (agp_bridge->driver->needs_scratch_page) {
 		for (i = intel_private.gtt_entries; i < current_size->num_entries; i++) {
-			writel(agp_bridge->scratch_page, intel_private.gtt+i);
+			writel(agp_bridge->scratch_page_dma, intel_private.gtt+i);
 		}
 		readl(intel_private.gtt+i-1);	/* PCI Posting. */
 	}
@@ -1039,7 +1177,7 @@ static void intel_i915_chipset_flush(struct agp_bridge_data *bridge)
 static int intel_i915_insert_entries(struct agp_memory *mem, off_t pg_start,
 				     int type)
 {
-	int i, j, num_entries;
+	int num_entries;
 	void *temp;
 	int ret = -EINVAL;
 	int mask_type;
@@ -1063,7 +1201,7 @@ static int intel_i915_insert_entries(struct agp_memory *mem, off_t pg_start,
 	if ((pg_start + mem->page_count) > num_entries)
 		goto out_err;
 
-	/* The i915 can't check the GTT for entries since its read only,
+	/* The i915 can't check the GTT for entries since it's read only;
 	 * depend on the caller to make the correct offset decisions.
 	 */
 
@@ -1079,12 +1217,7 @@ static int intel_i915_insert_entries(struct agp_memory *mem, off_t pg_start,
 	if (!mem->is_flushed)
 		global_cache_flush();
 
-	for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
-		writel(agp_bridge->driver->mask_memory(agp_bridge,
-						       mem->pages[i], mask_type), intel_private.gtt+j);
-	}
-
-	readl(intel_private.gtt+j-1);
+	intel_agp_insert_sg_entries(mem, pg_start, mask_type);
 	agp_bridge->driver->tlb_flush(mem);
 
  out:
@@ -1109,7 +1242,7 @@ static int intel_i915_remove_entries(struct agp_memory *mem, off_t pg_start,
 	}
 
 	for (i = pg_start; i < (mem->page_count + pg_start); i++)
-		writel(agp_bridge->scratch_page, intel_private.gtt+i);
+		writel(agp_bridge->scratch_page_dma, intel_private.gtt+i);
 
 	readl(intel_private.gtt+i-1);
 
@@ -1196,9 +1329,8 @@ static int intel_i915_create_gatt_table(struct agp_bridge_data *bridge)
  * this conditional.
  */
 static unsigned long intel_i965_mask_memory(struct agp_bridge_data *bridge,
-					    struct page *page, int type)
+					    dma_addr_t addr, int type)
 {
-	dma_addr_t addr = phys_to_gart(page_to_phys(page));
 	/* Shift high bits down */
 	addr |= (addr >> 28) & 0xf0;
 
@@ -2003,6 +2135,12 @@ static const struct agp_bridge_driver intel_915_driver = {
 	.agp_destroy_pages      = agp_generic_destroy_pages,
 	.agp_type_to_mask_type  = intel_i830_type_to_mask_type,
 	.chipset_flush		= intel_i915_chipset_flush,
+#ifdef USE_PCI_DMA_API
+	.agp_map_page		= intel_agp_map_page,
+	.agp_unmap_page		= intel_agp_unmap_page,
+	.agp_map_memory		= intel_agp_map_memory,
+	.agp_unmap_memory	= intel_agp_unmap_memory,
+#endif
 };
 
 static const struct agp_bridge_driver intel_i965_driver = {
@@ -2031,6 +2169,12 @@ static const struct agp_bridge_driver intel_i965_driver = {
 	.agp_destroy_pages      = agp_generic_destroy_pages,
 	.agp_type_to_mask_type	= intel_i830_type_to_mask_type,
 	.chipset_flush		= intel_i915_chipset_flush,
+#ifdef USE_PCI_DMA_API
+	.agp_map_page		= intel_agp_map_page,
+	.agp_unmap_page		= intel_agp_unmap_page,
+	.agp_map_memory		= intel_agp_map_memory,
+	.agp_unmap_memory	= intel_agp_unmap_memory,
+#endif
 };
 
 static const struct agp_bridge_driver intel_7505_driver = {
@@ -2085,6 +2229,12 @@ static const struct agp_bridge_driver intel_g33_driver = {
 	.agp_destroy_pages      = agp_generic_destroy_pages,
 	.agp_type_to_mask_type	= intel_i830_type_to_mask_type,
 	.chipset_flush		= intel_i915_chipset_flush,
+#ifdef USE_PCI_DMA_API
+	.agp_map_page		= intel_agp_map_page,
+	.agp_unmap_page		= intel_agp_unmap_page,
+	.agp_map_memory		= intel_agp_map_memory,
+	.agp_unmap_memory	= intel_agp_unmap_memory,
+#endif
 };
 
 static int find_gmch(u16 device)
diff --git a/drivers/char/agp/nvidia-agp.c b/drivers/char/agp/nvidia-agp.c
index 263d71d..cedacee 100644
--- a/drivers/char/agp/nvidia-agp.c
+++ b/drivers/char/agp/nvidia-agp.c
@@ -225,7 +225,7 @@ static int nvidia_insert_memory(struct agp_memory *mem, off_t pg_start, int type
 	}
 	for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
 		writel(agp_bridge->driver->mask_memory(agp_bridge,
-			mem->pages[i], mask_type),
+			       phys_to_gart(page_to_phys(mem->pages[i])), mask_type),
 			agp_bridge->gatt_table+nvidia_private.pg_offset+j);
 	}
 
diff --git a/drivers/char/agp/parisc-agp.c b/drivers/char/agp/parisc-agp.c
index f4bb43f..1c12921 100644
--- a/drivers/char/agp/parisc-agp.c
+++ b/drivers/char/agp/parisc-agp.c
@@ -32,7 +32,7 @@
 #define AGP8X_MODE		(1 << AGP8X_MODE_BIT)
 
 static unsigned long
-parisc_agp_mask_memory(struct agp_bridge_data *bridge, unsigned long addr,
+parisc_agp_mask_memory(struct agp_bridge_data *bridge, dma_addr_t addr,
 		       int type);
 
 static struct _parisc_agp_info {
@@ -189,20 +189,12 @@ parisc_agp_remove_memory(struct agp_memory *mem, off_t pg_start, int type)
 }
 
 static unsigned long
-parisc_agp_mask_memory(struct agp_bridge_data *bridge, unsigned long addr,
+parisc_agp_mask_memory(struct agp_bridge_data *bridge, dma_addr_t addr,
 		       int type)
 {
 	return SBA_PDIR_VALID_BIT | addr;
 }
 
-static unsigned long
-parisc_agp_page_mask_memory(struct agp_bridge_data *bridge, struct page *page,
-			    int type)
-{
-	unsigned long addr = phys_to_gart(page_to_phys(page));
-	return SBA_PDIR_VALID_BIT | addr;
-}
-
 static void
 parisc_agp_enable(struct agp_bridge_data *bridge, u32 mode)
 {
diff --git a/drivers/char/agp/sgi-agp.c b/drivers/char/agp/sgi-agp.c
index d3ea2e4..0d47fa8 100644
--- a/drivers/char/agp/sgi-agp.c
+++ b/drivers/char/agp/sgi-agp.c
@@ -70,10 +70,9 @@ static void sgi_tioca_tlbflush(struct agp_memory *mem)
  * entry.
  */
 static unsigned long
-sgi_tioca_mask_memory(struct agp_bridge_data *bridge,
-		      struct page *page, int type)
+sgi_tioca_mask_memory(struct agp_bridge_data *bridge, dma_addr_t addr,
+		      int type)
 {
-	unsigned long addr = phys_to_gart(page_to_phys(page));
 	return tioca_physpage_to_gart(addr);
 }
 
@@ -190,7 +189,8 @@ static int sgi_tioca_insert_memory(struct agp_memory *mem, off_t pg_start,
 
 	for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
 		table[j] =
-		    bridge->driver->mask_memory(bridge, mem->pages[i],
+		    bridge->driver->mask_memory(bridge,
+						phys_to_gart(page_to_phys(mem->pages[i])),
 						mem->type);
 	}
 
diff --git a/drivers/char/agp/sworks-agp.c b/drivers/char/agp/sworks-agp.c
index b964a21..0725995 100644
--- a/drivers/char/agp/sworks-agp.c
+++ b/drivers/char/agp/sworks-agp.c
@@ -349,7 +349,9 @@ static int serverworks_insert_memory(struct agp_memory *mem,
 	for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
 		addr = (j * PAGE_SIZE) + agp_bridge->gart_bus_addr;
 		cur_gatt = SVRWRKS_GET_GATT(addr);
-		writel(agp_bridge->driver->mask_memory(agp_bridge, mem->pages[i], mem->type), cur_gatt+GET_GATT_OFF(addr));
+		writel(agp_bridge->driver->mask_memory(agp_bridge, 
+				phys_to_gart(page_to_phys(mem->pages[i])), mem->type),
+		       cur_gatt+GET_GATT_OFF(addr));
 	}
 	serverworks_tlbflush(mem);
 	return 0;
diff --git a/include/linux/agp_backend.h b/include/linux/agp_backend.h
index 76fa794..8a294d6 100644
--- a/include/linux/agp_backend.h
+++ b/include/linux/agp_backend.h
@@ -79,9 +79,13 @@ struct agp_memory {
 	u32 physical;
 	bool is_bound;
 	bool is_flushed;
-        bool vmalloc_flag;
+	bool vmalloc_flag;
+	bool sg_vmalloc_flag;
 	/* list of agp_memory mapped to the aperture */
 	struct list_head mapped_list;
+	/* DMA-mapped addresses */
+	struct scatterlist *sg_list;
+	int num_sg;
 };
 
 #define AGP_NORMAL_MEMORY 0

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC] Make AGP work with IOMMU

[Dave Airlie]

> 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@intel.com>
> 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@intel.com>
>    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>
> commit ca8cdb1f983c508f586b90870703e85aab87659a
> Author: Zhenyu Wang <zhenyu.z.wang@intel.com>
> 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@intel.com>
>    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>
> commit 42a509b9cd513ddd536c38cc302c7457a3c811d7
> Author: David Woodhouse <David.Woodhouse@intel.com>
> 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.

Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Wed, 2009-07-29 at 16:28 +1000, Dave Airlie wrote:
> 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.

Ah, really? One thing which was bothering me was what happens when I use
non-onboard graphics in one of these beasts -- are individual gfx
drivers going to need to be fixed too?

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

I pondered that briefly. But then observed that phys_to_gart() and
gart_to_phys() _always_ describe an identity mapping, so perhaps they
could just be ditched completely?

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

That's called page_address(), and it (as well as the above construct) is
broken with highmem pages. It's actually OK here, since this page is
allocated with GFP_DMA32 -- but for cleanliness' sake I should probably
switch agp_map_page() to take a 'struct page *' rather than a virtual
address.

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

I suppose so -- we could eliminate the other vmalloc_flag field in
'struct agp_memory' that way too? Doesn't shrink the structure any --
we'd just end up with padding where the flags were.

> (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.)

http://lwn.net/Articles/342915/ ?

In fact, can't scatterlists do something like that already?

> > +       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?
 ...
> since we reproduce it here.

Yeah, probably a good idea.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC] Make AGP work with IOMMU

[Dave Airlie]

On Wed, Jul 29, 2009 at 5:15 PM, David Woodhouse<dwmw2@infradead.org> wrote:
> On Wed, 2009-07-29 at 16:28 +1000, Dave Airlie wrote:
>> 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.
>
> Ah, really? One thing which was bothering me was what happens when I use
> non-onboard graphics in one of these beasts -- are individual gfx
> drivers going to need to be fixed too?

Yes, more than likely. I think radeon and radeon kms drivers dtrt, at
least they
call pci_map_single for the pages they use in their on-chip translation units,
and the cards have been used in sparc64 and ppc64 with iommu stuff happening.

nouveau might work when it gets upstream but it may not.

Other PCI and/or PCIE1x cards still around would be MGA and SiS based,
and I suspect
they would need work, but they might just work.

nvidia/fglrx binary drivers, who knows.

>
>> >                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?
>
> I pondered that briefly. But then observed that phys_to_gart() and
> gart_to_phys() _always_ describe an identity mapping, so perhaps they
> could just be ditched completely?

Yeah that could work too, no idea why they were introduced, well before my time.

>
>> > @@ -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.
>
> That's called page_address(), and it (as well as the above construct) is
> broken with highmem pages. It's actually OK here, since this page is
> allocated with GFP_DMA32 -- but for cleanliness' sake I should probably
> switch agp_map_page() to take a 'struct page *' rather than a virtual
> address.

Yes that might be best, having a function called map_page taking an
address seems wrong.

>
>> > +       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?
>
> I suppose so -- we could eliminate the other vmalloc_flag field in
> 'struct agp_memory' that way too? Doesn't shrink the structure any --
> we'd just end up with padding where the flags were.

Yeah we should drop both of them maybe I can do that as a cleanup
after this patch.

>
>> (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.)
>
> http://lwn.net/Articles/342915/ ?
>
> In fact, can't scatterlists do something like that already?

no idea, I think I've seen vmalloc used in other places before.

>
>> > +       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?
>  ...
>> since we reproduce it here.
>
> Yeah, probably a good idea.
>
> --
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation
>
>

Dave.

Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Wed, 2009-07-29 at 18:43 +1000, Dave Airlie wrote:
> On Wed, Jul 29, 2009 at 5:15 PM, David Woodhouse<dwmw2@infradead.org> wrote:
> > On Wed, 2009-07-29 at 16:28 +1000, Dave Airlie wrote:
> >> 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.
> >
> > Ah, really? One thing which was bothering me was what happens when I use
> > non-onboard graphics in one of these beasts -- are individual gfx
> > drivers going to need to be fixed too?
> 
> Yes, more than likely. I think radeon and radeon kms drivers dtrt, at
> least they call pci_map_single for the pages they use in their on-chip
> translation units, and the cards have been used in sparc64 and ppc64
> with iommu stuff happening.

It's going to perform a lot better if we use pci_map_sg() to map as many
pages as possible at once.

> nouveau might work when it gets upstream but it may not.
> 
> Other PCI and/or PCIE1x cards still around would be MGA and SiS based,
> and I suspect they would need work, but they might just work.
>
> nvidia/fglrx binary drivers, who knows.

... or cares.

> >
> >> >                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?
> >
> > I pondered that briefly. But then observed that phys_to_gart() and
> > gart_to_phys() _always_ describe an identity mapping, so perhaps they
> > could just be ditched completely?
> 
> Yeah that could work too, no idea why they were introduced, well before my time.
> 
> >
> >> > @@ -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.
> >
> > That's called page_address(), and it (as well as the above construct) is
> > broken with highmem pages. It's actually OK here, since this page is
> > allocated with GFP_DMA32 -- but for cleanliness' sake I should probably
> > switch agp_map_page() to take a 'struct page *' rather than a virtual
> > address.
> 
> Yes that might be best, having a function called map_page taking an
> address seems wrong.

http://git.infradead.org/users/dwmw2/iommu-agp.git?a=commitdiff;h=71fc2ae5

> >
> >> > +       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?
> >
> > I suppose so -- we could eliminate the other vmalloc_flag field in
> > 'struct agp_memory' that way too? Doesn't shrink the structure any --
> > we'd just end up with padding where the flags were.
> 
> Yeah we should drop both of them maybe I can do that as a cleanup
> after this patch.
> 
> >
> >> (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.)
> >
> > http://lwn.net/Articles/342915/ ?
> >
> > In fact, can't scatterlists do something like that already?
> 
> no idea, I think I've seen vmalloc used in other places before.

This fixes it for scatterlists:
http://git.infradead.org/users/dwmw2/iommu-agp.git?a=commitdiff;h=89187864

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Wed, 2009-07-29 at 18:43 +1000, Dave Airlie wrote:
> >> don't suppose we want page_to_gart or is the double function nicer?
> >
> > I pondered that briefly. But then observed that phys_to_gart() and
> > gart_to_phys() _always_ describe an identity mapping, so perhaps they
> > could just be ditched completely?
> 
> Yeah that could work too, no idea why they were introduced, well before my time.

http://git.infradead.org/users/dwmw2/iommu-agp.git?a=commitdiff;h=8bf2f3a9

It was introduced by Keir in 2005 for Xen (commit 07eee78e). I suspect
it can die though, and that the right answer there is also to use the
DMA API correctly.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC] Make AGP work with IOMMU

[Zhenyu Wang]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

On 2009.07.29 17:36:45 +0800, David Woodhouse wrote:
> On Wed, 2009-07-29 at 18:43 +1000, Dave Airlie wrote:
> > >> don't suppose we want page_to_gart or is the double function nicer?
> > >
> > > I pondered that briefly. But then observed that phys_to_gart() and
> > > gart_to_phys() _always_ describe an identity mapping, so perhaps they
> > > could just be ditched completely?
> > 
> > Yeah that could work too, no idea why they were introduced, well before my time.
> 
> http://git.infradead.org/users/dwmw2/iommu-agp.git?a=commitdiff;h=8bf2f3a9
> 
> It was introduced by Keir in 2005 for Xen (commit 07eee78e). I suspect
> it can die though, and that the right answer there is also to use the
> DMA API correctly.
> 

The commits on that tree looks fine to me. Thanks David! 
I've also done some testing on G45 here, I've also seen the write
faults before graphics device init or after dma unmap pages. I tried
to look it up in my memo, which turns out one message like this should
be a BIOS bug that doesn't initialize the GTT table correctly. But I'm
quite sure about it now, better to ask our chipset people to verify if
this problem has been fixed or not.

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Mon, 2009-08-03 at 14:50 +0800, Zhenyu Wang wrote:
> 
> The commits on that tree looks fine to me. Thanks David! 
> I've also done some testing on G45 here, I've also seen the write
> faults before graphics device init or after dma unmap pages. I tried
> to look it up in my memo, which turns out one message like this should
> be a BIOS bug that doesn't initialize the GTT table correctly. But I'm
> quite sure about it now, better to ask our chipset people to verify if
> this problem has been fixed or not.

Thanks. I've rebased the tree so that it isn't based on my other iommu
changes, which are largely irrelevant. Since the drm-2.6.git tree as
advertised in MAINTAINERS seems to be a pristine 2.6.29-rc2, I've done
it based on 2.6.31-rc5 instead.

Dave, please pull from
	git://git.infradead.org/~dwmw2/iommu-agp.git

David Woodhouse (6):
      agp: Switch mask_memory() method to take address argument again, not page
      agp: tidy up handling of scratch pages w.r.t. DMA API
      agp: Switch agp_{un,}map_page() to take struct page * argument
      intel-agp: Move repeated sglist free into separate function
      intel-agp: fix sglist allocation to avoid vmalloc()
      agp: kill phys_to_gart() and gart_to_phys()

Zhenyu Wang (2):
      agp: Add generic support for graphics dma remapping
      intel_agp: Use PCI DMA API correctly on chipsets new enough to have IOMMU

 arch/alpha/include/asm/agp.h    |    4 -
 arch/ia64/include/asm/agp.h     |    4 -
 arch/parisc/include/asm/agp.h   |    4 -
 arch/powerpc/include/asm/agp.h  |    4 -
 arch/sparc/include/asm/agp.h    |    4 -
 arch/x86/include/asm/agp.h      |    4 -
 drivers/char/agp/agp.h          |   15 ++--
 drivers/char/agp/ali-agp.c      |    4 +-
 drivers/char/agp/amd-k7-agp.c   |   10 ++-
 drivers/char/agp/amd64-agp.c    |    7 +-
 drivers/char/agp/ati-agp.c      |    7 +-
 drivers/char/agp/backend.c      |   32 ++++++-
 drivers/char/agp/efficeon-agp.c |    4 +-
 drivers/char/agp/generic.c      |   20 ++++-
 drivers/char/agp/hp-agp.c       |    8 +-
 drivers/char/agp/i460-agp.c     |   17 +---
 drivers/char/agp/intel-agp.c    |  167 +++++++++++++++++++++++++++++++++++----
 drivers/char/agp/nvidia-agp.c   |    2 +-
 drivers/char/agp/parisc-agp.c   |   12 +---
 drivers/char/agp/sgi-agp.c      |    8 +-
 drivers/char/agp/sworks-agp.c   |   10 ++-
 drivers/char/agp/uninorth-agp.c |    2 +-
 include/linux/agp_backend.h     |    5 +-
 23 files changed, 247 insertions(+), 107 deletions(-)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Mon, 2009-08-03 at 14:50 +0800, Zhenyu Wang wrote:
> 
> The commits on that tree looks fine to me. Thanks David! 
> I've also done some testing on G45 here, I've also seen the write
> faults before graphics device init or after dma unmap pages. I tried
> to look it up in my memo, which turns out one message like this should
> be a BIOS bug that doesn't initialize the GTT table correctly. But I'm
> quite sure about it now, better to ask our chipset people to verify if
> this problem has been fixed or not.

Thanks. I've rebased the tree so that it isn't based on my other iommu
changes, which are largely irrelevant. Since the drm-2.6.git tree as
advertised in MAINTAINERS seems to be a pristine 2.6.29-rc2, I've done
it based on 2.6.31-rc5 instead.

Dave, please pull from
	git://git.infradead.org/~dwmw2/iommu-agp.git

David Woodhouse (6):
      agp: Switch mask_memory() method to take address argument again, not page
      agp: tidy up handling of scratch pages w.r.t. DMA API
      agp: Switch agp_{un,}map_page() to take struct page * argument
      intel-agp: Move repeated sglist free into separate function
      intel-agp: fix sglist allocation to avoid vmalloc()
      agp: kill phys_to_gart() and gart_to_phys()

Zhenyu Wang (2):
      agp: Add generic support for graphics dma remapping
      intel_agp: Use PCI DMA API correctly on chipsets new enough to have IOMMU

 arch/alpha/include/asm/agp.h    |    4 -
 arch/ia64/include/asm/agp.h     |    4 -
 arch/parisc/include/asm/agp.h   |    4 -
 arch/powerpc/include/asm/agp.h  |    4 -
 arch/sparc/include/asm/agp.h    |    4 -
 arch/x86/include/asm/agp.h      |    4 -
 drivers/char/agp/agp.h          |   15 ++--
 drivers/char/agp/ali-agp.c      |    4 +-
 drivers/char/agp/amd-k7-agp.c   |   10 ++-
 drivers/char/agp/amd64-agp.c    |    7 +-
 drivers/char/agp/ati-agp.c      |    7 +-
 drivers/char/agp/backend.c      |   32 ++++++-
 drivers/char/agp/efficeon-agp.c |    4 +-
 drivers/char/agp/generic.c      |   20 ++++-
 drivers/char/agp/hp-agp.c       |    8 +-
 drivers/char/agp/i460-agp.c     |   17 +---
 drivers/char/agp/intel-agp.c    |  167 +++++++++++++++++++++++++++++++++++----
 drivers/char/agp/nvidia-agp.c   |    2 +-
 drivers/char/agp/parisc-agp.c   |   12 +---
 drivers/char/agp/sgi-agp.c      |    8 +-
 drivers/char/agp/sworks-agp.c   |   10 ++-
 drivers/char/agp/uninorth-agp.c |    2 +-
 include/linux/agp_backend.h     |    5 +-
 23 files changed, 247 insertions(+), 107 deletions(-)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [Xen-devel] Re: [RFC] Make AGP work with IOMMU

[Jeremy Fitzhardinge]

On 07/29/09 02:36, David Woodhouse wrote:
> On Wed, 2009-07-29 at 18:43 +1000, Dave Airlie wrote:
>   
>>>> don't suppose we want page_to_gart or is the double function nicer?
>>>>         
>>> I pondered that briefly. But then observed that phys_to_gart() and
>>> gart_to_phys() _always_ describe an identity mapping, so perhaps they
>>> could just be ditched completely?
>>>       
>> Yeah that could work too, no idea why they were introduced, well before my time.
>>     
>
> http://git.infradead.org/users/dwmw2/iommu-agp.git?a=commitdiff;h=8bf2f3a9
>
> It was introduced by Keir in 2005 for Xen (commit 07eee78e). I suspect
> it can die though, and that the right answer there is also to use the
> DMA API correctly.
>   

And just when I have patches to use them for their original purpose...

Looking back over the thread, are you saying that most users are already
using the DMA API correctly for AGP accesses?  If that's true then we
should be just fine.

    J

Re: Re: [RFC] Make AGP work with IOMMU

[Jeremy Fitzhardinge]

On 07/29/09 02:36, David Woodhouse wrote:
> On Wed, 2009-07-29 at 18:43 +1000, Dave Airlie wrote:
>   
>>>> don't suppose we want page_to_gart or is the double function nicer?
>>>>         
>>> I pondered that briefly. But then observed that phys_to_gart() and
>>> gart_to_phys() _always_ describe an identity mapping, so perhaps they
>>> could just be ditched completely?
>>>       
>> Yeah that could work too, no idea why they were introduced, well before my time.
>>     
>
> http://git.infradead.org/users/dwmw2/iommu-agp.git?a=commitdiff;h=8bf2f3a9
>
> It was introduced by Keir in 2005 for Xen (commit 07eee78e). I suspect
> it can die though, and that the right answer there is also to use the
> DMA API correctly.
>   

And just when I have patches to use them for their original purpose...

Looking back over the thread, are you saying that most users are already
using the DMA API correctly for AGP accesses?  If that's true then we
should be just fine.

    J

Re: [Xen-devel] Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Tue, 2009-08-04 at 16:52 -0700, Jeremy Fitzhardinge wrote:
> And just when I have patches to use them for their original purpose...
> 
> Looking back over the thread, are you saying that most users are already
> using the DMA API correctly for AGP accesses?  If that's true then we
> should be just fine.

No, not 'most users'. But perhaps we should.

All we've done so far is make intel-agp use the DMA API correctly. And
that's conditional on CONFIG_DMAR. But we could make it unconditional,
and make the other drivers do it too. The code is all fairly generic.

Without an IOMMU, the overhead would be fairly minimal.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Tue, 2009-08-04 at 16:52 -0700, Jeremy Fitzhardinge wrote:
> And just when I have patches to use them for their original purpose...
> 
> Looking back over the thread, are you saying that most users are already
> using the DMA API correctly for AGP accesses?  If that's true then we
> should be just fine.

No, not 'most users'. But perhaps we should.

All we've done so far is make intel-agp use the DMA API correctly. And
that's conditional on CONFIG_DMAR. But we could make it unconditional,
and make the other drivers do it too. The code is all fairly generic.

Without an IOMMU, the overhead would be fairly minimal.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [Xen-devel] Re: [RFC] Make AGP work with IOMMU

[Dave Airlie]

On Wed, Aug 5, 2009 at 4:44 PM, David Woodhouse<dwmw2@infradead.org> wrote:
> On Tue, 2009-08-04 at 16:52 -0700, Jeremy Fitzhardinge wrote:
>> And just when I have patches to use them for their original purpose...
>>
>> Looking back over the thread, are you saying that most users are already
>> using the DMA API correctly for AGP accesses?  If that's true then we
>> should be just fine.
>
> No, not 'most users'. But perhaps we should.
>
> All we've done so far is make intel-agp use the DMA API correctly. And
> that's conditional on CONFIG_DMAR. But we could make it unconditional,
> and make the other drivers do it too. The code is all fairly generic.
>
> Without an IOMMU, the overhead would be fairly minimal.
>

I'm not sure how prevalent Xen is on AGP systems, I'm going to guess
not enough that anyone cares. Intel integrated chips are the only AGP
codebase user that are made any more, and they don't really use AGP
its just a legacy of the previous designs.

Dave.

Re: [Xen-devel] Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Wed, 2009-08-05 at 16:57 +1000, Dave Airlie wrote:
> On Wed, Aug 5, 2009 at 4:44 PM, David Woodhouse<dwmw2@infradead.org> wrote:
> > On Tue, 2009-08-04 at 16:52 -0700, Jeremy Fitzhardinge wrote:
> >> And just when I have patches to use them for their original purpose...
> >>
> >> Looking back over the thread, are you saying that most users are already
> >> using the DMA API correctly for AGP accesses?  If that's true then we
> >> should be just fine.
> >
> > No, not 'most users'. But perhaps we should.
> >
> > All we've done so far is make intel-agp use the DMA API correctly. And
> > that's conditional on CONFIG_DMAR. But we could make it unconditional,
> > and make the other drivers do it too. The code is all fairly generic.
> >
> > Without an IOMMU, the overhead would be fairly minimal.
> >
> 
> I'm not sure how prevalent Xen is on AGP systems, I'm going to guess
> not enough that anyone cares. Intel integrated chips are the only AGP
> codebase user that are made any more, and they don't really use AGP
> its just a legacy of the previous designs.

On the other hand, extending the existing code so that it uses the DMA
API correctly _unconditionally_ rather than only for the Intel driver
would be fairly simple and should be harmless...

-- 
dwmw2

Re: Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Wed, 2009-08-05 at 16:57 +1000, Dave Airlie wrote:
> On Wed, Aug 5, 2009 at 4:44 PM, David Woodhouse<dwmw2@infradead.org> wrote:
> > On Tue, 2009-08-04 at 16:52 -0700, Jeremy Fitzhardinge wrote:
> >> And just when I have patches to use them for their original purpose...
> >>
> >> Looking back over the thread, are you saying that most users are already
> >> using the DMA API correctly for AGP accesses?  If that's true then we
> >> should be just fine.
> >
> > No, not 'most users'. But perhaps we should.
> >
> > All we've done so far is make intel-agp use the DMA API correctly. And
> > that's conditional on CONFIG_DMAR. But we could make it unconditional,
> > and make the other drivers do it too. The code is all fairly generic.
> >
> > Without an IOMMU, the overhead would be fairly minimal.
> >
> 
> I'm not sure how prevalent Xen is on AGP systems, I'm going to guess
> not enough that anyone cares. Intel integrated chips are the only AGP
> codebase user that are made any more, and they don't really use AGP
> its just a legacy of the previous designs.

On the other hand, extending the existing code so that it uses the DMA
API correctly _unconditionally_ rather than only for the Intel driver
would be fairly simple and should be harmless...

-- 
dwmw2

Re: [Xen-devel] Re: [RFC] Make AGP work with IOMMU

[Jeremy Fitzhardinge]

On 08/04/09 23:57, Dave Airlie wrote:
>> All we've done so far is make intel-agp use the DMA API correctly. And
>> that's conditional on CONFIG_DMAR. But we could make it unconditional,
>> and make the other drivers do it too. The code is all fairly generic.
>>
>> Without an IOMMU, the overhead would be fairly minimal.
>>
>>     
>
> I'm not sure how prevalent Xen is on AGP systems, I'm going to guess
> not enough that anyone cares.

What do you mean by AGP here?  Do you mean "machine with physical AGP
port", or "machine with graphics using the AGP aperature"?  There's an
increasing interest in putting Xen on desktop machines, so the former is
debatable, but the latter (I think?) definitely isn't true.

>  Intel integrated chips are the only AGP
> codebase user that are made any more, and they don't really use AGP
> its just a legacy of the previous designs.
>
>   

Plenty of people are using older machines for testing, etc, so I
wouldn't want to exclude them too lightly.  I've got bug reports from
people using older Radeons, etc.

    J

Re: Re: [RFC] Make AGP work with IOMMU

[Jeremy Fitzhardinge]

On 08/04/09 23:57, Dave Airlie wrote:
>> All we've done so far is make intel-agp use the DMA API correctly. And
>> that's conditional on CONFIG_DMAR. But we could make it unconditional,
>> and make the other drivers do it too. The code is all fairly generic.
>>
>> Without an IOMMU, the overhead would be fairly minimal.
>>
>>     
>
> I'm not sure how prevalent Xen is on AGP systems, I'm going to guess
> not enough that anyone cares.

What do you mean by AGP here?  Do you mean "machine with physical AGP
port", or "machine with graphics using the AGP aperature"?  There's an
increasing interest in putting Xen on desktop machines, so the former is
debatable, but the latter (I think?) definitely isn't true.

>  Intel integrated chips are the only AGP
> codebase user that are made any more, and they don't really use AGP
> its just a legacy of the previous designs.
>
>   

Plenty of people are using older machines for testing, etc, so I
wouldn't want to exclude them too lightly.  I've got bug reports from
people using older Radeons, etc.

    J

Re: [Xen-devel] Re: [RFC] Make AGP work with IOMMU

[Dave Airlie]

On Thu, Aug 6, 2009 at 4:26 AM, Jeremy Fitzhardinge<jeremy@goop.org> wrote:
> On 08/04/09 23:57, Dave Airlie wrote:
>>> All we've done so far is make intel-agp use the DMA API correctly. And
>>> that's conditional on CONFIG_DMAR. But we could make it unconditional,
>>> and make the other drivers do it too. The code is all fairly generic.
>>>
>>> Without an IOMMU, the overhead would be fairly minimal.
>>>
>>>
>>
>> I'm not sure how prevalent Xen is on AGP systems, I'm going to guess
>> not enough that anyone cares.
>
> What do you mean by AGP here?  Do you mean "machine with physical AGP
> port", or "machine with graphics using the AGP aperature"?  There's an
> increasing interest in putting Xen on desktop machines, so the former is
> debatable, but the latter (I think?) definitely isn't true.
>

I split my mail along those lines already, an AGP system is one
with a physical AGP port, I think you'll probably find a few people
still running some Athlons with VIA chipset and radeon 9200s I think,

The graphics using AGP-like apertures are only Intel and we've fixed those.

>>  Intel integrated chips are the only AGP
>> codebase user that are made any more, and they don't really use AGP
>> its just a legacy of the previous designs.
>>
>>
>
> Plenty of people are using older machines for testing, etc, so I
> wouldn't want to exclude them too lightly.  I've got bug reports from
> people using older Radeons, etc.

My main problem with doing this is getting testing coverage on older
systems, if we change all the AGP drivers, the time to find we busted
something is probably not until the next major distro release with this code
in it. So people can run stuff on 5 year old hardware in 6 months? I'm
not sure the gains justify the costs, but if you guys really want to do it
I'll take the patch, just be prepared to have it ripped out the minute
we get a regression we can't figure out because we haven't got the hw anymore.

Dave.

Re: Re: [RFC] Make AGP work with IOMMU

[Dave Airlie]

On Thu, Aug 6, 2009 at 4:26 AM, Jeremy Fitzhardinge<jeremy@goop.org> wrote:
> On 08/04/09 23:57, Dave Airlie wrote:
>>> All we've done so far is make intel-agp use the DMA API correctly. And
>>> that's conditional on CONFIG_DMAR. But we could make it unconditional,
>>> and make the other drivers do it too. The code is all fairly generic.
>>>
>>> Without an IOMMU, the overhead would be fairly minimal.
>>>
>>>
>>
>> I'm not sure how prevalent Xen is on AGP systems, I'm going to guess
>> not enough that anyone cares.
>
> What do you mean by AGP here?  Do you mean "machine with physical AGP
> port", or "machine with graphics using the AGP aperature"?  There's an
> increasing interest in putting Xen on desktop machines, so the former is
> debatable, but the latter (I think?) definitely isn't true.
>

I split my mail along those lines already, an AGP system is one
with a physical AGP port, I think you'll probably find a few people
still running some Athlons with VIA chipset and radeon 9200s I think,

The graphics using AGP-like apertures are only Intel and we've fixed those.

>>  Intel integrated chips are the only AGP
>> codebase user that are made any more, and they don't really use AGP
>> its just a legacy of the previous designs.
>>
>>
>
> Plenty of people are using older machines for testing, etc, so I
> wouldn't want to exclude them too lightly.  I've got bug reports from
> people using older Radeons, etc.

My main problem with doing this is getting testing coverage on older
systems, if we change all the AGP drivers, the time to find we busted
something is probably not until the next major distro release with this code
in it. So people can run stuff on 5 year old hardware in 6 months? I'm
not sure the gains justify the costs, but if you guys really want to do it
I'll take the patch, just be prepared to have it ripped out the minute
we get a regression we can't figure out because we haven't got the hw anymore.

Dave.

Re: [Xen-devel] Re: [RFC] Make AGP work with IOMMU

[Jeremy Fitzhardinge]

On 08/05/09 16:12, Dave Airlie wrote:
> My main problem with doing this is getting testing coverage on older
> systems, if we change all the AGP drivers, the time to find we busted
> something is probably not until the next major distro release with this code
> in it. So people can run stuff on 5 year old hardware in 6 months? I'm
> not sure the gains justify the costs, but if you guys really want to do it
> I'll take the patch, just be prepared to have it ripped out the minute
> we get a regression we can't figure out because we haven't got the hw anymore.
>   

Can you point to some changesets/patches to act as an example/template
for the changes needed?

If we come up with someone reporting crashes when using old hardware, we
can then make change then get them to test both native and Xen.

    J

Re: Re: [RFC] Make AGP work with IOMMU

[Jeremy Fitzhardinge]

On 08/05/09 16:12, Dave Airlie wrote:
> My main problem with doing this is getting testing coverage on older
> systems, if we change all the AGP drivers, the time to find we busted
> something is probably not until the next major distro release with this code
> in it. So people can run stuff on 5 year old hardware in 6 months? I'm
> not sure the gains justify the costs, but if you guys really want to do it
> I'll take the patch, just be prepared to have it ripped out the minute
> we get a regression we can't figure out because we haven't got the hw anymore.
>   

Can you point to some changesets/patches to act as an example/template
for the changes needed?

If we come up with someone reporting crashes when using old hardware, we
can then make change then get them to test both native and Xen.

    J

Re: [Xen-devel] Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Thu, 2009-08-06 at 12:32 -0700, Jeremy Fitzhardinge wrote:
> On 08/05/09 16:12, Dave Airlie wrote:
> > My main problem with doing this is getting testing coverage on older
> > systems, if we change all the AGP drivers, the time to find we busted
> > something is probably not until the next major distro release with this code
> > in it. So people can run stuff on 5 year old hardware in 6 months? I'm
> > not sure the gains justify the costs, but if you guys really want to do it
> > I'll take the patch, just be prepared to have it ripped out the minute
> > we get a regression we can't figure out because we haven't got the hw anymore.
> >   
> 
> Can you point to some changesets/patches to act as an example/template
> for the changes needed?

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

Some of the code added to intel-agp should probably be generic.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: Re: [RFC] Make AGP work with IOMMU

[David Woodhouse]

On Thu, 2009-08-06 at 12:32 -0700, Jeremy Fitzhardinge wrote:
> On 08/05/09 16:12, Dave Airlie wrote:
> > My main problem with doing this is getting testing coverage on older
> > systems, if we change all the AGP drivers, the time to find we busted
> > something is probably not until the next major distro release with this code
> > in it. So people can run stuff on 5 year old hardware in 6 months? I'm
> > not sure the gains justify the costs, but if you guys really want to do it
> > I'll take the patch, just be prepared to have it ripped out the minute
> > we get a regression we can't figure out because we haven't got the hw anymore.
> >   
> 
> Can you point to some changesets/patches to act as an example/template
> for the changes needed?

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

Some of the code added to intel-agp should probably be generic.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation