* [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
@ 2021-09-08 5:56 Lang Yu
2021-09-08 6:37 ` Christian König
0 siblings, 1 reply; 18+ messages in thread
From: Lang Yu @ 2021-09-08 5:56 UTC (permalink / raw)
To: amd-gfx; +Cc: Alex Deucher, Huang Rui, Lijo Lazar, Lang Yu
sysfs_emit and sysfs_emit_at requrie a page boundary
aligned buf address. Make them happy!
Warning Log:
[ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0
[ 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765 sysfs_emit_at+0x4a/0xa0
[ 492.654805] Call Trace:
[ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu]
[ 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu]
[ 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu]
[ 492.659733] ? preempt_schedule_common+0x18/0x30
[ 492.660713] smu_print_ppclk_levels+0x65/0x90 [amdgpu]
[ 492.662107] amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu]
[ 492.663620] dev_attr_show+0x1d/0x40
Signed-off-by: Lang Yu <lang.yu@amd.com>
---
drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 9 +++++++--
drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 5 ++++-
.../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 5 ++++-
drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 15 +++++++++------
drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 3 +++
.../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 13 +++++++++----
.../gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c | 7 +++++--
7 files changed, 41 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
index e343cc218990..53185fe96d83 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
@@ -771,8 +771,13 @@ static int arcturus_print_clk_levels(struct smu_context *smu,
struct smu_11_0_dpm_context *dpm_context = NULL;
uint32_t gen_speed, lane_width;
- if (amdgpu_ras_intr_triggered())
- return sysfs_emit(buf, "unavailable\n");
+ size = offset_in_page(buf);
+ buf = buf - size;
+
+ if (amdgpu_ras_intr_triggered()) {
+ size += sysfs_emit_at(buf, size, "unavailable\n");
+ return size;
+ }
dpm_context = smu_dpm->dpm_context;
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
index 4c81989b8162..5490e8e66e14 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
@@ -1279,6 +1279,9 @@ static int navi10_print_clk_levels(struct smu_context *smu,
struct smu_11_0_overdrive_table *od_settings = smu->od_settings;
uint32_t min_value, max_value;
+ size = offset_in_page(buf);
+ buf = buf - size;
+
switch (clk_type) {
case SMU_GFXCLK:
case SMU_SCLK:
@@ -1392,7 +1395,7 @@ static int navi10_print_clk_levels(struct smu_context *smu,
case SMU_OD_RANGE:
if (!smu->od_enabled || !od_table || !od_settings)
break;
- size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
+ size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
if (navi10_od_feature_is_supported(od_settings, SMU_11_0_ODCAP_GFXCLK_LIMITS)) {
navi10_od_setting_get_range(od_settings, SMU_11_0_ODSETTING_GFXCLKFMIN,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
index 5e292c3f5050..817ad6de3854 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
@@ -1058,6 +1058,9 @@ static int sienna_cichlid_print_clk_levels(struct smu_context *smu,
uint32_t min_value, max_value;
uint32_t smu_version;
+ size = offset_in_page(buf);
+ buf = buf - size;
+
switch (clk_type) {
case SMU_GFXCLK:
case SMU_SCLK:
@@ -1180,7 +1183,7 @@ static int sienna_cichlid_print_clk_levels(struct smu_context *smu,
if (!smu->od_enabled || !od_table || !od_settings)
break;
- size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
+ size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
if (sienna_cichlid_is_od_feature_supported(od_settings, SMU_11_0_7_ODCAP_GFXCLK_LIMITS)) {
sienna_cichlid_get_od_setting_range(od_settings, SMU_11_0_7_ODSETTING_GFXCLKFMIN,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
index 3a3421452e57..c7842c69b570 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
@@ -592,7 +592,7 @@ static int vangogh_print_legacy_clk_levels(struct smu_context *smu,
switch (clk_type) {
case SMU_OD_SCLK:
if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
- size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
+ size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
(smu->gfx_actual_hard_min_freq > 0) ? smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
@@ -601,7 +601,7 @@ static int vangogh_print_legacy_clk_levels(struct smu_context *smu,
break;
case SMU_OD_CCLK:
if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
- size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n", smu->cpu_core_id_select);
+ size += sysfs_emit_at(buf, size, "CCLK_RANGE in Core%d:\n", smu->cpu_core_id_select);
size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
(smu->cpu_actual_soft_min_freq > 0) ? smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
@@ -610,7 +610,7 @@ static int vangogh_print_legacy_clk_levels(struct smu_context *smu,
break;
case SMU_OD_RANGE:
if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
- size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
+ size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
smu->gfx_default_hard_min_freq, smu->gfx_default_soft_max_freq);
size += sysfs_emit_at(buf, size, "CCLK: %7uMhz %10uMhz\n",
@@ -682,6 +682,9 @@ static int vangogh_print_clk_levels(struct smu_context *smu,
uint32_t cur_value = 0, value = 0, count = 0;
bool cur_value_match_level = false;
+ size = offset_in_page(buf);
+ buf = buf - size;
+
memset(&metrics, 0, sizeof(metrics));
ret = smu_cmn_get_metrics_table(smu, &metrics, false);
@@ -691,7 +694,7 @@ static int vangogh_print_clk_levels(struct smu_context *smu,
switch (clk_type) {
case SMU_OD_SCLK:
if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
- size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
+ size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
(smu->gfx_actual_hard_min_freq > 0) ? smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
@@ -700,7 +703,7 @@ static int vangogh_print_clk_levels(struct smu_context *smu,
break;
case SMU_OD_CCLK:
if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
- size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n", smu->cpu_core_id_select);
+ size += sysfs_emit_at(buf, size, "CCLK_RANGE in Core%d:\n", smu->cpu_core_id_select);
size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
(smu->cpu_actual_soft_min_freq > 0) ? smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
@@ -709,7 +712,7 @@ static int vangogh_print_clk_levels(struct smu_context *smu,
break;
case SMU_OD_RANGE:
if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
- size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
+ size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
smu->gfx_default_hard_min_freq, smu->gfx_default_soft_max_freq);
size += sysfs_emit_at(buf, size, "CCLK: %7uMhz %10uMhz\n",
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
index 5aa175e12a78..86e7978b6d63 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
@@ -491,6 +491,9 @@ static int renoir_print_clk_levels(struct smu_context *smu,
struct smu_dpm_context *smu_dpm_ctx = &(smu->smu_dpm);
bool cur_value_match_level = false;
+ size = offset_in_page(buf);
+ buf = buf - size;
+
memset(&metrics, 0, sizeof(metrics));
ret = smu_cmn_get_metrics_table(smu, &metrics, false);
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
index ab652028e003..6349f27e9efc 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -733,15 +733,20 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
uint32_t freq_values[3] = {0};
uint32_t min_clk, max_clk;
- if (amdgpu_ras_intr_triggered())
- return sysfs_emit(buf, "unavailable\n");
+ size = offset_in_page(buf);
+ buf = buf - size;
+
+ if (amdgpu_ras_intr_triggered()) {
+ size += sysfs_emit_at(buf, size, "unavailable\n");
+ return size;
+ }
dpm_context = smu_dpm->dpm_context;
switch (type) {
case SMU_OD_SCLK:
- size = sysfs_emit(buf, "%s:\n", "GFXCLK");
+ size += sysfs_emit_at(buf, size, "%s:\n", "GFXCLK");
fallthrough;
case SMU_SCLK:
ret = aldebaran_get_current_clk_freq_by_table(smu, SMU_GFXCLK, &now);
@@ -795,7 +800,7 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
break;
case SMU_OD_MCLK:
- size = sysfs_emit(buf, "%s:\n", "MCLK");
+ size += sysfs_emit_at(buf, size, "%s:\n", "MCLK");
fallthrough;
case SMU_MCLK:
ret = aldebaran_get_current_clk_freq_by_table(smu, SMU_UCLK, &now);
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
index 627ba2eec7fd..3b21d9143b96 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
@@ -1052,16 +1052,19 @@ static int yellow_carp_print_clk_levels(struct smu_context *smu,
int i, size = 0, ret = 0;
uint32_t cur_value = 0, value = 0, count = 0;
+ size = offset_in_page(buf);
+ buf = buf - size;
+
switch (clk_type) {
case SMU_OD_SCLK:
- size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
+ size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
(smu->gfx_actual_hard_min_freq > 0) ? smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
(smu->gfx_actual_soft_max_freq > 0) ? smu->gfx_actual_soft_max_freq : smu->gfx_default_soft_max_freq);
break;
case SMU_OD_RANGE:
- size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
+ size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
smu->gfx_default_hard_min_freq, smu->gfx_default_soft_max_freq);
break;
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 5:56 [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings Lang Yu
@ 2021-09-08 6:37 ` Christian König
2021-09-08 7:36 ` Lazar, Lijo
0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2021-09-08 6:37 UTC (permalink / raw)
To: Lang Yu, amd-gfx; +Cc: Alex Deucher, Huang Rui, Lijo Lazar, Tian Tao
Am 08.09.21 um 07:56 schrieb Lang Yu:
> sysfs_emit and sysfs_emit_at requrie a page boundary
> aligned buf address. Make them happy!
>
> Warning Log:
> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0
> [ 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765 sysfs_emit_at+0x4a/0xa0
> [ 492.654805] Call Trace:
> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu]
> [ 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu]
> [ 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu]
> [ 492.659733] ? preempt_schedule_common+0x18/0x30
> [ 492.660713] smu_print_ppclk_levels+0x65/0x90 [amdgpu]
> [ 492.662107] amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu]
> [ 492.663620] dev_attr_show+0x1d/0x40
Mhm, that at least partially doesn't looks like the right approach to me.
Why do we have string printing and sysfs code in the hardware version
specific backend in the first place?
That stuff needs to be implemented for each hardware generation and is
now cluttered with sysfs buffer offset calculations.
Regards,
Christian.
>
> Signed-off-by: Lang Yu <lang.yu@amd.com>
> ---
> drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 9 +++++++--
> drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 5 ++++-
> .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 5 ++++-
> drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 15 +++++++++------
> drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 3 +++
> .../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 13 +++++++++----
> .../gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c | 7 +++++--
> 7 files changed, 41 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> index e343cc218990..53185fe96d83 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
> @@ -771,8 +771,13 @@ static int arcturus_print_clk_levels(struct smu_context *smu,
> struct smu_11_0_dpm_context *dpm_context = NULL;
> uint32_t gen_speed, lane_width;
>
> - if (amdgpu_ras_intr_triggered())
> - return sysfs_emit(buf, "unavailable\n");
> + size = offset_in_page(buf);
> + buf = buf - size;
> +
> + if (amdgpu_ras_intr_triggered()) {
> + size += sysfs_emit_at(buf, size, "unavailable\n");
> + return size;
> + }
>
> dpm_context = smu_dpm->dpm_context;
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> index 4c81989b8162..5490e8e66e14 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> @@ -1279,6 +1279,9 @@ static int navi10_print_clk_levels(struct smu_context *smu,
> struct smu_11_0_overdrive_table *od_settings = smu->od_settings;
> uint32_t min_value, max_value;
>
> + size = offset_in_page(buf);
> + buf = buf - size;
> +
> switch (clk_type) {
> case SMU_GFXCLK:
> case SMU_SCLK:
> @@ -1392,7 +1395,7 @@ static int navi10_print_clk_levels(struct smu_context *smu,
> case SMU_OD_RANGE:
> if (!smu->od_enabled || !od_table || !od_settings)
> break;
> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>
> if (navi10_od_feature_is_supported(od_settings, SMU_11_0_ODCAP_GFXCLK_LIMITS)) {
> navi10_od_setting_get_range(od_settings, SMU_11_0_ODSETTING_GFXCLKFMIN,
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> index 5e292c3f5050..817ad6de3854 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
> @@ -1058,6 +1058,9 @@ static int sienna_cichlid_print_clk_levels(struct smu_context *smu,
> uint32_t min_value, max_value;
> uint32_t smu_version;
>
> + size = offset_in_page(buf);
> + buf = buf - size;
> +
> switch (clk_type) {
> case SMU_GFXCLK:
> case SMU_SCLK:
> @@ -1180,7 +1183,7 @@ static int sienna_cichlid_print_clk_levels(struct smu_context *smu,
> if (!smu->od_enabled || !od_table || !od_settings)
> break;
>
> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>
> if (sienna_cichlid_is_od_feature_supported(od_settings, SMU_11_0_7_ODCAP_GFXCLK_LIMITS)) {
> sienna_cichlid_get_od_setting_range(od_settings, SMU_11_0_7_ODSETTING_GFXCLKFMIN,
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> index 3a3421452e57..c7842c69b570 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
> @@ -592,7 +592,7 @@ static int vangogh_print_legacy_clk_levels(struct smu_context *smu,
> switch (clk_type) {
> case SMU_OD_SCLK:
> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
> (smu->gfx_actual_hard_min_freq > 0) ? smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
> @@ -601,7 +601,7 @@ static int vangogh_print_legacy_clk_levels(struct smu_context *smu,
> break;
> case SMU_OD_CCLK:
> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n", smu->cpu_core_id_select);
> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in Core%d:\n", smu->cpu_core_id_select);
> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
> (smu->cpu_actual_soft_min_freq > 0) ? smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
> @@ -610,7 +610,7 @@ static int vangogh_print_legacy_clk_levels(struct smu_context *smu,
> break;
> case SMU_OD_RANGE:
> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
> smu->gfx_default_hard_min_freq, smu->gfx_default_soft_max_freq);
> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz %10uMhz\n",
> @@ -682,6 +682,9 @@ static int vangogh_print_clk_levels(struct smu_context *smu,
> uint32_t cur_value = 0, value = 0, count = 0;
> bool cur_value_match_level = false;
>
> + size = offset_in_page(buf);
> + buf = buf - size;
> +
> memset(&metrics, 0, sizeof(metrics));
>
> ret = smu_cmn_get_metrics_table(smu, &metrics, false);
> @@ -691,7 +694,7 @@ static int vangogh_print_clk_levels(struct smu_context *smu,
> switch (clk_type) {
> case SMU_OD_SCLK:
> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
> (smu->gfx_actual_hard_min_freq > 0) ? smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
> @@ -700,7 +703,7 @@ static int vangogh_print_clk_levels(struct smu_context *smu,
> break;
> case SMU_OD_CCLK:
> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n", smu->cpu_core_id_select);
> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in Core%d:\n", smu->cpu_core_id_select);
> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
> (smu->cpu_actual_soft_min_freq > 0) ? smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
> @@ -709,7 +712,7 @@ static int vangogh_print_clk_levels(struct smu_context *smu,
> break;
> case SMU_OD_RANGE:
> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
> smu->gfx_default_hard_min_freq, smu->gfx_default_soft_max_freq);
> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz %10uMhz\n",
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
> index 5aa175e12a78..86e7978b6d63 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
> @@ -491,6 +491,9 @@ static int renoir_print_clk_levels(struct smu_context *smu,
> struct smu_dpm_context *smu_dpm_ctx = &(smu->smu_dpm);
> bool cur_value_match_level = false;
>
> + size = offset_in_page(buf);
> + buf = buf - size;
> +
> memset(&metrics, 0, sizeof(metrics));
>
> ret = smu_cmn_get_metrics_table(smu, &metrics, false);
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> index ab652028e003..6349f27e9efc 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
> @@ -733,15 +733,20 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
> uint32_t freq_values[3] = {0};
> uint32_t min_clk, max_clk;
>
> - if (amdgpu_ras_intr_triggered())
> - return sysfs_emit(buf, "unavailable\n");
> + size = offset_in_page(buf);
> + buf = buf - size;
> +
> + if (amdgpu_ras_intr_triggered()) {
> + size += sysfs_emit_at(buf, size, "unavailable\n");
> + return size;
> + }
>
> dpm_context = smu_dpm->dpm_context;
>
> switch (type) {
>
> case SMU_OD_SCLK:
> - size = sysfs_emit(buf, "%s:\n", "GFXCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "GFXCLK");
> fallthrough;
> case SMU_SCLK:
> ret = aldebaran_get_current_clk_freq_by_table(smu, SMU_GFXCLK, &now);
> @@ -795,7 +800,7 @@ static int aldebaran_print_clk_levels(struct smu_context *smu,
> break;
>
> case SMU_OD_MCLK:
> - size = sysfs_emit(buf, "%s:\n", "MCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "MCLK");
> fallthrough;
> case SMU_MCLK:
> ret = aldebaran_get_current_clk_freq_by_table(smu, SMU_UCLK, &now);
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
> index 627ba2eec7fd..3b21d9143b96 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
> @@ -1052,16 +1052,19 @@ static int yellow_carp_print_clk_levels(struct smu_context *smu,
> int i, size = 0, ret = 0;
> uint32_t cur_value = 0, value = 0, count = 0;
>
> + size = offset_in_page(buf);
> + buf = buf - size;
> +
> switch (clk_type) {
> case SMU_OD_SCLK:
> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
> (smu->gfx_actual_hard_min_freq > 0) ? smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
> (smu->gfx_actual_soft_max_freq > 0) ? smu->gfx_actual_soft_max_freq : smu->gfx_default_soft_max_freq);
> break;
> case SMU_OD_RANGE:
> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
> smu->gfx_default_hard_min_freq, smu->gfx_default_soft_max_freq);
> break;
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 6:37 ` Christian König
@ 2021-09-08 7:36 ` Lazar, Lijo
2021-09-08 7:44 ` Yu, Lang
0 siblings, 1 reply; 18+ messages in thread
From: Lazar, Lijo @ 2021-09-08 7:36 UTC (permalink / raw)
To: Christian König, Lang Yu, amd-gfx; +Cc: Alex Deucher, Huang Rui, Tian Tao
On 9/8/2021 12:07 PM, Christian König wrote:
> Am 08.09.21 um 07:56 schrieb Lang Yu:
>> sysfs_emit and sysfs_emit_at requrie a page boundary
>> aligned buf address. Make them happy!
>>
>> Warning Log:
>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0
>> [ 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>> sysfs_emit_at+0x4a/0xa0
>> [ 492.654805] Call Trace:
>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu]
>> [ 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu]
>> [ 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu]
>> [ 492.659733] ? preempt_schedule_common+0x18/0x30
>> [ 492.660713] smu_print_ppclk_levels+0x65/0x90 [amdgpu]
>> [ 492.662107] amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu]
>> [ 492.663620] dev_attr_show+0x1d/0x40
>
> Mhm, that at least partially doesn't looks like the right approach to me.
>
> Why do we have string printing and sysfs code in the hardware version
> specific backend in the first place?
>
This is a callback meant for printing ASIC specific information to sysfs
node. The buffer passed in sysfs read is passed as it is to the callback
API.
> That stuff needs to be implemented for each hardware generation and is
> now cluttered with sysfs buffer offset calculations.
>
Looks like the warning happened because of this usage.
size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK,
buf+size);
size += amdgpu_dpm_print_clock_levels(adev,
OD_VDDC_CURVE, buf+size);
size += amdgpu_dpm_print_clock_levels(adev,
OD_VDDGFX_OFFSET, buf+size);
size += amdgpu_dpm_print_clock_levels(adev, OD_RANGE,
buf+size);
size += amdgpu_dpm_print_clock_levels(adev, OD_CCLK,
buf+size);
> Regards,
> Christian.
>
>>
>> Signed-off-by: Lang Yu <lang.yu@amd.com>
>> ---
>> drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 9 +++++++--
>> drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 5 ++++-
>> .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 5 ++++-
>> drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 15 +++++++++------
>> drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 3 +++
>> .../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 13 +++++++++----
>> .../gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c | 7 +++++--
>> 7 files changed, 41 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>> index e343cc218990..53185fe96d83 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>> @@ -771,8 +771,13 @@ static int arcturus_print_clk_levels(struct
>> smu_context *smu,
>> struct smu_11_0_dpm_context *dpm_context = NULL;
>> uint32_t gen_speed, lane_width;
>> - if (amdgpu_ras_intr_triggered())
>> - return sysfs_emit(buf, "unavailable\n");
>> + size = offset_in_page(buf);
>> + buf = buf - size;
>> +
>> + if (amdgpu_ras_intr_triggered()) {
>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>> + return size;
>> + }
>> dpm_context = smu_dpm->dpm_context;
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>> index 4c81989b8162..5490e8e66e14 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>> @@ -1279,6 +1279,9 @@ static int navi10_print_clk_levels(struct
>> smu_context *smu,
>> struct smu_11_0_overdrive_table *od_settings = smu->od_settings;
>> uint32_t min_value, max_value;
>> + size = offset_in_page(buf);
>> + buf = buf - size;
>> +
>> switch (clk_type) {
>> case SMU_GFXCLK:
>> case SMU_SCLK:
>> @@ -1392,7 +1395,7 @@ static int navi10_print_clk_levels(struct
>> smu_context *smu,
>> case SMU_OD_RANGE:
>> if (!smu->od_enabled || !od_table || !od_settings)
>> break;
>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>> if (navi10_od_feature_is_supported(od_settings,
>> SMU_11_0_ODCAP_GFXCLK_LIMITS)) {
>> navi10_od_setting_get_range(od_settings,
>> SMU_11_0_ODSETTING_GFXCLKFMIN,
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>> index 5e292c3f5050..817ad6de3854 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>> @@ -1058,6 +1058,9 @@ static int
>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>> uint32_t min_value, max_value;
>> uint32_t smu_version;
>> + size = offset_in_page(buf);
>> + buf = buf - size;
>> +
>> switch (clk_type) {
>> case SMU_GFXCLK:
>> case SMU_SCLK:
>> @@ -1180,7 +1183,7 @@ static int
>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>> if (!smu->od_enabled || !od_table || !od_settings)
>> break;
>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>> if (sienna_cichlid_is_od_feature_supported(od_settings,
>> SMU_11_0_7_ODCAP_GFXCLK_LIMITS)) {
>> sienna_cichlid_get_od_setting_range(od_settings,
>> SMU_11_0_7_ODSETTING_GFXCLKFMIN,
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>> index 3a3421452e57..c7842c69b570 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>> @@ -592,7 +592,7 @@ static int vangogh_print_legacy_clk_levels(struct
>> smu_context *smu,
>> switch (clk_type) {
>> case SMU_OD_SCLK:
>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>> (smu->gfx_actual_hard_min_freq > 0) ?
>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>> @@ -601,7 +601,7 @@ static int vangogh_print_legacy_clk_levels(struct
>> smu_context *smu,
>> break;
>> case SMU_OD_CCLK:
>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>> smu->cpu_core_id_select);
>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>> Core%d:\n", smu->cpu_core_id_select);
>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>> (smu->cpu_actual_soft_min_freq > 0) ?
>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>> @@ -610,7 +610,7 @@ static int vangogh_print_legacy_clk_levels(struct
>> smu_context *smu,
>> break;
>> case SMU_OD_RANGE:
>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
>> smu->gfx_default_hard_min_freq,
>> smu->gfx_default_soft_max_freq);
>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz %10uMhz\n",
>> @@ -682,6 +682,9 @@ static int vangogh_print_clk_levels(struct
>> smu_context *smu,
>> uint32_t cur_value = 0, value = 0, count = 0;
>> bool cur_value_match_level = false;
>> + size = offset_in_page(buf);
>> + buf = buf - size;
>> +
>> memset(&metrics, 0, sizeof(metrics));
>> ret = smu_cmn_get_metrics_table(smu, &metrics, false);
>> @@ -691,7 +694,7 @@ static int vangogh_print_clk_levels(struct
>> smu_context *smu,
>> switch (clk_type) {
>> case SMU_OD_SCLK:
>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>> (smu->gfx_actual_hard_min_freq > 0) ?
>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>> @@ -700,7 +703,7 @@ static int vangogh_print_clk_levels(struct
>> smu_context *smu,
>> break;
>> case SMU_OD_CCLK:
>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>> smu->cpu_core_id_select);
>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>> Core%d:\n", smu->cpu_core_id_select);
>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>> (smu->cpu_actual_soft_min_freq > 0) ?
>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>> @@ -709,7 +712,7 @@ static int vangogh_print_clk_levels(struct
>> smu_context *smu,
>> break;
>> case SMU_OD_RANGE:
>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL) {
>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
>> smu->gfx_default_hard_min_freq,
>> smu->gfx_default_soft_max_freq);
>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz %10uMhz\n",
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>> b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>> index 5aa175e12a78..86e7978b6d63 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>> @@ -491,6 +491,9 @@ static int renoir_print_clk_levels(struct
>> smu_context *smu,
>> struct smu_dpm_context *smu_dpm_ctx = &(smu->smu_dpm);
>> bool cur_value_match_level = false;
>> + size = offset_in_page(buf);
>> + buf = buf - size;
>> +
>> memset(&metrics, 0, sizeof(metrics));
>> ret = smu_cmn_get_metrics_table(smu, &metrics, false);
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>> index ab652028e003..6349f27e9efc 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>> @@ -733,15 +733,20 @@ static int aldebaran_print_clk_levels(struct
>> smu_context *smu,
>> uint32_t freq_values[3] = {0};
>> uint32_t min_clk, max_clk;
>> - if (amdgpu_ras_intr_triggered())
>> - return sysfs_emit(buf, "unavailable\n");
>> + size = offset_in_page(buf);
>> + buf = buf - size;
>> +
>> + if (amdgpu_ras_intr_triggered()) {
>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>> + return size;
>> + }
>> dpm_context = smu_dpm->dpm_context;
>> switch (type) {
>> case SMU_OD_SCLK:
>> - size = sysfs_emit(buf, "%s:\n", "GFXCLK");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "GFXCLK");
>> fallthrough;
>> case SMU_SCLK:
>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>> SMU_GFXCLK, &now);
>> @@ -795,7 +800,7 @@ static int aldebaran_print_clk_levels(struct
>> smu_context *smu,
>> break;
>> case SMU_OD_MCLK:
>> - size = sysfs_emit(buf, "%s:\n", "MCLK");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "MCLK");
>> fallthrough;
>> case SMU_MCLK:
>> ret = aldebaran_get_current_clk_freq_by_table(smu, SMU_UCLK,
>> &now);
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>> index 627ba2eec7fd..3b21d9143b96 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>> @@ -1052,16 +1052,19 @@ static int yellow_carp_print_clk_levels(struct
>> smu_context *smu,
>> int i, size = 0, ret = 0;
>> uint32_t cur_value = 0, value = 0, count = 0;
>> + size = offset_in_page(buf);
>> + buf = buf - size;
>> +
>> switch (clk_type) {
>> case SMU_OD_SCLK:
>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>> (smu->gfx_actual_hard_min_freq > 0) ?
>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>> (smu->gfx_actual_soft_max_freq > 0) ?
>> smu->gfx_actual_soft_max_freq : smu->gfx_default_soft_max_freq);
>> break;
>> case SMU_OD_RANGE:
>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
>> smu->gfx_default_hard_min_freq,
>> smu->gfx_default_soft_max_freq);
>> break;
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 7:36 ` Lazar, Lijo
@ 2021-09-08 7:44 ` Yu, Lang
2021-09-08 8:55 ` Lazar, Lijo
0 siblings, 1 reply; 18+ messages in thread
From: Yu, Lang @ 2021-09-08 7:44 UTC (permalink / raw)
To: Lazar, Lijo, Christian König, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
[AMD Official Use Only]
>-----Original Message-----
>From: Lazar, Lijo <Lijo.Lazar@amd.com>
>Sent: Wednesday, September 8, 2021 3:36 PM
>To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
><Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
><Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>
>
>
>On 9/8/2021 12:07 PM, Christian König wrote:
>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>> address. Make them happy!
>>>
>>> Warning Log:
>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>> sysfs_emit_at+0x4a/0xa0
>>> [ 492.654805] Call Trace:
>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>> dev_attr_show+0x1d/0x40
>>
>> Mhm, that at least partially doesn't looks like the right approach to me.
>>
>> Why do we have string printing and sysfs code in the hardware version
>> specific backend in the first place?
>>
>
>This is a callback meant for printing ASIC specific information to sysfs node. The
>buffer passed in sysfs read is passed as it is to the callback API.
>
>> That stuff needs to be implemented for each hardware generation and is
>> now cluttered with sysfs buffer offset calculations.
>>
>
>Looks like the warning happened because of this usage.
>
> size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
> size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK,
>buf+size);
> size += amdgpu_dpm_print_clock_levels(adev,
>OD_VDDC_CURVE, buf+size);
> size += amdgpu_dpm_print_clock_levels(adev,
>OD_VDDGFX_OFFSET, buf+size);
> size += amdgpu_dpm_print_clock_levels(adev, OD_RANGE,
>buf+size);
> size += amdgpu_dpm_print_clock_levels(adev, OD_CCLK,
>buf+size);
>
>
[Yu, Lang]
Yes. So it is fine we just fix the caller amdgpu_get_pp_od_clk_voltage like following:
static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
struct device_attribute *attr,
char *buf)
{
struct drm_device *ddev = dev_get_drvdata(dev);
struct amdgpu_device *adev = drm_to_adev(ddev);
ssize_t size, offset;
int ret, i;
char temp_buf[512];
char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
if (amdgpu_in_reset(adev))
return -EPERM;
if (adev->in_suspend && !adev->in_runpm)
return -EPERM;
ret = pm_runtime_get_sync(ddev->dev);
if (ret < 0) {
pm_runtime_put_autosuspend(ddev->dev);
return ret;
}
offset = 0;
if (adev->powerplay.pp_funcs->print_clock_levels) {
for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
size = amdgpu_dpm_print_clock_levels(adev, clock_type[i], buf);
if (offset + size > PAGE_SIZE)
break;
memcpy(temp_buf + offset, buf, size);
offset += size;
}
memcpy(buf, temp_buf, offset);
size = offset;
} else {
size = sysfs_emit(buf, "\n");
}
pm_runtime_mark_last_busy(ddev->dev);
pm_runtime_put_autosuspend(ddev->dev);
return size;
}
Regards,
Lang
>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Lang Yu <lang.yu@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 9 +++++++--
>>> drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 5 ++++-
>>> .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 5 ++++-
>>> drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 15
>>> +++++++++------
>>> drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 3 +++
>>> .../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 13
>>> +++++++++----
>>> .../gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c | 7 +++++--
>>> 7 files changed, 41 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>> index e343cc218990..53185fe96d83 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>> @@ -771,8 +771,13 @@ static int arcturus_print_clk_levels(struct
>>> smu_context *smu,
>>> struct smu_11_0_dpm_context *dpm_context = NULL;
>>> uint32_t gen_speed, lane_width;
>>> - if (amdgpu_ras_intr_triggered())
>>> - return sysfs_emit(buf, "unavailable\n");
>>> + size = offset_in_page(buf);
>>> + buf = buf - size;
>>> +
>>> + if (amdgpu_ras_intr_triggered()) {
>>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>>> + return size;
>>> + }
>>> dpm_context = smu_dpm->dpm_context; diff --git
>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> index 4c81989b8162..5490e8e66e14 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> @@ -1279,6 +1279,9 @@ static int navi10_print_clk_levels(struct
>>> smu_context *smu,
>>> struct smu_11_0_overdrive_table *od_settings =
>>> smu->od_settings;
>>> uint32_t min_value, max_value;
>>> + size = offset_in_page(buf);
>>> + buf = buf - size;
>>> +
>>> switch (clk_type) {
>>> case SMU_GFXCLK:
>>> case SMU_SCLK:
>>> @@ -1392,7 +1395,7 @@ static int navi10_print_clk_levels(struct
>>> smu_context *smu,
>>> case SMU_OD_RANGE:
>>> if (!smu->od_enabled || !od_table || !od_settings)
>>> break;
>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>> if (navi10_od_feature_is_supported(od_settings,
>>> SMU_11_0_ODCAP_GFXCLK_LIMITS)) {
>>> navi10_od_setting_get_range(od_settings,
>>> SMU_11_0_ODSETTING_GFXCLKFMIN,
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>> index 5e292c3f5050..817ad6de3854 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>> @@ -1058,6 +1058,9 @@ static int
>>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>>> uint32_t min_value, max_value;
>>> uint32_t smu_version;
>>> + size = offset_in_page(buf);
>>> + buf = buf - size;
>>> +
>>> switch (clk_type) {
>>> case SMU_GFXCLK:
>>> case SMU_SCLK:
>>> @@ -1180,7 +1183,7 @@ static int
>>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>>> if (!smu->od_enabled || !od_table || !od_settings)
>>> break;
>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>> if (sienna_cichlid_is_od_feature_supported(od_settings,
>>> SMU_11_0_7_ODCAP_GFXCLK_LIMITS)) {
>>> sienna_cichlid_get_od_setting_range(od_settings,
>>> SMU_11_0_7_ODSETTING_GFXCLKFMIN,
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>> index 3a3421452e57..c7842c69b570 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>> @@ -592,7 +592,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>> smu_context *smu,
>>> switch (clk_type) {
>>> case SMU_OD_SCLK:
>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>> {
>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>> -601,7 +601,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>> smu_context *smu,
>>> break;
>>> case SMU_OD_CCLK:
>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>> {
>>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>>> smu->cpu_core_id_select);
>>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>>> Core%d:\n", smu->cpu_core_id_select);
>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>> (smu->cpu_actual_soft_min_freq > 0) ?
>>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>> -610,7 +610,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>> smu_context *smu,
>>> break;
>>> case SMU_OD_RANGE:
>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>> {
>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>> %10uMhz\n",
>>> smu->gfx_default_hard_min_freq,
>>> smu->gfx_default_soft_max_freq);
>>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz
>>> %10uMhz\n", @@ -682,6 +682,9 @@ static int
>>> vangogh_print_clk_levels(struct smu_context *smu,
>>> uint32_t cur_value = 0, value = 0, count = 0;
>>> bool cur_value_match_level = false;
>>> + size = offset_in_page(buf);
>>> + buf = buf - size;
>>> +
>>> memset(&metrics, 0, sizeof(metrics));
>>> ret = smu_cmn_get_metrics_table(smu, &metrics, false); @@
>>> -691,7 +694,7 @@ static int vangogh_print_clk_levels(struct
>>> smu_context *smu,
>>> switch (clk_type) {
>>> case SMU_OD_SCLK:
>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>> {
>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>> -700,7 +703,7 @@ static int vangogh_print_clk_levels(struct
>>> smu_context *smu,
>>> break;
>>> case SMU_OD_CCLK:
>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>> {
>>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>>> smu->cpu_core_id_select);
>>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>>> Core%d:\n", smu->cpu_core_id_select);
>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>> (smu->cpu_actual_soft_min_freq > 0) ?
>>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>> -709,7 +712,7 @@ static int vangogh_print_clk_levels(struct
>>> smu_context *smu,
>>> break;
>>> case SMU_OD_RANGE:
>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>> {
>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>> %10uMhz\n",
>>> smu->gfx_default_hard_min_freq,
>>> smu->gfx_default_soft_max_freq);
>>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz
>>> %10uMhz\n", diff --git
>>> a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>> index 5aa175e12a78..86e7978b6d63 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>> @@ -491,6 +491,9 @@ static int renoir_print_clk_levels(struct
>>> smu_context *smu,
>>> struct smu_dpm_context *smu_dpm_ctx = &(smu->smu_dpm);
>>> bool cur_value_match_level = false;
>>> + size = offset_in_page(buf);
>>> + buf = buf - size;
>>> +
>>> memset(&metrics, 0, sizeof(metrics));
>>> ret = smu_cmn_get_metrics_table(smu, &metrics, false); diff
>>> --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> index ab652028e003..6349f27e9efc 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>> @@ -733,15 +733,20 @@ static int aldebaran_print_clk_levels(struct
>>> smu_context *smu,
>>> uint32_t freq_values[3] = {0};
>>> uint32_t min_clk, max_clk;
>>> - if (amdgpu_ras_intr_triggered())
>>> - return sysfs_emit(buf, "unavailable\n");
>>> + size = offset_in_page(buf);
>>> + buf = buf - size;
>>> +
>>> + if (amdgpu_ras_intr_triggered()) {
>>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>>> + return size;
>>> + }
>>> dpm_context = smu_dpm->dpm_context;
>>> switch (type) {
>>> case SMU_OD_SCLK:
>>> - size = sysfs_emit(buf, "%s:\n", "GFXCLK");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "GFXCLK");
>>> fallthrough;
>>> case SMU_SCLK:
>>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>>> SMU_GFXCLK, &now);
>>> @@ -795,7 +800,7 @@ static int aldebaran_print_clk_levels(struct
>>> smu_context *smu,
>>> break;
>>> case SMU_OD_MCLK:
>>> - size = sysfs_emit(buf, "%s:\n", "MCLK");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "MCLK");
>>> fallthrough;
>>> case SMU_MCLK:
>>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>>> SMU_UCLK, &now); diff --git
>>> a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>> index 627ba2eec7fd..3b21d9143b96 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>> @@ -1052,16 +1052,19 @@ static int
>>> yellow_carp_print_clk_levels(struct
>>> smu_context *smu,
>>> int i, size = 0, ret = 0;
>>> uint32_t cur_value = 0, value = 0, count = 0;
>>> + size = offset_in_page(buf);
>>> + buf = buf - size;
>>> +
>>> switch (clk_type) {
>>> case SMU_OD_SCLK:
>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>>> (smu->gfx_actual_soft_max_freq > 0) ?
>>> smu->gfx_actual_soft_max_freq : smu->gfx_default_soft_max_freq);
>>> break;
>>> case SMU_OD_RANGE:
>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
>>> smu->gfx_default_hard_min_freq,
>>> smu->gfx_default_soft_max_freq);
>>> break;
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 7:44 ` Yu, Lang
@ 2021-09-08 8:55 ` Lazar, Lijo
2021-09-08 9:02 ` Yu, Lang
0 siblings, 1 reply; 18+ messages in thread
From: Lazar, Lijo @ 2021-09-08 8:55 UTC (permalink / raw)
To: Yu, Lang, Christian König, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
On 9/8/2021 1:14 PM, Yu, Lang wrote:
> [AMD Official Use Only]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Wednesday, September 8, 2021 3:36 PM
>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>
>>
>>
>> On 9/8/2021 12:07 PM, Christian König wrote:
>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>>> address. Make them happy!
>>>>
>>>> Warning Log:
>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>> sysfs_emit_at+0x4a/0xa0
>>>> [ 492.654805] Call Trace:
>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>> dev_attr_show+0x1d/0x40
>>>
>>> Mhm, that at least partially doesn't looks like the right approach to me.
>>>
>>> Why do we have string printing and sysfs code in the hardware version
>>> specific backend in the first place?
>>>
>>
>> This is a callback meant for printing ASIC specific information to sysfs node. The
>> buffer passed in sysfs read is passed as it is to the callback API.
>>
>>> That stuff needs to be implemented for each hardware generation and is
>>> now cluttered with sysfs buffer offset calculations.
>>>
>>
>> Looks like the warning happened because of this usage.
>>
>> size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
>> size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK,
>> buf+size);
>> size += amdgpu_dpm_print_clock_levels(adev,
>> OD_VDDC_CURVE, buf+size);
>> size += amdgpu_dpm_print_clock_levels(adev,
>> OD_VDDGFX_OFFSET, buf+size);
>> size += amdgpu_dpm_print_clock_levels(adev, OD_RANGE,
>> buf+size);
>> size += amdgpu_dpm_print_clock_levels(adev, OD_CCLK,
>> buf+size);
>>
>>
> [Yu, Lang]
> Yes. So it is fine we just fix the caller amdgpu_get_pp_od_clk_voltage like following:
>
> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> struct drm_device *ddev = dev_get_drvdata(dev);
> struct amdgpu_device *adev = drm_to_adev(ddev);
> ssize_t size, offset;
> int ret, i;
> char temp_buf[512];
> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>
> if (amdgpu_in_reset(adev))
> return -EPERM;
> if (adev->in_suspend && !adev->in_runpm)
> return -EPERM;
>
> ret = pm_runtime_get_sync(ddev->dev);
> if (ret < 0) {
> pm_runtime_put_autosuspend(ddev->dev);
> return ret;
> }
>
> offset = 0;
>
> if (adev->powerplay.pp_funcs->print_clock_levels) {
> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
> size = amdgpu_dpm_print_clock_levels(adev, clock_type[i], buf);
> if (offset + size > PAGE_SIZE)
> break;
> memcpy(temp_buf + offset, buf, size);
> offset += size;
> }
> memcpy(buf, temp_buf, offset);
> size = offset;
> } else {
> size = sysfs_emit(buf, "\n");
> }
> pm_runtime_mark_last_busy(ddev->dev);
> pm_runtime_put_autosuspend(ddev->dev);
>
> return size;
> }
>
Prefer to avoid any extra stack or heap usage for buffer. Maybe another
arg to pass offset along with buf?
Thanks,
Lijo
> Regards,
> Lang
>
>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Signed-off-by: Lang Yu <lang.yu@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 9 +++++++--
>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 5 ++++-
>>>> .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 5 ++++-
>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 15
>>>> +++++++++------
>>>> drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 3 +++
>>>> .../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 13
>>>> +++++++++----
>>>> .../gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c | 7 +++++--
>>>> 7 files changed, 41 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>> index e343cc218990..53185fe96d83 100644
>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>> @@ -771,8 +771,13 @@ static int arcturus_print_clk_levels(struct
>>>> smu_context *smu,
>>>> struct smu_11_0_dpm_context *dpm_context = NULL;
>>>> uint32_t gen_speed, lane_width;
>>>> - if (amdgpu_ras_intr_triggered())
>>>> - return sysfs_emit(buf, "unavailable\n");
>>>> + size = offset_in_page(buf);
>>>> + buf = buf - size;
>>>> +
>>>> + if (amdgpu_ras_intr_triggered()) {
>>>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>>>> + return size;
>>>> + }
>>>> dpm_context = smu_dpm->dpm_context; diff --git
>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>> index 4c81989b8162..5490e8e66e14 100644
>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>> @@ -1279,6 +1279,9 @@ static int navi10_print_clk_levels(struct
>>>> smu_context *smu,
>>>> struct smu_11_0_overdrive_table *od_settings =
>>>> smu->od_settings;
>>>> uint32_t min_value, max_value;
>>>> + size = offset_in_page(buf);
>>>> + buf = buf - size;
>>>> +
>>>> switch (clk_type) {
>>>> case SMU_GFXCLK:
>>>> case SMU_SCLK:
>>>> @@ -1392,7 +1395,7 @@ static int navi10_print_clk_levels(struct
>>>> smu_context *smu,
>>>> case SMU_OD_RANGE:
>>>> if (!smu->od_enabled || !od_table || !od_settings)
>>>> break;
>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>> if (navi10_od_feature_is_supported(od_settings,
>>>> SMU_11_0_ODCAP_GFXCLK_LIMITS)) {
>>>> navi10_od_setting_get_range(od_settings,
>>>> SMU_11_0_ODSETTING_GFXCLKFMIN,
>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>> index 5e292c3f5050..817ad6de3854 100644
>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>> @@ -1058,6 +1058,9 @@ static int
>>>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>>>> uint32_t min_value, max_value;
>>>> uint32_t smu_version;
>>>> + size = offset_in_page(buf);
>>>> + buf = buf - size;
>>>> +
>>>> switch (clk_type) {
>>>> case SMU_GFXCLK:
>>>> case SMU_SCLK:
>>>> @@ -1180,7 +1183,7 @@ static int
>>>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>>>> if (!smu->od_enabled || !od_table || !od_settings)
>>>> break;
>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>> if (sienna_cichlid_is_od_feature_supported(od_settings,
>>>> SMU_11_0_7_ODCAP_GFXCLK_LIMITS)) {
>>>> sienna_cichlid_get_od_setting_range(od_settings,
>>>> SMU_11_0_7_ODSETTING_GFXCLKFMIN,
>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>> index 3a3421452e57..c7842c69b570 100644
>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>> @@ -592,7 +592,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>>> smu_context *smu,
>>>> switch (clk_type) {
>>>> case SMU_OD_SCLK:
>>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>>> {
>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>> -601,7 +601,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>>> smu_context *smu,
>>>> break;
>>>> case SMU_OD_CCLK:
>>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>>> {
>>>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>>>> smu->cpu_core_id_select);
>>>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>>>> Core%d:\n", smu->cpu_core_id_select);
>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>> (smu->cpu_actual_soft_min_freq > 0) ?
>>>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>> -610,7 +610,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>>> smu_context *smu,
>>>> break;
>>>> case SMU_OD_RANGE:
>>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>>> {
>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>>> %10uMhz\n",
>>>> smu->gfx_default_hard_min_freq,
>>>> smu->gfx_default_soft_max_freq);
>>>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz
>>>> %10uMhz\n", @@ -682,6 +682,9 @@ static int
>>>> vangogh_print_clk_levels(struct smu_context *smu,
>>>> uint32_t cur_value = 0, value = 0, count = 0;
>>>> bool cur_value_match_level = false;
>>>> + size = offset_in_page(buf);
>>>> + buf = buf - size;
>>>> +
>>>> memset(&metrics, 0, sizeof(metrics));
>>>> ret = smu_cmn_get_metrics_table(smu, &metrics, false); @@
>>>> -691,7 +694,7 @@ static int vangogh_print_clk_levels(struct
>>>> smu_context *smu,
>>>> switch (clk_type) {
>>>> case SMU_OD_SCLK:
>>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>>> {
>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>> -700,7 +703,7 @@ static int vangogh_print_clk_levels(struct
>>>> smu_context *smu,
>>>> break;
>>>> case SMU_OD_CCLK:
>>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>>> {
>>>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>>>> smu->cpu_core_id_select);
>>>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>>>> Core%d:\n", smu->cpu_core_id_select);
>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>> (smu->cpu_actual_soft_min_freq > 0) ?
>>>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>> -709,7 +712,7 @@ static int vangogh_print_clk_levels(struct
>>>> smu_context *smu,
>>>> break;
>>>> case SMU_OD_RANGE:
>>>> if (smu_dpm_ctx->dpm_level == AMD_DPM_FORCED_LEVEL_MANUAL)
>>>> {
>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>>> %10uMhz\n",
>>>> smu->gfx_default_hard_min_freq,
>>>> smu->gfx_default_soft_max_freq);
>>>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz
>>>> %10uMhz\n", diff --git
>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>> index 5aa175e12a78..86e7978b6d63 100644
>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>> @@ -491,6 +491,9 @@ static int renoir_print_clk_levels(struct
>>>> smu_context *smu,
>>>> struct smu_dpm_context *smu_dpm_ctx = &(smu->smu_dpm);
>>>> bool cur_value_match_level = false;
>>>> + size = offset_in_page(buf);
>>>> + buf = buf - size;
>>>> +
>>>> memset(&metrics, 0, sizeof(metrics));
>>>> ret = smu_cmn_get_metrics_table(smu, &metrics, false); diff
>>>> --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>> index ab652028e003..6349f27e9efc 100644
>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>> @@ -733,15 +733,20 @@ static int aldebaran_print_clk_levels(struct
>>>> smu_context *smu,
>>>> uint32_t freq_values[3] = {0};
>>>> uint32_t min_clk, max_clk;
>>>> - if (amdgpu_ras_intr_triggered())
>>>> - return sysfs_emit(buf, "unavailable\n");
>>>> + size = offset_in_page(buf);
>>>> + buf = buf - size;
>>>> +
>>>> + if (amdgpu_ras_intr_triggered()) {
>>>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>>>> + return size;
>>>> + }
>>>> dpm_context = smu_dpm->dpm_context;
>>>> switch (type) {
>>>> case SMU_OD_SCLK:
>>>> - size = sysfs_emit(buf, "%s:\n", "GFXCLK");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "GFXCLK");
>>>> fallthrough;
>>>> case SMU_SCLK:
>>>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>>>> SMU_GFXCLK, &now);
>>>> @@ -795,7 +800,7 @@ static int aldebaran_print_clk_levels(struct
>>>> smu_context *smu,
>>>> break;
>>>> case SMU_OD_MCLK:
>>>> - size = sysfs_emit(buf, "%s:\n", "MCLK");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "MCLK");
>>>> fallthrough;
>>>> case SMU_MCLK:
>>>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>>>> SMU_UCLK, &now); diff --git
>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>> index 627ba2eec7fd..3b21d9143b96 100644
>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>> @@ -1052,16 +1052,19 @@ static int
>>>> yellow_carp_print_clk_levels(struct
>>>> smu_context *smu,
>>>> int i, size = 0, ret = 0;
>>>> uint32_t cur_value = 0, value = 0, count = 0;
>>>> + size = offset_in_page(buf);
>>>> + buf = buf - size;
>>>> +
>>>> switch (clk_type) {
>>>> case SMU_OD_SCLK:
>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>>>> (smu->gfx_actual_soft_max_freq > 0) ?
>>>> smu->gfx_actual_soft_max_freq : smu->gfx_default_soft_max_freq);
>>>> break;
>>>> case SMU_OD_RANGE:
>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz %10uMhz\n",
>>>> smu->gfx_default_hard_min_freq,
>>>> smu->gfx_default_soft_max_freq);
>>>> break;
>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 8:55 ` Lazar, Lijo
@ 2021-09-08 9:02 ` Yu, Lang
2021-09-08 9:29 ` Lazar, Lijo
0 siblings, 1 reply; 18+ messages in thread
From: Yu, Lang @ 2021-09-08 9:02 UTC (permalink / raw)
To: Lazar, Lijo, Christian König, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
[AMD Official Use Only]
>-----Original Message-----
>From: Lazar, Lijo <Lijo.Lazar@amd.com>
>Sent: Wednesday, September 8, 2021 4:55 PM
>To: Yu, Lang <Lang.Yu@amd.com>; Christian König
><ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
><Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>
>
>
>On 9/8/2021 1:14 PM, Yu, Lang wrote:
>> [AMD Official Use Only]
>>
>>
>>
>>> -----Original Message-----
>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>> warnings
>>>
>>>
>>>
>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>>>> address. Make them happy!
>>>>>
>>>>> Warning Log:
>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>> sysfs_emit_at+0x4a/0xa0
>>>>> [ 492.654805] Call Trace:
>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>> dev_attr_show+0x1d/0x40
>>>>
>>>> Mhm, that at least partially doesn't looks like the right approach to me.
>>>>
>>>> Why do we have string printing and sysfs code in the hardware
>>>> version specific backend in the first place?
>>>>
>>>
>>> This is a callback meant for printing ASIC specific information to
>>> sysfs node. The buffer passed in sysfs read is passed as it is to the callback API.
>>>
>>>> That stuff needs to be implemented for each hardware generation and
>>>> is now cluttered with sysfs buffer offset calculations.
>>>>
>>>
>>> Looks like the warning happened because of this usage.
>>>
>>> size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
>>> size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK,
>>> buf+size);
>>> size += amdgpu_dpm_print_clock_levels(adev,
>>> OD_VDDC_CURVE, buf+size);
>>> size += amdgpu_dpm_print_clock_levels(adev,
>>> OD_VDDGFX_OFFSET, buf+size);
>>> size += amdgpu_dpm_print_clock_levels(adev,
>>> OD_RANGE,
>>> buf+size);
>>> size += amdgpu_dpm_print_clock_levels(adev, OD_CCLK,
>>> buf+size);
>>>
>>>
>> [Yu, Lang]
>> Yes. So it is fine we just fix the caller amdgpu_get_pp_od_clk_voltage like
>following:
>>
>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>> struct device_attribute *attr,
>> char *buf)
>> {
>> struct drm_device *ddev = dev_get_drvdata(dev);
>> struct amdgpu_device *adev = drm_to_adev(ddev);
>> ssize_t size, offset;
>> int ret, i;
>> char temp_buf[512];
>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>
>> if (amdgpu_in_reset(adev))
>> return -EPERM;
>> if (adev->in_suspend && !adev->in_runpm)
>> return -EPERM;
>>
>> ret = pm_runtime_get_sync(ddev->dev);
>> if (ret < 0) {
>> pm_runtime_put_autosuspend(ddev->dev);
>> return ret;
>> }
>>
>> offset = 0;
>>
>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>> size = amdgpu_dpm_print_clock_levels(adev,
>clock_type[i], buf);
>> if (offset + size > PAGE_SIZE)
>> break;
>> memcpy(temp_buf + offset, buf, size);
>> offset += size;
>> }
>> memcpy(buf, temp_buf, offset);
>> size = offset;
>> } else {
>> size = sysfs_emit(buf, "\n");
>> }
>> pm_runtime_mark_last_busy(ddev->dev);
>> pm_runtime_put_autosuspend(ddev->dev);
>>
>> return size;
>> }
>>
>Prefer to avoid any extra stack or heap usage for buffer. Maybe another arg to
>pass offset along with buf?
>
[Yu, Lang]
Actually, the buf address contains the offset(offset_in_page(buf)) .
Or we just rollback to sprintf/snprintf.
Regards,
Lang
>Thanks,
>Lijo
>
>> Regards,
>> Lang
>>
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Signed-off-by: Lang Yu <lang.yu@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 9 +++++++--
>>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 5 ++++-
>>>>> .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 5 ++++-
>>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 15
>>>>> +++++++++------
>>>>> drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 3 +++
>>>>> .../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 13
>>>>> +++++++++----
>>>>> .../gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c | 7 +++++--
>>>>> 7 files changed, 41 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>> index e343cc218990..53185fe96d83 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>> @@ -771,8 +771,13 @@ static int arcturus_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> struct smu_11_0_dpm_context *dpm_context = NULL;
>>>>> uint32_t gen_speed, lane_width;
>>>>> - if (amdgpu_ras_intr_triggered())
>>>>> - return sysfs_emit(buf, "unavailable\n");
>>>>> + size = offset_in_page(buf);
>>>>> + buf = buf - size;
>>>>> +
>>>>> + if (amdgpu_ras_intr_triggered()) {
>>>>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>>>>> + return size;
>>>>> + }
>>>>> dpm_context = smu_dpm->dpm_context; diff --git
>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>> index 4c81989b8162..5490e8e66e14 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>> @@ -1279,6 +1279,9 @@ static int navi10_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> struct smu_11_0_overdrive_table *od_settings =
>>>>> smu->od_settings;
>>>>> uint32_t min_value, max_value;
>>>>> + size = offset_in_page(buf);
>>>>> + buf = buf - size;
>>>>> +
>>>>> switch (clk_type) {
>>>>> case SMU_GFXCLK:
>>>>> case SMU_SCLK:
>>>>> @@ -1392,7 +1395,7 @@ static int navi10_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> case SMU_OD_RANGE:
>>>>> if (!smu->od_enabled || !od_table || !od_settings)
>>>>> break;
>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>> if (navi10_od_feature_is_supported(od_settings,
>>>>> SMU_11_0_ODCAP_GFXCLK_LIMITS)) {
>>>>> navi10_od_setting_get_range(od_settings,
>>>>> SMU_11_0_ODSETTING_GFXCLKFMIN,
>>>>> diff --git
>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>> index 5e292c3f5050..817ad6de3854 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>> @@ -1058,6 +1058,9 @@ static int
>>>>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>>>>> uint32_t min_value, max_value;
>>>>> uint32_t smu_version;
>>>>> + size = offset_in_page(buf);
>>>>> + buf = buf - size;
>>>>> +
>>>>> switch (clk_type) {
>>>>> case SMU_GFXCLK:
>>>>> case SMU_SCLK:
>>>>> @@ -1180,7 +1183,7 @@ static int
>>>>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>>>>> if (!smu->od_enabled || !od_table || !od_settings)
>>>>> break;
>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>> if (sienna_cichlid_is_od_feature_supported(od_settings,
>>>>> SMU_11_0_7_ODCAP_GFXCLK_LIMITS)) {
>>>>> sienna_cichlid_get_od_setting_range(od_settings,
>>>>> SMU_11_0_7_ODSETTING_GFXCLKFMIN,
>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>> index 3a3421452e57..c7842c69b570 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>> @@ -592,7 +592,7 @@ static int
>>>>> vangogh_print_legacy_clk_levels(struct
>>>>> smu_context *smu,
>>>>> switch (clk_type) {
>>>>> case SMU_OD_SCLK:
>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>>> -601,7 +601,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>>>> smu_context *smu,
>>>>> break;
>>>>> case SMU_OD_CCLK:
>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>>>>> smu->cpu_core_id_select);
>>>>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>>>>> Core%d:\n", smu->cpu_core_id_select);
>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>> (smu->cpu_actual_soft_min_freq > 0) ?
>>>>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>>> -610,7 +610,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>>>> smu_context *smu,
>>>>> break;
>>>>> case SMU_OD_RANGE:
>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>>>> %10uMhz\n",
>>>>> smu->gfx_default_hard_min_freq,
>>>>> smu->gfx_default_soft_max_freq);
>>>>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz
>>>>> %10uMhz\n", @@ -682,6 +682,9 @@ static int
>>>>> vangogh_print_clk_levels(struct smu_context *smu,
>>>>> uint32_t cur_value = 0, value = 0, count = 0;
>>>>> bool cur_value_match_level = false;
>>>>> + size = offset_in_page(buf);
>>>>> + buf = buf - size;
>>>>> +
>>>>> memset(&metrics, 0, sizeof(metrics));
>>>>> ret = smu_cmn_get_metrics_table(smu, &metrics, false); @@
>>>>> -691,7 +694,7 @@ static int vangogh_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> switch (clk_type) {
>>>>> case SMU_OD_SCLK:
>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>>> -700,7 +703,7 @@ static int vangogh_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> break;
>>>>> case SMU_OD_CCLK:
>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>>>>> smu->cpu_core_id_select);
>>>>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>>>>> Core%d:\n", smu->cpu_core_id_select);
>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>> (smu->cpu_actual_soft_min_freq > 0) ?
>>>>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>>> -709,7 +712,7 @@ static int vangogh_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> break;
>>>>> case SMU_OD_RANGE:
>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>>>> %10uMhz\n",
>>>>> smu->gfx_default_hard_min_freq,
>>>>> smu->gfx_default_soft_max_freq);
>>>>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz
>>>>> %10uMhz\n", diff --git
>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>> index 5aa175e12a78..86e7978b6d63 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>> @@ -491,6 +491,9 @@ static int renoir_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> struct smu_dpm_context *smu_dpm_ctx = &(smu->smu_dpm);
>>>>> bool cur_value_match_level = false;
>>>>> + size = offset_in_page(buf);
>>>>> + buf = buf - size;
>>>>> +
>>>>> memset(&metrics, 0, sizeof(metrics));
>>>>> ret = smu_cmn_get_metrics_table(smu, &metrics, false); diff
>>>>> --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>> index ab652028e003..6349f27e9efc 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>> @@ -733,15 +733,20 @@ static int aldebaran_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> uint32_t freq_values[3] = {0};
>>>>> uint32_t min_clk, max_clk;
>>>>> - if (amdgpu_ras_intr_triggered())
>>>>> - return sysfs_emit(buf, "unavailable\n");
>>>>> + size = offset_in_page(buf);
>>>>> + buf = buf - size;
>>>>> +
>>>>> + if (amdgpu_ras_intr_triggered()) {
>>>>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>>>>> + return size;
>>>>> + }
>>>>> dpm_context = smu_dpm->dpm_context;
>>>>> switch (type) {
>>>>> case SMU_OD_SCLK:
>>>>> - size = sysfs_emit(buf, "%s:\n", "GFXCLK");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "GFXCLK");
>>>>> fallthrough;
>>>>> case SMU_SCLK:
>>>>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>>>>> SMU_GFXCLK, &now);
>>>>> @@ -795,7 +800,7 @@ static int aldebaran_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> break;
>>>>> case SMU_OD_MCLK:
>>>>> - size = sysfs_emit(buf, "%s:\n", "MCLK");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "MCLK");
>>>>> fallthrough;
>>>>> case SMU_MCLK:
>>>>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>>>>> SMU_UCLK, &now); diff --git
>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>> index 627ba2eec7fd..3b21d9143b96 100644
>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>> @@ -1052,16 +1052,19 @@ static int
>>>>> yellow_carp_print_clk_levels(struct
>>>>> smu_context *smu,
>>>>> int i, size = 0, ret = 0;
>>>>> uint32_t cur_value = 0, value = 0, count = 0;
>>>>> + size = offset_in_page(buf);
>>>>> + buf = buf - size;
>>>>> +
>>>>> switch (clk_type) {
>>>>> case SMU_OD_SCLK:
>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>>>>> (smu->gfx_actual_soft_max_freq > 0) ?
>>>>> smu->gfx_actual_soft_max_freq : smu->gfx_default_soft_max_freq);
>>>>> break;
>>>>> case SMU_OD_RANGE:
>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>>>> %10uMhz\n",
>>>>> smu->gfx_default_hard_min_freq,
>>>>> smu->gfx_default_soft_max_freq);
>>>>> break;
>>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 9:02 ` Yu, Lang
@ 2021-09-08 9:29 ` Lazar, Lijo
2021-09-08 9:38 ` Christian König
0 siblings, 1 reply; 18+ messages in thread
From: Lazar, Lijo @ 2021-09-08 9:29 UTC (permalink / raw)
To: Yu, Lang, Christian König, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao, darren.powell
On 9/8/2021 2:32 PM, Yu, Lang wrote:
> [AMD Official Use Only]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Wednesday, September 8, 2021 4:55 PM
>> To: Yu, Lang <Lang.Yu@amd.com>; Christian König
>> <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>
>>
>>
>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>> [AMD Official Use Only]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>>>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>> warnings
>>>>
>>>>
>>>>
>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>>>>> address. Make them happy!
>>>>>>
>>>>>> Warning Log:
>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>>> sysfs_emit_at+0x4a/0xa0
>>>>>> [ 492.654805] Call Trace:
>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>>> dev_attr_show+0x1d/0x40
>>>>>
>>>>> Mhm, that at least partially doesn't looks like the right approach to me.
>>>>>
>>>>> Why do we have string printing and sysfs code in the hardware
>>>>> version specific backend in the first place?
>>>>>
>>>>
>>>> This is a callback meant for printing ASIC specific information to
>>>> sysfs node. The buffer passed in sysfs read is passed as it is to the callback API.
>>>>
>>>>> That stuff needs to be implemented for each hardware generation and
>>>>> is now cluttered with sysfs buffer offset calculations.
>>>>>
>>>>
>>>> Looks like the warning happened because of this usage.
>>>>
>>>> size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
>>>> size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK,
>>>> buf+size);
>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>> OD_VDDC_CURVE, buf+size);
>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>> OD_VDDGFX_OFFSET, buf+size);
>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>> OD_RANGE,
>>>> buf+size);
>>>> size += amdgpu_dpm_print_clock_levels(adev, OD_CCLK,
>>>> buf+size);
>>>>
>>>>
>>> [Yu, Lang]
>>> Yes. So it is fine we just fix the caller amdgpu_get_pp_od_clk_voltage like
>> following:
>>>
>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>> struct device_attribute *attr,
>>> char *buf)
>>> {
>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>> ssize_t size, offset;
>>> int ret, i;
>>> char temp_buf[512];
>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>
>>> if (amdgpu_in_reset(adev))
>>> return -EPERM;
>>> if (adev->in_suspend && !adev->in_runpm)
>>> return -EPERM;
>>>
>>> ret = pm_runtime_get_sync(ddev->dev);
>>> if (ret < 0) {
>>> pm_runtime_put_autosuspend(ddev->dev);
>>> return ret;
>>> }
>>>
>>> offset = 0;
>>>
>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>> size = amdgpu_dpm_print_clock_levels(adev,
>> clock_type[i], buf);
>>> if (offset + size > PAGE_SIZE)
>>> break;
>>> memcpy(temp_buf + offset, buf, size);
>>> offset += size;
>>> }
>>> memcpy(buf, temp_buf, offset);
>>> size = offset;
>>> } else {
>>> size = sysfs_emit(buf, "\n");
>>> }
>>> pm_runtime_mark_last_busy(ddev->dev);
>>> pm_runtime_put_autosuspend(ddev->dev);
>>>
>>> return size;
>>> }
>>>
>> Prefer to avoid any extra stack or heap usage for buffer. Maybe another arg to
>> pass offset along with buf?
>>
> [Yu, Lang]
> Actually, the buf address contains the offset(offset_in_page(buf)) .
Though it's not a problem based on codeflow, static analysis tools might
complain.
> Or we just rollback to sprintf/snprintf.
>
snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took the
effort to convert these, he may have some other ideas.
Thanks,
Lijo
> Regards,
> Lang
>
>> Thanks,
>> Lijo
>>
>>> Regards,
>>> Lang
>>>
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Lang Yu <lang.yu@amd.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 9 +++++++--
>>>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 5 ++++-
>>>>>> .../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 5 ++++-
>>>>>> drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 15
>>>>>> +++++++++------
>>>>>> drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c | 3 +++
>>>>>> .../gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 13
>>>>>> +++++++++----
>>>>>> .../gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c | 7 +++++--
>>>>>> 7 files changed, 41 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>>> index e343cc218990..53185fe96d83 100644
>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
>>>>>> @@ -771,8 +771,13 @@ static int arcturus_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> struct smu_11_0_dpm_context *dpm_context = NULL;
>>>>>> uint32_t gen_speed, lane_width;
>>>>>> - if (amdgpu_ras_intr_triggered())
>>>>>> - return sysfs_emit(buf, "unavailable\n");
>>>>>> + size = offset_in_page(buf);
>>>>>> + buf = buf - size;
>>>>>> +
>>>>>> + if (amdgpu_ras_intr_triggered()) {
>>>>>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>>>>>> + return size;
>>>>>> + }
>>>>>> dpm_context = smu_dpm->dpm_context; diff --git
>>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>>> index 4c81989b8162..5490e8e66e14 100644
>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>>>>> @@ -1279,6 +1279,9 @@ static int navi10_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> struct smu_11_0_overdrive_table *od_settings =
>>>>>> smu->od_settings;
>>>>>> uint32_t min_value, max_value;
>>>>>> + size = offset_in_page(buf);
>>>>>> + buf = buf - size;
>>>>>> +
>>>>>> switch (clk_type) {
>>>>>> case SMU_GFXCLK:
>>>>>> case SMU_SCLK:
>>>>>> @@ -1392,7 +1395,7 @@ static int navi10_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> case SMU_OD_RANGE:
>>>>>> if (!smu->od_enabled || !od_table || !od_settings)
>>>>>> break;
>>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>>> if (navi10_od_feature_is_supported(od_settings,
>>>>>> SMU_11_0_ODCAP_GFXCLK_LIMITS)) {
>>>>>> navi10_od_setting_get_range(od_settings,
>>>>>> SMU_11_0_ODSETTING_GFXCLKFMIN,
>>>>>> diff --git
>>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>>> index 5e292c3f5050..817ad6de3854 100644
>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c
>>>>>> @@ -1058,6 +1058,9 @@ static int
>>>>>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>>>>>> uint32_t min_value, max_value;
>>>>>> uint32_t smu_version;
>>>>>> + size = offset_in_page(buf);
>>>>>> + buf = buf - size;
>>>>>> +
>>>>>> switch (clk_type) {
>>>>>> case SMU_GFXCLK:
>>>>>> case SMU_SCLK:
>>>>>> @@ -1180,7 +1183,7 @@ static int
>>>>>> sienna_cichlid_print_clk_levels(struct smu_context *smu,
>>>>>> if (!smu->od_enabled || !od_table || !od_settings)
>>>>>> break;
>>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>>> if (sienna_cichlid_is_od_feature_supported(od_settings,
>>>>>> SMU_11_0_7_ODCAP_GFXCLK_LIMITS)) {
>>>>>> sienna_cichlid_get_od_setting_range(od_settings,
>>>>>> SMU_11_0_7_ODSETTING_GFXCLKFMIN,
>>>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>>> index 3a3421452e57..c7842c69b570 100644
>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
>>>>>> @@ -592,7 +592,7 @@ static int
>>>>>> vangogh_print_legacy_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> switch (clk_type) {
>>>>>> case SMU_OD_SCLK:
>>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>>>> -601,7 +601,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> break;
>>>>>> case SMU_OD_CCLK:
>>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>>>>>> smu->cpu_core_id_select);
>>>>>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>>>>>> Core%d:\n", smu->cpu_core_id_select);
>>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>>> (smu->cpu_actual_soft_min_freq > 0) ?
>>>>>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>>>> -610,7 +610,7 @@ static int vangogh_print_legacy_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> break;
>>>>>> case SMU_OD_RANGE:
>>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>>>>> %10uMhz\n",
>>>>>> smu->gfx_default_hard_min_freq,
>>>>>> smu->gfx_default_soft_max_freq);
>>>>>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz
>>>>>> %10uMhz\n", @@ -682,6 +682,9 @@ static int
>>>>>> vangogh_print_clk_levels(struct smu_context *smu,
>>>>>> uint32_t cur_value = 0, value = 0, count = 0;
>>>>>> bool cur_value_match_level = false;
>>>>>> + size = offset_in_page(buf);
>>>>>> + buf = buf - size;
>>>>>> +
>>>>>> memset(&metrics, 0, sizeof(metrics));
>>>>>> ret = smu_cmn_get_metrics_table(smu, &metrics, false); @@
>>>>>> -691,7 +694,7 @@ static int vangogh_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> switch (clk_type) {
>>>>>> case SMU_OD_SCLK:
>>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>>>> -700,7 +703,7 @@ static int vangogh_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> break;
>>>>>> case SMU_OD_CCLK:
>>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>>> - size = sysfs_emit(buf, "CCLK_RANGE in Core%d:\n",
>>>>>> smu->cpu_core_id_select);
>>>>>> + size += sysfs_emit_at(buf, size, "CCLK_RANGE in
>>>>>> Core%d:\n", smu->cpu_core_id_select);
>>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>>> (smu->cpu_actual_soft_min_freq > 0) ?
>>>>>> smu->cpu_actual_soft_min_freq : smu->cpu_default_soft_min_freq);
>>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n", @@
>>>>>> -709,7 +712,7 @@ static int vangogh_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> break;
>>>>>> case SMU_OD_RANGE:
>>>>>> if (smu_dpm_ctx->dpm_level ==
>>>>>> AMD_DPM_FORCED_LEVEL_MANUAL) {
>>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>>>>> %10uMhz\n",
>>>>>> smu->gfx_default_hard_min_freq,
>>>>>> smu->gfx_default_soft_max_freq);
>>>>>> size += sysfs_emit_at(buf, size, "CCLK: %7uMhz
>>>>>> %10uMhz\n", diff --git
>>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>>> index 5aa175e12a78..86e7978b6d63 100644
>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu12/renoir_ppt.c
>>>>>> @@ -491,6 +491,9 @@ static int renoir_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> struct smu_dpm_context *smu_dpm_ctx = &(smu->smu_dpm);
>>>>>> bool cur_value_match_level = false;
>>>>>> + size = offset_in_page(buf);
>>>>>> + buf = buf - size;
>>>>>> +
>>>>>> memset(&metrics, 0, sizeof(metrics));
>>>>>> ret = smu_cmn_get_metrics_table(smu, &metrics, false); diff
>>>>>> --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>>> index ab652028e003..6349f27e9efc 100644
>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
>>>>>> @@ -733,15 +733,20 @@ static int aldebaran_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> uint32_t freq_values[3] = {0};
>>>>>> uint32_t min_clk, max_clk;
>>>>>> - if (amdgpu_ras_intr_triggered())
>>>>>> - return sysfs_emit(buf, "unavailable\n");
>>>>>> + size = offset_in_page(buf);
>>>>>> + buf = buf - size;
>>>>>> +
>>>>>> + if (amdgpu_ras_intr_triggered()) {
>>>>>> + size += sysfs_emit_at(buf, size, "unavailable\n");
>>>>>> + return size;
>>>>>> + }
>>>>>> dpm_context = smu_dpm->dpm_context;
>>>>>> switch (type) {
>>>>>> case SMU_OD_SCLK:
>>>>>> - size = sysfs_emit(buf, "%s:\n", "GFXCLK");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "GFXCLK");
>>>>>> fallthrough;
>>>>>> case SMU_SCLK:
>>>>>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>>>>>> SMU_GFXCLK, &now);
>>>>>> @@ -795,7 +800,7 @@ static int aldebaran_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> break;
>>>>>> case SMU_OD_MCLK:
>>>>>> - size = sysfs_emit(buf, "%s:\n", "MCLK");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "MCLK");
>>>>>> fallthrough;
>>>>>> case SMU_MCLK:
>>>>>> ret = aldebaran_get_current_clk_freq_by_table(smu,
>>>>>> SMU_UCLK, &now); diff --git
>>>>>> a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>>> b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>>> index 627ba2eec7fd..3b21d9143b96 100644
>>>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/yellow_carp_ppt.c
>>>>>> @@ -1052,16 +1052,19 @@ static int
>>>>>> yellow_carp_print_clk_levels(struct
>>>>>> smu_context *smu,
>>>>>> int i, size = 0, ret = 0;
>>>>>> uint32_t cur_value = 0, value = 0, count = 0;
>>>>>> + size = offset_in_page(buf);
>>>>>> + buf = buf - size;
>>>>>> +
>>>>>> switch (clk_type) {
>>>>>> case SMU_OD_SCLK:
>>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
>>>>>> size += sysfs_emit_at(buf, size, "0: %10uMhz\n",
>>>>>> (smu->gfx_actual_hard_min_freq > 0) ?
>>>>>> smu->gfx_actual_hard_min_freq : smu->gfx_default_hard_min_freq);
>>>>>> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
>>>>>> (smu->gfx_actual_soft_max_freq > 0) ?
>>>>>> smu->gfx_actual_soft_max_freq : smu->gfx_default_soft_max_freq);
>>>>>> break;
>>>>>> case SMU_OD_RANGE:
>>>>>> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
>>>>>> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>>>>>> size += sysfs_emit_at(buf, size, "SCLK: %7uMhz
>>>>>> %10uMhz\n",
>>>>>> smu->gfx_default_hard_min_freq,
>>>>>> smu->gfx_default_soft_max_freq);
>>>>>> break;
>>>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 9:29 ` Lazar, Lijo
@ 2021-09-08 9:38 ` Christian König
2021-09-08 10:22 ` Lazar, Lijo
0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2021-09-08 9:38 UTC (permalink / raw)
To: Lazar, Lijo, Yu, Lang, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao, darren.powell
Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>
>
> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>> [AMD Official Use Only]
>>
>>
>>
>>> -----Original Message-----
>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>> To: Yu, Lang <Lang.Yu@amd.com>; Christian König
>>> <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>>
>>>
>>>
>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>>> [AMD Official Use Only]
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>>>>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>> warnings
>>>>>
>>>>>
>>>>>
>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>>>>>> address. Make them happy!
>>>>>>>
>>>>>>> Warning Log:
>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>>>> sysfs_emit_at+0x4a/0xa0
>>>>>>> [ 492.654805] Call Trace:
>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>>>> dev_attr_show+0x1d/0x40
>>>>>>
>>>>>> Mhm, that at least partially doesn't looks like the right
>>>>>> approach to me.
>>>>>>
>>>>>> Why do we have string printing and sysfs code in the hardware
>>>>>> version specific backend in the first place?
>>>>>>
>>>>>
>>>>> This is a callback meant for printing ASIC specific information to
>>>>> sysfs node. The buffer passed in sysfs read is passed as it is to
>>>>> the callback API.
>>>>>
>>>>>> That stuff needs to be implemented for each hardware generation and
>>>>>> is now cluttered with sysfs buffer offset calculations.
>>>>>>
>>>>>
>>>>> Looks like the warning happened because of this usage.
>>>>>
>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>> OD_SCLK, buf);
>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>> OD_MCLK,
>>>>> buf+size);
>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>> OD_VDDC_CURVE, buf+size);
>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>> OD_VDDGFX_OFFSET, buf+size);
>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>> OD_RANGE,
>>>>> buf+size);
>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>> OD_CCLK,
>>>>> buf+size);
>>>>>
>>>>>
>>>> [Yu, Lang]
>>>> Yes. So it is fine we just fix the caller
>>>> amdgpu_get_pp_od_clk_voltage like
>>> following:
>>>>
>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>>> struct device_attribute *attr,
>>>> char *buf)
>>>> {
>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>>> ssize_t size, offset;
>>>> int ret, i;
>>>> char temp_buf[512];
>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>>
>>>> if (amdgpu_in_reset(adev))
>>>> return -EPERM;
>>>> if (adev->in_suspend && !adev->in_runpm)
>>>> return -EPERM;
>>>>
>>>> ret = pm_runtime_get_sync(ddev->dev);
>>>> if (ret < 0) {
>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>> return ret;
>>>> }
>>>>
>>>> offset = 0;
>>>>
>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>> clock_type[i], buf);
>>>> if (offset + size > PAGE_SIZE)
>>>> break;
>>>> memcpy(temp_buf + offset, buf, size);
>>>> offset += size;
>>>> }
>>>> memcpy(buf, temp_buf, offset);
>>>> size = offset;
>>>> } else {
>>>> size = sysfs_emit(buf, "\n");
>>>> }
>>>> pm_runtime_mark_last_busy(ddev->dev);
>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>
>>>> return size;
>>>> }
>>>>
>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>> another arg to
>>> pass offset along with buf?
>>>
>> [Yu, Lang]
>> Actually, the buf address contains the offset(offset_in_page(buf)) .
>
> Though it's not a problem based on codeflow, static analysis tools
> might complain.
>
>> Or we just rollback to sprintf/snprintf.
>>
>
> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took the
> effort to convert these, he may have some other ideas.
This is not what I meant. See from the design point of view the
print_clock_levels() callback is the bad idea to begin with.
What we should have instead is a callback which returns the clock as a
value which is then printed in the amdgpu_get_pp_od_clk_voltage() function.
This avoids passing around the buffer and remaining size everywhere and
also guarantees that the sysfs have unified printing over all hardware
generations.
Regards,
Christian.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 9:38 ` Christian König
@ 2021-09-08 10:22 ` Lazar, Lijo
2021-09-08 10:55 ` Yu, Lang
2021-09-08 12:43 ` Christian König
0 siblings, 2 replies; 18+ messages in thread
From: Lazar, Lijo @ 2021-09-08 10:22 UTC (permalink / raw)
To: Christian König, Yu, Lang, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao, darren.powell
On 9/8/2021 3:08 PM, Christian König wrote:
> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>>
>>
>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>>> [AMD Official Use Only]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>>> To: Yu, Lang <Lang.Yu@amd.com>; Christian König
>>>> <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>>>
>>>>
>>>>
>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>>>> [AMD Official Use Only]
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>>>>>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>>> warnings
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>>>>>>> address. Make them happy!
>>>>>>>>
>>>>>>>> Warning Log:
>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>>>>> sysfs_emit_at+0x4a/0xa0
>>>>>>>> [ 492.654805] Call Trace:
>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>>>>> dev_attr_show+0x1d/0x40
>>>>>>>
>>>>>>> Mhm, that at least partially doesn't looks like the right
>>>>>>> approach to me.
>>>>>>>
>>>>>>> Why do we have string printing and sysfs code in the hardware
>>>>>>> version specific backend in the first place?
>>>>>>>
>>>>>>
>>>>>> This is a callback meant for printing ASIC specific information to
>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is to
>>>>>> the callback API.
>>>>>>
>>>>>>> That stuff needs to be implemented for each hardware generation and
>>>>>>> is now cluttered with sysfs buffer offset calculations.
>>>>>>>
>>>>>>
>>>>>> Looks like the warning happened because of this usage.
>>>>>>
>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>>> OD_SCLK, buf);
>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>> OD_MCLK,
>>>>>> buf+size);
>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>> OD_VDDC_CURVE, buf+size);
>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>> OD_VDDGFX_OFFSET, buf+size);
>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>> OD_RANGE,
>>>>>> buf+size);
>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>> OD_CCLK,
>>>>>> buf+size);
>>>>>>
>>>>>>
>>>>> [Yu, Lang]
>>>>> Yes. So it is fine we just fix the caller
>>>>> amdgpu_get_pp_od_clk_voltage like
>>>> following:
>>>>>
>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>>>> struct device_attribute *attr,
>>>>> char *buf)
>>>>> {
>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>>>> ssize_t size, offset;
>>>>> int ret, i;
>>>>> char temp_buf[512];
>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>>>
>>>>> if (amdgpu_in_reset(adev))
>>>>> return -EPERM;
>>>>> if (adev->in_suspend && !adev->in_runpm)
>>>>> return -EPERM;
>>>>>
>>>>> ret = pm_runtime_get_sync(ddev->dev);
>>>>> if (ret < 0) {
>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>> return ret;
>>>>> }
>>>>>
>>>>> offset = 0;
>>>>>
>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>> clock_type[i], buf);
>>>>> if (offset + size > PAGE_SIZE)
>>>>> break;
>>>>> memcpy(temp_buf + offset, buf, size);
>>>>> offset += size;
>>>>> }
>>>>> memcpy(buf, temp_buf, offset);
>>>>> size = offset;
>>>>> } else {
>>>>> size = sysfs_emit(buf, "\n");
>>>>> }
>>>>> pm_runtime_mark_last_busy(ddev->dev);
>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>
>>>>> return size;
>>>>> }
>>>>>
>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>>> another arg to
>>>> pass offset along with buf?
>>>>
>>> [Yu, Lang]
>>> Actually, the buf address contains the offset(offset_in_page(buf)) .
>>
>> Though it's not a problem based on codeflow, static analysis tools
>> might complain.
>>
>>> Or we just rollback to sprintf/snprintf.
>>>
>>
>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took the
>> effort to convert these, he may have some other ideas.
>
> This is not what I meant. See from the design point of view the
> print_clock_levels() callback is the bad idea to begin with.
>
> What we should have instead is a callback which returns the clock as a
> value which is then printed in the amdgpu_get_pp_od_clk_voltage() function.
>
> This avoids passing around the buffer and remaining size everywhere and
> also guarantees that the sysfs have unified printing over all hardware
> generations.
>
The scenario is one node used for multiple parameters - OD_SCLK,
OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with
lots of nodes). On top of it, the parameters supported (for ex: CCLK is
not valid on dGPUs), the number of levels supported etc. vary across
ASICs. There has to be multiple calls or the call needs to return
multiple values for a single parameter (for ex: up to 4, 8 or 16 levels
of GFXCLK depending on ASIC).
I don't know the history of the callback, mostly it was considered more
efficient to print it directly rather than fetch and print. Alex/Evan
may know the details.
Thanks,
Lijo
> Regards,
> Christian.
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 10:22 ` Lazar, Lijo
@ 2021-09-08 10:55 ` Yu, Lang
2021-09-08 12:40 ` Christian König
2021-09-08 12:43 ` Christian König
1 sibling, 1 reply; 18+ messages in thread
From: Yu, Lang @ 2021-09-08 10:55 UTC (permalink / raw)
To: Lazar, Lijo, Christian König, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao, Powell, Darren
[AMD Official Use Only]
Or try to send a patch to remove page boundary aligned limitation. Any considerations? Thanks.
int sysfs_emit(char *buf, const char *fmt, ...)
{
va_list args;
- int len;
+ int len, offset;
- if (WARN(!buf || offset_in_page(buf),
+ offset = offset_in_page(buf);
+
+ if (WARN(!buf,
"invalid sysfs_emit: buf:%p\n", buf))
return 0;
va_start(args, fmt);
- len = vscnprintf(buf, PAGE_SIZE, fmt, args);
+ len = vscnprintf(buf, PAGE_SIZE - offset, fmt, args);
va_end(args);
return len;
@@ -760,14 +762,16 @@ EXPORT_SYMBOL_GPL(sysfs_emit);
int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
{
va_list args;
- int len;
+ int len, offset;
+
+ offset = offset_in_page(buf);
- if (WARN(!buf || offset_in_page(buf) || at < 0 || at >= PAGE_SIZE,
+ if (WARN(!buf || at < 0 || at + offset >= PAGE_SIZE,
"invalid sysfs_emit_at: buf:%p at:%d\n", buf, at))
return 0;
va_start(args, fmt);
- len = vscnprintf(buf + at, PAGE_SIZE - at, fmt, args);
+ len = vscnprintf(buf + at, PAGE_SIZE - at - offset, fmt, args);
va_end(args);
return len;
Regards,
Lang
>-----Original Message-----
>From: Lazar, Lijo <Lijo.Lazar@amd.com>
>Sent: Wednesday, September 8, 2021 6:22 PM
>To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
><Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
><Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>; Powell, Darren
><Darren.Powell@amd.com>
>Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>
>
>
>On 9/8/2021 3:08 PM, Christian König wrote:
>> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>>>
>>>
>>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>>>> [AMD Official Use Only]
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>>>> To: Yu, Lang <Lang.Yu@amd.com>; Christian König
>>>>> <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>> warnings
>>>>>
>>>>>
>>>>>
>>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>>>>> [AMD Official Use Only]
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>>>>>>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>>>> warnings
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned
>>>>>>>>> buf address. Make them happy!
>>>>>>>>>
>>>>>>>>> Warning Log:
>>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0
>>>>>>>>> [ 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>>>>>> sysfs_emit_at+0x4a/0xa0
>>>>>>>>> [ 492.654805] Call Trace:
>>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu]
>>>>>>>>> [ 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu]
>>>>>>>>> [ 492.659733] ? preempt_schedule_common+0x18/0x30 [
>>>>>>>>> 492.660713]
>>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>>>>>> dev_attr_show+0x1d/0x40
>>>>>>>>
>>>>>>>> Mhm, that at least partially doesn't looks like the right
>>>>>>>> approach to me.
>>>>>>>>
>>>>>>>> Why do we have string printing and sysfs code in the hardware
>>>>>>>> version specific backend in the first place?
>>>>>>>>
>>>>>>>
>>>>>>> This is a callback meant for printing ASIC specific information
>>>>>>> to sysfs node. The buffer passed in sysfs read is passed as it is
>>>>>>> to the callback API.
>>>>>>>
>>>>>>>> That stuff needs to be implemented for each hardware generation
>>>>>>>> and is now cluttered with sysfs buffer offset calculations.
>>>>>>>>
>>>>>>>
>>>>>>> Looks like the warning happened because of this usage.
>>>>>>>
>>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_SCLK, buf);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_MCLK,
>>>>>>> buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_VDDC_CURVE, buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_VDDGFX_OFFSET, buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_RANGE,
>>>>>>> buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_CCLK,
>>>>>>> buf+size);
>>>>>>>
>>>>>>>
>>>>>> [Yu, Lang]
>>>>>> Yes. So it is fine we just fix the caller
>>>>>> amdgpu_get_pp_od_clk_voltage like
>>>>> following:
>>>>>>
>>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>>>>> struct device_attribute *attr,
>>>>>> char *buf)
>>>>>> {
>>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>>>>> ssize_t size, offset;
>>>>>> int ret, i;
>>>>>> char temp_buf[512];
>>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>>>>
>>>>>> if (amdgpu_in_reset(adev))
>>>>>> return -EPERM;
>>>>>> if (adev->in_suspend && !adev->in_runpm)
>>>>>> return -EPERM;
>>>>>>
>>>>>> ret = pm_runtime_get_sync(ddev->dev);
>>>>>> if (ret < 0) {
>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> offset = 0;
>>>>>>
>>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>> clock_type[i], buf);
>>>>>> if (offset + size > PAGE_SIZE)
>>>>>> break;
>>>>>> memcpy(temp_buf + offset, buf, size);
>>>>>> offset += size;
>>>>>> }
>>>>>> memcpy(buf, temp_buf, offset);
>>>>>> size = offset;
>>>>>> } else {
>>>>>> size = sysfs_emit(buf, "\n");
>>>>>> }
>>>>>> pm_runtime_mark_last_busy(ddev->dev);
>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>>
>>>>>> return size;
>>>>>> }
>>>>>>
>>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>>>> another arg to pass offset along with buf?
>>>>>
>>>> [Yu, Lang]
>>>> Actually, the buf address contains the offset(offset_in_page(buf)) .
>>>
>>> Though it's not a problem based on codeflow, static analysis tools
>>> might complain.
>>>
>>>> Or we just rollback to sprintf/snprintf.
>>>>
>>>
>>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
>>> the effort to convert these, he may have some other ideas.
>>
>> This is not what I meant. See from the design point of view the
>> print_clock_levels() callback is the bad idea to begin with.
>>
>> What we should have instead is a callback which returns the clock as a
>> value which is then printed in the amdgpu_get_pp_od_clk_voltage() function.
>>
>> This avoids passing around the buffer and remaining size everywhere
>> and also guarantees that the sysfs have unified printing over all
>> hardware generations.
>>
>
>The scenario is one node used for multiple parameters - OD_SCLK, OD_CCLK,
>OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with lots of nodes). On
>top of it, the parameters supported (for ex: CCLK is not valid on dGPUs), the
>number of levels supported etc. vary across ASICs. There has to be multiple calls
>or the call needs to return multiple values for a single parameter (for ex: up to 4,
>8 or 16 levels of GFXCLK depending on ASIC).
>
>I don't know the history of the callback, mostly it was considered more efficient
>to print it directly rather than fetch and print. Alex/Evan may know the details.
>
>Thanks,
>Lijo
>
>> Regards,
>> Christian.
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 10:55 ` Yu, Lang
@ 2021-09-08 12:40 ` Christian König
0 siblings, 0 replies; 18+ messages in thread
From: Christian König @ 2021-09-08 12:40 UTC (permalink / raw)
To: Yu, Lang, Lazar, Lijo, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao, Powell, Darren
Well that's complete nonsense, in that case we should rather go back to
the s*printf functionality.
The problem is that we are not using the sysfs_emit/sysfs_emit_at as
intended, e.g. something like this:
offs = sysfs_emit(page, "first...);
offs += sysfs_emit_at(page, offs, "second....);
offs += sysfs_emit_at(page, offs, "third....);
...
This is usually done inside the same function and not spread around in
the driver.
Regards,
Christian.
Am 08.09.21 um 12:55 schrieb Yu, Lang:
> [AMD Official Use Only]
>
> Or try to send a patch to remove page boundary aligned limitation. Any considerations? Thanks.
>
> int sysfs_emit(char *buf, const char *fmt, ...)
> {
> va_list args;
> - int len;
> + int len, offset;
>
> - if (WARN(!buf || offset_in_page(buf),
> + offset = offset_in_page(buf);
> +
> + if (WARN(!buf,
> "invalid sysfs_emit: buf:%p\n", buf))
> return 0;
>
> va_start(args, fmt);
> - len = vscnprintf(buf, PAGE_SIZE, fmt, args);
> + len = vscnprintf(buf, PAGE_SIZE - offset, fmt, args);
> va_end(args);
>
> return len;
> @@ -760,14 +762,16 @@ EXPORT_SYMBOL_GPL(sysfs_emit);
> int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
> {
> va_list args;
> - int len;
> + int len, offset;
> +
> + offset = offset_in_page(buf);
>
> - if (WARN(!buf || offset_in_page(buf) || at < 0 || at >= PAGE_SIZE,
> + if (WARN(!buf || at < 0 || at + offset >= PAGE_SIZE,
> "invalid sysfs_emit_at: buf:%p at:%d\n", buf, at))
> return 0;
>
> va_start(args, fmt);
> - len = vscnprintf(buf + at, PAGE_SIZE - at, fmt, args);
> + len = vscnprintf(buf + at, PAGE_SIZE - at - offset, fmt, args);
> va_end(args);
>
> return len;
>
> Regards,
> Lang
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Wednesday, September 8, 2021 6:22 PM
>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>; Powell, Darren
>> <Darren.Powell@amd.com>
>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>
>>
>>
>> On 9/8/2021 3:08 PM, Christian König wrote:
>>> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>>>>
>>>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>>>>> [AMD Official Use Only]
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>>>>> To: Yu, Lang <Lang.Yu@amd.com>; Christian König
>>>>>> <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>>> warnings
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>>>>>> [AMD Official Use Only]
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>>>>>>>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>>>>> warnings
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned
>>>>>>>>>> buf address. Make them happy!
>>>>>>>>>>
>>>>>>>>>> Warning Log:
>>>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0
>>>>>>>>>> [ 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>>>>>>> sysfs_emit_at+0x4a/0xa0
>>>>>>>>>> [ 492.654805] Call Trace:
>>>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu]
>>>>>>>>>> [ 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu]
>>>>>>>>>> [ 492.659733] ? preempt_schedule_common+0x18/0x30 [
>>>>>>>>>> 492.660713]
>>>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>>>>>>> dev_attr_show+0x1d/0x40
>>>>>>>>> Mhm, that at least partially doesn't looks like the right
>>>>>>>>> approach to me.
>>>>>>>>>
>>>>>>>>> Why do we have string printing and sysfs code in the hardware
>>>>>>>>> version specific backend in the first place?
>>>>>>>>>
>>>>>>>> This is a callback meant for printing ASIC specific information
>>>>>>>> to sysfs node. The buffer passed in sysfs read is passed as it is
>>>>>>>> to the callback API.
>>>>>>>>
>>>>>>>>> That stuff needs to be implemented for each hardware generation
>>>>>>>>> and is now cluttered with sysfs buffer offset calculations.
>>>>>>>>>
>>>>>>>> Looks like the warning happened because of this usage.
>>>>>>>>
>>>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>>>>> OD_SCLK, buf);
>>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>>> OD_MCLK,
>>>>>>>> buf+size);
>>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>>> OD_VDDC_CURVE, buf+size);
>>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>>> OD_VDDGFX_OFFSET, buf+size);
>>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>>> OD_RANGE,
>>>>>>>> buf+size);
>>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>>> OD_CCLK,
>>>>>>>> buf+size);
>>>>>>>>
>>>>>>>>
>>>>>>> [Yu, Lang]
>>>>>>> Yes. So it is fine we just fix the caller
>>>>>>> amdgpu_get_pp_od_clk_voltage like
>>>>>> following:
>>>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>>>>>> struct device_attribute *attr,
>>>>>>> char *buf)
>>>>>>> {
>>>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>>>>>> ssize_t size, offset;
>>>>>>> int ret, i;
>>>>>>> char temp_buf[512];
>>>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>>>>>
>>>>>>> if (amdgpu_in_reset(adev))
>>>>>>> return -EPERM;
>>>>>>> if (adev->in_suspend && !adev->in_runpm)
>>>>>>> return -EPERM;
>>>>>>>
>>>>>>> ret = pm_runtime_get_sync(ddev->dev);
>>>>>>> if (ret < 0) {
>>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>>> return ret;
>>>>>>> }
>>>>>>>
>>>>>>> offset = 0;
>>>>>>>
>>>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>>> clock_type[i], buf);
>>>>>>> if (offset + size > PAGE_SIZE)
>>>>>>> break;
>>>>>>> memcpy(temp_buf + offset, buf, size);
>>>>>>> offset += size;
>>>>>>> }
>>>>>>> memcpy(buf, temp_buf, offset);
>>>>>>> size = offset;
>>>>>>> } else {
>>>>>>> size = sysfs_emit(buf, "\n");
>>>>>>> }
>>>>>>> pm_runtime_mark_last_busy(ddev->dev);
>>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>>>
>>>>>>> return size;
>>>>>>> }
>>>>>>>
>>>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>>>>> another arg to pass offset along with buf?
>>>>>>
>>>>> [Yu, Lang]
>>>>> Actually, the buf address contains the offset(offset_in_page(buf)) .
>>>> Though it's not a problem based on codeflow, static analysis tools
>>>> might complain.
>>>>
>>>>> Or we just rollback to sprintf/snprintf.
>>>>>
>>>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
>>>> the effort to convert these, he may have some other ideas.
>>> This is not what I meant. See from the design point of view the
>>> print_clock_levels() callback is the bad idea to begin with.
>>>
>>> What we should have instead is a callback which returns the clock as a
>>> value which is then printed in the amdgpu_get_pp_od_clk_voltage() function.
>>>
>>> This avoids passing around the buffer and remaining size everywhere
>>> and also guarantees that the sysfs have unified printing over all
>>> hardware generations.
>>>
>> The scenario is one node used for multiple parameters - OD_SCLK, OD_CCLK,
>> OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with lots of nodes). On
>> top of it, the parameters supported (for ex: CCLK is not valid on dGPUs), the
>> number of levels supported etc. vary across ASICs. There has to be multiple calls
>> or the call needs to return multiple values for a single parameter (for ex: up to 4,
>> 8 or 16 levels of GFXCLK depending on ASIC).
>>
>> I don't know the history of the callback, mostly it was considered more efficient
>> to print it directly rather than fetch and print. Alex/Evan may know the details.
>>
>> Thanks,
>> Lijo
>>
>>> Regards,
>>> Christian.
>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 10:22 ` Lazar, Lijo
2021-09-08 10:55 ` Yu, Lang
@ 2021-09-08 12:43 ` Christian König
2021-09-08 22:17 ` Powell, Darren
1 sibling, 1 reply; 18+ messages in thread
From: Christian König @ 2021-09-08 12:43 UTC (permalink / raw)
To: Lazar, Lijo, Yu, Lang, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao, darren.powell
Am 08.09.21 um 12:22 schrieb Lazar, Lijo:
> On 9/8/2021 3:08 PM, Christian König wrote:
>> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>>>> [AMD Official Use Only]
>>>>> -----Original Message-----
>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>>>> To: Yu, Lang <Lang.Yu@amd.com>; Christian König
>>>>> <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>> warnings
>>>>>
>>>>>
>>>>>
>>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>>>>> [AMD Official Use Only]
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>>>>>>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>>>> warnings
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>>>>>>>> address. Make them happy!
>>>>>>>>>
>>>>>>>>> Warning Log:
>>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>>>>>> sysfs_emit_at+0x4a/0xa0
>>>>>>>>> [ 492.654805] Call Trace:
>>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>>>>>> dev_attr_show+0x1d/0x40
>>>>>>>>
>>>>>>>> Mhm, that at least partially doesn't looks like the right
>>>>>>>> approach to me.
>>>>>>>>
>>>>>>>> Why do we have string printing and sysfs code in the hardware
>>>>>>>> version specific backend in the first place?
>>>>>>>>
>>>>>>>
>>>>>>> This is a callback meant for printing ASIC specific information to
>>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is
>>>>>>> to the callback API.
>>>>>>>
>>>>>>>> That stuff needs to be implemented for each hardware generation
>>>>>>>> and
>>>>>>>> is now cluttered with sysfs buffer offset calculations.
>>>>>>>>
>>>>>>>
>>>>>>> Looks like the warning happened because of this usage.
>>>>>>>
>>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_SCLK, buf);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_MCLK,
>>>>>>> buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_VDDC_CURVE, buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_VDDGFX_OFFSET, buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_RANGE,
>>>>>>> buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_CCLK,
>>>>>>> buf+size);
>>>>>>>
>>>>>>>
>>>>>> [Yu, Lang]
>>>>>> Yes. So it is fine we just fix the caller
>>>>>> amdgpu_get_pp_od_clk_voltage like
>>>>> following:
>>>>>>
>>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>>>>> struct device_attribute *attr,
>>>>>> char *buf)
>>>>>> {
>>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>>>>> ssize_t size, offset;
>>>>>> int ret, i;
>>>>>> char temp_buf[512];
>>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>>>>
>>>>>> if (amdgpu_in_reset(adev))
>>>>>> return -EPERM;
>>>>>> if (adev->in_suspend && !adev->in_runpm)
>>>>>> return -EPERM;
>>>>>>
>>>>>> ret = pm_runtime_get_sync(ddev->dev);
>>>>>> if (ret < 0) {
>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> offset = 0;
>>>>>>
>>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>> clock_type[i], buf);
>>>>>> if (offset + size > PAGE_SIZE)
>>>>>> break;
>>>>>> memcpy(temp_buf + offset, buf, size);
>>>>>> offset += size;
>>>>>> }
>>>>>> memcpy(buf, temp_buf, offset);
>>>>>> size = offset;
>>>>>> } else {
>>>>>> size = sysfs_emit(buf, "\n");
>>>>>> }
>>>>>> pm_runtime_mark_last_busy(ddev->dev);
>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>>
>>>>>> return size;
>>>>>> }
>>>>>>
>>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>>>> another arg to
>>>>> pass offset along with buf?
>>>>>
>>>> [Yu, Lang]
>>>> Actually, the buf address contains the offset(offset_in_page(buf)) .
>>>
>>> Though it's not a problem based on codeflow, static analysis tools
>>> might complain.
>>>
>>>> Or we just rollback to sprintf/snprintf.
>>>>
>>>
>>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
>>> the effort to convert these, he may have some other ideas.
>>
>> This is not what I meant. See from the design point of view the
>> print_clock_levels() callback is the bad idea to begin with.
>>
>> What we should have instead is a callback which returns the clock as
>> a value which is then printed in the amdgpu_get_pp_od_clk_voltage()
>> function.
>>
>> This avoids passing around the buffer and remaining size everywhere
>> and also guarantees that the sysfs have unified printing over all
>> hardware generations.
>>
>
> The scenario is one node used for multiple parameters - OD_SCLK,
> OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with
> lots of nodes). On top of it, the parameters supported (for ex: CCLK
> is not valid on dGPUs), the number of levels supported etc. vary
> across ASICs. There has to be multiple calls or the call needs to
> return multiple values for a single parameter (for ex: up to 4, 8 or
> 16 levels of GFXCLK depending on ASIC).
Well exactly that is questionable design for sysfs.
See the sysfs_emit() and sysfs_emit_at() functions are designed the way
they are because you should have only one value per file, which is then
printed at exactly one location.
Take a look at the documentation for sysfs for more details.
> I don't know the history of the callback, mostly it was considered
> more efficient to print it directly rather than fetch and print.
> Alex/Evan may know the details.
Yeah, somebody with a bit more background in power management needs to
take a closer look at this here. Just keep me looped in.
Regards,
Christian.
>
> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 12:43 ` Christian König
@ 2021-09-08 22:17 ` Powell, Darren
2021-09-09 2:43 ` Yu, Lang
0 siblings, 1 reply; 18+ messages in thread
From: Powell, Darren @ 2021-09-08 22:17 UTC (permalink / raw)
To: Christian König, Lazar, Lijo, Yu, Lang, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
[-- Attachment #1: Type: text/plain, Size: 8423 bytes --]
[AMD Official Use Only]
________________________________
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Wednesday, September 8, 2021 8:43 AM
To: Lazar, Lijo <Lijo.Lazar@amd.com>; Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>; Powell, Darren <Darren.Powell@amd.com>
Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
Am 08.09.21 um 12:22 schrieb Lazar, Lijo:
> On 9/8/2021 3:08 PM, Christian König wrote:
>> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>>>> [AMD Official Use Only]
>>>>> -----Original Message-----
>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>>>> To: Yu, Lang <Lang.Yu@amd.com>; Christian König
>>>>> <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org
>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>> warnings
>>>>>
>>>>>
>>>>>
>>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>>>>> [AMD Official Use Only]
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com>; Yu, Lang
>>>>>>> <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
>>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>>>> warnings
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>>>>>>>> address. Make them happy!
>>>>>>>>>
>>>>>>>>> Warning Log:
>>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>>>>>> sysfs_emit_at+0x4a/0xa0
>>>>>>>>> [ 492.654805] Call Trace:
>>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>>>>>> dev_attr_show+0x1d/0x40
>>>>>>>>
>>>>>>>> Mhm, that at least partially doesn't looks like the right
>>>>>>>> approach to me.
>>>>>>>>
>>>>>>>> Why do we have string printing and sysfs code in the hardware
>>>>>>>> version specific backend in the first place?
>>>>>>>>
>>>>>>>
>>>>>>> This is a callback meant for printing ASIC specific information to
>>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is
>>>>>>> to the callback API.
>>>>>>>
>>>>>>>> That stuff needs to be implemented for each hardware generation
>>>>>>>> and
>>>>>>>> is now cluttered with sysfs buffer offset calculations.
>>>>>>>>
>>>>>>>
>>>>>>> Looks like the warning happened because of this usage.
>>>>>>>
>>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_SCLK, buf);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_MCLK,
>>>>>>> buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_VDDC_CURVE, buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_VDDGFX_OFFSET, buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_RANGE,
>>>>>>> buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_CCLK,
>>>>>>> buf+size);
>>>>>>>
>>>>>>>
>>>>>> [Yu, Lang]
>>>>>> Yes. So it is fine we just fix the caller
>>>>>> amdgpu_get_pp_od_clk_voltage like
>>>>> following:
>>>>>>
>>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>>>>> struct device_attribute *attr,
>>>>>> char *buf)
>>>>>> {
>>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>>>>> ssize_t size, offset;
>>>>>> int ret, i;
>>>>>> char temp_buf[512];
>>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>>>>
>>>>>> if (amdgpu_in_reset(adev))
>>>>>> return -EPERM;
>>>>>> if (adev->in_suspend && !adev->in_runpm)
>>>>>> return -EPERM;
>>>>>>
>>>>>> ret = pm_runtime_get_sync(ddev->dev);
>>>>>> if (ret < 0) {
>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> offset = 0;
>>>>>>
>>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>> clock_type[i], buf);
>>>>>> if (offset + size > PAGE_SIZE)
>>>>>> break;
>>>>>> memcpy(temp_buf + offset, buf, size);
>>>>>> offset += size;
>>>>>> }
>>>>>> memcpy(buf, temp_buf, offset);
>>>>>> size = offset;
>>>>>> } else {
>>>>>> size = sysfs_emit(buf, "\n");
>>>>>> }
>>>>>> pm_runtime_mark_last_busy(ddev->dev);
>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>>
>>>>>> return size;
>>>>>> }
>>>>>>
>>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>>>> another arg to
>>>>> pass offset along with buf?
>>>>>
>>>> [Yu, Lang]
>>>> Actually, the buf address contains the offset(offset_in_page(buf)) .
>>>
>>> Though it's not a problem based on codeflow, static analysis tools
>>> might complain.
>>>
>>>> Or we just rollback to sprintf/snprintf.
>>>>
>>>
>>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
>>> the effort to convert these, he may have some other ideas.
The changes I made were intended to simply replace snprintf with sysfs_emit as per
linux/Documentation/filesystems/sysfs.rst:246,247
- show() should only use sysfs_emit() or sysfs_emit_at() when formatting
the value to be returned to user space.
I specifically tried not to change the design, but only as I didn't have
background in Power Management.
>>
>> This is not what I meant. See from the design point of view the
>> print_clock_levels() callback is the bad idea to begin with.
>>
>> What we should have instead is a callback which returns the clock as
>> a value which is then printed in the amdgpu_get_pp_od_clk_voltage()
>> function.
>>
>> This avoids passing around the buffer and remaining size everywhere
>> and also guarantees that the sysfs have unified printing over all
>> hardware generations.
>>
>
> The scenario is one node used for multiple parameters - OD_SCLK,
> OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with
> lots of nodes). On top of it, the parameters supported (for ex: CCLK
> is not valid on dGPUs), the number of levels supported etc. vary
> across ASICs. There has to be multiple calls or the call needs to
> return multiple values for a single parameter (for ex: up to 4, 8 or
> 16 levels of GFXCLK depending on ASIC).
Well exactly that is questionable design for sysfs.
See the sysfs_emit() and sysfs_emit_at() functions are designed the way
they are because you should have only one value per file, which is then
printed at exactly one location.
Take a look at the documentation for sysfs for more details.
> I don't know the history of the callback, mostly it was considered
> more efficient to print it directly rather than fetch and print.
> Alex/Evan may know the details.
Yeah, somebody with a bit more background in power management needs to
take a closer look at this here. Just keep me looped in.
Regards,
Christian.
>
> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
[-- Attachment #2: Type: text/html, Size: 15280 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-08 22:17 ` Powell, Darren
@ 2021-09-09 2:43 ` Yu, Lang
2021-09-09 3:28 ` Lazar, Lijo
0 siblings, 1 reply; 18+ messages in thread
From: Yu, Lang @ 2021-09-09 2:43 UTC (permalink / raw)
To: Powell, Darren, Christian König, Lazar, Lijo, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
[-- Attachment #1: Type: text/plain, Size: 9823 bytes --]
[AMD Official Use Only]
So the final decision is rollback to scnprintf().
If we can define our own helper functions like sysfs_emit/sysfs_emit_at
but without page boundary aligned limitation to make life easier?
Regards,
Lang
From: Powell, Darren <Darren.Powell@amd.com>
Sent: Thursday, September 9, 2021 6:18 AM
To: Christian König <ckoenig.leichtzumerken@gmail.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
[AMD Official Use Only]
________________________________
From: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>
Sent: Wednesday, September 8, 2021 8:43 AM
To: Lazar, Lijo <Lijo.Lazar@amd.com<mailto:Lijo.Lazar@amd.com>>; Yu, Lang <Lang.Yu@amd.com<mailto:Lang.Yu@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com<mailto:Alexander.Deucher@amd.com>>; Huang, Ray <Ray.Huang@amd.com<mailto:Ray.Huang@amd.com>>; Tian Tao <tiantao6@hisilicon.com<mailto:tiantao6@hisilicon.com>>; Powell, Darren <Darren.Powell@amd.com<mailto:Darren.Powell@amd.com>>
Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
Am 08.09.21 um 12:22 schrieb Lazar, Lijo:
> On 9/8/2021 3:08 PM, Christian König wrote:
>> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>>>> [AMD Official Use Only]
>>>>> -----Original Message-----
>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com<mailto:Lijo.Lazar@amd.com>>
>>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>>>> To: Yu, Lang <Lang.Yu@amd.com<mailto:Lang.Yu@amd.com>>; Christian König
>>>>> <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com<mailto:Alexander.Deucher@amd.com>>; Huang, Ray
>>>>> <Ray.Huang@amd.com<mailto:Ray.Huang@amd.com>>; Tian Tao <tiantao6@hisilicon.com<mailto:tiantao6@hisilicon.com>>
>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>> warnings
>>>>>
>>>>>
>>>>>
>>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>>>>> [AMD Official Use Only]
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com<mailto:Lijo.Lazar@amd.com>>
>>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com<mailto:ckoenig.leichtzumerken@gmail.com>>; Yu, Lang
>>>>>>> <Lang.Yu@amd.com<mailto:Lang.Yu@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
>>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com<mailto:Alexander.Deucher@amd.com>>; Huang, Ray
>>>>>>> <Ray.Huang@amd.com<mailto:Ray.Huang@amd.com>>; Tian Tao <tiantao6@hisilicon.com<mailto:tiantao6@hisilicon.com>>
>>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>>>>> warnings
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
>>>>>>>>> address. Make them happy!
>>>>>>>>>
>>>>>>>>> Warning Log:
>>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
>>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>>>>>>>> sysfs_emit_at+0x4a/0xa0
>>>>>>>>> [ 492.654805] Call Trace:
>>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
>>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
>>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
>>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
>>>>>>>>> dev_attr_show+0x1d/0x40
>>>>>>>>
>>>>>>>> Mhm, that at least partially doesn't looks like the right
>>>>>>>> approach to me.
>>>>>>>>
>>>>>>>> Why do we have string printing and sysfs code in the hardware
>>>>>>>> version specific backend in the first place?
>>>>>>>>
>>>>>>>
>>>>>>> This is a callback meant for printing ASIC specific information to
>>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is
>>>>>>> to the callback API.
>>>>>>>
>>>>>>>> That stuff needs to be implemented for each hardware generation
>>>>>>>> and
>>>>>>>> is now cluttered with sysfs buffer offset calculations.
>>>>>>>>
>>>>>>>
>>>>>>> Looks like the warning happened because of this usage.
>>>>>>>
>>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_SCLK, buf);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_MCLK,
>>>>>>> buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_VDDC_CURVE, buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_VDDGFX_OFFSET, buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_RANGE,
>>>>>>> buf+size);
>>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>>>>> OD_CCLK,
>>>>>>> buf+size);
>>>>>>>
>>>>>>>
>>>>>> [Yu, Lang]
>>>>>> Yes. So it is fine we just fix the caller
>>>>>> amdgpu_get_pp_od_clk_voltage like
>>>>> following:
>>>>>>
>>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>>>>> struct device_attribute *attr,
>>>>>> char *buf)
>>>>>> {
>>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>>>>> ssize_t size, offset;
>>>>>> int ret, i;
>>>>>> char temp_buf[512];
>>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>>>>
>>>>>> if (amdgpu_in_reset(adev))
>>>>>> return -EPERM;
>>>>>> if (adev->in_suspend && !adev->in_runpm)
>>>>>> return -EPERM;
>>>>>>
>>>>>> ret = pm_runtime_get_sync(ddev->dev);
>>>>>> if (ret < 0) {
>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> offset = 0;
>>>>>>
>>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>>> clock_type[i], buf);
>>>>>> if (offset + size > PAGE_SIZE)
>>>>>> break;
>>>>>> memcpy(temp_buf + offset, buf, size);
>>>>>> offset += size;
>>>>>> }
>>>>>> memcpy(buf, temp_buf, offset);
>>>>>> size = offset;
>>>>>> } else {
>>>>>> size = sysfs_emit(buf, "\n");
>>>>>> }
>>>>>> pm_runtime_mark_last_busy(ddev->dev);
>>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>>>>
>>>>>> return size;
>>>>>> }
>>>>>>
>>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>>>> another arg to
>>>>> pass offset along with buf?
>>>>>
>>>> [Yu, Lang]
>>>> Actually, the buf address contains the offset(offset_in_page(buf)) .
>>>
>>> Though it's not a problem based on codeflow, static analysis tools
>>> might complain.
>>>
>>>> Or we just rollback to sprintf/snprintf.
>>>>
>>>
>>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
>>> the effort to convert these, he may have some other ideas.
The changes I made were intended to simply replace snprintf with sysfs_emit as per
linux/Documentation/filesystems/sysfs.rst:246,247
- show() should only use sysfs_emit() or sysfs_emit_at() when formatting
the value to be returned to user space.
I specifically tried not to change the design, but only as I didn't have
background in Power Management.
>>
>> This is not what I meant. See from the design point of view the
>> print_clock_levels() callback is the bad idea to begin with.
>>
>> What we should have instead is a callback which returns the clock as
>> a value which is then printed in the amdgpu_get_pp_od_clk_voltage()
>> function.
>>
>> This avoids passing around the buffer and remaining size everywhere
>> and also guarantees that the sysfs have unified printing over all
>> hardware generations.
>>
>
> The scenario is one node used for multiple parameters - OD_SCLK,
> OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with
> lots of nodes). On top of it, the parameters supported (for ex: CCLK
> is not valid on dGPUs), the number of levels supported etc. vary
> across ASICs. There has to be multiple calls or the call needs to
> return multiple values for a single parameter (for ex: up to 4, 8 or
> 16 levels of GFXCLK depending on ASIC).
Well exactly that is questionable design for sysfs.
See the sysfs_emit() and sysfs_emit_at() functions are designed the way
they are because you should have only one value per file, which is then
printed at exactly one location.
Take a look at the documentation for sysfs for more details.
> I don't know the history of the callback, mostly it was considered
> more efficient to print it directly rather than fetch and print.
> Alex/Evan may know the details.
Yeah, somebody with a bit more background in power management needs to
take a closer look at this here. Just keep me looped in.
Regards,
Christian.
>
> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
[-- Attachment #2: Type: text/html, Size: 19979 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-09 2:43 ` Yu, Lang
@ 2021-09-09 3:28 ` Lazar, Lijo
2021-09-09 8:01 ` Christian König
0 siblings, 1 reply; 18+ messages in thread
From: Lazar, Lijo @ 2021-09-09 3:28 UTC (permalink / raw)
To: Yu, Lang, Powell, Darren, Christian König, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
On 9/9/2021 8:13 AM, Yu, Lang wrote:
> [AMD Official Use Only]
>
> So the final decision is rollback to scnprintf().
>
> If we can define our own helper functions like sysfs_emit/sysfs_emit_at
>
> but without page boundary aligned limitation to make life easier?
>
No, we do want to make it clear that this function is used for sysfs
files and make use of the extra checks provided by the sysfs_emit*
functions. Looking at the origins of sysf_emit_at() specifically, there
are indeed some cases of printing more than one values per file and
multi-line usage.
So I'm fine with your original patch. Maybe, you can make the intention
explicit by keeping the offset and buf start calculations in a separate
inline function.
smu_get_sysfs_buf()
Thanks,
Lijo
> Regards,
>
> Lang
>
> *From:* Powell, Darren <Darren.Powell@amd.com>
> *Sent:* Thursday, September 9, 2021 6:18 AM
> *To:* Christian König <ckoenig.leichtzumerken@gmail.com>; Lazar, Lijo
> <Lijo.Lazar@amd.com>; Yu, Lang <Lang.Yu@amd.com>;
> amd-gfx@lists.freedesktop.org
> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
> *Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>
> [AMD Official Use Only]
>
> ------------------------------------------------------------------------
>
> *From:*Christian König <ckoenig.leichtzumerken@gmail.com
> <mailto:ckoenig.leichtzumerken@gmail.com>>
> *Sent:* Wednesday, September 8, 2021 8:43 AM
> *To:* Lazar, Lijo <Lijo.Lazar@amd.com <mailto:Lijo.Lazar@amd.com>>; Yu,
> Lang <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
> <amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>>
> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com
> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray <Ray.Huang@amd.com
> <mailto:Ray.Huang@amd.com>>; Tian Tao <tiantao6@hisilicon.com
> <mailto:tiantao6@hisilicon.com>>; Powell, Darren <Darren.Powell@amd.com
> <mailto:Darren.Powell@amd.com>>
> *Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>
> Am 08.09.21 um 12:22 schrieb Lazar, Lijo:
> > On 9/8/2021 3:08 PM, Christian König wrote:
> >> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
> >>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
> >>>> [AMD Official Use Only]
> >>>>> -----Original Message-----
> >>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com <mailto:Lijo.Lazar@amd.com>>
> >>>>> Sent: Wednesday, September 8, 2021 4:55 PM
> >>>>> To: Yu, Lang <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
> Christian König
> >>>>> <ckoenig.leichtzumerken@gmail.com
> <mailto:ckoenig.leichtzumerken@gmail.com>>;
> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
> >>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com
> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray
> >>>>> <Ray.Huang@amd.com <mailto:Ray.Huang@amd.com>>; Tian Tao
> <tiantao6@hisilicon.com <mailto:tiantao6@hisilicon.com>>
> >>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
> >>>>> warnings
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
> >>>>>> [AMD Official Use Only]
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com <mailto:Lijo.Lazar@amd.com>>
> >>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
> >>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com
> <mailto:ckoenig.leichtzumerken@gmail.com>>; Yu, Lang
> >>>>>>> <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
> >>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com
> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray
> >>>>>>> <Ray.Huang@amd.com <mailto:Ray.Huang@amd.com>>; Tian Tao
> <tiantao6@hisilicon.com <mailto:tiantao6@hisilicon.com>>
> >>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
> >>>>>>> warnings
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
> >>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
> >>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary aligned buf
> >>>>>>>>> address. Make them happy!
> >>>>>>>>>
> >>>>>>>>> Warning Log:
> >>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde at:0 [
> >>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
> >>>>>>>>> sysfs_emit_at+0x4a/0xa0
> >>>>>>>>> [ 492.654805] Call Trace:
> >>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50 [amdgpu] [
> >>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
> >>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80 [amdgpu] [
> >>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [ 492.660713]
> >>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
> >>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [ 492.663620]
> >>>>>>>>> dev_attr_show+0x1d/0x40
> >>>>>>>>
> >>>>>>>> Mhm, that at least partially doesn't looks like the right
> >>>>>>>> approach to me.
> >>>>>>>>
> >>>>>>>> Why do we have string printing and sysfs code in the hardware
> >>>>>>>> version specific backend in the first place?
> >>>>>>>>
> >>>>>>>
> >>>>>>> This is a callback meant for printing ASIC specific information to
> >>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is
> >>>>>>> to the callback API.
> >>>>>>>
> >>>>>>>> That stuff needs to be implemented for each hardware generation
> >>>>>>>> and
> >>>>>>>> is now cluttered with sysfs buffer offset calculations.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Looks like the warning happened because of this usage.
> >>>>>>>
> >>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
> >>>>>>> OD_SCLK, buf);
> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
> >>>>>>> OD_MCLK,
> >>>>>>> buf+size);
> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
> >>>>>>> OD_VDDC_CURVE, buf+size);
> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
> >>>>>>> OD_VDDGFX_OFFSET, buf+size);
> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
> >>>>>>> OD_RANGE,
> >>>>>>> buf+size);
> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
> >>>>>>> OD_CCLK,
> >>>>>>> buf+size);
> >>>>>>>
> >>>>>>>
> >>>>>> [Yu, Lang]
> >>>>>> Yes. So it is fine we just fix the caller
> >>>>>> amdgpu_get_pp_od_clk_voltage like
> >>>>> following:
> >>>>>>
> >>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
> >>>>>> struct device_attribute *attr,
> >>>>>> char *buf)
> >>>>>> {
> >>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
> >>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
> >>>>>> ssize_t size, offset;
> >>>>>> int ret, i;
> >>>>>> char temp_buf[512];
> >>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
> >>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
> >>>>>>
> >>>>>> if (amdgpu_in_reset(adev))
> >>>>>> return -EPERM;
> >>>>>> if (adev->in_suspend && !adev->in_runpm)
> >>>>>> return -EPERM;
> >>>>>>
> >>>>>> ret = pm_runtime_get_sync(ddev->dev);
> >>>>>> if (ret < 0) {
> >>>>>> pm_runtime_put_autosuspend(ddev->dev);
> >>>>>> return ret;
> >>>>>> }
> >>>>>>
> >>>>>> offset = 0;
> >>>>>>
> >>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
> >>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
> >>>>>> size = amdgpu_dpm_print_clock_levels(adev,
> >>>>> clock_type[i], buf);
> >>>>>> if (offset + size > PAGE_SIZE)
> >>>>>> break;
> >>>>>> memcpy(temp_buf + offset, buf, size);
> >>>>>> offset += size;
> >>>>>> }
> >>>>>> memcpy(buf, temp_buf, offset);
> >>>>>> size = offset;
> >>>>>> } else {
> >>>>>> size = sysfs_emit(buf, "\n");
> >>>>>> }
> >>>>>> pm_runtime_mark_last_busy(ddev->dev);
> >>>>>> pm_runtime_put_autosuspend(ddev->dev);
> >>>>>>
> >>>>>> return size;
> >>>>>> }
> >>>>>>
> >>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
> >>>>> another arg to
> >>>>> pass offset along with buf?
> >>>>>
> >>>> [Yu, Lang]
> >>>> Actually, the buf address contains the offset(offset_in_page(buf)) .
> >>>
> >>> Though it's not a problem based on codeflow, static analysis tools
> >>> might complain.
> >>>
> >>>> Or we just rollback to sprintf/snprintf.
> >>>>
> >>>
> >>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
> >>> the effort to convert these, he may have some other ideas.
> The changes I made were intended to simply replace snprintf with
> sysfs_emit as per
>
> linux/Documentation/filesystems/sysfs.rst:246,247
>
> - show() should only use sysfs_emit() or sysfs_emit_at() when formatting
>
> the value to be returned to user space.
>
> I specifically tried not to change the design, but only as I didn't have
>
> background in Power Management.
>
> >>
> >> This is not what I meant. See from the design point of view the
> >> print_clock_levels() callback is the bad idea to begin with.
> >>
> >> What we should have instead is a callback which returns the clock as
> >> a value which is then printed in the amdgpu_get_pp_od_clk_voltage()
> >> function.
> >>
> >> This avoids passing around the buffer and remaining size everywhere
> >> and also guarantees that the sysfs have unified printing over all
> >> hardware generations.
> >>
> >
> > The scenario is one node used for multiple parameters - OD_SCLK,
> > OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with
> > lots of nodes). On top of it, the parameters supported (for ex: CCLK
> > is not valid on dGPUs), the number of levels supported etc. vary
> > across ASICs. There has to be multiple calls or the call needs to
> > return multiple values for a single parameter (for ex: up to 4, 8 or
> > 16 levels of GFXCLK depending on ASIC).
>
> Well exactly that is questionable design for sysfs.
>
> See the sysfs_emit() and sysfs_emit_at() functions are designed the way
> they are because you should have only one value per file, which is then
> printed at exactly one location.
>
> Take a look at the documentation for sysfs for more details.
>
> > I don't know the history of the callback, mostly it was considered
> > more efficient to print it directly rather than fetch and print.
> > Alex/Evan may know the details.
>
> Yeah, somebody with a bit more background in power management needs to
> take a closer look at this here. Just keep me looped in.
>
> Regards,
> Christian.
>
> >
> > Thanks,
> > Lijo
> >
> >> Regards,
> >> Christian.
> >>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-09 3:28 ` Lazar, Lijo
@ 2021-09-09 8:01 ` Christian König
2021-09-09 8:36 ` Lazar, Lijo
0 siblings, 1 reply; 18+ messages in thread
From: Christian König @ 2021-09-09 8:01 UTC (permalink / raw)
To: Lazar, Lijo, Yu, Lang, Powell, Darren, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
Am 09.09.21 um 05:28 schrieb Lazar, Lijo:
> On 9/9/2021 8:13 AM, Yu, Lang wrote:
>> [AMD Official Use Only]
>>
>> So the final decision is rollback to scnprintf().
>>
>> If we can define our own helper functions like sysfs_emit/sysfs_emit_at
>>
>> but without page boundary aligned limitation to make life easier?
>>
>
> No, we do want to make it clear that this function is used for sysfs
> files and make use of the extra checks provided by the sysfs_emit*
> functions. Looking at the origins of sysf_emit_at() specifically,
> there are indeed some cases of printing more than one values per file
> and multi-line usage.
Correct, but those are rather limited and well documented special cases.
E.g. for example if you need to grab a lock to get multiple values which
are supposed to be coherent to each other.
I think that's the case here, so printing multiple values is probably ok
in general. But we still need to get the implementation straight.
> So I'm fine with your original patch. Maybe, you can make the
> intention explicit by keeping the offset and buf start calculations in
> a separate inline function.
> smu_get_sysfs_buf()
Exactly that is what is not ok. So once more the intended use case of
those functions is:
offs = sysfs_emit(page, ...);
offs += sysfs_emit_at(page, offs, ....);
offs += sysfs_emit_at(page, offs, ....);
...
Another possible alternative which I think should be allowed is:
offs = 0;
for_each_clock_in_my_device(..) {
offs += sysfs_emit_at(page, offs, ....);
}
But when you are calculating the initial offset manually then there is
certainly something wrong here and that is not the intended usage pattern.
Regards,
Christian.
>
> Thanks,
> Lijo
>
>> Regards,
>>
>> Lang
>>
>> *From:* Powell, Darren <Darren.Powell@amd.com>
>> *Sent:* Thursday, September 9, 2021 6:18 AM
>> *To:* Christian König <ckoenig.leichtzumerken@gmail.com>; Lazar, Lijo
>> <Lijo.Lazar@amd.com>; Yu, Lang <Lang.Yu@amd.com>;
>> amd-gfx@lists.freedesktop.org
>> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>> *Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>
>> [AMD Official Use Only]
>>
>> ------------------------------------------------------------------------
>>
>> *From:*Christian König <ckoenig.leichtzumerken@gmail.com
>> <mailto:ckoenig.leichtzumerken@gmail.com>>
>> *Sent:* Wednesday, September 8, 2021 8:43 AM
>> *To:* Lazar, Lijo <Lijo.Lazar@amd.com <mailto:Lijo.Lazar@amd.com>>;
>> Yu, Lang <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>> <amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>>
>> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com
>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray <Ray.Huang@amd.com
>> <mailto:Ray.Huang@amd.com>>; Tian Tao <tiantao6@hisilicon.com
>> <mailto:tiantao6@hisilicon.com>>; Powell, Darren
>> <Darren.Powell@amd.com <mailto:Darren.Powell@amd.com>>
>> *Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>
>> Am 08.09.21 um 12:22 schrieb Lazar, Lijo:
>> > On 9/8/2021 3:08 PM, Christian König wrote:
>> >> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>> >>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>> >>>> [AMD Official Use Only]
>> >>>>> -----Original Message-----
>> >>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com
>> <mailto:Lijo.Lazar@amd.com>>
>> >>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>> >>>>> To: Yu, Lang <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>> Christian König
>> >>>>> <ckoenig.leichtzumerken@gmail.com
>> <mailto:ckoenig.leichtzumerken@gmail.com>>;
>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>> >>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com
>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray
>> >>>>> <Ray.Huang@amd.com <mailto:Ray.Huang@amd.com>>; Tian Tao
>> <tiantao6@hisilicon.com <mailto:tiantao6@hisilicon.com>>
>> >>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>> >>>>> warnings
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>> >>>>>> [AMD Official Use Only]
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>>> -----Original Message-----
>> >>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com
>> <mailto:Lijo.Lazar@amd.com>>
>> >>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>> >>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com
>> <mailto:ckoenig.leichtzumerken@gmail.com>>; Yu, Lang
>> >>>>>>> <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>> >>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com
>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray
>> >>>>>>> <Ray.Huang@amd.com <mailto:Ray.Huang@amd.com>>; Tian Tao
>> <tiantao6@hisilicon.com <mailto:tiantao6@hisilicon.com>>
>> >>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>> >>>>>>> warnings
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>> >>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>> >>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary
>> aligned buf
>> >>>>>>>>> address. Make them happy!
>> >>>>>>>>>
>> >>>>>>>>> Warning Log:
>> >>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde
>> at:0 [
>> >>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>> >>>>>>>>> sysfs_emit_at+0x4a/0xa0
>> >>>>>>>>> [ 492.654805] Call Trace:
>> >>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50
>> [amdgpu] [
>> >>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>> >>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80
>> [amdgpu] [
>> >>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [
>> 492.660713]
>> >>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>> >>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [
>> 492.663620]
>> >>>>>>>>> dev_attr_show+0x1d/0x40
>> >>>>>>>>
>> >>>>>>>> Mhm, that at least partially doesn't looks like the right
>> >>>>>>>> approach to me.
>> >>>>>>>>
>> >>>>>>>> Why do we have string printing and sysfs code in the hardware
>> >>>>>>>> version specific backend in the first place?
>> >>>>>>>>
>> >>>>>>>
>> >>>>>>> This is a callback meant for printing ASIC specific
>> information to
>> >>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is
>> >>>>>>> to the callback API.
>> >>>>>>>
>> >>>>>>>> That stuff needs to be implemented for each hardware
>> generation
>> >>>>>>>> and
>> >>>>>>>> is now cluttered with sysfs buffer offset calculations.
>> >>>>>>>>
>> >>>>>>>
>> >>>>>>> Looks like the warning happened because of this usage.
>> >>>>>>>
>> >>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>> >>>>>>> OD_SCLK, buf);
>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>> >>>>>>> OD_MCLK,
>> >>>>>>> buf+size);
>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>> >>>>>>> OD_VDDC_CURVE, buf+size);
>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>> >>>>>>> OD_VDDGFX_OFFSET, buf+size);
>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>> >>>>>>> OD_RANGE,
>> >>>>>>> buf+size);
>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>> >>>>>>> OD_CCLK,
>> >>>>>>> buf+size);
>> >>>>>>>
>> >>>>>>>
>> >>>>>> [Yu, Lang]
>> >>>>>> Yes. So it is fine we just fix the caller
>> >>>>>> amdgpu_get_pp_od_clk_voltage like
>> >>>>> following:
>> >>>>>>
>> >>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>> >>>>>> struct device_attribute *attr,
>> >>>>>> char *buf)
>> >>>>>> {
>> >>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>> >>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>> >>>>>> ssize_t size, offset;
>> >>>>>> int ret, i;
>> >>>>>> char temp_buf[512];
>> >>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>> >>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>> >>>>>>
>> >>>>>> if (amdgpu_in_reset(adev))
>> >>>>>> return -EPERM;
>> >>>>>> if (adev->in_suspend && !adev->in_runpm)
>> >>>>>> return -EPERM;
>> >>>>>>
>> >>>>>> ret = pm_runtime_get_sync(ddev->dev);
>> >>>>>> if (ret < 0) {
>> >>>>>> pm_runtime_put_autosuspend(ddev->dev);
>> >>>>>> return ret;
>> >>>>>> }
>> >>>>>>
>> >>>>>> offset = 0;
>> >>>>>>
>> >>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>> >>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>> >>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>> >>>>> clock_type[i], buf);
>> >>>>>> if (offset + size > PAGE_SIZE)
>> >>>>>> break;
>> >>>>>> memcpy(temp_buf + offset, buf, size);
>> >>>>>> offset += size;
>> >>>>>> }
>> >>>>>> memcpy(buf, temp_buf, offset);
>> >>>>>> size = offset;
>> >>>>>> } else {
>> >>>>>> size = sysfs_emit(buf, "\n");
>> >>>>>> }
>> >>>>>> pm_runtime_mark_last_busy(ddev->dev);
>> >>>>>> pm_runtime_put_autosuspend(ddev->dev);
>> >>>>>>
>> >>>>>> return size;
>> >>>>>> }
>> >>>>>>
>> >>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>> >>>>> another arg to
>> >>>>> pass offset along with buf?
>> >>>>>
>> >>>> [Yu, Lang]
>> >>>> Actually, the buf address contains the
>> offset(offset_in_page(buf)) .
>> >>>
>> >>> Though it's not a problem based on codeflow, static analysis tools
>> >>> might complain.
>> >>>
>> >>>> Or we just rollback to sprintf/snprintf.
>> >>>>
>> >>>
>> >>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
>> >>> the effort to convert these, he may have some other ideas.
>> The changes I made were intended to simply replace snprintf with
>> sysfs_emit as per
>>
>> linux/Documentation/filesystems/sysfs.rst:246,247
>>
>> - show() should only use sysfs_emit() or sysfs_emit_at() when
>> formatting
>>
>> the value to be returned to user space.
>>
>> I specifically tried not to change the design, but only as I didn't have
>>
>> background in Power Management.
>>
>> >>
>> >> This is not what I meant. See from the design point of view the
>> >> print_clock_levels() callback is the bad idea to begin with.
>> >>
>> >> What we should have instead is a callback which returns the clock as
>> >> a value which is then printed in the amdgpu_get_pp_od_clk_voltage()
>> >> function.
>> >>
>> >> This avoids passing around the buffer and remaining size everywhere
>> >> and also guarantees that the sysfs have unified printing over all
>> >> hardware generations.
>> >>
>> >
>> > The scenario is one node used for multiple parameters - OD_SCLK,
>> > OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with
>> > lots of nodes). On top of it, the parameters supported (for ex: CCLK
>> > is not valid on dGPUs), the number of levels supported etc. vary
>> > across ASICs. There has to be multiple calls or the call needs to
>> > return multiple values for a single parameter (for ex: up to 4, 8 or
>> > 16 levels of GFXCLK depending on ASIC).
>>
>> Well exactly that is questionable design for sysfs.
>>
>> See the sysfs_emit() and sysfs_emit_at() functions are designed the way
>> they are because you should have only one value per file, which is then
>> printed at exactly one location.
>>
>> Take a look at the documentation for sysfs for more details.
>>
>> > I don't know the history of the callback, mostly it was considered
>> > more efficient to print it directly rather than fetch and print.
>> > Alex/Evan may know the details.
>>
>> Yeah, somebody with a bit more background in power management needs to
>> take a closer look at this here. Just keep me looped in.
>>
>> Regards,
>> Christian.
>>
>> >
>> > Thanks,
>> > Lijo
>> >
>> >> Regards,
>> >> Christian.
>> >>
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-09 8:01 ` Christian König
@ 2021-09-09 8:36 ` Lazar, Lijo
2021-09-09 8:50 ` Christian König
0 siblings, 1 reply; 18+ messages in thread
From: Lazar, Lijo @ 2021-09-09 8:36 UTC (permalink / raw)
To: Christian König, Yu, Lang, Powell, Darren, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
On 9/9/2021 1:31 PM, Christian König wrote:
> Am 09.09.21 um 05:28 schrieb Lazar, Lijo:
>> On 9/9/2021 8:13 AM, Yu, Lang wrote:
>>> [AMD Official Use Only]
>>>
>>> So the final decision is rollback to scnprintf().
>>>
>>> If we can define our own helper functions like sysfs_emit/sysfs_emit_at
>>>
>>> but without page boundary aligned limitation to make life easier?
>>>
>>
>> No, we do want to make it clear that this function is used for sysfs
>> files and make use of the extra checks provided by the sysfs_emit*
>> functions. Looking at the origins of sysf_emit_at() specifically,
>> there are indeed some cases of printing more than one values per file
>> and multi-line usage.
>
> Correct, but those are rather limited and well documented special cases.
> E.g. for example if you need to grab a lock to get multiple values which
> are supposed to be coherent to each other.
>
> I think that's the case here, so printing multiple values is probably ok
> in general. But we still need to get the implementation straight.
>
>> So I'm fine with your original patch. Maybe, you can make the
>> intention explicit by keeping the offset and buf start calculations in
>> a separate inline function.
>> smu_get_sysfs_buf()
>
> Exactly that is what is not ok. So once more the intended use case of
> those functions is:
>
> offs = sysfs_emit(page, ...);
> offs += sysfs_emit_at(page, offs, ....);
> offs += sysfs_emit_at(page, offs, ....);
> ...
>
> Another possible alternative which I think should be allowed is:
>
> offs = 0;
> for_each_clock_in_my_device(..) {
> offs += sysfs_emit_at(page, offs, ....);
> }
>
> But when you are calculating the initial offset manually then there is
> certainly something wrong here and that is not the intended usage pattern.
>
Actually, the issue is not within one function invocation. The issue is
at the caller side with multiple invocations -
size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK, buf);
size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK,
buf+size);
Having amdgpu_dpm_print_clock_levels() helped to consolidate sysfs calls
in single function for different parameters and used for different
nodes. However in this case, different parameters are presented as a
single "logical entity" in a sysfs node and the function is called
multiple times for different parameters (questionable as per sysfs
guidelines, Alex needs to come back on this).
Within one invocation of amdgpu_dpm_print_clock_levels(), the APIs are
used correctly. For the second call, it needs to pass the page aligned
buf pointer correctly to sysfs_emit* calls.
Presently, amdgpu_dpm_print_clock_levels() takes only buff pointer as
argument and probably it is that way since the function existed before
sysfs_emit* patches got added and was originally using sprintfs.
Now, two possible options are -
1) Make a pre-requisite that this function is always going to print to
sysfs files. For that use sysfs_emit*. Also, as with a sysfs buffer
calculate the page aligned start address of buf and offset for use with
sysfs_emit* in the beginning. At least for now, this assumption is
inline with the buffer start address requirement in sysfs_emit* patches.
This is what the original patch does. That said, if the buffer
properties change in future this will not hold good.
2) Pass the offset along with the buff in API. That will be extensive
since it affects older powerplay based HWs also.
There may be other ways, but those could be even more extensive than 2).
Thanks,
Lijo
> Regards,
> Christian.
>
>>
>> Thanks,
>> Lijo
>>
>>> Regards,
>>>
>>> Lang
>>>
>>> *From:* Powell, Darren <Darren.Powell@amd.com>
>>> *Sent:* Thursday, September 9, 2021 6:18 AM
>>> *To:* Christian König <ckoenig.leichtzumerken@gmail.com>; Lazar, Lijo
>>> <Lijo.Lazar@amd.com>; Yu, Lang <Lang.Yu@amd.com>;
>>> amd-gfx@lists.freedesktop.org
>>> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>> *Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>>
>>> [AMD Official Use Only]
>>>
>>> ------------------------------------------------------------------------
>>>
>>> *From:*Christian König <ckoenig.leichtzumerken@gmail.com
>>> <mailto:ckoenig.leichtzumerken@gmail.com>>
>>> *Sent:* Wednesday, September 8, 2021 8:43 AM
>>> *To:* Lazar, Lijo <Lijo.Lazar@amd.com <mailto:Lijo.Lazar@amd.com>>;
>>> Yu, Lang <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>>> <amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>>
>>> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com
>>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray <Ray.Huang@amd.com
>>> <mailto:Ray.Huang@amd.com>>; Tian Tao <tiantao6@hisilicon.com
>>> <mailto:tiantao6@hisilicon.com>>; Powell, Darren
>>> <Darren.Powell@amd.com <mailto:Darren.Powell@amd.com>>
>>> *Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
>>>
>>> Am 08.09.21 um 12:22 schrieb Lazar, Lijo:
>>> > On 9/8/2021 3:08 PM, Christian König wrote:
>>> >> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>>> >>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>>> >>>> [AMD Official Use Only]
>>> >>>>> -----Original Message-----
>>> >>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com
>>> <mailto:Lijo.Lazar@amd.com>>
>>> >>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>> >>>>> To: Yu, Lang <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>>> Christian König
>>> >>>>> <ckoenig.leichtzumerken@gmail.com
>>> <mailto:ckoenig.leichtzumerken@gmail.com>>;
>>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>>> >>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com
>>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray
>>> >>>>> <Ray.Huang@amd.com <mailto:Ray.Huang@amd.com>>; Tian Tao
>>> <tiantao6@hisilicon.com <mailto:tiantao6@hisilicon.com>>
>>> >>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>> >>>>> warnings
>>> >>>>>
>>> >>>>>
>>> >>>>>
>>> >>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>> >>>>>> [AMD Official Use Only]
>>> >>>>>>
>>> >>>>>>
>>> >>>>>>
>>> >>>>>>> -----Original Message-----
>>> >>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com
>>> <mailto:Lijo.Lazar@amd.com>>
>>> >>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>> >>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com
>>> <mailto:ckoenig.leichtzumerken@gmail.com>>; Yu, Lang
>>> >>>>>>> <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>>> >>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com
>>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray
>>> >>>>>>> <Ray.Huang@amd.com <mailto:Ray.Huang@amd.com>>; Tian Tao
>>> <tiantao6@hisilicon.com <mailto:tiantao6@hisilicon.com>>
>>> >>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>> >>>>>>> warnings
>>> >>>>>>>
>>> >>>>>>>
>>> >>>>>>>
>>> >>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>> >>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>> >>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary
>>> aligned buf
>>> >>>>>>>>> address. Make them happy!
>>> >>>>>>>>>
>>> >>>>>>>>> Warning Log:
>>> >>>>>>>>> [ 492.545174] invalid sysfs_emit_at: buf:00000000f19bdfde
>>> at:0 [
>>> >>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at fs/sysfs/file.c:765
>>> >>>>>>>>> sysfs_emit_at+0x4a/0xa0
>>> >>>>>>>>> [ 492.654805] Call Trace:
>>> >>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50
>>> [amdgpu] [
>>> >>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>> >>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80
>>> [amdgpu] [
>>> >>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [
>>> 492.660713]
>>> >>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>> >>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [
>>> 492.663620]
>>> >>>>>>>>> dev_attr_show+0x1d/0x40
>>> >>>>>>>>
>>> >>>>>>>> Mhm, that at least partially doesn't looks like the right
>>> >>>>>>>> approach to me.
>>> >>>>>>>>
>>> >>>>>>>> Why do we have string printing and sysfs code in the hardware
>>> >>>>>>>> version specific backend in the first place?
>>> >>>>>>>>
>>> >>>>>>>
>>> >>>>>>> This is a callback meant for printing ASIC specific
>>> information to
>>> >>>>>>> sysfs node. The buffer passed in sysfs read is passed as it is
>>> >>>>>>> to the callback API.
>>> >>>>>>>
>>> >>>>>>>> That stuff needs to be implemented for each hardware
>>> generation
>>> >>>>>>>> and
>>> >>>>>>>> is now cluttered with sysfs buffer offset calculations.
>>> >>>>>>>>
>>> >>>>>>>
>>> >>>>>>> Looks like the warning happened because of this usage.
>>> >>>>>>>
>>> >>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>> >>>>>>> OD_SCLK, buf);
>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>> >>>>>>> OD_MCLK,
>>> >>>>>>> buf+size);
>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>> >>>>>>> OD_VDDC_CURVE, buf+size);
>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>> >>>>>>> OD_VDDGFX_OFFSET, buf+size);
>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>> >>>>>>> OD_RANGE,
>>> >>>>>>> buf+size);
>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>> >>>>>>> OD_CCLK,
>>> >>>>>>> buf+size);
>>> >>>>>>>
>>> >>>>>>>
>>> >>>>>> [Yu, Lang]
>>> >>>>>> Yes. So it is fine we just fix the caller
>>> >>>>>> amdgpu_get_pp_od_clk_voltage like
>>> >>>>> following:
>>> >>>>>>
>>> >>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device *dev,
>>> >>>>>> struct device_attribute *attr,
>>> >>>>>> char *buf)
>>> >>>>>> {
>>> >>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>> >>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>> >>>>>> ssize_t size, offset;
>>> >>>>>> int ret, i;
>>> >>>>>> char temp_buf[512];
>>> >>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>> >>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>> >>>>>>
>>> >>>>>> if (amdgpu_in_reset(adev))
>>> >>>>>> return -EPERM;
>>> >>>>>> if (adev->in_suspend && !adev->in_runpm)
>>> >>>>>> return -EPERM;
>>> >>>>>>
>>> >>>>>> ret = pm_runtime_get_sync(ddev->dev);
>>> >>>>>> if (ret < 0) {
>>> >>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>> >>>>>> return ret;
>>> >>>>>> }
>>> >>>>>>
>>> >>>>>> offset = 0;
>>> >>>>>>
>>> >>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>> >>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>> >>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>> >>>>> clock_type[i], buf);
>>> >>>>>> if (offset + size > PAGE_SIZE)
>>> >>>>>> break;
>>> >>>>>> memcpy(temp_buf + offset, buf, size);
>>> >>>>>> offset += size;
>>> >>>>>> }
>>> >>>>>> memcpy(buf, temp_buf, offset);
>>> >>>>>> size = offset;
>>> >>>>>> } else {
>>> >>>>>> size = sysfs_emit(buf, "\n");
>>> >>>>>> }
>>> >>>>>> pm_runtime_mark_last_busy(ddev->dev);
>>> >>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>> >>>>>>
>>> >>>>>> return size;
>>> >>>>>> }
>>> >>>>>>
>>> >>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>> >>>>> another arg to
>>> >>>>> pass offset along with buf?
>>> >>>>>
>>> >>>> [Yu, Lang]
>>> >>>> Actually, the buf address contains the
>>> offset(offset_in_page(buf)) .
>>> >>>
>>> >>> Though it's not a problem based on codeflow, static analysis tools
>>> >>> might complain.
>>> >>>
>>> >>>> Or we just rollback to sprintf/snprintf.
>>> >>>>
>>> >>>
>>> >>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren took
>>> >>> the effort to convert these, he may have some other ideas.
>>> The changes I made were intended to simply replace snprintf with
>>> sysfs_emit as per
>>>
>>> linux/Documentation/filesystems/sysfs.rst:246,247
>>>
>>> - show() should only use sysfs_emit() or sysfs_emit_at() when
>>> formatting
>>>
>>> the value to be returned to user space.
>>>
>>> I specifically tried not to change the design, but only as I didn't have
>>>
>>> background in Power Management.
>>>
>>> >>
>>> >> This is not what I meant. See from the design point of view the
>>> >> print_clock_levels() callback is the bad idea to begin with.
>>> >>
>>> >> What we should have instead is a callback which returns the clock as
>>> >> a value which is then printed in the amdgpu_get_pp_od_clk_voltage()
>>> >> function.
>>> >>
>>> >> This avoids passing around the buffer and remaining size everywhere
>>> >> and also guarantees that the sysfs have unified printing over all
>>> >> hardware generations.
>>> >>
>>> >
>>> > The scenario is one node used for multiple parameters - OD_SCLK,
>>> > OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs with
>>> > lots of nodes). On top of it, the parameters supported (for ex: CCLK
>>> > is not valid on dGPUs), the number of levels supported etc. vary
>>> > across ASICs. There has to be multiple calls or the call needs to
>>> > return multiple values for a single parameter (for ex: up to 4, 8 or
>>> > 16 levels of GFXCLK depending on ASIC).
>>>
>>> Well exactly that is questionable design for sysfs.
>>>
>>> See the sysfs_emit() and sysfs_emit_at() functions are designed the way
>>> they are because you should have only one value per file, which is then
>>> printed at exactly one location.
>>>
>>> Take a look at the documentation for sysfs for more details.
>>>
>>> > I don't know the history of the callback, mostly it was considered
>>> > more efficient to print it directly rather than fetch and print.
>>> > Alex/Evan may know the details.
>>>
>>> Yeah, somebody with a bit more background in power management needs to
>>> take a closer look at this here. Just keep me looped in.
>>>
>>> Regards,
>>> Christian.
>>>
>>> >
>>> > Thanks,
>>> > Lijo
>>> >
>>> >> Regards,
>>> >> Christian.
>>> >>
>>>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings
2021-09-09 8:36 ` Lazar, Lijo
@ 2021-09-09 8:50 ` Christian König
0 siblings, 0 replies; 18+ messages in thread
From: Christian König @ 2021-09-09 8:50 UTC (permalink / raw)
To: Lazar, Lijo, Yu, Lang, Powell, Darren, amd-gfx
Cc: Deucher, Alexander, Huang, Ray, Tian Tao
Am 09.09.21 um 10:36 schrieb Lazar, Lijo:
> On 9/9/2021 1:31 PM, Christian König wrote:
>> Am 09.09.21 um 05:28 schrieb Lazar, Lijo:
>>> On 9/9/2021 8:13 AM, Yu, Lang wrote:
>>>> [AMD Official Use Only]
>>>>
>>>> So the final decision is rollback to scnprintf().
>>>>
>>>> If we can define our own helper functions like
>>>> sysfs_emit/sysfs_emit_at
>>>>
>>>> but without page boundary aligned limitation to make life easier?
>>>>
>>>
>>> No, we do want to make it clear that this function is used for sysfs
>>> files and make use of the extra checks provided by the sysfs_emit*
>>> functions. Looking at the origins of sysf_emit_at() specifically,
>>> there are indeed some cases of printing more than one values per
>>> file and multi-line usage.
>>
>> Correct, but those are rather limited and well documented special
>> cases. E.g. for example if you need to grab a lock to get multiple
>> values which are supposed to be coherent to each other.
>>
>> I think that's the case here, so printing multiple values is probably
>> ok in general. But we still need to get the implementation straight.
>>
>>> So I'm fine with your original patch. Maybe, you can make the
>>> intention explicit by keeping the offset and buf start calculations
>>> in a separate inline function.
>>> smu_get_sysfs_buf()
>>
>> Exactly that is what is not ok. So once more the intended use case of
>> those functions is:
>>
>> offs = sysfs_emit(page, ...);
>> offs += sysfs_emit_at(page, offs, ....);
>> offs += sysfs_emit_at(page, offs, ....);
>> ...
>>
>> Another possible alternative which I think should be allowed is:
>>
>> offs = 0;
>> for_each_clock_in_my_device(..) {
>> offs += sysfs_emit_at(page, offs, ....);
>> }
>>
>> But when you are calculating the initial offset manually then there
>> is certainly something wrong here and that is not the intended usage
>> pattern.
>>
>
> Actually, the issue is not within one function invocation. The issue
> is at the caller side with multiple invocations -
>
> size = amdgpu_dpm_print_clock_levels(adev, OD_SCLK,
> buf);
> size += amdgpu_dpm_print_clock_levels(adev, OD_MCLK,
> buf+size);
>
> Having amdgpu_dpm_print_clock_levels() helped to consolidate sysfs
> calls in single function for different parameters and used for
> different nodes. However in this case, different parameters are
> presented as a single "logical entity" in a sysfs node and the
> function is called multiple times for different parameters
> (questionable as per sysfs guidelines, Alex needs to come back on this).
Yes, exactly that. But as I noted above multiple values are ok when you
need to keep them coherent, e.g. returning SCLK and MCLK without a
performance level switch in between.
> Within one invocation of amdgpu_dpm_print_clock_levels(), the APIs are
> used correctly. For the second call, it needs to pass the page aligned
> buf pointer correctly to sysfs_emit* calls.
>
> Presently, amdgpu_dpm_print_clock_levels() takes only buff pointer as
> argument and probably it is that way since the function existed before
> sysfs_emit* patches got added and was originally using sprintfs.
>
> Now, two possible options are -
>
> 1) Make a pre-requisite that this function is always going to print to
> sysfs files. For that use sysfs_emit*. Also, as with a sysfs buffer
> calculate the page aligned start address of buf and offset for use
> with sysfs_emit* in the beginning. At least for now, this assumption
> is inline with the buffer start address requirement in sysfs_emit*
> patches. This is what the original patch does. That said, if the
> buffer properties change in future this will not hold good.
>
> 2) Pass the offset along with the buff in API. That will be extensive
> since it affects older powerplay based HWs also.
I think that should be the way to go then.
> There may be other ways, but those could be even more extensive than 2).
From a high level view my feeling says me that returning the values as
numbers and printing them in the higher level function is a better
design, but there might as well be reason against that.
Regards,
Christian.
>
> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks,
>>> Lijo
>>>
>>>> Regards,
>>>>
>>>> Lang
>>>>
>>>> *From:* Powell, Darren <Darren.Powell@amd.com>
>>>> *Sent:* Thursday, September 9, 2021 6:18 AM
>>>> *To:* Christian König <ckoenig.leichtzumerken@gmail.com>; Lazar,
>>>> Lijo <Lijo.Lazar@amd.com>; Yu, Lang <Lang.Yu@amd.com>;
>>>> amd-gfx@lists.freedesktop.org
>>>> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
>>>> <Ray.Huang@amd.com>; Tian Tao <tiantao6@hisilicon.com>
>>>> *Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>> warnings
>>>>
>>>> [AMD Official Use Only]
>>>>
>>>> ------------------------------------------------------------------------
>>>>
>>>>
>>>> *From:*Christian König <ckoenig.leichtzumerken@gmail.com
>>>> <mailto:ckoenig.leichtzumerken@gmail.com>>
>>>> *Sent:* Wednesday, September 8, 2021 8:43 AM
>>>> *To:* Lazar, Lijo <Lijo.Lazar@amd.com <mailto:Lijo.Lazar@amd.com>>;
>>>> Yu, Lang <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>>>> amd-gfx@lists.freedesktop.org
>>>> <mailto:amd-gfx@lists.freedesktop.org>
>>>> <amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>>
>>>> *Cc:* Deucher, Alexander <Alexander.Deucher@amd.com
>>>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray <Ray.Huang@amd.com
>>>> <mailto:Ray.Huang@amd.com>>; Tian Tao <tiantao6@hisilicon.com
>>>> <mailto:tiantao6@hisilicon.com>>; Powell, Darren
>>>> <Darren.Powell@amd.com <mailto:Darren.Powell@amd.com>>
>>>> *Subject:* Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>> warnings
>>>>
>>>> Am 08.09.21 um 12:22 schrieb Lazar, Lijo:
>>>> > On 9/8/2021 3:08 PM, Christian König wrote:
>>>> >> Am 08.09.21 um 11:29 schrieb Lazar, Lijo:
>>>> >>> On 9/8/2021 2:32 PM, Yu, Lang wrote:
>>>> >>>> [AMD Official Use Only]
>>>> >>>>> -----Original Message-----
>>>> >>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com
>>>> <mailto:Lijo.Lazar@amd.com>>
>>>> >>>>> Sent: Wednesday, September 8, 2021 4:55 PM
>>>> >>>>> To: Yu, Lang <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>>>> Christian König
>>>> >>>>> <ckoenig.leichtzumerken@gmail.com
>>>> <mailto:ckoenig.leichtzumerken@gmail.com>>;
>>>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>>>> >>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com
>>>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray
>>>> >>>>> <Ray.Huang@amd.com <mailto:Ray.Huang@amd.com>>; Tian Tao
>>>> <tiantao6@hisilicon.com <mailto:tiantao6@hisilicon.com>>
>>>> >>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>> >>>>> warnings
>>>> >>>>>
>>>> >>>>>
>>>> >>>>>
>>>> >>>>> On 9/8/2021 1:14 PM, Yu, Lang wrote:
>>>> >>>>>> [AMD Official Use Only]
>>>> >>>>>>
>>>> >>>>>>
>>>> >>>>>>
>>>> >>>>>>> -----Original Message-----
>>>> >>>>>>> From: Lazar, Lijo <Lijo.Lazar@amd.com
>>>> <mailto:Lijo.Lazar@amd.com>>
>>>> >>>>>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>>> >>>>>>> To: Christian König <ckoenig.leichtzumerken@gmail.com
>>>> <mailto:ckoenig.leichtzumerken@gmail.com>>; Yu, Lang
>>>> >>>>>>> <Lang.Yu@amd.com <mailto:Lang.Yu@amd.com>>;
>>>> amd-gfx@lists.freedesktop.org <mailto:amd-gfx@lists.freedesktop.org>
>>>> >>>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com
>>>> <mailto:Alexander.Deucher@amd.com>>; Huang, Ray
>>>> >>>>>>> <Ray.Huang@amd.com <mailto:Ray.Huang@amd.com>>; Tian Tao
>>>> <tiantao6@hisilicon.com <mailto:tiantao6@hisilicon.com>>
>>>> >>>>>>> Subject: Re: [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at
>>>> >>>>>>> warnings
>>>> >>>>>>>
>>>> >>>>>>>
>>>> >>>>>>>
>>>> >>>>>>> On 9/8/2021 12:07 PM, Christian König wrote:
>>>> >>>>>>>> Am 08.09.21 um 07:56 schrieb Lang Yu:
>>>> >>>>>>>>> sysfs_emit and sysfs_emit_at requrie a page boundary
>>>> aligned buf
>>>> >>>>>>>>> address. Make them happy!
>>>> >>>>>>>>>
>>>> >>>>>>>>> Warning Log:
>>>> >>>>>>>>> [ 492.545174] invalid sysfs_emit_at:
>>>> buf:00000000f19bdfde at:0 [
>>>> >>>>>>>>> 492.546416] WARNING: CPU: 7 PID: 1304 at
>>>> fs/sysfs/file.c:765
>>>> >>>>>>>>> sysfs_emit_at+0x4a/0xa0
>>>> >>>>>>>>> [ 492.654805] Call Trace:
>>>> >>>>>>>>> [ 492.655353] ? smu_cmn_get_metrics_table+0x40/0x50
>>>> [amdgpu] [
>>>> >>>>>>>>> 492.656780] vangogh_print_clk_levels+0x369/0x410 [amdgpu] [
>>>> >>>>>>>>> 492.658245] vangogh_common_print_clk_levels+0x77/0x80
>>>> [amdgpu] [
>>>> >>>>>>>>> 492.659733] ? preempt_schedule_common+0x18/0x30 [
>>>> 492.660713]
>>>> >>>>>>>>> smu_print_ppclk_levels+0x65/0x90 [amdgpu] [ 492.662107]
>>>> >>>>>>>>> amdgpu_get_pp_od_clk_voltage+0x13d/0x190 [amdgpu] [
>>>> 492.663620]
>>>> >>>>>>>>> dev_attr_show+0x1d/0x40
>>>> >>>>>>>>
>>>> >>>>>>>> Mhm, that at least partially doesn't looks like the right
>>>> >>>>>>>> approach to me.
>>>> >>>>>>>>
>>>> >>>>>>>> Why do we have string printing and sysfs code in the
>>>> hardware
>>>> >>>>>>>> version specific backend in the first place?
>>>> >>>>>>>>
>>>> >>>>>>>
>>>> >>>>>>> This is a callback meant for printing ASIC specific
>>>> information to
>>>> >>>>>>> sysfs node. The buffer passed in sysfs read is passed as
>>>> it is
>>>> >>>>>>> to the callback API.
>>>> >>>>>>>
>>>> >>>>>>>> That stuff needs to be implemented for each hardware
>>>> generation
>>>> >>>>>>>> and
>>>> >>>>>>>> is now cluttered with sysfs buffer offset calculations.
>>>> >>>>>>>>
>>>> >>>>>>>
>>>> >>>>>>> Looks like the warning happened because of this usage.
>>>> >>>>>>>
>>>> >>>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>> >>>>>>> OD_SCLK, buf);
>>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>> >>>>>>> OD_MCLK,
>>>> >>>>>>> buf+size);
>>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>> >>>>>>> OD_VDDC_CURVE, buf+size);
>>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>> >>>>>>> OD_VDDGFX_OFFSET, buf+size);
>>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>> >>>>>>> OD_RANGE,
>>>> >>>>>>> buf+size);
>>>> >>>>>>> size += amdgpu_dpm_print_clock_levels(adev,
>>>> >>>>>>> OD_CCLK,
>>>> >>>>>>> buf+size);
>>>> >>>>>>>
>>>> >>>>>>>
>>>> >>>>>> [Yu, Lang]
>>>> >>>>>> Yes. So it is fine we just fix the caller
>>>> >>>>>> amdgpu_get_pp_od_clk_voltage like
>>>> >>>>> following:
>>>> >>>>>>
>>>> >>>>>> static ssize_t amdgpu_get_pp_od_clk_voltage(struct device
>>>> *dev,
>>>> >>>>>> struct device_attribute *attr,
>>>> >>>>>> char *buf)
>>>> >>>>>> {
>>>> >>>>>> struct drm_device *ddev = dev_get_drvdata(dev);
>>>> >>>>>> struct amdgpu_device *adev = drm_to_adev(ddev);
>>>> >>>>>> ssize_t size, offset;
>>>> >>>>>> int ret, i;
>>>> >>>>>> char temp_buf[512];
>>>> >>>>>> char clock_type[] = {OD_SCLK, OD_MCLK, OD_VDDC_CURVE,
>>>> >>>>>> OD_VDDGFX_OFFSET, OD_RANGE, OD_CCLK};
>>>> >>>>>>
>>>> >>>>>> if (amdgpu_in_reset(adev))
>>>> >>>>>> return -EPERM;
>>>> >>>>>> if (adev->in_suspend && !adev->in_runpm)
>>>> >>>>>> return -EPERM;
>>>> >>>>>>
>>>> >>>>>> ret = pm_runtime_get_sync(ddev->dev);
>>>> >>>>>> if (ret < 0) {
>>>> >>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>> >>>>>> return ret;
>>>> >>>>>> }
>>>> >>>>>>
>>>> >>>>>> offset = 0;
>>>> >>>>>>
>>>> >>>>>> if (adev->powerplay.pp_funcs->print_clock_levels) {
>>>> >>>>>> for (i = 0; i < ARRAY_SIZE(clock_type); i++) {
>>>> >>>>>> size = amdgpu_dpm_print_clock_levels(adev,
>>>> >>>>> clock_type[i], buf);
>>>> >>>>>> if (offset + size > PAGE_SIZE)
>>>> >>>>>> break;
>>>> >>>>>> memcpy(temp_buf + offset, buf, size);
>>>> >>>>>> offset += size;
>>>> >>>>>> }
>>>> >>>>>> memcpy(buf, temp_buf, offset);
>>>> >>>>>> size = offset;
>>>> >>>>>> } else {
>>>> >>>>>> size = sysfs_emit(buf, "\n");
>>>> >>>>>> }
>>>> >>>>>> pm_runtime_mark_last_busy(ddev->dev);
>>>> >>>>>> pm_runtime_put_autosuspend(ddev->dev);
>>>> >>>>>>
>>>> >>>>>> return size;
>>>> >>>>>> }
>>>> >>>>>>
>>>> >>>>> Prefer to avoid any extra stack or heap usage for buffer. Maybe
>>>> >>>>> another arg to
>>>> >>>>> pass offset along with buf?
>>>> >>>>>
>>>> >>>> [Yu, Lang]
>>>> >>>> Actually, the buf address contains the
>>>> offset(offset_in_page(buf)) .
>>>> >>>
>>>> >>> Though it's not a problem based on codeflow, static analysis
>>>> tools
>>>> >>> might complain.
>>>> >>>
>>>> >>>> Or we just rollback to sprintf/snprintf.
>>>> >>>>
>>>> >>>
>>>> >>> snprintf with (PAGE_SIZE-size) may be simpler. I think Darren
>>>> took
>>>> >>> the effort to convert these, he may have some other ideas.
>>>> The changes I made were intended to simply replace snprintf with
>>>> sysfs_emit as per
>>>>
>>>> linux/Documentation/filesystems/sysfs.rst:246,247
>>>>
>>>> - show() should only use sysfs_emit() or sysfs_emit_at() when
>>>> formatting
>>>>
>>>> the value to be returned to user space.
>>>>
>>>> I specifically tried not to change the design, but only as I didn't
>>>> have
>>>>
>>>> background in Power Management.
>>>>
>>>> >>
>>>> >> This is not what I meant. See from the design point of view the
>>>> >> print_clock_levels() callback is the bad idea to begin with.
>>>> >>
>>>> >> What we should have instead is a callback which returns the
>>>> clock as
>>>> >> a value which is then printed in the
>>>> amdgpu_get_pp_od_clk_voltage()
>>>> >> function.
>>>> >>
>>>> >> This avoids passing around the buffer and remaining size
>>>> everywhere
>>>> >> and also guarantees that the sysfs have unified printing over all
>>>> >> hardware generations.
>>>> >>
>>>> >
>>>> > The scenario is one node used for multiple parameters - OD_SCLK,
>>>> > OD_CCLK, OD_VDDGFX_OFFSET etc.(mostly to avoid cluttering sysfs
>>>> with
>>>> > lots of nodes). On top of it, the parameters supported (for ex:
>>>> CCLK
>>>> > is not valid on dGPUs), the number of levels supported etc. vary
>>>> > across ASICs. There has to be multiple calls or the call needs to
>>>> > return multiple values for a single parameter (for ex: up to 4,
>>>> 8 or
>>>> > 16 levels of GFXCLK depending on ASIC).
>>>>
>>>> Well exactly that is questionable design for sysfs.
>>>>
>>>> See the sysfs_emit() and sysfs_emit_at() functions are designed the
>>>> way
>>>> they are because you should have only one value per file, which is
>>>> then
>>>> printed at exactly one location.
>>>>
>>>> Take a look at the documentation for sysfs for more details.
>>>>
>>>> > I don't know the history of the callback, mostly it was considered
>>>> > more efficient to print it directly rather than fetch and print.
>>>> > Alex/Evan may know the details.
>>>>
>>>> Yeah, somebody with a bit more background in power management needs to
>>>> take a closer look at this here. Just keep me looped in.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> >
>>>> > Thanks,
>>>> > Lijo
>>>> >
>>>> >> Regards,
>>>> >> Christian.
>>>> >>
>>>>
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-09-09 8:50 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-08 5:56 [PATCH] drm/amdgpu: fix sysfs_emit/sysfs_emit_at warnings Lang Yu
2021-09-08 6:37 ` Christian König
2021-09-08 7:36 ` Lazar, Lijo
2021-09-08 7:44 ` Yu, Lang
2021-09-08 8:55 ` Lazar, Lijo
2021-09-08 9:02 ` Yu, Lang
2021-09-08 9:29 ` Lazar, Lijo
2021-09-08 9:38 ` Christian König
2021-09-08 10:22 ` Lazar, Lijo
2021-09-08 10:55 ` Yu, Lang
2021-09-08 12:40 ` Christian König
2021-09-08 12:43 ` Christian König
2021-09-08 22:17 ` Powell, Darren
2021-09-09 2:43 ` Yu, Lang
2021-09-09 3:28 ` Lazar, Lijo
2021-09-09 8:01 ` Christian König
2021-09-09 8:36 ` Lazar, Lijo
2021-09-09 8:50 ` Christian König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.