* Re: [PATCH] drm/amd/amdgpu: Add a GPU_LOAD entry to sysfs (v2)
[not found] ` <20180620164852.26424-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
@ 2018-06-20 19:01 ` Abramov, Slava
2018-06-20 19:14 ` Alex Deucher
2018-06-21 9:23 ` Christian König
2 siblings, 0 replies; 4+ messages in thread
From: Abramov, Slava @ 2018-06-20 19:01 UTC (permalink / raw)
To: StDenis, Tom,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
[-- Attachment #1.1: Type: text/plain, Size: 4141 bytes --]
Acked-by: Slava Abramov <slava.abramov-5C7GfCeVMHo@public.gmane.org>
________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org>
Sent: Wednesday, June 20, 2018 12:48:52 PM
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Cc: StDenis, Tom
Subject: [PATCH] drm/amd/amdgpu: Add a GPU_LOAD entry to sysfs (v2)
This adds what should be a stable interface to read GPU
load from userspace.
(v2): Fix comments and name of file per recommendations.
Signed-off-by: Tom St Denis <tom.stdenis-5C7GfCeVMHo@public.gmane.org>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 47 ++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 113edffb5960..49138ac2be24 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -918,6 +918,43 @@ static ssize_t amdgpu_set_pp_power_profile_mode(struct device *dev,
return -EINVAL;
}
+/**
+ * DOC: busy_percent
+ *
+ * The firmware computes a percentage of load based on the activity
+ * level in the IP cores.
+ */
+static ssize_t amdgpu_get_busy_percent(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct drm_device *ddev = dev_get_drvdata(dev);
+ struct amdgpu_device *adev = ddev->dev_private;
+ int r, value, size = sizeof(value);
+
+ /* sanity check PP is enabled */
+ if (!(adev->powerplay.pp_funcs &&
+ adev->powerplay.pp_funcs->read_sensor))
+ return -EINVAL;
+
+ /* read the IP busy sensor */
+ r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_LOAD,
+ (void *)&value, &size);
+ if (r)
+ return r;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static ssize_t amdgpu_set_busy_percent(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ return -EINVAL;
+}
+
+
static DEVICE_ATTR(power_dpm_state, S_IRUGO | S_IWUSR, amdgpu_get_dpm_state, amdgpu_set_dpm_state);
static DEVICE_ATTR(power_dpm_force_performance_level, S_IRUGO | S_IWUSR,
amdgpu_get_dpm_forced_performance_level,
@@ -951,6 +988,8 @@ static DEVICE_ATTR(pp_power_profile_mode, S_IRUGO | S_IWUSR,
static DEVICE_ATTR(pp_od_clk_voltage, S_IRUGO | S_IWUSR,
amdgpu_get_pp_od_clk_voltage,
amdgpu_set_pp_od_clk_voltage);
+static DEVICE_ATTR(gpu_busy_percent, S_IRUGO | S_IWUSR,
+ amdgpu_get_busy_percent, amdgpu_set_busy_percent);
static ssize_t amdgpu_hwmon_show_temp(struct device *dev,
struct device_attribute *attr,
@@ -1854,6 +1893,13 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
"pp_od_clk_voltage\n");
return ret;
}
+ ret = device_create_file(adev->dev,
+ &dev_attr_gpu_busy_percent);
+ if (ret) {
+ DRM_ERROR("failed to create device file "
+ "gpu_busy_level\n");
+ return ret;
+ }
ret = amdgpu_debugfs_pm_init(adev);
if (ret) {
DRM_ERROR("Failed to register debugfs file for dpm!\n");
@@ -1889,6 +1935,7 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
&dev_attr_pp_power_profile_mode);
device_remove_file(adev->dev,
&dev_attr_pp_od_clk_voltage);
+ device_remove_file(adev->dev, &dev_attr_gpu_busy_percent);
}
void amdgpu_pm_compute_clocks(struct amdgpu_device *adev)
--
2.14.4
_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[-- Attachment #1.2: Type: text/html, Size: 8672 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/amd/amdgpu: Add a GPU_LOAD entry to sysfs (v2)
[not found] ` <20180620164852.26424-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
2018-06-20 19:01 ` Abramov, Slava
@ 2018-06-20 19:14 ` Alex Deucher
2018-06-21 9:23 ` Christian König
2 siblings, 0 replies; 4+ messages in thread
From: Alex Deucher @ 2018-06-20 19:14 UTC (permalink / raw)
To: Tom St Denis; +Cc: amd-gfx list
On Wed, Jun 20, 2018 at 12:48 PM, Tom St Denis <tom.stdenis@amd.com> wrote:
> This adds what should be a stable interface to read GPU
> load from userspace.
>
> (v2): Fix comments and name of file per recommendations.
>
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 47 ++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 113edffb5960..49138ac2be24 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -918,6 +918,43 @@ static ssize_t amdgpu_set_pp_power_profile_mode(struct device *dev,
> return -EINVAL;
> }
>
> +/**
> + * DOC: busy_percent
> + *
> + * The firmware computes a percentage of load based on the activity
> + * level in the IP cores.
> + */
I would reword this a bit. the tag next to the DOC is only for
reference purposes, it doesn't show up when you reference a DOC
section in the amdgpu.rst file. In order to make it clear what file
you are referring to, you might want to so something like:
/**
* DOC: busy_percent
*
* The amdgpu driver provides a sysfs API for reading how busy the GPU
is as a percentage.
* The file gpu_busy_percent is used for this. The SMU firmware
computes a percentage of
* load based on the aggregate activity level in the IP cores.
*/
Bonus points if you add the reference to this in the "GPU
Power/Thermal Controls and Monitoring" chapter in amdgpu.rst in
Documentation/gpu/. But that can be a separate follow on patch.
With the fix above, patch is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> +static ssize_t amdgpu_get_busy_percent(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> + int r, value, size = sizeof(value);
> +
> + /* sanity check PP is enabled */
> + if (!(adev->powerplay.pp_funcs &&
> + adev->powerplay.pp_funcs->read_sensor))
> + return -EINVAL;
> +
> + /* read the IP busy sensor */
> + r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_LOAD,
> + (void *)&value, &size);
> + if (r)
> + return r;
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static ssize_t amdgpu_set_busy_percent(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + return -EINVAL;
> +}
> +
> +
> static DEVICE_ATTR(power_dpm_state, S_IRUGO | S_IWUSR, amdgpu_get_dpm_state, amdgpu_set_dpm_state);
> static DEVICE_ATTR(power_dpm_force_performance_level, S_IRUGO | S_IWUSR,
> amdgpu_get_dpm_forced_performance_level,
> @@ -951,6 +988,8 @@ static DEVICE_ATTR(pp_power_profile_mode, S_IRUGO | S_IWUSR,
> static DEVICE_ATTR(pp_od_clk_voltage, S_IRUGO | S_IWUSR,
> amdgpu_get_pp_od_clk_voltage,
> amdgpu_set_pp_od_clk_voltage);
> +static DEVICE_ATTR(gpu_busy_percent, S_IRUGO | S_IWUSR,
> + amdgpu_get_busy_percent, amdgpu_set_busy_percent);
>
> static ssize_t amdgpu_hwmon_show_temp(struct device *dev,
> struct device_attribute *attr,
> @@ -1854,6 +1893,13 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> "pp_od_clk_voltage\n");
> return ret;
> }
> + ret = device_create_file(adev->dev,
> + &dev_attr_gpu_busy_percent);
> + if (ret) {
> + DRM_ERROR("failed to create device file "
> + "gpu_busy_level\n");
> + return ret;
> + }
> ret = amdgpu_debugfs_pm_init(adev);
> if (ret) {
> DRM_ERROR("Failed to register debugfs file for dpm!\n");
> @@ -1889,6 +1935,7 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
> &dev_attr_pp_power_profile_mode);
> device_remove_file(adev->dev,
> &dev_attr_pp_od_clk_voltage);
> + device_remove_file(adev->dev, &dev_attr_gpu_busy_percent);
> }
>
> void amdgpu_pm_compute_clocks(struct amdgpu_device *adev)
> --
> 2.14.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/amd/amdgpu: Add a GPU_LOAD entry to sysfs (v2)
[not found] ` <20180620164852.26424-1-tom.stdenis-5C7GfCeVMHo@public.gmane.org>
2018-06-20 19:01 ` Abramov, Slava
2018-06-20 19:14 ` Alex Deucher
@ 2018-06-21 9:23 ` Christian König
2 siblings, 0 replies; 4+ messages in thread
From: Christian König @ 2018-06-21 9:23 UTC (permalink / raw)
To: Tom St Denis, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Am 20.06.2018 um 18:48 schrieb Tom St Denis:
> This adds what should be a stable interface to read GPU
> load from userspace.
>
> (v2): Fix comments and name of file per recommendations.
>
> Signed-off-by: Tom St Denis <tom.stdenis@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 47 ++++++++++++++++++++++++++++++++++
> 1 file changed, 47 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 113edffb5960..49138ac2be24 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -918,6 +918,43 @@ static ssize_t amdgpu_set_pp_power_profile_mode(struct device *dev,
> return -EINVAL;
> }
>
> +/**
> + * DOC: busy_percent
> + *
> + * The firmware computes a percentage of load based on the activity
> + * level in the IP cores.
> + */
> +static ssize_t amdgpu_get_busy_percent(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct drm_device *ddev = dev_get_drvdata(dev);
> + struct amdgpu_device *adev = ddev->dev_private;
> + int r, value, size = sizeof(value);
> +
> + /* sanity check PP is enabled */
> + if (!(adev->powerplay.pp_funcs &&
> + adev->powerplay.pp_funcs->read_sensor))
> + return -EINVAL;
> +
> + /* read the IP busy sensor */
> + r = amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_LOAD,
> + (void *)&value, &size);
> + if (r)
> + return r;
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static ssize_t amdgpu_set_busy_percent(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + return -EINVAL;
> +}
> +
> +
For read only attributes the set function can be specified as NULL as well.
Apart from that this looks good to me,
Christian.
> static DEVICE_ATTR(power_dpm_state, S_IRUGO | S_IWUSR, amdgpu_get_dpm_state, amdgpu_set_dpm_state);
> static DEVICE_ATTR(power_dpm_force_performance_level, S_IRUGO | S_IWUSR,
> amdgpu_get_dpm_forced_performance_level,
> @@ -951,6 +988,8 @@ static DEVICE_ATTR(pp_power_profile_mode, S_IRUGO | S_IWUSR,
> static DEVICE_ATTR(pp_od_clk_voltage, S_IRUGO | S_IWUSR,
> amdgpu_get_pp_od_clk_voltage,
> amdgpu_set_pp_od_clk_voltage);
> +static DEVICE_ATTR(gpu_busy_percent, S_IRUGO | S_IWUSR,
> + amdgpu_get_busy_percent, amdgpu_set_busy_percent);
>
> static ssize_t amdgpu_hwmon_show_temp(struct device *dev,
> struct device_attribute *attr,
> @@ -1854,6 +1893,13 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device *adev)
> "pp_od_clk_voltage\n");
> return ret;
> }
> + ret = device_create_file(adev->dev,
> + &dev_attr_gpu_busy_percent);
> + if (ret) {
> + DRM_ERROR("failed to create device file "
> + "gpu_busy_level\n");
> + return ret;
> + }
> ret = amdgpu_debugfs_pm_init(adev);
> if (ret) {
> DRM_ERROR("Failed to register debugfs file for dpm!\n");
> @@ -1889,6 +1935,7 @@ void amdgpu_pm_sysfs_fini(struct amdgpu_device *adev)
> &dev_attr_pp_power_profile_mode);
> device_remove_file(adev->dev,
> &dev_attr_pp_od_clk_voltage);
> + device_remove_file(adev->dev, &dev_attr_gpu_busy_percent);
> }
>
> void amdgpu_pm_compute_clocks(struct amdgpu_device *adev)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread