[AMD Official Use Only]
It seems this patch should be combined with patch1. Rather than as a separate patch.
BR
Evan
> -----Original Message-----
> From: Powell, Darren <Darren.Powell@xxxxxxx>
> Sent: Saturday, January 22, 2022 11:47 AM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Powell, Darren <Darren.Powell@xxxxxxx>
> Subject: [PATCH 3/3] amdgpu/pm: Add Error Handling to emit_clocks stack
>
> Previous implementation of print_clocks required return of bytes written
> to calling function via return value. Passing this value in by reference
> allows us now to pass back error codes up the calling stack.
>
> (v1)
> - Errors from get_current_clk_freq, get_dpm_level_count & get_dpm_freq
> now passed back up the stack
> - Added -EOPNOTSUPP when encountering for OD clocks
> !od_enabled
> missing od_table or od_settings
> - OD_RANGE continues to print any subset of the ODCAP settings it can find
> without reporting error for any missing
> - smu_emit_ppclk returns ENOENT if emit_clk_levels is not supported by the
> device
> - modified calling logic so fallback to print_clock_levels is only attempted
> if emit_clk_levels is not (yet) supported by the device
>
> (v2)
> - change return from amdgpu_dpm_emit_clock_levels if emit_clock_levels
> not found
> - updated documentation of pptable_func members print_clk_levels,
> emit_clk_levels
>
> == Test ==
> LOGFILE=pp_clk.test.log
> 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_od_clk_voltage
> pp_dpm_sclk
> pp_dpm_mclk
> pp_dpm_pcie
> pp_dpm_socclk
> pp_dpm_fclk
> pp_dpm_dcefclk
> pp_dpm_vclk
> pp_dpm_dclk "
>
> 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>
> ---
> drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 2 +-
> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 13 ++++++------
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 8 +++----
> drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 6 +++++-
> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 21 +++++++++---------
> -
> 5 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> index e45578124928..03a690a3abb7 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> @@ -915,7 +915,7 @@ int amdgpu_dpm_emit_clock_levels(struct
> amdgpu_device *adev,
> int ret = 0;
>
> if (!pp_funcs->emit_clock_levels)
> - return 0;
> + return -ENOENT;
>
> mutex_lock(&adev->pm.mutex);
> ret = pp_funcs->emit_clock_levels(adev->powerplay.pp_handle,
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index 97a8dcbe6eaf..a11def0ee761 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -855,13 +855,12 @@ static ssize_t
> amdgpu_get_pp_od_clk_voltage(struct device *dev,
> return ret;
> }
>
> - ret = amdgpu_dpm_emit_clock_levels(adev, od_clocks[0], buf,
> &size);
> - if (ret >= 0) {
> - /* Proceed with emit for other od clocks if the first call
> succeeds */
> - for(clk_index = 1 ; clk_index < 6 ; clk_index++)
> - amdgpu_dpm_emit_clock_levels(adev,
> od_clocks[clk_index], buf, &size);
> + for(clk_index = 0 ; clk_index < 6 ; clk_index++) {
> + ret = amdgpu_dpm_emit_clock_levels(adev,
> od_clocks[clk_index], buf, &size);
> + if (ret)
> + break;
> }
> - else {
> + if (ret == -ENOENT) {
> size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
> if (size > 0) {
> size += amdgpu_dpm_print_clock_levels(adev,
> OD_MCLK, buf+size);
> @@ -1014,7 +1013,7 @@ static ssize_t amdgpu_get_pp_dpm_clock(struct
> device *dev,
> }
>
> ret = amdgpu_dpm_emit_clock_levels(adev, type, buf, &size);
> - if (ret < 0)
> + if (ret == -ENOENT)
> size = amdgpu_dpm_print_clock_levels(adev, type, buf);
>
> if (size == 0)
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index d82ea7ee223f..29c101a93dc4 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -2454,12 +2454,10 @@ static int smu_emit_ppclk_levels(void *handle,
> enum pp_clock_type type, char *bu
> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> return -EOPNOTSUPP;
>
> - if (smu->ppt_funcs->emit_clk_levels) {
> + if (smu->ppt_funcs->emit_clk_levels)
> ret = smu->ppt_funcs->emit_clk_levels(smu, clk_type, buf,
> offset);
> - }
> - else {
> - ret = -EOPNOTSUPP;
> - }
> + else
> + ret = -ENOENT;
>
> return ret;
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> index 224b869eda70..1429581dca9c 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h
> @@ -612,6 +612,7 @@ struct pptable_funcs {
> * to buffer. Star current level.
> *
> * Used for sysfs interfaces.
> + * Return: Number of characters written to the buffer
> */
> int (*print_clk_levels)(struct smu_context *smu, enum
> smu_clk_type clk_type, char *buf);
>
> @@ -621,7 +622,10 @@ struct pptable_funcs {
> *
> * Used for sysfs interfaces.
> * &buf: sysfs buffer
> - * &offset: offset within buffer to start printing
> + * &offset: offset within buffer to start printing, which is updated by
> the
> + * function.
> + *
> + * Return: 0 on Success or Negative to indicate an error occurred.
> */
> int (*emit_clk_levels)(struct smu_context *smu, enum
> smu_clk_type clk_type, char *buf, int *offset);
>
> 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 50022de5337f..4bcef7d1a0d6 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> @@ -1278,7 +1278,6 @@ static int navi10_emit_clk_levels(struct
> smu_context *smu,
> (OverDriveTable_t *)table_context->overdrive_table;
> struct smu_11_0_overdrive_table *od_settings = smu->od_settings;
> uint32_t min_value, max_value;
> - int save_offset = *offset;
>
> switch (clk_type) {
> case SMU_GFXCLK:
> @@ -1292,17 +1291,17 @@ static int navi10_emit_clk_levels(struct
> smu_context *smu,
> case SMU_DCEFCLK:
> ret = navi10_get_current_clk_freq_by_table(smu, clk_type,
> &cur_value);
> if (ret)
> - return 0;
> + return ret;
>
> ret = smu_v11_0_get_dpm_level_count(smu, clk_type,
> &count);
> if (ret)
> - return 0;
> + return ret;
>
> 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)
> - return (*offset - save_offset);
> + return ret;
>
> *offset += sysfs_emit_at(buf, *offset,
> "%d: %uMhz %s\n", i, value,
> cur_value == value ? "*" : "");
> @@ -1311,10 +1310,10 @@ static int navi10_emit_clk_levels(struct
> smu_context *smu,
> } else {
> ret = smu_v11_0_get_dpm_freq_by_index(smu,
> clk_type, 0, &freq_values[0]);
> if (ret)
> - return 0;
> + return ret;
> ret = smu_v11_0_get_dpm_freq_by_index(smu,
> clk_type, count - 1, &freq_values[2]);
> if (ret)
> - return 0;
> + return ret;
>
> freq_values[1] = cur_value;
> mark_index = cur_value == freq_values[0] ? 0 :
> @@ -1355,7 +1354,7 @@ static int navi10_emit_clk_levels(struct
> smu_context *smu,
> break;
> case SMU_OD_SCLK:
> if (!smu->od_enabled || !od_table || !od_settings)
> - break;
> + return -EOPNOTSUPP;
> if (!navi10_od_feature_is_supported(od_settings,
> SMU_11_0_ODCAP_GFXCLK_LIMITS))
> break;
> *offset += sysfs_emit_at(buf, *offset,
> "OD_SCLK:\n0: %uMhz\n1: %uMhz\n",
> @@ -1363,14 +1362,14 @@ static int navi10_emit_clk_levels(struct
> smu_context *smu,
> break;
> case SMU_OD_MCLK:
> if (!smu->od_enabled || !od_table || !od_settings)
> - break;
> + return -EOPNOTSUPP;
> if (!navi10_od_feature_is_supported(od_settings,
> SMU_11_0_ODCAP_UCLK_MAX))
> break;
> *offset += sysfs_emit_at(buf, *offset,
> "OD_MCLK:\n1: %uMHz\n", od_table->UclkFmax);
> break;
> case SMU_OD_VDDC_CURVE:
> if (!smu->od_enabled || !od_table || !od_settings)
> - break;
> + return -EOPNOTSUPP;
> if (!navi10_od_feature_is_supported(od_settings,
> SMU_11_0_ODCAP_GFXCLK_CURVE))
> break;
> *offset += sysfs_emit_at(buf, *offset,
> "OD_VDDC_CURVE:\n");
> @@ -1395,7 +1394,7 @@ static int navi10_emit_clk_levels(struct
> smu_context *smu,
> break;
> case SMU_OD_RANGE:
> if (!smu->od_enabled || !od_table || !od_settings)
> - break;
> + return -EOPNOTSUPP;
> *offset += sysfs_emit_at(buf, *offset, "%s:\n",
> "OD_RANGE");
>
> if (navi10_od_feature_is_supported(od_settings,
> SMU_11_0_ODCAP_GFXCLK_LIMITS)) {
> @@ -1446,7 +1445,7 @@ static int navi10_emit_clk_levels(struct
> smu_context *smu,
> break;
> }
>
> - return (*offset - save_offset);
> + return 0;
> }
>
> static int navi10_print_clk_levels(struct smu_context *smu,
> --
> 2.34.1
