Tegra GART fixes and improvements

Page mapping could overwritten by an accident (a bug). We can catch this case by checking 'VALID' bit of GART's page entry prior to mapping of a page. Since that check introduces a small performance impact, it should be enabled explicitly using new GART's kernel module 'debug' parameter.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

It must return the number of unmapped bytes on success, returning 0 means that unmapping failed and in result only one page is unmapped.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

GART has a fixed aperture size, hence the number of pages is constant.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Currently GART writes one page entry at a time. More optimal would be to aggregate the writes and flush BUS buffer in the end, this gives map/unmap 10-40% (depending on size of mapping) performance boost compared to a flushing after each entry update.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 63 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 15 deletions(-)

drivers/iommu/tegra-gart.c +0 -0

index 4a0607669d34..9f59f5f17661 100644

drivers/iommu/tegra-gart.c +47 -14

 #define GART_REG_BASE		0x24
 #define GART_CONFIG		(0x24 - GART_REG_BASE)

@@ -269,25 +269,21 @@ static void gart_iommu_domain_free(struct iommu_domain *domain)
 	kfree(gart_domain);
 }
 
-static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
-			  phys_addr_t pa, size_t bytes, int prot)
+static int gart_iommu_map_page(struct gart_device *gart,
+			       unsigned long iova,
+			       phys_addr_t pa)
 {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
 	unsigned long flags;
 	unsigned long pfn;
 	unsigned long pte;
 
-	if (!gart_iova_range_valid(gart, iova, bytes))
-		return -EINVAL;
-
-	spin_lock_irqsave(&gart->pte_lock, flags);
 	pfn = __phys_to_pfn(pa);
 	if (!pfn_valid(pfn)) {
 		dev_err(gart->dev, "Invalid page: %pa\n", &pa);
-		spin_unlock_irqrestore(&gart->pte_lock, flags);
 		return -EINVAL;
 	}
+
+	spin_lock_irqsave(&gart->pte_lock, flags);
 	if (gart_debug) {
 		pte = gart_read_pte(gart, iova);
 		if (pte & GART_ENTRY_PHYS_ADDR_VALID) {

@@ -297,8 +293,41 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
 		}
 	}
 	gart_set_pte(gart, iova, GART_PTE(pfn));
+	spin_unlock_irqrestore(&gart->pte_lock, flags);
+
+	return 0;
+}
+
+static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
+			  phys_addr_t pa, size_t bytes, int prot)
+{
+	struct gart_domain *gart_domain = to_gart_domain(domain);
+	struct gart_device *gart = gart_domain->gart;
+	size_t mapped;
+	int ret = -1;
+
+	if (!gart_iova_range_valid(gart, iova, bytes))
+		return -EINVAL;
+
+	for (mapped = 0; mapped < bytes; mapped += GART_PAGE_SIZE) {
+		ret = gart_iommu_map_page(gart, iova + mapped, pa + mapped);
+		if (ret)
+			break;
+	}
+
 	FLUSH_GART_REGS(gart);
+	return ret;
+}
+
+static int gart_iommu_unmap_page(struct gart_device *gart,
+				 unsigned long iova)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&gart->pte_lock, flags);
+	gart_set_pte(gart, iova, 0);
 	spin_unlock_irqrestore(&gart->pte_lock, flags);
+
 	return 0;
 }
 

