Skip to contentSkip to conversation

Query on patch to be upstream ?

PATCH Superseded Ritesh Harjani 4 participants 18 messages Mar 5, 2014 – Mar 28, 2014 1 message not in the archive arm64

lore
Share & tools
lore

Ritesh Harjani

author
#0 lore raw

Hi Catalin,

Your Patch (arm64: Implement coherent DMA API based on swiotlb) from your devel branch going upstream (without modifications) ?

Thanks
Ritesh

maintainer
#1 lore raw
Quoted 2 lines · Ritesh Harjani

Your Patch (arm64: Implement coherent DMA API based on swiotlb) from your devel branch going upstream (without modifications) ?

That's the plan. But once we get the system topology or Santosh's dma I coherent DT bindings plan to make it DT-driven on whether a device is coherent or not. For now we need some solution as I get people asking about cache management for DMA buffers (though none of them got back with a Tested-by ;)).

Signature · 1 line
Catalin
#2 lore raw

With one caveat below, you can add

Tested-by Laura Abbott <[email protected]>

for "arm64: Implement coherent DMA API based on swiotlb"

Thanks,
Laura

-- 8< --
From: Laura Abbott <[email protected]>
Date: Thu, 6 Mar 2014 21:05:41 -0800
Subject: [PATCH] arm64: Use custom mmap function for noncoherent dma ops

The non-coherent dma ops remap memory with appropriate attributes. This remapped address cannot be used with virt_to_page which dma_common_mmap uses. Implement a custom (but very similar) function which correctly calculates the physical address and remaps to userspace.

Signed-off-by Laura Abbott <[email protected]>
---
 arch/arm64/mm/dma-mapping.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)
arch/arm64/mm/dma-mapping.c +27 -0
index e923a5b..5ac7a14 100644
@@ -194,9 +194,36 @@ static void arm64_swiotlb_sync_sg_for_device(struct device *dev,
 			       sg->length, dir);
 }
 
+int arm64_swiotlb_mmap(struct device *dev, struct vm_area_struct *vma,
+		 void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		 struct dma_attrs *attrs)
+{
+	int ret = -ENXIO;
+	unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >>
+					PAGE_SHIFT;
+	unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT;
+	unsigned long off = vma->vm_pgoff;
+
+	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
+
+	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
+		return ret;
+
+	if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) {
+		ret = remap_pfn_range(vma, vma->vm_start,
+				      pfn + off,
+				      vma->vm_end - vma->vm_start,
+				      vma->vm_page_prot);
+	}
+
+	return ret;
+}
+
 struct dma_map_ops noncoherent_swiotlb_dma_ops = {
 	.alloc = arm64_swiotlb_alloc_noncoherent,
 	.free = arm64_swiotlb_free_noncoherent,
+	.mmap = arm64_swiotlb_mmap,
 	.map_page = arm64_swiotlb_map_page,
 	.unmap_page = arm64_swiotlb_unmap_page,
 	.map_sg = arm64_swiotlb_map_sg_attrs,
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
maintainer
#3 lore raw
Quoted 3 lines · Laura Abbott

With one caveat below, you can add

Tested-by Laura Abbott <[email protected]>

Thanks.

Quoted 8 lines

From: Laura Abbott <[email protected]>
Date: Thu, 6 Mar 2014 21:05:41 -0800
Subject: [PATCH] arm64: Use custom mmap function for noncoherent dma ops

The non-coherent dma ops remap memory with appropriate attributes. This remapped address cannot be used with virt_to_page which dma_common_mmap uses. Implement a custom (but very similar) function which correctly calculates the physical address and remaps to userspace.

Looking at the coherent implementation, I think we also have a (performance) problem with dma_common_mmap(). It uses pgprot_noncached() by default and if the DMA is coherent we don't really need strongly ordered memory.

Would you mind writing a __dma_common_mmap() for arm64 with separate coherent/non-coherent dma mmap functions that set the vm_page_prot accordingly?

Thanks.

Signature · 1 line
Catalin
#4 lore raw
Quoted 21 lines · Catalin Marinas
Quoted 3 lines · Laura Abbott

With one caveat below, you can add

Tested-by Laura Abbott <[email protected]>

Thanks.

Quoted 8 lines

From: Laura Abbott <[email protected]>
Date: Thu, 6 Mar 2014 21:05:41 -0800
Subject: [PATCH] arm64: Use custom mmap function for noncoherent dma ops

The non-coherent dma ops remap memory with appropriate attributes. This remapped address cannot be used with virt_to_page which dma_common_mmap uses. Implement a custom (but very similar) function which correctly calculates the physical address and remaps to userspace.

Looking at the coherent implementation, I think we also have a (performance) problem with dma_common_mmap(). It uses pgprot_noncached() by default and if the DMA is coherent we don't really need strongly ordered memory.

Quoted 6 lines

Would you mind writing a __dma_common_mmap() for arm64 with separate coherent/non-coherent dma mmap functions that set the vm_page_prot accordingly?

Thanks.

Yes, I will add that to my TODO list.

Thanks,
Laura

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
#5 lore raw

PATCH 1/2 arm64: Implement custom mmap functions for dma mapping

The current dma_ops do not specify an mmap function so maping falls back to the default implementation. There are at least two issues with using the default implementation:

1) The pgprot is always pgprot_noncached (strongly ordered) memory even with coherent operations
2) dma_common_mmap calls virt_to_page on the remapped non-coherent address which leads to invalid memory being mapped.

Fix both these issue by implementing a custom mmap function which correctly accounts for remapped addresses and sets vm_pg_prot appropriately.

Signed-off-by Laura Abbott <[email protected]>
---
 arch/arm64/mm/dma-mapping.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
arch/arm64/mm/dma-mapping.c +44 -0
index e923a5b..9a639bf 100644
@@ -194,9 +194,52 @@ static void arm64_swiotlb_sync_sg_for_device(struct device *dev,
 			       sg->length, dir);
 }
 
+/* vma->vm_page_prot must be set appropriately before calling this function */
+static int __dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
+			     void *cpu_addr, dma_addr_t dma_addr, size_t size)
+{
+	int ret = -ENXIO;
+	unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >>
+					PAGE_SHIFT;
+	unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT;
+	unsigned long off = vma->vm_pgoff;
+
+	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
+		return ret;
+
+	if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) {
+		ret = remap_pfn_range(vma, vma->vm_start,
+				      pfn + off,
+				      vma->vm_end - vma->vm_start,
+				      vma->vm_page_prot);
+	}
+
+	return ret;
+}
+
+static int arm64_swiotlb_mmap_noncoherent(struct device *dev,
+		struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		struct dma_attrs *attrs)
+{
+	/* Just use whatever page_prot attributes were specified */
+	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
+}
+
+static int arm64_swiotlb_mmap_coherent(struct device *dev,
+		struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		struct dma_attrs *attrs)
+{
+	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
+	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
+}
+
 struct dma_map_ops noncoherent_swiotlb_dma_ops = {
 	.alloc = arm64_swiotlb_alloc_noncoherent,
 	.free = arm64_swiotlb_free_noncoherent,
+	.mmap = arm64_swiotlb_mmap_noncoherent,
 	.map_page = arm64_swiotlb_map_page,
 	.unmap_page = arm64_swiotlb_unmap_page,
 	.map_sg = arm64_swiotlb_map_sg_attrs,

@@ -213,6 +256,7 @@ EXPORT_SYMBOL(noncoherent_swiotlb_dma_ops);
 struct dma_map_ops coherent_swiotlb_dma_ops = {
 	.alloc = arm64_swiotlb_alloc_coherent,
 	.free = arm64_swiotlb_free_coherent,
+	.mmap = arm64_swiotlb_mmap_coherent,
 	.map_page = swiotlb_map_page,
 	.unmap_page = swiotlb_unmap_page,
 	.map_sg = swiotlb_map_sg_attrs,
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
maintainer
#6 lore raw

PATCH 1/2 arm64: Implement custom mmap functions for dma mapping

Quoted 31 lines · Laura Abbott
arch/arm64/mm/dma-mapping.c +31 -0
+/* vma->vm_page_prot must be set appropriately before calling this function */
+static int __dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
+			     void *cpu_addr, dma_addr_t dma_addr, size_t size)
+{
+	int ret = -ENXIO;
+	unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >>
+					PAGE_SHIFT;
+	unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT;
+	unsigned long off = vma->vm_pgoff;
+
+	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
+		return ret;
+
+	if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) {
+		ret = remap_pfn_range(vma, vma->vm_start,
+				      pfn + off,
+				      vma->vm_end - vma->vm_start,
+				      vma->vm_page_prot);
+	}
+
+	return ret;
+}
+
+static int arm64_swiotlb_mmap_noncoherent(struct device *dev,
+		struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		struct dma_attrs *attrs)
+{
+	/* Just use whatever page_prot attributes were specified */
+	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);

arch/arm64/mm/dma-mapping.c static void arm64_swiotlb_sync_sg_for_device(struct device *dev, · line 237

Quoted 10 lines
arch/arm64/mm/dma-mapping.c +10 -0
+}
+
+static int arm64_swiotlb_mmap_coherent(struct device *dev,
+		struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		struct dma_attrs *attrs)
+{
+	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
+	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
+}

So I think we got the naming the other way around. The noncoherent_dma_ops assume that the DMA is non-coherent and therefore must change the pgprot.

Signature · 1 line
Catalin
#7 lore raw

PATCH 1/2 arm64: Implement custom mmap functions for dma mapping

Quoted 38 lines · Catalin Marinas
Quoted 37 lines · Laura Abbott
arch/arm64/mm/dma-mapping.c +37 -0
+/* vma->vm_page_prot must be set appropriately before calling this function */
+static int __dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
+			     void *cpu_addr, dma_addr_t dma_addr, size_t size)
+{
+	int ret = -ENXIO;
+	unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >>
+					PAGE_SHIFT;
+	unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT;
+	unsigned long off = vma->vm_pgoff;
+
+	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
+		return ret;
+
+	if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) {
+		ret = remap_pfn_range(vma, vma->vm_start,
+				      pfn + off,
+				      vma->vm_end - vma->vm_start,
+				      vma->vm_page_prot);
+	}
+
+	return ret;
+}
+
+static int arm64_swiotlb_mmap_noncoherent(struct device *dev,
+		struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		struct dma_attrs *attrs)
+{
+	/* Just use whatever page_prot attributes were specified */
+	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
+}
+
+static int arm64_swiotlb_mmap_coherent(struct device *dev,
+		struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		struct dma_attrs *attrs)
Quoted 8 lines

arch/arm64/mm/dma-mapping.c static void arm64_swiotlb_sync_sg_for_device(struct device *dev, · line 237

Quoted 4 lines
arch/arm64/mm/dma-mapping.c +4 -0
+{
+	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
+	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
+}

So I think we got the naming the other way around. The noncoherent_dma_ops assume that the DMA is non-coherent and therefore must change the pgprot.

I spent way too long convincing myself this was right and I was still wrong. I'll fix it up for v2 (I have one more patch I need to put in for CMA integration as well)

Thanks,
Laura

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
#8 lore raw

PATCH 2/2 arm64: Support DMA_ATTR_WRITE_COMBINE

In reply to Laura Abbott

DMA_ATTR_WRITE_COMBINE is currently ignored. Set the pgprot appropriately for non coherent opperations.

Signed-off-by Laura Abbott <[email protected]>

---
Potential addition on top of the coherent work as well. Writecombine and dmacoherent seem to be the same at the moment but it might be good practice to have the two be separate?

 arch/arm64/mm/dma-mapping.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
arch/arm64/mm/dma-mapping.c +11 -2
index 9a639bf..d2c0027 100644
@@ -29,6 +29,15 @@
 struct dma_map_ops *dma_ops;
 EXPORT_SYMBOL(dma_ops);
 
+
+static inline pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot)
+{
+	prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ?
+			    pgprot_writecombine(prot) :
+			    pgprot_dmacoherent(prot);
+	return prot;
+}
+
 static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size,
 					  dma_addr_t *dma_handle, gfp_t flags,
 					  struct dma_attrs *attrs)

@@ -72,7 +81,7 @@ static void *arm64_swiotlb_alloc_noncoherent(struct device *dev, size_t size,
 	for (i = 0; i < (size >> PAGE_SHIFT); i++)
 		map[i] = page + i;
 	coherent_ptr = vmap(map, size >> PAGE_SHIFT, VM_MAP,
-			    pgprot_dmacoherent(pgprot_default));
+			    __get_dma_pgprot(attrs, pgprot_default));
 	kfree(map);
 	if (!coherent_ptr)
 		goto no_map;

@@ -232,7 +241,7 @@ static int arm64_swiotlb_mmap_coherent(struct device *dev,
 		void *cpu_addr, dma_addr_t dma_addr, size_t size,
 		struct dma_attrs *attrs)
 {
-	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
+	vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot);
 	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
 }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
maintainer
#9 lore raw

PATCH 2/2 arm64: Support DMA_ATTR_WRITE_COMBINE

Quoted 18 lines · Laura Abbott

DMA_ATTR_WRITE_COMBINE is currently ignored. Set the pgprot appropriately for non coherent opperations.

Signed-off-by Laura Abbott <[email protected]>

---
Potential addition on top of the coherent work as well. Writecombine and dmacoherent seem to be the same at the moment but it might be good practice to have the two be separate?

 arch/arm64/mm/dma-mapping.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
arch/arm64/mm/dma-mapping.c +0 -0
index 9a639bf..d2c0027 100644
@@ -29,6 +29,15 @@
 struct dma_map_ops *dma_ops;

arch/arm64/mm/dma-mapping.c line 39

Quoted 10 lines
arch/arm64/mm/dma-mapping.c +8 -0
 EXPORT_SYMBOL(dma_ops);
 
+
+static inline pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot)
+{
+	prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ?
+			    pgprot_writecombine(prot) :
+			    pgprot_dmacoherent(prot);
+	return prot;
+}

pgprot_writecombine and pgprot_dmacoherent are the same on arm64 (and ARMv6/v7). So when the DMA is coherent on an SoC, we need to leave the prot unchanged if !DMA_ATTR_WRITE_COMBINE.

Signature · 1 line
Catalin
#10 lore raw

PATCH 2/2 arm64: Support DMA_ATTR_WRITE_COMBINE

Quoted 21 lines · Catalin Marinas
Quoted 20 lines · Laura Abbott

DMA_ATTR_WRITE_COMBINE is currently ignored. Set the pgprot appropriately for non coherent opperations.

Signed-off-by Laura Abbott <[email protected]>

---
Potential addition on top of the coherent work as well. Writecombine and dmacoherent seem to be the same at the moment but it might be good practice to have the two be separate?

  arch/arm64/mm/dma-mapping.c | 13 +++++++++++--
  1 file changed, 11 insertions(+), 2 deletions(-)
arch/arm64/mm/dma-mapping.c +0 -0
index 9a639bf..d2c0027 100644
@@ -29,6 +29,15 @@
  struct dma_map_ops *dma_ops;
  EXPORT_SYMBOL(dma_ops);
Quoted 13 lines

arch/arm64/mm/dma-mapping.c line 39

Quoted 8 lines
arch/arm64/mm/dma-mapping.c +8 -0
+
+static inline pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot)
+{
+	prot = dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs) ?
+			    pgprot_writecombine(prot) :
+			    pgprot_dmacoherent(prot);
+	return prot;
+}

pgprot_writecombine and pgprot_dmacoherent are the same on arm64 (and ARMv6/v7). So when the DMA is coherent on an SoC, we need to leave the prot unchanged if !DMA_ATTR_WRITE_COMBINE.

Ah yes I missed that. I'll fix it up.

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
#11 lore raw

PATCH v2 1/3 arm64: Implement custom mmap functions for dma mapping

In reply to Laura Abbott

The current dma_ops do not specify an mmap function so maping falls back to the default implementation. There are at least two issues with using the default implementation:

1) The pgprot is always pgprot_noncached (strongly ordered) memory even with coherent operations
2) dma_common_mmap calls virt_to_page on the remapped non-coherent address which leads to invalid memory being mapped.

