Re: [PATCH v2 06/16] platform/x86/amd/pmf: Add support to get inputs from other subsystems

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

On 10/4/2023 5:44 PM, Ilpo Järvinen wrote:
> On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> 
>> PMF driver sends changing inputs from each subystem to TA for evaluating
>> the conditions in the policy binary.
>>
>> Add initial support of plumbing in the PMF driver for Smart PC to get
>> information from other subsystems in the kernel.
>>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmf/Makefile |   2 +-
>>  drivers/platform/x86/amd/pmf/pmf.h    |  18 ++++
>>  drivers/platform/x86/amd/pmf/spc.c    | 119 ++++++++++++++++++++++++++
>>  drivers/platform/x86/amd/pmf/tee-if.c |   3 +
>>  4 files changed, 141 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/platform/x86/amd/pmf/spc.c
>>
>> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
>> index d2746ee7369f..6b26e48ce8ad 100644
>> --- a/drivers/platform/x86/amd/pmf/Makefile
>> +++ b/drivers/platform/x86/amd/pmf/Makefile
>> @@ -7,4 +7,4 @@
>>  obj-$(CONFIG_AMD_PMF) += amd-pmf.o
>>  amd-pmf-objs := core.o acpi.o sps.o \
>>  		auto-mode.o cnqf.o \
>> -		tee-if.o
>> +		tee-if.o spc.o
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 6f4b6f4ecee4..60b11455dadf 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -149,6 +149,21 @@ struct smu_pmf_metrics {
>>  	u16 infra_gfx_maxfreq; /* in MHz */
>>  	u16 skin_temp; /* in centi-Celsius */
>>  	u16 device_state;
>> +	u16 curtemp; /* in centi-Celsius */
>> +	u16 filter_alpha_value;
>> +	u16 avg_gfx_clkfrequency;
>> +	u16 avg_fclk_frequency;
>> +	u16 avg_gfx_activity;
>> +	u16 avg_socclk_frequency;
>> +	u16 avg_vclk_frequency;
>> +	u16 avg_vcn_activity;
>> +	u16 avg_dram_reads;
>> +	u16 avg_dram_writes;
>> +	u16 avg_socket_power;
>> +	u16 avg_core_power[2];
>> +	u16 avg_core_c0residency[16];
>> +	u16 spare1;
>> +	u32 metrics_counter;
>>  } __packed;
>>  
>>  enum amd_stt_skin_temp {
>> @@ -595,4 +610,7 @@ extern const struct attribute_group cnqf_feature_attribute_group;
>>  int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
>>  void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
>>  int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
>> +
>> +/* Smart PC - TA interfaces */
>> +void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
>>  #endif /* PMF_H */
>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
>> new file mode 100644
>> index 000000000000..3113bde051d9
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>> @@ -0,0 +1,119 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD Platform Management Framework Driver - Smart PC Capabilities
>> + *
>> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> + *          Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> + */
>> +
>> +#include <acpi/button.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/units.h>
>> +#include "pmf.h"
>> +
>> +static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, 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 C0 residency of all the cores */
>> +	for (i = 0; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++)
>> +		avg += dev->m_table.avg_core_c0residency[i];
>> +
>> +	/* get the max C0 residency of all the cores */
>> +	max = dev->m_table.avg_core_c0residency[0];
>> +	for (i = 1; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++) {
>> +		if (dev->m_table.avg_core_c0residency[i] > max)
>> +			max = dev->m_table.avg_core_c0residency[i];
>> +	}
> 
> My comments weren't either answered adequately or changes made here.
> Please check the v1 comments. I hope it's not because you feel hurry to 
> get the next version out...
> 
> I'm still unsure if the u16 thing can overflow because I don't know what's 
> the max value for avg_core_c0residency[i].

the highest value for avg_core_c0residency[i] is merely a small number
and hence I retained the avg variable as u16. Not sure if there a
'real' case where it can overflow.

Sorry, I missed to merge both into a single for loop. I will address
this in v3.

Thanks,
Shyam

> 
>> +
>> +	in->ev_info.avg_c0residency = avg / ARRAY_SIZE(dev->m_table.avg_core_c0residency);
>> +	in->ev_info.max_c0residency = max;
>> +	in->ev_info.gfx_busy = dev->m_table.avg_gfx_activity;
>> +}
>> +
>> +static const char * const pmf_battery_supply_name[] = {
>> +	"BATT",
>> +	"BAT0",
>> +};
>> +
>> +static int get_battery_prop(enum power_supply_property prop)
>> +{
>> +	union power_supply_propval value;
>> +	struct power_supply *psy;
>> +	int i, ret = -EINVAL;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(pmf_battery_supply_name); i++) {
>> +		psy = power_supply_get_by_name(pmf_battery_supply_name[i]);
>> +		if (!psy)
>> +			continue;
>> +
>> +		ret = power_supply_get_property(psy, prop, &value);
>> +		if (ret) {
>> +			power_supply_put(psy);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return value.intval;
>> +}
>> +
>> +static int amd_pmf_get_battery_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
>> +{
>> +	int val;
>> +
>> +	val = get_battery_prop(POWER_SUPPLY_PROP_PRESENT);
>> +	if (val != 1)
>> +		return -EINVAL;
>> +
>> +	in->ev_info.bat_percentage = get_battery_prop(POWER_SUPPLY_PROP_CAPACITY);
>> +	/* all values in mWh metrics */
>> +	in->ev_info.bat_design = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN) / MILLI;
>> +	in->ev_info.full_charge_capacity = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL) / MILLI;
>> +	in->ev_info.drain_rate = get_battery_prop(POWER_SUPPLY_PROP_POWER_NOW) / MILLI;
> 
> There are defines specifically for watts so you should use them rather 
> than generic MILLI.
> 
>