[PATCH 2/3] drm/amdgpu: Fix Vega10 VM initialization

[Christian König]

Am 29.03.2017 um 03:48 schrieb Felix Kuehling:
> On 17-03-28 09:39 PM, Zhang, Jerry (Junwei) wrote:
>> On 03/29/2017 09:00 AM, Felix Kuehling wrote:
>>> adev->family is not initialized yet when amdgpu_get_block_size is
>>> called. Use adev->asic_type instead.
>>>
>>> Minimum VM size is 512GB, not 256GB, for a single page table entry
>>> in the root page table.
>>>
>>> gmc_v9_0_vm_init is called after adev->vm_manager.max_pfn is
>>> initialized. Move the minimum VM-size enforcement ahead of max_pfn
>>> initializtion. Cast to 64-bit before the left-shift.
>>>
>>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>> Reviewed-by: Junwei Zhang <Jerry.Zhang at amd.com>
>>
>> Just note:
>> For now, it's OK to set the minimum vm size 256G,
>> In this case, there is no PD bit anyway.
> With VM size of 256GB, the amdgpu_vm_bo_size(adev, 0) calculates 0, and
> the page directory allocation with amdgpu_bo_create fails in amdgpu_vm_init.
>
> In other words, amdgpu_vm_num_entries(adev, 0) needs to return at least
> 1. That means vm_size needs to be at least 512GB.

Those fixes are correct, but doesn't address the real problem.

The intention of amdgpu_vm_size and amgpu_vm_block_size is to save 
memory by reducing the size of the PD/PTs.

Since we now use 4 level PDs/PTs the size of each is 4KB, which is 
usually the smallest allocation size anyway.

So I suggest to just drop support for amdgpu_vm_size for Vega10 and 
always use 256TB instead.

Jerry do you want to take care of this or should I look into it? Should 
be trivial to do.

Regards,
Christian.

>
> Regards,
>    Felix
>
>> Christian also mentioned that PD should be 4k size, then 256T was
>> required.
>> When reach an agreement, we may update them all.
>>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  6 +++---
>>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 17 +++++++----------
>>>    2 files changed, 10 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 3500da3..57ccac4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -1042,10 +1042,10 @@ static bool amdgpu_check_pot_argument(int arg)
>>>    static void amdgpu_get_block_size(struct amdgpu_device *adev)
>>>    {
>>>        /* from AI, asic starts to support multiple level VMPT */
>>> -    if (adev->family >= AMDGPU_FAMILY_AI) {
>>> +    if (adev->asic_type >= CHIP_VEGA10) {
>>>            if (amdgpu_vm_block_size != 9)
>>> -            dev_warn(adev->dev, "Multi-VMPT limits block size to"
>>> -                 "one page!\n");
>>> +            dev_warn(adev->dev,
>>> +                 "Multi-VMPT limits block size to one page!\n");
>>>            amdgpu_vm_block_size = 9;
>>>            return;
>>>        }
>> Nice catch.
>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> index 1e4734d..df69aae 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> @@ -511,12 +511,6 @@ static int gmc_v9_0_vm_init(struct amdgpu_device
>>> *adev)
>>>         * amdkfd will use VMIDs 8-15
>>>         */
>>>        adev->vm_manager.num_ids = AMDGPU_NUM_OF_VMIDS;
>>> -    /* Because of four level VMPTs, vm size at least is 256GB.
>>> -    256TB is OK as well */
>>> -    if (amdgpu_vm_size < 256) {
>>> -        DRM_WARN("vm size at least is 256GB!\n");
>>> -        amdgpu_vm_size = 256;
>>> -    }
>> David had a patch to fix it yesterday.
>> But your patch involves by vm size checking. :)
>>
>>>        adev->vm_manager.num_level = 3;
>>>        amdgpu_vm_manager_init(adev);
>>>
>>> @@ -563,11 +557,14 @@ static int gmc_v9_0_sw_init(void *handle)
>>>        if (r)
>>>            return r;
>>>
>>> -    /* Adjust VM size here.
>>> -     * Currently default to 64GB ((16 << 20) 4k pages).
>>> -     * Max GPUVM size is 48 bits.
>>> +    /* Because of four level VMPTs, vm size is at least 512GB.
>>> +     * The maximum size is 256TB (48bit).
>>>         */
>>> -    adev->vm_manager.max_pfn = amdgpu_vm_size << 18;
>>> +    if (amdgpu_vm_size < 512) {
>>> +        DRM_WARN("VM size is at least 512GB!\n");
>>> +        amdgpu_vm_size = 512;
>>> +    }
>>> +    adev->vm_manager.max_pfn = (uint64_t)amdgpu_vm_size << 18;
>>>
>>>        /* Set the internal MC address mask
>>>         * This is the max address of the GPU's
>>>