All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange
@ 2022-11-18  2:56 Tong Liu01
  2022-11-21  8:14 ` Liu01, Tong (Esther)
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Tong Liu01 @ 2022-11-18  2:56 UTC (permalink / raw)
  To: amd-gfx
  Cc: Jack Xiao, Feifei Xu, horace.chen, Kevin Wang, Tong Liu01,
	Tuikov Luben, Deucher Alexander, yifan.zha, Evan Quan,
	Christian König, Monk Liu, Hawking Zhang

For vram_usagebyfirmware_v2_2, fw_vram_reserve is not used. So
fw_vram_usage_va is NULL, and cannot do virt data exchange
anymore. Should add drv_vram_usage_va to do virt data exchange
in vram_usagebyfirmware_v2_2 case. And refine some code style
checks in pre add vram reservation logic patch

Signed-off-by: Tong Liu01 <Tong.Liu01@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 16 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  7 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h       |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 59 +++++++++++++------
 4 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
index 9b97fa39d47a..e40df72c138a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
@@ -104,7 +104,7 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev)
 static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
 	struct vram_usagebyfirmware_v2_1 *fw_usage, int *usage_bytes)
 {
-	uint32_t start_addr, fw_size, drv_size;
+	u32 start_addr, fw_size, drv_size;
 
 	start_addr = le32_to_cpu(fw_usage->start_address_in_kb);
 	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
@@ -116,7 +116,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
 			  drv_size);
 
 	if ((start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
-		(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
+		(u32)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
 		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
 		/* Firmware request VRAM reservation for SR-IOV */
 		adev->mman.fw_vram_usage_start_offset = (start_addr &
@@ -133,7 +133,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
 static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
 		struct vram_usagebyfirmware_v2_2 *fw_usage, int *usage_bytes)
 {
-	uint32_t fw_start_addr, fw_size, drv_start_addr, drv_size;
+	u32 fw_start_addr, fw_size, drv_start_addr, drv_size;
 
 	fw_start_addr = le32_to_cpu(fw_usage->fw_region_start_address_in_kb);
 	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
@@ -147,14 +147,16 @@ static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
 			  drv_start_addr,
 			  drv_size);
 
-	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
+	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
+		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
 		/* Firmware request VRAM reservation for SR-IOV */
 		adev->mman.fw_vram_usage_start_offset = (fw_start_addr &
 			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
 		adev->mman.fw_vram_usage_size = fw_size << 10;
 	}
 
-	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
+	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
+		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
 		/* driver request VRAM reservation for SR-IOV */
 		adev->mman.drv_vram_usage_start_offset = (drv_start_addr &
 			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
@@ -172,8 +174,8 @@ int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev)
 						vram_usagebyfirmware);
 	struct vram_usagebyfirmware_v2_1 *fw_usage_v2_1;
 	struct vram_usagebyfirmware_v2_2 *fw_usage_v2_2;
-	uint16_t data_offset;
-	uint8_t frev, crev;
+	u16 data_offset;
+	u8 frev, crev;
 	int usage_bytes = 0;
 
 	if (amdgpu_atom_parse_data_header(ctx, index, NULL, &frev, &crev, &data_offset)) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 52f2282411cb..dd8b6a11db9a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1570,7 +1570,7 @@ static void amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device *adev)
 {
 	amdgpu_bo_free_kernel(&adev->mman.drv_vram_usage_reserved_bo,
 						  NULL,
-						  NULL);
+						  &adev->mman.drv_vram_usage_va);
 }
 
 /**
@@ -1608,8 +1608,9 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
  */
 static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
 {
-	uint64_t vram_size = adev->gmc.visible_vram_size;
+	u64 vram_size = adev->gmc.visible_vram_size;
 
+	adev->mman.drv_vram_usage_va = NULL;
 	adev->mman.drv_vram_usage_reserved_bo = NULL;
 
 	if (adev->mman.drv_vram_usage_size == 0 ||
@@ -1621,7 +1622,7 @@ static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
 					  adev->mman.drv_vram_usage_size,
 					  AMDGPU_GEM_DOMAIN_VRAM,
 					  &adev->mman.drv_vram_usage_reserved_bo,
-					  NULL);
+					  &adev->mman.drv_vram_usage_va);
 }
 
 /*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index b391c8d076ff..b4d8ba2789f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -90,6 +90,7 @@ struct amdgpu_mman {
 	u64		drv_vram_usage_start_offset;
 	u64		drv_vram_usage_size;
 	struct amdgpu_bo	*drv_vram_usage_reserved_bo;
+	void		*drv_vram_usage_va;
 
 	/* PAGE_SIZE'd BO for process memory r/w over SDMA. */
 	struct amdgpu_bo	*sdma_access_bo;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index a226a6c48fb7..e80033e24d48 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -428,11 +428,18 @@ static void amdgpu_virt_add_bad_page(struct amdgpu_device *adev,
 	struct eeprom_table_record bp;
 	uint64_t retired_page;
 	uint32_t bp_idx, bp_cnt;
+	void *vram_usage_va = NULL;
+
+	if (adev->mman.fw_vram_usage_va != NULL) {
+		vram_usage_va = adev->mman.fw_vram_usage_va;
+	} else {
+		vram_usage_va = adev->mman.drv_vram_usage_va;
+	}
 
 	if (bp_block_size) {
 		bp_cnt = bp_block_size / sizeof(uint64_t);
 		for (bp_idx = 0; bp_idx < bp_cnt; bp_idx++) {
-			retired_page = *(uint64_t *)(adev->mman.fw_vram_usage_va +
+			retired_page = *(uint64_t *)(vram_usage_va +
 					bp_block_offset + bp_idx * sizeof(uint64_t));
 			bp.retired_page = retired_page;
 
@@ -643,7 +650,11 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
 	adev->virt.fw_reserve.p_vf2pf = NULL;
 	adev->virt.vf2pf_update_interval_ms = 0;
 
-	if (adev->mman.fw_vram_usage_va != NULL) {
+	if (adev->mman.fw_vram_usage_va != NULL &&
+		adev->mman.drv_vram_usage_va != NULL) {
+		DRM_WARN("Currently fw_vram and drv_vram should not have values at the same time!");
+	} else if (adev->mman.fw_vram_usage_va != NULL ||
+		adev->mman.drv_vram_usage_va != NULL) {
 		/* go through this logic in ip_init and reset to init workqueue*/
 		amdgpu_virt_exchange_data(adev);
 
@@ -666,32 +677,42 @@ void amdgpu_virt_exchange_data(struct amdgpu_device *adev)
 	uint32_t bp_block_size = 0;
 	struct amd_sriov_msg_pf2vf_info *pf2vf_v2 = NULL;
 
-	if (adev->mman.fw_vram_usage_va != NULL) {
-
-		adev->virt.fw_reserve.p_pf2vf =
-			(struct amd_sriov_msg_pf2vf_info_header *)
-			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
-		adev->virt.fw_reserve.p_vf2pf =
-			(struct amd_sriov_msg_vf2pf_info_header *)
-			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+	if (adev->mman.fw_vram_usage_va != NULL ||
+		adev->mman.drv_vram_usage_va != NULL) {
+
+		if (adev->mman.fw_vram_usage_va != NULL) {
+			adev->virt.fw_reserve.p_pf2vf =
+				(struct amd_sriov_msg_pf2vf_info_header *)
+				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
+			adev->virt.fw_reserve.p_vf2pf =
+				(struct amd_sriov_msg_vf2pf_info_header *)
+				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+		} else if (adev->mman.drv_vram_usage_va != NULL) {
+			adev->virt.fw_reserve.p_pf2vf =
+				(struct amd_sriov_msg_pf2vf_info_header *)
+				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
+			adev->virt.fw_reserve.p_vf2pf =
+				(struct amd_sriov_msg_vf2pf_info_header *)
+				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+		}
 
 		amdgpu_virt_read_pf2vf_data(adev);
 		amdgpu_virt_write_vf2pf_data(adev);
 
 		/* bad page handling for version 2 */
 		if (adev->virt.fw_reserve.p_pf2vf->version == 2) {
-				pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
+			pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
 
-				bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
-						((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
-				bp_block_size = pf2vf_v2->bp_block_size;
+			bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
+				((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
+			bp_block_size = pf2vf_v2->bp_block_size;
 
-				if (bp_block_size && !adev->virt.ras_init_done)
-					amdgpu_virt_init_ras_err_handler_data(adev);
+			if (bp_block_size && !adev->virt.ras_init_done)
+				amdgpu_virt_init_ras_err_handler_data(adev);
 
-				if (adev->virt.ras_init_done)
-					amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
-			}
+			if (adev->virt.ras_init_done)
+				amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
+		}
 	}
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* RE: [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange
  2022-11-18  2:56 [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange Tong Liu01
@ 2022-11-21  8:14 ` Liu01, Tong (Esther)
  2022-11-21  8:24 ` Christian König
  2022-11-21 16:42 ` Luben Tuikov
  2 siblings, 0 replies; 7+ messages in thread
From: Liu01, Tong (Esther) @ 2022-11-21  8:14 UTC (permalink / raw)
  To: Liu01, Tong (Esther), amd-gfx
  Cc: Xiao, Jack, Wang, Yang(Kevin),
	Xu, Feifei, Chen, Horace, Tuikov, Luben, Deucher, Alexander, Zha,
	YiFan(Even),
	Quan, Evan, Koenig, Christian, Liu, Monk, Zhang, Hawking

[AMD Official Use Only - General]

Hi,

Please review, thanks

Kind regards,
Esther

-----Original Message-----
From: Tong Liu01 <Tong.Liu01@amd.com> 
Sent: 2022年11月18日星期五 上午10:56
To: amd-gfx@lists.freedesktop.org
Cc: Quan, Evan <Evan.Quan@amd.com>; Chen, Horace <Horace.Chen@amd.com>; Tuikov, Luben <Luben.Tuikov@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Xiao, Jack <Jack.Xiao@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>; Liu, Monk <Monk.Liu@amd.com>; Xu, Feifei <Feifei.Xu@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>; Liu01, Tong (Esther) <Tong.Liu01@amd.com>; Zha, YiFan(Even) <Yifan.Zha@amd.com>; Liu01, Tong (Esther) <Tong.Liu01@amd.com>
Subject: [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange

For vram_usagebyfirmware_v2_2, fw_vram_reserve is not used. So fw_vram_usage_va is NULL, and cannot do virt data exchange anymore. Should add drv_vram_usage_va to do virt data exchange in vram_usagebyfirmware_v2_2 case. And refine some code style checks in pre add vram reservation logic patch

Signed-off-by: Tong Liu01 <Tong.Liu01@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 16 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  7 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h       |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 59 +++++++++++++------
 4 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
index 9b97fa39d47a..e40df72c138a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
@@ -104,7 +104,7 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev)  static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
 	struct vram_usagebyfirmware_v2_1 *fw_usage, int *usage_bytes)  {
-	uint32_t start_addr, fw_size, drv_size;
+	u32 start_addr, fw_size, drv_size;
 
 	start_addr = le32_to_cpu(fw_usage->start_address_in_kb);
 	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
@@ -116,7 +116,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
 			  drv_size);
 
 	if ((start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
-		(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
+		(u32)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
 		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
 		/* Firmware request VRAM reservation for SR-IOV */
 		adev->mman.fw_vram_usage_start_offset = (start_addr & @@ -133,7 +133,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,  static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
 		struct vram_usagebyfirmware_v2_2 *fw_usage, int *usage_bytes)  {
-	uint32_t fw_start_addr, fw_size, drv_start_addr, drv_size;
+	u32 fw_start_addr, fw_size, drv_start_addr, drv_size;
 
 	fw_start_addr = le32_to_cpu(fw_usage->fw_region_start_address_in_kb);
 	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
@@ -147,14 +147,16 @@ static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
 			  drv_start_addr,
 			  drv_size);
 
-	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
+	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
+		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
 		/* Firmware request VRAM reservation for SR-IOV */
 		adev->mman.fw_vram_usage_start_offset = (fw_start_addr &
 			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
 		adev->mman.fw_vram_usage_size = fw_size << 10;
 	}
 
-	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
+	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
+		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
 		/* driver request VRAM reservation for SR-IOV */
 		adev->mman.drv_vram_usage_start_offset = (drv_start_addr &
 			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10; @@ -172,8 +174,8 @@ int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev)
 						vram_usagebyfirmware);
 	struct vram_usagebyfirmware_v2_1 *fw_usage_v2_1;
 	struct vram_usagebyfirmware_v2_2 *fw_usage_v2_2;
-	uint16_t data_offset;
-	uint8_t frev, crev;
+	u16 data_offset;
+	u8 frev, crev;
 	int usage_bytes = 0;
 
 	if (amdgpu_atom_parse_data_header(ctx, index, NULL, &frev, &crev, &data_offset)) { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 52f2282411cb..dd8b6a11db9a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1570,7 +1570,7 @@ static void amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device *adev)  {
 	amdgpu_bo_free_kernel(&adev->mman.drv_vram_usage_reserved_bo,
 						  NULL,
-						  NULL);
+						  &adev->mman.drv_vram_usage_va);
 }
 
 /**
@@ -1608,8 +1608,9 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
  */
 static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)  {
-	uint64_t vram_size = adev->gmc.visible_vram_size;
+	u64 vram_size = adev->gmc.visible_vram_size;
 
+	adev->mman.drv_vram_usage_va = NULL;
 	adev->mman.drv_vram_usage_reserved_bo = NULL;
 
 	if (adev->mman.drv_vram_usage_size == 0 || @@ -1621,7 +1622,7 @@ static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
 					  adev->mman.drv_vram_usage_size,
 					  AMDGPU_GEM_DOMAIN_VRAM,
 					  &adev->mman.drv_vram_usage_reserved_bo,
-					  NULL);
+					  &adev->mman.drv_vram_usage_va);
 }
 
 /*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index b391c8d076ff..b4d8ba2789f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -90,6 +90,7 @@ struct amdgpu_mman {
 	u64		drv_vram_usage_start_offset;
 	u64		drv_vram_usage_size;
 	struct amdgpu_bo	*drv_vram_usage_reserved_bo;
+	void		*drv_vram_usage_va;
 
 	/* PAGE_SIZE'd BO for process memory r/w over SDMA. */
 	struct amdgpu_bo	*sdma_access_bo;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index a226a6c48fb7..e80033e24d48 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -428,11 +428,18 @@ static void amdgpu_virt_add_bad_page(struct amdgpu_device *adev,
 	struct eeprom_table_record bp;
 	uint64_t retired_page;
 	uint32_t bp_idx, bp_cnt;
+	void *vram_usage_va = NULL;
+
+	if (adev->mman.fw_vram_usage_va != NULL) {
+		vram_usage_va = adev->mman.fw_vram_usage_va;
+	} else {
+		vram_usage_va = adev->mman.drv_vram_usage_va;
+	}
 
 	if (bp_block_size) {
 		bp_cnt = bp_block_size / sizeof(uint64_t);
 		for (bp_idx = 0; bp_idx < bp_cnt; bp_idx++) {
-			retired_page = *(uint64_t *)(adev->mman.fw_vram_usage_va +
+			retired_page = *(uint64_t *)(vram_usage_va +
 					bp_block_offset + bp_idx * sizeof(uint64_t));
 			bp.retired_page = retired_page;
 
@@ -643,7 +650,11 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
 	adev->virt.fw_reserve.p_vf2pf = NULL;
 	adev->virt.vf2pf_update_interval_ms = 0;
 
-	if (adev->mman.fw_vram_usage_va != NULL) {
+	if (adev->mman.fw_vram_usage_va != NULL &&
+		adev->mman.drv_vram_usage_va != NULL) {
+		DRM_WARN("Currently fw_vram and drv_vram should not have values at the same time!");
+	} else if (adev->mman.fw_vram_usage_va != NULL ||
+		adev->mman.drv_vram_usage_va != NULL) {
 		/* go through this logic in ip_init and reset to init workqueue*/
 		amdgpu_virt_exchange_data(adev);
 
@@ -666,32 +677,42 @@ void amdgpu_virt_exchange_data(struct amdgpu_device *adev)
 	uint32_t bp_block_size = 0;
 	struct amd_sriov_msg_pf2vf_info *pf2vf_v2 = NULL;
 
-	if (adev->mman.fw_vram_usage_va != NULL) {
-
-		adev->virt.fw_reserve.p_pf2vf =
-			(struct amd_sriov_msg_pf2vf_info_header *)
-			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
-		adev->virt.fw_reserve.p_vf2pf =
-			(struct amd_sriov_msg_vf2pf_info_header *)
-			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+	if (adev->mman.fw_vram_usage_va != NULL ||
+		adev->mman.drv_vram_usage_va != NULL) {
+
+		if (adev->mman.fw_vram_usage_va != NULL) {
+			adev->virt.fw_reserve.p_pf2vf =
+				(struct amd_sriov_msg_pf2vf_info_header *)
+				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
+			adev->virt.fw_reserve.p_vf2pf =
+				(struct amd_sriov_msg_vf2pf_info_header *)
+				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+		} else if (adev->mman.drv_vram_usage_va != NULL) {
+			adev->virt.fw_reserve.p_pf2vf =
+				(struct amd_sriov_msg_pf2vf_info_header *)
+				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
+			adev->virt.fw_reserve.p_vf2pf =
+				(struct amd_sriov_msg_vf2pf_info_header *)
+				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+		}
 
 		amdgpu_virt_read_pf2vf_data(adev);
 		amdgpu_virt_write_vf2pf_data(adev);
 
 		/* bad page handling for version 2 */
 		if (adev->virt.fw_reserve.p_pf2vf->version == 2) {
-				pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
+			pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info 
+*)adev->virt.fw_reserve.p_pf2vf;
 
-				bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
-						((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
-				bp_block_size = pf2vf_v2->bp_block_size;
+			bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
+				((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
+			bp_block_size = pf2vf_v2->bp_block_size;
 
-				if (bp_block_size && !adev->virt.ras_init_done)
-					amdgpu_virt_init_ras_err_handler_data(adev);
+			if (bp_block_size && !adev->virt.ras_init_done)
+				amdgpu_virt_init_ras_err_handler_data(adev);
 
-				if (adev->virt.ras_init_done)
-					amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
-			}
+			if (adev->virt.ras_init_done)
+				amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
+		}
 	}
 }
 
--
2.25.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange
  2022-11-18  2:56 [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange Tong Liu01
  2022-11-21  8:14 ` Liu01, Tong (Esther)
@ 2022-11-21  8:24 ` Christian König
  2022-11-21 16:26   ` Luben Tuikov
  2022-11-21 16:42 ` Luben Tuikov
  2 siblings, 1 reply; 7+ messages in thread
From: Christian König @ 2022-11-21  8:24 UTC (permalink / raw)
  To: Tong Liu01, amd-gfx
  Cc: Jack Xiao, Feifei Xu, horace.chen, Kevin Wang, Tuikov Luben,
	Deucher Alexander, yifan.zha, Evan Quan, Monk Liu, Hawking Zhang

Am 18.11.22 um 03:56 schrieb Tong Liu01:
> For vram_usagebyfirmware_v2_2, fw_vram_reserve is not used. So
> fw_vram_usage_va is NULL, and cannot do virt data exchange
> anymore. Should add drv_vram_usage_va to do virt data exchange
> in vram_usagebyfirmware_v2_2 case. And refine some code style
> checks in pre add vram reservation logic patch
>
> Signed-off-by: Tong Liu01 <Tong.Liu01@amd.com>
> ---
>   .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 16 ++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  7 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h       |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 59 +++++++++++++------
>   4 files changed, 54 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> index 9b97fa39d47a..e40df72c138a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> @@ -104,7 +104,7 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev)
>   static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>   	struct vram_usagebyfirmware_v2_1 *fw_usage, int *usage_bytes)
>   {
> -	uint32_t start_addr, fw_size, drv_size;
> +	u32 start_addr, fw_size, drv_size;
>   
>   	start_addr = le32_to_cpu(fw_usage->start_address_in_kb);
>   	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
> @@ -116,7 +116,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>   			  drv_size);
>   
>   	if ((start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
> -		(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
> +		(u32)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
>   		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
>   		/* Firmware request VRAM reservation for SR-IOV */
>   		adev->mman.fw_vram_usage_start_offset = (start_addr &
> @@ -133,7 +133,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>   static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
>   		struct vram_usagebyfirmware_v2_2 *fw_usage, int *usage_bytes)
>   {
> -	uint32_t fw_start_addr, fw_size, drv_start_addr, drv_size;
> +	u32 fw_start_addr, fw_size, drv_start_addr, drv_size;
>   
>   	fw_start_addr = le32_to_cpu(fw_usage->fw_region_start_address_in_kb);
>   	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
> @@ -147,14 +147,16 @@ static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
>   			  drv_start_addr,
>   			  drv_size);
>   
> -	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
> +	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
> +		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
>   		/* Firmware request VRAM reservation for SR-IOV */
>   		adev->mman.fw_vram_usage_start_offset = (fw_start_addr &
>   			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
>   		adev->mman.fw_vram_usage_size = fw_size << 10;
>   	}
>   
> -	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
> +	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
> +		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
>   		/* driver request VRAM reservation for SR-IOV */
>   		adev->mman.drv_vram_usage_start_offset = (drv_start_addr &
>   			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
> @@ -172,8 +174,8 @@ int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev)
>   						vram_usagebyfirmware);
>   	struct vram_usagebyfirmware_v2_1 *fw_usage_v2_1;
>   	struct vram_usagebyfirmware_v2_2 *fw_usage_v2_2;
> -	uint16_t data_offset;
> -	uint8_t frev, crev;
> +	u16 data_offset;
> +	u8 frev, crev;
>   	int usage_bytes = 0;
>   
>   	if (amdgpu_atom_parse_data_header(ctx, index, NULL, &frev, &crev, &data_offset)) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 52f2282411cb..dd8b6a11db9a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1570,7 +1570,7 @@ static void amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device *adev)
>   {
>   	amdgpu_bo_free_kernel(&adev->mman.drv_vram_usage_reserved_bo,
>   						  NULL,
> -						  NULL);
> +						  &adev->mman.drv_vram_usage_va);

Your indentations of the second like with "if"s and function parameters 
like here still looks completely off.

>   }
>   
>   /**
> @@ -1608,8 +1608,9 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
>    */
>   static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
>   {
> -	uint64_t vram_size = adev->gmc.visible_vram_size;
> +	u64 vram_size = adev->gmc.visible_vram_size;
>   
> +	adev->mman.drv_vram_usage_va = NULL;
>   	adev->mman.drv_vram_usage_reserved_bo = NULL;
>   
>   	if (adev->mman.drv_vram_usage_size == 0 ||
> @@ -1621,7 +1622,7 @@ static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
>   					  adev->mman.drv_vram_usage_size,
>   					  AMDGPU_GEM_DOMAIN_VRAM,
>   					  &adev->mman.drv_vram_usage_reserved_bo,
> -					  NULL);
> +					  &adev->mman.drv_vram_usage_va);
>   }
>   
>   /*
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index b391c8d076ff..b4d8ba2789f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -90,6 +90,7 @@ struct amdgpu_mman {
>   	u64		drv_vram_usage_start_offset;
>   	u64		drv_vram_usage_size;
>   	struct amdgpu_bo	*drv_vram_usage_reserved_bo;
> +	void		*drv_vram_usage_va;
>   
>   	/* PAGE_SIZE'd BO for process memory r/w over SDMA. */
>   	struct amdgpu_bo	*sdma_access_bo;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index a226a6c48fb7..e80033e24d48 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -428,11 +428,18 @@ static void amdgpu_virt_add_bad_page(struct amdgpu_device *adev,
>   	struct eeprom_table_record bp;
>   	uint64_t retired_page;
>   	uint32_t bp_idx, bp_cnt;
> +	void *vram_usage_va = NULL;
> +
> +	if (adev->mman.fw_vram_usage_va != NULL) {
> +		vram_usage_va = adev->mman.fw_vram_usage_va;
> +	} else {
> +		vram_usage_va = adev->mman.drv_vram_usage_va;
> +	}

Please no {} for single line "if"s.

Apart from that looks sane of hand, but I'm not the right person to 
fully judge the technical implementation.

Luben can you tale a look as well?

Thanks,
Christian.

>   
>   	if (bp_block_size) {
>   		bp_cnt = bp_block_size / sizeof(uint64_t);
>   		for (bp_idx = 0; bp_idx < bp_cnt; bp_idx++) {
> -			retired_page = *(uint64_t *)(adev->mman.fw_vram_usage_va +
> +			retired_page = *(uint64_t *)(vram_usage_va +
>   					bp_block_offset + bp_idx * sizeof(uint64_t));
>   			bp.retired_page = retired_page;
>   
> @@ -643,7 +650,11 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
>   	adev->virt.fw_reserve.p_vf2pf = NULL;
>   	adev->virt.vf2pf_update_interval_ms = 0;
>   
> -	if (adev->mman.fw_vram_usage_va != NULL) {
> +	if (adev->mman.fw_vram_usage_va != NULL &&
> +		adev->mman.drv_vram_usage_va != NULL) {
> +		DRM_WARN("Currently fw_vram and drv_vram should not have values at the same time!");
> +	} else if (adev->mman.fw_vram_usage_va != NULL ||
> +		adev->mman.drv_vram_usage_va != NULL) {
>   		/* go through this logic in ip_init and reset to init workqueue*/
>   		amdgpu_virt_exchange_data(adev);
>   
> @@ -666,32 +677,42 @@ void amdgpu_virt_exchange_data(struct amdgpu_device *adev)
>   	uint32_t bp_block_size = 0;
>   	struct amd_sriov_msg_pf2vf_info *pf2vf_v2 = NULL;
>   
> -	if (adev->mman.fw_vram_usage_va != NULL) {
> -
> -		adev->virt.fw_reserve.p_pf2vf =
> -			(struct amd_sriov_msg_pf2vf_info_header *)
> -			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> -		adev->virt.fw_reserve.p_vf2pf =
> -			(struct amd_sriov_msg_vf2pf_info_header *)
> -			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +	if (adev->mman.fw_vram_usage_va != NULL ||
> +		adev->mman.drv_vram_usage_va != NULL) {
> +
> +		if (adev->mman.fw_vram_usage_va != NULL) {
> +			adev->virt.fw_reserve.p_pf2vf =
> +				(struct amd_sriov_msg_pf2vf_info_header *)
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +			adev->virt.fw_reserve.p_vf2pf =
> +				(struct amd_sriov_msg_vf2pf_info_header *)
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +		} else if (adev->mman.drv_vram_usage_va != NULL) {
> +			adev->virt.fw_reserve.p_pf2vf =
> +				(struct amd_sriov_msg_pf2vf_info_header *)
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +			adev->virt.fw_reserve.p_vf2pf =
> +				(struct amd_sriov_msg_vf2pf_info_header *)
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +		}
>   
>   		amdgpu_virt_read_pf2vf_data(adev);
>   		amdgpu_virt_write_vf2pf_data(adev);
>   
>   		/* bad page handling for version 2 */
>   		if (adev->virt.fw_reserve.p_pf2vf->version == 2) {
> -				pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
> +			pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
>   
> -				bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
> -						((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
> -				bp_block_size = pf2vf_v2->bp_block_size;
> +			bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
> +				((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
> +			bp_block_size = pf2vf_v2->bp_block_size;
>   
> -				if (bp_block_size && !adev->virt.ras_init_done)
> -					amdgpu_virt_init_ras_err_handler_data(adev);
> +			if (bp_block_size && !adev->virt.ras_init_done)
> +				amdgpu_virt_init_ras_err_handler_data(adev);
>   
> -				if (adev->virt.ras_init_done)
> -					amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
> -			}
> +			if (adev->virt.ras_init_done)
> +				amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
> +		}
>   	}
>   }
>   


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange
  2022-11-21  8:24 ` Christian König
@ 2022-11-21 16:26   ` Luben Tuikov
  0 siblings, 0 replies; 7+ messages in thread
From: Luben Tuikov @ 2022-11-21 16:26 UTC (permalink / raw)
  To: Christian König, Tong Liu01, amd-gfx
  Cc: Jack Xiao, Feifei Xu, horace.chen, Kevin Wang, Deucher Alexander,
	yifan.zha, Evan Quan, Monk Liu, Hawking Zhang

On 2022-11-21 03:24, Christian König wrote:
> Am 18.11.22 um 03:56 schrieb Tong Liu01:
>> For vram_usagebyfirmware_v2_2, fw_vram_reserve is not used. So
>> fw_vram_usage_va is NULL, and cannot do virt data exchange
>> anymore. Should add drv_vram_usage_va to do virt data exchange
>> in vram_usagebyfirmware_v2_2 case. And refine some code style
>> checks in pre add vram reservation logic patch
>>
>> Signed-off-by: Tong Liu01 <Tong.Liu01@amd.com>
>> ---
>>   .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 16 ++---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  7 ++-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h       |  1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 59 +++++++++++++------
>>   4 files changed, 54 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
>> index 9b97fa39d47a..e40df72c138a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
>> @@ -104,7 +104,7 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev)
>>   static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>>   	struct vram_usagebyfirmware_v2_1 *fw_usage, int *usage_bytes)
>>   {
>> -	uint32_t start_addr, fw_size, drv_size;
>> +	u32 start_addr, fw_size, drv_size;
>>   
>>   	start_addr = le32_to_cpu(fw_usage->start_address_in_kb);
>>   	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
>> @@ -116,7 +116,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>>   			  drv_size);
>>   
>>   	if ((start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
>> -		(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
>> +		(u32)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
>>   		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
>>   		/* Firmware request VRAM reservation for SR-IOV */
>>   		adev->mman.fw_vram_usage_start_offset = (start_addr &
>> @@ -133,7 +133,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>>   static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
>>   		struct vram_usagebyfirmware_v2_2 *fw_usage, int *usage_bytes)
>>   {
>> -	uint32_t fw_start_addr, fw_size, drv_start_addr, drv_size;
>> +	u32 fw_start_addr, fw_size, drv_start_addr, drv_size;
>>   
>>   	fw_start_addr = le32_to_cpu(fw_usage->fw_region_start_address_in_kb);
>>   	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
>> @@ -147,14 +147,16 @@ static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
>>   			  drv_start_addr,
>>   			  drv_size);
>>   
>> -	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
>> +	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
>> +		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
>>   		/* Firmware request VRAM reservation for SR-IOV */
>>   		adev->mman.fw_vram_usage_start_offset = (fw_start_addr &
>>   			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
>>   		adev->mman.fw_vram_usage_size = fw_size << 10;
>>   	}
>>   
>> -	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
>> +	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
>> +		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
>>   		/* driver request VRAM reservation for SR-IOV */
>>   		adev->mman.drv_vram_usage_start_offset = (drv_start_addr &
>>   			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
>> @@ -172,8 +174,8 @@ int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev)
>>   						vram_usagebyfirmware);
>>   	struct vram_usagebyfirmware_v2_1 *fw_usage_v2_1;
>>   	struct vram_usagebyfirmware_v2_2 *fw_usage_v2_2;
>> -	uint16_t data_offset;
>> -	uint8_t frev, crev;
>> +	u16 data_offset;
>> +	u8 frev, crev;
>>   	int usage_bytes = 0;
>>   
>>   	if (amdgpu_atom_parse_data_header(ctx, index, NULL, &frev, &crev, &data_offset)) {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 52f2282411cb..dd8b6a11db9a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1570,7 +1570,7 @@ static void amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device *adev)
>>   {
>>   	amdgpu_bo_free_kernel(&adev->mman.drv_vram_usage_reserved_bo,
>>   						  NULL,
>> -						  NULL);
>> +						  &adev->mman.drv_vram_usage_va);
> 
> Your indentations of the second like with "if"s and function parameters 
> like here still looks completely off.
> 
>>   }
>>   
>>   /**
>> @@ -1608,8 +1608,9 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
>>    */
>>   static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
>>   {
>> -	uint64_t vram_size = adev->gmc.visible_vram_size;
>> +	u64 vram_size = adev->gmc.visible_vram_size;
>>   
>> +	adev->mman.drv_vram_usage_va = NULL;
>>   	adev->mman.drv_vram_usage_reserved_bo = NULL;
>>   
>>   	if (adev->mman.drv_vram_usage_size == 0 ||
>> @@ -1621,7 +1622,7 @@ static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
>>   					  adev->mman.drv_vram_usage_size,
>>   					  AMDGPU_GEM_DOMAIN_VRAM,
>>   					  &adev->mman.drv_vram_usage_reserved_bo,
>> -					  NULL);
>> +					  &adev->mman.drv_vram_usage_va);
>>   }
>>   
>>   /*
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> index b391c8d076ff..b4d8ba2789f3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> @@ -90,6 +90,7 @@ struct amdgpu_mman {
>>   	u64		drv_vram_usage_start_offset;
>>   	u64		drv_vram_usage_size;
>>   	struct amdgpu_bo	*drv_vram_usage_reserved_bo;
>> +	void		*drv_vram_usage_va;
>>   
>>   	/* PAGE_SIZE'd BO for process memory r/w over SDMA. */
>>   	struct amdgpu_bo	*sdma_access_bo;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> index a226a6c48fb7..e80033e24d48 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
>> @@ -428,11 +428,18 @@ static void amdgpu_virt_add_bad_page(struct amdgpu_device *adev,
>>   	struct eeprom_table_record bp;
>>   	uint64_t retired_page;
>>   	uint32_t bp_idx, bp_cnt;
>> +	void *vram_usage_va = NULL;
>> +
>> +	if (adev->mman.fw_vram_usage_va != NULL) {
>> +		vram_usage_va = adev->mman.fw_vram_usage_va;
>> +	} else {
>> +		vram_usage_va = adev->mman.drv_vram_usage_va;
>> +	}
> 
> Please no {} for single line "if"s.
> 
> Apart from that looks sane of hand, but I'm not the right person to 
> fully judge the technical implementation.
> 
> Luben can you tale a look as well?

I looked at it and I thought it looks good, but was waiting for other ppl
to take a look. I'll give an in-depth review now too. Thanks!

Regards,
Luben

> 
> Thanks,
> Christian.
> 
>>   
>>   	if (bp_block_size) {
>>   		bp_cnt = bp_block_size / sizeof(uint64_t);
>>   		for (bp_idx = 0; bp_idx < bp_cnt; bp_idx++) {
>> -			retired_page = *(uint64_t *)(adev->mman.fw_vram_usage_va +
>> +			retired_page = *(uint64_t *)(vram_usage_va +
>>   					bp_block_offset + bp_idx * sizeof(uint64_t));
>>   			bp.retired_page = retired_page;
>>   
>> @@ -643,7 +650,11 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
>>   	adev->virt.fw_reserve.p_vf2pf = NULL;
>>   	adev->virt.vf2pf_update_interval_ms = 0;
>>   
>> -	if (adev->mman.fw_vram_usage_va != NULL) {
>> +	if (adev->mman.fw_vram_usage_va != NULL &&
>> +		adev->mman.drv_vram_usage_va != NULL) {
>> +		DRM_WARN("Currently fw_vram and drv_vram should not have values at the same time!");
>> +	} else if (adev->mman.fw_vram_usage_va != NULL ||
>> +		adev->mman.drv_vram_usage_va != NULL) {
>>   		/* go through this logic in ip_init and reset to init workqueue*/
>>   		amdgpu_virt_exchange_data(adev);
>>   
>> @@ -666,32 +677,42 @@ void amdgpu_virt_exchange_data(struct amdgpu_device *adev)
>>   	uint32_t bp_block_size = 0;
>>   	struct amd_sriov_msg_pf2vf_info *pf2vf_v2 = NULL;
>>   
>> -	if (adev->mman.fw_vram_usage_va != NULL) {
>> -
>> -		adev->virt.fw_reserve.p_pf2vf =
>> -			(struct amd_sriov_msg_pf2vf_info_header *)
>> -			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
>> -		adev->virt.fw_reserve.p_vf2pf =
>> -			(struct amd_sriov_msg_vf2pf_info_header *)
>> -			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
>> +	if (adev->mman.fw_vram_usage_va != NULL ||
>> +		adev->mman.drv_vram_usage_va != NULL) {
>> +
>> +		if (adev->mman.fw_vram_usage_va != NULL) {
>> +			adev->virt.fw_reserve.p_pf2vf =
>> +				(struct amd_sriov_msg_pf2vf_info_header *)
>> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
>> +			adev->virt.fw_reserve.p_vf2pf =
>> +				(struct amd_sriov_msg_vf2pf_info_header *)
>> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
>> +		} else if (adev->mman.drv_vram_usage_va != NULL) {
>> +			adev->virt.fw_reserve.p_pf2vf =
>> +				(struct amd_sriov_msg_pf2vf_info_header *)
>> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
>> +			adev->virt.fw_reserve.p_vf2pf =
>> +				(struct amd_sriov_msg_vf2pf_info_header *)
>> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
>> +		}
>>   
>>   		amdgpu_virt_read_pf2vf_data(adev);
>>   		amdgpu_virt_write_vf2pf_data(adev);
>>   
>>   		/* bad page handling for version 2 */
>>   		if (adev->virt.fw_reserve.p_pf2vf->version == 2) {
>> -				pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
>> +			pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
>>   
>> -				bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
>> -						((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
>> -				bp_block_size = pf2vf_v2->bp_block_size;
>> +			bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
>> +				((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
>> +			bp_block_size = pf2vf_v2->bp_block_size;
>>   
>> -				if (bp_block_size && !adev->virt.ras_init_done)
>> -					amdgpu_virt_init_ras_err_handler_data(adev);
>> +			if (bp_block_size && !adev->virt.ras_init_done)
>> +				amdgpu_virt_init_ras_err_handler_data(adev);
>>   
>> -				if (adev->virt.ras_init_done)
>> -					amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
>> -			}
>> +			if (adev->virt.ras_init_done)
>> +				amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
>> +		}
>>   	}
>>   }
>>   
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange
  2022-11-18  2:56 [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange Tong Liu01
  2022-11-21  8:14 ` Liu01, Tong (Esther)
  2022-11-21  8:24 ` Christian König
@ 2022-11-21 16:42 ` Luben Tuikov
  2 siblings, 0 replies; 7+ messages in thread
From: Luben Tuikov @ 2022-11-21 16:42 UTC (permalink / raw)
  To: Tong Liu01, amd-gfx
  Cc: Jack Xiao, Feifei Xu, horace.chen, Kevin Wang, Deucher Alexander,
	yifan.zha, Evan Quan, Christian König, Monk Liu,
	Hawking Zhang

On 2022-11-17 21:56, Tong Liu01 wrote:
> For vram_usagebyfirmware_v2_2, fw_vram_reserve is not used. So
> fw_vram_usage_va is NULL, and cannot do virt data exchange
> anymore. Should add drv_vram_usage_va to do virt data exchange
> in vram_usagebyfirmware_v2_2 case. And refine some code style
> checks in pre add vram reservation logic patch
> 
> Signed-off-by: Tong Liu01 <Tong.Liu01@amd.com>
> ---
>  .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 16 ++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  7 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h       |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 59 +++++++++++++------
>  4 files changed, 54 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> index 9b97fa39d47a..e40df72c138a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> @@ -104,7 +104,7 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev)
>  static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>  	struct vram_usagebyfirmware_v2_1 *fw_usage, int *usage_bytes)
>  {
> -	uint32_t start_addr, fw_size, drv_size;
> +	u32 start_addr, fw_size, drv_size;
>  
>  	start_addr = le32_to_cpu(fw_usage->start_address_in_kb);
>  	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
> @@ -116,7 +116,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>  			  drv_size);
>  
>  	if ((start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
> -		(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
> +		(u32)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
>  		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
>  		/* Firmware request VRAM reservation for SR-IOV */
>  		adev->mman.fw_vram_usage_start_offset = (start_addr &
> @@ -133,7 +133,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>  static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
>  		struct vram_usagebyfirmware_v2_2 *fw_usage, int *usage_bytes)
>  {
> -	uint32_t fw_start_addr, fw_size, drv_start_addr, drv_size;
> +	u32 fw_start_addr, fw_size, drv_start_addr, drv_size;
>  
>  	fw_start_addr = le32_to_cpu(fw_usage->fw_region_start_address_in_kb);
>  	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
> @@ -147,14 +147,16 @@ static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
>  			  drv_start_addr,
>  			  drv_size);
>  
> -	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
> +	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
> +		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
>  		/* Firmware request VRAM reservation for SR-IOV */
>  		adev->mman.fw_vram_usage_start_offset = (fw_start_addr &
>  			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
>  		adev->mman.fw_vram_usage_size = fw_size << 10;
>  	}
>  
> -	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
> +	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
> +		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
>  		/* driver request VRAM reservation for SR-IOV */
>  		adev->mman.drv_vram_usage_start_offset = (drv_start_addr &
>  			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
> @@ -172,8 +174,8 @@ int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev)
>  						vram_usagebyfirmware);
>  	struct vram_usagebyfirmware_v2_1 *fw_usage_v2_1;
>  	struct vram_usagebyfirmware_v2_2 *fw_usage_v2_2;
> -	uint16_t data_offset;
> -	uint8_t frev, crev;
> +	u16 data_offset;
> +	u8 frev, crev;
>  	int usage_bytes = 0;
>  
>  	if (amdgpu_atom_parse_data_header(ctx, index, NULL, &frev, &crev, &data_offset)) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 52f2282411cb..dd8b6a11db9a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1570,7 +1570,7 @@ static void amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device *adev)
>  {
>  	amdgpu_bo_free_kernel(&adev->mman.drv_vram_usage_reserved_bo,
>  						  NULL,
> -						  NULL);
> +						  &adev->mman.drv_vram_usage_va);
>  }
>  
>  /**
> @@ -1608,8 +1608,9 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
>   */
>  static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
>  {
> -	uint64_t vram_size = adev->gmc.visible_vram_size;
> +	u64 vram_size = adev->gmc.visible_vram_size;
>  
> +	adev->mman.drv_vram_usage_va = NULL;
>  	adev->mman.drv_vram_usage_reserved_bo = NULL;
>  
>  	if (adev->mman.drv_vram_usage_size == 0 ||
> @@ -1621,7 +1622,7 @@ static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
>  					  adev->mman.drv_vram_usage_size,
>  					  AMDGPU_GEM_DOMAIN_VRAM,
>  					  &adev->mman.drv_vram_usage_reserved_bo,
> -					  NULL);
> +					  &adev->mman.drv_vram_usage_va);
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index b391c8d076ff..b4d8ba2789f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -90,6 +90,7 @@ struct amdgpu_mman {
>  	u64		drv_vram_usage_start_offset;
>  	u64		drv_vram_usage_size;
>  	struct amdgpu_bo	*drv_vram_usage_reserved_bo;
> +	void		*drv_vram_usage_va;
>  
>  	/* PAGE_SIZE'd BO for process memory r/w over SDMA. */
>  	struct amdgpu_bo	*sdma_access_bo;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index a226a6c48fb7..e80033e24d48 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -428,11 +428,18 @@ static void amdgpu_virt_add_bad_page(struct amdgpu_device *adev,
>  	struct eeprom_table_record bp;
>  	uint64_t retired_page;
>  	uint32_t bp_idx, bp_cnt;
> +	void *vram_usage_va = NULL;
> +
> +	if (adev->mman.fw_vram_usage_va != NULL) {
> +		vram_usage_va = adev->mman.fw_vram_usage_va;
> +	} else {
> +		vram_usage_va = adev->mman.drv_vram_usage_va;
> +	}

Comparison to NULL, please use if (!adev->mman.fw_vram_usage_va), and
also to drop the braces for single line statements.

>  
>  	if (bp_block_size) {
>  		bp_cnt = bp_block_size / sizeof(uint64_t);
>  		for (bp_idx = 0; bp_idx < bp_cnt; bp_idx++) {
> -			retired_page = *(uint64_t *)(adev->mman.fw_vram_usage_va +
> +			retired_page = *(uint64_t *)(vram_usage_va +
>  					bp_block_offset + bp_idx * sizeof(uint64_t));
>  			bp.retired_page = retired_page;
>  
> @@ -643,7 +650,11 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
>  	adev->virt.fw_reserve.p_vf2pf = NULL;
>  	adev->virt.vf2pf_update_interval_ms = 0;
>  
> -	if (adev->mman.fw_vram_usage_va != NULL) {
> +	if (adev->mman.fw_vram_usage_va != NULL &&
> +		adev->mman.drv_vram_usage_va != NULL) {
> +		DRM_WARN("Currently fw_vram and drv_vram should not have values at the same time!");
> +	} else if (adev->mman.fw_vram_usage_va != NULL ||
> +		adev->mman.drv_vram_usage_va != NULL) {
>  		/* go through this logic in ip_init and reset to init workqueue*/
>  		amdgpu_virt_exchange_data(adev);

The alignment here is off. Most editors would align for you automatically.
If you're using Emacs, just press the TAB key anywhere on the line to align
according to mode. (Pressing TAB continuously doesn't do anything, if the line
is already aligned.)

The alignment should look as follows:

	if (adev->mman.fw_vram_usage_va != NULL &&
	    adev->mman.drv_vram_usage_va != NULL) {
		DRM_WARN("Currently fw_vram and drv_vram should not have values at the same time!");
	} else if (adev->mman.fw_vram_usage_va != NULL ||
		   adev->mman.drv_vram_usage_va != NULL) {

Note the second conditional inside both "if"-statements' check that it is aligned to
the one on the previous line.

Also please use !x instead of x != NULL.

>  
> @@ -666,32 +677,42 @@ void amdgpu_virt_exchange_data(struct amdgpu_device *adev)
>  	uint32_t bp_block_size = 0;
>  	struct amd_sriov_msg_pf2vf_info *pf2vf_v2 = NULL;
>  
> -	if (adev->mman.fw_vram_usage_va != NULL) {
> -
> -		adev->virt.fw_reserve.p_pf2vf =
> -			(struct amd_sriov_msg_pf2vf_info_header *)
> -			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> -		adev->virt.fw_reserve.p_vf2pf =
> -			(struct amd_sriov_msg_vf2pf_info_header *)
> -			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +	if (adev->mman.fw_vram_usage_va != NULL ||
> +		adev->mman.drv_vram_usage_va != NULL) {
> +
> +		if (adev->mman.fw_vram_usage_va != NULL) {

Alignment inside the if () doesn't follow the contained statement, but
follows the parenthesis alignment of the previous line of the if ():

	if (!adev->mman.fw_vram_usage_va ||
	    !adev->mman.drv_vram_usage_va) {

Also please use !x as shown. And you shouldn't have an empty line
after an if () { before the first line of the containing statement.

The rest looks good.

Regards,
Luben

> +			adev->virt.fw_reserve.p_pf2vf =
> +				(struct amd_sriov_msg_pf2vf_info_header *)
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +			adev->virt.fw_reserve.p_vf2pf =
> +				(struct amd_sriov_msg_vf2pf_info_header *)
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +		} else if (adev->mman.drv_vram_usage_va != NULL) {
> +			adev->virt.fw_reserve.p_pf2vf =
> +				(struct amd_sriov_msg_pf2vf_info_header *)
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +			adev->virt.fw_reserve.p_vf2pf =
> +				(struct amd_sriov_msg_vf2pf_info_header *)
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +		}
>  
>  		amdgpu_virt_read_pf2vf_data(adev);
>  		amdgpu_virt_write_vf2pf_data(adev);
>  
>  		/* bad page handling for version 2 */
>  		if (adev->virt.fw_reserve.p_pf2vf->version == 2) {
> -				pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
> +			pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
>  
> -				bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
> -						((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
> -				bp_block_size = pf2vf_v2->bp_block_size;
> +			bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
> +				((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
> +			bp_block_size = pf2vf_v2->bp_block_size;
>  
> -				if (bp_block_size && !adev->virt.ras_init_done)
> -					amdgpu_virt_init_ras_err_handler_data(adev);
> +			if (bp_block_size && !adev->virt.ras_init_done)
> +				amdgpu_virt_init_ras_err_handler_data(adev);
>  
> -				if (adev->virt.ras_init_done)
> -					amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
> -			}
> +			if (adev->virt.ras_init_done)
> +				amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
> +		}
>  	}
>  }
>  

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange
  2022-11-22  5:52 Tong Liu01
@ 2022-11-22  5:57 ` Luben Tuikov
  0 siblings, 0 replies; 7+ messages in thread
From: Luben Tuikov @ 2022-11-22  5:57 UTC (permalink / raw)
  To: Tong Liu01, amd-gfx
  Cc: Jack Xiao, Feifei Xu, horace.chen, Kevin Wang, Deucher Alexander,
	yifan.zha, Evan Quan, Christian König, Monk Liu,
	Hawking Zhang

On 2022-11-22 00:52, Tong Liu01 wrote:
> For vram_usagebyfirmware_v2_2, fw_vram_reserve is not used. So
> fw_vram_usage_va is NULL, and cannot do virt data exchange
> anymore. Should add drv_vram_usage_va to do virt data exchange
> in vram_usagebyfirmware_v2_2 case. And refine some code style
> checks in pre add vram reservation logic patch
> 
> Signed-off-by: Tong Liu01 <Tong.Liu01@amd.com>
> ---
>  .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 16 +++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  9 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h       |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 54 ++++++++++++-------
>  4 files changed, 50 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> index 9b97fa39d47a..e40df72c138a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
> @@ -104,7 +104,7 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev)
>  static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>  	struct vram_usagebyfirmware_v2_1 *fw_usage, int *usage_bytes)
>  {
> -	uint32_t start_addr, fw_size, drv_size;
> +	u32 start_addr, fw_size, drv_size;
>  
>  	start_addr = le32_to_cpu(fw_usage->start_address_in_kb);
>  	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
> @@ -116,7 +116,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>  			  drv_size);
>  
>  	if ((start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
> -		(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
> +		(u32)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
>  		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
>  		/* Firmware request VRAM reservation for SR-IOV */
>  		adev->mman.fw_vram_usage_start_offset = (start_addr &
> @@ -133,7 +133,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
>  static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
>  		struct vram_usagebyfirmware_v2_2 *fw_usage, int *usage_bytes)
>  {
> -	uint32_t fw_start_addr, fw_size, drv_start_addr, drv_size;
> +	u32 fw_start_addr, fw_size, drv_start_addr, drv_size;
>  
>  	fw_start_addr = le32_to_cpu(fw_usage->fw_region_start_address_in_kb);
>  	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
> @@ -147,14 +147,16 @@ static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
>  			  drv_start_addr,
>  			  drv_size);
>  
> -	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
> +	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
> +		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
>  		/* Firmware request VRAM reservation for SR-IOV */
>  		adev->mman.fw_vram_usage_start_offset = (fw_start_addr &
>  			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
>  		adev->mman.fw_vram_usage_size = fw_size << 10;
>  	}
>  
> -	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
> +	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
> +		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
>  		/* driver request VRAM reservation for SR-IOV */
>  		adev->mman.drv_vram_usage_start_offset = (drv_start_addr &
>  			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
> @@ -172,8 +174,8 @@ int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev)
>  						vram_usagebyfirmware);
>  	struct vram_usagebyfirmware_v2_1 *fw_usage_v2_1;
>  	struct vram_usagebyfirmware_v2_2 *fw_usage_v2_2;
> -	uint16_t data_offset;
> -	uint8_t frev, crev;
> +	u16 data_offset;
> +	u8 frev, crev;
>  	int usage_bytes = 0;
>  
>  	if (amdgpu_atom_parse_data_header(ctx, index, NULL, &frev, &crev, &data_offset)) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 52f2282411cb..5922f94241a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1569,8 +1569,8 @@ static void amdgpu_ttm_fw_reserve_vram_fini(struct amdgpu_device *adev)
>  static void amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device *adev)
>  {
>  	amdgpu_bo_free_kernel(&adev->mman.drv_vram_usage_reserved_bo,
> -						  NULL,
> -						  NULL);
> +						 NULL,
> +						 &adev->mman.drv_vram_usage_va);
>  }

This should be aligned according to C mode, under the &.
(Use a good editor with C mode and it'll do it for you. Emacs, perhaps... :-) )

With this fixed, this patch is
Acked-by: Luben Tuikov <luben.tuikov@amd.com>

Regards,
Luben

>  
>  /**
> @@ -1608,8 +1608,9 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
>   */
>  static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
>  {
> -	uint64_t vram_size = adev->gmc.visible_vram_size;
> +	u64 vram_size = adev->gmc.visible_vram_size;
>  
> +	adev->mman.drv_vram_usage_va = NULL;
>  	adev->mman.drv_vram_usage_reserved_bo = NULL;
>  
>  	if (adev->mman.drv_vram_usage_size == 0 ||
> @@ -1621,7 +1622,7 @@ static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
>  					  adev->mman.drv_vram_usage_size,
>  					  AMDGPU_GEM_DOMAIN_VRAM,
>  					  &adev->mman.drv_vram_usage_reserved_bo,
> -					  NULL);
> +					  &adev->mman.drv_vram_usage_va);
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index b391c8d076ff..b4d8ba2789f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -90,6 +90,7 @@ struct amdgpu_mman {
>  	u64		drv_vram_usage_start_offset;
>  	u64		drv_vram_usage_size;
>  	struct amdgpu_bo	*drv_vram_usage_reserved_bo;
> +	void		*drv_vram_usage_va;
>  
>  	/* PAGE_SIZE'd BO for process memory r/w over SDMA. */
>  	struct amdgpu_bo	*sdma_access_bo;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index a226a6c48fb7..15544f262ec1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -428,11 +428,17 @@ static void amdgpu_virt_add_bad_page(struct amdgpu_device *adev,
>  	struct eeprom_table_record bp;
>  	uint64_t retired_page;
>  	uint32_t bp_idx, bp_cnt;
> +	void *vram_usage_va = NULL;
> +
> +	if (adev->mman.fw_vram_usage_va)
> +		vram_usage_va = adev->mman.fw_vram_usage_va;
> +	else
> +		vram_usage_va = adev->mman.drv_vram_usage_va;
>  
>  	if (bp_block_size) {
>  		bp_cnt = bp_block_size / sizeof(uint64_t);
>  		for (bp_idx = 0; bp_idx < bp_cnt; bp_idx++) {
> -			retired_page = *(uint64_t *)(adev->mman.fw_vram_usage_va +
> +			retired_page = *(uint64_t *)(vram_usage_va +
>  					bp_block_offset + bp_idx * sizeof(uint64_t));
>  			bp.retired_page = retired_page;
>  
> @@ -643,7 +649,9 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
>  	adev->virt.fw_reserve.p_vf2pf = NULL;
>  	adev->virt.vf2pf_update_interval_ms = 0;
>  
> -	if (adev->mman.fw_vram_usage_va != NULL) {
> +	if (adev->mman.fw_vram_usage_va && adev->mman.drv_vram_usage_va) {
> +		DRM_WARN("Currently fw_vram and drv_vram should not have values at the same time!");
> +	} else if (adev->mman.fw_vram_usage_va || adev->mman.drv_vram_usage_va) {
>  		/* go through this logic in ip_init and reset to init workqueue*/
>  		amdgpu_virt_exchange_data(adev);
>  
> @@ -666,32 +674,40 @@ void amdgpu_virt_exchange_data(struct amdgpu_device *adev)
>  	uint32_t bp_block_size = 0;
>  	struct amd_sriov_msg_pf2vf_info *pf2vf_v2 = NULL;
>  
> -	if (adev->mman.fw_vram_usage_va != NULL) {
> -
> -		adev->virt.fw_reserve.p_pf2vf =
> -			(struct amd_sriov_msg_pf2vf_info_header *)
> -			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> -		adev->virt.fw_reserve.p_vf2pf =
> -			(struct amd_sriov_msg_vf2pf_info_header *)
> -			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +	if (adev->mman.fw_vram_usage_va || adev->mman.drv_vram_usage_va) {
> +		if (adev->mman.fw_vram_usage_va) {
> +			adev->virt.fw_reserve.p_pf2vf =
> +				(struct amd_sriov_msg_pf2vf_info_header *)
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +			adev->virt.fw_reserve.p_vf2pf =
> +				(struct amd_sriov_msg_vf2pf_info_header *)
> +				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +		} else if (adev->mman.drv_vram_usage_va) {
> +			adev->virt.fw_reserve.p_pf2vf =
> +				(struct amd_sriov_msg_pf2vf_info_header *)
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
> +			adev->virt.fw_reserve.p_vf2pf =
> +				(struct amd_sriov_msg_vf2pf_info_header *)
> +				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
> +		}
>  
>  		amdgpu_virt_read_pf2vf_data(adev);
>  		amdgpu_virt_write_vf2pf_data(adev);
>  
>  		/* bad page handling for version 2 */
>  		if (adev->virt.fw_reserve.p_pf2vf->version == 2) {
> -				pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
> +			pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
>  
> -				bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
> -						((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
> -				bp_block_size = pf2vf_v2->bp_block_size;
> +			bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
> +				((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
> +			bp_block_size = pf2vf_v2->bp_block_size;
>  
> -				if (bp_block_size && !adev->virt.ras_init_done)
> -					amdgpu_virt_init_ras_err_handler_data(adev);
> +			if (bp_block_size && !adev->virt.ras_init_done)
> +				amdgpu_virt_init_ras_err_handler_data(adev);
>  
> -				if (adev->virt.ras_init_done)
> -					amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
> -			}
> +			if (adev->virt.ras_init_done)
> +				amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
> +		}
>  	}
>  }
>  


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange
@ 2022-11-22  5:52 Tong Liu01
  2022-11-22  5:57 ` Luben Tuikov
  0 siblings, 1 reply; 7+ messages in thread
From: Tong Liu01 @ 2022-11-22  5:52 UTC (permalink / raw)
  To: amd-gfx
  Cc: Jack Xiao, Feifei Xu, horace.chen, Kevin Wang, Tong Liu01,
	Tuikov Luben, Deucher Alexander, yifan.zha, Evan Quan,
	Christian König, Monk Liu, Hawking Zhang

For vram_usagebyfirmware_v2_2, fw_vram_reserve is not used. So
fw_vram_usage_va is NULL, and cannot do virt data exchange
anymore. Should add drv_vram_usage_va to do virt data exchange
in vram_usagebyfirmware_v2_2 case. And refine some code style
checks in pre add vram reservation logic patch

Signed-off-by: Tong Liu01 <Tong.Liu01@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c  | 16 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       |  9 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h       |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 54 ++++++++++++-------
 4 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
index 9b97fa39d47a..e40df72c138a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atomfirmware.c
@@ -104,7 +104,7 @@ void amdgpu_atomfirmware_scratch_regs_init(struct amdgpu_device *adev)
 static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
 	struct vram_usagebyfirmware_v2_1 *fw_usage, int *usage_bytes)
 {
-	uint32_t start_addr, fw_size, drv_size;
+	u32 start_addr, fw_size, drv_size;
 
 	start_addr = le32_to_cpu(fw_usage->start_address_in_kb);
 	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
@@ -116,7 +116,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
 			  drv_size);
 
 	if ((start_addr & ATOM_VRAM_OPERATION_FLAGS_MASK) ==
-		(uint32_t)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
+		(u32)(ATOM_VRAM_BLOCK_SRIOV_MSG_SHARE_RESERVATION <<
 		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) {
 		/* Firmware request VRAM reservation for SR-IOV */
 		adev->mman.fw_vram_usage_start_offset = (start_addr &
@@ -133,7 +133,7 @@ static int amdgpu_atomfirmware_allocate_fb_v2_1(struct amdgpu_device *adev,
 static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
 		struct vram_usagebyfirmware_v2_2 *fw_usage, int *usage_bytes)
 {
-	uint32_t fw_start_addr, fw_size, drv_start_addr, drv_size;
+	u32 fw_start_addr, fw_size, drv_start_addr, drv_size;
 
 	fw_start_addr = le32_to_cpu(fw_usage->fw_region_start_address_in_kb);
 	fw_size = le16_to_cpu(fw_usage->used_by_firmware_in_kb);
@@ -147,14 +147,16 @@ static int amdgpu_atomfirmware_allocate_fb_v2_2(struct amdgpu_device *adev,
 			  drv_start_addr,
 			  drv_size);
 
-	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
+	if ((fw_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
+		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
 		/* Firmware request VRAM reservation for SR-IOV */
 		adev->mman.fw_vram_usage_start_offset = (fw_start_addr &
 			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
 		adev->mman.fw_vram_usage_size = fw_size << 10;
 	}
 
-	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION << 30)) == 0) {
+	if ((drv_start_addr & (ATOM_VRAM_BLOCK_NEEDS_NO_RESERVATION <<
+		ATOM_VRAM_OPERATION_FLAGS_SHIFT)) == 0) {
 		/* driver request VRAM reservation for SR-IOV */
 		adev->mman.drv_vram_usage_start_offset = (drv_start_addr &
 			(~ATOM_VRAM_OPERATION_FLAGS_MASK)) << 10;
@@ -172,8 +174,8 @@ int amdgpu_atomfirmware_allocate_fb_scratch(struct amdgpu_device *adev)
 						vram_usagebyfirmware);
 	struct vram_usagebyfirmware_v2_1 *fw_usage_v2_1;
 	struct vram_usagebyfirmware_v2_2 *fw_usage_v2_2;
-	uint16_t data_offset;
-	uint8_t frev, crev;
+	u16 data_offset;
+	u8 frev, crev;
 	int usage_bytes = 0;
 
 	if (amdgpu_atom_parse_data_header(ctx, index, NULL, &frev, &crev, &data_offset)) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 52f2282411cb..5922f94241a3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1569,8 +1569,8 @@ static void amdgpu_ttm_fw_reserve_vram_fini(struct amdgpu_device *adev)
 static void amdgpu_ttm_drv_reserve_vram_fini(struct amdgpu_device *adev)
 {
 	amdgpu_bo_free_kernel(&adev->mman.drv_vram_usage_reserved_bo,
-						  NULL,
-						  NULL);
+						 NULL,
+						 &adev->mman.drv_vram_usage_va);
 }
 
 /**
@@ -1608,8 +1608,9 @@ static int amdgpu_ttm_fw_reserve_vram_init(struct amdgpu_device *adev)
  */
 static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
 {
-	uint64_t vram_size = adev->gmc.visible_vram_size;
+	u64 vram_size = adev->gmc.visible_vram_size;
 
+	adev->mman.drv_vram_usage_va = NULL;
 	adev->mman.drv_vram_usage_reserved_bo = NULL;
 
 	if (adev->mman.drv_vram_usage_size == 0 ||
@@ -1621,7 +1622,7 @@ static int amdgpu_ttm_drv_reserve_vram_init(struct amdgpu_device *adev)
 					  adev->mman.drv_vram_usage_size,
 					  AMDGPU_GEM_DOMAIN_VRAM,
 					  &adev->mman.drv_vram_usage_reserved_bo,
-					  NULL);
+					  &adev->mman.drv_vram_usage_va);
 }
 
 /*
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index b391c8d076ff..b4d8ba2789f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -90,6 +90,7 @@ struct amdgpu_mman {
 	u64		drv_vram_usage_start_offset;
 	u64		drv_vram_usage_size;
 	struct amdgpu_bo	*drv_vram_usage_reserved_bo;
+	void		*drv_vram_usage_va;
 
 	/* PAGE_SIZE'd BO for process memory r/w over SDMA. */
 	struct amdgpu_bo	*sdma_access_bo;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index a226a6c48fb7..15544f262ec1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -428,11 +428,17 @@ static void amdgpu_virt_add_bad_page(struct amdgpu_device *adev,
 	struct eeprom_table_record bp;
 	uint64_t retired_page;
 	uint32_t bp_idx, bp_cnt;
+	void *vram_usage_va = NULL;
+
+	if (adev->mman.fw_vram_usage_va)
+		vram_usage_va = adev->mman.fw_vram_usage_va;
+	else
+		vram_usage_va = adev->mman.drv_vram_usage_va;
 
 	if (bp_block_size) {
 		bp_cnt = bp_block_size / sizeof(uint64_t);
 		for (bp_idx = 0; bp_idx < bp_cnt; bp_idx++) {
-			retired_page = *(uint64_t *)(adev->mman.fw_vram_usage_va +
+			retired_page = *(uint64_t *)(vram_usage_va +
 					bp_block_offset + bp_idx * sizeof(uint64_t));
 			bp.retired_page = retired_page;
 
@@ -643,7 +649,9 @@ void amdgpu_virt_init_data_exchange(struct amdgpu_device *adev)
 	adev->virt.fw_reserve.p_vf2pf = NULL;
 	adev->virt.vf2pf_update_interval_ms = 0;
 
-	if (adev->mman.fw_vram_usage_va != NULL) {
+	if (adev->mman.fw_vram_usage_va && adev->mman.drv_vram_usage_va) {
+		DRM_WARN("Currently fw_vram and drv_vram should not have values at the same time!");
+	} else if (adev->mman.fw_vram_usage_va || adev->mman.drv_vram_usage_va) {
 		/* go through this logic in ip_init and reset to init workqueue*/
 		amdgpu_virt_exchange_data(adev);
 
@@ -666,32 +674,40 @@ void amdgpu_virt_exchange_data(struct amdgpu_device *adev)
 	uint32_t bp_block_size = 0;
 	struct amd_sriov_msg_pf2vf_info *pf2vf_v2 = NULL;
 
-	if (adev->mman.fw_vram_usage_va != NULL) {
-
-		adev->virt.fw_reserve.p_pf2vf =
-			(struct amd_sriov_msg_pf2vf_info_header *)
-			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
-		adev->virt.fw_reserve.p_vf2pf =
-			(struct amd_sriov_msg_vf2pf_info_header *)
-			(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+	if (adev->mman.fw_vram_usage_va || adev->mman.drv_vram_usage_va) {
+		if (adev->mman.fw_vram_usage_va) {
+			adev->virt.fw_reserve.p_pf2vf =
+				(struct amd_sriov_msg_pf2vf_info_header *)
+				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
+			adev->virt.fw_reserve.p_vf2pf =
+				(struct amd_sriov_msg_vf2pf_info_header *)
+				(adev->mman.fw_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+		} else if (adev->mman.drv_vram_usage_va) {
+			adev->virt.fw_reserve.p_pf2vf =
+				(struct amd_sriov_msg_pf2vf_info_header *)
+				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_PF2VF_OFFSET_KB << 10));
+			adev->virt.fw_reserve.p_vf2pf =
+				(struct amd_sriov_msg_vf2pf_info_header *)
+				(adev->mman.drv_vram_usage_va + (AMD_SRIOV_MSG_VF2PF_OFFSET_KB << 10));
+		}
 
 		amdgpu_virt_read_pf2vf_data(adev);
 		amdgpu_virt_write_vf2pf_data(adev);
 
 		/* bad page handling for version 2 */
 		if (adev->virt.fw_reserve.p_pf2vf->version == 2) {
-				pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
+			pf2vf_v2 = (struct amd_sriov_msg_pf2vf_info *)adev->virt.fw_reserve.p_pf2vf;
 
-				bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
-						((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
-				bp_block_size = pf2vf_v2->bp_block_size;
+			bp_block_offset = ((uint64_t)pf2vf_v2->bp_block_offset_low & 0xFFFFFFFF) |
+				((((uint64_t)pf2vf_v2->bp_block_offset_high) << 32) & 0xFFFFFFFF00000000);
+			bp_block_size = pf2vf_v2->bp_block_size;
 
-				if (bp_block_size && !adev->virt.ras_init_done)
-					amdgpu_virt_init_ras_err_handler_data(adev);
+			if (bp_block_size && !adev->virt.ras_init_done)
+				amdgpu_virt_init_ras_err_handler_data(adev);
 
-				if (adev->virt.ras_init_done)
-					amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
-			}
+			if (adev->virt.ras_init_done)
+				amdgpu_virt_add_bad_page(adev, bp_block_offset, bp_block_size);
+		}
 	}
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-11-22  5:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18  2:56 [PATCH] drm/amdgpu: add drv_vram_usage_va for virt data exchange Tong Liu01
2022-11-21  8:14 ` Liu01, Tong (Esther)
2022-11-21  8:24 ` Christian König
2022-11-21 16:26   ` Luben Tuikov
2022-11-21 16:42 ` Luben Tuikov
2022-11-22  5:52 Tong Liu01
2022-11-22  5:57 ` Luben Tuikov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.