diff mbox

[06/13] drm/etnaviv: Use sychronized interface of the IOMMU-API

Message ID 1502974596-23835-7-git-send-email-joro@8bytes.org (mailing list archive)
State New, archived
Headers show

Commit Message

Joerg Roedel Aug. 17, 2017, 12:56 p.m. UTC 
From: Joerg Roedel <jroedel@suse.de>

The map and unmap functions of the IOMMU-API changed their
semantics: They do no longer guarantee that the hardware
TLBs are synchronized with the page-table updates they made.

To make conversion easier, new synchronized functions have
been introduced which give these guarantees again until the
code is converted to use the new TLB-flush interface of the
IOMMU-API, which allows certain optimizations.

But for now, just convert this code to use the synchronized
functions so that it will behave as before.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: David Airlie <airlied@linux.ie>
Cc: etnaviv@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Lucas Stach Aug. 17, 2017, 1:32 p.m. UTC | #1 
Hi Joerg,

Am Donnerstag, den 17.08.2017, 14:56 +0200 schrieb Joerg Roedel:
> From: Joerg Roedel <jroedel@suse.de>
> 
> The map and unmap functions of the IOMMU-API changed their
> semantics: They do no longer guarantee that the hardware
> TLBs are synchronized with the page-table updates they made.
> 
> To make conversion easier, new synchronized functions have
> been introduced which give these guarantees again until the
> code is converted to use the new TLB-flush interface of the
> IOMMU-API, which allows certain optimizations.
> 
> But for now, just convert this code to use the synchronized
> functions so that it will behave as before.

I don't think this is necessary. Etnaviv has managed and batched up TLB
flushes from day 1, as they don't happen through the MMU MMIO interface,
but through the GPU command stream.

So if my understanding of this series is correct, Etnaviv is just fine
with the changed semantics of the unsynchronized map/unmap calls.

Regards,
Lucas

> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: etnaviv@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> index f103e78..ae0247c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> @@ -47,7 +47,7 @@ int etnaviv_iommu_map(struct etnaviv_iommu *iommu, u32 iova,
>  
>  		VERB("map[%d]: %08x %08x(%zx)", i, iova, pa, bytes);
>  
> -		ret = iommu_map(domain, da, pa, bytes, prot);
> +		ret = iommu_map_sync(domain, da, pa, bytes, prot);
>  		if (ret)
>  			goto fail;
>  
> @@ -62,7 +62,7 @@ int etnaviv_iommu_map(struct etnaviv_iommu *iommu, u32 iova,
>  	for_each_sg(sgt->sgl, sg, i, j) {
>  		size_t bytes = sg_dma_len(sg) + sg->offset;
>  
> -		iommu_unmap(domain, da, bytes);
> +		iommu_unmap_sync(domain, da, bytes);
>  		da += bytes;
>  	}
>  	return ret;
> @@ -80,7 +80,7 @@ int etnaviv_iommu_unmap(struct etnaviv_iommu *iommu, u32 iova,
>  		size_t bytes = sg_dma_len(sg) + sg->offset;
>  		size_t unmapped;
>  
> -		unmapped = iommu_unmap(domain, da, bytes);
> +		unmapped = iommu_unmap_sync(domain, da, bytes);
>  		if (unmapped < bytes)
>  			return unmapped;
>  
> @@ -338,7 +338,7 @@ int etnaviv_iommu_get_suballoc_va(struct etnaviv_gpu *gpu, dma_addr_t paddr,
>  			mutex_unlock(&mmu->lock);
>  			return ret;
>  		}
> -		ret = iommu_map(mmu->domain, vram_node->start, paddr, size,
> +		ret = iommu_map_sync(mmu->domain, vram_node->start, paddr, size,
>  				IOMMU_READ);
>  		if (ret < 0) {
>  			drm_mm_remove_node(vram_node);
> @@ -362,7 +362,7 @@ void etnaviv_iommu_put_suballoc_va(struct etnaviv_gpu *gpu,
>  
>  	if (mmu->version == ETNAVIV_IOMMU_V2) {
>  		mutex_lock(&mmu->lock);
> -		iommu_unmap(mmu->domain,iova, size);
> +		iommu_unmap_sync(mmu->domain,iova, size);
>  		drm_mm_remove_node(vram_node);
>  		mutex_unlock(&mmu->lock);
>  	}
Joerg Roedel Aug. 17, 2017, 1:45 p.m. UTC | #2 
Hi Lucas,

On Thu, Aug 17, 2017 at 03:32:38PM +0200, Lucas Stach wrote:
> I don't think this is necessary. Etnaviv has managed and batched up TLB
> flushes from day 1, as they don't happen through the MMU MMIO interface,
> but through the GPU command stream.
> 
> So if my understanding of this series is correct, Etnaviv is just fine
> with the changed semantics of the unsynchronized map/unmap calls.

This is not about any TLB on the GPU that could be flushed through the
GPU command stream, but about the TLB in the IOMMU device. Or is this
actually the same on this hardware? Which IOMMU-driver is use there?


Regards,

	Joerg
Lucas Stach Aug. 17, 2017, 2:03 p.m. UTC | #3 
Am Donnerstag, den 17.08.2017, 15:45 +0200 schrieb Joerg Roedel:
> Hi Lucas,
> 
> On Thu, Aug 17, 2017 at 03:32:38PM +0200, Lucas Stach wrote:
> > I don't think this is necessary. Etnaviv has managed and batched up TLB
> > flushes from day 1, as they don't happen through the MMU MMIO interface,
> > but through the GPU command stream.
> > 
> > So if my understanding of this series is correct, Etnaviv is just fine
> > with the changed semantics of the unsynchronized map/unmap calls.
> 
> This is not about any TLB on the GPU that could be flushed through the
> GPU command stream, but about the TLB in the IOMMU device. Or is this
> actually the same on this hardware? Which IOMMU-driver is use there?

There is no IOMMU driver in use. Etnaviv just uses part of the IOMMU API
to manage the GPU internal MMU, see
drivers/gpu/drm/etnaviv/etnaviv_iommu.c

Regards,
Lucas
Joerg Roedel Aug. 17, 2017, 2:18 p.m. UTC | #4 
On Thu, Aug 17, 2017 at 04:03:54PM +0200, Lucas Stach wrote:
> There is no IOMMU driver in use. Etnaviv just uses part of the IOMMU API
> to manage the GPU internal MMU, see
> drivers/gpu/drm/etnaviv/etnaviv_iommu.c

That looks like a very fragile construct, because it relies on internal
behavior of the iommu code that can change in the future.

I strongly suggest to either make etnaviv_iommu.c a real iommu driver an
move it to drivers/iommu, or (prefered by me) just call directly into
the map/unmap functions of this driver from the rest of the
etnaviv_iommu.c. I don't really see a reason why the IOMMU-API needs to
be used there as another layer of indirection.

Regards,

	Joerg
Lucas Stach Aug. 17, 2017, 2:30 p.m. UTC | #5 
Am Donnerstag, den 17.08.2017, 16:18 +0200 schrieb Joerg Roedel:
> On Thu, Aug 17, 2017 at 04:03:54PM +0200, Lucas Stach wrote:
> > There is no IOMMU driver in use. Etnaviv just uses part of the IOMMU API
> > to manage the GPU internal MMU, see
> > drivers/gpu/drm/etnaviv/etnaviv_iommu.c
> 
> That looks like a very fragile construct, because it relies on internal
> behavior of the iommu code that can change in the future.
> 
> I strongly suggest to either make etnaviv_iommu.c a real iommu driver an
> move it to drivers/iommu, or (prefered by me) just call directly into
> the map/unmap functions of this driver from the rest of the
> etnaviv_iommu.c. I don't really see a reason why the IOMMU-API needs to
> be used there as another layer of indirection.

Yeah, the IOMMU API being used internally is a historical accident, that
we didn't get around to rectify yet. It's on my things-we-need-to-do
list to prune the usage of the IOMMU API in etnaviv.

Regards,
Lucas
Joerg Roedel Aug. 17, 2017, 2:35 p.m. UTC | #6 
On Thu, Aug 17, 2017 at 04:30:48PM +0200, Lucas Stach wrote:
> Yeah, the IOMMU API being used internally is a historical accident, that
> we didn't get around to rectify yet. It's on my things-we-need-to-do
> list to prune the usage of the IOMMU API in etnaviv.

Okay, so for the time being, I drop the etnaviv patch from this series.


Thanks,

	Joerg
diff mbox

Patch

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index f103e78..ae0247c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -47,7 +47,7 @@  int etnaviv_iommu_map(struct etnaviv_iommu *iommu, u32 iova,
 
 		VERB("map[%d]: %08x %08x(%zx)", i, iova, pa, bytes);
 
-		ret = iommu_map(domain, da, pa, bytes, prot);
+		ret = iommu_map_sync(domain, da, pa, bytes, prot);
 		if (ret)
 			goto fail;
 
@@ -62,7 +62,7 @@  int etnaviv_iommu_map(struct etnaviv_iommu *iommu, u32 iova,
 	for_each_sg(sgt->sgl, sg, i, j) {
 		size_t bytes = sg_dma_len(sg) + sg->offset;
 
-		iommu_unmap(domain, da, bytes);
+		iommu_unmap_sync(domain, da, bytes);
 		da += bytes;
 	}
 	return ret;
@@ -80,7 +80,7 @@  int etnaviv_iommu_unmap(struct etnaviv_iommu *iommu, u32 iova,
 		size_t bytes = sg_dma_len(sg) + sg->offset;
 		size_t unmapped;
 
-		unmapped = iommu_unmap(domain, da, bytes);
+		unmapped = iommu_unmap_sync(domain, da, bytes);
 		if (unmapped < bytes)
 			return unmapped;
 
@@ -338,7 +338,7 @@  int etnaviv_iommu_get_suballoc_va(struct etnaviv_gpu *gpu, dma_addr_t paddr,
 			mutex_unlock(&mmu->lock);
 			return ret;
 		}
-		ret = iommu_map(mmu->domain, vram_node->start, paddr, size,
+		ret = iommu_map_sync(mmu->domain, vram_node->start, paddr, size,
 				IOMMU_READ);
 		if (ret < 0) {
 			drm_mm_remove_node(vram_node);
@@ -362,7 +362,7 @@  void etnaviv_iommu_put_suballoc_va(struct etnaviv_gpu *gpu,
 
 	if (mmu->version == ETNAVIV_IOMMU_V2) {
 		mutex_lock(&mmu->lock);
-		iommu_unmap(mmu->domain,iova, size);
+		iommu_unmap_sync(mmu->domain,iova, size);
 		drm_mm_remove_node(vram_node);
 		mutex_unlock(&mmu->lock);
 	}