RE: [PATCH 1/3] amdgpu/pm: Implement new API function "emit" that accepts buffer base and write offset
[AMD Official Use Only]
> -Original Message-
> From: Powell, Darren
> Sent: Saturday, January 22, 2022 11:47 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Powell, Darren
> Subject: [PATCH 1/3] amdgpu/pm: Implement new API function "emit" that
> accepts buffer base and write offset
>
>(v1)
> - new power management function emit_clk_levels implemented by
> navi10_emit_clk_levels()
>This function currently duplicates the functionality of
> navi10_print_clk_levels, where
>snprintf is used write to the sysfs buffer. The first implementation
> to use
> sysfs_emit
>was unsuccessful due to requirement that buf must be aligned to a
> specific memory
>boundary. This solution passes the buffer base and write offset down
> the
> stack.
> - new power management function emit_clock_levels implemented by
> smu_emit_ppclk_levels()
>This function combines implementation of smu_print_ppclk_levels and
>smu_print_smuclk_levels and calls emit_clk_levels
> - new helper function smu_convert_to_smuclk called by
> smu_print_ppclk_levels and
>smu_emit_ppclk_levels
> - emit_clock_levels currently returns the length of the string written to the
> buffer,
>maintaining the behaviour of print_clock_levels, but will be upgraded
> to
> return
>error values in a subsequent patch
> - failure of the emit_clock_levels at the top level
> (amdgpu_get_pp_dpm_clock) triggers a
>fallback to the print_clock_levels, which can be removed after
> emit_clock_levels is
>implemented across all platforms.
>
>(v2)
> Update to apply on commit 801771de03
> adjust printing of empty carriage return only if size == 0
>
> == 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
> ---
> .../gpu/drm/amd/include/kgd_pp_interface.h| 1 +
> drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 21 ++
> drivers/gpu/drm/amd/pm/amdgpu_pm.c| 11 +-
> drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 4 +
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 46 -
> drivers/gpu/drm/amd/pm/swsmu/inc/amdgpu_smu.h | 10 +
> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 189
> ++
> 7 files changed, 273 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 a8eec91c0995..8a8eb9411cdf 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -321,6 +321,7 @@ 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 (*emit_clock_levels)(void *handle, enum pp_clock_type type,
> char *buf, int *offset);
> 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_dpm.c
> b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> index 728b6e10f302..e45578124928 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> @@ -906,6 +906,27 @@ int amdgpu_dpm_print_clock_levels(struct
> amdgpu_device *adev,
> return ret;
> }
>
> +int amdgpu_dpm_emit_clock_levels(struct amdgpu_device *adev,
> + enum pp_clock_type type,
> + char *buf,
> + int *offset)
> +{
> + const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
> + int ret = 0;
> +
> + if (!pp_funcs->emit_clock_levels)
> + return 0;
> +
> + mutex_lock(>pm.mutex);
> + ret = pp_funcs->emit_clock_levels(adev->powerplay.pp_handle,
> +type,
> +buf,
> +offset);
> + mutex_unlock(>pm.mutex);
> +
> + return ret;
> +}
> +
> int amdgpu_dpm_set_ppfeature_status(struct amdgpu_device *adev,
> uint64_t ppfeature_masks)
> {
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index d2823aaeca09..ec2729b3930e 100644
> ---
RE: [PATCH 1/3] amdgpu/pm: Implement new API function "emit" that accepts buffer base and write offset
[AMD Official Use Only]
> -Original Message-
> From: amd-gfx On Behalf Of
> Darren Powell
> Sent: Wednesday, December 8, 2021 2:36 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Powell, Darren
> Subject: [PATCH 1/3] amdgpu/pm: Implement new API function "emit" that
> accepts buffer base and write offset
>
> - new power management function emit_clk_levels implemented by
> navi10_emit_clk_levels()
>This function currently duplicates the functionality of
> navi10_print_clk_levels, where
>snprintf is used write to the sysfs buffer. The first implementation
> to use
> sysfs_emit
>was unsuccessful due to requirement that buf must be aligned to a
> specific memory
>boundary. This solution passes the buffer base and write offset down
> the
> stack.
> - new power management function emit_clock_levels implemented by
> smu_emit_ppclk_levels()
>This function combines implementation of smu_print_ppclk_levels and
>smu_print_smuclk_levels and calls emit_clk_levels
> - new helper function smu_convert_to_smuclk called by
> smu_print_ppclk_levels and
>smu_emit_ppclk_levels
> - emit_clock_levels currently returns the length of the string written to the
> buffer,
>maintaining the behaviour of print_clock_levels, but will be upgraded
> to
> return
>error values in a subsequent patch
> - failure of the emit_clock_levels at the top level
> (amdgpu_get_pp_dpm_clock) triggers a
>fallback to the print_clock_levels, which can be removed after
> emit_clock_levels is
>implemented across all platforms.
>
> == 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
> ---
> .../gpu/drm/amd/include/kgd_pp_interface.h| 1 +
> drivers/gpu/drm/amd/pm/amdgpu_pm.c| 20 +-
> drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 3 +
> drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 10 +
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 50 -
> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 189
> ++
> 6 files changed, 262 insertions(+), 11 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..43f43a4f321b 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -310,6 +310,7 @@ 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 (*emit_clock_levels)(void *handle, enum pp_clock_type type,
> char *buf, int *offset);
> 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 49df4c20f09e..a1169a2397ca 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -1056,8 +1056,8 @@ static ssize_t amdgpu_get_pp_dpm_clock(struct
> device *dev,
> {
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> - ssize_t size;
> - int ret;
> + int size = 0;
> + int ret = 0;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> @@ -1070,10 +1070,18 @@ 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");
> + ret = -EOPNOTSUPP;
> + if (adev->powerplay.pp_funcs->emit_clock_levels) {
> + ret = amdgpu_dpm_emit_clock_levels(adev, type, buf,
> );
> + }
The whole idea seems fine to me. However, we are trying to do some cleanups to
avoid spiking into power internals(as above via
adev->powerplay.pp_funcs->emit_clock_levels).
Check the patch series below:
https://lists.freedesktop.org/archives/amd-gfx/2021-December/072096.html
So, it would be better if you can rebase the patches here based on that. Or you
can wait a while to let me land them first.
BR
Evan
> +
> + if (ret < 0) {
> + if
Re: [PATCH 1/3] amdgpu/pm: Implement new API function "emit" that accepts buffer base and write offset
[AMD Official Use Only]
> The whole idea seems fine to me. However, we are trying to do some cleanups
> to avoid spiking into power internals(as above via
> adev->powerplay.pp_funcs->emit_clock_levels).
> Check the patch series below:
> https://lists.freedesktop.org/archives/amd-gfx/2021-December/072096.html
> So, it would be better if you can rebase the patches here based on that. Or
> you can wait a while to let me land them first.
> BR
> Evan
I can wait for it to land and rebase then, will send v2 of series once this
happens
Darren
From: Quan, Evan
Sent: Wednesday, December 8, 2021 8:48 PM
To: Powell, Darren ; amd-gfx@lists.freedesktop.org
Cc: Powell, Darren
Subject: RE: [PATCH 1/3] amdgpu/pm: Implement new API function "emit" that
accepts buffer base and write offset
[AMD Official Use Only]
> -Original Message-
> From: amd-gfx On Behalf Of
> Darren Powell
> Sent: Wednesday, December 8, 2021 2:36 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Powell, Darren
> Subject: [PATCH 1/3] amdgpu/pm: Implement new API function "emit" that
> accepts buffer base and write offset
>
> - new power management function emit_clk_levels implemented by
> navi10_emit_clk_levels()
>This function currently duplicates the functionality of
> navi10_print_clk_levels, where
>snprintf is used write to the sysfs buffer. The first implementation
> to use
> sysfs_emit
>was unsuccessful due to requirement that buf must be aligned to a
> specific memory
>boundary. This solution passes the buffer base and write offset down
> the
> stack.
> - new power management function emit_clock_levels implemented by
> smu_emit_ppclk_levels()
>This function combines implementation of smu_print_ppclk_levels and
>smu_print_smuclk_levels and calls emit_clk_levels
> - new helper function smu_convert_to_smuclk called by
> smu_print_ppclk_levels and
>smu_emit_ppclk_levels
> - emit_clock_levels currently returns the length of the string written to the
> buffer,
>maintaining the behaviour of print_clock_levels, but will be upgraded
> to
> return
>error values in a subsequent patch
> - failure of the emit_clock_levels at the top level
> (amdgpu_get_pp_dpm_clock) triggers a
>fallback to the print_clock_levels, which can be removed after
> emit_clock_levels is
>implemented across all platforms.
>
> == 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
> ---
> .../gpu/drm/amd/include/kgd_pp_interface.h| 1 +
> drivers/gpu/drm/amd/pm/amdgpu_pm.c| 20 +-
> drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h | 3 +
> drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 10 +
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 50 -
> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 189
> ++
> 6 files changed, 262 insertions(+), 11 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..43f43a4f321b 100644
> --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> @@ -310,6 +310,7 @@ 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 (*emit_clock_levels)(void *handle, enum pp_clock_type type,
> char *buf, int *offset);
>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 49df4c20f09e..a1169a2397ca 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -1056,8 +1056,8 @@ static ssize_t amdgpu_get_pp_dpm_cloc
