[PATCH Review 1/4] drm/amdgpu: Update smu driver interface for aldebaran

[PATCH Review 1/4] drm/amdgpu: Update smu driver interface for aldebaran

[Stanley.Yang]

update smu driver if version to 0x08 to avoid mismatch log
A version mismatch can still happen with an older FW

Change-Id: I97f2bc4ed9a9cba313b744e2ff6812c90b244935
Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
---
 .../drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 18 +++++++++++++++++-
 drivers/gpu/drm/amd/pm/inc/smu_v13_0.h         |  2 +-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h b/drivers/gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h
index a017983ff1fa..0f67c56c2863 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h
@@ -140,6 +140,8 @@
 
 #define MAX_SW_I2C_COMMANDS                24
 
+#define ALDEBARAN_UMC_CHANNEL_NUM    32
+
 typedef enum {
   I2C_CONTROLLER_PORT_0, //CKSVII2C0
   I2C_CONTROLLER_PORT_1, //CKSVII2C1
@@ -507,6 +509,19 @@ typedef struct {
   uint32_t MmHubPadding[8]; // SMU internal use
 } AvfsDebugTable_t;
 
+typedef struct {
+	uint64_t mca_umc_status;
+	uint64_t mca_umc_addr;
+	uint16_t ce_count_lo_chip;
+	uint16_t ce_count_hi_chip;
+
+	uint32_t eccPadding;
+} EccInfo_t;
+
+typedef struct {
+	EccInfo_t  EccInfo[ALDEBARAN_UMC_CHANNEL_NUM];
+} EccInfoTable_t;
+
 // These defines are used with the following messages:
 // SMC_MSG_TransferTableDram2Smu
 // SMC_MSG_TransferTableSmu2Dram
@@ -517,6 +532,7 @@ typedef struct {
 #define TABLE_SMU_METRICS             4
 #define TABLE_DRIVER_SMU_CONFIG       5
 #define TABLE_I2C_COMMANDS            6
-#define TABLE_COUNT                   7
+#define TABLE_ECCINFO                 7
+#define TABLE_COUNT                   8
 
 #endif
diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
index bbc608c990b0..44af23ae059e 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu_v13_0.h
@@ -27,7 +27,7 @@
 
 #define SMU13_DRIVER_IF_VERSION_INV 0xFFFFFFFF
 #define SMU13_DRIVER_IF_VERSION_YELLOW_CARP 0x04
-#define SMU13_DRIVER_IF_VERSION_ALDE 0x07
+#define SMU13_DRIVER_IF_VERSION_ALDE 0x08
 
 #define SMU13_MODE1_RESET_WAIT_TIME_IN_MS 500  //500ms
 
-- 
2.17.1

[PATCH Review 2/4] drm/amdgpu: add new query interface for umc block

[Stanley.Yang]

add message smu to query error information

Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h |  16 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h |   4 +
 drivers/gpu/drm/amd/amdgpu/umc_v6_7.c   | 161 ++++++++++++++++++++++++
 3 files changed, 181 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
index cdd0010a5389..bcbf3264d92f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
@@ -320,6 +320,19 @@ struct ras_common_if {
 	char name[32];
 };
 
+#define MAX_UMC_CHANNEL_NUM 32
+
+struct ecc_info_per_ch {
+	uint16_t ce_count_lo_chip;
+	uint16_t ce_count_hi_chip;
+	uint64_t mca_umc_status;
+	uint64_t mca_umc_addr;
+};
+
+struct umc_ecc_info {
+	struct ecc_info_per_ch ecc[MAX_UMC_CHANNEL_NUM];
+};
+
 struct amdgpu_ras {
 	/* ras infrastructure */
 	/* for ras itself. */
@@ -359,6 +372,9 @@ struct amdgpu_ras {
 	struct delayed_work ras_counte_delay_work;
 	atomic_t ras_ue_count;
 	atomic_t ras_ce_count;
+
+	/* record umc error info queried from smu */
+	struct umc_ecc_info umc_ecc;
 };
 
 struct ras_fs_data {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
index 1f5fe2315236..7aa9b21eb906 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
@@ -49,6 +49,10 @@ struct amdgpu_umc_ras_funcs {
 	void (*query_ras_error_address)(struct amdgpu_device *adev,
 					void *ras_error_status);
 	bool (*query_ras_poison_mode)(struct amdgpu_device *adev);
+	void (*message_smu_query_ras_error_count)(struct amdgpu_device *adev,
+				      void *ras_error_status);
+	void (*message_smu_query_ras_error_address)(struct amdgpu_device *adev,
+					void *ras_error_status);
 };
 
 struct amdgpu_umc_funcs {
diff --git a/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c b/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
index f7ec3fe134e5..cd96e8b734cb 100644
--- a/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
+++ b/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
@@ -50,6 +50,165 @@ static inline uint32_t get_umc_v6_7_reg_offset(struct amdgpu_device *adev,
 	return adev->umc.channel_offs * ch_inst + UMC_V6_7_INST_DIST * umc_inst;
 }
 
+static inline uint32_t get_umc_v6_7_channel_index(struct amdgpu_device *adev,
+					      uint32_t umc_inst,
+					      uint32_t ch_inst)
+{
+	return adev->umc.channel_idx_tbl[umc_inst * adev->umc.channel_inst_num + ch_inst];
+}
+
+static void umc_v6_7_message_smu_query_correctable_error_count(struct amdgpu_device *adev,
+						   uint32_t channel_index,
+						   unsigned long *error_count)
+{
+	uint32_t ecc_err_cnt;
+	uint64_t mc_umc_status;
+	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
+
+	/*
+	 * select the lower chip and check the error count
+	 * skip add error count, calc error counter only from mca_umc_status
+	 */
+	ecc_err_cnt = ras->umc_ecc.ecc[channel_index].ce_count_lo_chip;
+
+	/*
+	 * select the higher chip and check the err counter
+	 * skip add error count, calc error counter only from mca_umc_status
+	 */
+	ecc_err_cnt = ras->umc_ecc.ecc[channel_index].ce_count_hi_chip;
+
+	/* check for SRAM correctable error
+	  MCUMC_STATUS is a 64 bit register */
+	mc_umc_status = ras->umc_ecc.ecc[channel_index].mca_umc_status;
+	if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1 &&
+	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 1)
+		*error_count += 1;
+}
+
+static void umc_v6_7_message_smu_querry_uncorrectable_error_count(struct amdgpu_device *adev,
+						      uint32_t channel_index,
+						      unsigned long *error_count)
+{
+	uint64_t mc_umc_status;
+	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
+
+	/* check the MCUMC_STATUS */
+	mc_umc_status = ras->umc_ecc.ecc[channel_index].mca_umc_status;
+	if ((REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1) &&
+	    (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Deferred) == 1 ||
+	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, UECC) == 1 ||
+	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, PCC) == 1 ||
+	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, UC) == 1 ||
+	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, TCC) == 1))
+		*error_count += 1;
+}
+
+static void umc_v6_7_message_smu_query_ras_error_count(struct amdgpu_device *adev,
+					   void *ras_error_status)
+{
+	struct ras_err_data *err_data = (struct ras_err_data *)ras_error_status;
+
+	uint32_t umc_inst        = 0;
+	uint32_t ch_inst         = 0;
+	uint32_t umc_reg_offset  = 0;
+	uint32_t channel_index	 = 0;
+
+	/*TODO: driver needs to toggle DF Cstate to ensure
+	 * safe access of UMC registers. Will add the protection */
+	LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
+		umc_reg_offset = get_umc_v6_7_reg_offset(adev,
+							 umc_inst,
+							 ch_inst);
+		channel_index = get_umc_v6_7_channel_index(adev,
+							 umc_inst,
+							 ch_inst);
+		umc_v6_7_message_smu_query_correctable_error_count(adev,
+						      channel_index,
+						      &(err_data->ce_count));
+		umc_v6_7_message_smu_querry_uncorrectable_error_count(adev,
+							  channel_index,
+							  &(err_data->ue_count));
+	}
+}
+
+static void umc_v6_7_message_smu_query_error_address(struct amdgpu_device *adev,
+					 struct ras_err_data *err_data,
+					 uint32_t umc_reg_offset,
+					 uint32_t ch_inst,
+					 uint32_t umc_inst)
+{
+	uint64_t mc_umc_status, err_addr, retired_page;
+	struct eeprom_table_record *err_rec;
+	uint32_t channel_index;
+	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
+
+	channel_index =
+		adev->umc.channel_idx_tbl[umc_inst * adev->umc.channel_inst_num + ch_inst];
+
+	mc_umc_status = ras->umc_ecc.ecc[channel_index].mca_umc_status;
+
+	if (mc_umc_status == 0)
+		return;
+
+	if (!err_data->err_addr)
+		return;
+
+	err_rec = &err_data->err_addr[err_data->err_addr_cnt];
+
+	/* calculate error address if ue/ce error is detected */
+	if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1 &&
+	    (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, UECC) == 1 ||
+	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 1)) {
+
+		err_addr = ras->umc_ecc.ecc[channel_index].mca_umc_addr;
+		err_addr = REG_GET_FIELD(err_addr, MCA_UMC_UMC0_MCUMC_ADDRT0, ErrorAddr);
+
+		/* translate umc channel address to soc pa, 3 parts are included */
+		retired_page = ADDR_OF_8KB_BLOCK(err_addr) |
+				ADDR_OF_256B_BLOCK(channel_index) |
+				OFFSET_IN_256B_BLOCK(err_addr);
+
+		/* we only save ue error information currently, ce is skipped */
+		if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, UECC)
+				== 1) {
+			err_rec->address = err_addr;
+			/* page frame address is saved */
+			err_rec->retired_page = retired_page >> AMDGPU_GPU_PAGE_SHIFT;
+			err_rec->ts = (uint64_t)ktime_get_real_seconds();
+			err_rec->err_type = AMDGPU_RAS_EEPROM_ERR_NON_RECOVERABLE;
+			err_rec->cu = 0;
+			err_rec->mem_channel = channel_index;
+			err_rec->mcumc_id = umc_inst;
+
+			err_data->err_addr_cnt++;
+		}
+	}
+}
+
+static void umc_v6_7_message_smu_query_ras_error_address(struct amdgpu_device *adev,
+					     void *ras_error_status)
+{
+	struct ras_err_data *err_data = (struct ras_err_data *)ras_error_status;
+
+	uint32_t umc_inst        = 0;
+	uint32_t ch_inst         = 0;
+	uint32_t umc_reg_offset  = 0;
+
+	/*TODO: driver needs to toggle DF Cstate to ensure
+	 * safe access of UMC resgisters. Will add the protection
+	 * when firmware interface is ready */
+	LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
+		umc_reg_offset = get_umc_v6_7_reg_offset(adev,
+							 umc_inst,
+							 ch_inst);
+		umc_v6_7_message_smu_query_error_address(adev,
+					     err_data,
+					     umc_reg_offset,
+					     ch_inst,
+					     umc_inst);
+	}
+}
+
 static void umc_v6_7_query_correctable_error_count(struct amdgpu_device *adev,
 						   uint32_t umc_reg_offset,
 						   unsigned long *error_count)
@@ -327,4 +486,6 @@ const struct amdgpu_umc_ras_funcs umc_v6_7_ras_funcs = {
 	.query_ras_error_count = umc_v6_7_query_ras_error_count,
 	.query_ras_error_address = umc_v6_7_query_ras_error_address,
 	.query_ras_poison_mode = umc_v6_7_query_ras_poison_mode,
+	.message_smu_query_ras_error_count = umc_v6_7_message_smu_query_ras_error_count,
+	.message_smu_query_ras_error_address = umc_v6_7_message_smu_query_ras_error_address,
 };
-- 
2.17.1

[PATCH Review 3/4] drm/amdgpu: add message smu to get ecc_table

[Stanley.Yang]

support ECC TABLE message, this table include unc ras error count
and error address

Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
---
 drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  7 ++++
 .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 38 +++++++++++++++++++
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
 drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c        | 24 ++++++++++++
 drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h        |  3 ++
 5 files changed, 74 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 3557f4e7fc30..ea65de0160c3 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -324,6 +324,7 @@ enum smu_table_id
 	SMU_TABLE_OVERDRIVE,
 	SMU_TABLE_I2C_COMMANDS,
 	SMU_TABLE_PACE,
+	SMU_TABLE_ECCINFO,
 	SMU_TABLE_COUNT,
 };
 
@@ -340,6 +341,7 @@ struct smu_table_context
 	void				*max_sustainable_clocks;
 	struct smu_bios_boot_up_values	boot_values;
 	void                            *driver_pptable;
+	void                            *ecc_table;
 	struct smu_table		tables[SMU_TABLE_COUNT];
 	/*
 	 * The driver table is just a staging buffer for
@@ -1261,6 +1263,11 @@ struct pptable_funcs {
 	 *										of SMUBUS table.
 	 */
 	int (*send_hbm_bad_pages_num)(struct smu_context *smu, uint32_t size);
+
+	/**
+	 * @get_ecc_table:  message SMU to get ECC INFO table.
+	 */
+	ssize_t (*get_ecc_info)(struct smu_context *smu, void *table);
 };
 
 typedef enum {
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
index f835d86cc2f5..5e4ba0e14a91 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -190,6 +190,7 @@ static const struct cmn2asic_mapping aldebaran_table_map[SMU_TABLE_COUNT] = {
 	TAB_MAP(SMU_METRICS),
 	TAB_MAP(DRIVER_SMU_CONFIG),
 	TAB_MAP(I2C_COMMANDS),
+	TAB_MAP(ECCINFO),
 };
 
 static const uint8_t aldebaran_throttler_map[] = {
@@ -223,6 +224,9 @@ static int aldebaran_tables_init(struct smu_context *smu)
 	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS, sizeof(SwI2cRequest_t),
 		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
 
+	SMU_TABLE_INIT(tables, SMU_TABLE_ECCINFO, sizeof(EccInfoTable_t),
+		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
+
 	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t), GFP_KERNEL);
 	if (!smu_table->metrics_table)
 		return -ENOMEM;
@@ -235,6 +239,10 @@ static int aldebaran_tables_init(struct smu_context *smu)
 		return -ENOMEM;
 	}
 
+	smu_table->ecc_table = kzalloc(tables[SMU_TABLE_ECCINFO].size, GFP_KERNEL);
+	if (!smu_table->ecc_table)
+		return -ENOMEM;
+
 	return 0;
 }
 
@@ -1765,6 +1773,35 @@ static ssize_t aldebaran_get_gpu_metrics(struct smu_context *smu,
 	return sizeof(struct gpu_metrics_v1_3);
 }
 
+static ssize_t aldebaran_get_ecc_info(struct smu_context *smu,
+					 void *table)
+{
+	struct smu_table_context *smu_table = &smu->smu_table;
+	EccInfoTable_t ecc_table;
+	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
+	int i, ret = 0;
+	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
+
+	ret = smu_cmn_get_ecc_info_table(smu,
+					&ecc_table);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ALDEBARAN_UMC_CHANNEL_NUM; i++) {
+		ecc_info_per_channel = &(eccinfo->ecc[i]);
+		ecc_info_per_channel->ce_count_lo_chip =
+			ecc_table.EccInfo[i].ce_count_lo_chip;
+		ecc_info_per_channel->ce_count_hi_chip =
+			ecc_table.EccInfo[i].ce_count_hi_chip;
+		ecc_info_per_channel->mca_umc_status =
+			ecc_table.EccInfo[i].mca_umc_status;
+		ecc_info_per_channel->mca_umc_addr =
+			ecc_table.EccInfo[i].mca_umc_addr;
+	}
+
+	return ret;
+}
+
 static int aldebaran_mode1_reset(struct smu_context *smu)
 {
 	u32 smu_version, fatal_err, param;
@@ -1967,6 +2004,7 @@ static const struct pptable_funcs aldebaran_ppt_funcs = {
 	.i2c_init = aldebaran_i2c_control_init,
 	.i2c_fini = aldebaran_i2c_control_fini,
 	.send_hbm_bad_pages_num = aldebaran_smu_send_hbm_bad_page_num,
+	.get_ecc_info = aldebaran_get_ecc_info,
 };
 
 void aldebaran_set_ppt_funcs(struct smu_context *smu)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
index 4d96099a9bb1..55421ea622fb 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
@@ -428,8 +428,10 @@ int smu_v13_0_fini_smc_tables(struct smu_context *smu)
 	kfree(smu_table->hardcode_pptable);
 	smu_table->hardcode_pptable = NULL;
 
+	kfree(smu_table->ecc_table);
 	kfree(smu_table->metrics_table);
 	kfree(smu_table->watermarks_table);
+	smu_table->ecc_table = NULL;
 	smu_table->metrics_table = NULL;
 	smu_table->watermarks_table = NULL;
 	smu_table->metrics_time = 0;
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
index 843d2cbfc71d..e229c9b09d80 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
@@ -983,6 +983,30 @@ int smu_cmn_get_metrics_table(struct smu_context *smu,
 	return ret;
 }
 
+int smu_cmn_get_ecc_info_table(struct smu_context *smu,
+				     void *ecc_table)
+{
+	struct smu_table_context *smu_table= &smu->smu_table;
+	uint32_t table_size =
+		smu_table->tables[SMU_TABLE_ECCINFO].size;
+	int ret = 0;
+
+	ret = smu_cmn_update_table(smu,
+			       SMU_TABLE_ECCINFO,
+			       0,
+			       smu_table->ecc_table,
+			       false);
+	if (ret) {
+		dev_info(smu->adev->dev, "Failed to export SMU ecc table!\n");
+		return ret;
+	}
+
+	if (ecc_table)
+		memcpy(ecc_table, smu_table->ecc_table, table_size);
+
+	return 0;
+}
+
 void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev, uint8_t crev)
 {
 	struct metrics_table_header *header = (struct metrics_table_header *)table;
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
index beea03810bca..0adc5451373b 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
@@ -105,6 +105,9 @@ int smu_cmn_get_metrics_table(struct smu_context *smu,
 			      void *metrics_table,
 			      bool bypass_cache);
 
+int smu_cmn_get_ecc_info_table(struct smu_context *smu,
+			      void *table);
+
 void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev, uint8_t crev);
 
 int smu_cmn_set_mp1_state(struct smu_context *smu,
-- 
2.17.1

[PATCH Review 4/4] query umc error info from ecc_table

[Stanley.Yang]

if smu support ECCTABLE, driver can message smu to get ecc_table
then query umc error info from ECCTABLE
apply pmfw version check to ensure backward compatibility

Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c       | 42 ++++++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h       |  7 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c       | 71 +++++++++++++------
 drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  1 +
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 12 ++++
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  4 ++
 6 files changed, 107 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 90f0db3b4f65..6b0f2ba1e420 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -888,6 +888,38 @@ void amdgpu_ras_mca_query_error_status(struct amdgpu_device *adev,
 	}
 }
 
+static void amdgpu_ras_get_ecc_info(struct amdgpu_device *adev, struct ras_err_data *err_data)
+{
+	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
+
+	/*
+	 * choosing right query method according to
+	 * whether smu support query error information
+	 */
+	if ((ras->smu_version >= SUPPORT_ECCTABLE_SMU_VERSION) &&
+			!smu_get_ecc_info(&adev->smu, (void *)&(ras->umc_ecc))) {
+
+		if (adev->umc.ras_funcs &&
+			adev->umc.ras_funcs->message_smu_query_ras_error_count)
+			adev->umc.ras_funcs->message_smu_query_ras_error_count(adev, err_data);
+
+		if (adev->umc.ras_funcs &&
+			adev->umc.ras_funcs->message_smu_query_ras_error_address)
+			adev->umc.ras_funcs->message_smu_query_ras_error_address(adev, err_data);
+	} else {
+		if (adev->umc.ras_funcs &&
+			adev->umc.ras_funcs->query_ras_error_count)
+			adev->umc.ras_funcs->query_ras_error_count(adev, err_data);
+
+		/* umc query_ras_error_address is also responsible for clearing
+		 * error status
+		 */
+		if (adev->umc.ras_funcs &&
+		    adev->umc.ras_funcs->query_ras_error_address)
+			adev->umc.ras_funcs->query_ras_error_address(adev, err_data);
+	}
+}
+
 /* query/inject/cure begin */
 int amdgpu_ras_query_error_status(struct amdgpu_device *adev,
 				  struct ras_query_if *info)
@@ -901,15 +933,7 @@ int amdgpu_ras_query_error_status(struct amdgpu_device *adev,
 
 	switch (info->head.block) {
 	case AMDGPU_RAS_BLOCK__UMC:
-		if (adev->umc.ras_funcs &&
-		    adev->umc.ras_funcs->query_ras_error_count)
-			adev->umc.ras_funcs->query_ras_error_count(adev, &err_data);
-		/* umc query_ras_error_address is also responsible for clearing
-		 * error status
-		 */
-		if (adev->umc.ras_funcs &&
-		    adev->umc.ras_funcs->query_ras_error_address)
-			adev->umc.ras_funcs->query_ras_error_address(adev, &err_data);
+		amdgpu_ras_get_ecc_info(adev, &err_data);
 		break;
 	case AMDGPU_RAS_BLOCK__SDMA:
 		if (adev->sdma.funcs->query_ras_error_count) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
index bcbf3264d92f..3f0de0cc8403 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
@@ -322,6 +322,12 @@ struct ras_common_if {
 
 #define MAX_UMC_CHANNEL_NUM 32
 
+/*
+ * SMU support ECCTABLE since version 68.42.0,
+ * use this to decide query umc error info method
+ */
+#define SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
+
 struct ecc_info_per_ch {
 	uint16_t ce_count_lo_chip;
 	uint16_t ce_count_hi_chip;
@@ -375,6 +381,7 @@ struct amdgpu_ras {
 
 	/* record umc error info queried from smu */
 	struct umc_ecc_info umc_ecc;
+	uint32_t smu_version;
 };
 
 struct ras_fs_data {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
index 0c7c56a91b25..2c3e97c9410b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
@@ -97,28 +97,57 @@ int amdgpu_umc_process_ras_data_cb(struct amdgpu_device *adev,
 	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
 
 	kgd2kfd_set_sram_ecc_flag(adev->kfd.dev);
-	if (adev->umc.ras_funcs &&
-	    adev->umc.ras_funcs->query_ras_error_count)
-	    adev->umc.ras_funcs->query_ras_error_count(adev, ras_error_status);
 
-	if (adev->umc.ras_funcs &&
-	    adev->umc.ras_funcs->query_ras_error_address &&
-	    adev->umc.max_ras_err_cnt_per_query) {
-		err_data->err_addr =
-			kcalloc(adev->umc.max_ras_err_cnt_per_query,
-				sizeof(struct eeprom_table_record), GFP_KERNEL);
-
-		/* still call query_ras_error_address to clear error status
-		 * even NOMEM error is encountered
-		 */
-		if(!err_data->err_addr)
-			dev_warn(adev->dev, "Failed to alloc memory for "
-					"umc error address record!\n");
-
-		/* umc query_ras_error_address is also responsible for clearing
-		 * error status
-		 */
-		adev->umc.ras_funcs->query_ras_error_address(adev, ras_error_status);
+	if ((con->smu_version >= SUPPORT_ECCTABLE_SMU_VERSION) &&
+			!smu_get_ecc_info(&adev->smu, (void *)&(con->umc_ecc))) {
+
+		if (adev->umc.ras_funcs &&
+		    adev->umc.ras_funcs->message_smu_query_ras_error_count)
+		    adev->umc.ras_funcs->message_smu_query_ras_error_count(adev, ras_error_status);
+
+		if (adev->umc.ras_funcs &&
+		    adev->umc.ras_funcs->message_smu_query_ras_error_address &&
+		    adev->umc.max_ras_err_cnt_per_query) {
+			err_data->err_addr =
+				kcalloc(adev->umc.max_ras_err_cnt_per_query,
+					sizeof(struct eeprom_table_record), GFP_KERNEL);
+
+			/* still call query_ras_error_address to clear error status
+			 * even NOMEM error is encountered
+			 */
+			if(!err_data->err_addr)
+				dev_warn(adev->dev, "Failed to alloc memory for "
+						"umc error address record!\n");
+
+			/* umc query_ras_error_address is also responsible for clearing
+			 * error status
+			 */
+			adev->umc.ras_funcs->message_smu_query_ras_error_address(adev, ras_error_status);
+		}
+	} else {
+		if (adev->umc.ras_funcs &&
+		    adev->umc.ras_funcs->query_ras_error_count)
+		    adev->umc.ras_funcs->query_ras_error_count(adev, ras_error_status);
+
+		if (adev->umc.ras_funcs &&
+		    adev->umc.ras_funcs->query_ras_error_address &&
+		    adev->umc.max_ras_err_cnt_per_query) {
+			err_data->err_addr =
+				kcalloc(adev->umc.max_ras_err_cnt_per_query,
+					sizeof(struct eeprom_table_record), GFP_KERNEL);
+
+			/* still call query_ras_error_address to clear error status
+			 * even NOMEM error is encountered
+			 */
+			if(!err_data->err_addr)
+				dev_warn(adev->dev, "Failed to alloc memory for "
+						"umc error address record!\n");
+
+			/* umc query_ras_error_address is also responsible for clearing
+			 * error status
+			 */
+			adev->umc.ras_funcs->query_ras_error_address(adev, ras_error_status);
+		}
 	}
 
 	/* only uncorrectable error needs gpu reset */
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index ea65de0160c3..7a06021a58f0 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -1404,6 +1404,7 @@ int smu_set_light_sbr(struct smu_context *smu, bool enable);
 
 int smu_wait_for_event(struct amdgpu_device *adev, enum smu_event_type event,
 		       uint64_t event_arg);
+int smu_get_ecc_info(struct smu_context *smu, void *umc_ecc);
 
 #endif
 #endif
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 01168b8955bf..6340c079f35e 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -3072,6 +3072,18 @@ int smu_set_light_sbr(struct smu_context *smu, bool enable)
 	return ret;
 }
 
+int smu_get_ecc_info(struct smu_context *smu, void *umc_ecc)
+{
+	int ret = -1;
+
+	if (smu->ppt_funcs &&
+		smu->ppt_funcs->get_ecc_info)
+		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
+
+	return ret;
+
+}
+
 static int smu_get_prv_buffer_details(void *handle, void **addr, size_t *size)
 {
 	struct smu_context *smu = handle;
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
index 55421ea622fb..55ef10ca684a 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
@@ -200,11 +200,15 @@ int smu_v13_0_check_fw_version(struct smu_context *smu)
 	uint16_t smu_major;
 	uint8_t smu_minor, smu_debug;
 	int ret = 0;
+	struct amdgpu_ras *ras = amdgpu_ras_get_context(smu->adev);
 
 	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
 	if (ret)
 		return ret;
 
+	/* record smu interface version, help umc query error method */
+	ras->smu_version = smu_version;
+
 	smu_major = (smu_version >> 16) & 0xffff;
 	smu_minor = (smu_version >> 8) & 0xff;
 	smu_debug = (smu_version >> 0) & 0xff;
-- 
2.17.1

Re: [PATCH Review 4/4] query umc error info from ecc_table

[Lazar, Lijo]

On 11/17/2021 3:41 PM, Stanley.Yang wrote:
> if smu support ECCTABLE, driver can message smu to get ecc_table
> then query umc error info from ECCTABLE
> apply pmfw version check to ensure backward compatibility
> 
> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c       | 42 ++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h       |  7 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c       | 71 +++++++++++++------
>   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  1 +
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 12 ++++
>   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  4 ++
>   6 files changed, 107 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 90f0db3b4f65..6b0f2ba1e420 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -888,6 +888,38 @@ void amdgpu_ras_mca_query_error_status(struct amdgpu_device *adev,
>   	}
>   }
>   
> +static void amdgpu_ras_get_ecc_info(struct amdgpu_device *adev, struct ras_err_data *err_data)
> +{
> +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> +
> +	/*
> +	 * choosing right query method according to
> +	 * whether smu support query error information
> +	 */
> +	if ((ras->smu_version >= SUPPORT_ECCTABLE_SMU_VERSION) &&
> +			!smu_get_ecc_info(&adev->smu, (void *)&(ras->umc_ecc))) {
> +

This version check should be in aldebaran_ppt implementation. In general 
the callback will check the FW version that supports ECC table for the 
corresponding ASIC. It may return ENOTSUPP or similar if the FW version 
doesn't support ECC table and that may be checked here. Keeping 
smu_version in ras context is not needed.

> +		if (adev->umc.ras_funcs &&
> +			adev->umc.ras_funcs->message_smu_query_ras_error_count)
> +			adev->umc.ras_funcs->message_smu_query_ras_error_count(adev, err_data);
> +
> +		if (adev->umc.ras_funcs &&
> +			adev->umc.ras_funcs->message_smu_query_ras_error_address)
> +			adev->umc.ras_funcs->message_smu_query_ras_error_address(adev, err_data);
> +	} else {
> +		if (adev->umc.ras_funcs &&
> +			adev->umc.ras_funcs->query_ras_error_count)
> +			adev->umc.ras_funcs->query_ras_error_count(adev, err_data);
> +
> +		/* umc query_ras_error_address is also responsible for clearing
> +		 * error status
> +		 */
> +		if (adev->umc.ras_funcs &&
> +		    adev->umc.ras_funcs->query_ras_error_address)
> +			adev->umc.ras_funcs->query_ras_error_address(adev, err_data);
> +	}
> +}
> +
>   /* query/inject/cure begin */
>   int amdgpu_ras_query_error_status(struct amdgpu_device *adev,
>   				  struct ras_query_if *info)
> @@ -901,15 +933,7 @@ int amdgpu_ras_query_error_status(struct amdgpu_device *adev,
>   
>   	switch (info->head.block) {
>   	case AMDGPU_RAS_BLOCK__UMC:
> -		if (adev->umc.ras_funcs &&
> -		    adev->umc.ras_funcs->query_ras_error_count)
> -			adev->umc.ras_funcs->query_ras_error_count(adev, &err_data);
> -		/* umc query_ras_error_address is also responsible for clearing
> -		 * error status
> -		 */
> -		if (adev->umc.ras_funcs &&
> -		    adev->umc.ras_funcs->query_ras_error_address)
> -			adev->umc.ras_funcs->query_ras_error_address(adev, &err_data);
> +		amdgpu_ras_get_ecc_info(adev, &err_data);
>   		break;
>   	case AMDGPU_RAS_BLOCK__SDMA:
>   		if (adev->sdma.funcs->query_ras_error_count) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> index bcbf3264d92f..3f0de0cc8403 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> @@ -322,6 +322,12 @@ struct ras_common_if {
>   
>   #define MAX_UMC_CHANNEL_NUM 32
>   
> +/*
> + * SMU support ECCTABLE since version 68.42.0,
> + * use this to decide query umc error info method
> + */
> +#define SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
> +
>   struct ecc_info_per_ch {
>   	uint16_t ce_count_lo_chip;
>   	uint16_t ce_count_hi_chip;
> @@ -375,6 +381,7 @@ struct amdgpu_ras {
>   
>   	/* record umc error info queried from smu */
>   	struct umc_ecc_info umc_ecc;
> +	uint32_t smu_version;
>   };
>   
>   struct ras_fs_data {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> index 0c7c56a91b25..2c3e97c9410b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> @@ -97,28 +97,57 @@ int amdgpu_umc_process_ras_data_cb(struct amdgpu_device *adev,
>   	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
>   
>   	kgd2kfd_set_sram_ecc_flag(adev->kfd.dev);
> -	if (adev->umc.ras_funcs &&
> -	    adev->umc.ras_funcs->query_ras_error_count)
> -	    adev->umc.ras_funcs->query_ras_error_count(adev, ras_error_status);
>   
> -	if (adev->umc.ras_funcs &&
> -	    adev->umc.ras_funcs->query_ras_error_address &&
> -	    adev->umc.max_ras_err_cnt_per_query) {
> -		err_data->err_addr =
> -			kcalloc(adev->umc.max_ras_err_cnt_per_query,
> -				sizeof(struct eeprom_table_record), GFP_KERNEL);
> -
> -		/* still call query_ras_error_address to clear error status
> -		 * even NOMEM error is encountered
> -		 */
> -		if(!err_data->err_addr)
> -			dev_warn(adev->dev, "Failed to alloc memory for "
> -					"umc error address record!\n");
> -
> -		/* umc query_ras_error_address is also responsible for clearing
> -		 * error status
> -		 */
> -		adev->umc.ras_funcs->query_ras_error_address(adev, ras_error_status);
> +	if ((con->smu_version >= SUPPORT_ECCTABLE_SMU_VERSION) &&
> +			!smu_get_ecc_info(&adev->smu, (void *)&(con->umc_ecc))) {
> +
Same comment as above.

> +		if (adev->umc.ras_funcs &&
> +		    adev->umc.ras_funcs->message_smu_query_ras_error_count)
> +		    adev->umc.ras_funcs->message_smu_query_ras_error_count(adev, ras_error_status);
> +
> +		if (adev->umc.ras_funcs &&
> +		    adev->umc.ras_funcs->message_smu_query_ras_error_address &&
> +		    adev->umc.max_ras_err_cnt_per_query) {
> +			err_data->err_addr =
> +				kcalloc(adev->umc.max_ras_err_cnt_per_query,
> +					sizeof(struct eeprom_table_record), GFP_KERNEL);
> +
> +			/* still call query_ras_error_address to clear error status
> +			 * even NOMEM error is encountered
> +			 */
> +			if(!err_data->err_addr)
> +				dev_warn(adev->dev, "Failed to alloc memory for "
> +						"umc error address record!\n");
> +
> +			/* umc query_ras_error_address is also responsible for clearing
> +			 * error status
> +			 */
> +			adev->umc.ras_funcs->message_smu_query_ras_error_address(adev, ras_error_status);
> +		}
> +	} else {
> +		if (adev->umc.ras_funcs &&
> +		    adev->umc.ras_funcs->query_ras_error_count)
> +		    adev->umc.ras_funcs->query_ras_error_count(adev, ras_error_status);
> +
> +		if (adev->umc.ras_funcs &&
> +		    adev->umc.ras_funcs->query_ras_error_address &&
> +		    adev->umc.max_ras_err_cnt_per_query) {
> +			err_data->err_addr =
> +				kcalloc(adev->umc.max_ras_err_cnt_per_query,
> +					sizeof(struct eeprom_table_record), GFP_KERNEL);
> +
> +			/* still call query_ras_error_address to clear error status
> +			 * even NOMEM error is encountered
> +			 */
> +			if(!err_data->err_addr)
> +				dev_warn(adev->dev, "Failed to alloc memory for "
> +						"umc error address record!\n");
> +
> +			/* umc query_ras_error_address is also responsible for clearing
> +			 * error status
> +			 */
> +			adev->umc.ras_funcs->query_ras_error_address(adev, ras_error_status);
> +		}
>   	}
>   
>   	/* only uncorrectable error needs gpu reset */
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> index ea65de0160c3..7a06021a58f0 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> @@ -1404,6 +1404,7 @@ int smu_set_light_sbr(struct smu_context *smu, bool enable);
>   
>   int smu_wait_for_event(struct amdgpu_device *adev, enum smu_event_type event,
>   		       uint64_t event_arg);
> +int smu_get_ecc_info(struct smu_context *smu, void *umc_ecc);
>   
>   #endif
>   #endif
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 01168b8955bf..6340c079f35e 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -3072,6 +3072,18 @@ int smu_set_light_sbr(struct smu_context *smu, bool enable)
>   	return ret;
>   }
>   
> +int smu_get_ecc_info(struct smu_context *smu, void *umc_ecc)
> +{
> +	int ret = -1;
> +
> +	if (smu->ppt_funcs &&
> +		smu->ppt_funcs->get_ecc_info)
> +		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
> +

Shouldn't return -1 if ppt func is not present. If ppt func is not 
present, that means this method is not supported for the SOC; return 
ENOTSUPP.

> +	return ret;
> +
> +}
> +

Probably the above function should be clubbed with patch 3 - smu support 
for getting ras ecc info.

>   static int smu_get_prv_buffer_details(void *handle, void **addr, size_t *size)
>   {
>   	struct smu_context *smu = handle;
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> index 55421ea622fb..55ef10ca684a 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> @@ -200,11 +200,15 @@ int smu_v13_0_check_fw_version(struct smu_context *smu)
>   	uint16_t smu_major;
>   	uint8_t smu_minor, smu_debug;
>   	int ret = 0;
> +	struct amdgpu_ras *ras = amdgpu_ras_get_context(smu->adev);
>   
>   	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
>   	if (ret)
>   		return ret;
>   
> +	/* record smu interface version, help umc query error method */
> +	ras->smu_version = smu_version;
> +

This is not needed. ASIC specific functions can check the FW version for 
ECC table support.

Thanks,
Lijo

>   	smu_major = (smu_version >> 16) & 0xffff;
>   	smu_minor = (smu_version >> 8) & 0xff;
>   	smu_debug = (smu_version >> 0) & 0xff;
> 

Re: [PATCH Review 3/4] drm/amdgpu: add message smu to get ecc_table

[Lazar, Lijo]

On 11/17/2021 3:41 PM, Stanley.Yang wrote:
> support ECC TABLE message, this table include unc ras error count
> and error address
> 
> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  7 ++++
>   .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 38 +++++++++++++++++++
>   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
>   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c        | 24 ++++++++++++
>   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h        |  3 ++
>   5 files changed, 74 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> index 3557f4e7fc30..ea65de0160c3 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> @@ -324,6 +324,7 @@ enum smu_table_id
>   	SMU_TABLE_OVERDRIVE,
>   	SMU_TABLE_I2C_COMMANDS,
>   	SMU_TABLE_PACE,
> +	SMU_TABLE_ECCINFO,
>   	SMU_TABLE_COUNT,
>   };
>   
> @@ -340,6 +341,7 @@ struct smu_table_context
>   	void				*max_sustainable_clocks;
>   	struct smu_bios_boot_up_values	boot_values;
>   	void                            *driver_pptable;
> +	void                            *ecc_table;
>   	struct smu_table		tables[SMU_TABLE_COUNT];
>   	/*
>   	 * The driver table is just a staging buffer for
> @@ -1261,6 +1263,11 @@ struct pptable_funcs {
>   	 *										of SMUBUS table.
>   	 */
>   	int (*send_hbm_bad_pages_num)(struct smu_context *smu, uint32_t size);
> +
> +	/**
> +	 * @get_ecc_table:  message SMU to get ECC INFO table.
> +	 */
> +	ssize_t (*get_ecc_info)(struct smu_context *smu, void *table);
>   };
>   
>   typedef enum {
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> index f835d86cc2f5..5e4ba0e14a91 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> @@ -190,6 +190,7 @@ static const struct cmn2asic_mapping aldebaran_table_map[SMU_TABLE_COUNT] = {
>   	TAB_MAP(SMU_METRICS),
>   	TAB_MAP(DRIVER_SMU_CONFIG),
>   	TAB_MAP(I2C_COMMANDS),
> +	TAB_MAP(ECCINFO),
>   };
>   
>   static const uint8_t aldebaran_throttler_map[] = {
> @@ -223,6 +224,9 @@ static int aldebaran_tables_init(struct smu_context *smu)
>   	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS, sizeof(SwI2cRequest_t),
>   		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
>   
> +	SMU_TABLE_INIT(tables, SMU_TABLE_ECCINFO, sizeof(EccInfoTable_t),
> +		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> +
>   	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t), GFP_KERNEL);
>   	if (!smu_table->metrics_table)
>   		return -ENOMEM;
> @@ -235,6 +239,10 @@ static int aldebaran_tables_init(struct smu_context *smu)
>   		return -ENOMEM;
>   	}
>   
> +	smu_table->ecc_table = kzalloc(tables[SMU_TABLE_ECCINFO].size, GFP_KERNEL);
> +	if (!smu_table->ecc_table)
> +		return -ENOMEM;
> +
>   	return 0;
>   }
>   
> @@ -1765,6 +1773,35 @@ static ssize_t aldebaran_get_gpu_metrics(struct smu_context *smu,
>   	return sizeof(struct gpu_metrics_v1_3);
>   }
>   
> +static ssize_t aldebaran_get_ecc_info(struct smu_context *smu,
> +					 void *table)
> +{
> +	struct smu_table_context *smu_table = &smu->smu_table;
> +	EccInfoTable_t ecc_table;
> +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
> +	int i, ret = 0;
> +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
> +
> +	ret = smu_cmn_get_ecc_info_table(smu,
> +					&ecc_table);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < ALDEBARAN_UMC_CHANNEL_NUM; i++) {
> +		ecc_info_per_channel = &(eccinfo->ecc[i]);
> +		ecc_info_per_channel->ce_count_lo_chip =
> +			ecc_table.EccInfo[i].ce_count_lo_chip;
> +		ecc_info_per_channel->ce_count_hi_chip =
> +			ecc_table.EccInfo[i].ce_count_hi_chip;
> +		ecc_info_per_channel->mca_umc_status =
> +			ecc_table.EccInfo[i].mca_umc_status;
> +		ecc_info_per_channel->mca_umc_addr =
> +			ecc_table.EccInfo[i].mca_umc_addr;
> +	}
> +
> +	return ret;
> +}
> +
>   static int aldebaran_mode1_reset(struct smu_context *smu)
>   {
>   	u32 smu_version, fatal_err, param;
> @@ -1967,6 +2004,7 @@ static const struct pptable_funcs aldebaran_ppt_funcs = {
>   	.i2c_init = aldebaran_i2c_control_init,
>   	.i2c_fini = aldebaran_i2c_control_fini,
>   	.send_hbm_bad_pages_num = aldebaran_smu_send_hbm_bad_page_num,
> +	.get_ecc_info = aldebaran_get_ecc_info,
>   };
>   
>   void aldebaran_set_ppt_funcs(struct smu_context *smu)
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> index 4d96099a9bb1..55421ea622fb 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> @@ -428,8 +428,10 @@ int smu_v13_0_fini_smc_tables(struct smu_context *smu)
>   	kfree(smu_table->hardcode_pptable);
>   	smu_table->hardcode_pptable = NULL;
>   
> +	kfree(smu_table->ecc_table);
>   	kfree(smu_table->metrics_table);
>   	kfree(smu_table->watermarks_table);
> +	smu_table->ecc_table = NULL;
>   	smu_table->metrics_table = NULL;
>   	smu_table->watermarks_table = NULL;
>   	smu_table->metrics_time = 0;
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> index 843d2cbfc71d..e229c9b09d80 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> @@ -983,6 +983,30 @@ int smu_cmn_get_metrics_table(struct smu_context *smu,
>   	return ret;
>   }
>   
> +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
> +				     void *ecc_table)
> +{
> +	struct smu_table_context *smu_table= &smu->smu_table;
> +	uint32_t table_size =
> +		smu_table->tables[SMU_TABLE_ECCINFO].size;
> +	int ret = 0;
> +
> +	ret = smu_cmn_update_table(smu,
> +			       SMU_TABLE_ECCINFO,
> +			       0,
> +			       smu_table->ecc_table,
> +			       false);
> +	if (ret) {
> +		dev_info(smu->adev->dev, "Failed to export SMU ecc table!\n");
> +		return ret;
> +	}
> +
> +	if (ecc_table)
> +		memcpy(ecc_table, smu_table->ecc_table, table_size);

This copy to another buffer is redundant. You may use ecc_table directly 
in the callback, then this method itself looks unnecessary. Instead of 
calling smu_cmn_get_ecc_info_table(), call smu_cmn_update_table() and 
copy directly from ecc_table.

Thanks,
Lijo

> +
> +	return 0;
> +}
> +
>   void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev, uint8_t crev)
>   {
>   	struct metrics_table_header *header = (struct metrics_table_header *)table;
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> index beea03810bca..0adc5451373b 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> @@ -105,6 +105,9 @@ int smu_cmn_get_metrics_table(struct smu_context *smu,
>   			      void *metrics_table,
>   			      bool bypass_cache);
>   
> +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
> +			      void *table);
> +
>   void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev, uint8_t crev);
>   
>   int smu_cmn_set_mp1_state(struct smu_context *smu,
> 

Re: [PATCH Review 2/4] drm/amdgpu: add new query interface for umc block

[Lazar, Lijo]

On 11/17/2021 3:41 PM, Stanley.Yang wrote:
> add message smu to query error information
> 
> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h |  16 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h |   4 +
>   drivers/gpu/drm/amd/amdgpu/umc_v6_7.c   | 161 ++++++++++++++++++++++++
>   3 files changed, 181 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> index cdd0010a5389..bcbf3264d92f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> @@ -320,6 +320,19 @@ struct ras_common_if {
>   	char name[32];
>   };
>   
> +#define MAX_UMC_CHANNEL_NUM 32
> +
> +struct ecc_info_per_ch {
> +	uint16_t ce_count_lo_chip;
> +	uint16_t ce_count_hi_chip;
> +	uint64_t mca_umc_status;
> +	uint64_t mca_umc_addr;
> +};
> +
> +struct umc_ecc_info {
> +	struct ecc_info_per_ch ecc[MAX_UMC_CHANNEL_NUM];
> +};
> +
>   struct amdgpu_ras {
>   	/* ras infrastructure */
>   	/* for ras itself. */
> @@ -359,6 +372,9 @@ struct amdgpu_ras {
>   	struct delayed_work ras_counte_delay_work;
>   	atomic_t ras_ue_count;
>   	atomic_t ras_ce_count;
> +
> +	/* record umc error info queried from smu */
> +	struct umc_ecc_info umc_ecc;
>   };
>   
>   struct ras_fs_data {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> index 1f5fe2315236..7aa9b21eb906 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> @@ -49,6 +49,10 @@ struct amdgpu_umc_ras_funcs {
>   	void (*query_ras_error_address)(struct amdgpu_device *adev,
>   					void *ras_error_status);
>   	bool (*query_ras_poison_mode)(struct amdgpu_device *adev);
> +	void (*message_smu_query_ras_error_count)(struct amdgpu_device *adev,
> +				      void *ras_error_status);
> +	void (*message_smu_query_ras_error_address)(struct amdgpu_device *adev,
> +					void *ras_error_status);

Maybe rename message_smu to ecc_info. These methods fetch the error from 
umc_ecc_info table. They don't deal with smu or care about how the 
information gets filled. As long as ecc_info_table is filled, they could 
get the info.

Thanks,
Lijo

>   };
>   
>   struct amdgpu_umc_funcs {
> diff --git a/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c b/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
> index f7ec3fe134e5..cd96e8b734cb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
> +++ b/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
> @@ -50,6 +50,165 @@ static inline uint32_t get_umc_v6_7_reg_offset(struct amdgpu_device *adev,
>   	return adev->umc.channel_offs * ch_inst + UMC_V6_7_INST_DIST * umc_inst;
>   }
>   
> +static inline uint32_t get_umc_v6_7_channel_index(struct amdgpu_device *adev,
> +					      uint32_t umc_inst,
> +					      uint32_t ch_inst)
> +{
> +	return adev->umc.channel_idx_tbl[umc_inst * adev->umc.channel_inst_num + ch_inst];
> +}
> +
> +static void umc_v6_7_message_smu_query_correctable_error_count(struct amdgpu_device *adev,
> +						   uint32_t channel_index,
> +						   unsigned long *error_count)
> +{
> +	uint32_t ecc_err_cnt;
> +	uint64_t mc_umc_status;
> +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> +
> +	/*
> +	 * select the lower chip and check the error count
> +	 * skip add error count, calc error counter only from mca_umc_status
> +	 */
> +	ecc_err_cnt = ras->umc_ecc.ecc[channel_index].ce_count_lo_chip;
> +
> +	/*
> +	 * select the higher chip and check the err counter
> +	 * skip add error count, calc error counter only from mca_umc_status
> +	 */
> +	ecc_err_cnt = ras->umc_ecc.ecc[channel_index].ce_count_hi_chip;
> +
> +	/* check for SRAM correctable error
> +	  MCUMC_STATUS is a 64 bit register */
> +	mc_umc_status = ras->umc_ecc.ecc[channel_index].mca_umc_status;
> +	if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1 &&
> +	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 1)
> +		*error_count += 1;
> +}
> +
> +static void umc_v6_7_message_smu_querry_uncorrectable_error_count(struct amdgpu_device *adev,
> +						      uint32_t channel_index,
> +						      unsigned long *error_count)
> +{
> +	uint64_t mc_umc_status;
> +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> +
> +	/* check the MCUMC_STATUS */
> +	mc_umc_status = ras->umc_ecc.ecc[channel_index].mca_umc_status;
> +	if ((REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1) &&
> +	    (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Deferred) == 1 ||
> +	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, UECC) == 1 ||
> +	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, PCC) == 1 ||
> +	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, UC) == 1 ||
> +	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, TCC) == 1))
> +		*error_count += 1;
> +}
> +
> +static void umc_v6_7_message_smu_query_ras_error_count(struct amdgpu_device *adev,
> +					   void *ras_error_status)
> +{
> +	struct ras_err_data *err_data = (struct ras_err_data *)ras_error_status;
> +
> +	uint32_t umc_inst        = 0;
> +	uint32_t ch_inst         = 0;
> +	uint32_t umc_reg_offset  = 0;
> +	uint32_t channel_index	 = 0;
> +
> +	/*TODO: driver needs to toggle DF Cstate to ensure
> +	 * safe access of UMC registers. Will add the protection */
> +	LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
> +		umc_reg_offset = get_umc_v6_7_reg_offset(adev,
> +							 umc_inst,
> +							 ch_inst);
> +		channel_index = get_umc_v6_7_channel_index(adev,
> +							 umc_inst,
> +							 ch_inst);
> +		umc_v6_7_message_smu_query_correctable_error_count(adev,
> +						      channel_index,
> +						      &(err_data->ce_count));
> +		umc_v6_7_message_smu_querry_uncorrectable_error_count(adev,
> +							  channel_index,
> +							  &(err_data->ue_count));
> +	}
> +}
> +
> +static void umc_v6_7_message_smu_query_error_address(struct amdgpu_device *adev,
> +					 struct ras_err_data *err_data,
> +					 uint32_t umc_reg_offset,
> +					 uint32_t ch_inst,
> +					 uint32_t umc_inst)
> +{
> +	uint64_t mc_umc_status, err_addr, retired_page;
> +	struct eeprom_table_record *err_rec;
> +	uint32_t channel_index;
> +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> +
> +	channel_index =
> +		adev->umc.channel_idx_tbl[umc_inst * adev->umc.channel_inst_num + ch_inst];
> +
> +	mc_umc_status = ras->umc_ecc.ecc[channel_index].mca_umc_status;
> +
> +	if (mc_umc_status == 0)
> +		return;
> +
> +	if (!err_data->err_addr)
> +		return;
> +
> +	err_rec = &err_data->err_addr[err_data->err_addr_cnt];
> +
> +	/* calculate error address if ue/ce error is detected */
> +	if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1 &&
> +	    (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, UECC) == 1 ||
> +	    REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 1)) {
> +
> +		err_addr = ras->umc_ecc.ecc[channel_index].mca_umc_addr;
> +		err_addr = REG_GET_FIELD(err_addr, MCA_UMC_UMC0_MCUMC_ADDRT0, ErrorAddr);
> +
> +		/* translate umc channel address to soc pa, 3 parts are included */
> +		retired_page = ADDR_OF_8KB_BLOCK(err_addr) |
> +				ADDR_OF_256B_BLOCK(channel_index) |
> +				OFFSET_IN_256B_BLOCK(err_addr);
> +
> +		/* we only save ue error information currently, ce is skipped */
> +		if (REG_GET_FIELD(mc_umc_status, MCA_UMC_UMC0_MCUMC_STATUST0, UECC)
> +				== 1) {
> +			err_rec->address = err_addr;
> +			/* page frame address is saved */
> +			err_rec->retired_page = retired_page >> AMDGPU_GPU_PAGE_SHIFT;
> +			err_rec->ts = (uint64_t)ktime_get_real_seconds();
> +			err_rec->err_type = AMDGPU_RAS_EEPROM_ERR_NON_RECOVERABLE;
> +			err_rec->cu = 0;
> +			err_rec->mem_channel = channel_index;
> +			err_rec->mcumc_id = umc_inst;
> +
> +			err_data->err_addr_cnt++;
> +		}
> +	}
> +}
> +
> +static void umc_v6_7_message_smu_query_ras_error_address(struct amdgpu_device *adev,
> +					     void *ras_error_status)
> +{
> +	struct ras_err_data *err_data = (struct ras_err_data *)ras_error_status;
> +
> +	uint32_t umc_inst        = 0;
> +	uint32_t ch_inst         = 0;
> +	uint32_t umc_reg_offset  = 0;
> +
> +	/*TODO: driver needs to toggle DF Cstate to ensure
> +	 * safe access of UMC resgisters. Will add the protection
> +	 * when firmware interface is ready */
> +	LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
> +		umc_reg_offset = get_umc_v6_7_reg_offset(adev,
> +							 umc_inst,
> +							 ch_inst);
> +		umc_v6_7_message_smu_query_error_address(adev,
> +					     err_data,
> +					     umc_reg_offset,
> +					     ch_inst,
> +					     umc_inst);
> +	}
> +}
> +
>   static void umc_v6_7_query_correctable_error_count(struct amdgpu_device *adev,
>   						   uint32_t umc_reg_offset,
>   						   unsigned long *error_count)
> @@ -327,4 +486,6 @@ const struct amdgpu_umc_ras_funcs umc_v6_7_ras_funcs = {
>   	.query_ras_error_count = umc_v6_7_query_ras_error_count,
>   	.query_ras_error_address = umc_v6_7_query_ras_error_address,
>   	.query_ras_poison_mode = umc_v6_7_query_ras_poison_mode,
> +	.message_smu_query_ras_error_count = umc_v6_7_message_smu_query_ras_error_count,
> +	.message_smu_query_ras_error_address = umc_v6_7_message_smu_query_ras_error_address,
>   };
> 

回复: [PATCH Review 2/4] drm/amdgpu: add new query interface for umc block

[Yang, Stanley]

[AMD Official Use Only]



> -----邮件原件-----
> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
> 发送时间: Wednesday, November 17, 2021 7:36 PM
> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
> Clements, John <John.Clements@amd.com>; Quan, Evan
> <Evan.Quan@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> 主题: Re: [PATCH Review 2/4] drm/amdgpu: add new query interface for
> umc block
> 
> 
> 
> On 11/17/2021 3:41 PM, Stanley.Yang wrote:
> > add message smu to query error information
> >
> > Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h |  16 +++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h |   4 +
> >   drivers/gpu/drm/amd/amdgpu/umc_v6_7.c   | 161
> ++++++++++++++++++++++++
> >   3 files changed, 181 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > index cdd0010a5389..bcbf3264d92f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > @@ -320,6 +320,19 @@ struct ras_common_if {
> >   	char name[32];
> >   };
> >
> > +#define MAX_UMC_CHANNEL_NUM 32
> > +
> > +struct ecc_info_per_ch {
> > +	uint16_t ce_count_lo_chip;
> > +	uint16_t ce_count_hi_chip;
> > +	uint64_t mca_umc_status;
> > +	uint64_t mca_umc_addr;
> > +};
> > +
> > +struct umc_ecc_info {
> > +	struct ecc_info_per_ch ecc[MAX_UMC_CHANNEL_NUM]; };
> > +
> >   struct amdgpu_ras {
> >   	/* ras infrastructure */
> >   	/* for ras itself. */
> > @@ -359,6 +372,9 @@ struct amdgpu_ras {
> >   	struct delayed_work ras_counte_delay_work;
> >   	atomic_t ras_ue_count;
> >   	atomic_t ras_ce_count;
> > +
> > +	/* record umc error info queried from smu */
> > +	struct umc_ecc_info umc_ecc;
> >   };
> >
> >   struct ras_fs_data {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > index 1f5fe2315236..7aa9b21eb906 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > @@ -49,6 +49,10 @@ struct amdgpu_umc_ras_funcs {
> >   	void (*query_ras_error_address)(struct amdgpu_device *adev,
> >   					void *ras_error_status);
> >   	bool (*query_ras_poison_mode)(struct amdgpu_device *adev);
> > +	void (*message_smu_query_ras_error_count)(struct
> amdgpu_device *adev,
> > +				      void *ras_error_status);
> > +	void (*message_smu_query_ras_error_address)(struct
> amdgpu_device *adev,
> > +					void *ras_error_status);
> 
> Maybe rename message_smu to ecc_info. These methods fetch the error
> from umc_ecc_info table. They don't deal with smu or care about how the
> information gets filled. As long as ecc_info_table is filled, they could get the
> info.
[Yang, Stanley] yeah, it seems rename message_smu to ecc_info is better since ecc_table has been update before this call.

> 
> Thanks,
> Lijo
> 
> >   };
> >
> >   struct amdgpu_umc_funcs {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
> > b/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
> > index f7ec3fe134e5..cd96e8b734cb 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/umc_v6_7.c
> > @@ -50,6 +50,165 @@ static inline uint32_t
> get_umc_v6_7_reg_offset(struct amdgpu_device *adev,
> >   	return adev->umc.channel_offs * ch_inst + UMC_V6_7_INST_DIST *
> umc_inst;
> >   }
> >
> > +static inline uint32_t get_umc_v6_7_channel_index(struct
> amdgpu_device *adev,
> > +					      uint32_t umc_inst,
> > +					      uint32_t ch_inst)
> > +{
> > +	return adev->umc.channel_idx_tbl[umc_inst *
> > +adev->umc.channel_inst_num + ch_inst]; }
> > +
> > +static void
> umc_v6_7_message_smu_query_correctable_error_count(struct
> amdgpu_device *adev,
> > +						   uint32_t channel_index,
> > +						   unsigned long *error_count)
> > +{
> > +	uint32_t ecc_err_cnt;
> > +	uint64_t mc_umc_status;
> > +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> > +
> > +	/*
> > +	 * select the lower chip and check the error count
> > +	 * skip add error count, calc error counter only from mca_umc_status
> > +	 */
> > +	ecc_err_cnt = ras->umc_ecc.ecc[channel_index].ce_count_lo_chip;
> > +
> > +	/*
> > +	 * select the higher chip and check the err counter
> > +	 * skip add error count, calc error counter only from mca_umc_status
> > +	 */
> > +	ecc_err_cnt = ras->umc_ecc.ecc[channel_index].ce_count_hi_chip;
> > +
> > +	/* check for SRAM correctable error
> > +	  MCUMC_STATUS is a 64 bit register */
> > +	mc_umc_status = ras-
> >umc_ecc.ecc[channel_index].mca_umc_status;
> > +	if (REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1 &&
> > +	    REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, CECC) == 1)
> > +		*error_count += 1;
> > +}
> > +
> > +static void
> umc_v6_7_message_smu_querry_uncorrectable_error_count(struct
> amdgpu_device *adev,
> > +						      uint32_t channel_index,
> > +						      unsigned long
> *error_count) {
> > +	uint64_t mc_umc_status;
> > +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> > +
> > +	/* check the MCUMC_STATUS */
> > +	mc_umc_status = ras-
> >umc_ecc.ecc[channel_index].mca_umc_status;
> > +	if ((REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1) &&
> > +	    (REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, Deferred) == 1 ||
> > +	    REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, UECC) == 1 ||
> > +	    REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, PCC) == 1 ||
> > +	    REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, UC) == 1 ||
> > +	    REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, TCC) == 1))
> > +		*error_count += 1;
> > +}
> > +
> > +static void umc_v6_7_message_smu_query_ras_error_count(struct
> amdgpu_device *adev,
> > +					   void *ras_error_status)
> > +{
> > +	struct ras_err_data *err_data = (struct ras_err_data
> > +*)ras_error_status;
> > +
> > +	uint32_t umc_inst        = 0;
> > +	uint32_t ch_inst         = 0;
> > +	uint32_t umc_reg_offset  = 0;
> > +	uint32_t channel_index	 = 0;
> > +
> > +	/*TODO: driver needs to toggle DF Cstate to ensure
> > +	 * safe access of UMC registers. Will add the protection */
> > +	LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
> > +		umc_reg_offset = get_umc_v6_7_reg_offset(adev,
> > +							 umc_inst,
> > +							 ch_inst);
> > +		channel_index = get_umc_v6_7_channel_index(adev,
> > +							 umc_inst,
> > +							 ch_inst);
> > +
> 	umc_v6_7_message_smu_query_correctable_error_count(adev,
> > +						      channel_index,
> > +						      &(err_data->ce_count));
> > +
> 	umc_v6_7_message_smu_querry_uncorrectable_error_count(adev,
> > +							  channel_index,
> > +							  &(err_data-
> >ue_count));
> > +	}
> > +}
> > +
> > +static void umc_v6_7_message_smu_query_error_address(struct
> amdgpu_device *adev,
> > +					 struct ras_err_data *err_data,
> > +					 uint32_t umc_reg_offset,
> > +					 uint32_t ch_inst,
> > +					 uint32_t umc_inst)
> > +{
> > +	uint64_t mc_umc_status, err_addr, retired_page;
> > +	struct eeprom_table_record *err_rec;
> > +	uint32_t channel_index;
> > +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> > +
> > +	channel_index =
> > +		adev->umc.channel_idx_tbl[umc_inst * adev-
> >umc.channel_inst_num +
> > +ch_inst];
> > +
> > +	mc_umc_status = ras-
> >umc_ecc.ecc[channel_index].mca_umc_status;
> > +
> > +	if (mc_umc_status == 0)
> > +		return;
> > +
> > +	if (!err_data->err_addr)
> > +		return;
> > +
> > +	err_rec = &err_data->err_addr[err_data->err_addr_cnt];
> > +
> > +	/* calculate error address if ue/ce error is detected */
> > +	if (REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, Val) == 1 &&
> > +	    (REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, UECC) == 1 ||
> > +	    REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, CECC)
> > +== 1)) {
> > +
> > +		err_addr = ras-
> >umc_ecc.ecc[channel_index].mca_umc_addr;
> > +		err_addr = REG_GET_FIELD(err_addr,
> MCA_UMC_UMC0_MCUMC_ADDRT0,
> > +ErrorAddr);
> > +
> > +		/* translate umc channel address to soc pa, 3 parts are
> included */
> > +		retired_page = ADDR_OF_8KB_BLOCK(err_addr) |
> > +				ADDR_OF_256B_BLOCK(channel_index) |
> > +				OFFSET_IN_256B_BLOCK(err_addr);
> > +
> > +		/* we only save ue error information currently, ce is skipped
> */
> > +		if (REG_GET_FIELD(mc_umc_status,
> MCA_UMC_UMC0_MCUMC_STATUST0, UECC)
> > +				== 1) {
> > +			err_rec->address = err_addr;
> > +			/* page frame address is saved */
> > +			err_rec->retired_page = retired_page >>
> AMDGPU_GPU_PAGE_SHIFT;
> > +			err_rec->ts = (uint64_t)ktime_get_real_seconds();
> > +			err_rec->err_type =
> AMDGPU_RAS_EEPROM_ERR_NON_RECOVERABLE;
> > +			err_rec->cu = 0;
> > +			err_rec->mem_channel = channel_index;
> > +			err_rec->mcumc_id = umc_inst;
> > +
> > +			err_data->err_addr_cnt++;
> > +		}
> > +	}
> > +}
> > +
> > +static void umc_v6_7_message_smu_query_ras_error_address(struct
> amdgpu_device *adev,
> > +					     void *ras_error_status)
> > +{
> > +	struct ras_err_data *err_data = (struct ras_err_data
> > +*)ras_error_status;
> > +
> > +	uint32_t umc_inst        = 0;
> > +	uint32_t ch_inst         = 0;
> > +	uint32_t umc_reg_offset  = 0;
> > +
> > +	/*TODO: driver needs to toggle DF Cstate to ensure
> > +	 * safe access of UMC resgisters. Will add the protection
> > +	 * when firmware interface is ready */
> > +	LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
> > +		umc_reg_offset = get_umc_v6_7_reg_offset(adev,
> > +							 umc_inst,
> > +							 ch_inst);
> > +		umc_v6_7_message_smu_query_error_address(adev,
> > +					     err_data,
> > +					     umc_reg_offset,
> > +					     ch_inst,
> > +					     umc_inst);
> > +	}
> > +}
> > +
> >   static void umc_v6_7_query_correctable_error_count(struct
> amdgpu_device *adev,
> >   						   uint32_t umc_reg_offset,
> >   						   unsigned long *error_count)
> @@ -327,4 +486,6 @@ const
> > struct amdgpu_umc_ras_funcs umc_v6_7_ras_funcs = {
> >   	.query_ras_error_count = umc_v6_7_query_ras_error_count,
> >   	.query_ras_error_address = umc_v6_7_query_ras_error_address,
> >   	.query_ras_poison_mode = umc_v6_7_query_ras_poison_mode,
> > +	.message_smu_query_ras_error_count =
> umc_v6_7_message_smu_query_ras_error_count,
> > +	.message_smu_query_ras_error_address =
> > +umc_v6_7_message_smu_query_ras_error_address,
> >   };
> >

回复: [PATCH Review 3/4] drm/amdgpu: add message smu to get ecc_table

[Yang, Stanley]

[AMD Official Use Only]



> -----邮件原件-----
> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
> 发送时间: Wednesday, November 17, 2021 7:24 PM
> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
> Clements, John <John.Clements@amd.com>; Quan, Evan
> <Evan.Quan@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> 主题: Re: [PATCH Review 3/4] drm/amdgpu: add message smu to get
> ecc_table
> 
> 
> 
> On 11/17/2021 3:41 PM, Stanley.Yang wrote:
> > support ECC TABLE message, this table include unc ras error count and
> > error address
> >
> > Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  7 ++++
> >   .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 38
> +++++++++++++++++++
> >   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
> >   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c        | 24 ++++++++++++
> >   drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h        |  3 ++
> >   5 files changed, 74 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > index 3557f4e7fc30..ea65de0160c3 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > @@ -324,6 +324,7 @@ enum smu_table_id
> >   	SMU_TABLE_OVERDRIVE,
> >   	SMU_TABLE_I2C_COMMANDS,
> >   	SMU_TABLE_PACE,
> > +	SMU_TABLE_ECCINFO,
> >   	SMU_TABLE_COUNT,
> >   };
> >
> > @@ -340,6 +341,7 @@ struct smu_table_context
> >   	void				*max_sustainable_clocks;
> >   	struct smu_bios_boot_up_values	boot_values;
> >   	void                            *driver_pptable;
> > +	void                            *ecc_table;
> >   	struct smu_table		tables[SMU_TABLE_COUNT];
> >   	/*
> >   	 * The driver table is just a staging buffer for @@ -1261,6
> > +1263,11 @@ struct pptable_funcs {
> >   	 *
> 		of SMUBUS table.
> >   	 */
> >   	int (*send_hbm_bad_pages_num)(struct smu_context *smu,
> uint32_t
> > size);
> > +
> > +	/**
> > +	 * @get_ecc_table:  message SMU to get ECC INFO table.
> > +	 */
> > +	ssize_t (*get_ecc_info)(struct smu_context *smu, void *table);
> >   };
> >
> >   typedef enum {
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > index f835d86cc2f5..5e4ba0e14a91 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > @@ -190,6 +190,7 @@ static const struct cmn2asic_mapping
> aldebaran_table_map[SMU_TABLE_COUNT] = {
> >   	TAB_MAP(SMU_METRICS),
> >   	TAB_MAP(DRIVER_SMU_CONFIG),
> >   	TAB_MAP(I2C_COMMANDS),
> > +	TAB_MAP(ECCINFO),
> >   };
> >
> >   static const uint8_t aldebaran_throttler_map[] = { @@ -223,6 +224,9
> > @@ static int aldebaran_tables_init(struct smu_context *smu)
> >   	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS,
> sizeof(SwI2cRequest_t),
> >   		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> >
> > +	SMU_TABLE_INIT(tables, SMU_TABLE_ECCINFO,
> sizeof(EccInfoTable_t),
> > +		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> > +
> >   	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t),
> GFP_KERNEL);
> >   	if (!smu_table->metrics_table)
> >   		return -ENOMEM;
> > @@ -235,6 +239,10 @@ static int aldebaran_tables_init(struct smu_context
> *smu)
> >   		return -ENOMEM;
> >   	}
> >
> > +	smu_table->ecc_table = kzalloc(tables[SMU_TABLE_ECCINFO].size,
> GFP_KERNEL);
> > +	if (!smu_table->ecc_table)
> > +		return -ENOMEM;
> > +
> >   	return 0;
> >   }
> >
> > @@ -1765,6 +1773,35 @@ static ssize_t aldebaran_get_gpu_metrics(struct
> smu_context *smu,
> >   	return sizeof(struct gpu_metrics_v1_3);
> >   }
> >
> > +static ssize_t aldebaran_get_ecc_info(struct smu_context *smu,
> > +					 void *table)
> > +{
> > +	struct smu_table_context *smu_table = &smu->smu_table;
> > +	EccInfoTable_t ecc_table;
> > +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
> > +	int i, ret = 0;
> > +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
> > +
> > +	ret = smu_cmn_get_ecc_info_table(smu,
> > +					&ecc_table);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < ALDEBARAN_UMC_CHANNEL_NUM; i++) {
> > +		ecc_info_per_channel = &(eccinfo->ecc[i]);
> > +		ecc_info_per_channel->ce_count_lo_chip =
> > +			ecc_table.EccInfo[i].ce_count_lo_chip;
> > +		ecc_info_per_channel->ce_count_hi_chip =
> > +			ecc_table.EccInfo[i].ce_count_hi_chip;
> > +		ecc_info_per_channel->mca_umc_status =
> > +			ecc_table.EccInfo[i].mca_umc_status;
> > +		ecc_info_per_channel->mca_umc_addr =
> > +			ecc_table.EccInfo[i].mca_umc_addr;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >   static int aldebaran_mode1_reset(struct smu_context *smu)
> >   {
> >   	u32 smu_version, fatal_err, param;
> > @@ -1967,6 +2004,7 @@ static const struct pptable_funcs
> aldebaran_ppt_funcs = {
> >   	.i2c_init = aldebaran_i2c_control_init,
> >   	.i2c_fini = aldebaran_i2c_control_fini,
> >   	.send_hbm_bad_pages_num =
> aldebaran_smu_send_hbm_bad_page_num,
> > +	.get_ecc_info = aldebaran_get_ecc_info,
> >   };
> >
> >   void aldebaran_set_ppt_funcs(struct smu_context *smu) diff --git
> > a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> > index 4d96099a9bb1..55421ea622fb 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> > @@ -428,8 +428,10 @@ int smu_v13_0_fini_smc_tables(struct
> smu_context *smu)
> >   	kfree(smu_table->hardcode_pptable);
> >   	smu_table->hardcode_pptable = NULL;
> >
> > +	kfree(smu_table->ecc_table);
> >   	kfree(smu_table->metrics_table);
> >   	kfree(smu_table->watermarks_table);
> > +	smu_table->ecc_table = NULL;
> >   	smu_table->metrics_table = NULL;
> >   	smu_table->watermarks_table = NULL;
> >   	smu_table->metrics_time = 0;
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > index 843d2cbfc71d..e229c9b09d80 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> > @@ -983,6 +983,30 @@ int smu_cmn_get_metrics_table(struct
> smu_context *smu,
> >   	return ret;
> >   }
> >
> > +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
> > +				     void *ecc_table)
> > +{
> > +	struct smu_table_context *smu_table= &smu->smu_table;
> > +	uint32_t table_size =
> > +		smu_table->tables[SMU_TABLE_ECCINFO].size;
> > +	int ret = 0;
> > +
> > +	ret = smu_cmn_update_table(smu,
> > +			       SMU_TABLE_ECCINFO,
> > +			       0,
> > +			       smu_table->ecc_table,
> > +			       false);
> > +	if (ret) {
> > +		dev_info(smu->adev->dev, "Failed to export SMU ecc
> table!\n");
> > +		return ret;
> > +	}
> > +
> > +	if (ecc_table)
> > +		memcpy(ecc_table, smu_table->ecc_table, table_size);
> 
> This copy to another buffer is redundant. You may use ecc_table directly in
> the callback, then this method itself looks unnecessary. Instead of calling
> smu_cmn_get_ecc_info_table(), call smu_cmn_update_table() and copy
> directly from ecc_table.
[Yang, Stanley] This design consider to protect ecc_table in further if multi-thread call smu_cmn_get_ecc_info_table same time, it should add mutex lock just like metrics table handle if it is necessary, but now test case is simple I didn't do that.

>
> Thanks,
> Lijo
> 
> > +
> > +	return 0;
> > +}
> > +
> >   void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev, uint8_t
> crev)
> >   {
> >   	struct metrics_table_header *header = (struct metrics_table_header
> > *)table; diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > index beea03810bca..0adc5451373b 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> > @@ -105,6 +105,9 @@ int smu_cmn_get_metrics_table(struct
> smu_context *smu,
> >   			      void *metrics_table,
> >   			      bool bypass_cache);
> >
> > +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
> > +			      void *table);
> > +
> >   void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev,
> > uint8_t crev);
> >
> >   int smu_cmn_set_mp1_state(struct smu_context *smu,
> >

回复: [PATCH Review 4/4] query umc error info from ecc_table

[Yang, Stanley]

[AMD Official Use Only]



> -----邮件原件-----
> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
> 发送时间: Wednesday, November 17, 2021 7:15 PM
> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
> Clements, John <John.Clements@amd.com>; Quan, Evan
> <Evan.Quan@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> 主题: Re: [PATCH Review 4/4] query umc error info from ecc_table
> 
> 
> 
> On 11/17/2021 3:41 PM, Stanley.Yang wrote:
> > if smu support ECCTABLE, driver can message smu to get ecc_table then
> > query umc error info from ECCTABLE apply pmfw version check to ensure
> > backward compatibility
> >
> > Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c       | 42 ++++++++---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h       |  7 ++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c       | 71 +++++++++++++--
> ----
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  1 +
> >   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 12 ++++
> >   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  4 ++
> >   6 files changed, 107 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > index 90f0db3b4f65..6b0f2ba1e420 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> > @@ -888,6 +888,38 @@ void amdgpu_ras_mca_query_error_status(struct
> amdgpu_device *adev,
> >   	}
> >   }
> >
> > +static void amdgpu_ras_get_ecc_info(struct amdgpu_device *adev,
> > +struct ras_err_data *err_data) {
> > +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
> > +
> > +	/*
> > +	 * choosing right query method according to
> > +	 * whether smu support query error information
> > +	 */
> > +	if ((ras->smu_version >= SUPPORT_ECCTABLE_SMU_VERSION) &&
> > +			!smu_get_ecc_info(&adev->smu, (void *)&(ras-
> >umc_ecc))) {
> > +
> 
> This version check should be in aldebaran_ppt implementation. In general
> the callback will check the FW version that supports ECC table for the
> corresponding ASIC. It may return ENOTSUPP or similar if the FW version
> doesn't support ECC table and that may be checked here. Keeping
> smu_version in ras context is not needed.
[Yang, Stanley] I think just check Aldebaran_ppt callback function is not enough here, considering this scenario using amdgpu driver with get_ecc_info callback function but the pmfw is an older one without ecctable feature. PMFW support ecctable since 68.42.0 for Aldebaran.

> 
> > +		if (adev->umc.ras_funcs &&
> > +			adev->umc.ras_funcs-
> >message_smu_query_ras_error_count)
> > +			adev->umc.ras_funcs-
> >message_smu_query_ras_error_count(adev,
> > +err_data);
> > +
> > +		if (adev->umc.ras_funcs &&
> > +			adev->umc.ras_funcs-
> >message_smu_query_ras_error_address)
> > +			adev->umc.ras_funcs-
> >message_smu_query_ras_error_address(adev, err_data);
> > +	} else {
> > +		if (adev->umc.ras_funcs &&
> > +			adev->umc.ras_funcs->query_ras_error_count)
> > +			adev->umc.ras_funcs->query_ras_error_count(adev,
> err_data);
> > +
> > +		/* umc query_ras_error_address is also responsible for
> clearing
> > +		 * error status
> > +		 */
> > +		if (adev->umc.ras_funcs &&
> > +		    adev->umc.ras_funcs->query_ras_error_address)
> > +			adev->umc.ras_funcs-
> >query_ras_error_address(adev, err_data);
> > +	}
> > +}
> > +
> >   /* query/inject/cure begin */
> >   int amdgpu_ras_query_error_status(struct amdgpu_device *adev,
> >   				  struct ras_query_if *info)
> > @@ -901,15 +933,7 @@ int amdgpu_ras_query_error_status(struct
> > amdgpu_device *adev,
> >
> >   	switch (info->head.block) {
> >   	case AMDGPU_RAS_BLOCK__UMC:
> > -		if (adev->umc.ras_funcs &&
> > -		    adev->umc.ras_funcs->query_ras_error_count)
> > -			adev->umc.ras_funcs->query_ras_error_count(adev,
> &err_data);
> > -		/* umc query_ras_error_address is also responsible for
> clearing
> > -		 * error status
> > -		 */
> > -		if (adev->umc.ras_funcs &&
> > -		    adev->umc.ras_funcs->query_ras_error_address)
> > -			adev->umc.ras_funcs-
> >query_ras_error_address(adev, &err_data);
> > +		amdgpu_ras_get_ecc_info(adev, &err_data);
> >   		break;
> >   	case AMDGPU_RAS_BLOCK__SDMA:
> >   		if (adev->sdma.funcs->query_ras_error_count) { diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > index bcbf3264d92f..3f0de0cc8403 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> > @@ -322,6 +322,12 @@ struct ras_common_if {
> >
> >   #define MAX_UMC_CHANNEL_NUM 32
> >
> > +/*
> > + * SMU support ECCTABLE since version 68.42.0,
> > + * use this to decide query umc error info method  */ #define
> > +SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
> > +
> >   struct ecc_info_per_ch {
> >   	uint16_t ce_count_lo_chip;
> >   	uint16_t ce_count_hi_chip;
> > @@ -375,6 +381,7 @@ struct amdgpu_ras {
> >
> >   	/* record umc error info queried from smu */
> >   	struct umc_ecc_info umc_ecc;
> > +	uint32_t smu_version;
> >   };
> >
> >   struct ras_fs_data {
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > index 0c7c56a91b25..2c3e97c9410b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > @@ -97,28 +97,57 @@ int amdgpu_umc_process_ras_data_cb(struct
> amdgpu_device *adev,
> >   	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
> >
> >   	kgd2kfd_set_sram_ecc_flag(adev->kfd.dev);
> > -	if (adev->umc.ras_funcs &&
> > -	    adev->umc.ras_funcs->query_ras_error_count)
> > -	    adev->umc.ras_funcs->query_ras_error_count(adev,
> ras_error_status);
> >
> > -	if (adev->umc.ras_funcs &&
> > -	    adev->umc.ras_funcs->query_ras_error_address &&
> > -	    adev->umc.max_ras_err_cnt_per_query) {
> > -		err_data->err_addr =
> > -			kcalloc(adev->umc.max_ras_err_cnt_per_query,
> > -				sizeof(struct eeprom_table_record),
> GFP_KERNEL);
> > -
> > -		/* still call query_ras_error_address to clear error status
> > -		 * even NOMEM error is encountered
> > -		 */
> > -		if(!err_data->err_addr)
> > -			dev_warn(adev->dev, "Failed to alloc memory for "
> > -					"umc error address record!\n");
> > -
> > -		/* umc query_ras_error_address is also responsible for
> clearing
> > -		 * error status
> > -		 */
> > -		adev->umc.ras_funcs->query_ras_error_address(adev,
> ras_error_status);
> > +	if ((con->smu_version >= SUPPORT_ECCTABLE_SMU_VERSION) &&
> > +			!smu_get_ecc_info(&adev->smu, (void *)&(con-
> >umc_ecc))) {
> > +
> Same comment as above.
> 
> > +		if (adev->umc.ras_funcs &&
> > +		    adev->umc.ras_funcs-
> >message_smu_query_ras_error_count)
> > +		    adev->umc.ras_funcs-
> >message_smu_query_ras_error_count(adev,
> > +ras_error_status);
> > +
> > +		if (adev->umc.ras_funcs &&
> > +		    adev->umc.ras_funcs-
> >message_smu_query_ras_error_address &&
> > +		    adev->umc.max_ras_err_cnt_per_query) {
> > +			err_data->err_addr =
> > +				kcalloc(adev-
> >umc.max_ras_err_cnt_per_query,
> > +					sizeof(struct eeprom_table_record),
> GFP_KERNEL);
> > +
> > +			/* still call query_ras_error_address to clear error
> status
> > +			 * even NOMEM error is encountered
> > +			 */
> > +			if(!err_data->err_addr)
> > +				dev_warn(adev->dev, "Failed to alloc
> memory for "
> > +						"umc error address
> record!\n");
> > +
> > +			/* umc query_ras_error_address is also responsible
> for clearing
> > +			 * error status
> > +			 */
> > +			adev->umc.ras_funcs-
> >message_smu_query_ras_error_address(adev, ras_error_status);
> > +		}
> > +	} else {
> > +		if (adev->umc.ras_funcs &&
> > +		    adev->umc.ras_funcs->query_ras_error_count)
> > +		    adev->umc.ras_funcs->query_ras_error_count(adev,
> > +ras_error_status);
> > +
> > +		if (adev->umc.ras_funcs &&
> > +		    adev->umc.ras_funcs->query_ras_error_address &&
> > +		    adev->umc.max_ras_err_cnt_per_query) {
> > +			err_data->err_addr =
> > +				kcalloc(adev-
> >umc.max_ras_err_cnt_per_query,
> > +					sizeof(struct eeprom_table_record),
> GFP_KERNEL);
> > +
> > +			/* still call query_ras_error_address to clear error
> status
> > +			 * even NOMEM error is encountered
> > +			 */
> > +			if(!err_data->err_addr)
> > +				dev_warn(adev->dev, "Failed to alloc
> memory for "
> > +						"umc error address
> record!\n");
> > +
> > +			/* umc query_ras_error_address is also responsible
> for clearing
> > +			 * error status
> > +			 */
> > +			adev->umc.ras_funcs-
> >query_ras_error_address(adev, ras_error_status);
> > +		}
> >   	}
> >
> >   	/* only uncorrectable error needs gpu reset */ diff --git
> > a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > index ea65de0160c3..7a06021a58f0 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > @@ -1404,6 +1404,7 @@ int smu_set_light_sbr(struct smu_context *smu,
> > bool enable);
> >
> >   int smu_wait_for_event(struct amdgpu_device *adev, enum
> smu_event_type event,
> >   		       uint64_t event_arg);
> > +int smu_get_ecc_info(struct smu_context *smu, void *umc_ecc);
> >
> >   #endif
> >   #endif
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index 01168b8955bf..6340c079f35e 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -3072,6 +3072,18 @@ int smu_set_light_sbr(struct smu_context *smu,
> bool enable)
> >   	return ret;
> >   }
> >
> > +int smu_get_ecc_info(struct smu_context *smu, void *umc_ecc) {
> > +	int ret = -1;
> > +
> > +	if (smu->ppt_funcs &&
> > +		smu->ppt_funcs->get_ecc_info)
> > +		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
> > +
> 
> Shouldn't return -1 if ppt func is not present. If ppt func is not present, that
> means this method is not supported for the SOC; return ENOTSUPP.
[Yang, Stanley] thanks, return -ENOTSUPP is more reasonable.
> 
> > +	return ret;
> > +
> > +}
> > +
> 
> Probably the above function should be clubbed with patch 3 - smu support
> for getting ras ecc info.
> 
> >   static int smu_get_prv_buffer_details(void *handle, void **addr, size_t
> *size)
> >   {
> >   	struct smu_context *smu = handle;
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> > index 55421ea622fb..55ef10ca684a 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> > @@ -200,11 +200,15 @@ int smu_v13_0_check_fw_version(struct
> smu_context *smu)
> >   	uint16_t smu_major;
> >   	uint8_t smu_minor, smu_debug;
> >   	int ret = 0;
> > +	struct amdgpu_ras *ras = amdgpu_ras_get_context(smu->adev);
> >
> >   	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
> >   	if (ret)
> >   		return ret;
> >
> > +	/* record smu interface version, help umc query error method */
> > +	ras->smu_version = smu_version;
> > +
> 
> This is not needed. ASIC specific functions can check the FW version for ECC
> table support.
> 
> Thanks,
> Lijo
> 
> >   	smu_major = (smu_version >> 16) & 0xffff;
> >   	smu_minor = (smu_version >> 8) & 0xff;
> >   	smu_debug = (smu_version >> 0) & 0xff;
> >

Re: 回复: [PATCH Review 3/4] drm/amdgpu: add message smu to get ecc_table

[Lazar, Lijo]

On 11/18/2021 9:07 AM, Yang, Stanley wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----邮件原件-----
>> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
>> 发送时间: Wednesday, November 17, 2021 7:24 PM
>> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
>> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
>> Clements, John <John.Clements@amd.com>; Quan, Evan
>> <Evan.Quan@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
>> 主题: Re: [PATCH Review 3/4] drm/amdgpu: add message smu to get
>> ecc_table
>>
>>
>>
>> On 11/17/2021 3:41 PM, Stanley.Yang wrote:
>>> support ECC TABLE message, this table include unc ras error count and
>>> error address
>>>
>>> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  7 ++++
>>>    .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 38
>> +++++++++++++++++++
>>>    .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
>>>    drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c        | 24 ++++++++++++
>>>    drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h        |  3 ++
>>>    5 files changed, 74 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> index 3557f4e7fc30..ea65de0160c3 100644
>>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> @@ -324,6 +324,7 @@ enum smu_table_id
>>>    	SMU_TABLE_OVERDRIVE,
>>>    	SMU_TABLE_I2C_COMMANDS,
>>>    	SMU_TABLE_PACE,
>>> +	SMU_TABLE_ECCINFO,
>>>    	SMU_TABLE_COUNT,
>>>    };
>>>
>>> @@ -340,6 +341,7 @@ struct smu_table_context
>>>    	void				*max_sustainable_clocks;
>>>    	struct smu_bios_boot_up_values	boot_values;
>>>    	void                            *driver_pptable;
>>> +	void                            *ecc_table;
>>>    	struct smu_table		tables[SMU_TABLE_COUNT];
>>>    	/*
>>>    	 * The driver table is just a staging buffer for @@ -1261,6
>>> +1263,11 @@ struct pptable_funcs {
>>>    	 *
>> 		of SMUBUS table.
>>>    	 */
>>>    	int (*send_hbm_bad_pages_num)(struct smu_context *smu,
>> uint32_t
>>> size);
>>> +
>>> +	/**
>>> +	 * @get_ecc_table:  message SMU to get ECC INFO table.
>>> +	 */
>>> +	ssize_t (*get_ecc_info)(struct smu_context *smu, void *table);
>>>    };
>>>
>>>    typedef enum {
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> index f835d86cc2f5..5e4ba0e14a91 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> @@ -190,6 +190,7 @@ static const struct cmn2asic_mapping
>> aldebaran_table_map[SMU_TABLE_COUNT] = {
>>>    	TAB_MAP(SMU_METRICS),
>>>    	TAB_MAP(DRIVER_SMU_CONFIG),
>>>    	TAB_MAP(I2C_COMMANDS),
>>> +	TAB_MAP(ECCINFO),
>>>    };
>>>
>>>    static const uint8_t aldebaran_throttler_map[] = { @@ -223,6 +224,9
>>> @@ static int aldebaran_tables_init(struct smu_context *smu)
>>>    	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS,
>> sizeof(SwI2cRequest_t),
>>>    		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
>>>
>>> +	SMU_TABLE_INIT(tables, SMU_TABLE_ECCINFO,
>> sizeof(EccInfoTable_t),
>>> +		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
>>> +
>>>    	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t),
>> GFP_KERNEL);
>>>    	if (!smu_table->metrics_table)
>>>    		return -ENOMEM;
>>> @@ -235,6 +239,10 @@ static int aldebaran_tables_init(struct smu_context
>> *smu)
>>>    		return -ENOMEM;
>>>    	}
>>>
>>> +	smu_table->ecc_table = kzalloc(tables[SMU_TABLE_ECCINFO].size,
>> GFP_KERNEL);
>>> +	if (!smu_table->ecc_table)
>>> +		return -ENOMEM;
>>> +
>>>    	return 0;
>>>    }
>>>
>>> @@ -1765,6 +1773,35 @@ static ssize_t aldebaran_get_gpu_metrics(struct
>> smu_context *smu,
>>>    	return sizeof(struct gpu_metrics_v1_3);
>>>    }
>>>
>>> +static ssize_t aldebaran_get_ecc_info(struct smu_context *smu,
>>> +					 void *table)
>>> +{
>>> +	struct smu_table_context *smu_table = &smu->smu_table;
>>> +	EccInfoTable_t ecc_table;
>>> +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
>>> +	int i, ret = 0;
>>> +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
>>> +
>>> +	ret = smu_cmn_get_ecc_info_table(smu,
>>> +					&ecc_table);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	for (i = 0; i < ALDEBARAN_UMC_CHANNEL_NUM; i++) {
>>> +		ecc_info_per_channel = &(eccinfo->ecc[i]);
>>> +		ecc_info_per_channel->ce_count_lo_chip =
>>> +			ecc_table.EccInfo[i].ce_count_lo_chip;
>>> +		ecc_info_per_channel->ce_count_hi_chip =
>>> +			ecc_table.EccInfo[i].ce_count_hi_chip;
>>> +		ecc_info_per_channel->mca_umc_status =
>>> +			ecc_table.EccInfo[i].mca_umc_status;
>>> +		ecc_info_per_channel->mca_umc_addr =
>>> +			ecc_table.EccInfo[i].mca_umc_addr;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>    static int aldebaran_mode1_reset(struct smu_context *smu)
>>>    {
>>>    	u32 smu_version, fatal_err, param;
>>> @@ -1967,6 +2004,7 @@ static const struct pptable_funcs
>> aldebaran_ppt_funcs = {
>>>    	.i2c_init = aldebaran_i2c_control_init,
>>>    	.i2c_fini = aldebaran_i2c_control_fini,
>>>    	.send_hbm_bad_pages_num =
>> aldebaran_smu_send_hbm_bad_page_num,
>>> +	.get_ecc_info = aldebaran_get_ecc_info,
>>>    };
>>>
>>>    void aldebaran_set_ppt_funcs(struct smu_context *smu) diff --git
>>> a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> index 4d96099a9bb1..55421ea622fb 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> @@ -428,8 +428,10 @@ int smu_v13_0_fini_smc_tables(struct
>> smu_context *smu)
>>>    	kfree(smu_table->hardcode_pptable);
>>>    	smu_table->hardcode_pptable = NULL;
>>>
>>> +	kfree(smu_table->ecc_table);
>>>    	kfree(smu_table->metrics_table);
>>>    	kfree(smu_table->watermarks_table);
>>> +	smu_table->ecc_table = NULL;
>>>    	smu_table->metrics_table = NULL;
>>>    	smu_table->watermarks_table = NULL;
>>>    	smu_table->metrics_time = 0;
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>> index 843d2cbfc71d..e229c9b09d80 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>> @@ -983,6 +983,30 @@ int smu_cmn_get_metrics_table(struct
>> smu_context *smu,
>>>    	return ret;
>>>    }
>>>
>>> +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
>>> +				     void *ecc_table)
>>> +{
>>> +	struct smu_table_context *smu_table= &smu->smu_table;
>>> +	uint32_t table_size =
>>> +		smu_table->tables[SMU_TABLE_ECCINFO].size;
>>> +	int ret = 0;
>>> +
>>> +	ret = smu_cmn_update_table(smu,
>>> +			       SMU_TABLE_ECCINFO,
>>> +			       0,
>>> +			       smu_table->ecc_table,
>>> +			       false);
>>> +	if (ret) {
>>> +		dev_info(smu->adev->dev, "Failed to export SMU ecc
>> table!\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	if (ecc_table)
>>> +		memcpy(ecc_table, smu_table->ecc_table, table_size);
>>
>> This copy to another buffer is redundant. You may use ecc_table directly in
>> the callback, then this method itself looks unnecessary. Instead of calling
>> smu_cmn_get_ecc_info_table(), call smu_cmn_update_table() and copy
>> directly from ecc_table.
> [Yang, Stanley] This design consider to protect ecc_table in further if multi-thread call smu_cmn_get_ecc_info_table same time, it should add mutex lock just like metrics table handle if it is necessary, but now test case is simple I didn't do that.
This is not like a metrics table use case. RAS error harvesting is not a 
multithread case. The error registers are cleared after reading, so I 
thought it's always expected to be one user at a time. Besides, I don't 
know if there is a case where driver needs to report errors from 
multiple threads.

Thanks,
Lijo
> 
>>
>> Thanks,
>> Lijo
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev, uint8_t
>> crev)
>>>    {
>>>    	struct metrics_table_header *header = (struct metrics_table_header
>>> *)table; diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>> index beea03810bca..0adc5451373b 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>> @@ -105,6 +105,9 @@ int smu_cmn_get_metrics_table(struct
>> smu_context *smu,
>>>    			      void *metrics_table,
>>>    			      bool bypass_cache);
>>>
>>> +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
>>> +			      void *table);
>>> +
>>>    void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev,
>>> uint8_t crev);
>>>
>>>    int smu_cmn_set_mp1_state(struct smu_context *smu,
>>>

Re: 回复: [PATCH Review 4/4] query umc error info from ecc_table

[Lazar, Lijo]

On 11/18/2021 9:29 AM, Yang, Stanley wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----邮件原件-----
>> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
>> 发送时间: Wednesday, November 17, 2021 7:15 PM
>> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
>> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
>> Clements, John <John.Clements@amd.com>; Quan, Evan
>> <Evan.Quan@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
>> 主题: Re: [PATCH Review 4/4] query umc error info from ecc_table
>>
>>
>>
>> On 11/17/2021 3:41 PM, Stanley.Yang wrote:
>>> if smu support ECCTABLE, driver can message smu to get ecc_table then
>>> query umc error info from ECCTABLE apply pmfw version check to ensure
>>> backward compatibility
>>>
>>> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c       | 42 ++++++++---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h       |  7 ++
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c       | 71 +++++++++++++--
>> ----
>>>    drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  1 +
>>>    drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 12 ++++
>>>    .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  4 ++
>>>    6 files changed, 107 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>> index 90f0db3b4f65..6b0f2ba1e420 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>>> @@ -888,6 +888,38 @@ void amdgpu_ras_mca_query_error_status(struct
>> amdgpu_device *adev,
>>>    	}
>>>    }
>>>
>>> +static void amdgpu_ras_get_ecc_info(struct amdgpu_device *adev,
>>> +struct ras_err_data *err_data) {
>>> +	struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
>>> +
>>> +	/*
>>> +	 * choosing right query method according to
>>> +	 * whether smu support query error information
>>> +	 */
>>> +	if ((ras->smu_version >= SUPPORT_ECCTABLE_SMU_VERSION) &&
>>> +			!smu_get_ecc_info(&adev->smu, (void *)&(ras-
>>> umc_ecc))) {
>>> +
>>
>> This version check should be in aldebaran_ppt implementation. In general
>> the callback will check the FW version that supports ECC table for the
>> corresponding ASIC. It may return ENOTSUPP or similar if the FW version
>> doesn't support ECC table and that may be checked here. Keeping
>> smu_version in ras context is not needed.
> [Yang, Stanley] I think just check Aldebaran_ppt callback function is not enough here, considering this scenario using amdgpu driver with get_ecc_info callback function but the pmfw is an older one without ecctable feature. PMFW support ecctable since 68.42.0 for Aldebaran.

What I meant is the FW version check code should be part of 
aldebaran_get_ecc_info() function, and it shouldn't be here. That 
function checks if the FW supports ecc_info for aldebaran, if ont 
returns ENOTSUPP.

Similarly for a newer ASIC, the corresponding ppt_func checks 
compatibility. That is one of the purposes of ASIC specific callbacks.

Thanks,
Lijo
> 
>>
>>> +		if (adev->umc.ras_funcs &&
>>> +			adev->umc.ras_funcs-
>>> message_smu_query_ras_error_count)
>>> +			adev->umc.ras_funcs-
>>> message_smu_query_ras_error_count(adev,
>>> +err_data);
>>> +
>>> +		if (adev->umc.ras_funcs &&
>>> +			adev->umc.ras_funcs-
>>> message_smu_query_ras_error_address)
>>> +			adev->umc.ras_funcs-
>>> message_smu_query_ras_error_address(adev, err_data);
>>> +	} else {
>>> +		if (adev->umc.ras_funcs &&
>>> +			adev->umc.ras_funcs->query_ras_error_count)
>>> +			adev->umc.ras_funcs->query_ras_error_count(adev,
>> err_data);
>>> +
>>> +		/* umc query_ras_error_address is also responsible for
>> clearing
>>> +		 * error status
>>> +		 */
>>> +		if (adev->umc.ras_funcs &&
>>> +		    adev->umc.ras_funcs->query_ras_error_address)
>>> +			adev->umc.ras_funcs-
>>> query_ras_error_address(adev, err_data);
>>> +	}
>>> +}
>>> +
>>>    /* query/inject/cure begin */
>>>    int amdgpu_ras_query_error_status(struct amdgpu_device *adev,
>>>    				  struct ras_query_if *info)
>>> @@ -901,15 +933,7 @@ int amdgpu_ras_query_error_status(struct
>>> amdgpu_device *adev,
>>>
>>>    	switch (info->head.block) {
>>>    	case AMDGPU_RAS_BLOCK__UMC:
>>> -		if (adev->umc.ras_funcs &&
>>> -		    adev->umc.ras_funcs->query_ras_error_count)
>>> -			adev->umc.ras_funcs->query_ras_error_count(adev,
>> &err_data);
>>> -		/* umc query_ras_error_address is also responsible for
>> clearing
>>> -		 * error status
>>> -		 */
>>> -		if (adev->umc.ras_funcs &&
>>> -		    adev->umc.ras_funcs->query_ras_error_address)
>>> -			adev->umc.ras_funcs-
>>> query_ras_error_address(adev, &err_data);
>>> +		amdgpu_ras_get_ecc_info(adev, &err_data);
>>>    		break;
>>>    	case AMDGPU_RAS_BLOCK__SDMA:
>>>    		if (adev->sdma.funcs->query_ras_error_count) { diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
>>> index bcbf3264d92f..3f0de0cc8403 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
>>> @@ -322,6 +322,12 @@ struct ras_common_if {
>>>
>>>    #define MAX_UMC_CHANNEL_NUM 32
>>>
>>> +/*
>>> + * SMU support ECCTABLE since version 68.42.0,
>>> + * use this to decide query umc error info method  */ #define
>>> +SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
>>> +
>>>    struct ecc_info_per_ch {
>>>    	uint16_t ce_count_lo_chip;
>>>    	uint16_t ce_count_hi_chip;
>>> @@ -375,6 +381,7 @@ struct amdgpu_ras {
>>>
>>>    	/* record umc error info queried from smu */
>>>    	struct umc_ecc_info umc_ecc;
>>> +	uint32_t smu_version;
>>>    };
>>>
>>>    struct ras_fs_data {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
>>> index 0c7c56a91b25..2c3e97c9410b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
>>> @@ -97,28 +97,57 @@ int amdgpu_umc_process_ras_data_cb(struct
>> amdgpu_device *adev,
>>>    	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
>>>
>>>    	kgd2kfd_set_sram_ecc_flag(adev->kfd.dev);
>>> -	if (adev->umc.ras_funcs &&
>>> -	    adev->umc.ras_funcs->query_ras_error_count)
>>> -	    adev->umc.ras_funcs->query_ras_error_count(adev,
>> ras_error_status);
>>>
>>> -	if (adev->umc.ras_funcs &&
>>> -	    adev->umc.ras_funcs->query_ras_error_address &&
>>> -	    adev->umc.max_ras_err_cnt_per_query) {
>>> -		err_data->err_addr =
>>> -			kcalloc(adev->umc.max_ras_err_cnt_per_query,
>>> -				sizeof(struct eeprom_table_record),
>> GFP_KERNEL);
>>> -
>>> -		/* still call query_ras_error_address to clear error status
>>> -		 * even NOMEM error is encountered
>>> -		 */
>>> -		if(!err_data->err_addr)
>>> -			dev_warn(adev->dev, "Failed to alloc memory for "
>>> -					"umc error address record!\n");
>>> -
>>> -		/* umc query_ras_error_address is also responsible for
>> clearing
>>> -		 * error status
>>> -		 */
>>> -		adev->umc.ras_funcs->query_ras_error_address(adev,
>> ras_error_status);
>>> +	if ((con->smu_version >= SUPPORT_ECCTABLE_SMU_VERSION) &&
>>> +			!smu_get_ecc_info(&adev->smu, (void *)&(con-
>>> umc_ecc))) {
>>> +
>> Same comment as above.
>>
>>> +		if (adev->umc.ras_funcs &&
>>> +		    adev->umc.ras_funcs-
>>> message_smu_query_ras_error_count)
>>> +		    adev->umc.ras_funcs-
>>> message_smu_query_ras_error_count(adev,
>>> +ras_error_status);
>>> +
>>> +		if (adev->umc.ras_funcs &&
>>> +		    adev->umc.ras_funcs-
>>> message_smu_query_ras_error_address &&
>>> +		    adev->umc.max_ras_err_cnt_per_query) {
>>> +			err_data->err_addr =
>>> +				kcalloc(adev-
>>> umc.max_ras_err_cnt_per_query,
>>> +					sizeof(struct eeprom_table_record),
>> GFP_KERNEL);
>>> +
>>> +			/* still call query_ras_error_address to clear error
>> status
>>> +			 * even NOMEM error is encountered
>>> +			 */
>>> +			if(!err_data->err_addr)
>>> +				dev_warn(adev->dev, "Failed to alloc
>> memory for "
>>> +						"umc error address
>> record!\n");
>>> +
>>> +			/* umc query_ras_error_address is also responsible
>> for clearing
>>> +			 * error status
>>> +			 */
>>> +			adev->umc.ras_funcs-
>>> message_smu_query_ras_error_address(adev, ras_error_status);
>>> +		}
>>> +	} else {
>>> +		if (adev->umc.ras_funcs &&
>>> +		    adev->umc.ras_funcs->query_ras_error_count)
>>> +		    adev->umc.ras_funcs->query_ras_error_count(adev,
>>> +ras_error_status);
>>> +
>>> +		if (adev->umc.ras_funcs &&
>>> +		    adev->umc.ras_funcs->query_ras_error_address &&
>>> +		    adev->umc.max_ras_err_cnt_per_query) {
>>> +			err_data->err_addr =
>>> +				kcalloc(adev-
>>> umc.max_ras_err_cnt_per_query,
>>> +					sizeof(struct eeprom_table_record),
>> GFP_KERNEL);
>>> +
>>> +			/* still call query_ras_error_address to clear error
>> status
>>> +			 * even NOMEM error is encountered
>>> +			 */
>>> +			if(!err_data->err_addr)
>>> +				dev_warn(adev->dev, "Failed to alloc
>> memory for "
>>> +						"umc error address
>> record!\n");
>>> +
>>> +			/* umc query_ras_error_address is also responsible
>> for clearing
>>> +			 * error status
>>> +			 */
>>> +			adev->umc.ras_funcs-
>>> query_ras_error_address(adev, ras_error_status);
>>> +		}
>>>    	}
>>>
>>>    	/* only uncorrectable error needs gpu reset */ diff --git
>>> a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> index ea65de0160c3..7a06021a58f0 100644
>>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> @@ -1404,6 +1404,7 @@ int smu_set_light_sbr(struct smu_context *smu,
>>> bool enable);
>>>
>>>    int smu_wait_for_event(struct amdgpu_device *adev, enum
>> smu_event_type event,
>>>    		       uint64_t event_arg);
>>> +int smu_get_ecc_info(struct smu_context *smu, void *umc_ecc);
>>>
>>>    #endif
>>>    #endif
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> index 01168b8955bf..6340c079f35e 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> @@ -3072,6 +3072,18 @@ int smu_set_light_sbr(struct smu_context *smu,
>> bool enable)
>>>    	return ret;
>>>    }
>>>
>>> +int smu_get_ecc_info(struct smu_context *smu, void *umc_ecc) {
>>> +	int ret = -1;
>>> +
>>> +	if (smu->ppt_funcs &&
>>> +		smu->ppt_funcs->get_ecc_info)
>>> +		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
>>> +
>>
>> Shouldn't return -1 if ppt func is not present. If ppt func is not present, that
>> means this method is not supported for the SOC; return ENOTSUPP.
> [Yang, Stanley] thanks, return -ENOTSUPP is more reasonable.
>>
>>> +	return ret;
>>> +
>>> +}
>>> +
>>
>> Probably the above function should be clubbed with patch 3 - smu support
>> for getting ras ecc info.
>>
>>>    static int smu_get_prv_buffer_details(void *handle, void **addr, size_t
>> *size)
>>>    {
>>>    	struct smu_context *smu = handle;
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> index 55421ea622fb..55ef10ca684a 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> @@ -200,11 +200,15 @@ int smu_v13_0_check_fw_version(struct
>> smu_context *smu)
>>>    	uint16_t smu_major;
>>>    	uint8_t smu_minor, smu_debug;
>>>    	int ret = 0;
>>> +	struct amdgpu_ras *ras = amdgpu_ras_get_context(smu->adev);
>>>
>>>    	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
>>>    	if (ret)
>>>    		return ret;
>>>
>>> +	/* record smu interface version, help umc query error method */
>>> +	ras->smu_version = smu_version;
>>> +
>>
>> This is not needed. ASIC specific functions can check the FW version for ECC
>> table support.
>>
>> Thanks,
>> Lijo
>>
>>>    	smu_major = (smu_version >> 16) & 0xffff;
>>>    	smu_minor = (smu_version >> 8) & 0xff;
>>>    	smu_debug = (smu_version >> 0) & 0xff;
>>>

回复: 回复: [PATCH Review 3/4] drm/amdgpu: add message smu to get ecc_table

[Yang, Stanley]

[AMD Official Use Only]



> -----邮件原件-----
> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
> 发送时间: Thursday, November 18, 2021 12:04 PM
> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
> Clements, John <John.Clements@amd.com>; Quan, Evan
> <Evan.Quan@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> 主题: Re: 回复: [PATCH Review 3/4] drm/amdgpu: add message smu to get
> ecc_table
> 
> 
> 
> On 11/18/2021 9:07 AM, Yang, Stanley wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -----邮件原件-----
> >> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
> >> 发送时间: Wednesday, November 17, 2021 7:24 PM
> >> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
> >> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
> >> Clements, John <John.Clements@amd.com>; Quan, Evan
> >> <Evan.Quan@amd.com>; Wang, Yang(Kevin)
> <KevinYang.Wang@amd.com>
> >> 主题: Re: [PATCH Review 3/4] drm/amdgpu: add message smu to get
> >> ecc_table
> >>
> >>
> >>
> >> On 11/17/2021 3:41 PM, Stanley.Yang wrote:
> >>> support ECC TABLE message, this table include unc ras error count
> >>> and error address
> >>>
> >>> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> >>> ---
> >>>    drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  7 ++++
> >>>    .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 38
> >> +++++++++++++++++++
> >>>    .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
> >>>    drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c        | 24 ++++++++++++
> >>>    drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h        |  3 ++
> >>>    5 files changed, 74 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> index 3557f4e7fc30..ea65de0160c3 100644
> >>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> @@ -324,6 +324,7 @@ enum smu_table_id
> >>>    	SMU_TABLE_OVERDRIVE,
> >>>    	SMU_TABLE_I2C_COMMANDS,
> >>>    	SMU_TABLE_PACE,
> >>> +	SMU_TABLE_ECCINFO,
> >>>    	SMU_TABLE_COUNT,
> >>>    };
> >>>
> >>> @@ -340,6 +341,7 @@ struct smu_table_context
> >>>    	void				*max_sustainable_clocks;
> >>>    	struct smu_bios_boot_up_values	boot_values;
> >>>    	void                            *driver_pptable;
> >>> +	void                            *ecc_table;
> >>>    	struct smu_table		tables[SMU_TABLE_COUNT];
> >>>    	/*
> >>>    	 * The driver table is just a staging buffer for @@ -1261,6
> >>> +1263,11 @@ struct pptable_funcs {
> >>>    	 *
> >> 		of SMUBUS table.
> >>>    	 */
> >>>    	int (*send_hbm_bad_pages_num)(struct smu_context *smu,
> >> uint32_t
> >>> size);
> >>> +
> >>> +	/**
> >>> +	 * @get_ecc_table:  message SMU to get ECC INFO table.
> >>> +	 */
> >>> +	ssize_t (*get_ecc_info)(struct smu_context *smu, void *table);
> >>>    };
> >>>
> >>>    typedef enum {
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> >>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> >>> index f835d86cc2f5..5e4ba0e14a91 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> >>> @@ -190,6 +190,7 @@ static const struct cmn2asic_mapping
> >> aldebaran_table_map[SMU_TABLE_COUNT] = {
> >>>    	TAB_MAP(SMU_METRICS),
> >>>    	TAB_MAP(DRIVER_SMU_CONFIG),
> >>>    	TAB_MAP(I2C_COMMANDS),
> >>> +	TAB_MAP(ECCINFO),
> >>>    };
> >>>
> >>>    static const uint8_t aldebaran_throttler_map[] = { @@ -223,6
> >>> +224,9 @@ static int aldebaran_tables_init(struct smu_context *smu)
> >>>    	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS,
> >> sizeof(SwI2cRequest_t),
> >>>    		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> >>>
> >>> +	SMU_TABLE_INIT(tables, SMU_TABLE_ECCINFO,
> >> sizeof(EccInfoTable_t),
> >>> +		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
> >>> +
> >>>    	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t),
> >> GFP_KERNEL);
> >>>    	if (!smu_table->metrics_table)
> >>>    		return -ENOMEM;
> >>> @@ -235,6 +239,10 @@ static int aldebaran_tables_init(struct
> >>> smu_context
> >> *smu)
> >>>    		return -ENOMEM;
> >>>    	}
> >>>
> >>> +	smu_table->ecc_table = kzalloc(tables[SMU_TABLE_ECCINFO].size,
> >> GFP_KERNEL);
> >>> +	if (!smu_table->ecc_table)
> >>> +		return -ENOMEM;
> >>> +
> >>>    	return 0;
> >>>    }
> >>>
> >>> @@ -1765,6 +1773,35 @@ static ssize_t
> >>> aldebaran_get_gpu_metrics(struct
> >> smu_context *smu,
> >>>    	return sizeof(struct gpu_metrics_v1_3);
> >>>    }
> >>>
> >>> +static ssize_t aldebaran_get_ecc_info(struct smu_context *smu,
> >>> +					 void *table)
> >>> +{
> >>> +	struct smu_table_context *smu_table = &smu->smu_table;
> >>> +	EccInfoTable_t ecc_table;
> >>> +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
> >>> +	int i, ret = 0;
> >>> +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
> >>> +
> >>> +	ret = smu_cmn_get_ecc_info_table(smu,
> >>> +					&ecc_table);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	for (i = 0; i < ALDEBARAN_UMC_CHANNEL_NUM; i++) {
> >>> +		ecc_info_per_channel = &(eccinfo->ecc[i]);
> >>> +		ecc_info_per_channel->ce_count_lo_chip =
> >>> +			ecc_table.EccInfo[i].ce_count_lo_chip;
> >>> +		ecc_info_per_channel->ce_count_hi_chip =
> >>> +			ecc_table.EccInfo[i].ce_count_hi_chip;
> >>> +		ecc_info_per_channel->mca_umc_status =
> >>> +			ecc_table.EccInfo[i].mca_umc_status;
> >>> +		ecc_info_per_channel->mca_umc_addr =
> >>> +			ecc_table.EccInfo[i].mca_umc_addr;
> >>> +	}
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>>    static int aldebaran_mode1_reset(struct smu_context *smu)
> >>>    {
> >>>    	u32 smu_version, fatal_err, param; @@ -1967,6 +2004,7 @@ static
> >>> const struct pptable_funcs
> >> aldebaran_ppt_funcs = {
> >>>    	.i2c_init = aldebaran_i2c_control_init,
> >>>    	.i2c_fini = aldebaran_i2c_control_fini,
> >>>    	.send_hbm_bad_pages_num =
> >> aldebaran_smu_send_hbm_bad_page_num,
> >>> +	.get_ecc_info = aldebaran_get_ecc_info,
> >>>    };
> >>>
> >>>    void aldebaran_set_ppt_funcs(struct smu_context *smu) diff --git
> >>> a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> >>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> >>> index 4d96099a9bb1..55421ea622fb 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
> >>> @@ -428,8 +428,10 @@ int smu_v13_0_fini_smc_tables(struct
> >> smu_context *smu)
> >>>    	kfree(smu_table->hardcode_pptable);
> >>>    	smu_table->hardcode_pptable = NULL;
> >>>
> >>> +	kfree(smu_table->ecc_table);
> >>>    	kfree(smu_table->metrics_table);
> >>>    	kfree(smu_table->watermarks_table);
> >>> +	smu_table->ecc_table = NULL;
> >>>    	smu_table->metrics_table = NULL;
> >>>    	smu_table->watermarks_table = NULL;
> >>>    	smu_table->metrics_time = 0;
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> >>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> >>> index 843d2cbfc71d..e229c9b09d80 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
> >>> @@ -983,6 +983,30 @@ int smu_cmn_get_metrics_table(struct
> >> smu_context *smu,
> >>>    	return ret;
> >>>    }
> >>>
> >>> +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
> >>> +				     void *ecc_table)
> >>> +{
> >>> +	struct smu_table_context *smu_table= &smu->smu_table;
> >>> +	uint32_t table_size =
> >>> +		smu_table->tables[SMU_TABLE_ECCINFO].size;
> >>> +	int ret = 0;
> >>> +
> >>> +	ret = smu_cmn_update_table(smu,
> >>> +			       SMU_TABLE_ECCINFO,
> >>> +			       0,
> >>> +			       smu_table->ecc_table,
> >>> +			       false);
> >>> +	if (ret) {
> >>> +		dev_info(smu->adev->dev, "Failed to export SMU ecc
> >> table!\n");
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	if (ecc_table)
> >>> +		memcpy(ecc_table, smu_table->ecc_table, table_size);
> >>
> >> This copy to another buffer is redundant. You may use ecc_table
> >> directly in the callback, then this method itself looks unnecessary.
> >> Instead of calling smu_cmn_get_ecc_info_table(), call
> >> smu_cmn_update_table() and copy directly from ecc_table.
> > [Yang, Stanley] This design consider to protect ecc_table in further if multi-
> thread call smu_cmn_get_ecc_info_table same time, it should add mutex
> lock just like metrics table handle if it is necessary, but now test case is simple
> I didn't do that.
> This is not like a metrics table use case. RAS error harvesting is not a
> multithread case. The error registers are cleared after reading, so I thought
> it's always expected to be one user at a time. Besides, I don't know if there is
> a case where driver needs to report errors from multiple threads.

[Yang, Stanley] not ras error harvesting, considering debugfs node file umc_error_cnt and sysfs node file umc_error_cnt, is there any mechanism ensure user read them only one thread on time?
> 
> Thanks,
> Lijo
> >
> >>
> >> Thanks,
> >> Lijo
> >>
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>    void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev,
> >>> uint8_t
> >> crev)
> >>>    {
> >>>    	struct metrics_table_header *header = (struct
> >>> metrics_table_header *)table; diff --git
> >>> a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> >>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> >>> index beea03810bca..0adc5451373b 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
> >>> @@ -105,6 +105,9 @@ int smu_cmn_get_metrics_table(struct
> >> smu_context *smu,
> >>>    			      void *metrics_table,
> >>>    			      bool bypass_cache);
> >>>
> >>> +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
> >>> +			      void *table);
> >>> +
> >>>    void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev,
> >>> uint8_t crev);
> >>>
> >>>    int smu_cmn_set_mp1_state(struct smu_context *smu,
> >>>

Re: 回复: 回复: [PATCH Review 3/4] drm/amdgpu: add message smu to get ecc_table

[Lazar, Lijo]

On 11/18/2021 9:56 AM, Yang, Stanley wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----邮件原件-----
>> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
>> 发送时间: Thursday, November 18, 2021 12:04 PM
>> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
>> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
>> Clements, John <John.Clements@amd.com>; Quan, Evan
>> <Evan.Quan@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
>> 主题: Re: 回复: [PATCH Review 3/4] drm/amdgpu: add message smu to get
>> ecc_table
>>
>>
>>
>> On 11/18/2021 9:07 AM, Yang, Stanley wrote:
>>> [AMD Official Use Only]
>>>
>>>
>>>
>>>> -----邮件原件-----
>>>> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>> 发送时间: Wednesday, November 17, 2021 7:24 PM
>>>> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
>>>> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
>>>> Clements, John <John.Clements@amd.com>; Quan, Evan
>>>> <Evan.Quan@amd.com>; Wang, Yang(Kevin)
>> <KevinYang.Wang@amd.com>
>>>> 主题: Re: [PATCH Review 3/4] drm/amdgpu: add message smu to get
>>>> ecc_table
>>>>
>>>>
>>>>
>>>> On 11/17/2021 3:41 PM, Stanley.Yang wrote:
>>>>> support ECC TABLE message, this table include unc ras error count
>>>>> and error address
>>>>>
>>>>> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  7 ++++
>>>>>     .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 38
>>>> +++++++++++++++++++
>>>>>     .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
>>>>>     drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c        | 24 ++++++++++++
>>>>>     drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h        |  3 ++
>>>>>     5 files changed, 74 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>>>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>>>> index 3557f4e7fc30..ea65de0160c3 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>>>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>>>> @@ -324,6 +324,7 @@ enum smu_table_id
>>>>>     	SMU_TABLE_OVERDRIVE,
>>>>>     	SMU_TABLE_I2C_COMMANDS,
>>>>>     	SMU_TABLE_PACE,
>>>>> +	SMU_TABLE_ECCINFO,
>>>>>     	SMU_TABLE_COUNT,
>>>>>     };
>>>>>
>>>>> @@ -340,6 +341,7 @@ struct smu_table_context
>>>>>     	void				*max_sustainable_clocks;
>>>>>     	struct smu_bios_boot_up_values	boot_values;
>>>>>     	void                            *driver_pptable;
>>>>> +	void                            *ecc_table;
>>>>>     	struct smu_table		tables[SMU_TABLE_COUNT];
>>>>>     	/*
>>>>>     	 * The driver table is just a staging buffer for @@ -1261,6
>>>>> +1263,11 @@ struct pptable_funcs {
>>>>>     	 *
>>>> 		of SMUBUS table.
>>>>>     	 */
>>>>>     	int (*send_hbm_bad_pages_num)(struct smu_context *smu,
>>>> uint32_t
>>>>> size);
>>>>> +
>>>>> +	/**
>>>>> +	 * @get_ecc_table:  message SMU to get ECC INFO table.
>>>>> +	 */
>>>>> +	ssize_t (*get_ecc_info)(struct smu_context *smu, void *table);
>>>>>     };
>>>>>
>>>>>     typedef enum {
>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>> index f835d86cc2f5..5e4ba0e14a91 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>> @@ -190,6 +190,7 @@ static const struct cmn2asic_mapping
>>>> aldebaran_table_map[SMU_TABLE_COUNT] = {
>>>>>     	TAB_MAP(SMU_METRICS),
>>>>>     	TAB_MAP(DRIVER_SMU_CONFIG),
>>>>>     	TAB_MAP(I2C_COMMANDS),
>>>>> +	TAB_MAP(ECCINFO),
>>>>>     };
>>>>>
>>>>>     static const uint8_t aldebaran_throttler_map[] = { @@ -223,6
>>>>> +224,9 @@ static int aldebaran_tables_init(struct smu_context *smu)
>>>>>     	SMU_TABLE_INIT(tables, SMU_TABLE_I2C_COMMANDS,
>>>> sizeof(SwI2cRequest_t),
>>>>>     		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
>>>>>
>>>>> +	SMU_TABLE_INIT(tables, SMU_TABLE_ECCINFO,
>>>> sizeof(EccInfoTable_t),
>>>>> +		       PAGE_SIZE, AMDGPU_GEM_DOMAIN_VRAM);
>>>>> +
>>>>>     	smu_table->metrics_table = kzalloc(sizeof(SmuMetrics_t),
>>>> GFP_KERNEL);
>>>>>     	if (!smu_table->metrics_table)
>>>>>     		return -ENOMEM;
>>>>> @@ -235,6 +239,10 @@ static int aldebaran_tables_init(struct
>>>>> smu_context
>>>> *smu)
>>>>>     		return -ENOMEM;
>>>>>     	}
>>>>>
>>>>> +	smu_table->ecc_table = kzalloc(tables[SMU_TABLE_ECCINFO].size,
>>>> GFP_KERNEL);
>>>>> +	if (!smu_table->ecc_table)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>>     	return 0;
>>>>>     }
>>>>>
>>>>> @@ -1765,6 +1773,35 @@ static ssize_t
>>>>> aldebaran_get_gpu_metrics(struct
>>>> smu_context *smu,
>>>>>     	return sizeof(struct gpu_metrics_v1_3);
>>>>>     }
>>>>>
>>>>> +static ssize_t aldebaran_get_ecc_info(struct smu_context *smu,
>>>>> +					 void *table)
>>>>> +{
>>>>> +	struct smu_table_context *smu_table = &smu->smu_table;
>>>>> +	EccInfoTable_t ecc_table;
>>>>> +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
>>>>> +	int i, ret = 0;
>>>>> +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
>>>>> +
>>>>> +	ret = smu_cmn_get_ecc_info_table(smu,
>>>>> +					&ecc_table);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	for (i = 0; i < ALDEBARAN_UMC_CHANNEL_NUM; i++) {
>>>>> +		ecc_info_per_channel = &(eccinfo->ecc[i]);
>>>>> +		ecc_info_per_channel->ce_count_lo_chip =
>>>>> +			ecc_table.EccInfo[i].ce_count_lo_chip;
>>>>> +		ecc_info_per_channel->ce_count_hi_chip =
>>>>> +			ecc_table.EccInfo[i].ce_count_hi_chip;
>>>>> +		ecc_info_per_channel->mca_umc_status =
>>>>> +			ecc_table.EccInfo[i].mca_umc_status;
>>>>> +		ecc_info_per_channel->mca_umc_addr =
>>>>> +			ecc_table.EccInfo[i].mca_umc_addr;
>>>>> +	}
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>     static int aldebaran_mode1_reset(struct smu_context *smu)
>>>>>     {
>>>>>     	u32 smu_version, fatal_err, param; @@ -1967,6 +2004,7 @@ static
>>>>> const struct pptable_funcs
>>>> aldebaran_ppt_funcs = {
>>>>>     	.i2c_init = aldebaran_i2c_control_init,
>>>>>     	.i2c_fini = aldebaran_i2c_control_fini,
>>>>>     	.send_hbm_bad_pages_num =
>>>> aldebaran_smu_send_hbm_bad_page_num,
>>>>> +	.get_ecc_info = aldebaran_get_ecc_info,
>>>>>     };
>>>>>
>>>>>     void aldebaran_set_ppt_funcs(struct smu_context *smu) diff --git
>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>>>> index 4d96099a9bb1..55421ea622fb 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>>>> @@ -428,8 +428,10 @@ int smu_v13_0_fini_smc_tables(struct
>>>> smu_context *smu)
>>>>>     	kfree(smu_table->hardcode_pptable);
>>>>>     	smu_table->hardcode_pptable = NULL;
>>>>>
>>>>> +	kfree(smu_table->ecc_table);
>>>>>     	kfree(smu_table->metrics_table);
>>>>>     	kfree(smu_table->watermarks_table);
>>>>> +	smu_table->ecc_table = NULL;
>>>>>     	smu_table->metrics_table = NULL;
>>>>>     	smu_table->watermarks_table = NULL;
>>>>>     	smu_table->metrics_time = 0;
>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>>>> index 843d2cbfc71d..e229c9b09d80 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
>>>>> @@ -983,6 +983,30 @@ int smu_cmn_get_metrics_table(struct
>>>> smu_context *smu,
>>>>>     	return ret;
>>>>>     }
>>>>>
>>>>> +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
>>>>> +				     void *ecc_table)
>>>>> +{
>>>>> +	struct smu_table_context *smu_table= &smu->smu_table;
>>>>> +	uint32_t table_size =
>>>>> +		smu_table->tables[SMU_TABLE_ECCINFO].size;
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	ret = smu_cmn_update_table(smu,
>>>>> +			       SMU_TABLE_ECCINFO,
>>>>> +			       0,
>>>>> +			       smu_table->ecc_table,
>>>>> +			       false);
>>>>> +	if (ret) {
>>>>> +		dev_info(smu->adev->dev, "Failed to export SMU ecc
>>>> table!\n");
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	if (ecc_table)
>>>>> +		memcpy(ecc_table, smu_table->ecc_table, table_size);
>>>>
>>>> This copy to another buffer is redundant. You may use ecc_table
>>>> directly in the callback, then this method itself looks unnecessary.
>>>> Instead of calling smu_cmn_get_ecc_info_table(), call
>>>> smu_cmn_update_table() and copy directly from ecc_table.
>>> [Yang, Stanley] This design consider to protect ecc_table in further if multi-
>> thread call smu_cmn_get_ecc_info_table same time, it should add mutex
>> lock just like metrics table handle if it is necessary, but now test case is simple
>> I didn't do that.
>> This is not like a metrics table use case. RAS error harvesting is not a
>> multithread case. The error registers are cleared after reading, so I thought
>> it's always expected to be one user at a time. Besides, I don't know if there is
>> a case where driver needs to report errors from multiple threads.
> 
> [Yang, Stanley] not ras error harvesting, considering debugfs node file umc_error_cnt and sysfs node file umc_error_cnt, is there any mechanism ensure user read them only one thread on time?

I see. For that, suggest to protect smu_get_ecc_info() with
	mutex_lock(&smu->mutex);

Usually we do that for swsmu APIs and that seems reasonable for 
sys/debugfs cases.

Thanks,
Lijo

>>
>> Thanks,
>> Lijo
>>>
>>>>
>>>> Thanks,
>>>> Lijo
>>>>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>     void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev,
>>>>> uint8_t
>>>> crev)
>>>>>     {
>>>>>     	struct metrics_table_header *header = (struct
>>>>> metrics_table_header *)table; diff --git
>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>>>> index beea03810bca..0adc5451373b 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.h
>>>>> @@ -105,6 +105,9 @@ int smu_cmn_get_metrics_table(struct
>>>> smu_context *smu,
>>>>>     			      void *metrics_table,
>>>>>     			      bool bypass_cache);
>>>>>
>>>>> +int smu_cmn_get_ecc_info_table(struct smu_context *smu,
>>>>> +			      void *table);
>>>>> +
>>>>>     void smu_cmn_init_soft_gpu_metrics(void *table, uint8_t frev,
>>>>> uint8_t crev);
>>>>>
>>>>>     int smu_cmn_set_mp1_state(struct smu_context *smu,
>>>>>