RE: [PATCH] drm/amdgpu: expose sclk and mclk via hwmon

2019-01-08 Thread Quan, Evan
Reviewed-by: Evan Quan 

> -Original Message-
> From: amd-gfx  On Behalf Of Alex
> Deucher
> Sent: 2018年12月11日 5:17
> To: amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander 
> Subject: [PATCH] drm/amdgpu: expose sclk and mclk via hwmon
> 
> Expose sclk (gfx clock) and mclk (memory clock) via
> hwmon compatible interface.  hwmon does not actually
> formally specify a frequency type attribute, but these
> are compatible with the format of the other attributes
> exposed via hwmon.  Units are hertz.
> 
> freq1_input - GPU gfx/compute clock in hertz
> freq2_input - GPU memory clock in hertz (dGPU only)
> 
> Signed-off-by: Alex Deucher 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 93
> ++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 1f61ed95727c..6d52428fc45b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1516,6 +1516,75 @@ static ssize_t
> amdgpu_hwmon_set_power_cap(struct device *dev,
>   return count;
>  }
> 
> +static ssize_t amdgpu_hwmon_show_sclk(struct device *dev,
> +   struct device_attribute *attr,
> +   char *buf)
> +{
> + struct amdgpu_device *adev = dev_get_drvdata(dev);
> + struct drm_device *ddev = adev->ddev;
> + uint32_t sclk;
> + int r, size = sizeof(sclk);
> +
> + /* Can't get voltage when the card is off */
> + if  ((adev->flags & AMD_IS_PX) &&
> +  (ddev->switch_power_state != DRM_SWITCH_POWER_ON))
> + return -EINVAL;
> +
> + /* sanity check PP is enabled */
> + if (!(adev->powerplay.pp_funcs &&
> +   adev->powerplay.pp_funcs->read_sensor))
> +   return -EINVAL;
> +
> + /* get the sclk */
> + r = amdgpu_dpm_read_sensor(adev,
> AMDGPU_PP_SENSOR_GFX_SCLK,
> +(void *), );
> + if (r)
> + return r;
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", sclk * 10 * 1000);
> +}
> +
> +static ssize_t amdgpu_hwmon_show_sclk_label(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "sclk\n");
> +}
> +
> +static ssize_t amdgpu_hwmon_show_mclk(struct device *dev,
> +   struct device_attribute *attr,
> +   char *buf)
> +{
> + struct amdgpu_device *adev = dev_get_drvdata(dev);
> + struct drm_device *ddev = adev->ddev;
> + uint32_t mclk;
> + int r, size = sizeof(mclk);
> +
> + /* Can't get voltage when the card is off */
> + if  ((adev->flags & AMD_IS_PX) &&
> +  (ddev->switch_power_state != DRM_SWITCH_POWER_ON))
> + return -EINVAL;
> +
> + /* sanity check PP is enabled */
> + if (!(adev->powerplay.pp_funcs &&
> +   adev->powerplay.pp_funcs->read_sensor))
> +   return -EINVAL;
> +
> + /* get the sclk */
> + r = amdgpu_dpm_read_sensor(adev,
> AMDGPU_PP_SENSOR_GFX_MCLK,
> +(void *), );
> + if (r)
> + return r;
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", mclk * 10 * 1000);
> +}
> +
> +static ssize_t amdgpu_hwmon_show_mclk_label(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "mclk\n");
> +}
> 
>  /**
>   * DOC: hwmon
> @@ -1532,6 +1601,10 @@ static ssize_t
> amdgpu_hwmon_set_power_cap(struct device *dev,
>   *
>   * - GPU fan
>   *
> + * - GPU gfx/compute engine clock
> + *
> + * - GPU memory clock (dGPU only)
> + *
>   * hwmon interfaces for GPU temperature:
>   *
>   * - temp1_input: the on die GPU temperature in millidegrees Celsius
> @@ -1576,6 +1649,12 @@ static ssize_t
> amdgpu_hwmon_set_power_cap(struct device *dev,
>   *
>   * - fan[1-*]_enable: Enable or disable the sensors.1: Enable 0: Disable
>   *
> + * hwmon interfaces for GPU clocks:
> + *
> + * - freq1_input: the gfx/compute clock in hertz
> + *
> + * - freq2_input: the memory clock in hertz
> + *
>   * You can use hwmon tools like sensors to view this information on your
> system.
>   *
>   */
> @@ -1600,6 +1679,10 @@ static SENSOR_DEVICE_ATTR(power1_average,
> S_IRUGO, amdgpu_hwmon_show_power_avg,
>  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 SENSOR_DEVICE_ATTR(freq1_input, S_IRUGO,
> amdgpu_hwmon_show_sclk, NULL, 0);
> +static SENSOR_DEVICE_ATTR(freq1_label, S_IRUGO,
> 

RE: [PATCH] drm/amdgpu: expose sclk and mclk via hwmon

2019-01-08 Thread Freehill, Chris
Ditto...except if those files Felix mentioned were to go away as 
pp_od_clk_voltage more widely used, then we would need this.

I think Alex had mentioned there was some redundancy between those, but 
pp_od_clk_voltage didn't indicate the current frequencies.

Chris



-Original Message-
From: Kuehling, Felix  
Sent: Tuesday, January 8, 2019 3:44 PM
To: Deucher, Alexander ; Alex Deucher 
; amd-gfx@lists.freedesktop.org; Russell, Kent 
; Freehill, Chris 
Subject: Re: [PATCH] drm/amdgpu: expose sclk and mclk via hwmon

I'm not saying it's useless. But rocm-smi can already get the clocks from sysfs 
(pp_dpm_sclk and pp_dpm_mclk). Are these clocks any different?


Regards,
  Felix


On 2019-01-08 4:32 p.m., Deucher, Alexander wrote:
>
> Ping?  Useful for rocm-smi?
>
> --
> --
> *From:* Alex Deucher 
> *Sent:* Monday, December 10, 2018 4:17:27 PM
> *To:* amd-gfx@lists.freedesktop.org
> *Cc:* Deucher, Alexander
> *Subject:* [PATCH] drm/amdgpu: expose sclk and mclk via hwmon
>  
> Expose sclk (gfx clock) and mclk (memory clock) via hwmon compatible 
> interface.  hwmon does not actually formally specify a frequency type 
> attribute, but these are compatible with the format of the other 
> attributes exposed via hwmon.  Units are hertz.
>
> freq1_input - GPU gfx/compute clock in hertz freq2_input - GPU memory 
> clock in hertz (dGPU only)
>
> Signed-off-by: Alex Deucher 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 93
> ++
>  1 file changed, 93 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 1f61ed95727c..6d52428fc45b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1516,6 +1516,75 @@ static ssize_t
> amdgpu_hwmon_set_power_cap(struct device *dev,
>  return count;
>  }
>  
> +static ssize_t amdgpu_hwmon_show_sclk(struct device *dev,
> + struct device_attribute *attr,
> + char *buf) {
> +   struct amdgpu_device *adev = dev_get_drvdata(dev);
> +   struct drm_device *ddev = adev->ddev;
> +   uint32_t sclk;
> +   int r, size = sizeof(sclk);
> +
> +   /* Can't get voltage when the card is off */
> +   if  ((adev->flags & AMD_IS_PX) &&
> +    (ddev->switch_power_state != DRM_SWITCH_POWER_ON))
> +   return -EINVAL;
> +
> +   /* sanity check PP is enabled */
> +   if (!(adev->powerplay.pp_funcs &&
> + adev->powerplay.pp_funcs->read_sensor))
> + return -EINVAL;
> +
> +   /* get the sclk */
> +   r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GFX_SCLK,
> +  (void *), );
> +   if (r)
> +   return r;
> +
> +   return snprintf(buf, PAGE_SIZE, "%d\n", sclk * 10 * 1000); }
> +
> +static ssize_t amdgpu_hwmon_show_sclk_label(struct device *dev,
> +   struct device_attribute 
> +*attr,
> +   char *buf) {
> +   return snprintf(buf, PAGE_SIZE, "sclk\n"); }
> +
> +static ssize_t amdgpu_hwmon_show_mclk(struct device *dev,
> + struct device_attribute *attr,
> + char *buf) {
> +   struct amdgpu_device *adev = dev_get_drvdata(dev);
> +   struct drm_device *ddev = adev->ddev;
> +   uint32_t mclk;
> +   int r, size = sizeof(mclk);
> +
> +   /* Can't get voltage when the card is off */
> +   if  ((adev->flags & AMD_IS_PX) &&
> +    (ddev->switch_power_state != DRM_SWITCH_POWER_ON))
> +   return -EINVAL;
> +
> +   /* sanity check PP is enabled */
> +   if (!(adev->powerplay.pp_funcs &&
> + adev->powerplay.pp_funcs->read_sensor))
> + return -EINVAL;
> +
> +   /* get the sclk */
> +   r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GFX_MCLK,
> +  (void *), );
> +   if (r)
> +   return r;
> +
> +   return snprintf(buf, PAGE_SIZE, "%d\n", mclk * 10 * 1000); }
> +
> +static ssize_t amdgpu_hwmon_show_mclk_label(struct device *dev,
> +   struct device_attribute 
> +*attr,
> +   char *buf) {
> +   return snprintf(buf, PAGE_SIZE, "mclk\n"); }
>  
>  /**
>   * DOC: hwmon
> @@ -1532,6 +1601,10 @@ static

Re: [PATCH] drm/amdgpu: expose sclk and mclk via hwmon

2019-01-08 Thread Kuehling, Felix
I'm not saying it's useless. But rocm-smi can already get the clocks
from sysfs (pp_dpm_sclk and pp_dpm_mclk). Are these clocks any different?


Regards,
  Felix


On 2019-01-08 4:32 p.m., Deucher, Alexander wrote:
>
> Ping?  Useful for rocm-smi?
>
> 
> *From:* Alex Deucher 
> *Sent:* Monday, December 10, 2018 4:17:27 PM
> *To:* amd-gfx@lists.freedesktop.org
> *Cc:* Deucher, Alexander
> *Subject:* [PATCH] drm/amdgpu: expose sclk and mclk via hwmon
>  
> Expose sclk (gfx clock) and mclk (memory clock) via
> hwmon compatible interface.  hwmon does not actually
> formally specify a frequency type attribute, but these
> are compatible with the format of the other attributes
> exposed via hwmon.  Units are hertz.
>
> freq1_input - GPU gfx/compute clock in hertz
> freq2_input - GPU memory clock in hertz (dGPU only)
>
> Signed-off-by: Alex Deucher 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 93
> ++
>  1 file changed, 93 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 1f61ed95727c..6d52428fc45b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -1516,6 +1516,75 @@ static ssize_t
> amdgpu_hwmon_set_power_cap(struct device *dev,
>  return count;
>  }
>  
> +static ssize_t amdgpu_hwmon_show_sclk(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> +   struct amdgpu_device *adev = dev_get_drvdata(dev);
> +   struct drm_device *ddev = adev->ddev;
> +   uint32_t sclk;
> +   int r, size = sizeof(sclk);
> +
> +   /* Can't get voltage when the card is off */
> +   if  ((adev->flags & AMD_IS_PX) &&
> +    (ddev->switch_power_state != DRM_SWITCH_POWER_ON))
> +   return -EINVAL;
> +
> +   /* sanity check PP is enabled */
> +   if (!(adev->powerplay.pp_funcs &&
> + adev->powerplay.pp_funcs->read_sensor))
> + return -EINVAL;
> +
> +   /* get the sclk */
> +   r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GFX_SCLK,
> +  (void *), );
> +   if (r)
> +   return r;
> +
> +   return snprintf(buf, PAGE_SIZE, "%d\n", sclk * 10 * 1000);
> +}
> +
> +static ssize_t amdgpu_hwmon_show_sclk_label(struct device *dev,
> +   struct device_attribute *attr,
> +   char *buf)
> +{
> +   return snprintf(buf, PAGE_SIZE, "sclk\n");
> +}
> +
> +static ssize_t amdgpu_hwmon_show_mclk(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> +   struct amdgpu_device *adev = dev_get_drvdata(dev);
> +   struct drm_device *ddev = adev->ddev;
> +   uint32_t mclk;
> +   int r, size = sizeof(mclk);
> +
> +   /* Can't get voltage when the card is off */
> +   if  ((adev->flags & AMD_IS_PX) &&
> +    (ddev->switch_power_state != DRM_SWITCH_POWER_ON))
> +   return -EINVAL;
> +
> +   /* sanity check PP is enabled */
> +   if (!(adev->powerplay.pp_funcs &&
> + adev->powerplay.pp_funcs->read_sensor))
> + return -EINVAL;
> +
> +   /* get the sclk */
> +   r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GFX_MCLK,
> +  (void *), );
> +   if (r)
> +   return r;
> +
> +   return snprintf(buf, PAGE_SIZE, "%d\n", mclk * 10 * 1000);
> +}
> +
> +static ssize_t amdgpu_hwmon_show_mclk_label(struct device *dev,
> +   struct device_attribute *attr,
> +   char *buf)
> +{
> +   return snprintf(buf, PAGE_SIZE, "mclk\n");
> +}
>  
>  /**
>   * DOC: hwmon
> @@ -1532,6 +1601,10 @@ static ssize_t
> amdgpu_hwmon_set_power_cap(struct device *dev,
>   *
>   * - GPU fan
>   *
> + * - GPU gfx/compute engine clock
> + *
> + * - GPU memory clock (dGPU only)
> + *
>   * hwmon interfaces for GPU temperature:
>   *
>   * - temp1_input: the on die GPU temperature in millidegrees Celsius
> @@ -1576,6 +1649,12 @@ static ssize_t
> amdgpu_hwmon_set_power_cap(struct device *dev,
>   *
>   * - fan[1-*]_enable: Enable or disable the sensors.1: Enable 0: Disable
>   *
> + * hwmon interfaces for GPU clocks:
> + *
> + * - freq1_input: the gfx/compute clock in hertz
> + *
> + * - freq2_input: the memory clock in hertz
> + *
>   * You can use hwmon tools like sensors to view this information on
> your system.
>   *
>   */
> @@ -1600,6 +1679,10 @@ static SENSOR_DEVICE_ATTR(power1_average,
> S_IRUGO, amdgpu_hwmon_show_power_avg,
>  static SENSOR_DEVICE_ATTR(power1_cap_max, S_IRUGO,
> amdgpu_hwmon_show_power_cap_max, NULL, 0);
>  static 

Re: [PATCH] drm/amdgpu: expose sclk and mclk via hwmon

2019-01-08 Thread Deucher, Alexander
Ping?  Useful for rocm-smi?


From: Alex Deucher 
Sent: Monday, December 10, 2018 4:17:27 PM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander
Subject: [PATCH] drm/amdgpu: expose sclk and mclk via hwmon

Expose sclk (gfx clock) and mclk (memory clock) via
hwmon compatible interface.  hwmon does not actually
formally specify a frequency type attribute, but these
are compatible with the format of the other attributes
exposed via hwmon.  Units are hertz.

freq1_input - GPU gfx/compute clock in hertz
freq2_input - GPU memory clock in hertz (dGPU only)

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 93 ++
 1 file changed, 93 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 1f61ed95727c..6d52428fc45b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -1516,6 +1516,75 @@ static ssize_t amdgpu_hwmon_set_power_cap(struct device 
*dev,
 return count;
 }

+static ssize_t amdgpu_hwmon_show_sclk(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+   struct amdgpu_device *adev = dev_get_drvdata(dev);
+   struct drm_device *ddev = adev->ddev;
+   uint32_t sclk;
+   int r, size = sizeof(sclk);
+
+   /* Can't get voltage when the card is off */
+   if  ((adev->flags & AMD_IS_PX) &&
+(ddev->switch_power_state != DRM_SWITCH_POWER_ON))
+   return -EINVAL;
+
+   /* sanity check PP is enabled */
+   if (!(adev->powerplay.pp_funcs &&
+ adev->powerplay.pp_funcs->read_sensor))
+ return -EINVAL;
+
+   /* get the sclk */
+   r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GFX_SCLK,
+  (void *), );
+   if (r)
+   return r;
+
+   return snprintf(buf, PAGE_SIZE, "%d\n", sclk * 10 * 1000);
+}
+
+static ssize_t amdgpu_hwmon_show_sclk_label(struct device *dev,
+   struct device_attribute *attr,
+   char *buf)
+{
+   return snprintf(buf, PAGE_SIZE, "sclk\n");
+}
+
+static ssize_t amdgpu_hwmon_show_mclk(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+   struct amdgpu_device *adev = dev_get_drvdata(dev);
+   struct drm_device *ddev = adev->ddev;
+   uint32_t mclk;
+   int r, size = sizeof(mclk);
+
+   /* Can't get voltage when the card is off */
+   if  ((adev->flags & AMD_IS_PX) &&
+(ddev->switch_power_state != DRM_SWITCH_POWER_ON))
+   return -EINVAL;
+
+   /* sanity check PP is enabled */
+   if (!(adev->powerplay.pp_funcs &&
+ adev->powerplay.pp_funcs->read_sensor))
+ return -EINVAL;
+
+   /* get the sclk */
+   r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GFX_MCLK,
+  (void *), );
+   if (r)
+   return r;
+
+   return snprintf(buf, PAGE_SIZE, "%d\n", mclk * 10 * 1000);
+}
+
+static ssize_t amdgpu_hwmon_show_mclk_label(struct device *dev,
+   struct device_attribute *attr,
+   char *buf)
+{
+   return snprintf(buf, PAGE_SIZE, "mclk\n");
+}

 /**
  * DOC: hwmon
@@ -1532,6 +1601,10 @@ static ssize_t amdgpu_hwmon_set_power_cap(struct device 
*dev,
  *
  * - GPU fan
  *
+ * - GPU gfx/compute engine clock
+ *
+ * - GPU memory clock (dGPU only)
+ *
  * hwmon interfaces for GPU temperature:
  *
  * - temp1_input: the on die GPU temperature in millidegrees Celsius
@@ -1576,6 +1649,12 @@ static ssize_t amdgpu_hwmon_set_power_cap(struct device 
*dev,
  *
  * - fan[1-*]_enable: Enable or disable the sensors.1: Enable 0: Disable
  *
+ * hwmon interfaces for GPU clocks:
+ *
+ * - freq1_input: the gfx/compute clock in hertz
+ *
+ * - freq2_input: the memory clock in hertz
+ *
  * You can use hwmon tools like sensors to view this information on your 
system.
  *
  */
@@ -1600,6 +1679,10 @@ static SENSOR_DEVICE_ATTR(power1_average, S_IRUGO, 
amdgpu_hwmon_show_power_avg,
 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 SENSOR_DEVICE_ATTR(freq1_input, S_IRUGO, amdgpu_hwmon_show_sclk, NULL, 
0);
+static SENSOR_DEVICE_ATTR(freq1_label, S_IRUGO, amdgpu_hwmon_show_sclk_label, 
NULL, 0);
+static SENSOR_DEVICE_ATTR(freq2_input, S_IRUGO, amdgpu_hwmon_show_mclk, NULL, 
0);
+static SENSOR_DEVICE_ATTR(freq2_label, S_IRUGO, amdgpu_hwmon_show_mclk_label,