Fix both these issue by implementing a custom mmap function which correctly accounts for remapped addresses and sets vm_pg_prot appropriately.

Signed-off-by Laura Abbott <[email protected]>
---
 arch/arm64/mm/dma-mapping.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
arch/arm64/mm/dma-mapping.c +44 -0
index e923a5b..0cdd2f6 100644
@@ -194,9 +194,52 @@ static void arm64_swiotlb_sync_sg_for_device(struct device *dev,
 			       sg->length, dir);
 }
 
+/* vma->vm_page_prot must be set appropriately before calling this function */
+static int __dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
+			     void *cpu_addr, dma_addr_t dma_addr, size_t size)
+{
+	int ret = -ENXIO;
+	unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >>
+					PAGE_SHIFT;
+	unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT;
+	unsigned long off = vma->vm_pgoff;
+
+	if (dma_mmap_from_coherent(dev, vma, cpu_addr, size, &ret))
+		return ret;
+
+	if (off < nr_pages && nr_vma_pages <= (nr_pages - off)) {
+		ret = remap_pfn_range(vma, vma->vm_start,
+				      pfn + off,
+				      vma->vm_end - vma->vm_start,
+				      vma->vm_page_prot);
+	}
+
+	return ret;
+}
+
+static int arm64_swiotlb_mmap_noncoherent(struct device *dev,
+		struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		struct dma_attrs *attrs)
+{
+	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
+	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
+}
+
+static int arm64_swiotlb_mmap_coherent(struct device *dev,
+		struct vm_area_struct *vma,
+		void *cpu_addr, dma_addr_t dma_addr, size_t size,
+		struct dma_attrs *attrs)
+{
+	/* Just use whatever page_prot attributes were specified */
+	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
+}
+
 struct dma_map_ops noncoherent_swiotlb_dma_ops = {
 	.alloc = arm64_swiotlb_alloc_noncoherent,
 	.free = arm64_swiotlb_free_noncoherent,
+	.mmap = arm64_swiotlb_mmap_noncoherent,
 	.map_page = arm64_swiotlb_map_page,
 	.unmap_page = arm64_swiotlb_unmap_page,
 	.map_sg = arm64_swiotlb_map_sg_attrs,

@@ -213,6 +256,7 @@ EXPORT_SYMBOL(noncoherent_swiotlb_dma_ops);
 struct dma_map_ops coherent_swiotlb_dma_ops = {
 	.alloc = arm64_swiotlb_alloc_coherent,
 	.free = arm64_swiotlb_free_coherent,
+	.mmap = arm64_swiotlb_mmap_coherent,
 	.map_page = swiotlb_map_page,
 	.unmap_page = swiotlb_unmap_page,
 	.map_sg = swiotlb_map_sg_attrs,
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
maintainer
#12 lore raw

PATCH v2 1/3 arm64: Implement custom mmap functions for dma mapping

Quoted 9 lines · Laura Abbott

The current dma_ops do not specify an mmap function so maping falls back to the default implementation. There are at least two issues with using the default implementation:

1) The pgprot is always pgprot_noncached (strongly ordered) memory even with coherent operations
2) dma_common_mmap calls virt_to_page on the remapped non-coherent address which leads to invalid memory being mapped.

Quoted 5 lines

Fix both these issue by implementing a custom mmap function which correctly accounts for remapped addresses and sets vm_pg_prot appropriately.

Signed-off-by Laura Abbott <[email protected]>

I thought there was still some update needed to this series but it turns out that patch 2/3 was what I was expecting already, so I merged the first two patches (with minor changes for s/arm64_/__/ prefix). Patch 3 seems to be folded onto my patch already.

Thanks.

Signature · 1 line
Catalin

A message that is not in the archive (sent off-list or not yet received): CALk7dXr3cZSkQ6dTUyCjUDStOd6=ghGN9-iO5RQiTfHCciGxLg@mail.gmail.com

maintainer
#14 lore raw

PATCH v2 1/3 arm64: Implement custom mmap functions for dma mapping

Quoted 12 lines · Ritesh Harjani

arch/arm64/mm/dma-mapping.c static void arm64_swiotlb_sync_sg_for_device(struct device *dev, · line 205

Quoted 8 lines · Laura Abbott
arch/arm64/mm/dma-mapping.c +8 -0
+static int __dma_common_mmap(struct device *dev, struct vm_area_struct *vma,
+                            void *cpu_addr, dma_addr_t dma_addr, size_t size)
+{
+       int ret = -ENXIO;
+       unsigned long nr_vma_pages = (vma->vm_end - vma->vm_start) >>
+                                       PAGE_SHIFT;
+       unsigned long nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+       unsigned long pfn = dma_to_phys(dev, dma_addr) >> PAGE_SHIFT;

Why not __phys_to_pfn here ?? just asking, I know there is nothing wrong in this too..

Because dma_addr is a DMA address (as seen by the device from the other side of the bus) rather than a CPU physical address. In many cases they are the same but not always.

Signature · 1 line
Catalin
#15 lore raw

PATCH v2 2/3 arm64: Support DMA_ATTR_WRITE_COMBINE

In reply to Laura Abbott

DMA_ATTR_WRITE_COMBINE is currently ignored. Set the pgprot appropriately for non coherent opperations.

Signed-off-by Laura Abbott <[email protected]>
---
 arch/arm64/mm/dma-mapping.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
arch/arm64/mm/dma-mapping.c +13 -2
index 0cdd2f6..608c343 100644
@@ -29,6 +29,17 @@
 struct dma_map_ops *dma_ops;
 EXPORT_SYMBOL(dma_ops);
 
+
+static inline pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
+					bool coherent)
+{
+	if (dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs))
+		return pgprot_writecombine(prot)
+	else if (!coherent)
+		return pgprot_dmacoherent(prot);
+	return prot;
+}
+
 static void *arm64_swiotlb_alloc_coherent(struct device *dev, size_t size,
 					  dma_addr_t *dma_handle, gfp_t flags,
 					  struct dma_attrs *attrs)

@@ -72,7 +83,7 @@ static void *arm64_swiotlb_alloc_noncoherent(struct device *dev, size_t size,
 	for (i = 0; i < (size >> PAGE_SHIFT); i++)
 		map[i] = page + i;
 	coherent_ptr = vmap(map, size >> PAGE_SHIFT, VM_MAP,
-			    pgprot_dmacoherent(pgprot_default));
+			    __get_dma_pgprot(attrs, pgprot_default, false));
 	kfree(map);
 	if (!coherent_ptr)
 		goto no_map;

@@ -223,7 +234,7 @@ static int arm64_swiotlb_mmap_noncoherent(struct device *dev,
 		void *cpu_addr, dma_addr_t dma_addr, size_t size,
 		struct dma_attrs *attrs)
 {
-	vma->vm_page_prot = pgprot_dmacoherent(vma->vm_page_prot);
+	vma->vm_page_prot = __get_dma_pgprot(attrs, vma->vm_page_prot, false);
 	return __dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
 }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
#16 lore raw

PATCH v2 2/3 arm64: Support DMA_ATTR_WRITE_COMBINE

Quoted 13 lines · Laura Abbott

DMA_ATTR_WRITE_COMBINE is currently ignored. Set the pgprot appropriately for non coherent opperations.

Signed-off-by Laura Abbott <[email protected]>
---
 arch/arm64/mm/dma-mapping.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
arch/arm64/mm/dma-mapping.c +0 -0
index 0cdd2f6..608c343 100644

arch/arm64/mm/dma-mapping.c line 37

Quoted 9 lines
arch/arm64/mm/dma-mapping.c +6 -0
@@ -29,6 +29,17 @@
 struct dma_map_ops *dma_ops;
 EXPORT_SYMBOL(dma_ops);
 
+
+static inline pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
+                                       bool coherent)
+{
+       if (dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs))
+               return pgprot_writecombine(prot)

Does this compile? Missing semicolon.

arch/arm64/mm/dma-mapping.c line 40

Quoted 3 lines
arch/arm64/mm/dma-mapping.c +3 -0
+       else if (!coherent)
+               return pgprot_dmacoherent(prot);
+       return prot;

