[PATCH 1/2 v2] drm/amdgpu: fix vm size and block size for Vega10 VMPT

[PATCH 1/2 v2] drm/amdgpu: fix vm size and block size for Vega10 VMPT

[Junwei Zhang]

From: "Zhang, Jerry" <Jerry.Zhang@amd.com>

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  8 --------
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 15 ++++++++-------
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 363d73c..98555fd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1041,14 +1041,6 @@ 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->asic_type >= CHIP_VEGA10) {
-		if (amdgpu_vm_block_size != 9)
-			dev_warn(adev->dev,
-				 "Multi-VMPT limits block size to one page!\n");
-		amdgpu_vm_block_size = 9;
-		return;
-	}
 	/* defines number of bits in page table versus page directory,
 	 * a page is 4KB so we have 12 bits offset, minimum 9 bits in the
 	 * page table and the remaining bits are in the page directory */
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index df69aae..081a676 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -546,6 +546,14 @@ static int gmc_v9_0_sw_init(void *handle)
 	} else {
 		/* XXX Don't know how to get VRAM type yet. */
 		adev->mc.vram_type = AMDGPU_VRAM_TYPE_HBM;
+
+		/*
+		 * To fulfill 4-level page support,
+		 * vm size is 256TB (48bit), maximum size of Vega10,
+		 * block size 512 (9bit)
+		 */
+		amdgpu_vm_size = 1U << 18;
+		amdgpu_vm_block_size = 9;
 	}
 
 	/* This interrupt is VMC page fault.*/
@@ -557,13 +565,6 @@ static int gmc_v9_0_sw_init(void *handle)
 	if (r)
 		return r;
 
-	/* Because of four level VMPTs, vm size is at least 512GB.
-	 * The maximum size is 256TB (48bit).
-	 */
-	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
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: rename amdgpu_get_block_size to amdgpu_check_block_size

[Junwei Zhang]

Here just check if the block size is reasonable,
otherwise set it as default vaule.
The entity actual vaule should be set globally or in gmc.

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 98555fd..714be3c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1039,7 +1039,7 @@ static bool amdgpu_check_pot_argument(int arg)
 	return (arg & (arg - 1)) == 0;
 }
 
-static void amdgpu_get_block_size(struct amdgpu_device *adev)
+static void amdgpu_check_block_size(struct amdgpu_device *adev)
 {
 	/* defines number of bits in page table versus page directory,
 	 * a page is 4KB so we have 12 bits offset, minimum 9 bits in the
@@ -1131,7 +1131,7 @@ static void amdgpu_check_arguments(struct amdgpu_device *adev)
 
 	amdgpu_check_vm_size(adev);
 
-	amdgpu_get_block_size(adev);
+	amdgpu_check_block_size(adev);
 
 	if (amdgpu_vram_page_split != -1 && (amdgpu_vram_page_split < 16 ||
 	    !amdgpu_check_pot_argument(amdgpu_vram_page_split))) {
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2 v2] drm/amdgpu: fix vm size and block size for Vega10 VMPT

[Deucher, Alexander]

> -----Original Message-----
> From: Junwei Zhang [mailto:Jerry.Zhang@amd.com]
> Sent: Thursday, March 30, 2017 10:52 PM
> To: Deucher, Alexander; Koenig, Christian
> Cc: amd-gfx@lists.freedesktop.org; Zhang, Jerry
> Subject: [PATCH 1/2 v2] drm/amdgpu: fix vm size and block size for Vega10
> VMPT
> 
> From: "Zhang, Jerry" <Jerry.Zhang@amd.com>
> 
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  8 --------
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 15 ++++++++-------
>  2 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 363d73c..98555fd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1041,14 +1041,6 @@ 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->asic_type >= CHIP_VEGA10) {
> -		if (amdgpu_vm_block_size != 9)
> -			dev_warn(adev->dev,
> -				 "Multi-VMPT limits block size to one
> page!\n");
> -		amdgpu_vm_block_size = 9;
> -		return;
> -	}
>  	/* defines number of bits in page table versus page directory,
>  	 * a page is 4KB so we have 12 bits offset, minimum 9 bits in the
>  	 * page table and the remaining bits are in the page directory */
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index df69aae..081a676 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -546,6 +546,14 @@ static int gmc_v9_0_sw_init(void *handle)
>  	} else {
>  		/* XXX Don't know how to get VRAM type yet. */
>  		adev->mc.vram_type = AMDGPU_VRAM_TYPE_HBM;
> +
> +		/*
> +		 * To fulfill 4-level page support,
> +		 * vm size is 256TB (48bit), maximum size of Vega10,
> +		 * block size 512 (9bit)
> +		 */
> +		amdgpu_vm_size = 1U << 18;
> +		amdgpu_vm_block_size = 9;

This won't work.  You are still changing the amdgpu_vm_* global variables so if you have 2 GPUs in the systems, you'll change them both.

>  	}
> 
>  	/* This interrupt is VMC page fault.*/
> @@ -557,13 +565,6 @@ static int gmc_v9_0_sw_init(void *handle)
>  	if (r)
>  		return r;
> 
> -	/* Because of four level VMPTs, vm size is at least 512GB.
> -	 * The maximum size is 256TB (48bit).
> -	 */
> -	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;

