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