Isn't DMA_ATTR_WRITE_COMBINE supposed to be an optimization over plain non-cached? But coherent would be more optimal over just write combine, so we would want to continue to ignore DMA_ATTR_WRITE_COMBINE in the coherent case. Something like this:

      if (coherent)
               return prot;
      else if (dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs))
               return pgprot_writecombine(prot);
      return pgprot_dmacoherent(prot);

But then this function is never used in the coherent case, so I'm confused by Catalin's comment. The original version seems sufficient to me.

Rob

maintainer
#17 lore raw

PATCH v2 2/3 arm64: Support DMA_ATTR_WRITE_COMBINE

Quoted 26 lines · Rob Herring
Quoted 13 lines · Laura Abbott

DMA_ATTR_WRITE_COMBINE is currently ignored. Set the pgprot appropriately for non coherent opperations.

Signed-off-by Laura Abbott <[email protected]>

---
arch/arm64/mm/dma-mapping.c | 15 +++++++++++++--

1 file changed, 13 insertions(+), 2 deletions(-)
arch/arm64/mm/dma-mapping.c +0 -0
index 0cdd2f6..608c343 100644

arch/arm64/mm/dma-mapping.c line 37

Quoted 9 lines
arch/arm64/mm/dma-mapping.c +6 -0
@@ -29,6 +29,17 @@
 struct dma_map_ops *dma_ops;
 EXPORT_SYMBOL(dma_ops);
 
+
+static inline pgprot_t __get_dma_pgprot(struct dma_attrs *attrs, pgprot_t prot,
+                                       bool coherent)
+{
+       if (dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs))
+               return pgprot_writecombine(prot)

Does this compile? Missing semicolon.

Quoted 6 lines

arch/arm64/mm/dma-mapping.c line 40

Quoted 3 lines
arch/arm64/mm/dma-mapping.c +3 -0
+       else if (!coherent)
+               return pgprot_dmacoherent(prot);
+       return prot;

Isn't DMA_ATTR_WRITE_COMBINE supposed to be an optimization over plain non-cached?

Since writecombine and dmacoherent are the same, we probably shouldn?t even bother with the check as it doesn?t have any effect.

Quoted 9 lines

But coherent would be more optimal over just write
combine, so we would want to continue to ignore DMA_ATTR_WRITE_COMBINE in the coherent case. Something like this:

     if (coherent)
              return prot;
     else if (dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs))
              return pgprot_writecombine(prot);
     return pgprot_dmacoherent(prot);

I?ve got request from graphics people in the past about using writecombine even though the DMA is fully coherent. The reason is that some (frame)buffers are more efficient to fill with non-cacheable memory than cacheable + write allocate which affects the whole CPU cache.

Quoted 3 lines

But then this function is never used in the coherent case, so I'm confused by Catalin's comment. The original version seems sufficient to me.

I think we need mmap function for both coherent and non-coherent cases since the default dma_mmap_coherent() just uses pgprot_noncached() which gives us Strongly Ordered.

So my proposal:

	if (!coherent)
		return pgprot_dmacoherent(prot);
	else if (dma_get_attr(DMA_ATTR_WRITE_COMBINE, attrs))
		return pgprot_writecombine(prot);
	return prot;

I could even use dmacoherent instead of writecombine to avoid confusion.

Catalin

#18 lore raw

PATCH v2 3/3 arm64: Use arm64 coherent APIs for non-coherent freeing

In reply to Laura Abbott

The noncoherent free function currently unconditionally frees back to the page allocator which is incorrect for CMA pages. call the coherent free function to correctly differentiate between pages.

Signed-off-by Laura Abbott <[email protected]>
---
 arch/arm64/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/mm/dma-mapping.c +1 -1
index 608c343..99ff063 100644
@@ -104,7 +104,7 @@ static void arm64_swiotlb_free_noncoherent(struct device *dev, size_t size,
 	void *swiotlb_addr = phys_to_virt(dma_to_phys(dev, dma_handle));
 
 	vunmap(vaddr);
-	swiotlb_free_coherent(dev, size, swiotlb_addr, dma_handle);
+	arm64_swiotlb_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
 }
 
 static dma_addr_t arm64_swiotlb_map_page(struct device *dev,
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation