[PATCH 0/3] Fix GPU virtual address collosion when CPU page size != GPU page size

[PATCH 0/3] Fix GPU virtual address collosion when CPU page size != GPU page size

[Sui Jingfeng]

Etnaviv assumes that GPU page size is 4KiB, however, when using
softpin capable GPUs on a different CPU page size configuration.
Userspace still doing the allocation with 4KiB page as unit. This
results in userspace allocated GPU virtual address ranges collision
and therefore unable to be inserted to the specified hole exactly.

The root cause is that kernel side BO takes up bigger address space
than userspace assumes when the size of it is not CPU page size aligned.

To solve it with no GPU VA range space wasting, we first track the size
of a buffer that userspace/GPU think of it is, then partially map and/or
unmap the tail physical page with respect to this 'user size'. Ensure
that GPU VA is fully mapped and/or unmapped.

Sui Jingfeng (3):
  drm/etnaviv: Track GPU VA size separately
  drm/etnaviv: Map and unmap the GPU VA range with respect to its user
    size
  drm/etnaviv: Print an error message if inserting IOVA range fails

 drivers/gpu/drm/etnaviv/etnaviv_gem.c |  8 +++--
 drivers/gpu/drm/etnaviv/etnaviv_gem.h |  1 +
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 42 ++++++++++++---------------
 3 files changed, 24 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH 1/3] drm/etnaviv: Track GPU VA size separately

[Sui Jingfeng]

Etnaviv assumes that GPU page size is 4KiB, yet on some systems, the CPU
page size is 16KiB. The size of etnaviv buffer objects will be aligned
to CPU page size on kernel side, however, userspace still assumes the
page size is 4KiB and doing allocation with 4KiB page as unit. This
results in userspace allocated GPU virtual address range collision and
therefore unable to be inserted to the specified hole exactly.

The root cause is that kernel side BO takes up bigger address space than
userspace assumes when the size of it is not CPU page size aligned. To
Preserve GPU VA continuous as much as possible, track the size that
userspace/GPU think of it is.

Yes, we still need to overallocate to suit the CPU, but there is no need
to waste GPU VA space anymore.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.c | 8 +++++---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h | 1 +
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 8 ++++----
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 5c0c9d4e3be1..943fc20093e6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -543,7 +543,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
 	.vm_ops = &vm_ops,
 };
 
-static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
+static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
 	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
 {
 	struct etnaviv_gem_object *etnaviv_obj;
@@ -570,6 +570,7 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
 	if (!etnaviv_obj)
 		return -ENOMEM;
 
+	etnaviv_obj->user_size = size;
 	etnaviv_obj->flags = flags;
 	etnaviv_obj->ops = ops;
 
@@ -588,11 +589,12 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 {
 	struct etnaviv_drm_private *priv = dev->dev_private;
 	struct drm_gem_object *obj = NULL;
+	unsigned int user_size = size;
 	int ret;
 
 	size = PAGE_ALIGN(size);
 
-	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
+	ret = etnaviv_gem_new_impl(dev, user_size, flags, &etnaviv_gem_shmem_ops, &obj);
 	if (ret)
 		goto fail;
 
@@ -627,7 +629,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
 	struct drm_gem_object *obj;
 	int ret;
 
-	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
+	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index a42d260cac2c..c6e27b9abb0c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -36,6 +36,7 @@ struct etnaviv_gem_object {
 	const struct etnaviv_gem_ops *ops;
 	struct mutex lock;
 
+	u32 user_size;
 	u32 flags;
 
 	struct list_head gem_node;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 1661d589bf3e..6fbc62772d85 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -281,6 +281,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
 {
 	struct sg_table *sgt = etnaviv_obj->sgt;
 	struct drm_mm_node *node;
+	unsigned int user_size;
 	int ret;
 
 	lockdep_assert_held(&etnaviv_obj->lock);
@@ -303,13 +304,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
 	}
 
 	node = &mapping->vram_node;
+	user_size = etnaviv_obj->user_size;
 
 	if (va)
-		ret = etnaviv_iommu_insert_exact(context, node,
-						 etnaviv_obj->base.size, va);
+		ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
 	else
-		ret = etnaviv_iommu_find_iova(context, node,
-					      etnaviv_obj->base.size);
+		ret = etnaviv_iommu_find_iova(context, node, user_size);
 	if (ret < 0)
 		goto unlock;
 
-- 
2.34.1

Re: [PATCH 1/3] drm/etnaviv: Track GPU VA size separately

[Lucas Stach]

Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
> Etnaviv assumes that GPU page size is 4KiB, yet on some systems, the CPU
> page size is 16KiB. The size of etnaviv buffer objects will be aligned
> to CPU page size on kernel side, however, userspace still assumes the
> page size is 4KiB and doing allocation with 4KiB page as unit. This
> results in userspace allocated GPU virtual address range collision and
> therefore unable to be inserted to the specified hole exactly.
> 
> The root cause is that kernel side BO takes up bigger address space than
> userspace assumes when the size of it is not CPU page size aligned. To
> Preserve GPU VA continuous as much as possible, track the size that
> userspace/GPU think of it is.
> 
> Yes, we still need to overallocate to suit the CPU, but there is no need
> to waste GPU VA space anymore.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 8 +++++---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h | 1 +
>  drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 8 ++++----
>  3 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 5c0c9d4e3be1..943fc20093e6 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -543,7 +543,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>  	.vm_ops = &vm_ops,
>  };
>  
> -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
> +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>  	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>  {
>  	struct etnaviv_gem_object *etnaviv_obj;
> @@ -570,6 +570,7 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>  	if (!etnaviv_obj)
>  		return -ENOMEM;
>  
> +	etnaviv_obj->user_size = size;
>  	etnaviv_obj->flags = flags;
>  	etnaviv_obj->ops = ops;
>  
> @@ -588,11 +589,12 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>  {
>  	struct etnaviv_drm_private *priv = dev->dev_private;
>  	struct drm_gem_object *obj = NULL;
> +	unsigned int user_size = size;

This still needs to be be aligned to 4K. Userspace may request
unaligned buffer sizes and we don't want to risk any confusion about
which part is visible to the GPU, so better make sure this size is
aligned to the GPU page size.

Also, that more personal preference, but I would call this gpu_size or
something like that, to avoid any confusion with the user_size in
etnaviv_cmdbuf, where user_size doesn't denote the GPU visible size.

Regards,
Lucas

>  	int ret;
>  
>  	size = PAGE_ALIGN(size);
>  
> -	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
> +	ret = etnaviv_gem_new_impl(dev, user_size, flags, &etnaviv_gem_shmem_ops, &obj);
>  	if (ret)
>  		goto fail;
>  
> @@ -627,7 +629,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>  	struct drm_gem_object *obj;
>  	int ret;
>  
> -	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
> +	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> index a42d260cac2c..c6e27b9abb0c 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> @@ -36,6 +36,7 @@ struct etnaviv_gem_object {
>  	const struct etnaviv_gem_ops *ops;
>  	struct mutex lock;
>  
> +	u32 user_size;
>  	u32 flags;
>  
>  	struct list_head gem_node;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> index 1661d589bf3e..6fbc62772d85 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> @@ -281,6 +281,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>  {
>  	struct sg_table *sgt = etnaviv_obj->sgt;
>  	struct drm_mm_node *node;
> +	unsigned int user_size;
>  	int ret;
>  
>  	lockdep_assert_held(&etnaviv_obj->lock);
> @@ -303,13 +304,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>  	}
>  
>  	node = &mapping->vram_node;
> +	user_size = etnaviv_obj->user_size;
>  
>  	if (va)
> -		ret = etnaviv_iommu_insert_exact(context, node,
> -						 etnaviv_obj->base.size, va);
> +		ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
>  	else
> -		ret = etnaviv_iommu_find_iova(context, node,
> -					      etnaviv_obj->base.size);
> +		ret = etnaviv_iommu_find_iova(context, node, user_size);
>  	if (ret < 0)
>  		goto unlock;
>  

Re: [PATCH 1/3] drm/etnaviv: Track GPU VA size separately

[Sui Jingfeng]

Hi,

On 2024/10/7 18:12, Lucas Stach wrote:
> Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
>> Etnaviv assumes that GPU page size is 4KiB, yet on some systems, the CPU
>> page size is 16KiB. The size of etnaviv buffer objects will be aligned
>> to CPU page size on kernel side, however, userspace still assumes the
>> page size is 4KiB and doing allocation with 4KiB page as unit. This
>> results in userspace allocated GPU virtual address range collision and
>> therefore unable to be inserted to the specified hole exactly.
>>
>> The root cause is that kernel side BO takes up bigger address space than
>> userspace assumes when the size of it is not CPU page size aligned. To
>> Preserve GPU VA continuous as much as possible, track the size that
>> userspace/GPU think of it is.
>>
>> Yes, we still need to overallocate to suit the CPU, but there is no need
>> to waste GPU VA space anymore.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c | 8 +++++---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.h | 1 +
>>   drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 8 ++++----
>>   3 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> index 5c0c9d4e3be1..943fc20093e6 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> @@ -543,7 +543,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>>   	.vm_ops = &vm_ops,
>>   };
>>   
>> -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>> +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>>   	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>>   {
>>   	struct etnaviv_gem_object *etnaviv_obj;
>> @@ -570,6 +570,7 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>>   	if (!etnaviv_obj)
>>   		return -ENOMEM;
>>   
>> +	etnaviv_obj->user_size = size;
>>   	etnaviv_obj->flags = flags;
>>   	etnaviv_obj->ops = ops;
>>   
>> @@ -588,11 +589,12 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>>   {
>>   	struct etnaviv_drm_private *priv = dev->dev_private;
>>   	struct drm_gem_object *obj = NULL;
>> +	unsigned int user_size = size;
> This still needs to be be aligned to 4K. Userspace may request
> unaligned buffer sizes and we don't want to risk any confusion about
> which part is visible to the GPU, so better make sure this size is
> aligned to the GPU page size.


OK,aligned to the GPU page size is reasonable. Since the buffer is very high likely be used by GPU.


> Also, that more personal preference, but I would call this gpu_size or
> something like that, to avoid any confusion with the user_size in
> etnaviv_cmdbuf, where user_size doesn't denote the GPU visible size.

Yeah, theuser_size denote the length of command buffer, it's usually just need to 
aligned to 8 bytes. And generally, the size command buffer won't larger 
than 4KiB (a GPU PAGE).

I'm imagine that just 'size' with some extra comment, as it's possible
that a buffer is only get used by CPU for specific purpose.

Best Regards,
Sui

> Regards,
> Lucas
>
>>   	int ret;
>>   
>>   	size = PAGE_ALIGN(size);
>>   
>> -	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
>> +	ret = etnaviv_gem_new_impl(dev, user_size, flags, &etnaviv_gem_shmem_ops, &obj);
>>   	if (ret)
>>   		goto fail;
>>   
>> @@ -627,7 +629,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>>   	struct drm_gem_object *obj;
>>   	int ret;
>>   
>> -	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
>> +	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
>>   	if (ret)
>>   		return ret;
>>   
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> index a42d260cac2c..c6e27b9abb0c 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> @@ -36,6 +36,7 @@ struct etnaviv_gem_object {
>>   	const struct etnaviv_gem_ops *ops;
>>   	struct mutex lock;
>>   
>> +	u32 user_size;
>>   	u32 flags;
>>   
>>   	struct list_head gem_node;
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> index 1661d589bf3e..6fbc62772d85 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> @@ -281,6 +281,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>>   {
>>   	struct sg_table *sgt = etnaviv_obj->sgt;
>>   	struct drm_mm_node *node;
>> +	unsigned int user_size;
>>   	int ret;
>>   
>>   	lockdep_assert_held(&etnaviv_obj->lock);
>> @@ -303,13 +304,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>>   	}
>>   
>>   	node = &mapping->vram_node;
>> +	user_size = etnaviv_obj->user_size;
>>   
>>   	if (va)
>> -		ret = etnaviv_iommu_insert_exact(context, node,
>> -						 etnaviv_obj->base.size, va);
>> +		ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
>>   	else
>> -		ret = etnaviv_iommu_find_iova(context, node,
>> -					      etnaviv_obj->base.size);
>> +		ret = etnaviv_iommu_find_iova(context, node, user_size);
>>   	if (ret < 0)
>>   		goto unlock;
>>   

-- 
Best regards,
Sui

Re: [PATCH 1/3] drm/etnaviv: Track GPU VA size separately

[Sui Jingfeng]

Hi,

On 10/7/24 18:12, Lucas Stach wrote:
> Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
>> Etnaviv assumes that GPU page size is 4KiB, yet on some systems, the CPU
>> page size is 16KiB. The size of etnaviv buffer objects will be aligned
>> to CPU page size on kernel side, however, userspace still assumes the
>> page size is 4KiB and doing allocation with 4KiB page as unit. This
>> results in userspace allocated GPU virtual address range collision and
>> therefore unable to be inserted to the specified hole exactly.
>>
>> The root cause is that kernel side BO takes up bigger address space than
>> userspace assumes when the size of it is not CPU page size aligned. To
>> Preserve GPU VA continuous as much as possible, track the size that
>> userspace/GPU think of it is.
>>
>> Yes, we still need to overallocate to suit the CPU, but there is no need
>> to waste GPU VA space anymore.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c | 8 +++++---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.h | 1 +
>>   drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 8 ++++----
>>   3 files changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> index 5c0c9d4e3be1..943fc20093e6 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> @@ -543,7 +543,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>>   	.vm_ops = &vm_ops,
>>   };
>>   
>> -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>> +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>>   	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>>   {
>>   	struct etnaviv_gem_object *etnaviv_obj;
>> @@ -570,6 +570,7 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
>>   	if (!etnaviv_obj)
>>   		return -ENOMEM;
>>   
>> +	etnaviv_obj->user_size = size;
>>   	etnaviv_obj->flags = flags;
>>   	etnaviv_obj->ops = ops;
>>   
>> @@ -588,11 +589,12 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
>>   {
>>   	struct etnaviv_drm_private *priv = dev->dev_private;
>>   	struct drm_gem_object *obj = NULL;
>> +	unsigned int user_size = size;
> 
> This still needs to be be aligned to 4K. 

Yes, extremely correct here, for the perspective of concept.

Have to be GPU page size aligned, because the GPU map 4KiB once a time.
GPU will access full range of a 4KiB page, and this is out of CPU's
control.

> Userspace may request unaligned buffer sizes 

User-space shall *NOT* request unaligned buffer, since user-space
*already* made the assumption GPU page is 4KiB. Then it's the user
space's responsibility that keeping requested buffer aligned.

- The kernel space actually can and *should* return aligned size
   to user-space though.

- Since softpin feature is landed, it becomes evident that kernel
   space need user-space *report* a correct length of GPUVA.

But I'm fine with the kernel pay some extra price for safe reasons.

Best regards,
Sui

> and we don't want to risk any confusion about
> which part is visible to the GPU, so better make sure this size is
> aligned to the GPU page size.
> Also, that more personal preference, but I would call this gpu_size or
> something like that, to avoid any confusion with the user_size in> etnaviv_cmdbuf, where user_size doesn't denote the GPU visible size.
>
> Regards,
> Lucas
> 
>>   	int ret;
>>   
>>   	size = PAGE_ALIGN(size);
>>   
>> -	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
>> +	ret = etnaviv_gem_new_impl(dev, user_size, flags, &etnaviv_gem_shmem_ops, &obj);
>>   	if (ret)
>>   		goto fail;
>>   
>> @@ -627,7 +629,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
>>   	struct drm_gem_object *obj;
>>   	int ret;
>>   
>> -	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
>> +	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
>>   	if (ret)
>>   		return ret;
>>   
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> index a42d260cac2c..c6e27b9abb0c 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> @@ -36,6 +36,7 @@ struct etnaviv_gem_object {
>>   	const struct etnaviv_gem_ops *ops;
>>   	struct mutex lock;
>>   
>> +	u32 user_size;
>>   	u32 flags;
>>   
>>   	struct list_head gem_node;
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> index 1661d589bf3e..6fbc62772d85 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> @@ -281,6 +281,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>>   {
>>   	struct sg_table *sgt = etnaviv_obj->sgt;
>>   	struct drm_mm_node *node;
>> +	unsigned int user_size;
>>   	int ret;
>>   
>>   	lockdep_assert_held(&etnaviv_obj->lock);
>> @@ -303,13 +304,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>>   	}
>>   
>>   	node = &mapping->vram_node;
>> +	user_size = etnaviv_obj->user_size;
>>   
>>   	if (va)
>> -		ret = etnaviv_iommu_insert_exact(context, node,
>> -						 etnaviv_obj->base.size, va);
>> +		ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
>>   	else
>> -		ret = etnaviv_iommu_find_iova(context, node,
>> -					      etnaviv_obj->base.size);
>> +		ret = etnaviv_iommu_find_iova(context, node, user_size);
>>   	if (ret < 0)
>>   		goto unlock;
>>   
> 

-- 
Best regards
Sui

[PATCH 2/3] drm/etnaviv: Map and unmap the GPU VA range with respect to its user size

[Sui Jingfeng]

Since the GPU VA space is compact in terms of 4KiB unit, map and/or unmap
the area that doesn't belong to a context breaks the philosophy of PPAS.
That results in severe errors: GPU hang and MMU fault (page not present)
and such.

Shrink the usuable size of etnaviv GEM buffer object to its user size,
instead of the original physical size of its backing memory.

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

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 6fbc62772d85..a52ec5eb0e3d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -70,8 +70,10 @@ static int etnaviv_context_map(struct etnaviv_iommu_context *context,
 }
 
 static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
+			     unsigned int user_len,
 			     struct sg_table *sgt, int prot)
-{	struct scatterlist *sg;
+{
+	struct scatterlist *sg;
 	unsigned int da = iova;
 	unsigned int i;
 	int ret;
@@ -81,7 +83,8 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 
 	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;
+		unsigned int phys_len = sg_dma_len(sg) + sg->offset;
+		size_t bytes = MIN(phys_len, user_len);
 
 		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
 
@@ -89,6 +92,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 		if (ret)
 			goto fail;
 
+		user_len -= bytes;
 		da += bytes;
 	}
 
@@ -104,21 +108,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
 static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
 				struct sg_table *sgt, unsigned len)
 {
-	struct scatterlist *sg;
-	unsigned int da = iova;
-	int i;
-
-	for_each_sgtable_dma_sg(sgt, sg, i) {
-		size_t bytes = sg_dma_len(sg) + sg->offset;
-
-		etnaviv_context_unmap(context, da, bytes);
-
-		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
-
-		BUG_ON(!PAGE_ALIGNED(bytes));
-
-		da += bytes;
-	}
+	etnaviv_context_unmap(context, iova, len);
 
 	context->flush_seq++;
 }
@@ -131,7 +121,7 @@ static void etnaviv_iommu_remove_mapping(struct etnaviv_iommu_context *context,
 	lockdep_assert_held(&context->lock);
 
 	etnaviv_iommu_unmap(context, mapping->vram_node.start,
-			    etnaviv_obj->sgt, etnaviv_obj->base.size);
+			    etnaviv_obj->sgt, etnaviv_obj->user_size);
 	drm_mm_remove_node(&mapping->vram_node);
 }
 
@@ -314,7 +304,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
 		goto unlock;
 
 	mapping->iova = node->start;
-	ret = etnaviv_iommu_map(context, node->start, sgt,
+	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,
 				ETNAVIV_PROT_READ | ETNAVIV_PROT_WRITE);
 
 	if (ret < 0) {
-- 
2.34.1

Re: [PATCH 2/3] drm/etnaviv: Map and unmap the GPU VA range with respect to its user size

[Lucas Stach]

Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
> Since the GPU VA space is compact in terms of 4KiB unit, map and/or unmap
> the area that doesn't belong to a context breaks the philosophy of PPAS.
> That results in severe errors: GPU hang and MMU fault (page not present)
> and such.
> 
> Shrink the usuable size of etnaviv GEM buffer object to its user size,
> instead of the original physical size of its backing memory.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 28 +++++++++------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> index 6fbc62772d85..a52ec5eb0e3d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> @@ -70,8 +70,10 @@ static int etnaviv_context_map(struct etnaviv_iommu_context *context,
>  }
>  
>  static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
> +			     unsigned int user_len,
>  			     struct sg_table *sgt, int prot)
> -{	struct scatterlist *sg;
> +{
> +	struct scatterlist *sg;
>  	unsigned int da = iova;
>  	unsigned int i;
>  	int ret;
> @@ -81,7 +83,8 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>  
>  	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;
> +		unsigned int phys_len = sg_dma_len(sg) + sg->offset;
> +		size_t bytes = MIN(phys_len, user_len);
>  
>  		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
>  
> @@ -89,6 +92,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>  		if (ret)
>  			goto fail;
>  
> +		user_len -= bytes;
>  		da += bytes;
>  	}

Since the MIN(phys_len, user_len) may limit the mapped amount in the
wrong direction, I would think it would be good to add a
WARN_ON(user_len != 0) after the dma SG iteration.

>  
> @@ -104,21 +108,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>  static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
>  				struct sg_table *sgt, unsigned len)
>  {
> -	struct scatterlist *sg;
> -	unsigned int da = iova;
> -	int i;
> -
> -	for_each_sgtable_dma_sg(sgt, sg, i) {
> -		size_t bytes = sg_dma_len(sg) + sg->offset;
> -
> -		etnaviv_context_unmap(context, da, bytes);
> -
> -		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
> -
> -		BUG_ON(!PAGE_ALIGNED(bytes));
> -
> -		da += bytes;
> -	}
> +	etnaviv_context_unmap(context, iova, len);

This drops some sanity checks, but I have only ever seen them fire when
we had other kernel memory corruption issues, so I'm fine with the
simplification you did here.

Regards,
Lucas

>  
>  	context->flush_seq++;
>  }
> @@ -131,7 +121,7 @@ static void etnaviv_iommu_remove_mapping(struct etnaviv_iommu_context *context,
>  	lockdep_assert_held(&context->lock);
>  
>  	etnaviv_iommu_unmap(context, mapping->vram_node.start,
> -			    etnaviv_obj->sgt, etnaviv_obj->base.size);
> +			    etnaviv_obj->sgt, etnaviv_obj->user_size);
>  	drm_mm_remove_node(&mapping->vram_node);
>  }
>  
> @@ -314,7 +304,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>  		goto unlock;
>  
>  	mapping->iova = node->start;
> -	ret = etnaviv_iommu_map(context, node->start, sgt,
> +	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,
>  				ETNAVIV_PROT_READ | ETNAVIV_PROT_WRITE);
>  
>  	if (ret < 0) {

Re: [PATCH 2/3] drm/etnaviv: Map and unmap the GPU VA range with respect to its user size

[Sui Jingfeng]

Hi,


On 2024/10/7 18:17, Lucas Stach wrote:
> Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
>> Since the GPU VA space is compact in terms of 4KiB unit, map and/or unmap
>> the area that doesn't belong to a context breaks the philosophy of PPAS.
>> That results in severe errors: GPU hang and MMU fault (page not present)
>> and such.
>>
>> Shrink the usuable size of etnaviv GEM buffer object to its user size,
>> instead of the original physical size of its backing memory.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 28 +++++++++------------------
>>   1 file changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> index 6fbc62772d85..a52ec5eb0e3d 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>> @@ -70,8 +70,10 @@ static int etnaviv_context_map(struct etnaviv_iommu_context *context,
>>   }
>>   
>>   static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>> +			     unsigned int user_len,
>>   			     struct sg_table *sgt, int prot)
>> -{	struct scatterlist *sg;
>> +{
>> +	struct scatterlist *sg;
>>   	unsigned int da = iova;
>>   	unsigned int i;
>>   	int ret;
>> @@ -81,7 +83,8 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>>   
>>   	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;
>> +		unsigned int phys_len = sg_dma_len(sg) + sg->offset;
>> +		size_t bytes = MIN(phys_len, user_len);
>>   
>>   		VERB("map[%d]: %08x %pap(%zx)", i, iova, &pa, bytes);
>>   
>> @@ -89,6 +92,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>>   		if (ret)
>>   			goto fail;
>>   
>> +		user_len -= bytes;
>>   		da += bytes;
>>   	}
> Since the MIN(phys_len, user_len) may limit the mapped amount in the
> wrong direction,

I was thinking that if this will could really happen.

'if (phys_len <= user_len)' is true, the 'bytes' is a number of multiple
of PAGE_SIZE. Since our sg table is created by drm_prime_pages_to_sg(),
so the program still works exactly some as before.

It only differs from previous when 'if (phys_len > user_len)' is true,
but then, it is the tail SG entry that the size of it is not a multiple
of PAGE_SIZE. The 'bytes' will be *exactly* the size of remain GPUVA
range we should map.

> I would think it would be good to add a
> WARN_ON(user_len != 0) after the dma SG iteration.

So the program here seems nearly always correct, no?

Or are you means that when the CPU PAGE size < 4KiB cases ?

I never ever have a CPU has < 4 KiB page configuration.

Regards,
Sui


>>   
>> @@ -104,21 +108,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>>   static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
>>   				struct sg_table *sgt, unsigned len)
>>   {
>> -	struct scatterlist *sg;
>> -	unsigned int da = iova;
>> -	int i;
>> -
>> -	for_each_sgtable_dma_sg(sgt, sg, i) {
>> -		size_t bytes = sg_dma_len(sg) + sg->offset;
>> -
>> -		etnaviv_context_unmap(context, da, bytes);
>> -
>> -		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
>> -
>> -		BUG_ON(!PAGE_ALIGNED(bytes));
>> -
>> -		da += bytes;
>> -	}
>> +	etnaviv_context_unmap(context, iova, len);
> This drops some sanity checks, but I have only ever seen them fire when
> we had other kernel memory corruption issues, so I'm fine with the
> simplification you did here.
>
> Regards,
> Lucas
>
>>   
>>   	context->flush_seq++;
>>   }
>> @@ -131,7 +121,7 @@ static void etnaviv_iommu_remove_mapping(struct etnaviv_iommu_context *context,
>>   	lockdep_assert_held(&context->lock);
>>   
>>   	etnaviv_iommu_unmap(context, mapping->vram_node.start,
>> -			    etnaviv_obj->sgt, etnaviv_obj->base.size);
>> +			    etnaviv_obj->sgt, etnaviv_obj->user_size);
>>   	drm_mm_remove_node(&mapping->vram_node);
>>   }
>>   
>> @@ -314,7 +304,7 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>>   		goto unlock;
>>   
>>   	mapping->iova = node->start;
>> -	ret = etnaviv_iommu_map(context, node->start, sgt,
>> +	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,
>>   				ETNAVIV_PROT_READ | ETNAVIV_PROT_WRITE);
>>   
>>   	if (ret < 0) {

-- 
Best regards,
Sui

Re: [PATCH 2/3] drm/etnaviv: Map and unmap the GPU VA range with respect to its user size

[Sui Jingfeng]

Hi,

On 2024/10/7 18:17, Lucas Stach wrote:
>>   
>> @@ -104,21 +108,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova,
>>   static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova,
>>   				struct sg_table *sgt, unsigned len)
>>   {
>> -	struct scatterlist *sg;
>> -	unsigned int da = iova;
>> -	int i;
>> -
>> -	for_each_sgtable_dma_sg(sgt, sg, i) {
>> -		size_t bytes = sg_dma_len(sg) + sg->offset;


Why the length of a single SG segment is `sg_dma_len(sg) + sg->offset` here?


>> -		etnaviv_context_unmap(context, da, bytes);
>> -
>> -		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
>> -
>> -		BUG_ON(!PAGE_ALIGNED(bytes));
>> -
>> -		da += bytes;
>> -	}
>> +	etnaviv_context_unmap(context, iova, len);
> This drops some sanity checks, but I have only ever seen them fire when
> we had other kernel memory corruption issues, so I'm fine with the
> simplification you did here.


Isn't that 'sg_dma_len(sg)' itself is the length of its backing memory ?


> Regards,
> Lucas

-- 
Best regards,
Sui

[PATCH 3/3] drm/etnaviv: Print an error message if inserting IOVA range fails

[Sui Jingfeng]

Print an error message to help debug when such an issue happen, since it's
not so obvious.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
v0 -> v1: Use dev_err_ratelimited() to prevent spamming the logs

v0 is at https://lore.kernel.org/dri-devel/20240930221706.399139-1-sui.jingfeng@linux.dev/
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index a52ec5eb0e3d..37866ed05c13 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -300,8 +300,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
 		ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
 	else
 		ret = etnaviv_iommu_find_iova(context, node, user_size);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err_ratelimited(context->global->dev,
+				    "Insert iova failed: 0x%llx(0x%x)\n",
+				    va, user_size);
 		goto unlock;
+	}
 
 	mapping->iova = node->start;
 	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,
-- 
2.34.1

Re: [PATCH 3/3] drm/etnaviv: Print an error message if inserting IOVA range fails

[Lucas Stach]

Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
> Print an error message to help debug when such an issue happen, since it's
> not so obvious.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> v0 -> v1: Use dev_err_ratelimited() to prevent spamming the logs
> 
> v0 is at https://lore.kernel.org/dri-devel/20240930221706.399139-1-sui.jingfeng@linux.dev/
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> index a52ec5eb0e3d..37866ed05c13 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> @@ -300,8 +300,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>  		ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
>  	else
>  		ret = etnaviv_iommu_find_iova(context, node, user_size);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		dev_err_ratelimited(context->global->dev,
> +				    "Insert iova failed: 0x%llx(0x%x)\n",
> +				    va, user_size);
>  		goto unlock;
> +	}
>  
>  	mapping->iova = node->start;
>  	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,

Re: [PATCH 3/3] drm/etnaviv: Print an error message if inserting IOVA range fails

[Lucas Stach]

Hi Sui,

Am Montag, dem 07.10.2024 um 12:20 +0200 schrieb Lucas Stach:
> Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
> > Print an error message to help debug when such an issue happen, since it's
> > not so obvious.
> > 
> > Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> 
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> 
What happened to this patch? It's not part of the updated series
anymore. Even though the problem at hand is solved right now, I still
think this patch is useful  to have.

Regards,
Lucas

> > ---
> > v0 -> v1: Use dev_err_ratelimited() to prevent spamming the logs
> > 
> > v0 is at https://lore.kernel.org/dri-devel/20240930221706.399139-1-sui.jingfeng@linux.dev/
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> > index a52ec5eb0e3d..37866ed05c13 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> > @@ -300,8 +300,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
> >  		ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
> >  	else
> >  		ret = etnaviv_iommu_find_iova(context, node, user_size);
> > -	if (ret < 0)
> > +	if (ret < 0) {
> > +		dev_err_ratelimited(context->global->dev,
> > +				    "Insert iova failed: 0x%llx(0x%x)\n",
> > +				    va, user_size);
> >  		goto unlock;
> > +	}
> >  
> >  	mapping->iova = node->start;
> >  	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,
> 

Re: [PATCH 3/3] drm/etnaviv: Print an error message if inserting IOVA range fails

[Sui Jingfeng]

Hi,

On 2024/10/29 00:01, Lucas Stach wrote:
> Hi Sui,
>
> Am Montag, dem 07.10.2024 um 12:20 +0200 schrieb Lucas Stach:
>> Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
>>> Print an error message to help debug when such an issue happen, since it's
>>> not so obvious.
>>>
>>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
>>
> What happened to this patch? It's not part of the updated series
> anymore. Even though the problem at hand is solved right now, I still
> think this patch is useful  to have.

I was thinking this patch should be a separate patch while making the v2.

In the future, I guess there may have 64K CPU and/or GPU page size combinations
from other users or contributors.

I think I should resend this.
But don't know take which Linux kernel as code base.
etnaviv-next or etnaviv-fixes?

If you have convenient code base at hand now, you could pick it up, I don't mind.

> Regards,
> Lucas
>
>>> ---
>>> v0 -> v1: Use dev_err_ratelimited() to prevent spamming the logs
>>>
>>> v0 is at https://lore.kernel.org/dri-devel/20240930221706.399139-1-sui.jingfeng@linux.dev/
>>> ---
>>>   drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>>> index a52ec5eb0e3d..37866ed05c13 100644
>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
>>> @@ -300,8 +300,12 @@ int etnaviv_iommu_map_gem(struct etnaviv_iommu_context *context,
>>>   		ret = etnaviv_iommu_insert_exact(context, node, user_size, va);
>>>   	else
>>>   		ret = etnaviv_iommu_find_iova(context, node, user_size);
>>> -	if (ret < 0)
>>> +	if (ret < 0) {
>>> +		dev_err_ratelimited(context->global->dev,
>>> +				    "Insert iova failed: 0x%llx(0x%x)\n",
>>> +				    va, user_size);
>>>   		goto unlock;
>>> +	}
>>>   
>>>   	mapping->iova = node->start;
>>>   	ret = etnaviv_iommu_map(context, node->start, user_size, sgt,

-- 
Best regards,
Sui

Re: [PATCH 0/3] Fix GPU virtual address collosion when CPU page size != GPU page size

[Lucas Stach]

Hi Sui,

Am Samstag, dem 05.10.2024 um 03:42 +0800 schrieb Sui Jingfeng:
> Etnaviv assumes that GPU page size is 4KiB, however, when using
> softpin capable GPUs on a different CPU page size configuration.
> Userspace still doing the allocation with 4KiB page as unit. This
> results in userspace allocated GPU virtual address ranges collision
> and therefore unable to be inserted to the specified hole exactly.
> 
> The root cause is that kernel side BO takes up bigger address space
> than userspace assumes when the size of it is not CPU page size aligned.
> 
> To solve it with no GPU VA range space wasting, we first track the size
> of a buffer that userspace/GPU think of it is, then partially map and/or
> unmap the tail physical page with respect to this 'user size'. Ensure
> that GPU VA is fully mapped and/or unmapped.
> 
Would you be able to get me a updated series with the feedback taken
care of? I would like to add this series to the next upstream pull
request, if possible.

Regards,
Lucas

> Sui Jingfeng (3):
>   drm/etnaviv: Track GPU VA size separately
>   drm/etnaviv: Map and unmap the GPU VA range with respect to its user
>     size
>   drm/etnaviv: Print an error message if inserting IOVA range fails
> 
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c |  8 +++--
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h |  1 +
>  drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 42 ++++++++++++---------------
>  3 files changed, 24 insertions(+), 27 deletions(-)
>