@@ -307,16 +336,20 @@ static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 {
 	struct gart_domain *gart_domain = to_gart_domain(domain);
 	struct gart_device *gart = gart_domain->gart;
-	unsigned long flags;
+	size_t unmapped;
+	int ret;
 
 	if (!gart_iova_range_valid(gart, iova, bytes))
 		return 0;
 
-	spin_lock_irqsave(&gart->pte_lock, flags);
-	gart_set_pte(gart, iova, 0);
+	for (unmapped = 0; unmapped < bytes; unmapped += GART_PAGE_SIZE) {
+		ret = gart_iommu_unmap_page(gart, iova + unmapped);
+		if (ret)
+			break;
+	}
+
 	FLUSH_GART_REGS(gart);
-	spin_unlock_irqrestore(&gart->pte_lock, flags);
-	return bytes;
+	return unmapped;
 }
 
 static phys_addr_t gart_iommu_iova_to_phys(struct iommu_domain *domain,

Signature · 1 line

2.16.3

Quoted 15 lines · Dmitry Osipenko

Currently GART writes one page entry at a time. More optimal would be to aggregate the writes and flush BUS buffer in the end, this gives map/unmap 10-40% (depending on size of mapping) performance boost compared to a flushing after each entry update.

Signed-off-by Dmitry Osipenko <[email protected]>

---
 drivers/iommu/tegra-gart.c | 63 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 15 deletions(-)

drivers/iommu/tegra-gart.c +0 -0

index 4a0607669d34..9f59f5f17661 100644

drivers/iommu/tegra-gart.c line 39

Quoted 5 lines

drivers/iommu/tegra-gart.c +1 -1

@@ -36,7 +36,7 @@
 #define GART_APERTURE_SIZE	SZ_32M
 
 /* bitmap of the page sizes currently supported */
-#define GART_IOMMU_PGSIZES	(SZ_4K)
+#define GART_IOMMU_PGSIZES	GENMASK(24, 12)

That doesn't look right. The GART really only supports 4 KiB pages. You seem to be "emulating" more page sizes here in order to improve mapping performance. That seems wrong to me. I'm wondering if this couldn't be improved by a similar factor by simply moving the flushing into an implementation of ->iotlb_sync().

That said, it seems like ->iotlb_sync() is only used for unmapping, but I don't see a reason why iommu_map() wouldn't need to call it as well after going through several calls to ->map(). It seems to me like a driver that implements ->iotlb_sync() would want to use it to optimize for both the mapping and unmapping cases.

Joerg, I've gone over the git log and header files and I see no mention of why the TLB flush interface isn't used for mapping. Do you recall any special reasons why the same shouldn't be applied for mapping? Would you accept any patches doing this?

Quoted 15 lines · Dmitry Osipenko

Currently GART writes one page entry at a time. More optimal would be to aggregate the writes and flush BUS buffer in the end, this gives map/unmap 10-40% (depending on size of mapping) performance boost compared to a flushing after each entry update.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/iommu/tegra-gart.c | 63 +++++++++++++++++++++++++++++++++++-----------
  1 file changed, 48 insertions(+), 15 deletions(-)

drivers/iommu/tegra-gart.c +0 -0

index 4a0607669d34..9f59f5f17661 100644

drivers/iommu/tegra-gart.c line 39

Quoted 5 lines

drivers/iommu/tegra-gart.c +1 -1

@@ -36,7 +36,7 @@
  #define GART_APERTURE_SIZE	SZ_32M
 
  /* bitmap of the page sizes currently supported */
-#define GART_IOMMU_PGSIZES	(SZ_4K)
+#define GART_IOMMU_PGSIZES	GENMASK(24, 12)

That doesn't look right. The GART really only supports 4 KiB pages. You seem to be "emulating" more page sizes here in order to improve mapping performance. That seems wrong to me. I'm wondering if this couldn't be improved by a similar factor by simply moving the flushing into an implementation of ->iotlb_sync().

That said, it seems like ->iotlb_sync() is only used for unmapping, but I don't see a reason why iommu_map() wouldn't need to call it as well after going through several calls to ->map(). It seems to me like a driver that implements ->iotlb_sync() would want to use it to optimize for both the mapping and unmapping cases.

Joerg, I've gone over the git log and header files and I see no mention of why the TLB flush interface isn't used for mapping. Do you recall any special reasons why the same shouldn't be applied for mapping? Would you accept any patches doing this?

Thierry

Quoted 111 lines

drivers/iommu/tegra-gart.c +47 -14

  #define GART_REG_BASE		0x24
  #define GART_CONFIG		(0x24 - GART_REG_BASE)

@@ -269,25 +269,21 @@ static void gart_iommu_domain_free(struct iommu_domain *domain)
  	kfree(gart_domain);
  }
 
-static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
-			  phys_addr_t pa, size_t bytes, int prot)
+static int gart_iommu_map_page(struct gart_device *gart,
+			       unsigned long iova,
+			       phys_addr_t pa)
  {
-	struct gart_domain *gart_domain = to_gart_domain(domain);
-	struct gart_device *gart = gart_domain->gart;
  	unsigned long flags;
  	unsigned long pfn;
  	unsigned long pte;
 
-	if (!gart_iova_range_valid(gart, iova, bytes))
-		return -EINVAL;
-
-	spin_lock_irqsave(&gart->pte_lock, flags);
  	pfn = __phys_to_pfn(pa);
  	if (!pfn_valid(pfn)) {
  		dev_err(gart->dev, "Invalid page: %pa\n", &pa);
-		spin_unlock_irqrestore(&gart->pte_lock, flags);
  		return -EINVAL;
  	}
+
+	spin_lock_irqsave(&gart->pte_lock, flags);
  	if (gart_debug) {
  		pte = gart_read_pte(gart, iova);
  		if (pte & GART_ENTRY_PHYS_ADDR_VALID) {

@@ -297,8 +293,41 @@ static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
  		}
  	}
  	gart_set_pte(gart, iova, GART_PTE(pfn));
+	spin_unlock_irqrestore(&gart->pte_lock, flags);
+
+	return 0;
+}
+
+static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
+			  phys_addr_t pa, size_t bytes, int prot)
+{
+	struct gart_domain *gart_domain = to_gart_domain(domain);
+	struct gart_device *gart = gart_domain->gart;
+	size_t mapped;
+	int ret = -1;
+
+	if (!gart_iova_range_valid(gart, iova, bytes))
+		return -EINVAL;
+
+	for (mapped = 0; mapped < bytes; mapped += GART_PAGE_SIZE) {
+		ret = gart_iommu_map_page(gart, iova + mapped, pa + mapped);
+		if (ret)
+			break;
+	}
+
  	FLUSH_GART_REGS(gart);
+	return ret;
+}
+
+static int gart_iommu_unmap_page(struct gart_device *gart,
+				 unsigned long iova)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&gart->pte_lock, flags);
+	gart_set_pte(gart, iova, 0);
  	spin_unlock_irqrestore(&gart->pte_lock, flags);
+
  	return 0;
  }
 

