[AMD Official Use Only]
>-----Original Message-----
>From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
>Sent: Wednesday, September 8, 2021 4:55 PM
>To: Yu, Lang <Lang.Yu@xxxxxxx>; Christian König
><ckoenig.leichtzumerken@xxxxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Ray
><Ray.Huang@xxxxxxx>; Tian Tao <tiantao6@xxxxxxxxxxxxx>
>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@xxxxxxx>
>>> Sent: Wednesday, September 8, 2021 3:36 PM
>>> To: Christian König <ckoenig.leichtzumerken@xxxxxxxxx>; Yu, Lang
>>> <Lang.Yu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>>> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Ray
>>> <Ray.Huang@xxxxxxx>; Tian Tao <tiantao6@xxxxxxxxxxxxx>
>>> 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@xxxxxxx>
>>>>> ---
>>>>> 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;
>>>>
