[PATCH] drm/amd/pm: Fix get_if_active usage
Lazar, Lijo
lijo.lazar at amd.com
Mon Feb 3 16:44:26 UTC 2025
On 2/3/2025 9:36 PM, Alex Deucher wrote:
> On Fri, Jan 31, 2025 at 11:53 PM Lijo Lazar <lijo.lazar at amd.com> 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.
>From what I see, this looks to be the generic way to use this API
regardless of CONFIG_PM. When CONFIG_PM is not set, the stub API gives
the error code which is inline with the case when runtime pm is disabled
for the device.
More future proof to move all of these
> duplicated checks into a common helper?
Yes, this makes sense.
Also is it possible we might
> miss errors with this change in the runtime pm enabled case? E.g., if
> a previous resume failed.
Probably, this needs to be addressed differently -
if (adev->in_suspend && !adev->in_runpm)
return -EPERM;
We have this check already; we may not need the second in_runpm anymore
as now the behavior is to allow operation on the device only if it's active.
Thanks,
Lijo
>
> Alex
>
>>
>> Signed-off-by: Lijo Lazar <lijo.lazar at amd.com>
>>
>> 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
>>
More information about the amd-gfx
mailing list