@@ -307,16 +336,20 @@ static size_t gart_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
  {
  	struct gart_domain *gart_domain = to_gart_domain(domain);
  	struct gart_device *gart = gart_domain->gart;
-	unsigned long flags;
+	size_t unmapped;
+	int ret;
 
  	if (!gart_iova_range_valid(gart, iova, bytes))
  		return 0;
 
-	spin_lock_irqsave(&gart->pte_lock, flags);
-	gart_set_pte(gart, iova, 0);
+	for (unmapped = 0; unmapped < bytes; unmapped += GART_PAGE_SIZE) {
+		ret = gart_iommu_unmap_page(gart, iova + unmapped);
+		if (ret)
+			break;
+	}
+
  	FLUSH_GART_REGS(gart);
-	spin_unlock_irqrestore(&gart->pte_lock, flags);
-	return bytes;
+	return unmapped;
  }
 
  static phys_addr_t gart_iommu_iova_to_phys(struct iommu_domain *domain,

Signature · 8 lines

2.16.3



_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Hi Thierry,

Quoted 33 lines · Thierry Reding

Quoted 15 lines · Dmitry Osipenko

Currently GART writes one page entry at a time. More optimal would be to aggregate the writes and flush BUS buffer in the end, this gives map/unmap 10-40% (depending on size of mapping) performance boost compared to a flushing after each entry update.

Signed-off-by Dmitry Osipenko <[email protected]>

---
  drivers/iommu/tegra-gart.c | 63 +++++++++++++++++++++++++++++++++++-----------

  1 file changed, 48 insertions(+), 15 deletions(-)

drivers/iommu/tegra-gart.c +0 -0

index 4a0607669d34..9f59f5f17661 100644

drivers/iommu/tegra-gart.c line 38

Quoted 4 lines

drivers/iommu/tegra-gart.c +1 -1

@@ -36,7 +36,7 @@
   #define GART_APERTURE_SIZE    SZ_32M
     /* bitmap of the page sizes currently supported */
-#define GART_IOMMU_PGSIZES    (SZ_4K)
+#define GART_IOMMU_PGSIZES    GENMASK(24, 12)

That doesn't look right. The GART really only supports 4 KiB pages. You seem to be "emulating" more page sizes here in order to improve mapping performance. That seems wrong to me. I'm wondering if this couldn't be improved by a similar factor by simply moving the flushing into an implementation of ->iotlb_sync().

That said, it seems like ->iotlb_sync() is only used for unmapping, but I don't see a reason why iommu_map() wouldn't need to call it as well after going through several calls to ->map(). It seems to me like a driver that implements ->iotlb_sync() would want to use it to optimize for both the mapping and unmapping cases.

Quoted 4 lines

Joerg, I've gone over the git log and header files and I see no mention of why the TLB flush interface isn't used for mapping. Do you recall any special reasons why the same shouldn't be applied for mapping? Would you accept any patches doing this?

In general, requiring TLB maintenance when transitioning from an invalid entry to a valid one tends to be the exception rather than the norm, and I think we ended up at the consensus that it wasn't worth the complication of trying to cater for this in the generic iotlb API.

To be fair, on simple hardware which doesn't implement multiple page sizes with associated walk depth/TLB pressure benefits for larger ones, there's no need for the IOMMU API (and/or the owner of the domain) to try harder to use them, so handling "compound" page sizes within the driver is a more reasonable thing to do. There is already some precedent for this in other drivers (e.g. mtk_iommu_v1).

Probably the best variant would be to give an explicit control over syncing to a user of the IOMMU API, like for example device driver may perform multiple mappings / unmappings and then sync/flush in the end. I'm not sure that it's really worth the hassle to shuffle the API right now, maybe we can implement it later if needed. Joerg, do you have objections to a 'compound page' approach?

Quoted 5 lines · Dmitry Osipenko

Probably the best variant would be to give an explicit control over syncing to a user of the IOMMU API, like for example device driver may perform multiple mappings / unmappings and then sync/flush in the end. I'm not sure that it's really worth the hassle to shuffle the API right now, maybe we can implement it later if needed. Joerg, do you have objections to a 'compound page' approach?

Have you measured the performance difference on both variants? The compound-page approach only works for cases when the physical memory you map contiguous and correctly aligned.

If it is really needed I would prefer a separate iotlb_sync_map() call-back that is just NULL when not needed. This way all users that don't need it only get a minimal penalty in the mapping path and you don't have any requirements on the physical memory you map to get good performance.

Secondly, the interesting part is that mapping / unmapping of a contiguous allocation (CMA using DMA API) is slower by ~50% then doing it for a sparse allocation (get_pages using bare IOMMU API). /I think/ it's a shortcoming of the arch/arm/mm/dma-mapping.c, which also suffers from other inflexibilities that Thierry faced recently. Though I haven't really tried to figure out what is the bottleneck yet and Thierry was going to re-write ARM's dma-mapping implementation anyway, I'll take a closer look at this issue a bit later.

Joerg, I've gone over the git log and header files and I see no mention of why the TLB flush interface isn't used for mapping. Do you recall any special reasons why the same shouldn't be applied for mapping? Would you accept any patches doing this?

Quoted 4 lines · Thierry Reding

Joerg, I've gone over the git log and header files and I see no mention of why the TLB flush interface isn't used for mapping. Do you recall any special reasons why the same shouldn't be applied for mapping? Would you accept any patches doing this?

Yeah, the reason was that we assumed that only emulated IOMMUs ever need flushing in the mapping path, and there is no reason to optimize for that. Especially when a call to iotlb_sync() in the mapping path would hurt most other users.

Does the tegra-gart actually need it too?

GART driver wasn't ever been utilized in upstream, but finally this should change sometime soon with Tegra's DRM driver rework. In general GART driver works fine, though there are couple things that could be improved.

Dmitry Osipenko (4):
iommu/tegra: gart: Add debugging facility
iommu/tegra: gart: Fix gart_iommu_unmap()