On Fri, Jan 31, 2025 at 11:53 PM Lijo Lazar <lijo.lazar@xxxxxxx> wrote:
>
> If a device supports runtime pm, then pm_runtime_get_if_active returns 0
> if a device is not active and 1 if already active. However, if a device
> doesn't support runtime pm, the API returns -EINVAL. A device not
> supporting runtime pm implies it's not affected by runtime pm and it's
> active. Hence no need to get() to increment usage count. Remove < 0
> return value check.
Might be worth mentioning that this happens when CONFIG_PM is not set
assuming that is the case. More future proof to move all of these
duplicated checks into a common helper? Also is it possible we might
miss errors with this change in the runtime pm enabled case? E.g., if
a previous resume failed.
Alex
>
> Signed-off-by: Lijo Lazar <lijo.lazar@xxxxxxx>
>
> Fixes: 6e796cb4a972b ("drm/amd/pm: use pm_runtime_get_if_active for debugfs getters")
> ---
> drivers/gpu/drm/amd/pm/amdgpu_pm.c | 129 +++++++++++------------------
> 1 file changed, 48 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index 0aca0803514e..a8db2d3c9154 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -138,16 +138,14 @@ static ssize_t amdgpu_get_power_dpm_state(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> enum amd_pm_state_type pm;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> amdgpu_dpm_get_current_power_state(adev, &pm);
>
> @@ -261,16 +259,14 @@ static ssize_t amdgpu_get_power_dpm_force_performance_level(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> enum amd_dpm_forced_level level = 0xff;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> level = amdgpu_dpm_get_performance_level(adev);
>
> @@ -357,16 +353,15 @@ static ssize_t amdgpu_get_pp_num_states(struct device *dev,
> struct amdgpu_device *adev = drm_to_adev(ddev);
> struct pp_states_info data;
> uint32_t i;
> - int buf_len, ret;
> + int buf_len;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> if (amdgpu_dpm_get_pp_num_states(adev, &data))
> memset(&data, 0, sizeof(data));
> @@ -399,9 +394,8 @@ static ssize_t amdgpu_get_pp_cur_state(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> amdgpu_dpm_get_current_power_state(adev, &pm);
>
> @@ -519,16 +513,15 @@ static ssize_t amdgpu_get_pp_table(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> char *table = NULL;
> - int size, ret;
> + int size;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> size = amdgpu_dpm_get_pp_table(adev, &table);
>
> @@ -840,9 +833,8 @@ static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> for (clk_index = 0 ; clk_index < 6 ; clk_index++) {
> ret = amdgpu_dpm_emit_clock_levels(adev, od_clocks[clk_index], buf, &size);
> @@ -923,16 +915,14 @@ static ssize_t amdgpu_get_pp_features(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> ssize_t size;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> size = amdgpu_dpm_get_ppfeature_status(adev, buf);
> if (size <= 0)
> @@ -996,9 +986,8 @@ static ssize_t amdgpu_get_pp_dpm_clock(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> ret = amdgpu_dpm_emit_clock_levels(adev, type, buf, &size);
> if (ret == -ENOENT)
> @@ -1238,16 +1227,14 @@ static ssize_t amdgpu_get_pp_sclk_od(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> uint32_t value = 0;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> value = amdgpu_dpm_get_sclk_od(adev);
>
> @@ -1295,16 +1282,14 @@ static ssize_t amdgpu_get_pp_mclk_od(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> uint32_t value = 0;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> value = amdgpu_dpm_get_mclk_od(adev);
>
> @@ -1376,16 +1361,14 @@ static ssize_t amdgpu_get_pp_power_profile_mode(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> ssize_t size;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> size = amdgpu_dpm_get_power_profile_mode(adev, buf);
> if (size <= 0)
> @@ -1471,9 +1454,8 @@ static int amdgpu_hwmon_get_sensor_generic(struct amdgpu_device *adev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - r = pm_runtime_get_if_active(adev->dev);
> - if (r <= 0)
> - return r ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> /* get the sensor value */
> r = amdgpu_dpm_read_sensor(adev, sensor, query, &size);
> @@ -1574,7 +1556,6 @@ static ssize_t amdgpu_get_pcie_bw(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> uint64_t count0 = 0, count1 = 0;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> @@ -1587,9 +1568,8 @@ static ssize_t amdgpu_get_pcie_bw(struct device *dev,
> if (!adev->asic_funcs->get_pcie_usage)
> return -ENODATA;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> amdgpu_asic_get_pcie_usage(adev, &count0, &count1);
>
> @@ -1715,9 +1695,8 @@ static ssize_t amdgpu_get_apu_thermal_cap(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> ret = amdgpu_dpm_get_apu_thermal_limit(adev, &limit);
> if (!ret)
> @@ -1784,16 +1763,14 @@ static ssize_t amdgpu_get_pm_metrics(struct device *dev,
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> ssize_t size = 0;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> size = amdgpu_dpm_get_pm_metrics(adev, buf, PAGE_SIZE);
>
> @@ -1822,16 +1799,14 @@ static ssize_t amdgpu_get_gpu_metrics(struct device *dev,
> struct amdgpu_device *adev = drm_to_adev(ddev);
> void *gpu_metrics;
> ssize_t size = 0;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(ddev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> size = amdgpu_dpm_get_gpu_metrics(adev, &gpu_metrics);
> if (size <= 0)
> @@ -2709,9 +2684,8 @@ static ssize_t amdgpu_hwmon_get_pwm1_enable(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(adev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> ret = amdgpu_dpm_get_fan_control_mode(adev, &pwm_mode);
>
> @@ -2837,9 +2811,8 @@ static ssize_t amdgpu_hwmon_get_pwm1(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - err = pm_runtime_get_if_active(adev->dev);
> - if (err <= 0)
> - return err ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> err = amdgpu_dpm_get_fan_speed_pwm(adev, &speed);
>
> @@ -2864,9 +2837,8 @@ static ssize_t amdgpu_hwmon_get_fan1_input(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - err = pm_runtime_get_if_active(adev->dev);
> - if (err <= 0)
> - return err ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> err = amdgpu_dpm_get_fan_speed_rpm(adev, &speed);
>
> @@ -2925,9 +2897,8 @@ static ssize_t amdgpu_hwmon_get_fan1_target(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - err = pm_runtime_get_if_active(adev->dev);
> - if (err <= 0)
> - return err ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> err = amdgpu_dpm_get_fan_speed_rpm(adev, &rpm);
>
> @@ -2995,9 +2966,8 @@ static ssize_t amdgpu_hwmon_get_fan1_enable(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(adev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> ret = amdgpu_dpm_get_fan_control_mode(adev, &pwm_mode);
>
> @@ -3162,9 +3132,8 @@ static ssize_t amdgpu_hwmon_show_power_cap_generic(struct device *dev,
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - r = pm_runtime_get_if_active(adev->dev);
> - if (r <= 0)
> - return r ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> r = amdgpu_dpm_get_power_limit(adev, &limit,
> pp_limit_level, power_type);
> @@ -3693,16 +3662,14 @@ static int amdgpu_retrieve_od_settings(struct amdgpu_device *adev,
> char *buf)
> {
> int size = 0;
> - int ret;
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> - ret = pm_runtime_get_if_active(adev->dev);
> - if (ret <= 0)
> - return ret ?: -EPERM;
> + if (!pm_runtime_get_if_active(adev->dev))
> + return -EPERM;
>
> size = amdgpu_dpm_print_clock_levels(adev, od_type, buf);
> if (size == 0)
> --
> 2.25.1
>
