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

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

On 03/29/2017 02:47 PM, Christian König wrote:
> 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@amd.com>
>>> Reviewed-by: Junwei Zhang <Jerry.Zhang@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.

I remember we use 512B (9-bit) for each PD/PT for Vega10.

>
> 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.

Do you mean to fix the vm_size for Vega10?
Yes, I will take over it.

Jerry

>
> 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
>>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx