Re: [PATCH] mips: Add dma_mmap_coherent()

[Takashi Iwai <tiwai@suse.de>]

At Thu, 21 Aug 2008 08:55:12 -0500,
James Bottomley wrote:
> 
> On Thu, 2008-08-21 at 12:19 +0200, Takashi Iwai wrote:
> > At Wed, 20 Aug 2008 12:58:08 -0500,
> > James Bottomley wrote:
> > > 
> > > On Wed, 2008-08-20 at 18:53 +0200, Takashi Iwai wrote:
> > > > > I'm afraid there are several problems.  The first is that it doesn't do
> > > > > what you want.  You can't map a coherent page to userspace (which is at
> > > > > a non congruent address on parisc) and still expect it to be
> > > > > coherent ... there's going to have to be fiddling with the page table
> > > > > caches to make sure coherency isn't destroyed by aliasing effects
> > > > 
> > > > Hmm...  how bad would be the coherency with such a simple mmap method?
> > > > In most cases, we don't need the "perfect" coherency.  Usually one
> > > > process mmaps the whole buffer and keep reading/writing.  There is
> > > > another use case (sharing the mmapped buffer by multiple processes),
> > > > but this can be disabled if we know it's not feasible beforehand.
> > > 
> > > Unfortunately, the incoherency is between the user and the kernel.
> > > That's where the aliasing effects occur, so realistically, even though
> > > you've mapped coherent memory to the user, the coherency of that memory
> > > is only device <-> kernel.  When the any single user space process
> > > writes to it, the device won't see the write unless the user issues a
> > > flush.
> > 
> > I see.  In the case of ALSA mmap mode, a user issues an ioctl to
> > notify after the read/write access, so it'd be relatively easy to add
> > a sync operation.
> > 
> > Does the call of dma_sync_*_for_device() suffice for that purpose?
> 
> No ... dma_sync_* sync's from the kernel to the device ... you don't
> need that if the memory is already coherent.
> 
> The problem is that the data you want the device to see is held in a
> cache above the user space mapping ... it's that cache that has to be
> flushed (i.e. you need to flush through the user mappings, not through
> the kernel ones).
> 
> > (BTW, how does the fb driver work on this?)
> 
> It sets the shared page to uncached on all its mappings.  Turning off
> caching (assuming the platform can do it ... not all can) is a good way
> to eliminate aliasing issues.

Thanks for clarification.
How about the revised patch below (for PARISC)?


Takashi


diff --git a/arch/parisc/kernel/pci-dma.c b/arch/parisc/kernel/pci-dma.c
index ccd61b9..680b075 100644
--- a/arch/parisc/kernel/pci-dma.c
+++ b/arch/parisc/kernel/pci-dma.c
@@ -545,6 +545,20 @@ static void pa11_dma_sync_sg_for_device(struct device *dev, struct scatterlist *
 		flush_kernel_dcache_range(sg_virt_addr(sglist), sglist->length);
 }
 
+static int pa11_dma_mmap_coherent(struct device *dev,
+				  struct vm_area_struct *vma,
+				  void *cpu_addr, dma_addr_t handle,
+				  size_t size)
+{
+	struct page *pg;
+	pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
+	cpu_addr = __va(handle);
+	pg = virt_to_page(cpu_addr);
+	return remap_pfn_range(vma, vma->vm_start,
+			       page_to_pfn(pg) + vma->vm_pgoff,
+			       size, vma->vm_page_prot);
+}
+
 struct hppa_dma_ops pcxl_dma_ops = {
 	.dma_supported =	pa11_dma_supported,
 	.alloc_consistent =	pa11_dma_alloc_consistent,
@@ -558,6 +572,7 @@ struct hppa_dma_ops pcxl_dma_ops = {
 	.dma_sync_single_for_device = pa11_dma_sync_single_for_device,
 	.dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu,
 	.dma_sync_sg_for_device = pa11_dma_sync_sg_for_device,
+	.mmap_coherent =	pa11_dma_mmap_coherent,
 };
 
 static void *fail_alloc_consistent(struct device *dev, size_t size,
@@ -598,4 +613,5 @@ struct hppa_dma_ops pcx_dma_ops = {
 	.dma_sync_single_for_device =	pa11_dma_sync_single_for_device,
 	.dma_sync_sg_for_cpu =		pa11_dma_sync_sg_for_cpu,
 	.dma_sync_sg_for_device =	pa11_dma_sync_sg_for_device,
+	.mmap_coherent =	pa11_dma_mmap_coherent,
 };
diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
index b30e38f..dd2ab2c 100644
--- a/drivers/parisc/ccio-dma.c
+++ b/drivers/parisc/ccio-dma.c
@@ -1014,6 +1014,19 @@ ccio_unmap_sg(struct device *dev, struct scatterlist *sglist, int nents,
 	DBG_RUN_SG("%s() DONE (nents %d)\n", __func__, nents);
 }
 
+static int ccio_dma_mmap_coherent(struct device *dev,
+				  struct vm_area_struct *vma,
+				  void *cpu_addr, dma_addr_t handle,
+				  size_t size)
+{
+	struct page *pg;
+	pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
+	pg = virt_to_page(cpu_addr);
+	return remap_pfn_range(vma, vma->vm_start,
+			       page_to_pfn(pg) + vma->vm_pgoff,
+			       size, vma->vm_page_prot);
+}
+
 static struct hppa_dma_ops ccio_ops = {
 	.dma_supported =	ccio_dma_supported,
 	.alloc_consistent =	ccio_alloc_consistent,
@@ -1027,6 +1040,7 @@ static struct hppa_dma_ops ccio_ops = {
 	.dma_sync_single_for_device =	NULL,	/* NOP for U2/Uturn */
 	.dma_sync_sg_for_cpu =		NULL,	/* ditto */
 	.dma_sync_sg_for_device =		NULL,	/* ditto */
+	.mmap_coherent =	ccio_dma_mmap_coherent,
 };
 
 #ifdef CONFIG_PROC_FS
diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
index bc73b96..403d66d 100644
--- a/drivers/parisc/sba_iommu.c
+++ b/drivers/parisc/sba_iommu.c
@@ -1057,6 +1057,19 @@ sba_unmap_sg(struct device *dev, struct scatterlist *sglist, int nents,
 
 }
 
+static int sba_dma_mmap_coherent(struct device *dev,
+				 struct vm_area_struct *vma,
+				 void *cpu_addr, dma_addr_t handle,
+				 size_t size)
+{
+	struct page *pg;
+	pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE;
+	pg = virt_to_page(cpu_addr);
+	return remap_pfn_range(vma, vma->vm_start,
+			       page_to_pfn(pg) + vma->vm_pgoff,
+			       size, vma->vm_page_prot);
+}
+
 static struct hppa_dma_ops sba_ops = {
 	.dma_supported =	sba_dma_supported,
 	.alloc_consistent =	sba_alloc_consistent,
@@ -1070,6 +1083,7 @@ static struct hppa_dma_ops sba_ops = {
 	.dma_sync_single_for_device =	NULL,
 	.dma_sync_sg_for_cpu =		NULL,
 	.dma_sync_sg_for_device =	NULL,
+	.mmap_coherent =	sba_dma_mmap_coherent,
 };
 
 
diff --git a/include/asm-parisc/dma-mapping.h b/include/asm-parisc/dma-mapping.h
index 53af696..5b357b3 100644
--- a/include/asm-parisc/dma-mapping.h
+++ b/include/asm-parisc/dma-mapping.h
@@ -19,6 +19,9 @@ struct hppa_dma_ops {
 	void (*dma_sync_single_for_device)(struct device *dev, dma_addr_t iova, unsigned long offset, size_t size, enum dma_data_direction direction);
 	void (*dma_sync_sg_for_cpu)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
 	void (*dma_sync_sg_for_device)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction);
+	int (*mmap_coherent)(struct device *dev, struct vm_area_struct *vma,
+			     void *cpu_addr, dma_addr_t handle, size_t size);
+
 };
 
 /*
@@ -204,6 +207,15 @@ dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 		flush_kernel_dcache_range((unsigned long)vaddr, size);
 }
 
+static inline int
+dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma,
+		  void *cpu_addr, dma_addr_t handle, size_t size)
+{
+	if (!hppa_dma_ops->mmap_coherent)
+		return -ENXIO;
+	return hppa_dma_ops->mmap_coherent(dev, vma, cpu_addr, handle, size);
+}
+
 static inline void *
 parisc_walk_tree(struct device *dev)
 {