Re: [PATCH v1 08/15] platform/x86/amd/pmf: Get performance metrics from PMFW

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 7/12/22 16:58, Shyam Sundar S K wrote:
> PMF driver polls for metrics information from PMFW to understand the system
> behavior, power consumption etc.
> 
> This metrics table information will be used the PMF features to tweak the
> thermal heuristics. The poll duration can also be changed by the user
> by changing the poll duration time.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/core.c | 57 +++++++++++++++++++++++++++++
>  drivers/platform/x86/amd/pmf/pmf.h  | 39 ++++++++++++++++++++
>  2 files changed, 96 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index ff26928e6a49..c6fd52c46818 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -47,6 +47,11 @@
>  #define DELAY_MIN_US	2000
>  #define DELAY_MAX_US	3000
>  
> +/* override Metrics Table sample size time (in ms) */
> +static int metrics_table_loop_ms = 1000;
> +module_param(metrics_table_loop_ms, int, 0644);
> +MODULE_PARM_DESC(metrics_table_loop_ms, " Metrics Table sample size time (default = 1000ms) ");
> +
>  #ifdef CONFIG_DEBUG_FS
>  static int current_power_limits_show(struct seq_file *seq, void *unused)
>  {
> @@ -98,6 +103,30 @@ int amd_pmf_get_power_source(void)
>  		return POWER_SOURCE_DC;
>  }
>  
> +static void amd_pmf_get_metrics(struct work_struct *work)
> +{
> +	struct amd_pmf_dev *dev = container_of(work, struct amd_pmf_dev, work_buffer.work);
> +	enum platform_profile_option current_profile;
> +	ktime_t time_elapsed_ms;
> +	int socket_power;
> +
> +	/* Get the current profile information */
> +	platform_profile_get(&current_profile);

This can simply be written as:

	current_profile = READ_ONCE(dev->current_profile);

Avoiding the need to add the platform_profile_get() helper and allowing
the dropping of patch 1/15.

> +
> +	/* Transfer table contents */
> +	memset(&dev->m_table, 0, sizeof(dev->m_table));

memset dev->buf I presume, not dev->m_table ?

> +	amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> +	memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
> +
> +	time_elapsed_ms = ktime_to_ms(ktime_get()) - dev->start_time;
> +	/* Calculate the avg SoC power consumption */
> +	socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
> +
> +	dev->start_time = ktime_to_ms(ktime_get());
> +	dev_dbg(dev->dev, "Metrics table sample size time:%d\n", metrics_table_loop_ms);
> +	schedule_delayed_work(&dev->work_buffer, msecs_to_jiffies(metrics_table_loop_ms));
> +}
> +
>  static inline u32 amd_pmf_reg_read(struct amd_pmf_dev *dev, int reg_offset)
>  {
>  	return ioread32(dev->regbase + reg_offset);
> @@ -191,6 +220,34 @@ static const struct pci_device_id pmf_pci_ids[] = {
>  	{ }
>  };
>  
> +int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev)
> +{
> +	struct smu_pmf_metrics *m_table;

Using a local variable just to use sizeof on it is weird,
please drop this.

> +	u64 phys_addr;
> +
> +	INIT_DELAYED_WORK(&dev->work_buffer, amd_pmf_get_metrics);
> +
> +	/*
> +	 * Start collecting the metrics data after a small delay
> +	 * or else, we might end up getting stale values from PMFW.
> +	 */
> +	schedule_delayed_work(&dev->work_buffer, msecs_to_jiffies(metrics_table_loop_ms * 3));

This work relies on dev->buf != NULL and on the DRAM address being
set, so this schedule_delayed_work() must be moved to lower in this function.

> +
> +	/* Get Metrics Table Address */
> +	dev->buf = kzalloc(sizeof(*m_table), GFP_KERNEL);

Use sizeof(dev->m_table) here, this is also what is used in
amd_pmf_get_metrics().

> +	if (!dev->buf)

Moving it lower also fixes it still running when existing with an error here.

> +		return -ENOMEM;
> +
> +	phys_addr = virt_to_phys(dev->buf);
> +	dev->hi = phys_addr >> 32;
> +	dev->low = phys_addr & GENMASK(31, 0);
> +
> +	amd_pmf_send_cmd(dev, SET_DRAM_ADDR_HIGH, 0, dev->hi, NULL);
> +	amd_pmf_send_cmd(dev, SET_DRAM_ADDR_LOW, 0, dev->low, NULL);

Are dev->hi and dev>low only ever used here? It seems so, in that case
please use local function variables for them instead of storing them
in the amd_pmf_dev struct.

> +

This is where the schedule_work() should be done.

> +	return 0;
> +}
> +
>  static void amd_pmf_init_features(struct amd_pmf_dev *dev)
>  {
>  	/* Enable Static Slider */
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 1e9e2e498d15..8f318ff59c2e 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -84,6 +84,41 @@ struct apmf_fan_idx {
>  	u32 fan_ctl_idx;
>  } __packed;
>  
> +struct smu_pmf_metrics {
> +	u16 gfxclk_freq; /* in MHz */
> +	u16 socclk_freq; /* in MHz */
> +	u16 vclk_freq; /* in MHz */
> +	u16 dclk_freq; /* in MHz */
> +	u16 memclk_freq; /* in MHz */
> +	u16 spare;
> +	u16 gfx_activity; /* in Centi */
> +	u16 uvd_activity; /* in Centi */
> +	u16 voltage[2]; /* in mV */
> +	u16 currents[2]; /* in mA */
> +	u16 power[2];/* in mW */
> +	u16 core_freq[8]; /* in MHz */
> +	u16 core_power[8]; /* in mW */
> +	u16 core_temp[8]; /* in centi-Celsius */
> +	u16 l3_freq; /* in MHz */
> +	u16 l3_temp; /* in centi-Celsius */
> +	u16 gfx_temp; /* in centi-Celsius */
> +	u16 soc_temp; /* in centi-Celsius */
> +	u16 throttler_status;
> +	u16 current_socketpower; /* in mW */
> +	u16 stapm_orig_limit; /* in W */
> +	u16 stapm_cur_limit; /* in W */
> +	u32 apu_power; /* in mW */
> +	u32 dgpu_power; /* in mW */
> +	u16 vdd_tdc_val; /* in mA */
> +	u16 soc_tdc_val; /* in mA */
> +	u16 vdd_edc_val; /* in mA */
> +	u16 soc_edcv_al; /* in mA */
> +	u16 infra_cpu_maxfreq; /* in MHz */
> +	u16 infra_gfx_maxfreq; /* in MHz */
> +	u16 skin_temp; /* in centi-Celsius */
> +	u16 device_state;
> +};
> +
>  enum amd_stt_skin_temp {
>  	STT_TEMP_APU,
>  	STT_TEMP_HS2,
> @@ -120,8 +155,11 @@ struct amd_pmf_dev {
>  	struct apmf_if *apmf_if;
>  	enum platform_profile_option current_profile;
>  	struct platform_profile_handler pprof;
> +	struct smu_pmf_metrics m_table;
> +	struct delayed_work work_buffer;
>  	struct delayed_work heart_beat;
>  	struct mutex lock; /* protects the PMF interface */
> +	ktime_t start_time;
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>  	struct dentry *dbgfs_dir;
>  #endif /* CONFIG_DEBUG_FS */
> @@ -158,6 +196,7 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
>  void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
>  int is_apmf_func_supported(unsigned long index);
>  int amd_pmf_send_cmd(struct amd_pmf_dev *dev, u8 message, bool get, u32 arg, u32 *data);
> +int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev);
>  int amd_pmf_get_power_source(void);
>  
>  /* SPS Layer */


Regards,

Hans