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@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
>>
