From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Alex Deucher <alexander.deucher@amd.com>, amd-gfx@lists.freedesktop.org
Cc: Lang Yu <lang.yu@amd.com>, Darren Powell <darren.powell@amd.com>
Subject: Re: [PATCH] drm/amdgpu/powerplay: fix sysfs_emit/sysfs_emit_at handling
Date: Thu, 4 Nov 2021 08:57:07 +0100 [thread overview]
Message-ID: <c0ce50c3-01ae-b727-873c-16fbb8de7c41@gmail.com> (raw)
In-Reply-To: <20211104005852.2454705-1-alexander.deucher@amd.com>
As discussed previous I don't think this is the right approach.
The distinction between sysfs_emit() and sysfs_emit_at() is exactly to
avoid that kind of stuff.
Instead we should probably add the size parameter to the functions in
question and so fix the calling convention.
Or even better move the sysfs print out of the powerplay backend.
Regards,
Christian.
Am 04.11.21 um 01:58 schrieb Alex Deucher:
> sysfs_emit and sysfs_emit_at requrie a page boundary
> aligned buf address. Make them happy!
>
> v2: fix sysfs_emit -> sysfs_emit_at missed conversions
>
> Cc: Lang Yu <lang.yu@amd.com>
> Cc: Darren Powell <darren.powell@amd.com>
> Fixes: 6db0c87a0a8e ("amdgpu/pm: Replace hwmgr smu usage of sprintf with sysfs_emit")
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1774
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> .../gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c | 8 ++++++--
> .../gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c | 10 +++++++---
> .../gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c | 2 ++
> .../gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.h | 13 +++++++++++++
> .../gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c | 12 +++++++++---
> .../gpu/drm/amd/pm/powerplay/hwmgr/vega12_hwmgr.c | 4 ++++
> .../gpu/drm/amd/pm/powerplay/hwmgr/vega20_hwmgr.c | 14 ++++++++++----
> 7 files changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c
> index 1de3ae77e03e..258c573acc97 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c
> @@ -1024,6 +1024,8 @@ static int smu10_print_clock_levels(struct pp_hwmgr *hwmgr,
> uint32_t min_freq, max_freq = 0;
> uint32_t ret = 0;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> switch (type) {
> case PP_SCLK:
> smum_send_msg_to_smc(hwmgr, PPSMC_MSG_GetGfxclkFrequency, &now);
> @@ -1065,7 +1067,7 @@ static int smu10_print_clock_levels(struct pp_hwmgr *hwmgr,
> if (ret)
> return ret;
>
> - 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",
> (data->gfx_actual_soft_min_freq > 0) ? data->gfx_actual_soft_min_freq : min_freq);
> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
> @@ -1081,7 +1083,7 @@ static int smu10_print_clock_levels(struct pp_hwmgr *hwmgr,
> if (ret)
> return ret;
>
> - 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",
> min_freq, max_freq);
> }
> @@ -1456,6 +1458,8 @@ static int smu10_get_power_profile_mode(struct pp_hwmgr *hwmgr, char *buf)
> if (!buf)
> return -EINVAL;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> size += sysfs_emit_at(buf, size, "%s %16s %s %s %s %s\n",title[0],
> title[1], title[2], title[3], title[4], title[5]);
>
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c
> index e7803ce8f67a..aceebf584225 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c
> @@ -4914,6 +4914,8 @@ static int smu7_print_clock_levels(struct pp_hwmgr *hwmgr,
> int size = 0;
> uint32_t i, now, clock, pcie_speed;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> switch (type) {
> case PP_SCLK:
> smum_send_msg_to_smc(hwmgr, PPSMC_MSG_API_GetSclkFrequency, &clock);
> @@ -4963,7 +4965,7 @@ static int smu7_print_clock_levels(struct pp_hwmgr *hwmgr,
> break;
> case OD_SCLK:
> if (hwmgr->od_enabled) {
> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
> for (i = 0; i < odn_sclk_table->num_of_pl; i++)
> size += sysfs_emit_at(buf, size, "%d: %10uMHz %10umV\n",
> i, odn_sclk_table->entries[i].clock/100,
> @@ -4972,7 +4974,7 @@ static int smu7_print_clock_levels(struct pp_hwmgr *hwmgr,
> break;
> case OD_MCLK:
> if (hwmgr->od_enabled) {
> - size = sysfs_emit(buf, "%s:\n", "OD_MCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_MCLK");
> for (i = 0; i < odn_mclk_table->num_of_pl; i++)
> size += sysfs_emit_at(buf, size, "%d: %10uMHz %10umV\n",
> i, odn_mclk_table->entries[i].clock/100,
> @@ -4981,7 +4983,7 @@ static int smu7_print_clock_levels(struct pp_hwmgr *hwmgr,
> break;
> case OD_RANGE:
> if (hwmgr->od_enabled) {
> - 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",
> data->golden_dpm_table.sclk_table.dpm_levels[0].value/100,
> hwmgr->platform_descriptor.overdriveLimit.engineClock/100);
> @@ -5518,6 +5520,8 @@ static int smu7_get_power_profile_mode(struct pp_hwmgr *hwmgr, char *buf)
> if (!buf)
> return -EINVAL;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> size += sysfs_emit_at(buf, size, "%s %16s %16s %16s %16s %16s %16s %16s\n",
> title[0], title[1], title[2], title[3],
> title[4], title[5], title[6], title[7]);
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
> index b94a77e4e714..8e28a8eecefc 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
> @@ -1550,6 +1550,8 @@ static int smu8_print_clock_levels(struct pp_hwmgr *hwmgr,
> uint32_t i, now;
> int size = 0;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> switch (type) {
> case PP_SCLK:
> now = PHM_GET_FIELD(cgs_read_ind_register(hwmgr->device,
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.h b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.h
> index ad33983a8064..2a75da1e9f03 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.h
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.h
> @@ -109,6 +109,19 @@ int phm_irq_process(struct amdgpu_device *adev,
> struct amdgpu_irq_src *source,
> struct amdgpu_iv_entry *entry);
>
> +/*
> + * Helper function to make sysfs_emit_at() happy. Align buf to
> + * the current page boundary and record the offset.
> + */
> +static inline void phm_get_sysfs_buf(char **buf, int *offset)
> +{
> + if (!*buf || !offset)
> + return;
> +
> + *offset = offset_in_page(*buf);
> + *buf -= *offset;
> +}
> +
> int smu9_register_irq_handlers(struct pp_hwmgr *hwmgr);
>
> void *smu_atom_get_data_table(void *dev, uint32_t table, uint16_t *size,
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
> index c152a61ddd2c..c981fc2882f0 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
> @@ -4548,6 +4548,8 @@ static int vega10_get_ppfeature_status(struct pp_hwmgr *hwmgr, char *buf)
> int ret = 0;
> int size = 0;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> ret = vega10_get_enabled_smc_features(hwmgr, &features_enabled);
> PP_ASSERT_WITH_CODE(!ret,
> "[EnableAllSmuFeatures] Failed to get enabled smc features!",
> @@ -4637,6 +4639,8 @@ static int vega10_print_clock_levels(struct pp_hwmgr *hwmgr,
>
> int i, now, size = 0, count = 0;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> switch (type) {
> case PP_SCLK:
> if (data->registry_data.sclk_dpm_key_disabled)
> @@ -4717,7 +4721,7 @@ static int vega10_print_clock_levels(struct pp_hwmgr *hwmgr,
>
> case OD_SCLK:
> if (hwmgr->od_enabled) {
> - size = sysfs_emit(buf, "%s:\n", "OD_SCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_SCLK");
> podn_vdd_dep = &data->odn_dpm_table.vdd_dep_on_sclk;
> for (i = 0; i < podn_vdd_dep->count; i++)
> size += sysfs_emit_at(buf, size, "%d: %10uMhz %10umV\n",
> @@ -4727,7 +4731,7 @@ static int vega10_print_clock_levels(struct pp_hwmgr *hwmgr,
> break;
> case OD_MCLK:
> if (hwmgr->od_enabled) {
> - size = sysfs_emit(buf, "%s:\n", "OD_MCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_MCLK");
> podn_vdd_dep = &data->odn_dpm_table.vdd_dep_on_mclk;
> for (i = 0; i < podn_vdd_dep->count; i++)
> size += sysfs_emit_at(buf, size, "%d: %10uMhz %10umV\n",
> @@ -4737,7 +4741,7 @@ static int vega10_print_clock_levels(struct pp_hwmgr *hwmgr,
> break;
> case OD_RANGE:
> if (hwmgr->od_enabled) {
> - 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",
> data->golden_dpm_table.gfx_table.dpm_levels[0].value/100,
> hwmgr->platform_descriptor.overdriveLimit.engineClock/100);
> @@ -5112,6 +5116,8 @@ static int vega10_get_power_profile_mode(struct pp_hwmgr *hwmgr, char *buf)
> if (!buf)
> return -EINVAL;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> size += sysfs_emit_at(buf, size, "%s %16s %s %s %s %s\n",title[0],
> title[1], title[2], title[3], title[4], title[5]);
>
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega12_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega12_hwmgr.c
> index 8558718e15a8..f7e783e1c888 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega12_hwmgr.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega12_hwmgr.c
> @@ -2141,6 +2141,8 @@ static int vega12_get_ppfeature_status(struct pp_hwmgr *hwmgr, char *buf)
> int ret = 0;
> int size = 0;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> ret = vega12_get_enabled_smc_features(hwmgr, &features_enabled);
> PP_ASSERT_WITH_CODE(!ret,
> "[EnableAllSmuFeatures] Failed to get enabled smc features!",
> @@ -2244,6 +2246,8 @@ static int vega12_print_clock_levels(struct pp_hwmgr *hwmgr,
> int i, now, size = 0;
> struct pp_clock_levels_with_latency clocks;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> switch (type) {
> case PP_SCLK:
> PP_ASSERT_WITH_CODE(
> diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega20_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega20_hwmgr.c
> index 0cf39c1244b1..03e63be4ee27 100644
> --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega20_hwmgr.c
> +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega20_hwmgr.c
> @@ -3238,6 +3238,8 @@ static int vega20_get_ppfeature_status(struct pp_hwmgr *hwmgr, char *buf)
> int ret = 0;
> int size = 0;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> ret = vega20_get_enabled_smc_features(hwmgr, &features_enabled);
> PP_ASSERT_WITH_CODE(!ret,
> "[EnableAllSmuFeatures] Failed to get enabled smc features!",
> @@ -3364,6 +3366,8 @@ static int vega20_print_clock_levels(struct pp_hwmgr *hwmgr,
> int ret = 0;
> uint32_t gen_speed, lane_width, current_gen_speed, current_lane_width;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> switch (type) {
> case PP_SCLK:
> ret = vega20_get_current_clk_freq(hwmgr, PPCLK_GFXCLK, &now);
> @@ -3479,7 +3483,7 @@ static int vega20_print_clock_levels(struct pp_hwmgr *hwmgr,
> case OD_SCLK:
> if (od8_settings[OD8_SETTING_GFXCLK_FMIN].feature_id &&
> od8_settings[OD8_SETTING_GFXCLK_FMAX].feature_id) {
> - 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",
> od_table->GfxclkFmin);
> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
> @@ -3489,7 +3493,7 @@ static int vega20_print_clock_levels(struct pp_hwmgr *hwmgr,
>
> case OD_MCLK:
> if (od8_settings[OD8_SETTING_UCLK_FMAX].feature_id) {
> - size = sysfs_emit(buf, "%s:\n", "OD_MCLK");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_MCLK");
> size += sysfs_emit_at(buf, size, "1: %10uMhz\n",
> od_table->UclkFmax);
> }
> @@ -3503,7 +3507,7 @@ static int vega20_print_clock_levels(struct pp_hwmgr *hwmgr,
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE1].feature_id &&
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE2].feature_id &&
> od8_settings[OD8_SETTING_GFXCLK_VOLTAGE3].feature_id) {
> - size = sysfs_emit(buf, "%s:\n", "OD_VDDC_CURVE");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_VDDC_CURVE");
> size += sysfs_emit_at(buf, size, "0: %10uMhz %10dmV\n",
> od_table->GfxclkFreq1,
> od_table->GfxclkVolt1 / VOLTAGE_SCALE);
> @@ -3518,7 +3522,7 @@ static int vega20_print_clock_levels(struct pp_hwmgr *hwmgr,
> break;
>
> case OD_RANGE:
> - size = sysfs_emit(buf, "%s:\n", "OD_RANGE");
> + size += sysfs_emit_at(buf, size, "%s:\n", "OD_RANGE");
>
> if (od8_settings[OD8_SETTING_GFXCLK_FMIN].feature_id &&
> od8_settings[OD8_SETTING_GFXCLK_FMAX].feature_id) {
> @@ -4003,6 +4007,8 @@ static int vega20_get_power_profile_mode(struct pp_hwmgr *hwmgr, char *buf)
> if (!buf)
> return -EINVAL;
>
> + phm_get_sysfs_buf(&buf, &size);
> +
> size += sysfs_emit_at(buf, size, "%16s %s %s %s %s %s %s %s %s %s %s\n",
> title[0], title[1], title[2], title[3], title[4], title[5],
> title[6], title[7], title[8], title[9], title[10]);
prev parent reply other threads:[~2021-11-04 7:57 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-04 0:58 [PATCH] drm/amdgpu/powerplay: fix sysfs_emit/sysfs_emit_at handling Alex Deucher
2021-11-04 2:22 ` Yu, Lang
2021-11-04 7:57 ` Christian König [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c0ce50c3-01ae-b727-873c-16fbb8de7c41@gmail.com \
--to=ckoenig.leichtzumerken@gmail.com \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=darren.powell@amd.com \
--cc=lang.yu@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox