[PATCH 1/2] drm/amdgpu: Use fw vram offset when allocating stolen vga memory.

[PATCH 1/2] drm/amdgpu: Use fw vram offset when allocating stolen vga memory.

[Yongqiang Sun]

[Why]
Memory access violation will happen in case of allocate stolen vga
memory with size isn't 0.

[How]
when allocating stolen vga memory, use fw vram offset as the start point
instead of hard code value 0.

Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
Change-Id: I7c555a6c1fb4b3fa8685753b4bdcbe215f89ea1e
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 31 +++++++++++++++++++------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 41d6f604813d..1f635fdb0395 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1708,6 +1708,7 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
 	uint64_t gtt_size;
 	int r;
 	u64 vis_vram_limit;
+	u64 memory_offset = adev->mman.fw_vram_usage_start_offset + adev->mman.fw_vram_usage_size;
 
 	mutex_init(&adev->mman.gtt_window_lock);
 
@@ -1774,24 +1775,40 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
 	 * This is used for VGA emulation and pre-OS scanout buffers to
 	 * avoid display artifacts while transitioning between pre-OS
 	 * and driver.  */
-	r = amdgpu_bo_create_kernel_at(adev, 0, adev->mman.stolen_vga_size,
+	r = amdgpu_bo_create_kernel_at(adev,
+					   memory_offset,
+			           adev->mman.stolen_vga_size,
 				       AMDGPU_GEM_DOMAIN_VRAM,
 				       &adev->mman.stolen_vga_memory,
 				       NULL);
 	if (r)
 		return r;
-	r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_vga_size,
+
+	memory_offset += adev->mman.stolen_vga_size;
+
+	r = amdgpu_bo_create_kernel_at(adev,
+					   memory_offset,
 				       adev->mman.stolen_extended_size,
 				       AMDGPU_GEM_DOMAIN_VRAM,
 				       &adev->mman.stolen_extended_memory,
 				       NULL);
 	if (r)
 		return r;
-	r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_reserved_offset,
-				       adev->mman.stolen_reserved_size,
-				       AMDGPU_GEM_DOMAIN_VRAM,
-				       &adev->mman.stolen_reserved_memory,
-				       NULL);
+
+	memory_offset += adev->mman.stolen_extended_size;
+
+	if (adev->mman.stolen_reserved_offset > memory_offset)
+		r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_reserved_offset,
+						   adev->mman.stolen_reserved_size,
+						   AMDGPU_GEM_DOMAIN_VRAM,
+						   &adev->mman.stolen_reserved_memory,
+						   NULL);
+	else if (adev->mman.stolen_reserved_offset + adev->mman.stolen_reserved_size > memory_offset)
+		r = amdgpu_bo_create_kernel_at(adev, memory_offset,
+						adev->mman.stolen_reserved_offset + adev->mman.stolen_reserved_size - memory_offset,
+						   AMDGPU_GEM_DOMAIN_VRAM,
+						   &adev->mman.stolen_reserved_memory,
+						   NULL);
 	if (r)
 		return r;
 
-- 
2.25.1

[PATCH 2/2] drm/amdgpu: Add stolen reserved memory for MI25 SRIOV.

[Yongqiang Sun]

[Why]
MI25 SRIOV guest driver loading failed due to allocate
memory overlaps with private memory area.

[How]
1. Allocate stolen reserved memory for MI25 SRIOV specifically to avoid
the memory overlap.
2. Move allocate reserve allocation to vbios allocation since both the
two functions are doing similar asic type check and no need to have
seperate functions.

Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
Change-Id: I142127513047a3e81573eb983c510d763b548a24
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 37 ++++++++++++-------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h |  1 -
 drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  |  1 -
 3 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 7c2a9555b7cc..f7f4f00dd2b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -626,6 +626,11 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
 {
 	unsigned size;
 
+	/* Some ASICs need to reserve a region of video memory to avoid access
+	 * from driver */
+	adev->mman.stolen_reserved_offset = 0;
+	adev->mman.stolen_reserved_size = 0;
+
 	/*
 	 * TODO:
 	 * Currently there is a bug where some memory client outside
@@ -635,11 +640,24 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
 	 * Keep the stolen memory reservation until the while this is not solved.
 	 */
 	switch (adev->asic_type) {
+
 	case CHIP_VEGA10:
+		adev->mman.keep_stolen_vga_memory = true;
+		if (amdgpu_sriov_vf(adev)) {
+			adev->mman.stolen_reserved_offset = 0x100000;
+			adev->mman.stolen_reserved_size = 0x600000;
+		}
+		break;
 	case CHIP_RAVEN:
 	case CHIP_RENOIR:
 		adev->mman.keep_stolen_vga_memory = true;
 		break;
+	case CHIP_YELLOW_CARP:
+		if (amdgpu_discovery == 0) {
+			adev->mman.stolen_reserved_offset = 0x1ffb0000;
+			adev->mman.stolen_reserved_size = 64 * PAGE_SIZE;
+		}
+		break;
 	default:
 		adev->mman.keep_stolen_vga_memory = false;
 		break;
@@ -760,25 +778,6 @@ uint64_t amdgpu_gmc_vram_cpu_pa(struct amdgpu_device *adev, struct amdgpu_bo *bo
 	return amdgpu_bo_gpu_offset(bo) - adev->gmc.vram_start + adev->gmc.aper_base;
 }
 
-void amdgpu_gmc_get_reserved_allocation(struct amdgpu_device *adev)
-{
-	/* Some ASICs need to reserve a region of video memory to avoid access
-	 * from driver */
-	adev->mman.stolen_reserved_offset = 0;
-	adev->mman.stolen_reserved_size = 0;
-
-	switch (adev->asic_type) {
-	case CHIP_YELLOW_CARP:
-		if (amdgpu_discovery == 0) {
-			adev->mman.stolen_reserved_offset = 0x1ffb0000;
-			adev->mman.stolen_reserved_size = 64 * PAGE_SIZE;
-		}
-		break;
-	default:
-		break;
-	}
-}
-
 int amdgpu_gmc_vram_checking(struct amdgpu_device *adev)
 {
 	struct amdgpu_bo *vram_bo = NULL;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
index 93505bb0a36c..032b0313f277 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
@@ -331,7 +331,6 @@ amdgpu_gmc_set_vm_fault_masks(struct amdgpu_device *adev, int hub_type,
 			      bool enable);
 
 void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev);
-void amdgpu_gmc_get_reserved_allocation(struct amdgpu_device *adev);
 
 void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev);
 uint64_t amdgpu_gmc_vram_mc2pa(struct amdgpu_device *adev, uint64_t mc_addr);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
index f60b7bd4dbf5..3c1d440824a7 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
@@ -948,7 +948,6 @@ static int gmc_v10_0_sw_init(void *handle)
 		return r;
 
 	amdgpu_gmc_get_vbios_allocations(adev);
-	amdgpu_gmc_get_reserved_allocation(adev);
 
 	/* Memory manager */
 	r = amdgpu_bo_init(adev);
-- 
2.25.1

Re: [PATCH 1/2] drm/amdgpu: Use fw vram offset when allocating stolen vga memory.

[Christian König]

Am 14.03.22 um 19:54 schrieb Yongqiang Sun:
> [Why]
> Memory access violation will happen in case of allocate stolen vga
> memory with size isn't 0.
>
> [How]
> when allocating stolen vga memory, use fw vram offset as the start point
> instead of hard code value 0.

Please stop separating commit message into [Why] and [How], that is not 
well received everywhere.

Apart from that the patch is a certain NAK, you are messing things quite 
up here.

>
> Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
> Change-Id: I7c555a6c1fb4b3fa8685753b4bdcbe215f89ea1e
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 31 +++++++++++++++++++------
>   1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 41d6f604813d..1f635fdb0395 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1708,6 +1708,7 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>   	uint64_t gtt_size;
>   	int r;
>   	u64 vis_vram_limit;
> +	u64 memory_offset = adev->mman.fw_vram_usage_start_offset + adev->mman.fw_vram_usage_size;
>   
>   	mutex_init(&adev->mman.gtt_window_lock);
>   
> @@ -1774,24 +1775,40 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>   	 * This is used for VGA emulation and pre-OS scanout buffers to
>   	 * avoid display artifacts while transitioning between pre-OS
>   	 * and driver.  */
> -	r = amdgpu_bo_create_kernel_at(adev, 0, adev->mman.stolen_vga_size,
> +	r = amdgpu_bo_create_kernel_at(adev,
> +					   memory_offset,
> +			           adev->mman.stolen_vga_size,

That is certainly incorrect. See function amdgpu_ttm_fw_reserve_vram_init().

The stolen VGA buffer always started at offset 0 and is independent of 
the range defined by fw_vram_usage_start_offset and fw_vram_usage_size.

Regards,
Christian.

>   				       AMDGPU_GEM_DOMAIN_VRAM,
>   				       &adev->mman.stolen_vga_memory,
>   				       NULL);
>   	if (r)
>   		return r;
> -	r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_vga_size,
> +
> +	memory_offset += adev->mman.stolen_vga_size;
> +
> +	r = amdgpu_bo_create_kernel_at(adev,
> +					   memory_offset,
>   				       adev->mman.stolen_extended_size,
>   				       AMDGPU_GEM_DOMAIN_VRAM,
>   				       &adev->mman.stolen_extended_memory,
>   				       NULL);
>   	if (r)
>   		return r;
> -	r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_reserved_offset,
> -				       adev->mman.stolen_reserved_size,
> -				       AMDGPU_GEM_DOMAIN_VRAM,
> -				       &adev->mman.stolen_reserved_memory,
> -				       NULL);
> +
> +	memory_offset += adev->mman.stolen_extended_size;
> +
> +	if (adev->mman.stolen_reserved_offset > memory_offset)
> +		r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_reserved_offset,
> +						   adev->mman.stolen_reserved_size,
> +						   AMDGPU_GEM_DOMAIN_VRAM,
> +						   &adev->mman.stolen_reserved_memory,
> +						   NULL);
> +	else if (adev->mman.stolen_reserved_offset + adev->mman.stolen_reserved_size > memory_offset)
> +		r = amdgpu_bo_create_kernel_at(adev, memory_offset,
> +						adev->mman.stolen_reserved_offset + adev->mman.stolen_reserved_size - memory_offset,
> +						   AMDGPU_GEM_DOMAIN_VRAM,
> +						   &adev->mman.stolen_reserved_memory,
> +						   NULL);
>   	if (r)
>   		return r;
>   

Re: [PATCH 2/2] drm/amdgpu: Add stolen reserved memory for MI25 SRIOV.

[Christian König]

Am 14.03.22 um 19:54 schrieb Yongqiang Sun:
> [Why]
> MI25 SRIOV guest driver loading failed due to allocate
> memory overlaps with private memory area.
>
> [How]
> 1. Allocate stolen reserved memory for MI25 SRIOV specifically to avoid
> the memory overlap.
> 2. Move allocate reserve allocation to vbios allocation since both the
> two functions are doing similar asic type check and no need to have
> seperate functions.
>
> Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
> Change-Id: I142127513047a3e81573eb983c510d763b548a24
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 37 ++++++++++++-------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h |  1 -
>   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  |  1 -
>   3 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 7c2a9555b7cc..f7f4f00dd2b2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -626,6 +626,11 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
>   {
>   	unsigned size;
>   
> +	/* Some ASICs need to reserve a region of video memory to avoid access
> +	 * from driver */
> +	adev->mman.stolen_reserved_offset = 0;
> +	adev->mman.stolen_reserved_size = 0;
> +
>   	/*
>   	 * TODO:
>   	 * Currently there is a bug where some memory client outside
> @@ -635,11 +640,24 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
>   	 * Keep the stolen memory reservation until the while this is not solved.
>   	 */
>   	switch (adev->asic_type) {
> +
>   	case CHIP_VEGA10:

Please don't add empty lines between switch and case. Good practice is 
to check your patches with checkpatch.pl before sending it out.

> +		adev->mman.keep_stolen_vga_memory = true;
> +		if (amdgpu_sriov_vf(adev)) {
> +			adev->mman.stolen_reserved_offset = 0x100000;
> +			adev->mman.stolen_reserved_size = 0x600000;
> +		}
> +		break;
>   	case CHIP_RAVEN:
>   	case CHIP_RENOIR:
>   		adev->mman.keep_stolen_vga_memory = true;
>   		break;
> +	case CHIP_YELLOW_CARP:
> +		if (amdgpu_discovery == 0) {
> +			adev->mman.stolen_reserved_offset = 0x1ffb0000;
> +			adev->mman.stolen_reserved_size = 64 * PAGE_SIZE;
> +		}
> +		break;

That looks like this is somehow mixed up. The stolen memory is for VGA 
emulation, but under SRIOV we should not have VGA emulation as far as I 
know.

Alex, what's going on here?

Regards,
Christian.

>   	default:
>   		adev->mman.keep_stolen_vga_memory = false;
>   		break;
> @@ -760,25 +778,6 @@ uint64_t amdgpu_gmc_vram_cpu_pa(struct amdgpu_device *adev, struct amdgpu_bo *bo
>   	return amdgpu_bo_gpu_offset(bo) - adev->gmc.vram_start + adev->gmc.aper_base;
>   }
>   
> -void amdgpu_gmc_get_reserved_allocation(struct amdgpu_device *adev)
> -{
> -	/* Some ASICs need to reserve a region of video memory to avoid access
> -	 * from driver */
> -	adev->mman.stolen_reserved_offset = 0;
> -	adev->mman.stolen_reserved_size = 0;
> -
> -	switch (adev->asic_type) {
> -	case CHIP_YELLOW_CARP:
> -		if (amdgpu_discovery == 0) {
> -			adev->mman.stolen_reserved_offset = 0x1ffb0000;
> -			adev->mman.stolen_reserved_size = 64 * PAGE_SIZE;
> -		}
> -		break;
> -	default:
> -		break;
> -	}
> -}
> -
>   int amdgpu_gmc_vram_checking(struct amdgpu_device *adev)
>   {
>   	struct amdgpu_bo *vram_bo = NULL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> index 93505bb0a36c..032b0313f277 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> @@ -331,7 +331,6 @@ amdgpu_gmc_set_vm_fault_masks(struct amdgpu_device *adev, int hub_type,
>   			      bool enable);
>   
>   void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev);
> -void amdgpu_gmc_get_reserved_allocation(struct amdgpu_device *adev);
>   
>   void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev);
>   uint64_t amdgpu_gmc_vram_mc2pa(struct amdgpu_device *adev, uint64_t mc_addr);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> index f60b7bd4dbf5..3c1d440824a7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> @@ -948,7 +948,6 @@ static int gmc_v10_0_sw_init(void *handle)
>   		return r;
>   
>   	amdgpu_gmc_get_vbios_allocations(adev);
> -	amdgpu_gmc_get_reserved_allocation(adev);
>   
>   	/* Memory manager */
>   	r = amdgpu_bo_init(adev);

Re: [PATCH 2/2] drm/amdgpu: Add stolen reserved memory for MI25 SRIOV.

[Alex Deucher]

On Mon, Mar 14, 2022 at 3:41 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 14.03.22 um 19:54 schrieb Yongqiang Sun:
> > [Why]
> > MI25 SRIOV guest driver loading failed due to allocate
> > memory overlaps with private memory area.
> >
> > [How]
> > 1. Allocate stolen reserved memory for MI25 SRIOV specifically to avoid
> > the memory overlap.
> > 2. Move allocate reserve allocation to vbios allocation since both the
> > two functions are doing similar asic type check and no need to have
> > seperate functions.
> >
> > Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
> > Change-Id: I142127513047a3e81573eb983c510d763b548a24
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 37 ++++++++++++-------------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h |  1 -
> >   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c  |  1 -
> >   3 files changed, 18 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > index 7c2a9555b7cc..f7f4f00dd2b2 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > @@ -626,6 +626,11 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
> >   {
> >       unsigned size;
> >
> > +     /* Some ASICs need to reserve a region of video memory to avoid access
> > +      * from driver */
> > +     adev->mman.stolen_reserved_offset = 0;
> > +     adev->mman.stolen_reserved_size = 0;
> > +
> >       /*
> >        * TODO:
> >        * Currently there is a bug where some memory client outside
> > @@ -635,11 +640,24 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
> >        * Keep the stolen memory reservation until the while this is not solved.
> >        */
> >       switch (adev->asic_type) {
> > +
> >       case CHIP_VEGA10:
>
> Please don't add empty lines between switch and case. Good practice is
> to check your patches with checkpatch.pl before sending it out.
>
> > +             adev->mman.keep_stolen_vga_memory = true;
> > +             if (amdgpu_sriov_vf(adev)) {
> > +                     adev->mman.stolen_reserved_offset = 0x100000;
> > +                     adev->mman.stolen_reserved_size = 0x600000;
> > +             }
> > +             break;
> >       case CHIP_RAVEN:
> >       case CHIP_RENOIR:
> >               adev->mman.keep_stolen_vga_memory = true;
> >               break;
> > +     case CHIP_YELLOW_CARP:
> > +             if (amdgpu_discovery == 0) {
> > +                     adev->mman.stolen_reserved_offset = 0x1ffb0000;
> > +                     adev->mman.stolen_reserved_size = 64 * PAGE_SIZE;
> > +             }
> > +             break;
>
> That looks like this is somehow mixed up. The stolen memory is for VGA
> emulation, but under SRIOV we should not have VGA emulation as far as I
> know.
>
> Alex, what's going on here?

I suggested calling amdgpu_gmc_get_reserved_allocation() from
amdgpu_gmc_get_vbios_allocations() rather calling
amdgpu_gmc_get_reserved_allocation() in every gmc file since it
handles additional reserved areas used allocated by firmware, etc.  My
understanding is that there is some additional reserved area set up in
the hypervisor that we want reserved in the guest.  The idea was to
just use stolen_reserved_offset for that similar to what we do for the
bring up case for yellow carp.

Alex


>
> Regards,
> Christian.
>
> >       default:
> >               adev->mman.keep_stolen_vga_memory = false;
> >               break;
> > @@ -760,25 +778,6 @@ uint64_t amdgpu_gmc_vram_cpu_pa(struct amdgpu_device *adev, struct amdgpu_bo *bo
> >       return amdgpu_bo_gpu_offset(bo) - adev->gmc.vram_start + adev->gmc.aper_base;
> >   }
> >
> > -void amdgpu_gmc_get_reserved_allocation(struct amdgpu_device *adev)
> > -{
> > -     /* Some ASICs need to reserve a region of video memory to avoid access
> > -      * from driver */
> > -     adev->mman.stolen_reserved_offset = 0;
> > -     adev->mman.stolen_reserved_size = 0;
> > -
> > -     switch (adev->asic_type) {
> > -     case CHIP_YELLOW_CARP:
> > -             if (amdgpu_discovery == 0) {
> > -                     adev->mman.stolen_reserved_offset = 0x1ffb0000;
> > -                     adev->mman.stolen_reserved_size = 64 * PAGE_SIZE;
> > -             }
> > -             break;
> > -     default:
> > -             break;
> > -     }
> > -}
> > -
> >   int amdgpu_gmc_vram_checking(struct amdgpu_device *adev)
> >   {
> >       struct amdgpu_bo *vram_bo = NULL;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > index 93505bb0a36c..032b0313f277 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > @@ -331,7 +331,6 @@ amdgpu_gmc_set_vm_fault_masks(struct amdgpu_device *adev, int hub_type,
> >                             bool enable);
> >
> >   void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev);
> > -void amdgpu_gmc_get_reserved_allocation(struct amdgpu_device *adev);
> >
> >   void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev);
> >   uint64_t amdgpu_gmc_vram_mc2pa(struct amdgpu_device *adev, uint64_t mc_addr);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> > index f60b7bd4dbf5..3c1d440824a7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> > @@ -948,7 +948,6 @@ static int gmc_v10_0_sw_init(void *handle)
> >               return r;
> >
> >       amdgpu_gmc_get_vbios_allocations(adev);
> > -     amdgpu_gmc_get_reserved_allocation(adev);
> >
> >       /* Memory manager */
> >       r = amdgpu_bo_init(adev);
>

Re: [PATCH 1/2] drm/amdgpu: Use fw vram offset when allocating stolen vga memory.

[Alex Deucher]

On Mon, Mar 14, 2022 at 3:35 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 14.03.22 um 19:54 schrieb Yongqiang Sun:
> > [Why]
> > Memory access violation will happen in case of allocate stolen vga
> > memory with size isn't 0.
> >
> > [How]
> > when allocating stolen vga memory, use fw vram offset as the start point
> > instead of hard code value 0.
>
> Please stop separating commit message into [Why] and [How], that is not
> well received everywhere.
>
> Apart from that the patch is a certain NAK, you are messing things quite
> up here.
>
> >
> > Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
> > Change-Id: I7c555a6c1fb4b3fa8685753b4bdcbe215f89ea1e
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 31 +++++++++++++++++++------
> >   1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > index 41d6f604813d..1f635fdb0395 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > @@ -1708,6 +1708,7 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
> >       uint64_t gtt_size;
> >       int r;
> >       u64 vis_vram_limit;
> > +     u64 memory_offset = adev->mman.fw_vram_usage_start_offset + adev->mman.fw_vram_usage_size;
> >
> >       mutex_init(&adev->mman.gtt_window_lock);
> >
> > @@ -1774,24 +1775,40 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
> >        * This is used for VGA emulation and pre-OS scanout buffers to
> >        * avoid display artifacts while transitioning between pre-OS
> >        * and driver.  */
> > -     r = amdgpu_bo_create_kernel_at(adev, 0, adev->mman.stolen_vga_size,
> > +     r = amdgpu_bo_create_kernel_at(adev,
> > +                                        memory_offset,
> > +                                adev->mman.stolen_vga_size,
>
> That is certainly incorrect. See function amdgpu_ttm_fw_reserve_vram_init().
>
> The stolen VGA buffer always started at offset 0 and is independent of
> the range defined by fw_vram_usage_start_offset and fw_vram_usage_size.
>

Yeah these are separate ranges.  I just had a chat with Yongqiang and
there was some confusion around this, but this patch can be dropped.

Alex


> Regards,
> Christian.
>
> >                                      AMDGPU_GEM_DOMAIN_VRAM,
> >                                      &adev->mman.stolen_vga_memory,
> >                                      NULL);
> >       if (r)
> >               return r;
> > -     r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_vga_size,
> > +
> > +     memory_offset += adev->mman.stolen_vga_size;
> > +
> > +     r = amdgpu_bo_create_kernel_at(adev,
> > +                                        memory_offset,
> >                                      adev->mman.stolen_extended_size,
> >                                      AMDGPU_GEM_DOMAIN_VRAM,
> >                                      &adev->mman.stolen_extended_memory,
> >                                      NULL);
> >       if (r)
> >               return r;
> > -     r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_reserved_offset,
> > -                                    adev->mman.stolen_reserved_size,
> > -                                    AMDGPU_GEM_DOMAIN_VRAM,
> > -                                    &adev->mman.stolen_reserved_memory,
> > -                                    NULL);
> > +
> > +     memory_offset += adev->mman.stolen_extended_size;
> > +
> > +     if (adev->mman.stolen_reserved_offset > memory_offset)
> > +             r = amdgpu_bo_create_kernel_at(adev, adev->mman.stolen_reserved_offset,
> > +                                                adev->mman.stolen_reserved_size,
> > +                                                AMDGPU_GEM_DOMAIN_VRAM,
> > +                                                &adev->mman.stolen_reserved_memory,
> > +                                                NULL);
> > +     else if (adev->mman.stolen_reserved_offset + adev->mman.stolen_reserved_size > memory_offset)
> > +             r = amdgpu_bo_create_kernel_at(adev, memory_offset,
> > +                                             adev->mman.stolen_reserved_offset + adev->mman.stolen_reserved_size - memory_offset,
> > +                                                AMDGPU_GEM_DOMAIN_VRAM,
> > +                                                &adev->mman.stolen_reserved_memory,
> > +                                                NULL);
> >       if (r)
> >               return r;
> >
>

Re: [PATCH 2/2] drm/amdgpu: Add stolen reserved memory for MI25 SRIOV.

[Deucher, Alexander]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

[Public]

Series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
________________________________
From: Sun, Yongqiang <Yongqiang.Sun@amd.com>
Sent: Tuesday, March 15, 2022 10:11 AM
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Sun, Yongqiang <Yongqiang.Sun@amd.com>
Subject: [PATCH 2/2] drm/amdgpu: Add stolen reserved memory for MI25 SRIOV.

MI25 SRIOV guest driver loading failed due to allocated memory overlaps
with firmware reserved area.
Allocate stolen reserved memory for MI25 SRIOV specifically to avoid the
memory overlap.

Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
Change-Id: Ia1d1c4392fb792fa0186250dfc6270f35ffd6bed
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index f18d050a14cf..7021e8f390bd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -643,6 +643,15 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
          */
         switch (adev->asic_type) {
         case CHIP_VEGA10:
+               adev->mman.keep_stolen_vga_memory = true;
+               /*
+                * VEGA10 SRIOV VF needs some firmware reserved area.
+                */
+               if (amdgpu_sriov_vf(adev)) {
+                       adev->mman.stolen_reserved_offset = 0x100000;
+                       adev->mman.stolen_reserved_size = 0x600000;
+               }
+               break;
         case CHIP_RAVEN:
         case CHIP_RENOIR:
                 adev->mman.keep_stolen_vga_memory = true;
--
2.25.1


[-- Attachment #2: Type: text/html, Size: 3905 bytes --]

[PATCH 2/2] drm/amdgpu: Add stolen reserved memory for MI25 SRIOV.

[Yongqiang Sun]

MI25 SRIOV guest driver loading failed due to allocated memory overlaps
with firmware reserved area.
Allocate stolen reserved memory for MI25 SRIOV specifically to avoid the
memory overlap.

Signed-off-by: Yongqiang Sun <yongqiang.sun@amd.com>
Change-Id: Ia1d1c4392fb792fa0186250dfc6270f35ffd6bed
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index f18d050a14cf..7021e8f390bd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -643,6 +643,15 @@ void amdgpu_gmc_get_vbios_allocations(struct amdgpu_device *adev)
 	 */
 	switch (adev->asic_type) {
 	case CHIP_VEGA10:
+		adev->mman.keep_stolen_vga_memory = true;
+		/*
+		 * VEGA10 SRIOV VF needs some firmware reserved area.
+		 */
+		if (amdgpu_sriov_vf(adev)) {
+			adev->mman.stolen_reserved_offset = 0x100000;
+			adev->mman.stolen_reserved_size = 0x600000;
+		}
+		break;
 	case CHIP_RAVEN:
 	case CHIP_RENOIR:
 		adev->mman.keep_stolen_vga_memory = true;
-- 
2.25.1