Re: [PATCH] x86-64: trivial gart clean-up

Re: [PATCH] x86-64: trivial gart clean-up

[Andi Kleen]

On Tuesday 25 April 2006 00:53, Jon Mason wrote:
> A trivial change to have gart_unmap_sg call gart_unmap_single directly,
> instead of bouncing through the dma_unmap_single wrapper in
> dma-mapping.h.  This change required moving the gart_unmap_single above
> gart_unmap_sg, and under gart_map_single (which seems a more logical
> place that its current location IMHO).

What advantage does that have? I think I prefer the old code.

-Andi

[PATCH] x86-64: trivial gart clean-up

[Jon Mason]

A trivial change to have gart_unmap_sg call gart_unmap_single directly,
instead of bouncing through the dma_unmap_single wrapper in
dma-mapping.h.  This change required moving the gart_unmap_single above
gart_unmap_sg, and under gart_map_single (which seems a more logical
place that its current location IMHO).

Signed-off-by: Jon Mason <jdmason@us.ibm.com>

diff -r 742849676406 arch/x86_64/kernel/pci-gart.c
--- a/arch/x86_64/kernel/pci-gart.c	Sun Apr 23 16:44:10 2006
+++ b/arch/x86_64/kernel/pci-gart.c	Mon Apr 24 15:17:52 2006
@@ -289,6 +289,28 @@
 }
 
 /*
+ * Free a DMA mapping.
+ */ 
+void gart_unmap_single(struct device *dev, dma_addr_t dma_addr,
+		      size_t size, int direction)
+{
+	unsigned long iommu_page; 
+	int npages;
+	int i;
+
+	if (dma_addr < iommu_bus_base + EMERGENCY_PAGES*PAGE_SIZE || 
+	    dma_addr >= iommu_bus_base + iommu_size)
+		return;
+	iommu_page = (dma_addr - iommu_bus_base)>>PAGE_SHIFT;	
+	npages = to_pages(dma_addr, size);
+	for (i = 0; i < npages; i++) { 
+		iommu_gatt_base[iommu_page + i] = gart_unmapped_entry; 
+		CLEAR_LEAK(iommu_page + i);
+	}
+	free_iommu(iommu_page, npages);
+}
+
+/*
  * Wrapper for pci_unmap_single working with scatterlists.
  */
 void gart_unmap_sg(struct device *dev, struct scatterlist *sg, int nents, int dir)
@@ -299,7 +321,7 @@
 		struct scatterlist *s = &sg[i];
 		if (!s->dma_length || !s->length)
 			break;
-		dma_unmap_single(dev, s->dma_address, s->dma_length, dir);
+		gart_unmap_single(dev, s->dma_address, s->dma_length, dir);
 	}
 }
 
@@ -457,28 +479,6 @@
 		sg[i].dma_address = bad_dma_address;
 	return 0;
 } 
-
-/*
- * Free a DMA mapping.
- */ 
-void gart_unmap_single(struct device *dev, dma_addr_t dma_addr,
-		      size_t size, int direction)
-{
-	unsigned long iommu_page; 
-	int npages;
-	int i;
-
-	if (dma_addr < iommu_bus_base + EMERGENCY_PAGES*PAGE_SIZE || 
-	    dma_addr >= iommu_bus_base + iommu_size)
-		return;
-	iommu_page = (dma_addr - iommu_bus_base)>>PAGE_SHIFT;	
-	npages = to_pages(dma_addr, size);
-	for (i = 0; i < npages; i++) { 
-		iommu_gatt_base[iommu_page + i] = gart_unmapped_entry; 
-		CLEAR_LEAK(iommu_page + i);
-	}
-	free_iommu(iommu_page, npages);
-}
 
 static int no_agp;
 

Re: [PATCH] x86-64: trivial gart clean-up

[Muli Ben-Yehuda]

On Tue, Apr 25, 2006 at 12:42:43AM +0200, Andi Kleen wrote:

> On Tuesday 25 April 2006 00:53, Jon Mason wrote:
> > A trivial change to have gart_unmap_sg call gart_unmap_single directly,
> > instead of bouncing through the dma_unmap_single wrapper in
> > dma-mapping.h.  This change required moving the gart_unmap_single above
> > gart_unmap_sg, and under gart_map_single (which seems a more logical
> > place that its current location IMHO).
> 
> What advantage does that have? I think I prefer the old code.

I don't know what Jon had in mind, but we do avoid a call through a
function pointer this way. I agree with Jon that it also makes more
sense - gart code can just call the gart code directly, without going
through the dma_xxx wrapper that ends up calling it anyway.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: [PATCH] x86-64: trivial gart clean-up

[Jon Mason]

On Tue, Apr 25, 2006 at 08:26:07AM +0300, Muli Ben-Yehuda wrote:
> On Tue, Apr 25, 2006 at 12:42:43AM +0200, Andi Kleen wrote:
> 
> > On Tuesday 25 April 2006 00:53, Jon Mason wrote:
> > > A trivial change to have gart_unmap_sg call gart_unmap_single directly,
> > > instead of bouncing through the dma_unmap_single wrapper in
> > > dma-mapping.h.  This change required moving the gart_unmap_single above
> > > gart_unmap_sg, and under gart_map_single (which seems a more logical
> > > place that its current location IMHO).
> > 
> > What advantage does that have? I think I prefer the old code.
> 
> I don't know what Jon had in mind, but we do avoid a call through a
> function pointer this way. I agree with Jon that it also makes more
> sense - gart code can just call the gart code directly, without going
> through the dma_xxx wrapper that ends up calling it anyway.

Yes, that is exactly what I was trying to say in my comment above.
Sorry for the confusion, and thanks for clearing it up Muli.

The dma_unmap_single call is the only dma_XXX type call in the gart
code.  All other calls use the gart_XXX equivalent.  It seems to me that
this was an oversight.  Also, the dma_XXX type calls go through a
wrapper in asm/dma-mapping.h, which translates this call to the gart
equivalent.  It also makes the code 8 bytes smaller :)

Thanks,
Jon

> 
> Cheers,
> Muli
> -- 
> Muli Ben-Yehuda
> http://www.mulix.org | http://mulix.livejournal.com/
> 

Re: [PATCH] x86-64: trivial gart clean-up

[Andi Kleen]

On Tuesday 25 April 2006 00:53, Jon Mason wrote:
> A trivial change to have gart_unmap_sg call gart_unmap_single directly,
> instead of bouncing through the dma_unmap_single wrapper in
> dma-mapping.h.  This change required moving the gart_unmap_single above
> gart_unmap_sg, and under gart_map_single (which seems a more logical
> place that its current location IMHO).

Merged thanks.
-Andi