On 8/14/2024 16:30, Ilpo Järvinen wrote:
> 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?
Yes. That's right.
>
>> + 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).
>
Other remarks have been addressed in the next revision.
Thanks,
Shyam
> 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[] = {
>>
>
