[PATCH 1/2] drm/amdgpu: fix start calculation in amdgpu_vram_mgr_new

[Christian König]

Am 10.05.22 um 17:20 schrieb Arunpravin Paneer Selvam:
> Hi Christian,
>
> On 5/10/2022 5:06 PM, Christian König wrote:
>> We still need to calculate a virtual start address for the resource to
>> aid checking of it is visible or not.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 22 +++++++++++++-------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> index 49e4092f447f..51d9d3a4456c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> @@ -496,16 +496,22 @@ static int amdgpu_vram_mgr_new(struct 
>> ttm_resource_manager *man,
>>               list_splice_tail(trim_list, &vres->blocks);
>>       }
>>   -    list_for_each_entry(block, &vres->blocks, link)
>> -        vis_usage += amdgpu_vram_mgr_vis_size(adev, block);
>> +    vres->base.start = 0;
>> +    list_for_each_entry(block, &vres->blocks, link) {
>> +        unsigned long start;
>>   -    block = amdgpu_vram_mgr_first_block(&vres->blocks);
>> -    if (!block) {
>> -        r = -EINVAL;
>> -        goto error_fini;
>> -    }
>> +        start = amdgpu_vram_mgr_block_start(block) +
>> +            amdgpu_vram_mgr_block_size(block);
>> +        start >>= PAGE_SHIFT;
>>   -    vres->base.start = amdgpu_vram_mgr_block_start(block) >> 
>> PAGE_SHIFT;
>> +        if (start > vres->base.num_pages)
>> +            start -= vres->base.num_pages;
> I think this works for continuous blocks of addresses, but for a non 
> continuous blocks allocated at different
> locations and linked together using list, this might fetch an invalid 
> address. I will check this patch on my
> setup and inform you.

It indeed does fetch an invalid address and that is perfectly expected 
behavior.

See the resource->start value for non-contiguous allocations is just the 
maximum allocated address.

This makes it possible to easily check if a buffer is inside the CPU 
accessible VRAM window or not.

As I said we should probably get rid of that, but for now it's necessary.

Regards,
Christian.

>
> Regards,
> Arun
>> +        else
>> +            start = 0;
>> +        vres->base.start = max(vres->base.start, start);
>> +
>> +        vis_usage += amdgpu_vram_mgr_vis_size(adev, block);
>> +    }
>>         if (amdgpu_is_vram_mgr_blocks_contiguous(&vres->blocks))
>>           vres->base.placement |= TTM_PL_FLAG_CONTIGUOUS;
>