Re: [PATCH 2/3] drm/amdgpu: Fix Vega10 VM initialization
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
Reviewed-by: Junwei Zhang
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
Re: [PATCH 2/3] drm/amdgpu: Fix Vega10 VM initialization
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
Reviewed-by: Junwei Zhang
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/3] drm/amdgpu: Fix Vega10 VM initialization
On 03/29/2017 09:48 AM, Felix Kuehling wrote:
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
Reviewed-by: Junwei Zhang
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.
Sorry for the typo, I'd like to say 512GB.
Maybe it will become 256TB, depending on the discussion in the future.
Jerry
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
Re: [PATCH 2/3] drm/amdgpu: Fix Vega10 VM initialization
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
> Reviewed-by: Junwei Zhang
>
> 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.
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
Re: [PATCH 2/3] drm/amdgpu: Fix Vega10 VM initialization
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
Reviewed-by: Junwei Zhang
Just note:
For now, it's OK to set the minimum vm size 256G,
In this case, there is no PD bit anyway.
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
