Re: [PATCH 1/1] amdgpu/pm: restructure reporting of clock values by smu

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 11/17/2021 11:50 AM, Darren Powell wrote:
>   Use of sysfs_emit by each of the specific device implementations is problematic.
>   To remove this back to a higher level, this patch adds a new function "get_clock_levels"
>   to the power-management API (amd_pm_funcs) and smu powerplay API (pptable_funcs). The
>   function returns an array of values to the power management module, which can then
>   interpret the array and print the values at the top layer.
>   This patch is only implemented for navi10 for some clocks to seek comment on the
>   implementation before extending the implementation to other clock values and devices.
>
>   == Changes ==
>    - new power management function get_clock_levels implemented by smu_get_ppclk_levels()
>    - new power management function amdgpu_dpm_print_clock_array to print most common clock
>         list, which can be later extended to use different units (currently only MHz), or
>         other functions added to handle more complex structure (eg pcie clocks)
>    - new powerplay function get_clk_levels implemented by navi10_get_clk_levels()
>    - new helper function smu_convert_to_smuclk called by smu_print_ppclk_levels and
>         smu_get_ppclk_levels
>    - new error return value -EOPNOTSUPP for clock levels not recognized, allowing fallback
>         to print_clock_levels. (NOTE: If pm or dpm not enabled current implementation will
>         try and fail with both methods)
>    - new error return value -EINVAL for unsupported clock level
>    - new error messages output for error conditions encountered at the device specific layer
>         which can be removed if suggested
>
>   == Test ==
>   LOGFILE=sysfs_emit.test.log
>   cd ~/workspace/linux
>   cp ${LOGFILE}{,.old}
>
>   AMDGPU_PCI_ADDR=`lspci -nn | grep "VGA\|Display" | cut -d " " -f 1`
>   AMDGPU_HWMON=`ls -la /sys/class/hwmon | grep $AMDGPU_PCI_ADDR | awk '{print $9}'`
>   HWMON_DIR=/sys/class/hwmon/${AMDGPU_HWMON}
>
>   lspci -nn | grep "VGA\|Display"  > $LOGFILE
>   FILES="pp_dpm_sclk
>   pp_dpm_pcie
>   "

There hasn't been any problem with sysfs_emit for these nodes.

The problem is with this kind of usage -

                size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
                size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK, 
buf+size);
                size += amdgpu_dpm_print_clock_levels(adev, 
OD_VDDC_CURVE, buf+size);
                size += amdgpu_dpm_print_clock_levels(adev, 
OD_VDDGFX_OFFSET, buf+size);
                size += amdgpu_dpm_print_clock_levels(adev, OD_RANGE, 
buf+size);
                size += amdgpu_dpm_print_clock_levels(adev, OD_CCLK, 
buf+size);


It's mixing a set of data in a particular node. Then the node needs to 
pass buf and offset separately to use sysfs_emit_at

Even then, if you notice, not every data is a clock level data. Some 
like VDDC_CURVE is ASIC specific and have a different format than clock 
DPM level data.

The max number of DPM levels are ASIC specific. In the below 
implementation a max of 8 levels is assumed for all ASICs, but there are 
ASICs which support 16 levels for a particualr clock. In fact the 
implementation needs to pass max+1 as in arg for list_length.

Sometimes, the current frequency may not match the exact DPM level, the 
approximation of what is the current level is ASIC specific. So instead 
of current freq, ASIC implementation needs to pass levels and the 
current level.

In general, the problem with sysfs_emit_at is not related to a common 
print format. pp_od_clk_voltage node is a representation of the problem 
- a node which is an aggregator of different data formats. Also, it is 
difficult to narrow down to a single one across ASICs given that there 
could be changes from one to next.

Thanks,
Lijo



>   for f in $FILES
>   do
>     echo === $f === >> $LOGFILE
>     cat $HWMON_DIR/device/$f >> $LOGFILE
>   done
>   cat $LOGFILE
>
> Signed-off-by: Darren Powell <darren.powell@xxxxxxx>
> ---
>   .../gpu/drm/amd/include/kgd_pp_interface.h    |  2 +
>   drivers/gpu/drm/amd/pm/amdgpu_pm.c            | 41 +++++++-
>   drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h       |  9 ++
>   drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 53 +++++++++-
>   .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c   | 96 +++++++++++++++++++
>   5 files changed, 192 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> index bac15c466733..fbe045811e60 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -310,6 +310,8 @@ struct amd_pm_funcs {
>   	int (*get_fan_speed_pwm)(void *handle, u32 *speed);
>   	int (*force_clock_level)(void *handle, enum pp_clock_type type, uint32_t mask);
>   	int (*print_clock_levels)(void *handle, enum pp_clock_type type, char *buf);
> +	int (*get_clock_levels)(void *handle, enum pp_clock_type type,
> +				uint32_t *clock_list, uint max_list_length);
>   	int (*force_performance_level)(void *handle, enum amd_dpm_forced_level level);
>   	int (*get_sclk_od)(void *handle);
>   	int (*set_sclk_od)(void *handle, uint32_t value);
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index 41472ed99253..ca7a6779e59b 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -1020,6 +1020,23 @@ static ssize_t amdgpu_get_pp_features(struct device *dev,
>   	return size;
>   }
>   
> +static int amdgpu_dpm_print_clock_array(enum pp_clock_type type,
> +					char *buf,
> +					uint32_t *clock_list, uint list_length) {
> +
> +	uint i;
> +	int size = 0;
> +
> +	for (i = 1; i < list_length; i++) {
> +		size += sysfs_emit_at(buf, size, "%d: %uMhz %s\n", i, clock_list[i],
> +				clock_list[i] == clock_list[0] ? "*" : "");
> +	}
> +
> +
> +	return size;
> +}
> +
> +
>   /**
>    * DOC: pp_dpm_sclk pp_dpm_mclk pp_dpm_socclk pp_dpm_fclk pp_dpm_dcefclk pp_dpm_pcie
>    *
> @@ -1058,6 +1075,7 @@ static ssize_t amdgpu_get_pp_dpm_clock(struct device *dev,
>   	struct amdgpu_device *adev = drm_to_adev(ddev);
>   	ssize_t size;
>   	int ret;
> +	uint32_t clock_list[8];
>   
>   	if (amdgpu_in_reset(adev))
>   		return -EPERM;
> @@ -1070,10 +1088,24 @@ static ssize_t amdgpu_get_pp_dpm_clock(struct device *dev,
>   		return ret;
>   	}
>   
> -	if (adev->powerplay.pp_funcs->print_clock_levels)
> -		size = amdgpu_dpm_print_clock_levels(adev, type, buf);
> -	else
> -		size = sysfs_emit(buf, "\n");
> +	if (adev->powerplay.pp_funcs->get_clock_levels) {
> +		ret = adev->powerplay.pp_funcs->get_clock_levels(
> +					adev->powerplay.pp_handle, type, clock_list, 8);
> +
> +		if (ret > 0)
> +			size = amdgpu_dpm_print_clock_array(type, buf, clock_list, ret);
> +	}
> +	else {
> +		ret = -EOPNOTSUPP;
> +	}
> +
> +	if  (ret == -EOPNOTSUPP)  {
> +		if (adev->powerplay.pp_funcs->print_clock_levels) {
> +			size = amdgpu_dpm_print_clock_levels(adev, type, buf);
> +		}
> +		else
> +			size = sysfs_emit(buf, "\n");
> +	}
>   
>   	pm_runtime_mark_last_busy(ddev->dev);
>   	pm_runtime_put_autosuspend(ddev->dev);
> @@ -1159,6 +1191,7 @@ static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev,
>   		struct device_attribute *attr,
>   		char *buf)
>   {
> +	dev_warn(dev, " %s ENTRY", __func__);
>   	return amdgpu_get_pp_dpm_clock(dev, PP_SCLK, buf);
>   }
>   
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> index 3557f4e7fc30..d8c20a134475 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> @@ -610,6 +610,15 @@ struct pptable_funcs {
>   	 */
>   	int (*print_clk_levels)(struct smu_context *smu, enum smu_clk_type clk_type, char *buf);
>   
> +	/**
> +	 * @get_clk_levels: Get array DPM clock levels for a clock domain
> +	 *                    to array clock_list. current level is first element.
> +	 *
> +	 * Used for sysfs interfaces.
> +	 */
> +	int (*get_clk_levels)(struct smu_context *smu, enum smu_clk_type clk_type,
> +			      uint32_t *clock_list, uint max_list_length);
> +
>   	/**
>   	 * @force_clk_levels: Set a range of allowed DPM levels for a clock
>   	 *                    domain.
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 01168b8955bf..2de566118422 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -2395,11 +2395,8 @@ static int smu_print_smuclk_levels(struct smu_context *smu, enum smu_clk_type cl
>   	return ret;
>   }
>   
> -static int smu_print_ppclk_levels(void *handle,
> -				  enum pp_clock_type type,
> -				  char *buf)
> +static enum smu_clk_type smu_convert_to_smuclk(enum pp_clock_type type)
>   {
> -	struct smu_context *smu = handle;
>   	enum smu_clk_type clk_type;
>   
>   	switch (type) {
> @@ -2432,12 +2429,57 @@ static int smu_print_ppclk_levels(void *handle,
>   	case OD_CCLK:
>   		clk_type = SMU_OD_CCLK; break;
>   	default:
> -		return -EINVAL;
> +		clk_type = SMU_CLK_COUNT; break;
>   	}
>   
> +	return clk_type;
> +}
> +
> +static int smu_print_ppclk_levels(void *handle,
> +				  enum pp_clock_type type,
> +				  char *buf)
> +{
> +	struct smu_context *smu = handle;
> +	enum smu_clk_type clk_type;
> +
> +	clk_type = smu_convert_to_smuclk(type);
> +	if (clk_type == SMU_CLK_COUNT)
> +		return -EINVAL;
> +
>   	return smu_print_smuclk_levels(smu, clk_type, buf);
>   }
>   
> +static int smu_get_ppclk_levels(void *handle,
> +				enum pp_clock_type type,
> +				uint32_t *clock_list, uint max_list_length)
> +{
> +	struct smu_context *smu = handle;
> +	enum smu_clk_type clk_type;
> +	int ret = 0;
> +
> +	clk_type = smu_convert_to_smuclk(type);
> +	if (clk_type == SMU_CLK_COUNT)
> +		return -EINVAL;
> +
> +
> +	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> +		return -EOPNOTSUPP;
> +
> +
> +	if (smu->ppt_funcs->get_clk_levels) {
> +
> +		mutex_lock(&smu->mutex);
> +		ret = smu->ppt_funcs->get_clk_levels(smu, clk_type, clock_list, max_list_length);
> +		mutex_unlock(&smu->mutex);
> +	}
> +	else {
> +		ret = -EOPNOTSUPP;
> +	}
> +
> +
> +	return ret;
> +}
> +
>   static int smu_od_edit_dpm_table(void *handle,
>   				 enum PP_OD_DPM_TABLE_COMMAND type,
>   				 long *input, uint32_t size)
> @@ -3100,6 +3142,7 @@ static const struct amd_pm_funcs swsmu_pm_funcs = {
>   	.set_fan_speed_pwm   = smu_set_fan_speed_pwm,
>   	.get_fan_speed_pwm   = smu_get_fan_speed_pwm,
>   	.force_clock_level       = smu_force_ppclk_levels,
> +	.get_clock_levels        = smu_get_ppclk_levels,
>   	.print_clock_levels      = smu_print_ppclk_levels,
>   	.force_performance_level = smu_force_performance_level,
>   	.read_sensor             = smu_read_sensor,
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> index 71161f6b78fe..55e6880c2964 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> @@ -1261,6 +1261,101 @@ static void navi10_od_setting_get_range(struct smu_11_0_overdrive_table *od_tabl
>   		*max = od_table->max[setting];
>   }
>   
> +static int navi10_get_clk_levels(struct smu_context *smu,
> +				 enum smu_clk_type clk_type,
> +				 uint32_t *clock_list, uint max_list_length)
> +{
> +	struct amdgpu_device *adev = smu->adev;
> +	int size = 0, ret = 0;
> +	uint32_t cur_value = 0, value = 0, count = 0, i;
> +	uint32_t freq_values[3] = {0};
> +	uint32_t mark_index = 0;
> +
> +	switch (clk_type) {
> +	case SMU_GFXCLK:
> +	case SMU_SCLK:
> +	case SMU_SOCCLK:
> +	case SMU_MCLK:
> +	case SMU_UCLK:
> +	case SMU_FCLK:
> +	case SMU_VCLK:
> +	case SMU_DCEFCLK:
> +	case SMU_DCLK:
> +		ret = navi10_get_current_clk_freq_by_table(smu, clk_type, &cur_value);
> +		if (ret) {
> +			dev_err(adev->dev, " %s Unable to retrieve clk freq : %u", __func__, clk_type);
> +			return -EIO;
> +		}
> +
> +		ret = smu_v11_0_get_dpm_level_count(smu, clk_type, &count);
> +		if (ret) {
> +			dev_err(adev->dev, " %s Unable to retrieve clk count : %u", __func__, clk_type);
> +			return -EIO;
> +		}
> +
> +		if (count > max_list_length-1) {
> +			dev_err(adev->dev, " %s Too many clock values", __func__);
> +			return -EPERM;
> +		}
> +
> +
> +		clock_list[0] = cur_value;
> +		if (!navi10_is_support_fine_grained_dpm(smu, clk_type)) {
> +			for (i = 0; i < count; i++) {
> +				ret = smu_v11_0_get_dpm_freq_by_index(smu, clk_type, i, &value);
> +				if (ret) {
> +					dev_err(adev->dev, " %s Unable to retrieve clk freq : %u", __func__, clk_type);
> +					return -EIO;
> +				}
> +
> +				clock_list[i+1] = value;
> +			}
> +			size = count+1;
> +		} else {
> +			// for fine-grained dpm, display cur_value as average of max and min
> +			ret = smu_v11_0_get_dpm_freq_by_index(smu, clk_type, 0, &freq_values[0]);
> +			if (ret) {
> +				dev_err(adev->dev, " %s Unable to retrieve clk freq : %u", __func__, clk_type);
> +				return -EIO;
> +			}
> +			ret = smu_v11_0_get_dpm_freq_by_index(smu, clk_type, count - 1, &freq_values[2]);
> +			if (ret) {
> +				dev_err(adev->dev, " %s Unable to retrieve clk freq : %u", __func__, clk_type);
> +				return -EIO;
> +			}
> +
> +			freq_values[1] = cur_value;
> +			mark_index = cur_value == freq_values[0] ? 0 :
> +				     cur_value == freq_values[2] ? 2 : 1;
> +			if (mark_index != 1)
> +				freq_values[1] = (freq_values[0] + freq_values[2]) / 2;
> +
> +			clock_list[0] = freq_values[1];
> +			for (i = 0; i < 3; i++) {
> +				clock_list[i+1] = freq_values[i];
> +			}
> +
> +			size = 3+1;
> +		}
> +		break;
> +	case SMU_PCIE:
> +	case SMU_OD_SCLK:
> +	case SMU_OD_MCLK:
> +	case SMU_OD_VDDC_CURVE:
> +	case SMU_OD_RANGE:
> +		dev_warn(adev->dev, " %s Unhandled clk_type : %u", __func__, clk_type);
> +		size = -EOPNOTSUPP;
> +		break;
> +
> +	default:
> +		dev_err(adev->dev, " %s Unknown clk_type : %u", __func__, clk_type);
> +		size = -EINVAL;
> +		break;
> +	}
> +
> +	return size;
> +}
> +
>   static int navi10_print_clk_levels(struct smu_context *smu,
>   			enum smu_clk_type clk_type, char *buf)
>   {
> @@ -3241,6 +3336,7 @@ static const struct pptable_funcs navi10_ppt_funcs = {
>   	.dpm_set_jpeg_enable = navi10_dpm_set_jpeg_enable,
>   	.i2c_init = navi10_i2c_control_init,
>   	.i2c_fini = navi10_i2c_control_fini,
> +	.get_clk_levels = navi10_get_clk_levels,
>   	.print_clk_levels = navi10_print_clk_levels,
>   	.force_clk_levels = navi10_force_clk_levels,
>   	.populate_umd_state_clk = navi10_populate_umd_state_clk,
>
> base-commit: eabeb4f20a0786188fba07a2dd1b0a614c4e15f6