On Tue, 13 Aug 2024, Shyam Sundar S K wrote:
> The SMU metrics table has been revised for the 1AH family series.
> Introduce a new metrics table structure to retrieve comprehensive metrics
> information from the PMFW. This information will be utilized by the PMF
> driver to adjust system thermals.
>
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> ---
> v3:
> - Address remarks from Ilpo on the helper for C0 residency calculation
>
> v2:
> - Align comments
> - add helper for max and avg calculation of C0 residency
>
> drivers/platform/x86/amd/pmf/core.c | 14 +++++++-
> drivers/platform/x86/amd/pmf/pmf.h | 49 ++++++++++++++++++++++++++
> drivers/platform/x86/amd/pmf/spc.c | 53 ++++++++++++++++++++---------
> 3 files changed, 99 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 88314b0277a3..0ba9045224d9 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -255,7 +255,19 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
>
> /* Get Metrics Table Address */
> if (alloc_buffer) {
> - dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
> + switch (dev->cpu_id) {
> + case AMD_CPU_ID_PS:
> + case AMD_CPU_ID_RMB:
> + dev->mtable_size = sizeof(dev->m_table);
> + break;
> + case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> + dev->mtable_size = sizeof(dev->m_table_v2);
> + break;
> + default:
> + dev_err(dev->dev, "Invalid cpu id: 0x%x", dev->cpu_id);
CPU
> + }
> +
> + dev->buf = kzalloc(dev->mtable_size, GFP_KERNEL);
> if (!dev->buf)
> return -ENOMEM;
> }
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 9fc26f672f12..8ce8816da9c1 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -198,6 +198,53 @@ struct apmf_fan_idx {
> u32 fan_ctl_idx;
> } __packed;
>
> +struct smu_pmf_metrics_v2 {
> + u16 core_frequency[16]; /* MHz */
> + u16 core_power[16]; /* mW */
> + u16 core_temp[16]; /* centi-C */
> + u16 gfx_temp; /* centi-C */
> + u16 soc_temp; /* centi-C */
> + u16 stapm_opn_limit; /* mW */
> + u16 stapm_cur_limit; /* mW */
> + u16 infra_cpu_maxfreq; /* MHz */
> + u16 infra_gfx_maxfreq; /* MHz */
> + u16 skin_temp; /* centi-C */
> + u16 gfxclk_freq; /* MHz */
> + u16 fclk_freq; /* MHz */
> + u16 gfx_activity; /* GFX busy % [0-100] */
> + u16 socclk_freq; /* MHz */
> + u16 vclk_freq; /* MHz */
> + u16 vcn_activity; /* VCN busy % [0-100] */
> + u16 vpeclk_freq; /* MHz */
> + u16 ipuclk_freq; /* MHz */
> + u16 ipu_busy[8]; /* NPU busy % [0-100] */
> + u16 dram_reads; /* MB/sec */
> + u16 dram_writes; /* MB/sec */
> + u16 core_c0residency[16]; /* C0 residency % [0-100] */
> + u16 ipu_power; /* mW */
> + u32 apu_power; /* mW */
> + u32 gfx_power; /* mW */
> + u32 dgpu_power; /* mW */
> + u32 socket_power; /* mW */
> + u32 all_core_power; /* mW */
> + u32 filter_alpha_value; /* time constant [us] */
> + u32 metrics_counter;
> + u16 memclk_freq; /* MHz */
> + u16 mpipuclk_freq; /* MHz */
> + u16 ipu_reads; /* MB/sec */
> + u16 ipu_writes; /* MB/sec */
> + u32 throttle_residency_prochot;
> + u32 throttle_residency_spl;
> + u32 throttle_residency_fppt;
> + u32 throttle_residency_sppt;
> + u32 throttle_residency_thm_core;
> + u32 throttle_residency_thm_gfx;
> + u32 throttle_residency_thm_soc;
> + u16 psys;
> + u16 spare1;
> + u32 spare[6];
> +} __packed;
> +
> struct smu_pmf_metrics {
> u16 gfxclk_freq; /* in MHz */
> u16 socclk_freq; /* in MHz */
> @@ -295,6 +342,7 @@ struct amd_pmf_dev {
> int hb_interval; /* SBIOS heartbeat interval */
> struct delayed_work heart_beat;
> struct smu_pmf_metrics m_table;
> + struct smu_pmf_metrics_v2 m_table_v2;
> struct delayed_work work_buffer;
> ktime_t start_time;
> int socket_power_history[AVG_SAMPLE_SIZE];
> @@ -319,6 +367,7 @@ struct amd_pmf_dev {
> bool smart_pc_enabled;
> u16 pmf_if_version;
> struct input_dev *pmf_idev;
> + size_t mtable_size;
> };
>
> struct apmf_sps_prop_granular_v2 {
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index 3c153fb1425e..74a5e325b6c3 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -53,30 +53,51 @@ void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *
> void amd_pmf_dump_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in) {}
> #endif
>
> -static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +static void amd_pmf_get_c0_residency(u16 *core_residency, u16 size, struct ta_pmf_enact_table *in)
> {
> u16 max, avg = 0;
> int i;
>
> - memset(dev->buf, 0, sizeof(dev->m_table));
> - amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> - memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
> -
> - in->ev_info.socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
> - in->ev_info.skin_temperature = dev->m_table.skin_temp;
> -
> /* Get the avg and max C0 residency of all the cores */
> - max = dev->m_table.avg_core_c0residency[0];
> - for (i = 0; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++) {
> - avg += dev->m_table.avg_core_c0residency[i];
> - if (dev->m_table.avg_core_c0residency[i] > max)
> - max = dev->m_table.avg_core_c0residency[i];
> + max = *core_residency;
> + for (i = 0; i < size; i++) {
> + avg += core_residency[i];
IIRC, we already talked earlier about the possibility of overflow when
summing n u16 into one u16 and you said it was not possible. I hope that
hasn't changed with v2?
> + if (core_residency[i] > max)
> + max = core_residency[i];
> }
> -
> - avg = DIV_ROUND_CLOSEST(avg, ARRAY_SIZE(dev->m_table.avg_core_c0residency));
> + avg = DIV_ROUND_CLOSEST(avg, size);
> in->ev_info.avg_c0residency = avg;
> in->ev_info.max_c0residency = max;
> - in->ev_info.gfx_busy = dev->m_table.avg_gfx_activity;
> +}
> +
> +static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + u16 size;
> +
> + /* Get the updated metrics table data */
> + memset(dev->buf, 0, dev->mtable_size);
> + amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> +
> + switch (dev->cpu_id) {
> + case AMD_CPU_ID_PS:
> + memcpy(&dev->m_table, dev->buf, dev->mtable_size);
> + in->ev_info.socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
> + in->ev_info.skin_temperature = dev->m_table.skin_temp;
> + in->ev_info.gfx_busy = dev->m_table.avg_gfx_activity;
> + size = ARRAY_SIZE(dev->m_table.avg_core_c0residency);
> + amd_pmf_get_c0_residency(dev->m_table.avg_core_c0residency, size, in);
> + break;
> + case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> + memcpy(&dev->m_table_v2, dev->buf, dev->mtable_size);
> + in->ev_info.socket_power = dev->m_table_v2.apu_power + dev->m_table_v2.dgpu_power;
> + in->ev_info.skin_temperature = dev->m_table_v2.skin_temp;
> + in->ev_info.gfx_busy = dev->m_table_v2.gfx_activity;
> + size = ARRAY_SIZE(dev->m_table_v2.core_c0residency);
> + amd_pmf_get_c0_residency(dev->m_table_v2.core_c0residency, size, in);
Thanks, looks much cleaner now!
I don't think there's any reason for size to be u16 though (I'd have
expected the size parameter type to be either size_t or unsigned int).
I also don't find the extra local variable for size very useful but it's
up to you if you want to do that in two steps or directly within the call
(it's quite normal pattern to pass the "ptr, ARRAY_SIZE(ptr)" pair to a
function).
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> + break;
> + default:
> + dev_err(dev->dev, "Unsupported cpuid: 0x%x", dev->cpu_id);
> + }
> }
>
> static const char * const pmf_battery_supply_name[] = {
>
--
i.
