[PATCH v3 0/3] drm/etnaviv: Trivial mmu map and ummap cleanups

[PATCH v3 0/3] drm/etnaviv: Trivial mmu map and ummap cleanups

[Sui Jingfeng]

The 'sg->offset' denotes the offset into a page in bytes, but under drm
subsystem, there has NO drivers that etnaviv can contact that actually
touch the 'offset' data member of SG anymore. This means that all DMA
addresses that sg_dma_address() gives us will be PAGE_SIZE aligned, in
other words, sg->offset will always equal to 0.

But if 'sg->offset != 0' really could happens, then the current implement
might be not correct. Previous commits[1] fix the 'sg->offset == 0' cases
effectively, below is a simple illustration.

       One CPU page       Another one CPU page
  +----+----+----+----+   +----+----+----+----+
  ||||||              |   ||||||              |
  +----+----+----+----+   +----+----+----+----+
  ^    ^                  ^    ^
  |    |                  |    |
  |    | .----------------'    |
  |    | |    .----------------'
  |    | |    |
  +----+ +----+ +----+
  |||||| |||||| |    |  GPU pages, each one is SZ_4K
  +----+ +----+ +----+
            Correct implementation.

--------------------------------------------------------------

       One CPU page       Another one CPU page
  +----+----+----+----+   +----+----+----+----+
  |///////////////////|   ||||||              |
  +----+----+----+----+   +----+----+----+----+
  ^                   ^   ^    ^
  |                   |   |    |
  |      .------------|---'    |
  |      |    .-------|--------'
  |      |    |       |
  |      +----+       |
  |      ||||||       |
  |      +----+       |
  |       IOVA        |  GPUVA range collision if use 'sg_dma_len(sg)'
  +----+ +----+-------+  directly to map. Because 'sg_dma_len(sg)' is
  |////|/|////////////|  frequently larger than SZ_4K.
  +----+ +----+-------+
            Wrong implementation.

If we map the address range with respect to the size of the backing memory,
it will occupy GPUVA ranges that doesn't belong to. Which results in GPUVA
range collision for different buffers.

Patch 0001 of this series give a fix, patch 0002 and 0003
do trivial cleanup which eliminates unnecessary overheads.

v2 -> v3
	* Reword and improve commit message
v1 -> v2
	* Reword and fix typos and mistakes

v1 Link: https://patchwork.freedesktop.org/series/140589/

Sui Jingfeng (3):
  drm/etnaviv: Drop the offset in page manipulation
  drm/etnaviv: Fix the debug log  of the etnaviv_iommu_map()
  drm/etnaviv: Improve VA, PA, SIZE alignment checking

 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] drm/etnaviv: Drop the offset in page manipulation

[Sui Jingfeng]

The etnaviv driver, both kernel space and user space, assumes that GPU page
size is 4KiB. Its IOMMU map/unmap 4KiB physical address range once a time.
If 'sg->offset != 0' is true, then the current implementation will map the
IOVA to a wrong area, which may lead to coherency problem. Picture 0 and 1
give the illustration, see below.

  PA start drifted
  |
  |<--- 'sg_dma_address(sg) - sg->offset'
  |               .------ sg_dma_address(sg)
  |              |  .---- sg_dma_len(sg)
  |<-sg->offset->|  |
  V              |<-->|    Another one cpu page
  +----+----+----+----+   +----+----+----+----+
  |xxxx|         ||||||   |||||||||||||||||||||
  +----+----+----+----+   +----+----+----+----+
  ^                   ^   ^                   ^
  |<---   da_len  --->|   |                   |
  |                   |   |                   |
  |    .--------------'   |                   |
  |    | .----------------'                   |
  |    | |                   .----------------'
  |    | |                   |
  |    | +----+----+----+----+
  |    | |||||||||||||||||||||
  |    | +----+----+----+----+
  |    |
  |    '--------------.  da_len = sg_dma_len(sg) + sg->offset, using
  |                   |  'sg_dma_len(sg) + sg->offset' will lead to GPUVA
  +----+ ~~~~~~~~~~~~~+  collision, but min_t(unsigned int, da_len, va_len)
  |xxxx|              |  will clamp it to correct size. But the IOVA will
  +----+ ~~~~~~~~~~~~~+  be redirect to wrong area.
  ^
  |             Picture 0: Possibly wrong implementation.
GPUVA (IOVA)

--------------------------------------------------------------------------

                 .------- sg_dma_address(sg)
                 |  .---- sg_dma_len(sg)
  |<-sg->offset->|  |
  |              |<-->|    another one cpu page
  +----+----+----+----+   +----+----+----+----+
  |              ||||||   |||||||||||||||||||||
  +----+----+----+----+   +----+----+----+----+
                 ^    ^   ^                   ^
                 |    |   |                   |
  .--------------'    |   |                   |
  |                   |   |                   |
  |    .--------------'   |                   |
  |    | .----------------'                   |
  |    | |                   .----------------'
  |    | |                   |
  +----+ +----+----+----+----+
  |||||| ||||||||||||||||||||| The first one is SZ_4K, the second is SZ_16K
  +----+ +----+----+----+----+
  ^
  |           Picture 1: Perfectly correct implementation.
GPUVA (IOVA)

If sg->offset != 0 is true, IOVA will be mapped to wrong physical address.
Either because there doesn't contain the data or there contains wrong data.
Strictly speaking, the memory area that before sg_dma_address(sg) doesn't
belong to us, and it's likely that the area is being used by other process.

Because we don't want to introduce confusions about which part is visible
to the GPU, we assumes that the size of GPUVA is always 4KiB aligned. This
is very relaxed requirement, since we already made the decision that GPU
page size is 4KiB (as a canonical decision). And softpin feature is landed,
Mesa's util_vma_heap_alloc() will certainly report correct length of GPUVA
to kernel with desired alignment ensured.

With above statements agreed, drop the "offset in page" manipulation will
return us a correct implementation at any case.

Fixes: a8c21a5451d8 ("drm/etnaviv: add initial etnaviv DRM driver")
Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 7e065b3723cf..c786df840a18 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -82,8 +82,8 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context,
 		return -EINVAL;
 
 	for_each_sgtable_dma_sg(sgt, sg, i) {
-		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
-		unsigned int da_len = sg_dma_len(sg) + sg->offset;
+		phys_addr_t pa = sg_dma_address(sg);
+		unsigned int da_len = sg_dma_len(sg);
 		unsigned int bytes = min_t(unsigned int, da_len, va_len);
 
 		VERB("map[%d]: %08x %pap(%x)", i, iova, &pa, bytes);
-- 
2.34.1

[PATCH v3 2/3] drm/etnaviv: Fix the debug log of the etnaviv_iommu_map()

[Sui Jingfeng]

The value of the 'iova' variable is the start GPUVA that is going to be
mapped, its value doesn't changed when the mapping is on going.

Replace it with the 'da' variable, which is incremental and it reflects
the actual address being mapped exactly.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index c786df840a18..ff90bf85c156 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -86,7 +86,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context,
 		unsigned int da_len = sg_dma_len(sg);
 		unsigned int bytes = min_t(unsigned int, da_len, va_len);
 
-		VERB("map[%d]: %08x %pap(%x)", i, iova, &pa, bytes);
+		VERB("map[%d]: %08x %pap(%x)", i, da, &pa, bytes);
 
 		ret = etnaviv_context_map(context, da, pa, bytes, prot);
 		if (ret)
-- 
2.34.1

[PATCH v3 3/3] drm/etnaviv: Improve VA, PA, SIZE alignment checking

[Sui Jingfeng]

Alignment checking is only needed to be done in the upper caller function.
If those address and sizes are able to pass the check, it will certainly
pass the same test in the etnaviv_context_unmap() function. We don't need
examine it more than once.

Remove redundant alignment tests, move the those useless to upper caller
function.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index ff90bf85c156..df5192083b20 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -19,12 +19,6 @@ static void etnaviv_context_unmap(struct etnaviv_iommu_context *context,
 	size_t unmapped_page, unmapped = 0;
 	size_t pgsize = SZ_4K;
 
-	if (!IS_ALIGNED(iova | size, pgsize)) {
-		pr_err("unaligned: iova 0x%lx size 0x%zx min_pagesz 0x%zx\n",
-		       iova, size, pgsize);
-		return;
-	}
-
 	while (unmapped < size) {
 		unmapped_page = context->global->ops->unmap(context, iova,
 							    pgsize);
@@ -45,12 +39,6 @@ static int etnaviv_context_map(struct etnaviv_iommu_context *context,
 	size_t orig_size = size;
 	int ret = 0;
 
-	if (!IS_ALIGNED(iova | paddr | size, pgsize)) {
-		pr_err("unaligned: iova 0x%lx pa %pa size 0x%zx min_pagesz 0x%zx\n",
-		       iova, &paddr, size, pgsize);
-		return -EINVAL;
-	}
-
 	while (size) {
 		ret = context->global->ops->map(context, iova, paddr, pgsize,
 						prot);
@@ -88,6 +76,14 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context,
 
 		VERB("map[%d]: %08x %pap(%x)", i, da, &pa, bytes);
 
+		if (!IS_ALIGNED(iova | pa | bytes, SZ_4K)) {
+			dev_err(context->global->dev,
+				"unaligned: iova 0x%x pa %pa size 0x%x\n",
+				iova, &pa, bytes);
+			ret = -EINVAL;
+			goto fail;
+		}
+
 		ret = etnaviv_context_map(context, da, pa, bytes, prot);
 		if (ret)
 			goto fail;
-- 
2.34.1

Re: [PATCH v3 0/3] drm/etnaviv: Trivial mmu map and ummap cleanups

[Lucas Stach]

Am Freitag, dem 15.11.2024 um 20:32 +0800 schrieb Sui Jingfeng:
> The 'sg->offset' denotes the offset into a page in bytes, but under drm
> subsystem, there has NO drivers that etnaviv can contact that actually
> touch the 'offset' data member of SG anymore. This means that all DMA
> addresses that sg_dma_address() gives us will be PAGE_SIZE aligned, in
> other words, sg->offset will always equal to 0.
> 
> But if 'sg->offset != 0' really could happens, then the current implement
> might be not correct. Previous commits[1] fix the 'sg->offset == 0' cases
> effectively, below is a simple illustration.
> 
>        One CPU page       Another one CPU page
>   +----+----+----+----+   +----+----+----+----+
>   ||||||              |   ||||||              |
>   +----+----+----+----+   +----+----+----+----+
>   ^    ^                  ^    ^
>   |    |                  |    |
>   |    | .----------------'    |
>   |    | |    .----------------'
>   |    | |    |
>   +----+ +----+ +----+
>   |||||| |||||| |    |  GPU pages, each one is SZ_4K
>   +----+ +----+ +----+
>             Correct implementation.
> 
> --------------------------------------------------------------
> 
>        One CPU page       Another one CPU page
>   +----+----+----+----+   +----+----+----+----+
>   |///////////////////|   ||||||              |
>   +----+----+----+----+   +----+----+----+----+
>   ^                   ^   ^    ^
>   |                   |   |    |
>   |      .------------|---'    |
>   |      |    .-------|--------'
>   |      |    |       |
>   |      +----+       |
>   |      ||||||       |
>   |      +----+       |
>   |       IOVA        |  GPUVA range collision if use 'sg_dma_len(sg)'
>   +----+ +----+-------+  directly to map. Because 'sg_dma_len(sg)' is
>   |////|/|////////////|  frequently larger than SZ_4K.
>   +----+ +----+-------+
>             Wrong implementation.
> 
> If we map the address range with respect to the size of the backing memory,
> it will occupy GPUVA ranges that doesn't belong to. Which results in GPUVA
> range collision for different buffers.
> 
> Patch 0001 of this series give a fix, patch 0002 and 0003
> do trivial cleanup which eliminates unnecessary overheads.

Thanks, applied to etnaviv/next.

Regards,
Lucas

> v2 -> v3
> 	* Reword and improve commit message
> v1 -> v2
> 	* Reword and fix typos and mistakes
> 
> v1 Link: https://patchwork.freedesktop.org/series/140589/
> 
> Sui Jingfeng (3):
>   drm/etnaviv: Drop the offset in page manipulation
>   drm/etnaviv: Fix the debug log  of the etnaviv_iommu_map()
>   drm/etnaviv: Improve VA, PA, SIZE alignment checking
> 
>  drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>