Instead of changing amdgpu_vm_size, change adev->vm_manager.max_pfn directly, that way you won't be changing the global variable and possibly messing up other driver instances.  The alternative, and cleaner in my opinion, would be to add new variables to the vm_manager, e.g., adev->vm_manager.size and adev->vm_manager.block_size, and set them to something reasonable based on the module parameters and them use them in the code rather than the global variables.

Alex

> 
>  	/* Set the internal MC address mask
> --
> 1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2 v2] drm/amdgpu: fix vm size and block size for Vega10 VMPT

[Zhang, Jerry (Junwei)]

On 03/31/2017 12:05 PM, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: Junwei Zhang [mailto:Jerry.Zhang@amd.com]
>> Sent: Thursday, March 30, 2017 10:52 PM
>> To: Deucher, Alexander; Koenig, Christian
>> Cc: amd-gfx@lists.freedesktop.org; Zhang, Jerry
>> Subject: [PATCH 1/2 v2] drm/amdgpu: fix vm size and block size for Vega10
>> VMPT
>>
>> From: "Zhang, Jerry" <Jerry.Zhang@amd.com>
>>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  8 --------
>>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 15 ++++++++-------
>>  2 files changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 363d73c..98555fd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -1041,14 +1041,6 @@ 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->asic_type >= CHIP_VEGA10) {
>> -             if (amdgpu_vm_block_size != 9)
>> -                     dev_warn(adev->dev,
>> -                              "Multi-VMPT limits block size to one
>> page!\n");
>> -             amdgpu_vm_block_size = 9;
>> -             return;
>> -     }
>>        /* defines number of bits in page table versus page directory,
>>         * a page is 4KB so we have 12 bits offset, minimum 9 bits in the
>>         * page table and the remaining bits are in the page directory */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index df69aae..081a676 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -546,6 +546,14 @@ static int gmc_v9_0_sw_init(void *handle)
>>        } else {
>>                /* XXX Don't know how to get VRAM type yet. */
>>                adev->mc.vram_type = AMDGPU_VRAM_TYPE_HBM;
>> +
>> +             /*
>> +              * To fulfill 4-level page support,
>> +              * vm size is 256TB (48bit), maximum size of Vega10,
>> +              * block size 512 (9bit)
>> +              */
>> +             amdgpu_vm_size = 1U << 18;
>> +             amdgpu_vm_block_size = 9;
>
> This won't work.  You are still changing the amdgpu_vm_* global variables so if
> you have 2 GPUs in the systems, you'll change them both.
>
>>        }
>>
>>        /* This interrupt is VMC page fault.*/
>> @@ -557,13 +565,6 @@ static int gmc_v9_0_sw_init(void *handle)
>>        if (r)
>>                return r;
>>
>> -     /* Because of four level VMPTs, vm size is at least 512GB.
>> -      * The maximum size is 256TB (48bit).
>> -      */
>> -     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;
>
> Instead of changing amdgpu_vm_size, change adev->vm_manager.max_pfn directly,
> that way you won't be changing the global variable and possibly messing up
> other driver instances.  The alternative, and cleaner in my opinion, would be
> to add new variables to the vm_manager, e.g., adev->vm_manager.size and
> adev->vm_manager.block_size, and set them to something reasonable based on the
> module parameters and them use them in the code rather than the global variables.

Yeah, thanks to point it out.
I prepared another patch, please check it too.

Jerry

>
> Alex
>
>>
>>        /* Set the internal MC address mask
>> --
>> 1.9.1
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx