[PATCH 2/2] drm/amdgpu/pm: get/set dgpu power cap via hwmon API

[Alex Deucher]

On Mon, Jan 29, 2018 at 5:14 AM, Rex Zhu <Rex.Zhu at amd.com> wrote:
> Adust power limit through power1_cap
> Get min/max power limit through power1_cap_min/power1_cap_max
>
> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
>
> Change-Id: Idca81ae12dc9fa4e4dd6c89fe47e0318df2859d3
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 75 ++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index b0cdb14..db6e2ba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1207,6 +1207,69 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
>         return snprintf(buf, PAGE_SIZE, "%u\n", uw);
>  }
>
> +static ssize_t amdgpu_hwmon_show_power_cap_min(struct device *dev,
> +                                        struct device_attribute *attr,
> +                                        char *buf)
> +{
> +       return sprintf(buf, "%i\n", 0);
> +}
> +
> +static ssize_t amdgpu_hwmon_show_power_cap_max(struct device *dev,
> +                                        struct device_attribute *attr,
> +                                        char *buf)
> +{
> +       struct amdgpu_device *adev = dev_get_drvdata(dev);
> +       uint32_t limit = 0;
> +
> +       if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->get_power_limit) {
> +               adev->powerplay.pp_funcs->get_power_limit(adev->powerplay.pp_handle, &limit, true);
> +               return snprintf(buf, PAGE_SIZE, "%d w\n", limit / 256);

Isn't the power limit only multiplied by 256 on smu7?  Please
normalize the internal interface.  Also the hwmon API takes the uses
microWatt units, please expose those and convert to normalized
internal units as necessary.
https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface


> +       } else {
> +               return snprintf(buf, PAGE_SIZE, "\n");
> +       }
> +}
> +
> +static ssize_t amdgpu_hwmon_show_power_cap(struct device *dev,
> +                                        struct device_attribute *attr,
> +                                        char *buf)
> +{
> +       struct amdgpu_device *adev = dev_get_drvdata(dev);
> +       uint32_t limit = 0;
> +
> +       if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->get_power_limit) {
> +               adev->powerplay.pp_funcs->get_power_limit(adev->powerplay.pp_handle, &limit, false);
> +               return snprintf(buf, PAGE_SIZE, "%d w\n", limit / 256);
> +       } else {
> +               return snprintf(buf, PAGE_SIZE, "\n");
> +       }
> +}
> +
> +
> +static ssize_t amdgpu_hwmon_set_power_cap(struct device *dev,
> +               struct device_attribute *attr,
> +               const char *buf,
> +               size_t count)
> +{
> +       struct amdgpu_device *adev = dev_get_drvdata(dev);
> +       int err;
> +       u32 value;
> +
> +       err = kstrtou32(buf, 10, &value);
> +       if (err)
> +               return err;
> +
> +       value = value * 256;

Same comment here.

Alex

> +       if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->set_power_limit) {
> +               err = adev->powerplay.pp_funcs->set_power_limit(adev->powerplay.pp_handle, value);
> +               if (err)
> +                       return err;
> +       } else {
> +               return -EINVAL;
> +       }
> +
> +       return count;
> +}
> +
>  static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, amdgpu_hwmon_show_temp, NULL, 0);
>  static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, amdgpu_hwmon_show_temp_thresh, NULL, 0);
>  static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IRUGO, amdgpu_hwmon_show_temp_thresh, NULL, 1);
> @@ -1220,6 +1283,9 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
>  static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, amdgpu_hwmon_show_vddnb, NULL, 0);
>  static SENSOR_DEVICE_ATTR(in1_label, S_IRUGO, amdgpu_hwmon_show_vddnb_label, NULL, 0);
>  static SENSOR_DEVICE_ATTR(power1_average, S_IRUGO, amdgpu_hwmon_show_power_avg, NULL, 0);
> +static SENSOR_DEVICE_ATTR(power1_cap_max, S_IRUGO, amdgpu_hwmon_show_power_cap_max, NULL, 0);
> +static SENSOR_DEVICE_ATTR(power1_cap_min, S_IRUGO, amdgpu_hwmon_show_power_cap_min, NULL, 0);
> +static SENSOR_DEVICE_ATTR(power1_cap, S_IRUGO | S_IWUSR, amdgpu_hwmon_show_power_cap, amdgpu_hwmon_set_power_cap, 0);
>
>  static struct attribute *hwmon_attributes[] = {
>         &sensor_dev_attr_temp1_input.dev_attr.attr,
> @@ -1235,6 +1301,9 @@ static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
>         &sensor_dev_attr_in1_input.dev_attr.attr,
>         &sensor_dev_attr_in1_label.dev_attr.attr,
>         &sensor_dev_attr_power1_average.dev_attr.attr,
> +       &sensor_dev_attr_power1_cap_max.dev_attr.attr,
> +       &sensor_dev_attr_power1_cap_min.dev_attr.attr,
> +       &sensor_dev_attr_power1_cap.dev_attr.attr,
>         NULL
>  };
>
> @@ -1282,6 +1351,12 @@ static umode_t hwmon_attributes_visible(struct kobject *kobj,
>              attr == &sensor_dev_attr_pwm1_enable.dev_attr.attr)) /* can't manage state */
>                 effective_mode &= ~S_IWUSR;
>
> +       if ((adev->flags & AMD_IS_APU) &&
> +           (attr == &sensor_dev_attr_power1_cap_max.dev_attr.attr ||
> +            attr == &sensor_dev_attr_power1_cap_min.dev_attr.attr||
> +            attr == &sensor_dev_attr_power1_cap.dev_attr.attr))
> +               return 0;
> +
>         /* hide max/min values if we can't both query and manage the fan */
>         if ((!adev->powerplay.pp_funcs->set_fan_speed_percent &&
>              !adev->powerplay.pp_funcs->get_fan_speed_percent) &&
> --
> 1.9.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx