[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 v2

[Stanley.Yang]

add message smu to query error information

v2:
    rename message_smu to ecc_info

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..9e40bade0a68 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 (*ecc_info_query_ras_error_count)(struct amdgpu_device *adev,
+				      void *ras_error_status);
+	void (*ecc_info_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..6dd1e19e8d43 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_ecc_info_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_ecc_info_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_ecc_info_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_ecc_info_query_correctable_error_count(adev,
+						      channel_index,
+						      &(err_data->ce_count));
+		umc_v6_7_ecc_info_querry_uncorrectable_error_count(adev,
+							  channel_index,
+							  &(err_data->ue_count));
+	}
+}
+
+static void umc_v6_7_ecc_info_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_ecc_info_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_ecc_info_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,
+	.ecc_info_query_ras_error_count = umc_v6_7_ecc_info_query_ras_error_count,
+	.ecc_info_query_ras_error_address = umc_v6_7_ecc_info_query_ras_error_address,
 };
-- 
2.17.1

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

[Stanley.Yang]

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

v2:
    add smu version check to query whether support ecctable
    call smu_cmn_update_table to get ecctable directly

Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
---
 drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  8 +++
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 14 ++++
 .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 70 +++++++++++++++++++
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
 4 files changed, 94 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..7a06021a58f0 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 {
@@ -1397,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..fd3b6b460b12 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -3072,6 +3072,20 @@ 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 = -EOPNOTSUPP;
+
+	mutex_lock(&smu->mutex);
+	if (smu->ppt_funcs &&
+		smu->ppt_funcs->get_ecc_info)
+		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
+	mutex_unlock(&smu->mutex);
+
+	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/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
index f835d86cc2f5..4c21609ccea5 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -78,6 +78,12 @@
 
 #define smnPCIE_ESM_CTRL			0x111003D0
 
+/*
+ * SMU support ECCTABLE since version 68.42.0,
+ * use this to check ECCTALE feature whether support
+ */
+#define SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
+
 static const struct smu_temperature_range smu13_thermal_policy[] =
 {
 	{-273150,  99000, 99000, -273150, 99000, 99000, -273150, 99000, 99000},
@@ -190,6 +196,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 +230,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 +245,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 +1779,61 @@ static ssize_t aldebaran_get_gpu_metrics(struct smu_context *smu,
 	return sizeof(struct gpu_metrics_v1_3);
 }
 
+static int aldebaran_check_ecc_table_support(struct smu_context *smu)
+{
+	uint32_t if_version = 0xff, smu_version = 0xff;
+	int ret = 0;
+
+	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
+	if (ret)
+		ret = -EOPNOTSUPP;	// return not support if failed get smu_version
+
+	if (smu_version < SUPPORT_ECCTABLE_SMU_VERSION)
+		ret = -EOPNOTSUPP;
+
+	return ret;
+}
+
+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 = NULL;
+	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 = aldebaran_check_ecc_table_support(smu);
+	if (ret)
+		return ret;
+
+	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;
+	}
+
+	ecc_table = (EccInfoTable_t *)smu_table->ecc_table;
+
+	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 +2036,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;
-- 
2.17.1

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

[Stanley.Yang]

if smu support ECCTABLE, driver can message smu to get ecc_table
then query umc error info from ECCTABLE

v2:
    optimize source code makes logical more reasonable

Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 42 +++++++++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c | 72 +++++++++++++++++--------
 2 files changed, 83 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 90f0db3b4f65..3a4e483cd5e2 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);
+	int ret = 0;
+
+	/*
+	 * choosing right query method according to
+	 * whether smu support query error information
+	 */
+	ret = smu_get_ecc_info(&adev->smu, (void *)&(ras->umc_ecc));
+	if (ret == -EOPNOTSUPP) {
+		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);
+	} else if (!ret) {
+		if (adev->umc.ras_funcs &&
+			adev->umc.ras_funcs->ecc_info_query_ras_error_count)
+			adev->umc.ras_funcs->ecc_info_query_ras_error_count(adev, err_data);
+
+		if (adev->umc.ras_funcs &&
+			adev->umc.ras_funcs->ecc_info_query_ras_error_address)
+			adev->umc.ras_funcs->ecc_info_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_umc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
index 0c7c56a91b25..2b37b1c293b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
@@ -95,30 +95,58 @@ int amdgpu_umc_process_ras_data_cb(struct amdgpu_device *adev,
 {
 	struct ras_err_data *err_data = (struct ras_err_data *)ras_error_status;
 	struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
+	int ret = 0;
 
 	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);
+	ret = smu_get_ecc_info(&adev->smu, (void *)&(con->umc_ecc));
+	if (ret == -EOPNOTSUPP) {
+		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);
+		}
+	} else if (!ret) {
+		if (adev->umc.ras_funcs &&
+		    adev->umc.ras_funcs->ecc_info_query_ras_error_count)
+		    adev->umc.ras_funcs->ecc_info_query_ras_error_count(adev, ras_error_status);
+
+		if (adev->umc.ras_funcs &&
+		    adev->umc.ras_funcs->ecc_info_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->ecc_info_query_ras_error_address(adev, ras_error_status);
+		}
 	}
 
 	/* only uncorrectable error needs gpu reset */
-- 
2.17.1

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

[Quan, Evan]

[AMD Official Use Only]

Better to update the patch title as "drm/amd/pm: Update smu driver interface for aldebaran" as all other power related patches.
And please update patch3 also.
Other than above, patch 1, 3 are reviewed-by: Evan Quan <evan.quan@amd.com>
> -----Original Message-----
> From: Stanley.Yang <Stanley.Yang@amd.com>
> Sent: Thursday, November 18, 2021 5:34 PM
> To: amd-gfx@lists.freedesktop.org; Zhang, Hawking
> <Hawking.Zhang@amd.com>; Clements, John <John.Clements@amd.com>;
> Quan, Evan <Evan.Quan@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>;
> Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> Cc: Yang, Stanley <Stanley.Yang@amd.com>
> Subject: [PATCH Review 1/4] drm/amdgpu: Update smu driver interface for
> aldebaran
> 
> 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 1/4] drm/amdgpu: Update smu driver interface for aldebaran

[Yang, Stanley]

[AMD Official Use Only]

Thanks Evan,

Will update patch 1 and 3 title before submit.

Regards,
Stanley
> -----邮件原件-----
> 发件人: Quan, Evan <Evan.Quan@amd.com>
> 发送时间: Thursday, November 18, 2021 5:58 PM
> 收件人: Yang, Stanley <Stanley.Yang@amd.com>; amd-
> gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com>;
> Clements, John <John.Clements@amd.com>; Lazar, Lijo
> <Lijo.Lazar@amd.com>; Wang, Yang(Kevin) <KevinYang.Wang@amd.com>
> 抄送: Yang, Stanley <Stanley.Yang@amd.com>
> 主题: RE: [PATCH Review 1/4] drm/amdgpu: Update smu driver interface for
> aldebaran
> 
> [AMD Official Use Only]
> 
> Better to update the patch title as "drm/amd/pm: Update smu driver
> interface for aldebaran" as all other power related patches.
> And please update patch3 also.
> Other than above, patch 1, 3 are reviewed-by: Evan Quan
> <evan.quan@amd.com>
> > -----Original Message-----
> > From: Stanley.Yang <Stanley.Yang@amd.com>
> > Sent: Thursday, November 18, 2021 5:34 PM
> > To: amd-gfx@lists.freedesktop.org; Zhang, Hawking
> > <Hawking.Zhang@amd.com>; Clements, John
> <John.Clements@amd.com>; Quan,
> > Evan <Evan.Quan@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Wang,
> > Yang(Kevin) <KevinYang.Wang@amd.com>
> > Cc: Yang, Stanley <Stanley.Yang@amd.com>
> > Subject: [PATCH Review 1/4] drm/amdgpu: Update smu driver interface
> > for aldebaran
> >
> > 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

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

[Lazar, Lijo]

On 11/18/2021 3:03 PM, Stanley.Yang wrote:
> support ECC TABLE message, this table include umc ras error count
> and error address
> 
> v2:
>      add smu version check to query whether support ecctable
>      call smu_cmn_update_table to get ecctable directly
> 
> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  8 +++
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 14 ++++
>   .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 70 +++++++++++++++++++
>   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
>   4 files changed, 94 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..7a06021a58f0 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 {
> @@ -1397,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..fd3b6b460b12 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -3072,6 +3072,20 @@ 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 = -EOPNOTSUPP;
> +
> +	mutex_lock(&smu->mutex);
> +	if (smu->ppt_funcs &&
> +		smu->ppt_funcs->get_ecc_info)
> +		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
> +	mutex_unlock(&smu->mutex);
> +
> +	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/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> index f835d86cc2f5..4c21609ccea5 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> @@ -78,6 +78,12 @@
>   
>   #define smnPCIE_ESM_CTRL			0x111003D0
>   
> +/*
> + * SMU support ECCTABLE since version 68.42.0,
> + * use this to check ECCTALE feature whether support
> + */
> +#define SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
> +
>   static const struct smu_temperature_range smu13_thermal_policy[] =
>   {
>   	{-273150,  99000, 99000, -273150, 99000, 99000, -273150, 99000, 99000},
> @@ -190,6 +196,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 +230,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 +245,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 +1779,61 @@ static ssize_t aldebaran_get_gpu_metrics(struct smu_context *smu,
>   	return sizeof(struct gpu_metrics_v1_3);
>   }
>   
> +static int aldebaran_check_ecc_table_support(struct smu_context *smu)
> +{
> +	uint32_t if_version = 0xff, smu_version = 0xff;
> +	int ret = 0;
> +
> +	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
> +	if (ret)
> +		ret = -EOPNOTSUPP;	// return not support if failed get smu_version
> +
> +	if (smu_version < SUPPORT_ECCTABLE_SMU_VERSION)
> +		ret = -EOPNOTSUPP;
> +
> +	return ret;
> +}
> +
> +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 = NULL;
> +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
> +	int i, ret = 0;
> +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
> +

Missed to ask last time. Since umc_ecc_info is a common struct, do you 
also want to pass back the number of channels having data?

Now this struct can hold max of 32 channel data. Let's say if the same 
interface is going to be used on another ASIC X having only 16 channels. 
Then the callback for ASIC X fills data only for 16 channels. Or, you 
expect that to be taken care at the caller side?

Thanks,
Lijo

> +	ret = aldebaran_check_ecc_table_support(smu);
> +	if (ret)
> +		return ret;
> +
> +	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;
> +	}
> +
> +	ecc_table = (EccInfoTable_t *)smu_table->ecc_table;
> +
> +	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 +2036,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;
> 

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

[Yang, Stanley]

[AMD Official Use Only]



> -----邮件原件-----
> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
> 发送时间: Thursday, November 18, 2021 7:33 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 v2
> 
> 
> 
> On 11/18/2021 3:03 PM, Stanley.Yang wrote:
> > support ECC TABLE message, this table include umc ras error count and
> > error address
> >
> > v2:
> >      add smu version check to query whether support ecctable
> >      call smu_cmn_update_table to get ecctable directly
> >
> > Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  8 +++
> >   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 14 ++++
> >   .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 70
> +++++++++++++++++++
> >   .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
> >   4 files changed, 94 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..7a06021a58f0 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 {
> > @@ -1397,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..fd3b6b460b12 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -3072,6 +3072,20 @@ 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 = -EOPNOTSUPP;
> > +
> > +	mutex_lock(&smu->mutex);
> > +	if (smu->ppt_funcs &&
> > +		smu->ppt_funcs->get_ecc_info)
> > +		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
> > +	mutex_unlock(&smu->mutex);
> > +
> > +	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/aldebaran_ppt.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > index f835d86cc2f5..4c21609ccea5 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> > @@ -78,6 +78,12 @@
> >
> >   #define smnPCIE_ESM_CTRL			0x111003D0
> >
> > +/*
> > + * SMU support ECCTABLE since version 68.42.0,
> > + * use this to check ECCTALE feature whether support  */ #define
> > +SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
> > +
> >   static const struct smu_temperature_range smu13_thermal_policy[] =
> >   {
> >   	{-273150,  99000, 99000, -273150, 99000, 99000, -273150, 99000,
> > 99000}, @@ -190,6 +196,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 +230,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 +245,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 +1779,61 @@ static ssize_t aldebaran_get_gpu_metrics(struct
> smu_context *smu,
> >   	return sizeof(struct gpu_metrics_v1_3);
> >   }
> >
> > +static int aldebaran_check_ecc_table_support(struct smu_context *smu)
> > +{
> > +	uint32_t if_version = 0xff, smu_version = 0xff;
> > +	int ret = 0;
> > +
> > +	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
> > +	if (ret)
> > +		ret = -EOPNOTSUPP;	// return not support if failed get
> smu_version
> > +
> > +	if (smu_version < SUPPORT_ECCTABLE_SMU_VERSION)
> > +		ret = -EOPNOTSUPP;
> > +
> > +	return ret;
> > +}
> > +
> > +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 = NULL;
> > +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
> > +	int i, ret = 0;
> > +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
> > +
> 
> Missed to ask last time. Since umc_ecc_info is a common struct, do you also
> want to pass back the number of channels having data?
> 
> Now this struct can hold max of 32 channel data. Let's say if the same
> interface is going to be used on another ASIC X having only 16 channels.
> Then the callback for ASIC X fills data only for 16 channels. Or, you expect that
> to be taken care at the caller side?

[Yang, Stanley] : If ASIC X have only 16 channels, the callback only fill data for 16 channels, and caller side also need consider its own channel number to handle with umc_ecc_info.

> 
> Thanks,
> Lijo
> 
> > +	ret = aldebaran_check_ecc_table_support(smu);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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;
> > +	}
> > +
> > +	ecc_table = (EccInfoTable_t *)smu_table->ecc_table;
> > +
> > +	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 +2036,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;
> >

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

[Lazar, Lijo]

On 11/18/2021 6:05 PM, Yang, Stanley wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----邮件原件-----
>> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
>> 发送时间: Thursday, November 18, 2021 7:33 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 v2
>>
>>
>>
>> On 11/18/2021 3:03 PM, Stanley.Yang wrote:
>>> support ECC TABLE message, this table include umc ras error count and
>>> error address
>>>
>>> v2:
>>>       add smu version check to query whether support ecctable
>>>       call smu_cmn_update_table to get ecctable directly
>>>
>>> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  8 +++
>>>    drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 14 ++++
>>>    .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 70
>> +++++++++++++++++++
>>>    .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
>>>    4 files changed, 94 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..7a06021a58f0 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 {
>>> @@ -1397,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..fd3b6b460b12 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> @@ -3072,6 +3072,20 @@ 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 = -EOPNOTSUPP;
>>> +
>>> +	mutex_lock(&smu->mutex);
>>> +	if (smu->ppt_funcs &&
>>> +		smu->ppt_funcs->get_ecc_info)
>>> +		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
>>> +	mutex_unlock(&smu->mutex);
>>> +
>>> +	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/aldebaran_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> index f835d86cc2f5..4c21609ccea5 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> @@ -78,6 +78,12 @@
>>>
>>>    #define smnPCIE_ESM_CTRL			0x111003D0
>>>
>>> +/*
>>> + * SMU support ECCTABLE since version 68.42.0,
>>> + * use this to check ECCTALE feature whether support  */ #define
>>> +SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
>>> +
>>>    static const struct smu_temperature_range smu13_thermal_policy[] =
>>>    {
>>>    	{-273150,  99000, 99000, -273150, 99000, 99000, -273150, 99000,
>>> 99000}, @@ -190,6 +196,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 +230,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 +245,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 +1779,61 @@ static ssize_t aldebaran_get_gpu_metrics(struct
>> smu_context *smu,
>>>    	return sizeof(struct gpu_metrics_v1_3);
>>>    }
>>>
>>> +static int aldebaran_check_ecc_table_support(struct smu_context *smu)
>>> +{
>>> +	uint32_t if_version = 0xff, smu_version = 0xff;
>>> +	int ret = 0;
>>> +
>>> +	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
>>> +	if (ret)
>>> +		ret = -EOPNOTSUPP;	// return not support if failed get

Nitpick - comment style

>> smu_version
>>> +
>>> +	if (smu_version < SUPPORT_ECCTABLE_SMU_VERSION)
>>> +		ret = -EOPNOTSUPP;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +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 = NULL;
>>> +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
>>> +	int i, ret = 0;
>>> +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
>>> +
>>
>> Missed to ask last time. Since umc_ecc_info is a common struct, do you also
>> want to pass back the number of channels having data?
>>
>> Now this struct can hold max of 32 channel data. Let's say if the same
>> interface is going to be used on another ASIC X having only 16 channels.
>> Then the callback for ASIC X fills data only for 16 channels. Or, you expect that
>> to be taken care at the caller side?
> 
> [Yang, Stanley] : If ASIC X have only 16 channels, the callback only fill data for 16 channels, and caller side also need consider its own channel number to handle with umc_ecc_info.
> 

Thanks for the details. With the nitpick above and Evan's comments on 
the patch subject addressed, patches 1 and 3 are

Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

2 and 4 look good to me. Hawking or John should a take look though.

Thanks,
Lijo

>>
>> Thanks,
>> Lijo
>>
>>> +	ret = aldebaran_check_ecc_table_support(smu);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	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;
>>> +	}
>>> +
>>> +	ecc_table = (EccInfoTable_t *)smu_table->ecc_table;
>>> +
>>> +	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 +2036,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;
>>>

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

[Zhang, Hawking]

[AMD Official Use Only]

Series looks good to me.

Reviewed-by: Hawking Zhang <Hawking.Zhang@amd.com>

Regards,
Hawking

-----Original Message-----
From: Lazar, Lijo <Lijo.Lazar@amd.com> 
Sent: Thursday, November 18, 2021 22:41
To: 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>
Subject: Re: 回复: [PATCH Review 3/4] drm/amdgpu: add message smu to get ecc_table v2



On 11/18/2021 6:05 PM, Yang, Stanley wrote:
> [AMD Official Use Only]
> 
> 
> 
>> -----邮件原件-----
>> 发件人: Lazar, Lijo <Lijo.Lazar@amd.com>
>> 发送时间: Thursday, November 18, 2021 7:33 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 v2
>>
>>
>>
>> On 11/18/2021 3:03 PM, Stanley.Yang wrote:
>>> support ECC TABLE message, this table include umc ras error count 
>>> and error address
>>>
>>> v2:
>>>       add smu version check to query whether support ecctable
>>>       call smu_cmn_update_table to get ecctable directly
>>>
>>> Signed-off-by: Stanley.Yang <Stanley.Yang@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  8 +++
>>>    drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 14 ++++
>>>    .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c    | 70
>> +++++++++++++++++++
>>>    .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +
>>>    4 files changed, 94 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..7a06021a58f0 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 {
>>> @@ -1397,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..fd3b6b460b12 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> @@ -3072,6 +3072,20 @@ 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 = -EOPNOTSUPP;
>>> +
>>> +	mutex_lock(&smu->mutex);
>>> +	if (smu->ppt_funcs &&
>>> +		smu->ppt_funcs->get_ecc_info)
>>> +		ret = smu->ppt_funcs->get_ecc_info(smu, umc_ecc);
>>> +	mutex_unlock(&smu->mutex);
>>> +
>>> +	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/aldebaran_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> index f835d86cc2f5..4c21609ccea5 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> @@ -78,6 +78,12 @@
>>>
>>>    #define smnPCIE_ESM_CTRL			0x111003D0
>>>
>>> +/*
>>> + * SMU support ECCTABLE since version 68.42.0,
>>> + * use this to check ECCTALE feature whether support  */ #define 
>>> +SUPPORT_ECCTABLE_SMU_VERSION 0x00442a00
>>> +
>>>    static const struct smu_temperature_range smu13_thermal_policy[] =
>>>    {
>>>    	{-273150,  99000, 99000, -273150, 99000, 99000, -273150, 99000, 
>>> 99000}, @@ -190,6 +196,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 
>>> +230,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 +245,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 +1779,61 @@ static ssize_t 
>>> aldebaran_get_gpu_metrics(struct
>> smu_context *smu,
>>>    	return sizeof(struct gpu_metrics_v1_3);
>>>    }
>>>
>>> +static int aldebaran_check_ecc_table_support(struct smu_context 
>>> +*smu) {
>>> +	uint32_t if_version = 0xff, smu_version = 0xff;
>>> +	int ret = 0;
>>> +
>>> +	ret = smu_cmn_get_smc_version(smu, &if_version, &smu_version);
>>> +	if (ret)
>>> +		ret = -EOPNOTSUPP;	// return not support if failed get

Nitpick - comment style

>> smu_version
>>> +
>>> +	if (smu_version < SUPPORT_ECCTABLE_SMU_VERSION)
>>> +		ret = -EOPNOTSUPP;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +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 = NULL;
>>> +	struct ecc_info_per_ch *ecc_info_per_channel = NULL;
>>> +	int i, ret = 0;
>>> +	struct umc_ecc_info *eccinfo = (struct umc_ecc_info *)table;
>>> +
>>
>> Missed to ask last time. Since umc_ecc_info is a common struct, do 
>> you also want to pass back the number of channels having data?
>>
>> Now this struct can hold max of 32 channel data. Let's say if the 
>> same interface is going to be used on another ASIC X having only 16 channels.
>> Then the callback for ASIC X fills data only for 16 channels. Or, you 
>> expect that to be taken care at the caller side?
> 
> [Yang, Stanley] : If ASIC X have only 16 channels, the callback only fill data for 16 channels, and caller side also need consider its own channel number to handle with umc_ecc_info.
> 

Thanks for the details. With the nitpick above and Evan's comments on the patch subject addressed, patches 1 and 3 are

Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

2 and 4 look good to me. Hawking or John should a take look though.

Thanks,
Lijo

>>
>> Thanks,
>> Lijo
>>
>>> +	ret = aldebaran_check_ecc_table_support(smu);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	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;
>>> +	}
>>> +
>>> +	ecc_table = (EccInfoTable_t *)smu_table->ecc_table;
>>> +
>>> +	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 +2036,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;
>>>