[PATCH] drm/amdgpu: take back kvmalloc_array for entries alloc because of kzalloc memory limit

[Das, Nirmoy]

On 6/2/2021 11:10 AM, Zhu, Changfeng wrote:
> [AMD Official Use Only]
>
> Hi Chris,
>
> Actually, I think about switching kzalloc to kvmalloc in amdgpu_bo_create.
> However, I observe bp.flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS in amdgpu_vm_pt_create.
>
> Does it matter we switch kzalloc to kvmalloc if there is a physical continuous memory request when creating bo? Such as AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS?


Only driver will access this BO struct, so kvmalloc will be fine.


Regards,

Nirmoy


>
> BR,
> Changfeng.
>
>
>
> -----Original Message---
> From: Koenig, Christian <Christian.Koenig at amd.com>
> Sent: Wednesday, June 2, 2021 4:57 PM
> To: Das, Nirmoy <Nirmoy.Das at amd.com>; Zhu, Changfeng <Changfeng.Zhu at amd.com>; Huang, Ray <Ray.Huang at amd.com>; amd-gfx at freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: take back kvmalloc_array for entries alloc because of kzalloc memory limit
>
>
>
> Am 02.06.21 um 10:54 schrieb Das, Nirmoy:
>> On 6/2/2021 10:30 AM, Changfeng wrote:
>>> From: changzhu <Changfeng.Zhu at amd.com>
>>>
>>> From: Changfeng <Changfeng.Zhu at amd.com>
>>>
>>> It will cause error when alloc memory larger than 128KB in
>>> amdgpu_bo_create->kzalloc.
>>
>> I wonder why I didn't see the error on my machine. Is there any config
>> I might be missing ?
> VM page table layout depends on hardware generation, APU vs dGPU and kernel command line settings.
>
> I think we just need to switch amdgpu_bo_create() from kzalloc to kvmalloc (and kfree to kvfree in amdgpu_bo_destroy of course).
>
> Shouldn't be more than a two line patch.
>
> Regards,
> Christian.
>
>>
>> Thanks,
>>
>> Nirmoy
>>
>>> Call Trace:
>>>      alloc_pages_current+0x6a/0xe0
>>>      kmalloc_order+0x32/0xb0
>>>      kmalloc_order_trace+0x1e/0x80
>>>      __kmalloc+0x249/0x2d0
>>>      amdgpu_bo_create+0x102/0x500 [amdgpu]
>>>      ? xas_create+0x264/0x3e0
>>>      amdgpu_bo_create_vm+0x32/0x60 [amdgpu]
>>>      amdgpu_vm_pt_create+0xf5/0x260 [amdgpu]
>>>      amdgpu_vm_init+0x1fd/0x4d0 [amdgpu]
>>>
>>> Change-Id: I29e479db45ead37c39449e856599fd4f6a0e34ce
>>> Signed-off-by: Changfeng <Changfeng.Zhu at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 27
>>> +++++++++++++++-----------
>>>    1 file changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 1923f035713a..714d613d020b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -894,6 +894,10 @@ static int amdgpu_vm_pt_create(struct
>>> amdgpu_device *adev,
>>>            num_entries = 0;
>>>          bp.bo_ptr_size = struct_size((*vmbo), entries, num_entries);
>>> +    if (bp.bo_ptr_size > 32*AMDGPU_GPU_PAGE_SIZE) {
>>> +        DRM_INFO("Can't alloc memory larger than 128KB by using
>>> kzalloc in amdgpu_bo_create\n");
>>> +        bp.bo_ptr_size = sizeof(struct amdgpu_bo_vm);
>>> +    }
>>>          if (vm->use_cpu_for_update)
>>>            bp.flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>>> @@ -965,15 +969,19 @@ static int amdgpu_vm_alloc_pts(struct
>>> amdgpu_device *adev,
>>>        struct amdgpu_bo_vm *pt;
>>>        int r;
>>>    -    if (entry->base.bo) {
>>> -        if (cursor->level < AMDGPU_VM_PTB)
>>> -            entry->entries =
>>> -                to_amdgpu_bo_vm(entry->base.bo)->entries;
>>> -        else
>>> -            entry->entries = NULL;
>>> -        return 0;
>>> +    if (cursor->level < AMDGPU_VM_PTB && !entry->entries) {
>>> +        unsigned num_entries;
>>> +        num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>>> +        entry->entries = kvmalloc_array(num_entries,
>>> +                        sizeof(*entry->entries),
>>> +                        GFP_KERNEL | __GFP_ZERO);
>>> +        if (!entry->entries)
>>> +            return -ENOMEM;
>>>        }
>>>    +    if (entry->base.bo)
>>> +        return 0;
>>> +
>>>        r = amdgpu_vm_pt_create(adev, vm, cursor->level, immediate,
>>> &pt);
>>>        if (r)
>>>            return r;
>>> @@ -984,10 +992,6 @@ static int amdgpu_vm_alloc_pts(struct
>>> amdgpu_device *adev,
>>>        pt_bo = &pt->bo;
>>>        pt_bo->parent = amdgpu_bo_ref(cursor->parent->base.bo);
>>>        amdgpu_vm_bo_base_init(&entry->base, vm, pt_bo);
>>> -    if (cursor->level < AMDGPU_VM_PTB)
>>> -        entry->entries = pt->entries;
>>> -    else
>>> -        entry->entries = NULL;
>>>          r = amdgpu_vm_clear_bo(adev, vm, pt, immediate);
>>>        if (r)
>>> @@ -1017,6 +1021,7 @@ static void amdgpu_vm_free_table(struct
>>> amdgpu_vm_pt *entry)
>>>            amdgpu_bo_unref(&shadow);
>>>            amdgpu_bo_unref(&entry->base.bo);
>>>        }
>>> +    kvfree(entry->entries);
>>>        entry->entries = NULL;
>>>    }