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

[Arunpravin Paneer Selvam]

On 5/10/2022 8:56 PM, Christian König wrote:
> 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.

ah. good point.
Reviewed-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam at amd.com>
>
> 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;
>>
>