[PATCH 0/6] enable switching to new gpu index for hibernate on SRIOV.

[PATCH 0/6] enable switching to new gpu index for hibernate on SRIOV.

[Samuel Zhang]

On SRIOV and VM environment, customer may need to switch to new vGPU indexes
after hibernate and then resume the VM. For GPUs with XGMI, `vram_start` will 
change in this case, the VRAM aperture gpu address of VRAM BOs will also change.
These gpu addresses need to be updated when resume. But these addresses are all
over the KMD codebase, updating each of them is error-prone and not acceptable. 

The solution is to use pdb0 page table to cover both vram and gart memory and
use pdb0 virtual gpu address instead. When gpu indexes change, the virtual gpu 
address won't change.

For psp and smu, pdb0's gpu address does not work, so the original gpu address
is used instead. They need to be updated when resume with changed vGPUs.

The last 2 patches fix the issues we hit when testing this feature.

Samuel Zhang (6):
  drm/amdgpu: update XGMI physical node id and GMC configs on resume
  drm/amdgpu: update cached GPU addresses for PSP and ucode
  drm/amdgpu: update cached GPU addresses for SMU
  drm/amdgpu: enable pdb0 for hibernation on SRIOV
  drm/amdgpu: fix sdma ring test fail when resume from hibernation
  drm/amdgpu: fix fence fallback timer expired error

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 25 ++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 45 +++++++++++++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c    |  7 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c    | 24 ++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c  |  3 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h   |  2 +
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 39 +++++++++++++------
 drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
 drivers/gpu/drm/amd/amdgpu/vega20_ih.c     | 18 ++++++++-
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c  | 30 +++++++++++++++
 14 files changed, 199 insertions(+), 36 deletions(-)

-- 
2.43.5

[PATCH 1/6] drm/amdgpu: update XGMI physical node id and GMC configs on resume

[Samuel Zhang]

For virtual machine with vGPUs in SRIOV single device mode and XGMI
is enabled, XGMI physical node ids may change when waking up from
hiberation with different vGPU devices. So update XGMI physical node
ids on resume.

Update GPU memory controller configuration on resume if XGMI physical
node ids are changed.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
Change-Id: I0bcac2d46fdeed66c9cf7e6a378134769c95df61
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 25 ++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h   |  2 ++
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      |  8 +++++++
 4 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index f212ce3f5d34..12f115602ab2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5098,6 +5098,28 @@ int amdgpu_device_suspend(struct drm_device *dev, bool notify_clients)
 	return 0;
 }
 
+static int amdgpu_device_update_xgmi_nodes(struct amdgpu_device *adev)
+{
+	int r = 0;
+	/* Get xgmi info again for sriov to detect device changes */
+	if (amdgpu_sriov_vf(adev) &&
+	    !(adev->flags & AMD_IS_APU) &&
+	    adev->gmc.xgmi.supported &&
+	    !adev->gmc.xgmi.connected_to_cpu) {
+		adev->gmc.xgmi.prev_physical_node_id = adev->gmc.xgmi.physical_node_id;
+		r = adev->gfxhub.funcs->get_xgmi_info(adev);
+		if (r)
+			return r;
+
+		adev->gmc.xgmi.physical_node_id_changed =
+			adev->gmc.xgmi.physical_node_id != adev->gmc.xgmi.prev_physical_node_id;
+		dev_info(adev->dev, "xgmi node, old id %d, new id %d\n",
+			adev->gmc.xgmi.prev_physical_node_id, adev->gmc.xgmi.physical_node_id);
+	}
+	return 0;
+}
+
+
 /**
  * amdgpu_device_resume - initiate device resume
  *
@@ -5117,6 +5139,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool notify_clients)
 		r = amdgpu_virt_request_full_gpu(adev, true);
 		if (r)
 			return r;
+		r = amdgpu_device_update_xgmi_nodes(adev);
+		if (r)
+			return r;
 	}
 
 	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index ecb74ccf1d90..5b60d714e089 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -1288,6 +1288,8 @@ int amdgpu_gmc_get_nps_memranges(struct amdgpu_device *adev,
 
 	refresh = (adev->init_lvl->level != AMDGPU_INIT_LEVEL_MINIMAL_XGMI) &&
 		  (adev->gmc.reset_flags & AMDGPU_GMC_INIT_RESET_NPS);
+	if (adev->gmc.xgmi.physical_node_id_changed)
+		refresh = true;
 	ret = amdgpu_discovery_get_nps_info(adev, &nps_type, &ranges,
 					    &range_cnt, refresh);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
index 32dabba4062f..3d5f01a1b657 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
@@ -89,6 +89,8 @@ struct amdgpu_xgmi {
 	u64 node_segment_size;
 	/* physical node (0-3) */
 	unsigned physical_node_id;
+	unsigned prev_physical_node_id;
+	bool physical_node_id_changed;
 	/* number of nodes (0-4) */
 	unsigned num_physical_nodes;
 	/* gpu list in the same hive */
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 8d3560314e5b..7c7a9fe6be6d 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -2515,6 +2515,14 @@ static int gmc_v9_0_resume(struct amdgpu_ip_block *ip_block)
 	struct amdgpu_device *adev = ip_block->adev;
 	int r;
 
+	/* Update MC configuration if XGMI physical node id has changed for dGPU. */
+	if (adev->gmc.xgmi.physical_node_id_changed) {
+		r = gmc_v9_0_mc_init(adev);
+		if (r)
+			return r;
+		gmc_v9_0_init_sw_mem_ranges(adev, adev->gmc.mem_partitions);
+	}
+
 	/* If a reset is done for NPS mode switch, read the memory range
 	 * information again.
 	 */
-- 
2.43.5

Re: [PATCH 1/6] drm/amdgpu: update XGMI physical node id and GMC configs on resume

[Christian König]

Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> For virtual machine with vGPUs in SRIOV single device mode and XGMI
> is enabled, XGMI physical node ids may change when waking up from
> hiberation with different vGPU devices. So update XGMI physical node
> ids on resume.
>
> Update GPU memory controller configuration on resume if XGMI physical
> node ids are changed.
>
> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> Change-Id: I0bcac2d46fdeed66c9cf7e6a378134769c95df61
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 25 ++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    |  2 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h   |  2 ++
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      |  8 +++++++
>  4 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index f212ce3f5d34..12f115602ab2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5098,6 +5098,28 @@ int amdgpu_device_suspend(struct drm_device *dev, bool notify_clients)
>  	return 0;
>  }
>  
> +static int amdgpu_device_update_xgmi_nodes(struct amdgpu_device *adev)
> +{
> +	int r = 0;
> +	/* Get xgmi info again for sriov to detect device changes */

General coding style: Don't initialize return values if not necessary and add an empty line between declaration and code.

> +	if (amdgpu_sriov_vf(adev) &&
> +	    !(adev->flags & AMD_IS_APU) &&
> +	    adev->gmc.xgmi.supported &&
> +	    !adev->gmc.xgmi.connected_to_cpu) {
> +		adev->gmc.xgmi.prev_physical_node_id = adev->gmc.xgmi.physical_node_id;
> +		r = adev->gfxhub.funcs->get_xgmi_info(adev);
> +		if (r)
> +			return r;
> +
> +		adev->gmc.xgmi.physical_node_id_changed =
> +			adev->gmc.xgmi.physical_node_id != adev->gmc.xgmi.prev_physical_node_id;
> +		dev_info(adev->dev, "xgmi node, old id %d, new id %d\n",
> +			adev->gmc.xgmi.prev_physical_node_id, adev->gmc.xgmi.physical_node_id);
> +	}
> +	return 0;
> +}
> +
> +
>  /**
>   * amdgpu_device_resume - initiate device resume
>   *
> @@ -5117,6 +5139,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool notify_clients)
>  		r = amdgpu_virt_request_full_gpu(adev, true);
>  		if (r)
>  			return r;
> +		r = amdgpu_device_update_xgmi_nodes(adev);
> +		if (r)
> +			return r;
>  	}
>  
>  	if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index ecb74ccf1d90..5b60d714e089 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -1288,6 +1288,8 @@ int amdgpu_gmc_get_nps_memranges(struct amdgpu_device *adev,
>  
>  	refresh = (adev->init_lvl->level != AMDGPU_INIT_LEVEL_MINIMAL_XGMI) &&
>  		  (adev->gmc.reset_flags & AMDGPU_GMC_INIT_RESET_NPS);
> +	if (adev->gmc.xgmi.physical_node_id_changed)
> +		refresh = true;
>  	ret = amdgpu_discovery_get_nps_info(adev, &nps_type, &ranges,
>  					    &range_cnt, refresh);
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
> index 32dabba4062f..3d5f01a1b657 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h
> @@ -89,6 +89,8 @@ struct amdgpu_xgmi {
>  	u64 node_segment_size;
>  	/* physical node (0-3) */
>  	unsigned physical_node_id;
> +	unsigned prev_physical_node_id;
> +	bool physical_node_id_changed;

Please drop that.

>  	/* number of nodes (0-4) */
>  	unsigned num_physical_nodes;
>  	/* gpu list in the same hive */
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 8d3560314e5b..7c7a9fe6be6d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -2515,6 +2515,14 @@ static int gmc_v9_0_resume(struct amdgpu_ip_block *ip_block)
>  	struct amdgpu_device *adev = ip_block->adev;
>  	int r;
>  
> +	/* Update MC configuration if XGMI physical node id has changed for dGPU. */
> +	if (adev->gmc.xgmi.physical_node_id_changed) {
> +		r = gmc_v9_0_mc_init(adev);
> +		if (r)
> +			return r;
> +		gmc_v9_0_init_sw_mem_ranges(adev, adev->gmc.mem_partitions);
> +	}
> +

Just always update that on resume.

Regards,
Christian.

>  	/* If a reset is done for NPS mode switch, read the memory range
>  	 * information again.
>  	 */

[PATCH 2/6] drm/amdgpu: update cached GPU addresses for PSP and ucode

[Samuel Zhang]

2 reasons for this change:
1. when pdb0 is enabled, gpu addr from amdgpu_bo_create_kernel() is based
on pdb0, it is not compatible with PSP and ucode, it need to updated
to use original gpu address.
2. Since original address will change after switching to new GPU
index after hibernation, it need to be updated after resume.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
Change-Id: I5d146f486fee4f225f10f8d551c6666720d7d21b
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c   | 24 +++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c |  3 +++
 2 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index dbc3313fc141..5d607057619b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -871,6 +871,8 @@ static int psp_tmr_init(struct psp_context *psp)
 					      &psp->tmr_bo, &psp->tmr_mc_addr,
 					      pptr);
 	}
+	if (psp->tmr_bo)
+		psp->tmr_mc_addr = amdgpu_bo_gpu_offset(psp->tmr_bo);
 
 	return ret;
 }
@@ -1210,6 +1212,9 @@ static void psp_prep_ta_load_cmd_buf(struct psp_gfx_cmd_resp *cmd,
 	cmd->cmd.cmd_load_ta.app_phy_addr_hi	= upper_32_bits(ta_bin_mc);
 	cmd->cmd.cmd_load_ta.app_len		= context->bin_desc.size_bytes;
 
+	if (context->mem_context.shared_bo)
+		context->mem_context.shared_mc_addr = amdgpu_bo_gpu_offset(context->mem_context.shared_bo);
+
 	cmd->cmd.cmd_load_ta.cmd_buf_phy_addr_lo =
 		lower_32_bits(context->mem_context.shared_mc_addr);
 	cmd->cmd.cmd_load_ta.cmd_buf_phy_addr_hi =
@@ -2336,11 +2341,28 @@ bool amdgpu_psp_tos_reload_needed(struct amdgpu_device *adev)
 	return false;
 }
 
+static void psp_bo_resume(struct amdgpu_device *adev)
+{
+	struct psp_context *psp = &adev->psp;
+
+	if (psp->cmd_buf_bo && psp->cmd_buf_mem) {
+		psp->fw_pri_mc_addr = amdgpu_bo_gpu_offset(psp->fw_pri_bo);
+		psp->fence_buf_mc_addr = amdgpu_bo_gpu_offset(psp->fence_buf_bo);
+		psp->cmd_buf_mc_addr = amdgpu_bo_gpu_offset(psp->cmd_buf_bo);
+	}
+	if (adev->firmware.rbuf && psp->km_ring.ring_mem)
+		psp->km_ring.ring_mem_mc_addr = amdgpu_bo_gpu_offset(adev->firmware.rbuf);
+	memset(psp->fence_buf, 0, PSP_FENCE_BUFFER_SIZE);
+}
+
+
 static int psp_hw_start(struct psp_context *psp)
 {
 	struct amdgpu_device *adev = psp->adev;
 	int ret;
 
+	psp_bo_resume(adev);
+
 	if (!amdgpu_sriov_vf(adev)) {
 		if ((is_psp_fw_valid(psp->kdb)) &&
 		    (psp->funcs->bootloader_load_kdb != NULL)) {
@@ -3980,6 +4002,7 @@ static ssize_t psp_usbc_pd_fw_sysfs_write(struct device *dev,
 	memcpy_toio(fw_pri_cpu_addr, usbc_pd_fw->data, usbc_pd_fw->size);
 
 	mutex_lock(&adev->psp.mutex);
+	fw_pri_mc_addr = amdgpu_bo_gpu_offset(fw_buf_bo);
 	ret = psp_load_usbc_pd_fw(&adev->psp, fw_pri_mc_addr);
 	mutex_unlock(&adev->psp.mutex);
 
@@ -4089,6 +4112,7 @@ static ssize_t amdgpu_psp_vbflash_read(struct file *filp, struct kobject *kobj,
 	memcpy_toio(fw_pri_cpu_addr, adev->psp.vbflash_tmp_buf, adev->psp.vbflash_image_size);
 
 	mutex_lock(&adev->psp.mutex);
+	fw_pri_mc_addr = amdgpu_bo_gpu_offset(fw_buf_bo);
 	ret = psp_update_spirom(&adev->psp, fw_pri_mc_addr);
 	mutex_unlock(&adev->psp.mutex);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
index 3d9e9fdc10b4..8e4d60aa1c34 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
@@ -1152,6 +1152,9 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
 		adev->firmware.max_ucodes = AMDGPU_UCODE_ID_MAXIMUM;
 	}
 
+	if (adev->firmware.fw_buf)
+		adev->firmware.fw_buf_mc = amdgpu_bo_gpu_offset(adev->firmware.fw_buf);
+
 	for (i = 0; i < adev->firmware.max_ucodes; i++) {
 		ucode = &adev->firmware.ucode[i];
 		if (ucode->fw) {
-- 
2.43.5

[PATCH 3/6] drm/amdgpu: update cached GPU addresses for SMU

[Samuel Zhang]

2 reasons for this change:
1. when pdb0 is enabled, gpu addr from amdgpu_bo_create_kernel() is
based on pdb0, it is not compatible with SMU, it need to updated
to use original gpu address.
2. Since original gpu address will change after switching to new GPU
index after hibernation, it need to be updated after resume.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
Change-Id: I5f9586ea156b9d5bb78596bb08681335de109c7b
---
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 30 +++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index f345c233bc47..72cd4374ca4c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -1004,6 +1004,31 @@ static int smu_fini_fb_allocations(struct smu_context *smu)
 	return 0;
 }
 
+static int smu_resume_fb_allocations(struct smu_context *smu)
+{
+	struct smu_table_context *smu_table = &smu->smu_table;
+	struct smu_table *tables = smu_table->tables;
+	struct smu_table *driver_table = &(smu_table->driver_table);
+
+	if (tables[SMU_TABLE_PMSTATUSLOG].bo)
+		tables[SMU_TABLE_PMSTATUSLOG].mc_address =
+			amdgpu_bo_gpu_offset(tables[SMU_TABLE_PMSTATUSLOG].bo);
+
+	if (driver_table->bo)
+		driver_table->mc_address = amdgpu_bo_gpu_offset(driver_table->bo);
+
+	return 0;
+}
+static void smu_resume_dummy_read_table(struct smu_context *smu)
+{
+	struct smu_table_context *smu_table = &smu->smu_table;
+	struct smu_table *dummy_read_1_table = &smu_table->dummy_read_1_table;
+
+	if (dummy_read_1_table->bo)
+		dummy_read_1_table->mc_address = amdgpu_bo_gpu_offset(dummy_read_1_table->bo);
+}
+
+
 /**
  * smu_alloc_memory_pool - allocate memory pool in the system memory
  *
@@ -1780,6 +1805,11 @@ static int smu_start_smc_engine(struct smu_context *smu)
 	struct amdgpu_device *adev = smu->adev;
 	int ret = 0;
 
+	/* Update cached GPU addresses of BOs, which may get staled. */
+	smu_resume_fb_allocations(smu);
+	smu_resume_dummy_read_table(smu);
+
+
 	smu->smc_fw_state = SMU_FW_INIT;
 
 	if (adev->firmware.load_type != AMDGPU_FW_LOAD_PSP) {
-- 
2.43.5

[PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Samuel Zhang]

When switching to new GPU index after hibernation and then resume,
VRAM offset of each VRAM BO will be changed, and the cached gpu
addresses needed to updated.

This is to enable pdb0 and switch to use pdb0-based virtual gpu
address by default in amdgpu_bo_create_reserved(). since the virtual
addresses do not change, this can avoid the need to update all
cached gpu addresses all over the codebase.

Signed-off-by: Emily Deng <Emily.Deng@amd.com>
Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
Change-Id: I2b20b9b94f1e41820a013ce5d05bb3fa96859b21
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 43 +++++++++++++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 29 +++++++++------
 drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
 6 files changed, 82 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
index 5b60d714e089..e706afcb7e95 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
@@ -248,18 +248,25 @@ void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc,
 void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc)
 {
 	u64 hive_vram_start = 0;
-	u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1;
-	mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
-	mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
+	u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes;
+
+	hive_vram_end = ALIGN(hive_vram_end, (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21) - 1;
+
+	if (!mc->vram_start) {
+		mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
+		mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
+		dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
+			mc->mc_vram_size >> 20, mc->vram_start,
+			mc->vram_end, mc->real_vram_size >> 20);
+	}
+
 	mc->gart_start = hive_vram_end + 1;
 	mc->gart_end = mc->gart_start + mc->gart_size - 1;
 	mc->fb_start = hive_vram_start;
 	mc->fb_end = hive_vram_end;
-	dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
-			mc->mc_vram_size >> 20, mc->vram_start,
-			mc->vram_end, mc->real_vram_size >> 20);
-	dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n",
-			mc->gart_size >> 20, mc->gart_start, mc->gart_end);
+
+	dev_info(adev->dev, "FB 0x%016llX - 0x%016llX, GART: %lluM 0x%016llX - 0x%016llX\n",
+			mc->fb_start, mc->fb_end, mc->gart_size >> 20, mc->gart_start, mc->gart_end);
 }
 
 /**
@@ -677,8 +684,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
 				     &job);
 	if (r)
 		goto error_alloc;
-
-	job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
+	job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
+							adev->gmc.pdb0_bo :
+							adev->gart.bo);
 	job->vm_needs_flush = true;
 	job->ibs->ptr[job->ibs->length_dw++] = ring->funcs->nop;
 	amdgpu_ring_pad_ib(ring, &job->ibs[0]);
@@ -1041,8 +1049,9 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
 	 */
 	u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
 	u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
-	u64 vram_addr = adev->vm_manager.vram_base_offset -
+	u64 vram_addr_first = adev->vm_manager.vram_base_offset -
 		adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size;
+	u64 vram_addr = adev->vm_manager.vram_base_offset;
 	u64 vram_end = vram_addr + vram_size;
 	u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
 	int idx;
@@ -1056,11 +1065,19 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
 	flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
 	flags |= AMDGPU_PDE_PTE_FLAG(adev);
 
+	if (adev->gmc.xgmi.connected_to_cpu) {
+		vram_addr = vram_addr_first;
+		vram_end = vram_addr + vram_size;
+	}
+
 	/* The first n PDE0 entries are used as PTE,
 	 * pointing to vram
 	 */
-	for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
-		amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
+	for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
+		amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i,
+			(vram_addr >= vram_addr_first + vram_size) ? (vram_addr - vram_size) : vram_addr,
+			flags);
+	}
 
 	/* The n+1'th PDE0 entry points to a huge
 	 * PTB who has more than 512 entries each
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
index bd7fc123b8f9..758b47240c6f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
@@ -307,6 +307,7 @@ struct amdgpu_gmc {
 	struct amdgpu_bo		*pdb0_bo;
 	/* CPU kmapped address of pdb0*/
 	void				*ptr_pdb0;
+	bool enable_pdb0;
 
 	/* MALL size */
 	u64 mall_size;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index d90e9daf5a50..83a3444c69d9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
 		goto error_unpin;
 	}
 
-	if (gpu_addr)
+	if (gpu_addr) {
 		*gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
+		if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
+			if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
+				*gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
+			}
+		}
+	}
 
 	if (cpu_addr) {
 		r = amdgpu_bo_kmap(*bo_ptr, cpu_addr);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
index cb25f7f0dfc1..5ebb92ac9fd7 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
@@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev,
 		/* In the case squeezing vram into GART aperture, we don't use
 		 * FB aperture and AGP aperture. Disable them.
 		 */
-		if (adev->gmc.pdb0_bo) {
+		if (adev->gmc.pdb0_bo && !amdgpu_sriov_vf(adev)) {
 			WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0);
 			WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF);
 			WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 7c7a9fe6be6d..73ac05b9a1bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -1677,6 +1677,10 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block)
 		adev->gmc.private_aperture_start + (4ULL << 30) - 1;
 	adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF;
 
+	if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
+	    amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4) ||
+	    amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 5, 0))
+	    adev->gmc.enable_pdb0 = amdgpu_sriov_vf(adev);
 	return 0;
 }
 
@@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
 {
 	u64 base = adev->mmhub.funcs->get_fb_location(adev);
 
+	if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
+		adev->gmc.vmid0_page_table_depth = 1;
+		adev->gmc.vmid0_page_table_block_size = 12;
+	} else {
+		adev->gmc.vmid0_page_table_depth = 0;
+		adev->gmc.vmid0_page_table_block_size = 0;
+	}
+
 	amdgpu_gmc_set_agp_default(adev, mc);
 
 	/* add the xgmi offset of the physical node */
@@ -1727,7 +1739,10 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
 		amdgpu_gmc_sysvm_location(adev, mc);
 	} else {
 		amdgpu_gmc_vram_location(adev, mc, base);
-		amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
+		if (!adev->gmc.enable_pdb0)
+			amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
+		else
+			amdgpu_gmc_sysvm_location(adev, mc);
 		if (!amdgpu_sriov_vf(adev) && (amdgpu_agp == 1))
 			amdgpu_gmc_agp_location(adev, mc);
 	}
@@ -1838,14 +1853,6 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
 		return 0;
 	}
 
-	if (adev->gmc.xgmi.connected_to_cpu) {
-		adev->gmc.vmid0_page_table_depth = 1;
-		adev->gmc.vmid0_page_table_block_size = 12;
-	} else {
-		adev->gmc.vmid0_page_table_depth = 0;
-		adev->gmc.vmid0_page_table_block_size = 0;
-	}
-
 	/* Initialize common gart structure */
 	r = amdgpu_gart_init(adev);
 	if (r)
@@ -1864,7 +1871,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
 		if (r)
 			return r;
 
-		if (adev->gmc.xgmi.connected_to_cpu)
+		if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)
 			r = amdgpu_gmc_pdb0_alloc(adev);
 	}
 
@@ -2361,7 +2368,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
 {
 	int r;
 
-	if (adev->gmc.xgmi.connected_to_cpu)
+	if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)
 		amdgpu_gmc_init_pdb0(adev);
 
 	if (adev->gart.bo == NULL) {
diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
index fe0710b55c3a..13b229d07ac4 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
@@ -74,27 +74,47 @@ static void mmhub_v9_4_setup_hubid_vm_pt_regs(struct amdgpu_device *adev, int hu
 static void mmhub_v9_4_init_gart_aperture_regs(struct amdgpu_device *adev,
 					       int hubid)
 {
-	uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gart.bo);
+	uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ? adev->gmc.pdb0_bo : adev->gart.bo);
 
 	mmhub_v9_4_setup_hubid_vm_pt_regs(adev, hubid, 0, pt_base);
 
-	WREG32_SOC15_OFFSET(MMHUB, 0,
+	if (adev->gmc.pdb0_bo) {
+		WREG32_SOC15_OFFSET(MMHUB, 0,
+				    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
+				    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
+				    (u32)(adev->gmc.fb_start >> 12));
+		WREG32_SOC15_OFFSET(MMHUB, 0,
+				    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
+				    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
+				    (u32)(adev->gmc.fb_start >> 44));
+
+		WREG32_SOC15_OFFSET(MMHUB, 0,
+				    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
+				    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
+				    (u32)(adev->gmc.gart_end >> 12));
+		WREG32_SOC15_OFFSET(MMHUB, 0,
+				    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
+				    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
+				    (u32)(adev->gmc.gart_end >> 44));
+	} else {
+		WREG32_SOC15_OFFSET(MMHUB, 0,
 			    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
 			    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
 			    (u32)(adev->gmc.gart_start >> 12));
-	WREG32_SOC15_OFFSET(MMHUB, 0,
+		WREG32_SOC15_OFFSET(MMHUB, 0,
 			    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
 			    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
 			    (u32)(adev->gmc.gart_start >> 44));
 
-	WREG32_SOC15_OFFSET(MMHUB, 0,
+		WREG32_SOC15_OFFSET(MMHUB, 0,
 			    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
 			    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
 			    (u32)(adev->gmc.gart_end >> 12));
-	WREG32_SOC15_OFFSET(MMHUB, 0,
+		WREG32_SOC15_OFFSET(MMHUB, 0,
 			    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
 			    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
 			    (u32)(adev->gmc.gart_end >> 44));
+	}
 }
 
 static void mmhub_v9_4_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,
-- 
2.43.5

Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Christian König]

Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> When switching to new GPU index after hibernation and then resume,
> VRAM offset of each VRAM BO will be changed, and the cached gpu
> addresses needed to updated.
>
> This is to enable pdb0 and switch to use pdb0-based virtual gpu
> address by default in amdgpu_bo_create_reserved(). since the virtual
> addresses do not change, this can avoid the need to update all
> cached gpu addresses all over the codebase.
>
> Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> Change-Id: I2b20b9b94f1e41820a013ce5d05bb3fa96859b21
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 43 +++++++++++++++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 29 +++++++++------
>  drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
>  6 files changed, 82 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 5b60d714e089..e706afcb7e95 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -248,18 +248,25 @@ void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc,
>  void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc)
>  {
>  	u64 hive_vram_start = 0;
> -	u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1;
> -	mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> -	mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> +	u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes;
> +
> +	hive_vram_end = ALIGN(hive_vram_end, (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21) - 1;
> +
> +	if (!mc->vram_start) {
> +		mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> +		mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> +		dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> +			mc->mc_vram_size >> 20, mc->vram_start,
> +			mc->vram_end, mc->real_vram_size >> 20);
> +	}
> +
>  	mc->gart_start = hive_vram_end + 1;
>  	mc->gart_end = mc->gart_start + mc->gart_size - 1;
>  	mc->fb_start = hive_vram_start;
>  	mc->fb_end = hive_vram_end;
> -	dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> -			mc->mc_vram_size >> 20, mc->vram_start,
> -			mc->vram_end, mc->real_vram_size >> 20);
> -	dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n",
> -			mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> +
> +	dev_info(adev->dev, "FB 0x%016llX - 0x%016llX, GART: %lluM 0x%016llX - 0x%016llX\n",
> +			mc->fb_start, mc->fb_end, mc->gart_size >> 20, mc->gart_start, mc->gart_end);
>  }
>  
>  /**
> @@ -677,8 +684,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>  				     &job);
>  	if (r)
>  		goto error_alloc;
> -
> -	job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
> +	job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
> +							adev->gmc.pdb0_bo :
> +							adev->gart.bo);
>  	job->vm_needs_flush = true;
>  	job->ibs->ptr[job->ibs->length_dw++] = ring->funcs->nop;
>  	amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> @@ -1041,8 +1049,9 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>  	 */
>  	u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
>  	u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
> -	u64 vram_addr = adev->vm_manager.vram_base_offset -
> +	u64 vram_addr_first = adev->vm_manager.vram_base_offset -
>  		adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size;
> +	u64 vram_addr = adev->vm_manager.vram_base_offset;
>  	u64 vram_end = vram_addr + vram_size;
>  	u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
>  	int idx;
> @@ -1056,11 +1065,19 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>  	flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
>  	flags |= AMDGPU_PDE_PTE_FLAG(adev);
>  
> +	if (adev->gmc.xgmi.connected_to_cpu) {
> +		vram_addr = vram_addr_first;
> +		vram_end = vram_addr + vram_size;
> +	}
> +
>  	/* The first n PDE0 entries are used as PTE,
>  	 * pointing to vram
>  	 */
> -	for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
> -		amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
> +	for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
> +		amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i,
> +			(vram_addr >= vram_addr_first + vram_size) ? (vram_addr - vram_size) : vram_addr,
> +			flags);
> +	}
>  
>  	/* The n+1'th PDE0 entry points to a huge
>  	 * PTB who has more than 512 entries each
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> index bd7fc123b8f9..758b47240c6f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> @@ -307,6 +307,7 @@ struct amdgpu_gmc {
>  	struct amdgpu_bo		*pdb0_bo;
>  	/* CPU kmapped address of pdb0*/
>  	void				*ptr_pdb0;
> +	bool enable_pdb0;
>  
>  	/* MALL size */
>  	u64 mall_size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index d90e9daf5a50..83a3444c69d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>  		goto error_unpin;
>  	}
>  
> -	if (gpu_addr)
> +	if (gpu_addr) {
>  		*gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
> +		if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
> +			if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
> +				*gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
> +			}
> +		}
> +	}

Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

>  
>  	if (cpu_addr) {
>  		r = amdgpu_bo_kmap(*bo_ptr, cpu_addr);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> index cb25f7f0dfc1..5ebb92ac9fd7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> @@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev,
>  		/* In the case squeezing vram into GART aperture, we don't use
>  		 * FB aperture and AGP aperture. Disable them.
>  		 */
> -		if (adev->gmc.pdb0_bo) {
> +		if (adev->gmc.pdb0_bo && !amdgpu_sriov_vf(adev)) {
>  			WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0);
>  			WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF);
>  			WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 7c7a9fe6be6d..73ac05b9a1bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -1677,6 +1677,10 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block)
>  		adev->gmc.private_aperture_start + (4ULL << 30) - 1;
>  	adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF;
>  
> +	if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> +	    amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4) ||
> +	    amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 5, 0))
> +	    adev->gmc.enable_pdb0 = amdgpu_sriov_vf(adev);
>  	return 0;
>  }
>  
> @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>  {
>  	u64 base = adev->mmhub.funcs->get_fb_location(adev);
>  
> +	if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
> +		adev->gmc.vmid0_page_table_depth = 1;
> +		adev->gmc.vmid0_page_table_block_size = 12;
> +	} else {
> +		adev->gmc.vmid0_page_table_depth = 0;
> +		adev->gmc.vmid0_page_table_block_size = 0;
> +	}
> +

What is the justification to moving that stuff around?

>  	amdgpu_gmc_set_agp_default(adev, mc);
>  
>  	/* add the xgmi offset of the physical node */
> @@ -1727,7 +1739,10 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>  		amdgpu_gmc_sysvm_location(adev, mc);
>  	} else {
>  		amdgpu_gmc_vram_location(adev, mc, base);
> -		amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> +		if (!adev->gmc.enable_pdb0)
> +			amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> +		else
> +			amdgpu_gmc_sysvm_location(adev, mc);
>  		if (!amdgpu_sriov_vf(adev) && (amdgpu_agp == 1))
>  			amdgpu_gmc_agp_location(adev, mc);
>  	}
> @@ -1838,14 +1853,6 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
>  		return 0;
>  	}
>  
> -	if (adev->gmc.xgmi.connected_to_cpu) {
> -		adev->gmc.vmid0_page_table_depth = 1;
> -		adev->gmc.vmid0_page_table_block_size = 12;
> -	} else {
> -		adev->gmc.vmid0_page_table_depth = 0;
> -		adev->gmc.vmid0_page_table_block_size = 0;
> -	}
> -
>  	/* Initialize common gart structure */
>  	r = amdgpu_gart_init(adev);
>  	if (r)
> @@ -1864,7 +1871,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
>  		if (r)
>  			return r;
>  
> -		if (adev->gmc.xgmi.connected_to_cpu)
> +		if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)

Drop the connected_to_cpu check here.

>  			r = amdgpu_gmc_pdb0_alloc(adev);
>  	}
>  
> @@ -2361,7 +2368,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
>  {
>  	int r;
>  
> -	if (adev->gmc.xgmi.connected_to_cpu)
> +	if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)

And here.

>  		amdgpu_gmc_init_pdb0(adev);
>  
>  	if (adev->gart.bo == NULL) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> index fe0710b55c3a..13b229d07ac4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> @@ -74,27 +74,47 @@ static void mmhub_v9_4_setup_hubid_vm_pt_regs(struct amdgpu_device *adev, int hu
>  static void mmhub_v9_4_init_gart_aperture_regs(struct amdgpu_device *adev,
>  					       int hubid)
>  {
> -	uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gart.bo);
> +	uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ? adev->gmc.pdb0_bo : adev->gart.bo);

That can be written as adev->gmc.pdb0_bo ?: adev->gart.bo

>  
>  	mmhub_v9_4_setup_hubid_vm_pt_regs(adev, hubid, 0, pt_base);
>  
> -	WREG32_SOC15_OFFSET(MMHUB, 0,
> +	if (adev->gmc.pdb0_bo) {
> +		WREG32_SOC15_OFFSET(MMHUB, 0,
> +				    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> +				    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +				    (u32)(adev->gmc.fb_start >> 12));
> +		WREG32_SOC15_OFFSET(MMHUB, 0,
> +				    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> +				    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +				    (u32)(adev->gmc.fb_start >> 44));
> +
> +		WREG32_SOC15_OFFSET(MMHUB, 0,
> +				    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> +				    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +				    (u32)(adev->gmc.gart_end >> 12));
> +		WREG32_SOC15_OFFSET(MMHUB, 0,
> +				    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> +				    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +				    (u32)(adev->gmc.gart_end >> 44));
> +	} else {
> ++		WREG32_SOC15_OFFSET(MMHUB, 0,
>  			    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
>  			    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>  			    (u32)(adev->gmc.gart_start >> 12));
> -	WREG32_SOC15_OFFSET(MMHUB, 0,
> +		WREG32_SOC15_OFFSET(MMHUB, 0,
>  			    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
>  			    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>  			    (u32)(adev->gmc.gart_start >> 44));

When you indent the WREG32_SOC15_OFFSET() you need to indent the following lines as well.

>  
> -	WREG32_SOC15_OFFSET(MMHUB, 0,
> +		WREG32_SOC15_OFFSET(MMHUB, 0,
>  			    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
>  			    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>  			    (u32)(adev->gmc.gart_end >> 12));
> -	WREG32_SOC15_OFFSET(MMHUB, 0,
> +		WREG32_SOC15_OFFSET(MMHUB, 0,
>  			    mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
>  			    hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>  			    (u32)(adev->gmc.gart_end >> 44));
> +	}

The programming of the end addr is still the same, you don't need to change anything here.

Regards,
Christian.

>  }
>  
>  static void mmhub_v9_4_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,

Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Thank you for the review and the feedback.

>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index d90e9daf5a50..83a3444c69d9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>>                goto error_unpin;
>>        }
>>
>> -     if (gpu_addr)
>> +     if (gpu_addr) {
>>                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
>> +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
>> +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
>> +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
>> +                     }
>> +             }
>> +     }
>
>Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

This change is to change to the default GPU address from FB aperture type to pdb0 type in this centralized place so that I don’t need to change every callsite of amdgpu_bo_create_reserved().
Could you suggest a better approach if this approach is not acceptable?

Thanks
Sam

From: Christian König <ckoenig.leichtzumerken@gmail.com>
Date: Wednesday, April 16, 2025 at 21:52
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> When switching to new GPU index after hibernation and then resume,
> VRAM offset of each VRAM BO will be changed, and the cached gpu
> addresses needed to updated.
>
> This is to enable pdb0 and switch to use pdb0-based virtual gpu
> address by default in amdgpu_bo_create_reserved(). since the virtual
> addresses do not change, this can avoid the need to update all
> cached gpu addresses all over the codebase.
>
> Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> Change-Id: I2b20b9b94f1e41820a013ce5d05bb3fa96859b21
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 43 +++++++++++++++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 29 +++++++++------
>  drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
>  6 files changed, 82 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 5b60d714e089..e706afcb7e95 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -248,18 +248,25 @@ void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc,
>  void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc)
>  {
>        u64 hive_vram_start = 0;
> -     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1;
> -     mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> -     mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> +     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes;
> +
> +     hive_vram_end = ALIGN(hive_vram_end, (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21) - 1;
> +
> +     if (!mc->vram_start) {
> +             mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> +             mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> +             dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> +                     mc->mc_vram_size >> 20, mc->vram_start,
> +                     mc->vram_end, mc->real_vram_size >> 20);
> +     }
> +
>        mc->gart_start = hive_vram_end + 1;
>        mc->gart_end = mc->gart_start + mc->gart_size - 1;
>        mc->fb_start = hive_vram_start;
>        mc->fb_end = hive_vram_end;
> -     dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> -                     mc->mc_vram_size >> 20, mc->vram_start,
> -                     mc->vram_end, mc->real_vram_size >> 20);
> -     dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n",
> -                     mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> +
> +     dev_info(adev->dev, "FB 0x%016llX - 0x%016llX, GART: %lluM 0x%016llX - 0x%016llX\n",
> +                     mc->fb_start, mc->fb_end, mc->gart_size >> 20, mc->gart_start, mc->gart_end);
>  }
>
>  /**
> @@ -677,8 +684,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                                     &job);
>        if (r)
>                goto error_alloc;
> -
> -     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
> +     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
> +                                                     adev->gmc.pdb0_bo :
> +                                                     adev->gart.bo);
>        job->vm_needs_flush = true;
>        job->ibs->ptr[job->ibs->length_dw++] = ring->funcs->nop;
>        amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> @@ -1041,8 +1049,9 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>         */
>        u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
>        u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
> -     u64 vram_addr = adev->vm_manager.vram_base_offset -
> +     u64 vram_addr_first = adev->vm_manager.vram_base_offset -
>                adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size;
> +     u64 vram_addr = adev->vm_manager.vram_base_offset;
>        u64 vram_end = vram_addr + vram_size;
>        u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
>        int idx;
> @@ -1056,11 +1065,19 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>        flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
>        flags |= AMDGPU_PDE_PTE_FLAG(adev);
>
> +     if (adev->gmc.xgmi.connected_to_cpu) {
> +             vram_addr = vram_addr_first;
> +             vram_end = vram_addr + vram_size;
> +     }
> +
>        /* The first n PDE0 entries are used as PTE,
>         * pointing to vram
>         */
> -     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
> -             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
> +     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
> +             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i,
> +                     (vram_addr >= vram_addr_first + vram_size) ? (vram_addr - vram_size) : vram_addr,
> +                     flags);
> +     }
>
>        /* The n+1'th PDE0 entry points to a huge
>         * PTB who has more than 512 entries each
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> index bd7fc123b8f9..758b47240c6f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> @@ -307,6 +307,7 @@ struct amdgpu_gmc {
>        struct amdgpu_bo                *pdb0_bo;
>        /* CPU kmapped address of pdb0*/
>        void                            *ptr_pdb0;
> +     bool enable_pdb0;
>
>        /* MALL size */
>        u64 mall_size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index d90e9daf5a50..83a3444c69d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>                goto error_unpin;
>        }
>
> -     if (gpu_addr)
> +     if (gpu_addr) {
>                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
> +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
> +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
> +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
> +                     }
> +             }
> +     }

Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

>
>        if (cpu_addr) {
>                r = amdgpu_bo_kmap(*bo_ptr, cpu_addr);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> index cb25f7f0dfc1..5ebb92ac9fd7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> @@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev,
>                /* In the case squeezing vram into GART aperture, we don't use
>                 * FB aperture and AGP aperture. Disable them.
>                 */
> -             if (adev->gmc.pdb0_bo) {
> +             if (adev->gmc.pdb0_bo && !amdgpu_sriov_vf(adev)) {
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0);
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF);
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 7c7a9fe6be6d..73ac05b9a1bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -1677,6 +1677,10 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block)
>                adev->gmc.private_aperture_start + (4ULL << 30) - 1;
>        adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF;
>
> +     if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4) ||
> +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 5, 0))
> +         adev->gmc.enable_pdb0 = amdgpu_sriov_vf(adev);
>        return 0;
>  }
>
> @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>  {
>        u64 base = adev->mmhub.funcs->get_fb_location(adev);
>
> +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
> +             adev->gmc.vmid0_page_table_depth = 1;
> +             adev->gmc.vmid0_page_table_block_size = 12;
> +     } else {
> +             adev->gmc.vmid0_page_table_depth = 0;
> +             adev->gmc.vmid0_page_table_block_size = 0;
> +     }
> +

What is the justification to moving that stuff around?

>        amdgpu_gmc_set_agp_default(adev, mc);
>
>        /* add the xgmi offset of the physical node */
> @@ -1727,7 +1739,10 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>                amdgpu_gmc_sysvm_location(adev, mc);
>        } else {
>                amdgpu_gmc_vram_location(adev, mc, base);
> -             amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> +             if (!adev->gmc.enable_pdb0)
> +                     amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> +             else
> +                     amdgpu_gmc_sysvm_location(adev, mc);
>                if (!amdgpu_sriov_vf(adev) && (amdgpu_agp == 1))
>                        amdgpu_gmc_agp_location(adev, mc);
>        }
> @@ -1838,14 +1853,6 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
>                return 0;
>        }
>
> -     if (adev->gmc.xgmi.connected_to_cpu) {
> -             adev->gmc.vmid0_page_table_depth = 1;
> -             adev->gmc.vmid0_page_table_block_size = 12;
> -     } else {
> -             adev->gmc.vmid0_page_table_depth = 0;
> -             adev->gmc.vmid0_page_table_block_size = 0;
> -     }
> -
>        /* Initialize common gart structure */
>        r = amdgpu_gart_init(adev);
>        if (r)
> @@ -1864,7 +1871,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
>                if (r)
>                        return r;
>
> -             if (adev->gmc.xgmi.connected_to_cpu)
> +             if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)

Drop the connected_to_cpu check here.

>                        r = amdgpu_gmc_pdb0_alloc(adev);
>        }
>
> @@ -2361,7 +2368,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
>  {
>        int r;
>
> -     if (adev->gmc.xgmi.connected_to_cpu)
> +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)

And here.

>                amdgpu_gmc_init_pdb0(adev);
>
>        if (adev->gart.bo == NULL) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> index fe0710b55c3a..13b229d07ac4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> @@ -74,27 +74,47 @@ static void mmhub_v9_4_setup_hubid_vm_pt_regs(struct amdgpu_device *adev, int hu
>  static void mmhub_v9_4_init_gart_aperture_regs(struct amdgpu_device *adev,
>                                               int hubid)
>  {
> -     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gart.bo);
> +     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ? adev->gmc.pdb0_bo : adev->gart.bo);

That can be written as adev->gmc.pdb0_bo ?: adev->gart.bo

>
>        mmhub_v9_4_setup_hubid_vm_pt_regs(adev, hubid, 0, pt_base);
>
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +     if (adev->gmc.pdb0_bo) {
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.fb_start >> 12));
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.fb_start >> 44));
> +
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.gart_end >> 12));
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.gart_end >> 44));
> +     } else {
> ++            WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_start >> 12));
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_start >> 44));

When you indent the WREG32_SOC15_OFFSET() you need to indent the following lines as well.

>
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_end >> 12));
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_end >> 44));
> +     }

The programming of the end addr is still the same, you don't need to change anything here.

Regards,
Christian.

>  }
>
>  static void mmhub_v9_4_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,

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

Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Ping…

Thanks
Sam

From: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
Date: Friday, April 18, 2025 at 14:26
To: Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
Thank you for the review and the feedback.

>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index d90e9daf5a50..83a3444c69d9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>>                goto error_unpin;
>>        }
>>
>> -     if (gpu_addr)
>> +     if (gpu_addr) {
>>                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
>> +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
>> +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
>> +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
>> +                     }
>> +             }
>> +     }
>
>Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

This change is to change to the default GPU address from FB aperture type to pdb0 type in this centralized place so that I don’t need to change every callsite of amdgpu_bo_create_reserved().
Could you suggest a better approach if this approach is not acceptable?

Thanks
Sam

From: Christian König <ckoenig.leichtzumerken@gmail.com>
Date: Wednesday, April 16, 2025 at 21:52
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> When switching to new GPU index after hibernation and then resume,
> VRAM offset of each VRAM BO will be changed, and the cached gpu
> addresses needed to updated.
>
> This is to enable pdb0 and switch to use pdb0-based virtual gpu
> address by default in amdgpu_bo_create_reserved(). since the virtual
> addresses do not change, this can avoid the need to update all
> cached gpu addresses all over the codebase.
>
> Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> Change-Id: I2b20b9b94f1e41820a013ce5d05bb3fa96859b21
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 43 +++++++++++++++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 29 +++++++++------
>  drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
>  6 files changed, 82 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 5b60d714e089..e706afcb7e95 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -248,18 +248,25 @@ void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc,
>  void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc)
>  {
>        u64 hive_vram_start = 0;
> -     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1;
> -     mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> -     mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> +     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes;
> +
> +     hive_vram_end = ALIGN(hive_vram_end, (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21) - 1;
> +
> +     if (!mc->vram_start) {
> +             mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> +             mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> +             dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> +                     mc->mc_vram_size >> 20, mc->vram_start,
> +                     mc->vram_end, mc->real_vram_size >> 20);
> +     }
> +
>        mc->gart_start = hive_vram_end + 1;
>        mc->gart_end = mc->gart_start + mc->gart_size - 1;
>        mc->fb_start = hive_vram_start;
>        mc->fb_end = hive_vram_end;
> -     dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> -                     mc->mc_vram_size >> 20, mc->vram_start,
> -                     mc->vram_end, mc->real_vram_size >> 20);
> -     dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n",
> -                     mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> +
> +     dev_info(adev->dev, "FB 0x%016llX - 0x%016llX, GART: %lluM 0x%016llX - 0x%016llX\n",
> +                     mc->fb_start, mc->fb_end, mc->gart_size >> 20, mc->gart_start, mc->gart_end);
>  }
>
>  /**
> @@ -677,8 +684,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                                     &job);
>        if (r)
>                goto error_alloc;
> -
> -     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
> +     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
> +                                                     adev->gmc.pdb0_bo :
> +                                                     adev->gart.bo);
>        job->vm_needs_flush = true;
>        job->ibs->ptr[job->ibs->length_dw++] = ring->funcs->nop;
>        amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> @@ -1041,8 +1049,9 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>         */
>        u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
>        u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
> -     u64 vram_addr = adev->vm_manager.vram_base_offset -
> +     u64 vram_addr_first = adev->vm_manager.vram_base_offset -
>                adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size;
> +     u64 vram_addr = adev->vm_manager.vram_base_offset;
>        u64 vram_end = vram_addr + vram_size;
>        u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
>        int idx;
> @@ -1056,11 +1065,19 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>        flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
>        flags |= AMDGPU_PDE_PTE_FLAG(adev);
>
> +     if (adev->gmc.xgmi.connected_to_cpu) {
> +             vram_addr = vram_addr_first;
> +             vram_end = vram_addr + vram_size;
> +     }
> +
>        /* The first n PDE0 entries are used as PTE,
>         * pointing to vram
>         */
> -     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
> -             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
> +     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
> +             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i,
> +                     (vram_addr >= vram_addr_first + vram_size) ? (vram_addr - vram_size) : vram_addr,
> +                     flags);
> +     }
>
>        /* The n+1'th PDE0 entry points to a huge
>         * PTB who has more than 512 entries each
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> index bd7fc123b8f9..758b47240c6f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> @@ -307,6 +307,7 @@ struct amdgpu_gmc {
>        struct amdgpu_bo                *pdb0_bo;
>        /* CPU kmapped address of pdb0*/
>        void                            *ptr_pdb0;
> +     bool enable_pdb0;
>
>        /* MALL size */
>        u64 mall_size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index d90e9daf5a50..83a3444c69d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>                goto error_unpin;
>        }
>
> -     if (gpu_addr)
> +     if (gpu_addr) {
>                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
> +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
> +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
> +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
> +                     }
> +             }
> +     }

Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

>
>        if (cpu_addr) {
>                r = amdgpu_bo_kmap(*bo_ptr, cpu_addr);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> index cb25f7f0dfc1..5ebb92ac9fd7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> @@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev,
>                /* In the case squeezing vram into GART aperture, we don't use
>                 * FB aperture and AGP aperture. Disable them.
>                 */
> -             if (adev->gmc.pdb0_bo) {
> +             if (adev->gmc.pdb0_bo && !amdgpu_sriov_vf(adev)) {
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0);
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF);
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 7c7a9fe6be6d..73ac05b9a1bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -1677,6 +1677,10 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block)
>                adev->gmc.private_aperture_start + (4ULL << 30) - 1;
>        adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF;
>
> +     if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4) ||
> +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 5, 0))
> +         adev->gmc.enable_pdb0 = amdgpu_sriov_vf(adev);
>        return 0;
>  }
>
> @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>  {
>        u64 base = adev->mmhub.funcs->get_fb_location(adev);
>
> +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
> +             adev->gmc.vmid0_page_table_depth = 1;
> +             adev->gmc.vmid0_page_table_block_size = 12;
> +     } else {
> +             adev->gmc.vmid0_page_table_depth = 0;
> +             adev->gmc.vmid0_page_table_block_size = 0;
> +     }
> +

What is the justification to moving that stuff around?

>        amdgpu_gmc_set_agp_default(adev, mc);
>
>        /* add the xgmi offset of the physical node */
> @@ -1727,7 +1739,10 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>                amdgpu_gmc_sysvm_location(adev, mc);
>        } else {
>                amdgpu_gmc_vram_location(adev, mc, base);
> -             amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> +             if (!adev->gmc.enable_pdb0)
> +                     amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> +             else
> +                     amdgpu_gmc_sysvm_location(adev, mc);
>                if (!amdgpu_sriov_vf(adev) && (amdgpu_agp == 1))
>                        amdgpu_gmc_agp_location(adev, mc);
>        }
> @@ -1838,14 +1853,6 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
>                return 0;
>        }
>
> -     if (adev->gmc.xgmi.connected_to_cpu) {
> -             adev->gmc.vmid0_page_table_depth = 1;
> -             adev->gmc.vmid0_page_table_block_size = 12;
> -     } else {
> -             adev->gmc.vmid0_page_table_depth = 0;
> -             adev->gmc.vmid0_page_table_block_size = 0;
> -     }
> -
>        /* Initialize common gart structure */
>        r = amdgpu_gart_init(adev);
>        if (r)
> @@ -1864,7 +1871,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
>                if (r)
>                        return r;
>
> -             if (adev->gmc.xgmi.connected_to_cpu)
> +             if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)

Drop the connected_to_cpu check here.

>                        r = amdgpu_gmc_pdb0_alloc(adev);
>        }
>
> @@ -2361,7 +2368,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
>  {
>        int r;
>
> -     if (adev->gmc.xgmi.connected_to_cpu)
> +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)

And here.

>                amdgpu_gmc_init_pdb0(adev);
>
>        if (adev->gart.bo == NULL) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> index fe0710b55c3a..13b229d07ac4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> @@ -74,27 +74,47 @@ static void mmhub_v9_4_setup_hubid_vm_pt_regs(struct amdgpu_device *adev, int hu
>  static void mmhub_v9_4_init_gart_aperture_regs(struct amdgpu_device *adev,
>                                               int hubid)
>  {
> -     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gart.bo);
> +     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ? adev->gmc.pdb0_bo : adev->gart.bo);

That can be written as adev->gmc.pdb0_bo ?: adev->gart.bo

>
>        mmhub_v9_4_setup_hubid_vm_pt_regs(adev, hubid, 0, pt_base);
>
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +     if (adev->gmc.pdb0_bo) {
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.fb_start >> 12));
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.fb_start >> 44));
> +
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.gart_end >> 12));
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.gart_end >> 44));
> +     } else {
> ++            WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_start >> 12));
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_start >> 44));

When you indent the WREG32_SOC15_OFFSET() you need to indent the following lines as well.

>
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_end >> 12));
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_end >> 44));
> +     }

The programming of the end addr is still the same, you don't need to change anything here.

Regards,
Christian.

>  }
>
>  static void mmhub_v9_4_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,

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

RE: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Liu, Monk]

[-- Attachment #1.1: Type: text/plain, Size: 18155 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]

+ @Zhang, Owen(SRDC)<mailto:Owen.Zhang2@amd.com> (PM) for awareness

Monk Liu | Cloud GPU & Virtualization | AMD
[cid:image001.png@01DBB45D.8D2D5C40]

From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Zhang, GuoQing (Sam)
Sent: Tuesday, April 22, 2025 6:39 PM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
Cc: Zhao, Victor <Victor.Zhao@amd.com>; Chang, HaiJun <HaiJun.Chang@amd.com>; Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV


[AMD Official Use Only - AMD Internal Distribution Only]


[AMD Official Use Only - AMD Internal Distribution Only]

Ping...

Thanks
Sam

From: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com<mailto:GuoQing.Zhang@amd.com>>
Date: Friday, April 18, 2025 at 14:26
To: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>, amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Cc: Zhao, Victor <Victor.Zhao@amd.com<mailto:Victor.Zhao@amd.com>>, Chang, HaiJun <HaiJun.Chang@amd.com<mailto:HaiJun.Chang@amd.com>>, Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
Thank you for the review and the feedback.

>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index d90e9daf5a50..83a3444c69d9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>>                goto error_unpin;
>>        }
>>
>> -     if (gpu_addr)
>> +     if (gpu_addr) {
>>                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
>> +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
>> +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
>> +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
>> +                     }
>> +             }
>> +     }
>
>Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

This change is to change to the default GPU address from FB aperture type to pdb0 type in this centralized place so that I don't need to change every callsite of amdgpu_bo_create_reserved().
Could you suggest a better approach if this approach is not acceptable?

Thanks
Sam

From: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>
Date: Wednesday, April 16, 2025 at 21:52
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com<mailto:GuoQing.Zhang@amd.com>>, amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Cc: Zhao, Victor <Victor.Zhao@amd.com<mailto:Victor.Zhao@amd.com>>, Chang, HaiJun <HaiJun.Chang@amd.com<mailto:HaiJun.Chang@amd.com>>, Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> When switching to new GPU index after hibernation and then resume,
> VRAM offset of each VRAM BO will be changed, and the cached gpu
> addresses needed to updated.
>
> This is to enable pdb0 and switch to use pdb0-based virtual gpu
> address by default in amdgpu_bo_create_reserved(). since the virtual
> addresses do not change, this can avoid the need to update all
> cached gpu addresses all over the codebase.
>
> Signed-off-by: Emily Deng <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com<mailto:guoqing.zhang@amd.com>>
> Change-Id: I2b20b9b94f1e41820a013ce5d05bb3fa96859b21
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 43 +++++++++++++++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
>  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 29 +++++++++------
>  drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
>  6 files changed, 82 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 5b60d714e089..e706afcb7e95 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -248,18 +248,25 @@ void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc,
>  void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc)
>  {
>        u64 hive_vram_start = 0;
> -     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1;
> -     mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> -     mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> +     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes;
> +
> +     hive_vram_end = ALIGN(hive_vram_end, (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21) - 1;
> +
> +     if (!mc->vram_start) {
> +             mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> +             mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> +             dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> +                     mc->mc_vram_size >> 20, mc->vram_start,
> +                     mc->vram_end, mc->real_vram_size >> 20);
> +     }
> +
>        mc->gart_start = hive_vram_end + 1;
>        mc->gart_end = mc->gart_start + mc->gart_size - 1;
>        mc->fb_start = hive_vram_start;
>        mc->fb_end = hive_vram_end;
> -     dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> -                     mc->mc_vram_size >> 20, mc->vram_start,
> -                     mc->vram_end, mc->real_vram_size >> 20);
> -     dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n",
> -                     mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> +
> +     dev_info(adev->dev, "FB 0x%016llX - 0x%016llX, GART: %lluM 0x%016llX - 0x%016llX\n",
> +                     mc->fb_start, mc->fb_end, mc->gart_size >> 20, mc->gart_start, mc->gart_end);
>  }
>
>  /**
> @@ -677,8 +684,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
>                                     &job);
>        if (r)
>                goto error_alloc;
> -
> -     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
> +     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
> +                                                     adev->gmc.pdb0_bo :
> +                                                     adev->gart.bo);
>        job->vm_needs_flush = true;
>        job->ibs->ptr[job->ibs->length_dw++] = ring->funcs->nop;
>        amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> @@ -1041,8 +1049,9 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>         */
>        u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
>        u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
> -     u64 vram_addr = adev->vm_manager.vram_base_offset -
> +     u64 vram_addr_first = adev->vm_manager.vram_base_offset -
>                adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size;
> +     u64 vram_addr = adev->vm_manager.vram_base_offset;
>        u64 vram_end = vram_addr + vram_size;
>        u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
>        int idx;
> @@ -1056,11 +1065,19 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
>        flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
>        flags |= AMDGPU_PDE_PTE_FLAG(adev);
>
> +     if (adev->gmc.xgmi.connected_to_cpu) {
> +             vram_addr = vram_addr_first;
> +             vram_end = vram_addr + vram_size;
> +     }
> +
>        /* The first n PDE0 entries are used as PTE,
>         * pointing to vram
>         */
> -     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
> -             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
> +     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
> +             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i,
> +                     (vram_addr >= vram_addr_first + vram_size) ? (vram_addr - vram_size) : vram_addr,
> +                     flags);
> +     }
>
>        /* The n+1'th PDE0 entry points to a huge
>         * PTB who has more than 512 entries each
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> index bd7fc123b8f9..758b47240c6f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> @@ -307,6 +307,7 @@ struct amdgpu_gmc {
>        struct amdgpu_bo                *pdb0_bo;
>        /* CPU kmapped address of pdb0*/
>        void                            *ptr_pdb0;
> +     bool enable_pdb0;
>
>        /* MALL size */
>        u64 mall_size;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index d90e9daf5a50..83a3444c69d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>                goto error_unpin;
>        }
>
> -     if (gpu_addr)
> +     if (gpu_addr) {
>                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
> +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
> +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
> +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
> +                     }
> +             }
> +     }

Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

>
>        if (cpu_addr) {
>                r = amdgpu_bo_kmap(*bo_ptr, cpu_addr);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> index cb25f7f0dfc1..5ebb92ac9fd7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> @@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev,
>                /* In the case squeezing vram into GART aperture, we don't use
>                 * FB aperture and AGP aperture. Disable them.
>                 */
> -             if (adev->gmc.pdb0_bo) {
> +             if (adev->gmc.pdb0_bo && !amdgpu_sriov_vf(adev)) {
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0);
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF);
>                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 7c7a9fe6be6d..73ac05b9a1bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -1677,6 +1677,10 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block)
>                adev->gmc.private_aperture_start + (4ULL << 30) - 1;
>        adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF;
>
> +     if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4) ||
> +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 5, 0))
> +         adev->gmc.enable_pdb0 = amdgpu_sriov_vf(adev);
>        return 0;
>  }
>
> @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>  {
>        u64 base = adev->mmhub.funcs->get_fb_location(adev);
>
> +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
> +             adev->gmc.vmid0_page_table_depth = 1;
> +             adev->gmc.vmid0_page_table_block_size = 12;
> +     } else {
> +             adev->gmc.vmid0_page_table_depth = 0;
> +             adev->gmc.vmid0_page_table_block_size = 0;
> +     }
> +

What is the justification to moving that stuff around?

>        amdgpu_gmc_set_agp_default(adev, mc);
>
>        /* add the xgmi offset of the physical node */
> @@ -1727,7 +1739,10 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>                amdgpu_gmc_sysvm_location(adev, mc);
>        } else {
>                amdgpu_gmc_vram_location(adev, mc, base);
> -             amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> +             if (!adev->gmc.enable_pdb0)
> +                     amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> +             else
> +                     amdgpu_gmc_sysvm_location(adev, mc);
>                if (!amdgpu_sriov_vf(adev) && (amdgpu_agp == 1))
>                        amdgpu_gmc_agp_location(adev, mc);
>        }
> @@ -1838,14 +1853,6 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
>                return 0;
>        }
>
> -     if (adev->gmc.xgmi.connected_to_cpu) {
> -             adev->gmc.vmid0_page_table_depth = 1;
> -             adev->gmc.vmid0_page_table_block_size = 12;
> -     } else {
> -             adev->gmc.vmid0_page_table_depth = 0;
> -             adev->gmc.vmid0_page_table_block_size = 0;
> -     }
> -
>        /* Initialize common gart structure */
>        r = amdgpu_gart_init(adev);
>        if (r)
> @@ -1864,7 +1871,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
>                if (r)
>                        return r;
>
> -             if (adev->gmc.xgmi.connected_to_cpu)
> +             if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)

Drop the connected_to_cpu check here.

>                        r = amdgpu_gmc_pdb0_alloc(adev);
>        }
>
> @@ -2361,7 +2368,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
>  {
>        int r;
>
> -     if (adev->gmc.xgmi.connected_to_cpu)
> +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)

And here.

>                amdgpu_gmc_init_pdb0(adev);
>
>        if (adev->gart.bo == NULL) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> index fe0710b55c3a..13b229d07ac4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> @@ -74,27 +74,47 @@ static void mmhub_v9_4_setup_hubid_vm_pt_regs(struct amdgpu_device *adev, int hu
>  static void mmhub_v9_4_init_gart_aperture_regs(struct amdgpu_device *adev,
>                                               int hubid)
>  {
> -     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gart.bo);
> +     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ? adev->gmc.pdb0_bo : adev->gart.bo);

That can be written as adev->gmc.pdb0_bo ?: adev->gart.bo

>
>        mmhub_v9_4_setup_hubid_vm_pt_regs(adev, hubid, 0, pt_base);
>
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +     if (adev->gmc.pdb0_bo) {
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.fb_start >> 12));
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.fb_start >> 44));
> +
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.gart_end >> 12));
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
> +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> +                                 (u32)(adev->gmc.gart_end >> 44));
> +     } else {
> ++            WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_start >> 12));
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_start >> 44));

When you indent the WREG32_SOC15_OFFSET() you need to indent the following lines as well.

>
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_end >> 12));
> -     WREG32_SOC15_OFFSET(MMHUB, 0,
> +             WREG32_SOC15_OFFSET(MMHUB, 0,
>                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
>                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
>                            (u32)(adev->gmc.gart_end >> 44));
> +     }

The programming of the end addr is still the same, you don't need to change anything here.

Regards,
Christian.

>  }
>
>  static void mmhub_v9_4_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,

[-- Attachment #1.2: Type: text/html, Size: 42561 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 12243 bytes --]

Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Christian,

Thank you for the review and the feedback. I will update the patch according to your feedback.
Please see my 2 inline comments below.

Regards
Sam


> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Date: Wednesday, April 16, 2025 at 21:52
> To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
> Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
> Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> > When switching to new GPU index after hibernation and then resume,
> > VRAM offset of each VRAM BO will be changed, and the cached gpu
> > addresses needed to updated.
> >
> > This is to enable pdb0 and switch to use pdb0-based virtual gpu
> > address by default in amdgpu_bo_create_reserved(). since the virtual
> > addresses do not change, this can avoid the need to update all
> > cached gpu addresses all over the codebase.
> >
> > Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> > Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> > Change-Id: I2b20b9b94f1e41820a013ce5d05bb3fa96859b21
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 43 +++++++++++++++-------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
> >  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
> >  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 29 +++++++++------
> >  drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
> >  6 files changed, 82 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > index 5b60d714e089..e706afcb7e95 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > @@ -248,18 +248,25 @@ void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc,
> >  void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc)
> >  {
> >        u64 hive_vram_start = 0;
> > -     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1;
> > -     mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> > -     mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> > +     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes;
> > +
> > +     hive_vram_end = ALIGN(hive_vram_end, (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21) - 1;
> > +
> > +     if (!mc->vram_start) {
> > +             mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> > +             mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> > +             dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> > +                     mc->mc_vram_size >> 20, mc->vram_start,
> > +                     mc->vram_end, mc->real_vram_size >> 20);
> > +     }
> > +
> >        mc->gart_start = hive_vram_end + 1;
> >        mc->gart_end = mc->gart_start + mc->gart_size - 1;
> >        mc->fb_start = hive_vram_start;
> >        mc->fb_end = hive_vram_end;
> > -     dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> > -                     mc->mc_vram_size >> 20, mc->vram_start,
> > -                     mc->vram_end, mc->real_vram_size >> 20);
> > -     dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n",
> > -                     mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> > +
> > +     dev_info(adev->dev, "FB 0x%016llX - 0x%016llX, GART: %lluM 0x%016llX - 0x%016llX\n",
> > +                     mc->fb_start, mc->fb_end, mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> >  }
> >
> >  /**
> > @@ -677,8 +684,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
> >                                     &job);
> >        if (r)
> >                goto error_alloc;
> > -
> > -     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
> > +     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
> > +                                                     adev->gmc.pdb0_bo :
> > +                                                     adev->gart.bo);
> >        job->vm_needs_flush = true;
> >        job->ibs->ptr[job->ibs->length_dw++] = ring->funcs->nop;
> >        amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> > @@ -1041,8 +1049,9 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
> >         */
> >        u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
> >        u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
> > -     u64 vram_addr = adev->vm_manager.vram_base_offset -
> > +     u64 vram_addr_first = adev->vm_manager.vram_base_offset -
> >                adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size;
> > +     u64 vram_addr = adev->vm_manager.vram_base_offset;
> >        u64 vram_end = vram_addr + vram_size;
> >        u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
> >        int idx;
> > @@ -1056,11 +1065,19 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
> >        flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
> >        flags |= AMDGPU_PDE_PTE_FLAG(adev);
> >
> > +     if (adev->gmc.xgmi.connected_to_cpu) {
> > +             vram_addr = vram_addr_first;
> > +             vram_end = vram_addr + vram_size;
> > +     }
> > +
> >        /* The first n PDE0 entries are used as PTE,
> >         * pointing to vram
> >         */
> > -     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
> > -             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
> > +     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
> > +             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i,
> > +                     (vram_addr >= vram_addr_first + vram_size) ? (vram_addr - vram_size) : vram_addr,
> > +                     flags);
> > +     }
> >
> >        /* The n+1'th PDE0 entry points to a huge
> >         * PTB who has more than 512 entries each
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > index bd7fc123b8f9..758b47240c6f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > @@ -307,6 +307,7 @@ struct amdgpu_gmc {
> >        struct amdgpu_bo                *pdb0_bo;
> >        /* CPU kmapped address of pdb0*/
> >        void                            *ptr_pdb0;
> > +     bool enable_pdb0;
> >
> >        /* MALL size */
> >        u64 mall_size;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index d90e9daf5a50..83a3444c69d9 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
> >                goto error_unpin;
> >        }
> >
> > -     if (gpu_addr)
> > +     if (gpu_addr) {
> >                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
> > +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
> > +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
> > +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
> > +                     }
> > +             }
> > +     }
>
> Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

This change is to change to the default GPU address from FB aperture type to pdb0 type in this centralized place so that I don’t need to change every callsite of amdgpu_bo_create_reserved().
Could you suggest a better approach if this approach is not acceptable?


>
>
> >
> >        if (cpu_addr) {
> >                r = amdgpu_bo_kmap(*bo_ptr, cpu_addr);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > index cb25f7f0dfc1..5ebb92ac9fd7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > @@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev,
> >                /* In the case squeezing vram into GART aperture, we don't use
> >                 * FB aperture and AGP aperture. Disable them.
> >                 */
> > -             if (adev->gmc.pdb0_bo) {
> > +             if (adev->gmc.pdb0_bo && !amdgpu_sriov_vf(adev)) {
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0);
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF);
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > index 7c7a9fe6be6d..73ac05b9a1bf 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > @@ -1677,6 +1677,10 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block)
> >                adev->gmc.private_aperture_start + (4ULL << 30) - 1;
> >        adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF;
> >
> > +     if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> > +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4) ||
> > +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 5, 0))
> > +         adev->gmc.enable_pdb0 = amdgpu_sriov_vf(adev);
> >        return 0;
> >  }
> >
> > @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
> >  {
> >        u64 base = adev->mmhub.funcs->get_fb_location(adev);
> >
> > +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
> > +             adev->gmc.vmid0_page_table_depth = 1;
> > +             adev->gmc.vmid0_page_table_block_size = 12;
> > +     } else {
> > +             adev->gmc.vmid0_page_table_depth = 0;
> > +             adev->gmc.vmid0_page_table_block_size = 0;
> > +     }
> > +
>
> What is the justification to moving that stuff around?

vmid0_page_table_block_size is used in new code in amdgpu_gmc_sysvm_location(). See the call sequence below.

gmc_v9_0_sw_init
- gmc_v9_0_mc_init
                - gmc_v9_0_vram_gtt_location,
                                - vmid0_page_table_block_size = 12, **new location**
                                - amdgpu_gmc_sysvm_location
                                                - use **vmid0_page_table_block_size**
- gmc_v9_0_gart_init,
                - assign vmid0_page_table_block_size, **old location**



>
> >        amdgpu_gmc_set_agp_default(adev, mc);
> >
> >        /* add the xgmi offset of the physical node */
> > @@ -1727,7 +1739,10 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
> >                amdgpu_gmc_sysvm_location(adev, mc);
> >        } else {
> >                amdgpu_gmc_vram_location(adev, mc, base);
> > -             amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> > +             if (!adev->gmc.enable_pdb0)
> > +                     amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> > +             else
> > +                     amdgpu_gmc_sysvm_location(adev, mc);
> >                if (!amdgpu_sriov_vf(adev) && (amdgpu_agp == 1))
> >                        amdgpu_gmc_agp_location(adev, mc);
> >        }
> > @@ -1838,14 +1853,6 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
> >                return 0;
> >        }
> >
> > -     if (adev->gmc.xgmi.connected_to_cpu) {
> > -             adev->gmc.vmid0_page_table_depth = 1;
> > -             adev->gmc.vmid0_page_table_block_size = 12;
> > -     } else {
> > -             adev->gmc.vmid0_page_table_depth = 0;
> > -             adev->gmc.vmid0_page_table_block_size = 0;
> > -     }
> > -
> >        /* Initialize common gart structure */
> >        r = amdgpu_gart_init(adev);
> >        if (r)
> > @@ -1864,7 +1871,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
> >                if (r)
> >                        return r;
> >
> > -             if (adev->gmc.xgmi.connected_to_cpu)
> > +             if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)
>
> Drop the connected_to_cpu check here.
>
> >                        r = amdgpu_gmc_pdb0_alloc(adev);
> >        }
> >
> > @@ -2361,7 +2368,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
> >  {
> >        int r;
> >
> > -     if (adev->gmc.xgmi.connected_to_cpu)
> > +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)
>
> And here.
>
> >                amdgpu_gmc_init_pdb0(adev);
> >
> >        if (adev->gart.bo == NULL) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > index fe0710b55c3a..13b229d07ac4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > @@ -74,27 +74,47 @@ static void mmhub_v9_4_setup_hubid_vm_pt_regs(struct amdgpu_device *adev, int hu
> >  static void mmhub_v9_4_init_gart_aperture_regs(struct amdgpu_device *adev,
> >                                               int hubid)
> >  {
> > -     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gart.bo);
> > +     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ? adev->gmc.pdb0_bo : adev->gart.bo);
>
> That can be written as adev->gmc.pdb0_bo ?: adev->gart.bo
>
> >
> >        mmhub_v9_4_setup_hubid_vm_pt_regs(adev, hubid, 0, pt_base);
> >
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +     if (adev->gmc.pdb0_bo) {
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.fb_start >> 12));
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.fb_start >> 44));
> > +
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.gart_end >> 12));
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.gart_end >> 44));
> > +     } else {
> > ++            WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_start >> 12));
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_start >> 44));
>
> When you indent the WREG32_SOC15_OFFSET() you need to indent the following lines as well.
>
> >
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_end >> 12));
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_end >> 44));
> > +     }
>
> The programming of the end addr is still the same, you don't need to change anything here.
>
> Regards,
> Christian.
>
> >  }
> >
> >  static void mmhub_v9_4_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,
>

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

Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Ping… @Koenig, Christian<mailto:Christian.Koenig@amd.com>

Thanks
Sam

From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
Date: Wednesday, April 23, 2025 at 15:25
To: Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[AMD Official Use Only - AMD Internal Distribution Only]


[AMD Official Use Only - AMD Internal Distribution Only]

Hi Christian,

Thank you for the review and the feedback. I will update the patch according to your feedback.
Please see my 2 inline comments below.

Regards
Sam


> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Date: Wednesday, April 16, 2025 at 21:52
> To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
> Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
> Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> > When switching to new GPU index after hibernation and then resume,
> > VRAM offset of each VRAM BO will be changed, and the cached gpu
> > addresses needed to updated.
> >
> > This is to enable pdb0 and switch to use pdb0-based virtual gpu
> > address by default in amdgpu_bo_create_reserved(). since the virtual
> > addresses do not change, this can avoid the need to update all
> > cached gpu addresses all over the codebase.
> >
> > Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> > Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> > Change-Id: I2b20b9b94f1e41820a013ce5d05bb3fa96859b21
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 43 +++++++++++++++-------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
> >  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
> >  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 29 +++++++++------
> >  drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
> >  6 files changed, 82 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > index 5b60d714e089..e706afcb7e95 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > @@ -248,18 +248,25 @@ void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc,
> >  void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc)
> >  {
> >        u64 hive_vram_start = 0;
> > -     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1;
> > -     mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> > -     mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> > +     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes;
> > +
> > +     hive_vram_end = ALIGN(hive_vram_end, (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21) - 1;
> > +
> > +     if (!mc->vram_start) {
> > +             mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> > +             mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> > +             dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> > +                     mc->mc_vram_size >> 20, mc->vram_start,
> > +                     mc->vram_end, mc->real_vram_size >> 20);
> > +     }
> > +
> >        mc->gart_start = hive_vram_end + 1;
> >        mc->gart_end = mc->gart_start + mc->gart_size - 1;
> >        mc->fb_start = hive_vram_start;
> >        mc->fb_end = hive_vram_end;
> > -     dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> > -                     mc->mc_vram_size >> 20, mc->vram_start,
> > -                     mc->vram_end, mc->real_vram_size >> 20);
> > -     dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n",
> > -                     mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> > +
> > +     dev_info(adev->dev, "FB 0x%016llX - 0x%016llX, GART: %lluM 0x%016llX - 0x%016llX\n",
> > +                     mc->fb_start, mc->fb_end, mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> >  }
> >
> >  /**
> > @@ -677,8 +684,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
> >                                     &job);
> >        if (r)
> >                goto error_alloc;
> > -
> > -     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
> > +     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
> > +                                                     adev->gmc.pdb0_bo :
> > +                                                     adev->gart.bo);
> >        job->vm_needs_flush = true;
> >        job->ibs->ptr[job->ibs->length_dw++] = ring->funcs->nop;
> >        amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> > @@ -1041,8 +1049,9 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
> >         */
> >        u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
> >        u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
> > -     u64 vram_addr = adev->vm_manager.vram_base_offset -
> > +     u64 vram_addr_first = adev->vm_manager.vram_base_offset -
> >                adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size;
> > +     u64 vram_addr = adev->vm_manager.vram_base_offset;
> >        u64 vram_end = vram_addr + vram_size;
> >        u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
> >        int idx;
> > @@ -1056,11 +1065,19 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
> >        flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
> >        flags |= AMDGPU_PDE_PTE_FLAG(adev);
> >
> > +     if (adev->gmc.xgmi.connected_to_cpu) {
> > +             vram_addr = vram_addr_first;
> > +             vram_end = vram_addr + vram_size;
> > +     }
> > +
> >        /* The first n PDE0 entries are used as PTE,
> >         * pointing to vram
> >         */
> > -     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
> > -             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
> > +     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
> > +             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i,
> > +                     (vram_addr >= vram_addr_first + vram_size) ? (vram_addr - vram_size) : vram_addr,
> > +                     flags);
> > +     }
> >
> >        /* The n+1'th PDE0 entry points to a huge
> >         * PTB who has more than 512 entries each
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > index bd7fc123b8f9..758b47240c6f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > @@ -307,6 +307,7 @@ struct amdgpu_gmc {
> >        struct amdgpu_bo                *pdb0_bo;
> >        /* CPU kmapped address of pdb0*/
> >        void                            *ptr_pdb0;
> > +     bool enable_pdb0;
> >
> >        /* MALL size */
> >        u64 mall_size;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index d90e9daf5a50..83a3444c69d9 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
> >                goto error_unpin;
> >        }
> >
> > -     if (gpu_addr)
> > +     if (gpu_addr) {
> >                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
> > +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
> > +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
> > +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
> > +                     }
> > +             }
> > +     }
>
> Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

This change is to change to the default GPU address from FB aperture type to pdb0 type in this centralized place so that I don’t need to change every callsite of amdgpu_bo_create_reserved().
Could you suggest a better approach if this approach is not acceptable?


>
>
> >
> >        if (cpu_addr) {
> >                r = amdgpu_bo_kmap(*bo_ptr, cpu_addr);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > index cb25f7f0dfc1..5ebb92ac9fd7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > @@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev,
> >                /* In the case squeezing vram into GART aperture, we don't use
> >                 * FB aperture and AGP aperture. Disable them.
> >                 */
> > -             if (adev->gmc.pdb0_bo) {
> > +             if (adev->gmc.pdb0_bo && !amdgpu_sriov_vf(adev)) {
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0);
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF);
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > index 7c7a9fe6be6d..73ac05b9a1bf 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > @@ -1677,6 +1677,10 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block)
> >                adev->gmc.private_aperture_start + (4ULL << 30) - 1;
> >        adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF;
> >
> > +     if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> > +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4) ||
> > +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 5, 0))
> > +         adev->gmc.enable_pdb0 = amdgpu_sriov_vf(adev);
> >        return 0;
> >  }
> >
> > @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
> >  {
> >        u64 base = adev->mmhub.funcs->get_fb_location(adev);
> >
> > +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
> > +             adev->gmc.vmid0_page_table_depth = 1;
> > +             adev->gmc.vmid0_page_table_block_size = 12;
> > +     } else {
> > +             adev->gmc.vmid0_page_table_depth = 0;
> > +             adev->gmc.vmid0_page_table_block_size = 0;
> > +     }
> > +
>
> What is the justification to moving that stuff around?

vmid0_page_table_block_size is used in new code in amdgpu_gmc_sysvm_location(). See the call sequence below.

gmc_v9_0_sw_init
- gmc_v9_0_mc_init
                - gmc_v9_0_vram_gtt_location,
                                - vmid0_page_table_block_size = 12, **new location**
                                - amdgpu_gmc_sysvm_location
                                                - use **vmid0_page_table_block_size**
- gmc_v9_0_gart_init,
                - assign vmid0_page_table_block_size, **old location**



>
> >        amdgpu_gmc_set_agp_default(adev, mc);
> >
> >        /* add the xgmi offset of the physical node */
> > @@ -1727,7 +1739,10 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
> >                amdgpu_gmc_sysvm_location(adev, mc);
> >        } else {
> >                amdgpu_gmc_vram_location(adev, mc, base);
> > -             amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> > +             if (!adev->gmc.enable_pdb0)
> > +                     amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> > +             else
> > +                     amdgpu_gmc_sysvm_location(adev, mc);
> >                if (!amdgpu_sriov_vf(adev) && (amdgpu_agp == 1))
> >                        amdgpu_gmc_agp_location(adev, mc);
> >        }
> > @@ -1838,14 +1853,6 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
> >                return 0;
> >        }
> >
> > -     if (adev->gmc.xgmi.connected_to_cpu) {
> > -             adev->gmc.vmid0_page_table_depth = 1;
> > -             adev->gmc.vmid0_page_table_block_size = 12;
> > -     } else {
> > -             adev->gmc.vmid0_page_table_depth = 0;
> > -             adev->gmc.vmid0_page_table_block_size = 0;
> > -     }
> > -
> >        /* Initialize common gart structure */
> >        r = amdgpu_gart_init(adev);
> >        if (r)
> > @@ -1864,7 +1871,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
> >                if (r)
> >                        return r;
> >
> > -             if (adev->gmc.xgmi.connected_to_cpu)
> > +             if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)
>
> Drop the connected_to_cpu check here.
>
> >                        r = amdgpu_gmc_pdb0_alloc(adev);
> >        }
> >
> > @@ -2361,7 +2368,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
> >  {
> >        int r;
> >
> > -     if (adev->gmc.xgmi.connected_to_cpu)
> > +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)
>
> And here.
>
> >                amdgpu_gmc_init_pdb0(adev);
> >
> >        if (adev->gart.bo == NULL) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > index fe0710b55c3a..13b229d07ac4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > @@ -74,27 +74,47 @@ static void mmhub_v9_4_setup_hubid_vm_pt_regs(struct amdgpu_device *adev, int hu
> >  static void mmhub_v9_4_init_gart_aperture_regs(struct amdgpu_device *adev,
> >                                               int hubid)
> >  {
> > -     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gart.bo);
> > +     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ? adev->gmc.pdb0_bo : adev->gart.bo);
>
> That can be written as adev->gmc.pdb0_bo ?: adev->gart.bo
>
> >
> >        mmhub_v9_4_setup_hubid_vm_pt_regs(adev, hubid, 0, pt_base);
> >
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +     if (adev->gmc.pdb0_bo) {
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.fb_start >> 12));
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.fb_start >> 44));
> > +
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.gart_end >> 12));
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.gart_end >> 44));
> > +     } else {
> > ++            WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_start >> 12));
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_start >> 44));
>
> When you indent the WREG32_SOC15_OFFSET() you need to indent the following lines as well.
>
> >
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_end >> 12));
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_end >> 44));
> > +     }
>
> The programming of the end addr is still the same, you don't need to change anything here.
>
> Regards,
> Christian.
>
> >  }
> >
> >  static void mmhub_v9_4_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,
>

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

RE: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Zhang, Owen(SRDC)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi, @Koenig, Christian<mailto:Christian.Koenig@amd.com>

Your time response will be highly appreciated. Thanks for support.


Rgds/Owen

From: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
Sent: Thursday, April 24, 2025 11:38 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>; Chang, HaiJun <HaiJun.Chang@amd.com>; Deng, Emily <Emily.Deng@amd.com>; Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV


[AMD Official Use Only - AMD Internal Distribution Only]

Ping... @Koenig, Christian<mailto:Christian.Koenig@amd.com>

Thanks
Sam

From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
Date: Wednesday, April 23, 2025 at 15:25
To: Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[AMD Official Use Only - AMD Internal Distribution Only]


[AMD Official Use Only - AMD Internal Distribution Only]

Hi Christian,

Thank you for the review and the feedback. I will update the patch according to your feedback.
Please see my 2 inline comments below.

Regards
Sam


> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Date: Wednesday, April 16, 2025 at 21:52
> To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
> Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
> Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> > When switching to new GPU index after hibernation and then resume,
> > VRAM offset of each VRAM BO will be changed, and the cached gpu
> > addresses needed to updated.
> >
> > This is to enable pdb0 and switch to use pdb0-based virtual gpu
> > address by default in amdgpu_bo_create_reserved(). since the virtual
> > addresses do not change, this can avoid the need to update all
> > cached gpu addresses all over the codebase.
> >
> > Signed-off-by: Emily Deng <Emily.Deng@amd.com>
> > Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> > Change-Id: I2b20b9b94f1e41820a013ce5d05bb3fa96859b21
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 43 +++++++++++++++-------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
> >  drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
> >  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 29 +++++++++------
> >  drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
> >  6 files changed, 82 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > index 5b60d714e089..e706afcb7e95 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> > @@ -248,18 +248,25 @@ void amdgpu_gmc_vram_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc,
> >  void amdgpu_gmc_sysvm_location(struct amdgpu_device *adev, struct amdgpu_gmc *mc)
> >  {
> >        u64 hive_vram_start = 0;
> > -     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes - 1;
> > -     mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> > -     mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> > +     u64 hive_vram_end = mc->xgmi.node_segment_size * mc->xgmi.num_physical_nodes;
> > +
> > +     hive_vram_end = ALIGN(hive_vram_end, (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21) - 1;
> > +
> > +     if (!mc->vram_start) {
> > +             mc->vram_start = mc->xgmi.node_segment_size * mc->xgmi.physical_node_id;
> > +             mc->vram_end = mc->vram_start + mc->xgmi.node_segment_size - 1;
> > +             dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> > +                     mc->mc_vram_size >> 20, mc->vram_start,
> > +                     mc->vram_end, mc->real_vram_size >> 20);
> > +     }
> > +
> >        mc->gart_start = hive_vram_end + 1;
> >        mc->gart_end = mc->gart_start + mc->gart_size - 1;
> >        mc->fb_start = hive_vram_start;
> >        mc->fb_end = hive_vram_end;
> > -     dev_info(adev->dev, "VRAM: %lluM 0x%016llX - 0x%016llX (%lluM used)\n",
> > -                     mc->mc_vram_size >> 20, mc->vram_start,
> > -                     mc->vram_end, mc->real_vram_size >> 20);
> > -     dev_info(adev->dev, "GART: %lluM 0x%016llX - 0x%016llX\n",
> > -                     mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> > +
> > +     dev_info(adev->dev, "FB 0x%016llX - 0x%016llX, GART: %lluM 0x%016llX - 0x%016llX\n",
> > +                     mc->fb_start, mc->fb_end, mc->gart_size >> 20, mc->gart_start, mc->gart_end);
> >  }
> >
> >  /**
> > @@ -677,8 +684,9 @@ void amdgpu_gmc_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,
> >                                     &job);
> >        if (r)
> >                goto error_alloc;
> > -
> > -     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gart.bo);
> > +     job->vm_pd_addr = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ?
> > +                                                     adev->gmc.pdb0_bo :
> > +                                                     adev->gart.bo);
> >        job->vm_needs_flush = true;
> >        job->ibs->ptr[job->ibs->length_dw++] = ring->funcs->nop;
> >        amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> > @@ -1041,8 +1049,9 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
> >         */
> >        u64 vram_size = adev->gmc.xgmi.node_segment_size * adev->gmc.xgmi.num_physical_nodes;
> >        u64 pde0_page_size = (1ULL<<adev->gmc.vmid0_page_table_block_size)<<21;
> > -     u64 vram_addr = adev->vm_manager.vram_base_offset -
> > +     u64 vram_addr_first = adev->vm_manager.vram_base_offset -
> >                adev->gmc.xgmi.physical_node_id * adev->gmc.xgmi.node_segment_size;
> > +     u64 vram_addr = adev->vm_manager.vram_base_offset;
> >        u64 vram_end = vram_addr + vram_size;
> >        u64 gart_ptb_gpu_pa = amdgpu_gmc_vram_pa(adev, adev->gart.bo);
> >        int idx;
> > @@ -1056,11 +1065,19 @@ void amdgpu_gmc_init_pdb0(struct amdgpu_device *adev)
> >        flags |= AMDGPU_PTE_FRAG((adev->gmc.vmid0_page_table_block_size + 9*1));
> >        flags |= AMDGPU_PDE_PTE_FLAG(adev);
> >
> > +     if (adev->gmc.xgmi.connected_to_cpu) {
> > +             vram_addr = vram_addr_first;
> > +             vram_end = vram_addr + vram_size;
> > +     }
> > +
> >        /* The first n PDE0 entries are used as PTE,
> >         * pointing to vram
> >         */
> > -     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size)
> > -             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i, vram_addr, flags);
> > +     for (i = 0; vram_addr < vram_end; i++, vram_addr += pde0_page_size) {
> > +             amdgpu_gmc_set_pte_pde(adev, adev->gmc.ptr_pdb0, i,
> > +                     (vram_addr >= vram_addr_first + vram_size) ? (vram_addr - vram_size) : vram_addr,
> > +                     flags);
> > +     }
> >
> >        /* The n+1'th PDE0 entry points to a huge
> >         * PTB who has more than 512 entries each
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > index bd7fc123b8f9..758b47240c6f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h
> > @@ -307,6 +307,7 @@ struct amdgpu_gmc {
> >        struct amdgpu_bo                *pdb0_bo;
> >        /* CPU kmapped address of pdb0*/
> >        void                            *ptr_pdb0;
> > +     bool enable_pdb0;
> >
> >        /* MALL size */
> >        u64 mall_size;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index d90e9daf5a50..83a3444c69d9 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
> >                goto error_unpin;
> >        }
> >
> > -     if (gpu_addr)
> > +     if (gpu_addr) {
> >                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
> > +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
> > +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
> > +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
> > +                     }
> > +             }
> > +     }
>
> Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.

This change is to change to the default GPU address from FB aperture type to pdb0 type in this centralized place so that I don't need to change every callsite of amdgpu_bo_create_reserved().
Could you suggest a better approach if this approach is not acceptable?


>
>
> >
> >        if (cpu_addr) {
> >                r = amdgpu_bo_kmap(*bo_ptr, cpu_addr);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > index cb25f7f0dfc1..5ebb92ac9fd7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c
> > @@ -180,7 +180,7 @@ gfxhub_v1_2_xcc_init_system_aperture_regs(struct amdgpu_device *adev,
> >                /* In the case squeezing vram into GART aperture, we don't use
> >                 * FB aperture and AGP aperture. Disable them.
> >                 */
> > -             if (adev->gmc.pdb0_bo) {
> > +             if (adev->gmc.pdb0_bo && !amdgpu_sriov_vf(adev)) {
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_TOP, 0);
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_FB_LOCATION_BASE, 0x00FFFFFF);
> >                        WREG32_SOC15(GC, GET_INST(GC, i), regMC_VM_AGP_TOP, 0);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > index 7c7a9fe6be6d..73ac05b9a1bf 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > @@ -1677,6 +1677,10 @@ static int gmc_v9_0_early_init(struct amdgpu_ip_block *ip_block)
> >                adev->gmc.private_aperture_start + (4ULL << 30) - 1;
> >        adev->gmc.noretry_flags = AMDGPU_VM_NORETRY_FLAGS_TF;
> >
> > +     if (amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 3) ||
> > +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4, 4) ||
> > +         amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 5, 0))
> > +         adev->gmc.enable_pdb0 = amdgpu_sriov_vf(adev);
> >        return 0;
> >  }
> >
> > @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
> >  {
> >        u64 base = adev->mmhub.funcs->get_fb_location(adev);
> >
> > +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
> > +             adev->gmc.vmid0_page_table_depth = 1;
> > +             adev->gmc.vmid0_page_table_block_size = 12;
> > +     } else {
> > +             adev->gmc.vmid0_page_table_depth = 0;
> > +             adev->gmc.vmid0_page_table_block_size = 0;
> > +     }
> > +
>
> What is the justification to moving that stuff around?

vmid0_page_table_block_size is used in new code in amdgpu_gmc_sysvm_location(). See the call sequence below.

gmc_v9_0_sw_init
- gmc_v9_0_mc_init
                - gmc_v9_0_vram_gtt_location,
                                - vmid0_page_table_block_size = 12, **new location**
                                - amdgpu_gmc_sysvm_location
                                                - use **vmid0_page_table_block_size**
- gmc_v9_0_gart_init,
                - assign vmid0_page_table_block_size, **old location**



>
> >        amdgpu_gmc_set_agp_default(adev, mc);
> >
> >        /* add the xgmi offset of the physical node */
> > @@ -1727,7 +1739,10 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
> >                amdgpu_gmc_sysvm_location(adev, mc);
> >        } else {
> >                amdgpu_gmc_vram_location(adev, mc, base);
> > -             amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> > +             if (!adev->gmc.enable_pdb0)
> > +                     amdgpu_gmc_gart_location(adev, mc, AMDGPU_GART_PLACEMENT_BEST_FIT);
> > +             else
> > +                     amdgpu_gmc_sysvm_location(adev, mc);
> >                if (!amdgpu_sriov_vf(adev) && (amdgpu_agp == 1))
> >                        amdgpu_gmc_agp_location(adev, mc);
> >        }
> > @@ -1838,14 +1853,6 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
> >                return 0;
> >        }
> >
> > -     if (adev->gmc.xgmi.connected_to_cpu) {
> > -             adev->gmc.vmid0_page_table_depth = 1;
> > -             adev->gmc.vmid0_page_table_block_size = 12;
> > -     } else {
> > -             adev->gmc.vmid0_page_table_depth = 0;
> > -             adev->gmc.vmid0_page_table_block_size = 0;
> > -     }
> > -
> >        /* Initialize common gart structure */
> >        r = amdgpu_gart_init(adev);
> >        if (r)
> > @@ -1864,7 +1871,7 @@ static int gmc_v9_0_gart_init(struct amdgpu_device *adev)
> >                if (r)
> >                        return r;
> >
> > -             if (adev->gmc.xgmi.connected_to_cpu)
> > +             if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)
>
> Drop the connected_to_cpu check here.
>
> >                        r = amdgpu_gmc_pdb0_alloc(adev);
> >        }
> >
> > @@ -2361,7 +2368,7 @@ static int gmc_v9_0_gart_enable(struct amdgpu_device *adev)
> >  {
> >        int r;
> >
> > -     if (adev->gmc.xgmi.connected_to_cpu)
> > +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0)
>
> And here.
>
> >                amdgpu_gmc_init_pdb0(adev);
> >
> >        if (adev->gart.bo == NULL) {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > index fe0710b55c3a..13b229d07ac4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c
> > @@ -74,27 +74,47 @@ static void mmhub_v9_4_setup_hubid_vm_pt_regs(struct amdgpu_device *adev, int hu
> >  static void mmhub_v9_4_init_gart_aperture_regs(struct amdgpu_device *adev,
> >                                               int hubid)
> >  {
> > -     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gart.bo);
> > +     uint64_t pt_base = amdgpu_gmc_pd_addr(adev->gmc.pdb0_bo ? adev->gmc.pdb0_bo : adev->gart.bo);
>
> That can be written as adev->gmc.pdb0_bo ?: adev->gart.bo
>
> >
> >        mmhub_v9_4_setup_hubid_vm_pt_regs(adev, hubid, 0, pt_base);
> >
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +     if (adev->gmc.pdb0_bo) {
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.fb_start >> 12));
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.fb_start >> 44));
> > +
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.gart_end >> 12));
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> > +                                 mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> > +                                 hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> > +                                 (u32)(adev->gmc.gart_end >> 44));
> > +     } else {
> > ++            WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_LO32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_start >> 12));
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_START_ADDR_HI32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_start >> 44));
>
> When you indent the WREG32_SOC15_OFFSET() you need to indent the following lines as well.
>
> >
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_LO32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_end >> 12));
> > -     WREG32_SOC15_OFFSET(MMHUB, 0,
> > +             WREG32_SOC15_OFFSET(MMHUB, 0,
> >                            mmVML2VC0_VM_CONTEXT0_PAGE_TABLE_END_ADDR_HI32,
> >                            hubid * MMHUB_INSTANCE_REGISTER_OFFSET,
> >                            (u32)(adev->gmc.gart_end >> 44));
> > +     }
>
> The programming of the end addr is still the same, you don't need to change anything here.
>
> Regards,
> Christian.
>
> >  }
> >
> >  static void mmhub_v9_4_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,
>

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

Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Christian König]

On 4/24/25 05:38, Zhang, GuoQing (Sam) wrote:
> Hi Christian,
> 
> Thank you for the review and the feedback.I will update the patch according to 
> your feedback.
> 
> Please see my 2 inline comments below.

Please make sure to always CC my work mail address, otherwise I will only take a look the next time I work through the mailing lists.
> 
>> > index d90e9daf5a50..83a3444c69d9 100644
> 
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> 
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> 
>> > @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
> 
>> >                goto error_unpin;
> 
>> >        }
> 
>> >  
> 
>> > -     if (gpu_addr)
> 
>> > +     if (gpu_addr) {
> 
>> >                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
> 
>> > +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
> 
>> > +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
> 
>> > +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
> 
>> > +                     }
> 
>> > +             }
> 
>> > +     }
> 
>> 
> 
>> Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.
> 
> This change is to change to the default GPU address from FB aperture type to 
> pdb0 type in this centralized place so that I don’t need to change every 
> callsite of amdgpu_bo_create_reserved().
> 
> Could you suggest a better approach if this approach is not acceptable?


The whole code is completely superflous. When PDB0 is used the vram_start is adjusted and you don't need to do anything here.

See function amdgpu_gmc_sysvm_location(). You probably need to adjust that to have a static setup instead of using the XGMI node infos.


>> > @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
> 
>> >  {
> 
>> >        u64 base = adev->mmhub.funcs->get_fb_location(adev);
> 
>> >  
> 
>> > +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
> 
>> > +             adev->gmc.vmid0_page_table_depth = 1;
> 
>> > +             adev->gmc.vmid0_page_table_block_size = 12;
> 
>> > +     } else {
> 
>> > +             adev->gmc.vmid0_page_table_depth = 0;
> 
>> > +             adev->gmc.vmid0_page_table_block_size = 0;
> 
>> > +     }
> 
>> > +
> 
>> 
> 
>> What is the justification to moving that stuff around?
> 
> vmid0_page_table_block_size is used in new code in amdgpu_gmc_sysvm_location(). 
> See the call sequence below.
> 
> gmc_v9_0_sw_init
> 
> - gmc_v9_0_mc_init
> 
>                  - gmc_v9_0_vram_gtt_location,
> 
>                                  - vmid0_page_table_block_size = 12, **new 
> location**
> 
>                                  - amdgpu_gmc_sysvm_location
> 
>                                                  - use 
> **vmid0_page_table_block_size**
> 
> - gmc_v9_0_gart_init,
> 
>                  - assign vmid0_page_table_block_size, **old location**


That is noteven remotely corect.

See the code in gmc_v9_0_vram_gtt_location(). You use amdgpu_gmc_sysvm_location() when PDB0 is allocted and you use gmc_v9_0_vram_gtt_location() when it isn't.

But adjusting this function here doesn't make any sense at all.

Regards,
Christian.

Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi @Koenig, Christian<mailto:Christian.Koenig@amd.com>,

Thank you for the feedback. I have revised the patch according to your suggestions and sent out the v2 patch list. Please help review. Thank you!

mail titles of v2 patchlist:
[PATCH v2 0/3] enable switching to new gpu index for hibernate on SRIOV.
[PATCH v2 1/3] drm/amdgpu: update XGMI physical node id and GMC configs on resume
[PATCH v2 2/3] drm/amdgpu: update GPU addresses for SMU and PSP
[PATCH v2 3/3] drm/amdgpu: enable pdb0 for hibernation on SRIOV

Regards
Sam

From: Koenig, Christian <Christian.Koenig@amd.com>
Date: Monday, April 28, 2025 at 19:30
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>, Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV
On 4/24/25 05:38, Zhang, GuoQing (Sam) wrote:
> Hi Christian,
>
> Thank you for the review and the feedback.I will update the patch according to
> your feedback.
>
> Please see my 2 inline comments below.

Please make sure to always CC my work mail address, otherwise I will only take a look the next time I work through the mailing lists.
>
>> > index d90e9daf5a50..83a3444c69d9 100644
>
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>
>> > @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>
>> >                goto error_unpin;
>
>> >        }
>
>> >
>
>> > -     if (gpu_addr)
>
>> > +     if (gpu_addr) {
>
>> >                *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
>
>> > +             if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) {
>
>> > +                     if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) {
>
>> > +                             *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM);
>
>> > +                     }
>
>> > +             }
>
>> > +     }
>
>>
>
>> Please NAK to that approach here. The GPU offset should still point into the mapped VRAM.
>
> This change is to change to the default GPU address from FB aperture type to
> pdb0 type in this centralized place so that I don’t need to change every
> callsite of amdgpu_bo_create_reserved().
>
> Could you suggest a better approach if this approach is not acceptable?


The whole code is completely superflous. When PDB0 is used the vram_start is adjusted and you don't need to do anything here.

See function amdgpu_gmc_sysvm_location(). You probably need to adjust that to have a static setup instead of using the XGMI node infos.


>> > @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev,
>
>> >  {
>
>> >        u64 base = adev->mmhub.funcs->get_fb_location(adev);
>
>> >
>
>> > +     if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) {
>
>> > +             adev->gmc.vmid0_page_table_depth = 1;
>
>> > +             adev->gmc.vmid0_page_table_block_size = 12;
>
>> > +     } else {
>
>> > +             adev->gmc.vmid0_page_table_depth = 0;
>
>> > +             adev->gmc.vmid0_page_table_block_size = 0;
>
>> > +     }
>
>> > +
>
>>
>
>> What is the justification to moving that stuff around?
>
> vmid0_page_table_block_size is used in new code in amdgpu_gmc_sysvm_location().
> See the call sequence below.
>
> gmc_v9_0_sw_init
>
> - gmc_v9_0_mc_init
>
>                  - gmc_v9_0_vram_gtt_location,
>
>                                  - vmid0_page_table_block_size = 12, **new
> location**
>
>                                  - amdgpu_gmc_sysvm_location
>
>                                                  - use
> **vmid0_page_table_block_size**
>
> - gmc_v9_0_gart_init,
>
>                  - assign vmid0_page_table_block_size, **old location**


That is noteven remotely corect.

See the code in gmc_v9_0_vram_gtt_location(). You use amdgpu_gmc_sysvm_location() when PDB0 is allocted and you use gmc_v9_0_vram_gtt_location() when it isn't.

But adjusting this function here doesn't make any sense at all.

Regards,
Christian.

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

[PATCH 5/6] drm/amdgpu: fix sdma ring test fail when resume from hibernation

[Samuel Zhang]

gart tlb may be staled when switch to a new gpu index. this cause gpu
fetchs wrong data from gtt memory. Flush gart tlb at the end of gmc
resume to fix it.

Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
Change-Id: If2a3780319f5ecf3dcb0f1c07f85151ed65f522d
---
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 73ac05b9a1bf..0e7957b4389a 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -2543,7 +2543,7 @@ static int gmc_v9_0_resume(struct amdgpu_ip_block *ip_block)
 		return r;
 
 	amdgpu_vmid_reset_all(ip_block->adev);
-
+	amdgpu_gart_invalidate_tlb(adev);
 	return 0;
 }
 
-- 
2.43.5

Re: [PATCH 5/6] drm/amdgpu: fix sdma ring test fail when resume from hibernation

[Christian König]

Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> gart tlb may be staled when switch to a new gpu index. this cause gpu
> fetchs wrong data from gtt memory. Flush gart tlb at the end of gmc
> resume to fix it.

Well that's complete nonsense. When the TLB contains entries after a resume than that is a major bug in the hypervisor.

Christian.

>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> Change-Id: If2a3780319f5ecf3dcb0f1c07f85151ed65f522d
> ---
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 73ac05b9a1bf..0e7957b4389a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -2543,7 +2543,7 @@ static int gmc_v9_0_resume(struct amdgpu_ip_block *ip_block)
>  		return r;
>  
>  	amdgpu_vmid_reset_all(ip_block->adev);
> -
> +	amdgpu_gart_invalidate_tlb(adev);
>  	return 0;
>  }
>  

[PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Samuel Zhang]

IH is not working after switching a new gpu index for the first time.
IH handler function need to be re-registered with kernel after switching
to new gpu index.

Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
Change-Id: Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +
 drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 19ce4da285e8..2292245a0c5d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
 	return r;
 }
 
-void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
+void amdgpu_irq_uninstall(struct amdgpu_device *adev)
 {
 	if (adev->irq.installed) {
 		free_irq(adev->irq.irq, adev_to_drm(adev));
@@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
 		if (adev->irq.msi_enabled)
 			pci_free_irq_vectors(adev->pdev);
 	}
-
+}
+void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
+{
+	amdgpu_irq_uninstall(adev);
 	amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
 	amdgpu_ih_ring_fini(adev, &adev->irq.ih);
 	amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
index 04c0b4fa17a4..c6e6681b4f71 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
@@ -123,6 +123,7 @@ extern const int node_id_to_phys_map[NODEID_MAX];
 void amdgpu_irq_disable_all(struct amdgpu_device *adev);
 
 int amdgpu_irq_init(struct amdgpu_device *adev);
+void amdgpu_irq_uninstall(struct amdgpu_device *adev);
 void amdgpu_irq_fini_sw(struct amdgpu_device *adev);
 void amdgpu_irq_fini_hw(struct amdgpu_device *adev);
 int amdgpu_irq_add_id(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
index faa0dd75dd6d..ef996505e4dc 100644
--- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
@@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct amdgpu_ip_block *ip_block)
 
 static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)
 {
-	return vega20_ih_hw_fini(ip_block);
+	struct amdgpu_device *adev = ip_block->adev;
+	int r = 0;
+
+	r = vega20_ih_hw_fini(ip_block);
+	amdgpu_irq_uninstall(adev);
+	return r;
 }
 
 static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)
 {
-	return vega20_ih_hw_init(ip_block);
+	struct amdgpu_device *adev = ip_block->adev;
+	int r = 0;
+
+	r = amdgpu_irq_init(adev);
+	if (r) {
+		dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n", __func__, r);
+		return r;
+	}
+	r = vega20_ih_hw_init(ip_block);
+	return r;
 }
 
 static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)
-- 
2.43.5

Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Christian König]

Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> IH is not working after switching a new gpu index for the first time.
> IH handler function need to be re-registered with kernel after switching
> to new gpu index.

Why?

Christian.

>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> Change-Id: Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--
>  3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 19ce4da285e8..2292245a0c5d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>  	return r;
>  }
>  
> -void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
> +void amdgpu_irq_uninstall(struct amdgpu_device *adev)
>  {
>  	if (adev->irq.installed) {
>  		free_irq(adev->irq.irq, adev_to_drm(adev));
> @@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>  		if (adev->irq.msi_enabled)
>  			pci_free_irq_vectors(adev->pdev);
>  	}
> -
> +}
> +void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
> +{
> +	amdgpu_irq_uninstall(adev);
>  	amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
>  	amdgpu_ih_ring_fini(adev, &adev->irq.ih);
>  	amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> index 04c0b4fa17a4..c6e6681b4f71 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> @@ -123,6 +123,7 @@ extern const int node_id_to_phys_map[NODEID_MAX];
>  void amdgpu_irq_disable_all(struct amdgpu_device *adev);
>  
>  int amdgpu_irq_init(struct amdgpu_device *adev);
> +void amdgpu_irq_uninstall(struct amdgpu_device *adev);
>  void amdgpu_irq_fini_sw(struct amdgpu_device *adev);
>  void amdgpu_irq_fini_hw(struct amdgpu_device *adev);
>  int amdgpu_irq_add_id(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> index faa0dd75dd6d..ef996505e4dc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> @@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct amdgpu_ip_block *ip_block)
>  
>  static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)
>  {
> -	return vega20_ih_hw_fini(ip_block);
> +	struct amdgpu_device *adev = ip_block->adev;
> +	int r = 0;
> +
> +	r = vega20_ih_hw_fini(ip_block);
> +	amdgpu_irq_uninstall(adev);
> +	return r;
>  }
>  
>  static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)
>  {
> -	return vega20_ih_hw_init(ip_block);
> +	struct amdgpu_device *adev = ip_block->adev;
> +	int r = 0;
> +
> +	r = amdgpu_irq_init(adev);
> +	if (r) {
> +		dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n", __func__, r);
> +		return r;
> +	}
> +	r = vega20_ih_hw_init(ip_block);
> +	return r;
>  }
>  
>  static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)

Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi @Christian König<mailto:ckoenig.leichtzumerken@gmail.com>,

On QEMU VM environment, when request_irq() is called in guest KMD, QEMU will enable interrupt for the device on the host.

When hibernate and resume with a new vGPU without calling request_irq() on the new vGPU, the interrupt of the new vGPU is not enabled. The IH handler in guest KMD will not be called in this case.

This change is to ensure request_irq() is called on resume for the new vGPUs.

Regards
Sam

From: Christian König <ckoenig.leichtzumerken@gmail.com>
Date: Wednesday, April 16, 2025 at 21:54
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error
Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> IH is not working after switching a new gpu index for the first time.
> IH handler function need to be re-registered with kernel after switching
> to new gpu index.

Why?

Christian.

>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> Change-Id: Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--
>  3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 19ce4da285e8..2292245a0c5d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>        return r;
>  }
>
> -void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
> +void amdgpu_irq_uninstall(struct amdgpu_device *adev)
>  {
>        if (adev->irq.installed) {
>                free_irq(adev->irq.irq, adev_to_drm(adev));
> @@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>                if (adev->irq.msi_enabled)
>                        pci_free_irq_vectors(adev->pdev);
>        }
> -
> +}
> +void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
> +{
> +     amdgpu_irq_uninstall(adev);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> index 04c0b4fa17a4..c6e6681b4f71 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> @@ -123,6 +123,7 @@ extern const int node_id_to_phys_map[NODEID_MAX];
>  void amdgpu_irq_disable_all(struct amdgpu_device *adev);
>
>  int amdgpu_irq_init(struct amdgpu_device *adev);
> +void amdgpu_irq_uninstall(struct amdgpu_device *adev);
>  void amdgpu_irq_fini_sw(struct amdgpu_device *adev);
>  void amdgpu_irq_fini_hw(struct amdgpu_device *adev);
>  int amdgpu_irq_add_id(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> index faa0dd75dd6d..ef996505e4dc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> @@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct amdgpu_ip_block *ip_block)
>
>  static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)
>  {
> -     return vega20_ih_hw_fini(ip_block);
> +     struct amdgpu_device *adev = ip_block->adev;
> +     int r = 0;
> +
> +     r = vega20_ih_hw_fini(ip_block);
> +     amdgpu_irq_uninstall(adev);
> +     return r;
>  }
>
>  static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)
>  {
> -     return vega20_ih_hw_init(ip_block);
> +     struct amdgpu_device *adev = ip_block->adev;
> +     int r = 0;
> +
> +     r = amdgpu_irq_init(adev);
> +     if (r) {
> +             dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n", __func__, r);
> +             return r;
> +     }
> +     r = vega20_ih_hw_init(ip_block);
> +     return r;
>  }
>
>  static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)

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

Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Ping… @Koenig, Christian<mailto:Christian.Koenig@amd.com>

Thanks
Sam

From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
Date: Wednesday, April 23, 2025 at 14:59
To: Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[AMD Official Use Only - AMD Internal Distribution Only]


[AMD Official Use Only - AMD Internal Distribution Only]

Hi @Christian König<mailto:ckoenig.leichtzumerken@gmail.com>,

On QEMU VM environment, when request_irq() is called in guest KMD, QEMU will enable interrupt for the device on the host.

When hibernate and resume with a new vGPU without calling request_irq() on the new vGPU, the interrupt of the new vGPU is not enabled. The IH handler in guest KMD will not be called in this case.

This change is to ensure request_irq() is called on resume for the new vGPUs.

Regards
Sam

From: Christian König <ckoenig.leichtzumerken@gmail.com>
Date: Wednesday, April 16, 2025 at 21:54
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error
Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> IH is not working after switching a new gpu index for the first time.
> IH handler function need to be re-registered with kernel after switching
> to new gpu index.

Why?

Christian.

>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
> Change-Id: Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--
>  3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 19ce4da285e8..2292245a0c5d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>        return r;
>  }
>
> -void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
> +void amdgpu_irq_uninstall(struct amdgpu_device *adev)
>  {
>        if (adev->irq.installed) {
>                free_irq(adev->irq.irq, adev_to_drm(adev));
> @@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>                if (adev->irq.msi_enabled)
>                        pci_free_irq_vectors(adev->pdev);
>        }
> -
> +}
> +void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
> +{
> +     amdgpu_irq_uninstall(adev);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> index 04c0b4fa17a4..c6e6681b4f71 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> @@ -123,6 +123,7 @@ extern const int node_id_to_phys_map[NODEID_MAX];
>  void amdgpu_irq_disable_all(struct amdgpu_device *adev);
>
>  int amdgpu_irq_init(struct amdgpu_device *adev);
> +void amdgpu_irq_uninstall(struct amdgpu_device *adev);
>  void amdgpu_irq_fini_sw(struct amdgpu_device *adev);
>  void amdgpu_irq_fini_hw(struct amdgpu_device *adev);
>  int amdgpu_irq_add_id(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> index faa0dd75dd6d..ef996505e4dc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> @@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct amdgpu_ip_block *ip_block)
>
>  static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)
>  {
> -     return vega20_ih_hw_fini(ip_block);
> +     struct amdgpu_device *adev = ip_block->adev;
> +     int r = 0;
> +
> +     r = vega20_ih_hw_fini(ip_block);
> +     amdgpu_irq_uninstall(adev);
> +     return r;
>  }
>
>  static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)
>  {
> -     return vega20_ih_hw_init(ip_block);
> +     struct amdgpu_device *adev = ip_block->adev;
> +     int r = 0;
> +
> +     r = amdgpu_irq_init(adev);
> +     if (r) {
> +             dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n", __func__, r);
> +             return r;
> +     }
> +     r = vega20_ih_hw_init(ip_block);
> +     return r;
>  }
>
>  static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)

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

RE: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Zhang, Owen(SRDC)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi, @Koenig, Christian<mailto:Christian.Koenig@amd.com>

Looking for your expertise... Thanks for support.


Rgds/Owen

From: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
Sent: Thursday, April 24, 2025 11:39 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org; Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>; Chang, HaiJun <HaiJun.Chang@amd.com>; Deng, Emily <Emily.Deng@amd.com>; Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error


[AMD Official Use Only - AMD Internal Distribution Only]

Ping... @Koenig, Christian<mailto:Christian.Koenig@amd.com>

Thanks
Sam

From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com<mailto:GuoQing.Zhang@amd.com>>
Date: Wednesday, April 23, 2025 at 14:59
To: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>, amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Cc: Zhao, Victor <Victor.Zhao@amd.com<mailto:Victor.Zhao@amd.com>>, Chang, HaiJun <HaiJun.Chang@amd.com<mailto:HaiJun.Chang@amd.com>>, Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com<mailto:Owen.Zhang2@amd.com>>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[AMD Official Use Only - AMD Internal Distribution Only]


[AMD Official Use Only - AMD Internal Distribution Only]

Hi @Christian König<mailto:ckoenig.leichtzumerken@gmail.com>,

On QEMU VM environment, when request_irq() is called in guest KMD, QEMU will enable interrupt for the device on the host.

When hibernate and resume with a new vGPU without calling request_irq() on the new vGPU, the interrupt of the new vGPU is not enabled. The IH handler in guest KMD will not be called in this case.

This change is to ensure request_irq() is called on resume for the new vGPUs.

Regards
Sam

From: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>
Date: Wednesday, April 16, 2025 at 21:54
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com<mailto:GuoQing.Zhang@amd.com>>, amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Cc: Zhao, Victor <Victor.Zhao@amd.com<mailto:Victor.Zhao@amd.com>>, Chang, HaiJun <HaiJun.Chang@amd.com<mailto:HaiJun.Chang@amd.com>>, Deng, Emily <Emily.Deng@amd.com<mailto:Emily.Deng@amd.com>>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error
Am 14.04.25 um 12:46 schrieb Samuel Zhang:
> IH is not working after switching a new gpu index for the first time.
> IH handler function need to be re-registered with kernel after switching
> to new gpu index.

Why?

Christian.

>
> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com<mailto:guoqing.zhang@amd.com>>
> Change-Id: Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--
>  3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index 19ce4da285e8..2292245a0c5d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>        return r;
>  }
>
> -void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
> +void amdgpu_irq_uninstall(struct amdgpu_device *adev)
>  {
>        if (adev->irq.installed) {
>                free_irq(adev->irq.irq, adev_to_drm(adev));
> @@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>                if (adev->irq.msi_enabled)
>                        pci_free_irq_vectors(adev->pdev);
>        }
> -
> +}
> +void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
> +{
> +     amdgpu_irq_uninstall(adev);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih);
>        amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> index 04c0b4fa17a4..c6e6681b4f71 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> @@ -123,6 +123,7 @@ extern const int node_id_to_phys_map[NODEID_MAX];
>  void amdgpu_irq_disable_all(struct amdgpu_device *adev);
>
>  int amdgpu_irq_init(struct amdgpu_device *adev);
> +void amdgpu_irq_uninstall(struct amdgpu_device *adev);
>  void amdgpu_irq_fini_sw(struct amdgpu_device *adev);
>  void amdgpu_irq_fini_hw(struct amdgpu_device *adev);
>  int amdgpu_irq_add_id(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> index faa0dd75dd6d..ef996505e4dc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
> @@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct amdgpu_ip_block *ip_block)
>
>  static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)
>  {
> -     return vega20_ih_hw_fini(ip_block);
> +     struct amdgpu_device *adev = ip_block->adev;
> +     int r = 0;
> +
> +     r = vega20_ih_hw_fini(ip_block);
> +     amdgpu_irq_uninstall(adev);
> +     return r;
>  }
>
>  static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)
>  {
> -     return vega20_ih_hw_init(ip_block);
> +     struct amdgpu_device *adev = ip_block->adev;
> +     int r = 0;
> +
> +     r = amdgpu_irq_init(adev);
> +     if (r) {
> +             dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n", __func__, r);
> +             return r;
> +     }
> +     r = vega20_ih_hw_init(ip_block);
> +     return r;
>  }
>
>  static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)

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

Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Christian König]

On 4/24/25 05:38, Zhang, GuoQing (Sam) wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> 
> Ping… @Koenig, Christian <mailto:Christian.Koenig@amd.com>
> 
> Thanks
> 
> Sam
> 
> *From: *amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Zhang, 
> GuoQing (Sam) <GuoQing.Zhang@amd.com>
> *Date: *Wednesday, April 23, 2025 at 14:59
> *To: *Christian König <ckoenig.leichtzumerken@gmail.com>, amd- 
> gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, 
> Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
> *Subject: *Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error
> 
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi @Christian König <mailto:ckoenig.leichtzumerken@gmail.com>,
> 
> On QEMU VM environment, when request_irq() is called in guest KMD, QEMU will 
> enable interrupt for the device on the host.
> 
> When hibernate and resume with a new vGPU without calling request_irq() on the 
> new vGPU, the interrupt of the new vGPU is not enabled. The IH handler in guest 
> KMD will not be called in this case.
> 
> This change is to ensure request_irq() is called on resume for the new vGPUs.

That doesn't make sense.

The MSI state is saved and restored by the core OS on suspend and resume, drivers should never mess with that.

If this doesn't work with the new vGPU for some reason then that is not something we can work around inside the driver.

Which state exactly isn't restored here?

Regards,
Christian.


> 
> Regards
> 
> Sam
> 
> *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
> *Date: *Wednesday, April 16, 2025 at 21:54
> *To: *Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd- 
> gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, 
> Deng, Emily <Emily.Deng@amd.com>
> *Subject: *Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error
> 
> Am 14.04.25 um 12:46 schrieb Samuel Zhang:
>  > IH is not working after switching a new gpu index for the first time.
>  > IH handler function need to be re-registered with kernel after switching
>  > to new gpu index.
> 
> Why?
> 
> Christian.
> 
>  >
>  > Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
>  > Change-Id: Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
>  > ---
>  >  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--
>  >  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +
>  >  drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--
>  >  3 files changed, 22 insertions(+), 4 deletions(-)
>  >
>  > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/ 
> amdgpu/amdgpu_irq.c
>  > index 19ce4da285e8..2292245a0c5d 100644
>  > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>  > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>  > @@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>  >        return r;
>  >  }
>  >
>  > -void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>  > +void amdgpu_irq_uninstall(struct amdgpu_device *adev)
>  >  {
>  >        if (adev->irq.installed) {
>  >                free_irq(adev->irq.irq, adev_to_drm(adev));
>  > @@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>  >                if (adev->irq.msi_enabled)
>  >                        pci_free_irq_vectors(adev->pdev);
>  >        }
>  > -
>  > +}
>  > +void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>  > +{
>  > +     amdgpu_irq_uninstall(adev);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
>  > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h b/drivers/gpu/drm/amd/ 
> amdgpu/amdgpu_irq.h
>  > index 04c0b4fa17a4..c6e6681b4f71 100644
>  > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>  > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>  > @@ -123,6 +123,7 @@ extern const int node_id_to_phys_map[NODEID_MAX];
>  >  void amdgpu_irq_disable_all(struct amdgpu_device *adev);
>  >
>  >  int amdgpu_irq_init(struct amdgpu_device *adev);
>  > +void amdgpu_irq_uninstall(struct amdgpu_device *adev);
>  >  void amdgpu_irq_fini_sw(struct amdgpu_device *adev);
>  >  void amdgpu_irq_fini_hw(struct amdgpu_device *adev);
>  >  int amdgpu_irq_add_id(struct amdgpu_device *adev,
>  > diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/ 
> amdgpu/vega20_ih.c
>  > index faa0dd75dd6d..ef996505e4dc 100644
>  > --- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
>  > +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
>  > @@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct amdgpu_ip_block 
> *ip_block)
>  >
>  >  static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)
>  >  {
>  > -     return vega20_ih_hw_fini(ip_block);
>  > +     struct amdgpu_device *adev = ip_block->adev;
>  > +     int r = 0;
>  > +
>  > +     r = vega20_ih_hw_fini(ip_block);
>  > +     amdgpu_irq_uninstall(adev);
>  > +     return r;
>  >  }
>  >
>  >  static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)
>  >  {
>  > -     return vega20_ih_hw_init(ip_block);
>  > +     struct amdgpu_device *adev = ip_block->adev;
>  > +     int r = 0;
>  > +
>  > +     r = amdgpu_irq_init(adev);
>  > +     if (r) {
>  > +             dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n", 
> __func__, r);
>  > +             return r;
>  > +     }
>  > +     r = vega20_ih_hw_init(ip_block);
>  > +     return r;
>  >  }
>  >
>  >  static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)
> 

RE: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Chang, HaiJun]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

The interrupt issue loss issue might be VF msix table isn't restored properly on resume.

The msix table in virtual machine is faked.  The real msix table will be programmed by QEMU when guest enable/disable msix interrupt.  But QEMU accessing VF msix table (GFXMSIX_* registers) could be blocked by nBIF protection if that time VF isn't in exclusive access.
We had a w/a in amdgpu driver to handle msix table loss case in amdgpu_restore_msix function.

Can you check the values of these GFXMSIX_* registers?  I think we should call amdgpu_restore_msix on resume to restore msix table.

Thanks,
HaiJun

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Monday, April 28, 2025 8:24 PM
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>; Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>; Chang, HaiJun <HaiJun.Chang@amd.com>; Deng, Emily <Emily.Deng@amd.com>; Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

On 4/24/25 05:38, Zhang, GuoQing (Sam) wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
>
> Ping… @Koenig, Christian <mailto:Christian.Koenig@amd.com>
>
> Thanks
>
> Sam
>
> *From: *amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of
> Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
> *Date: *Wednesday, April 23, 2025 at 14:59
> *To: *Christian König <ckoenig.leichtzumerken@gmail.com>, amd-
> gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun
> <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang,
> Owen(SRDC) <Owen.Zhang2@amd.com>
> *Subject: *Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer
> expired error
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi @Christian König <mailto:ckoenig.leichtzumerken@gmail.com>,
>
> On QEMU VM environment, when request_irq() is called in guest KMD,
> QEMU will enable interrupt for the device on the host.
>
> When hibernate and resume with a new vGPU without calling
> request_irq() on the new vGPU, the interrupt of the new vGPU is not
> enabled. The IH handler in guest KMD will not be called in this case.
>
> This change is to ensure request_irq() is called on resume for the new vGPUs.

That doesn't make sense.

The MSI state is saved and restored by the core OS on suspend and resume, drivers should never mess with that.

If this doesn't work with the new vGPU for some reason then that is not something we can work around inside the driver.

Which state exactly isn't restored here?

Regards,
Christian.


>
> Regards
>
> Sam
>
> *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
> *Date: *Wednesday, April 16, 2025 at 21:54
> *To: *Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-
> gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun
> <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
> *Subject: *Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer
> expired error
>
> Am 14.04.25 um 12:46 schrieb Samuel Zhang:
>  > IH is not working after switching a new gpu index for the first time.
>  > IH handler function need to be re-registered with kernel after
> switching  > to new gpu index.
>
> Why?
>
> Christian.
>
>  >
>  > Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>  > Change-Id:
> Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
>  > ---
>  >  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--  >
> drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +  >
> drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--  >  3
> files changed, 22 insertions(+), 4 deletions(-)  >  > diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/
> amdgpu/amdgpu_irq.c  > index 19ce4da285e8..2292245a0c5d 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>  > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>  > @@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>  >        return r;
>  >  }
>  >
>  > -void amdgpu_irq_fini_hw(struct amdgpu_device *adev)  > +void
> amdgpu_irq_uninstall(struct amdgpu_device *adev)  >  {
>  >        if (adev->irq.installed) {
>  >                free_irq(adev->irq.irq, adev_to_drm(adev));
>  > @@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>  >                if (adev->irq.msi_enabled)
>  >                        pci_free_irq_vectors(adev->pdev);
>  >        }
>  > -
>  > +}
>  > +void amdgpu_irq_fini_hw(struct amdgpu_device *adev)  > +{
>  > +     amdgpu_irq_uninstall(adev);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
>  > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> b/drivers/gpu/drm/amd/ amdgpu/amdgpu_irq.h  > index
> 04c0b4fa17a4..c6e6681b4f71 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>  > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>  > @@ -123,6 +123,7 @@ extern const int
> node_id_to_phys_map[NODEID_MAX];  >  void
> amdgpu_irq_disable_all(struct amdgpu_device *adev);  >  >  int
> amdgpu_irq_init(struct amdgpu_device *adev);  > +void
> amdgpu_irq_uninstall(struct amdgpu_device *adev);  >  void
> amdgpu_irq_fini_sw(struct amdgpu_device *adev);  >  void
> amdgpu_irq_fini_hw(struct amdgpu_device *adev);  >  int
> amdgpu_irq_add_id(struct amdgpu_device *adev,  > diff --git
> a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/
> amdgpu/vega20_ih.c  > index faa0dd75dd6d..ef996505e4dc 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
>  > +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
>  > @@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct
> amdgpu_ip_block
> *ip_block)
>  >
>  >  static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)  >
> {
>  > -     return vega20_ih_hw_fini(ip_block);
>  > +     struct amdgpu_device *adev = ip_block->adev;
>  > +     int r = 0;
>  > +
>  > +     r = vega20_ih_hw_fini(ip_block);
>  > +     amdgpu_irq_uninstall(adev);
>  > +     return r;
>  >  }
>  >
>  >  static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)  >
> {
>  > -     return vega20_ih_hw_init(ip_block);
>  > +     struct amdgpu_device *adev = ip_block->adev;
>  > +     int r = 0;
>  > +
>  > +     r = amdgpu_irq_init(adev);
>  > +     if (r) {
>  > +             dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n",
> __func__, r);
>  > +             return r;
>  > +     }
>  > +     r = vega20_ih_hw_init(ip_block);
>  > +     return r;
>  >  }
>  >
>  >  static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)
>

Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi @Chang, HaiJun<mailto:HaiJun.Chang@amd.com>

Thank you for the info.
You are right, GFXMSIX_VECT0_ADDR_LO and GFXMSIX_VECT0_CONTROL registers are not restored on resume with new VF.

source VF, normal value
0x42000: 0xFEE00138 // GFXMSIX_VECT0_ADDR_LO
0x4200C: 0x00000000 // GFXMSIX_VECT0_CONTROL

destination VF, abnormal value
0x42000: 0x00000000 // GFXMSIX_VECT0_ADDR_LO
0x4200C: 0x00000001 // GFXMSIX_VECT0_CONTROL

Calling amdgpu_restore_msix() on resume can fix this issue. I will upload a new patch for this.

static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)
{
+       struct amdgpu_device *adev = ip_block->adev;
+
+       if (amdgpu_xmgi_is_node_changed(adev))
+               amdgpu_restore_msix(adev);
        return vega20_ih_hw_init(ip_block);
}

Thanks
Sam

From: Chang, HaiJun <HaiJun.Chang@amd.com>
Date: Tuesday, April 29, 2025 at 10:43
To: Koenig, Christian <Christian.Koenig@amd.com>, Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>, Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: RE: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error
[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

The interrupt issue loss issue might be VF msix table isn't restored properly on resume.

The msix table in virtual machine is faked.  The real msix table will be programmed by QEMU when guest enable/disable msix interrupt.  But QEMU accessing VF msix table (GFXMSIX_* registers) could be blocked by nBIF protection if that time VF isn't in exclusive access.
We had a w/a in amdgpu driver to handle msix table loss case in amdgpu_restore_msix function.

Can you check the values of these GFXMSIX_* registers?  I think we should call amdgpu_restore_msix on resume to restore msix table.

Thanks,
HaiJun

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Monday, April 28, 2025 8:24 PM
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>; Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>; Chang, HaiJun <HaiJun.Chang@amd.com>; Deng, Emily <Emily.Deng@amd.com>; Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

On 4/24/25 05:38, Zhang, GuoQing (Sam) wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
>
> Ping… @Koenig, Christian <mailto:Christian.Koenig@amd.com>
>
> Thanks
>
> Sam
>
> *From: *amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of
> Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
> *Date: *Wednesday, April 23, 2025 at 14:59
> *To: *Christian König <ckoenig.leichtzumerken@gmail.com>, amd-
> gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun
> <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang,
> Owen(SRDC) <Owen.Zhang2@amd.com>
> *Subject: *Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer
> expired error
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi @Christian König <mailto:ckoenig.leichtzumerken@gmail.com>,
>
> On QEMU VM environment, when request_irq() is called in guest KMD,
> QEMU will enable interrupt for the device on the host.
>
> When hibernate and resume with a new vGPU without calling
> request_irq() on the new vGPU, the interrupt of the new vGPU is not
> enabled. The IH handler in guest KMD will not be called in this case.
>
> This change is to ensure request_irq() is called on resume for the new vGPUs.

That doesn't make sense.

The MSI state is saved and restored by the core OS on suspend and resume, drivers should never mess with that.

If this doesn't work with the new vGPU for some reason then that is not something we can work around inside the driver.

Which state exactly isn't restored here?

Regards,
Christian.


>
> Regards
>
> Sam
>
> *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
> *Date: *Wednesday, April 16, 2025 at 21:54
> *To: *Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-
> gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun
> <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
> *Subject: *Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer
> expired error
>
> Am 14.04.25 um 12:46 schrieb Samuel Zhang:
>  > IH is not working after switching a new gpu index for the first time.
>  > IH handler function need to be re-registered with kernel after
> switching  > to new gpu index.
>
> Why?
>
> Christian.
>
>  >
>  > Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>  > Change-Id:
> Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
>  > ---
>  >  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--  >
> drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +  >
> drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--  >  3
> files changed, 22 insertions(+), 4 deletions(-)  >  > diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/
> amdgpu/amdgpu_irq.c  > index 19ce4da285e8..2292245a0c5d 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>  > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>  > @@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>  >        return r;
>  >  }
>  >
>  > -void amdgpu_irq_fini_hw(struct amdgpu_device *adev)  > +void
> amdgpu_irq_uninstall(struct amdgpu_device *adev)  >  {
>  >        if (adev->irq.installed) {
>  >                free_irq(adev->irq.irq, adev_to_drm(adev));
>  > @@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>  >                if (adev->irq.msi_enabled)
>  >                        pci_free_irq_vectors(adev->pdev);
>  >        }
>  > -
>  > +}
>  > +void amdgpu_irq_fini_hw(struct amdgpu_device *adev)  > +{
>  > +     amdgpu_irq_uninstall(adev);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
>  > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> b/drivers/gpu/drm/amd/ amdgpu/amdgpu_irq.h  > index
> 04c0b4fa17a4..c6e6681b4f71 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>  > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>  > @@ -123,6 +123,7 @@ extern const int
> node_id_to_phys_map[NODEID_MAX];  >  void
> amdgpu_irq_disable_all(struct amdgpu_device *adev);  >  >  int
> amdgpu_irq_init(struct amdgpu_device *adev);  > +void
> amdgpu_irq_uninstall(struct amdgpu_device *adev);  >  void
> amdgpu_irq_fini_sw(struct amdgpu_device *adev);  >  void
> amdgpu_irq_fini_hw(struct amdgpu_device *adev);  >  int
> amdgpu_irq_add_id(struct amdgpu_device *adev,  > diff --git
> a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/
> amdgpu/vega20_ih.c  > index faa0dd75dd6d..ef996505e4dc 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
>  > +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
>  > @@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct
> amdgpu_ip_block
> *ip_block)
>  >
>  >  static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)  >
> {
>  > -     return vega20_ih_hw_fini(ip_block);
>  > +     struct amdgpu_device *adev = ip_block->adev;
>  > +     int r = 0;
>  > +
>  > +     r = vega20_ih_hw_fini(ip_block);
>  > +     amdgpu_irq_uninstall(adev);
>  > +     return r;
>  >  }
>  >
>  >  static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)  >
> {
>  > -     return vega20_ih_hw_init(ip_block);
>  > +     struct amdgpu_device *adev = ip_block->adev;
>  > +     int r = 0;
>  > +
>  > +     r = amdgpu_irq_init(adev);
>  > +     if (r) {
>  > +             dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n",
> __func__, r);
>  > +             return r;
>  > +     }
>  > +     r = vega20_ih_hw_init(ip_block);
>  > +     return r;
>  >  }
>  >
>  >  static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)
>

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

Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Hi @Koenig, Christian<mailto:Christian.Koenig@amd.com>,

This issue only happens when switching to new GPU, namely detecting xmgi node is changed.
Should I include the check in this case? If yes, use the new amdgpu_xmgi_is_node_changed() or just amdgpu_sriov_vf()?
Please advise. Thank you!

Regards
Sam

From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
Date: Thursday, May 8, 2025 at 14:53
To: Chang, HaiJun <HaiJun.Chang@amd.com>, Koenig, Christian <Christian.Koenig@amd.com>, Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>, Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

[AMD Official Use Only - AMD Internal Distribution Only]


[AMD Official Use Only - AMD Internal Distribution Only]

Hi @Chang, HaiJun<mailto:HaiJun.Chang@amd.com>

Thank you for the info.
You are right, GFXMSIX_VECT0_ADDR_LO and GFXMSIX_VECT0_CONTROL registers are not restored on resume with new VF.

source VF, normal value
0x42000: 0xFEE00138 // GFXMSIX_VECT0_ADDR_LO
0x4200C: 0x00000000 // GFXMSIX_VECT0_CONTROL

destination VF, abnormal value
0x42000: 0x00000000 // GFXMSIX_VECT0_ADDR_LO
0x4200C: 0x00000001 // GFXMSIX_VECT0_CONTROL

Calling amdgpu_restore_msix() on resume can fix this issue. I will upload a new patch for this.

static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)
{
+       struct amdgpu_device *adev = ip_block->adev;
+
+       if (amdgpu_xmgi_is_node_changed(adev))
+               amdgpu_restore_msix(adev);
        return vega20_ih_hw_init(ip_block);
}

Thanks
Sam

From: Chang, HaiJun <HaiJun.Chang@amd.com>
Date: Tuesday, April 29, 2025 at 10:43
To: Koenig, Christian <Christian.Koenig@amd.com>, Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>, Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: RE: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error
[AMD Official Use Only - AMD Internal Distribution Only]

Hi,

The interrupt issue loss issue might be VF msix table isn't restored properly on resume.

The msix table in virtual machine is faked.  The real msix table will be programmed by QEMU when guest enable/disable msix interrupt.  But QEMU accessing VF msix table (GFXMSIX_* registers) could be blocked by nBIF protection if that time VF isn't in exclusive access.
We had a w/a in amdgpu driver to handle msix table loss case in amdgpu_restore_msix function.

Can you check the values of these GFXMSIX_* registers?  I think we should call amdgpu_restore_msix on resume to restore msix table.

Thanks,
HaiJun

-----Original Message-----
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Monday, April 28, 2025 8:24 PM
To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>; Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: Zhao, Victor <Victor.Zhao@amd.com>; Chang, HaiJun <HaiJun.Chang@amd.com>; Deng, Emily <Emily.Deng@amd.com>; Zhang, Owen(SRDC) <Owen.Zhang2@amd.com>
Subject: Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer expired error

On 4/24/25 05:38, Zhang, GuoQing (Sam) wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
>
> Ping… @Koenig, Christian <mailto:Christian.Koenig@amd.com>
>
> Thanks
>
> Sam
>
> *From: *amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of
> Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
> *Date: *Wednesday, April 23, 2025 at 14:59
> *To: *Christian König <ckoenig.leichtzumerken@gmail.com>, amd-
> gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun
> <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang,
> Owen(SRDC) <Owen.Zhang2@amd.com>
> *Subject: *Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer
> expired error
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi @Christian König <mailto:ckoenig.leichtzumerken@gmail.com>,
>
> On QEMU VM environment, when request_irq() is called in guest KMD,
> QEMU will enable interrupt for the device on the host.
>
> When hibernate and resume with a new vGPU without calling
> request_irq() on the new vGPU, the interrupt of the new vGPU is not
> enabled. The IH handler in guest KMD will not be called in this case.
>
> This change is to ensure request_irq() is called on resume for the new vGPUs.

That doesn't make sense.

The MSI state is saved and restored by the core OS on suspend and resume, drivers should never mess with that.

If this doesn't work with the new vGPU for some reason then that is not something we can work around inside the driver.

Which state exactly isn't restored here?

Regards,
Christian.


>
> Regards
>
> Sam
>
> *From: *Christian König <ckoenig.leichtzumerken@gmail.com>
> *Date: *Wednesday, April 16, 2025 at 21:54
> *To: *Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, amd-
> gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> *Cc: *Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun
> <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>
> *Subject: *Re: [PATCH 6/6] drm/amdgpu: fix fence fallback timer
> expired error
>
> Am 14.04.25 um 12:46 schrieb Samuel Zhang:
>  > IH is not working after switching a new gpu index for the first time.
>  > IH handler function need to be re-registered with kernel after
> switching  > to new gpu index.
>
> Why?
>
> Christian.
>
>  >
>  > Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>  > Change-Id:
> Idece1c8fce24032fd08f5a8b6ac23793c51e56dd
>  > ---
>  >  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c |  7 +++++--  >
> drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h |  1 +  >
> drivers/gpu/drm/amd/amdgpu/vega20_ih.c  | 18 ++++++++++++++++--  >  3
> files changed, 22 insertions(+), 4 deletions(-)  >  > diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c b/drivers/gpu/drm/amd/
> amdgpu/amdgpu_irq.c  > index 19ce4da285e8..2292245a0c5d 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>  > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
>  > @@ -326,7 +326,7 @@ int amdgpu_irq_init(struct amdgpu_device *adev)
>  >        return r;
>  >  }
>  >
>  > -void amdgpu_irq_fini_hw(struct amdgpu_device *adev)  > +void
> amdgpu_irq_uninstall(struct amdgpu_device *adev)  >  {
>  >        if (adev->irq.installed) {
>  >                free_irq(adev->irq.irq, adev_to_drm(adev));
>  > @@ -334,7 +334,10 @@ void amdgpu_irq_fini_hw(struct amdgpu_device *adev)
>  >                if (adev->irq.msi_enabled)
>  >                        pci_free_irq_vectors(adev->pdev);
>  >        }
>  > -
>  > +}
>  > +void amdgpu_irq_fini_hw(struct amdgpu_device *adev)  > +{
>  > +     amdgpu_irq_uninstall(adev);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih_soft);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih);
>  >        amdgpu_ih_ring_fini(adev, &adev->irq.ih1);
>  > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
> b/drivers/gpu/drm/amd/ amdgpu/amdgpu_irq.h  > index
> 04c0b4fa17a4..c6e6681b4f71 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>  > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h
>  > @@ -123,6 +123,7 @@ extern const int
> node_id_to_phys_map[NODEID_MAX];  >  void
> amdgpu_irq_disable_all(struct amdgpu_device *adev);  >  >  int
> amdgpu_irq_init(struct amdgpu_device *adev);  > +void
> amdgpu_irq_uninstall(struct amdgpu_device *adev);  >  void
> amdgpu_irq_fini_sw(struct amdgpu_device *adev);  >  void
> amdgpu_irq_fini_hw(struct amdgpu_device *adev);  >  int
> amdgpu_irq_add_id(struct amdgpu_device *adev,  > diff --git
> a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/
> amdgpu/vega20_ih.c  > index faa0dd75dd6d..ef996505e4dc 100644  > ---
> a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
>  > +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c
>  > @@ -643,12 +643,26 @@ static int vega20_ih_hw_fini(struct
> amdgpu_ip_block
> *ip_block)
>  >
>  >  static int vega20_ih_suspend(struct amdgpu_ip_block *ip_block)  >
> {
>  > -     return vega20_ih_hw_fini(ip_block);
>  > +     struct amdgpu_device *adev = ip_block->adev;
>  > +     int r = 0;
>  > +
>  > +     r = vega20_ih_hw_fini(ip_block);
>  > +     amdgpu_irq_uninstall(adev);
>  > +     return r;
>  >  }
>  >
>  >  static int vega20_ih_resume(struct amdgpu_ip_block *ip_block)  >
> {
>  > -     return vega20_ih_hw_init(ip_block);
>  > +     struct amdgpu_device *adev = ip_block->adev;
>  > +     int r = 0;
>  > +
>  > +     r = amdgpu_irq_init(adev);
>  > +     if (r) {
>  > +             dev_err(adev->dev, "amdgpu_irq_init failed in %s, %d\n",
> __func__, r);
>  > +             return r;
>  > +     }
>  > +     r = vega20_ih_hw_init(ip_block);
>  > +     return r;
>  >  }
>  >
>  >  static bool vega20_ih_is_idle(struct amdgpu_ip_block *ip_block)
>

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

Re: [PATCH 0/6] enable switching to new gpu index for hibernate on SRIOV.

[Zhang, GuoQing (Sam)]

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

[AMD Official Use Only - AMD Internal Distribution Only]

Ping…

Regards
Sam

From: Samuel Zhang <guoqing.zhang@amd.com>
Date: Monday, April 14, 2025 at 18:47
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>
Subject: [PATCH 0/6] enable switching to new gpu index for hibernate on SRIOV.
On SRIOV and VM environment, customer may need to switch to new vGPU indexes
after hibernate and then resume the VM. For GPUs with XGMI, `vram_start` will
change in this case, the VRAM aperture gpu address of VRAM BOs will also change.
These gpu addresses need to be updated when resume. But these addresses are all
over the KMD codebase, updating each of them is error-prone and not acceptable.

The solution is to use pdb0 page table to cover both vram and gart memory and
use pdb0 virtual gpu address instead. When gpu indexes change, the virtual gpu
address won't change.

For psp and smu, pdb0's gpu address does not work, so the original gpu address
is used instead. They need to be updated when resume with changed vGPUs.

The last 2 patches fix the issues we hit when testing this feature.

Samuel Zhang (6):
  drm/amdgpu: update XGMI physical node id and GMC configs on resume
  drm/amdgpu: update cached GPU addresses for PSP and ucode
  drm/amdgpu: update cached GPU addresses for SMU
  drm/amdgpu: enable pdb0 for hibernation on SRIOV
  drm/amdgpu: fix sdma ring test fail when resume from hibernation
  drm/amdgpu: fix fence fallback timer expired error

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 25 ++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c    | 45 +++++++++++++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.h    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c    |  7 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.h    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  8 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c    | 24 ++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c  |  3 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.h   |  2 +
 drivers/gpu/drm/amd/amdgpu/gfxhub_v1_2.c   |  2 +-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c      | 39 +++++++++++++------
 drivers/gpu/drm/amd/amdgpu/mmhub_v9_4.c    | 30 ++++++++++++---
 drivers/gpu/drm/amd/amdgpu/vega20_ih.c     | 18 ++++++++-
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c  | 30 +++++++++++++++
 14 files changed, 199 insertions(+), 36 deletions(-)

--
2.43.5

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