[PATCH] drm/amdgpu: Fix the vram base start address

[PATCH] drm/amdgpu: Fix the vram base start address

[Arunpravin Paneer Selvam]

If the size returned by drm buddy allocator is higher than
the required size, we take the higher size to calculate
the buffer start address. This is required if we couldn't
trim the buffer to the requested size. This will fix the
display corruption issue on APU's which has limited VRAM
size.

gitlab issue link: https://gitlab.freedesktop.org/drm/amd/-/issues/2859
JIRA ticket link: https://ontrack-internal.amd.com/browse/SWDEV-425461

Fixes: 0a1844bf0b53 ("drm/buddy: Improve contiguous memory allocation")
Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 18f58efc9dc7..08916538a615 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -77,7 +77,16 @@ static inline bool amdgpu_is_vram_mgr_blocks_contiguous(struct list_head *head)
 	return true;
 }
 
+static inline u64 amdgpu_vram_mgr_blocks_size(struct list_head *head)
+{
+	struct drm_buddy_block *block;
+	u64 size = 0;
 
+	list_for_each_entry(block, head, link)
+		size += amdgpu_vram_mgr_block_size(block);
+
+	return size;
+}
 
 /**
  * DOC: mem_info_vram_total
@@ -516,6 +525,8 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
 	mutex_unlock(&mgr->lock);
 
 	vres->base.start = 0;
+	size = max_t(u64, amdgpu_vram_mgr_blocks_size(&vres->blocks),
+		     vres->base.size);
 	list_for_each_entry(block, &vres->blocks, link) {
 		unsigned long start;
 
@@ -523,8 +534,8 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
 			amdgpu_vram_mgr_block_size(block);
 		start >>= PAGE_SHIFT;
 
-		if (start > PFN_UP(vres->base.size))
-			start -= PFN_UP(vres->base.size);
+		if (start > PFN_UP(size))
+			start -= PFN_UP(size);
 		else
 			start = 0;
 		vres->base.start = max(vres->base.start, start);
-- 
2.25.1

Re: [PATCH] drm/amdgpu: Fix the vram base start address

[Christian König]

Am 30.10.23 um 13:22 schrieb Arunpravin Paneer Selvam:
> If the size returned by drm buddy allocator is higher than
> the required size, we take the higher size to calculate
> the buffer start address. This is required if we couldn't
> trim the buffer to the requested size. This will fix the
> display corruption issue on APU's which has limited VRAM
> size.
>
> gitlab issue link: https://gitlab.freedesktop.org/drm/amd/-/issues/2859
> JIRA ticket link: https://ontrack-internal.amd.com/browse/SWDEV-425461
>
> Fixes: 0a1844bf0b53 ("drm/buddy: Improve contiguous memory allocation")
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>

Acked-by: Christian König <christian.koenig@amd.com>

IIRC that hack with the start address is actually not needed any more, 
but we need to double check this.

Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> index 18f58efc9dc7..08916538a615 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> @@ -77,7 +77,16 @@ static inline bool amdgpu_is_vram_mgr_blocks_contiguous(struct list_head *head)
>   	return true;
>   }
>   
> +static inline u64 amdgpu_vram_mgr_blocks_size(struct list_head *head)
> +{
> +	struct drm_buddy_block *block;
> +	u64 size = 0;
>   
> +	list_for_each_entry(block, head, link)
> +		size += amdgpu_vram_mgr_block_size(block);
> +
> +	return size;
> +}
>   
>   /**
>    * DOC: mem_info_vram_total
> @@ -516,6 +525,8 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
>   	mutex_unlock(&mgr->lock);
>   
>   	vres->base.start = 0;
> +	size = max_t(u64, amdgpu_vram_mgr_blocks_size(&vres->blocks),
> +		     vres->base.size);
>   	list_for_each_entry(block, &vres->blocks, link) {
>   		unsigned long start;
>   
> @@ -523,8 +534,8 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
>   			amdgpu_vram_mgr_block_size(block);
>   		start >>= PAGE_SHIFT;
>   
> -		if (start > PFN_UP(vres->base.size))
> -			start -= PFN_UP(vres->base.size);
> +		if (start > PFN_UP(size))
> +			start -= PFN_UP(size);
>   		else
>   			start = 0;
>   		vres->base.start = max(vres->base.start, start);

Re: [PATCH] drm/amdgpu: Fix the vram base start address

[Arunpravin Paneer Selvam]

Hi Christian,

On 10/30/2023 9:34 PM, Christian König wrote:
>
>
> Am 30.10.23 um 13:22 schrieb Arunpravin Paneer Selvam:
>> If the size returned by drm buddy allocator is higher than
>> the required size, we take the higher size to calculate
>> the buffer start address. This is required if we couldn't
>> trim the buffer to the requested size. This will fix the
>> display corruption issue on APU's which has limited VRAM
>> size.
>>
>> gitlab issue link: https://gitlab.freedesktop.org/drm/amd/-/issues/2859
>> JIRA ticket link: https://ontrack-internal.amd.com/browse/SWDEV-425461
>>
>> Fixes: 0a1844bf0b53 ("drm/buddy: Improve contiguous memory allocation")
>> Signed-off-by: Arunpravin Paneer Selvam 
>> <Arunpravin.PaneerSelvam@amd.com>
>
> Acked-by: Christian König <christian.koenig@amd.com>
>
> IIRC that hack with the start address is actually not needed any more, 
> but we need to double check this.
okay, can we just remove this hack and keep the vres->base.start value 
as the start address of the first block from the
allocated list.

Thanks,
Arun
>
> Christian.
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> index 18f58efc9dc7..08916538a615 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> @@ -77,7 +77,16 @@ static inline bool 
>> amdgpu_is_vram_mgr_blocks_contiguous(struct list_head *head)
>>       return true;
>>   }
>>   +static inline u64 amdgpu_vram_mgr_blocks_size(struct list_head *head)
>> +{
>> +    struct drm_buddy_block *block;
>> +    u64 size = 0;
>>   +    list_for_each_entry(block, head, link)
>> +        size += amdgpu_vram_mgr_block_size(block);
>> +
>> +    return size;
>> +}
>>     /**
>>    * DOC: mem_info_vram_total
>> @@ -516,6 +525,8 @@ static int amdgpu_vram_mgr_new(struct 
>> ttm_resource_manager *man,
>>       mutex_unlock(&mgr->lock);
>>         vres->base.start = 0;
>> +    size = max_t(u64, amdgpu_vram_mgr_blocks_size(&vres->blocks),
>> +             vres->base.size);
>>       list_for_each_entry(block, &vres->blocks, link) {
>>           unsigned long start;
>>   @@ -523,8 +534,8 @@ static int amdgpu_vram_mgr_new(struct 
>> ttm_resource_manager *man,
>>               amdgpu_vram_mgr_block_size(block);
>>           start >>= PAGE_SHIFT;
>>   -        if (start > PFN_UP(vres->base.size))
>> -            start -= PFN_UP(vres->base.size);
>> +        if (start > PFN_UP(size))
>> +            start -= PFN_UP(size);
>>           else
>>               start = 0;
>>           vres->base.start = max(vres->base.start, start);
>

Re: [PATCH] drm/amdgpu: Fix the vram base start address

[Christian König]

Am 01.11.23 um 20:13 schrieb Arunpravin Paneer Selvam:
> Hi Christian,
>
> On 10/30/2023 9:34 PM, Christian König wrote:
>>
>>
>> Am 30.10.23 um 13:22 schrieb Arunpravin Paneer Selvam:
>>> If the size returned by drm buddy allocator is higher than
>>> the required size, we take the higher size to calculate
>>> the buffer start address. This is required if we couldn't
>>> trim the buffer to the requested size. This will fix the
>>> display corruption issue on APU's which has limited VRAM
>>> size.
>>>
>>> gitlab issue link: https://gitlab.freedesktop.org/drm/amd/-/issues/2859
>>> JIRA ticket link: https://ontrack-internal.amd.com/browse/SWDEV-425461
>>>
>>> Fixes: 0a1844bf0b53 ("drm/buddy: Improve contiguous memory allocation")
>>> Signed-off-by: Arunpravin Paneer Selvam 
>>> <Arunpravin.PaneerSelvam@amd.com>
>>
>> Acked-by: Christian König <christian.koenig@amd.com>
>>
>> IIRC that hack with the start address is actually not needed any 
>> more, but we need to double check this.
> okay, can we just remove this hack and keep the vres->base.start value 
> as the start address of the first block from the
> allocated list.

Please double check if we don't have any more cases where we compare the 
start address against the visible VRAM limit.

I think we now fixed all those cases and replaced them with calls to 
check if all segments are visible, but I'm not 100% sure.

Regards,
Christian.

>
> Thanks,
> Arun
>>
>> Christian.
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 15 +++++++++++++--
>>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>> index 18f58efc9dc7..08916538a615 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>> @@ -77,7 +77,16 @@ static inline bool 
>>> amdgpu_is_vram_mgr_blocks_contiguous(struct list_head *head)
>>>       return true;
>>>   }
>>>   +static inline u64 amdgpu_vram_mgr_blocks_size(struct list_head 
>>> *head)
>>> +{
>>> +    struct drm_buddy_block *block;
>>> +    u64 size = 0;
>>>   +    list_for_each_entry(block, head, link)
>>> +        size += amdgpu_vram_mgr_block_size(block);
>>> +
>>> +    return size;
>>> +}
>>>     /**
>>>    * DOC: mem_info_vram_total
>>> @@ -516,6 +525,8 @@ static int amdgpu_vram_mgr_new(struct 
>>> ttm_resource_manager *man,
>>>       mutex_unlock(&mgr->lock);
>>>         vres->base.start = 0;
>>> +    size = max_t(u64, amdgpu_vram_mgr_blocks_size(&vres->blocks),
>>> +             vres->base.size);
>>>       list_for_each_entry(block, &vres->blocks, link) {
>>>           unsigned long start;
>>>   @@ -523,8 +534,8 @@ static int amdgpu_vram_mgr_new(struct 
>>> ttm_resource_manager *man,
>>>               amdgpu_vram_mgr_block_size(block);
>>>           start >>= PAGE_SHIFT;
>>>   -        if (start > PFN_UP(vres->base.size))
>>> -            start -= PFN_UP(vres->base.size);
>>> +        if (start > PFN_UP(size))
>>> +            start -= PFN_UP(size);
>>>           else
>>>               start = 0;
>>>           vres->base.start = max(vres->base.start, start);
>>
>