[PATCH 1/2] drm/etnaviv: Fix misunderstanding about the scatterlist structure

[PATCH 1/2] drm/etnaviv: Fix misunderstanding about the scatterlist structure

[Sui Jingfeng]

The 'offset' data member of the 'struct scatterlist' denotes the offset
into a SG entry in bytes. The sg_dma_len() macro could be used to get
lengths of SG entries, those lengths are expected to be CPU page size
aligned. Since, at least for now, we call drm_prime_pages_to_sg() to
convert our various page array into an SG list. We pass the number of
CPU page as the third argoument, to tell the size of the backing memory
of GEM buffer object.

drm_prime_pages_to_sg() call sg_alloc_table_from_pages_segment() do the
work, sg_alloc_table_from_pages_segment() always hardcode the Offset to
ZERO. The sizes of *all* SG enties will be a multiple of CPU page size,
that is multiple of PAGE_SIZE.

If the GPU want to map/unmap a bigger page partially, we should use
'sg_dma_address(sg) + sg->offset' to calculate the destination DMA
address, and the size to be map/unmap is 'sg_dma_len(sg) - sg->offset'.

While the current implement is wrong, but since the 'sg->offset' is
alway equal to 0, drm/etnaviv works in practice by good luck. Fix this,
to make it looks right at least from the perspective of concept.

while at it, always fix the absue types:

- sg_dma_address returns DMA address, the type is dma_addr_t, not
  the phys_addr_t, for VRAM there may have another translation between
  the bus address and the final physical address of VRAM or carved out
  RAM.

- The type of sg_dma_len(sg) return is unsigned int, not the size_t.
  Avoid hint the compiler to do unnecessary integer promotion.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 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 1661d589bf3e..4ee9ed96b1d8 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -80,10 +80,10 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 		return -EINVAL;
 
 	for_each_sgtable_dma_sg(sgt, sg, i) {
-		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
-		size_t bytes = sg_dma_len(sg) + sg->offset;
+		dma_addr_t pa = sg_dma_address(sg) + sg->offset;
+		unsigned int bytes = sg_dma_len(sg) - sg->offset;
 
-		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
+		VERB("map[%d]: %08x %pap(%x)", i, iova, &pa, bytes);
 
 		ret = etnaviv_context_map(context, da, pa, bytes, prot);
 		if (ret)
@@ -109,11 +109,11 @@ static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
 	int i;
 
 	for_each_sgtable_dma_sg(sgt, sg, i) {
-		size_t bytes = sg_dma_len(sg) + sg->offset;
+		unsigned int bytes = sg_dma_len(sg) - sg->offset;
 
 		etnaviv_context_unmap(context, da, bytes);
 
-		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
+		VERB("unmap[%d]: %08x(%x)", i, iova, bytes);
 
 		BUG_ON(!PAGE_ALIGNED(bytes));
 
-- 
2.34.1

[PATCH 2/2] drm/etnaviv: Fix the debug log for the mmu map/unmap procudure

[Sui Jingfeng]

The 'iova' variable is invarant within one invoke of the
etnaviv_iommu_unmap()/etnaviv_iommu_unmap(), which means that the
debug log always print the same GPU virtual address while mapping
or unmaping.

Made the GPU virtual address being printed increment with real GPUVA
being mapped or unmapped.

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 4ee9ed96b1d8..f6c997c459ca 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -83,7 +83,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 		dma_addr_t pa = sg_dma_address(sg) + sg->offset;
 		unsigned int bytes = sg_dma_len(sg) - sg->offset;
 
-		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)
@@ -113,7 +113,7 @@ static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
 
 		etnaviv_context_unmap(context, da, bytes);
 
-		VERB("unmap[%d]: %08x(%x)", i, iova, bytes);
+		VERB("unmap[%d]: %08x(%x)", i, da, bytes);
 
 		BUG_ON(!PAGE_ALIGNED(bytes));
 
-- 
2.34.1

Re: [PATCH 1/2] drm/etnaviv: Fix misunderstanding about the scatterlist structure

[Sui Jingfeng]

Hi, Dear reviewers

On 2024/10/29 00:05, Sui Jingfeng wrote:
> The 'offset' data member of the 'struct scatterlist' denotes the offset
> into a SG entry in bytes. The sg_dma_len() macro could be used to get
> lengths of SG entries, those lengths are expected to be CPU page size
> aligned. Since, at least for now, we call drm_prime_pages_to_sg() to
> convert our various page array into an SG list. We pass the number of
> CPU page as the third argoument, to tell the size of the backing memory
> of GEM buffer object.
>
> drm_prime_pages_to_sg() call sg_alloc_table_from_pages_segment() do the
> work, sg_alloc_table_from_pages_segment() always hardcode the Offset to
> ZERO. The sizes of *all* SG enties will be a multiple of CPU page size,
> that is multiple of PAGE_SIZE.
>
> If the GPU want to map/unmap a bigger page partially, we should use
> 'sg_dma_address(sg) + sg->offset' to calculate the destination DMA
> address, and the size to be map/unmap is 'sg_dma_len(sg) - sg->offset'.
>
> While the current implement is wrong, but since the 'sg->offset' is
> alway equal to 0, drm/etnaviv works in practice by good luck. Fix this,
> to make it looks right at least from the perspective of concept.
>
> while at it, always fix the absue types:


'absue' -> 'abuse'


By the way, sorry I'm just receive your message from my Thunderbird client.


I sent those two patch first, then I run my Thunderbird client.

Not seeing that you have already merged part of my patch, then there will be
merge conflict I guess.

I think I could wait the next round and respin my patch.


> - sg_dma_address returns DMA address, the type is dma_addr_t, not
>    the phys_addr_t, for VRAM there may have another translation between
>    the bus address and the final physical address of VRAM or carved out
>    RAM.
>
> - The type of sg_dma_len(sg) return is unsigned int, not the size_t.
>    Avoid hint the compiler to do unnecessary integer promotion.
>
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>   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 1661d589bf3e..4ee9ed96b1d8 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> @@ -80,10 +80,10 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>   		return -EINVAL;
>   
>   	for_each_sgtable_dma_sg(sgt, sg, i) {
> -		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
> -		size_t bytes = sg_dma_len(sg) + sg->offset;
> +		dma_addr_t pa = sg_dma_address(sg) + sg->offset;
> +		unsigned int bytes = sg_dma_len(sg) - sg->offset;
>   
> -		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
> +		VERB("map[%d]: %08x %pap(%x)", i, iova, &pa, bytes);
>   
>   		ret = etnaviv_context_map(context, da, pa, bytes, prot);
>   		if (ret)
> @@ -109,11 +109,11 @@ static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
>   	int i;
>   
>   	for_each_sgtable_dma_sg(sgt, sg, i) {
> -		size_t bytes = sg_dma_len(sg) + sg->offset;
> +		unsigned int bytes = sg_dma_len(sg) - sg->offset;
>   
>   		etnaviv_context_unmap(context, da, bytes);
>   
> -		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
> +		VERB("unmap[%d]: %08x(%x)", i, iova, bytes);
>   
>   		BUG_ON(!PAGE_ALIGNED(bytes));
>   

-- 
Best regards,
Sui

Re: [PATCH 1/2] drm/etnaviv: Fix misunderstanding about the scatterlist structure

[Sui Jingfeng]

Hi,


On 2024/10/29 00:05, Sui Jingfeng wrote:
> The 'offset' data member of the 'struct scatterlist' denotes the offset
> into a SG entry in bytes. The sg_dma_len() macro could be used to get
> lengths of SG entries, those lengths are expected to be CPU page size
> aligned. Since, at least for now, we call drm_prime_pages_to_sg() to
> convert our various page array into an SG list. We pass the number of
> CPU page as the third argoument, to tell the size of the backing memory
> of GEM buffer object.
>
> drm_prime_pages_to_sg() call sg_alloc_table_from_pages_segment() do the
> work, sg_alloc_table_from_pages_segment() always hardcode the Offset to
> ZERO. The sizes of *all* SG enties will be a multiple of CPU page size,
> that is multiple of PAGE_SIZE.
>
> If the GPU want to map/unmap a bigger page partially, we should use
> 'sg_dma_address(sg) + sg->offset' to calculate the destination DMA
> address, and the size to be map/unmap is 'sg_dma_len(sg) - sg->offset'.
>
> While the current implement is wrong, but since the 'sg->offset' is
> alway equal to 0, drm/etnaviv works in practice by good luck. Fix this,
> to make it looks right at least from the perspective of concept.
>
> while at it, always fix the absue types:
>
> - sg_dma_address returns DMA address, the type is dma_addr_t, not
>    the phys_addr_t, for VRAM there may have another translation between
>    the bus address and the final physical address of VRAM or carved out
>    RAM.
>
> - The type of sg_dma_len(sg) return is unsigned int, not the size_t.
>    Avoid hint the compiler to do unnecessary integer promotion.
>
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>   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 1661d589bf3e..4ee9ed96b1d8 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> @@ -80,10 +80,10 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>   		return -EINVAL;
>   
>   	for_each_sgtable_dma_sg(sgt, sg, i) {
> -		phys_addr_t pa = sg_dma_address(sg) - sg->offset;
> -		size_t bytes = sg_dma_len(sg) + sg->offset;


Wow, I know what's want here now.

What's you want here is to let the GPU map the entire page, not partially mapping just implemented.

But the area doesn't belong to us isn't right? Could lead to GPU out-of-bound access?


 From the perfect mapping perspective, we should just map from where the
sg_dma_address(sg) tell us, just use the sg_dma_len(sg) as length.


> +		dma_addr_t pa = sg_dma_address(sg) + sg->offset;
> +		unsigned int bytes = sg_dma_len(sg) - sg->offset;

Neither 'sg_dma_len(sg) + sg->offset' nor 'sg_dma_len(sg) - sg->offset' is correct.

Considering that when we are PRIME sharing buffer with another driver or
sharing buffer with the CPU.

If CPU stores the data at the middle position(say 2KiB of 4KiB),
then we have to tell the GPU fetch the data from the 2KiB of 4KiB,
not the 0 KiB of 4KiB. Seems quite difficult.
  
It could lead to concurrency problem of CPU put data at
'sg_dma_address(sg) + sg->offset', and GPU fetch the data
from sg_dma_address(sg) if 'sg->offset != 0'

So have the 'sg->offset != 0' is a bad idea. So, let's ignore
this and force 'sg->offset = 0' everywhere.

Thanks.

>   
> -		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
> +		VERB("map[%d]: %08x %pap(%x)", i, iova, &pa, bytes);
>   
>   		ret = etnaviv_context_map(context, da, pa, bytes, prot);
>   		if (ret)
> @@ -109,11 +109,11 @@ static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
>   	int i;
>   
>   	for_each_sgtable_dma_sg(sgt, sg, i) {
> -		size_t bytes = sg_dma_len(sg) + sg->offset;
> +		unsigned int bytes = sg_dma_len(sg) - sg->offset;
>   
>   		etnaviv_context_unmap(context, da, bytes);
>   
> -		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
> +		VERB("unmap[%d]: %08x(%x)", i, iova, bytes);
>   
>   		BUG_ON(!PAGE_ALIGNED(bytes));
>   

-- 
Best regards,
Sui