* [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
@ 2021-07-22 3:20 Evan Quan
2021-07-22 3:20 ` [PATCH 2/2] drm/amd/pm: restore user customized OD settings properly for Sienna Cichlid Evan Quan
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Evan Quan @ 2021-07-22 3:20 UTC (permalink / raw)
To: amd-gfx; +Cc: Alexander.Deucher, Evan Quan
The customized OD settings can be divided into two parts: those
committed ones and non-committed ones.
- For those changes which had been fed to SMU before S3/S4/Runpm
suspend kicked, they are committed changes. They should be properly
restored and fed to SMU on S3/S4/Runpm resume.
- For those non-committed changes, they are restored only without feeding
to SMU.
Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
Signed-off-by: Evan Quan <evan.quan@amd.com>
---
drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 ++++
.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52 ++++++++++++++-----
.../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 12 +++++
4 files changed, 69 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 3e89852e4820..8ba53f16d2d9 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
uint32_t power_limit;
uint32_t fan_speed_percent;
uint32_t flags;
+ uint32_t committed_od;
/* user clock state information */
uint32_t clk_mask[SMU_CLK_COUNT];
@@ -352,6 +353,7 @@ struct smu_table_context
void *overdrive_table;
void *boot_overdrive_table;
+ void *committed_overdrive_table;
uint32_t gpu_metrics_table_size;
void *gpu_metrics_table;
@@ -623,6 +625,12 @@ struct pptable_funcs {
enum PP_OD_DPM_TABLE_COMMAND type,
long *input, uint32_t size);
+ /**
+ * @restore_committed_od_settings: Restore the customized and committed
+ * OD settings on S3/S4/Runpm resume.
+ */
+ int (*restore_committed_od_settings)(struct smu_context *smu);
+
/**
* @get_clock_by_type_with_latency: Get the speed and latency of a clock
* domain.
diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index ebe672142808..5f7d98e99f76 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -416,6 +416,15 @@ static void smu_restore_dpm_user_profile(struct smu_context *smu)
}
}
+ /* Restore customized and committed OD settings */
+ if (smu->user_dpm_profile.committed_od) {
+ if (smu->ppt_funcs->restore_committed_od_settings) {
+ ret = smu->ppt_funcs->restore_committed_od_settings(smu);
+ if (ret)
+ dev_err(smu->adev->dev, "Failed to upload customized OD settings\n");
+ }
+ }
+
/* Disable restore flag */
smu->user_dpm_profile.flags &= ~SMU_DPM_USER_PROFILE_RESTORE;
}
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 59ea59acfb00..4752299d7f91 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
@@ -2296,39 +2296,45 @@ static int navi10_set_default_od_settings(struct smu_context *smu)
(OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
int ret = 0;
- ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)od_table, false);
+ ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)boot_od_table, false);
if (ret) {
dev_err(smu->adev->dev, "Failed to get overdrive table!\n");
return ret;
}
- if (!od_table->GfxclkVolt1) {
+ if (!boot_od_table->GfxclkVolt1) {
ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
- &od_table->GfxclkVolt1,
- od_table->GfxclkFreq1);
+ &boot_od_table->GfxclkVolt1,
+ boot_od_table->GfxclkFreq1);
if (ret)
return ret;
}
- if (!od_table->GfxclkVolt2) {
+ if (!boot_od_table->GfxclkVolt2) {
ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
- &od_table->GfxclkVolt2,
- od_table->GfxclkFreq2);
+ &boot_od_table->GfxclkVolt2,
+ boot_od_table->GfxclkFreq2);
if (ret)
return ret;
}
- if (!od_table->GfxclkVolt3) {
+ if (!boot_od_table->GfxclkVolt3) {
ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
- &od_table->GfxclkVolt3,
- od_table->GfxclkFreq3);
+ &boot_od_table->GfxclkVolt3,
+ boot_od_table->GfxclkFreq3);
if (ret)
return ret;
}
- memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
+ navi10_dump_od_table(smu, boot_od_table);
- navi10_dump_od_table(smu, od_table);
+ /*
+ * For S3/S4/Runpm, no need to install boot od table to overdrive_table as
+ * - either they already share the same OD settings on bootup
+ * - or they have user customized OD settings
+ */
+ if (!smu->adev->in_suspend)
+ memcpy(od_table, boot_od_table, sizeof(OverDriveTable_t));
return 0;
}
@@ -2435,6 +2441,14 @@ static int navi10_od_edit_dpm_table(struct smu_context *smu, enum PP_OD_DPM_TABL
dev_err(smu->adev->dev, "Failed to import overdrive table!\n");
return ret;
}
+ if (memcmp(table_context->overdrive_table, table_context->boot_overdrive_table,
+ sizeof(OverDriveTable_t))) {
+ smu->user_dpm_profile.committed_od = true;
+ memcpy(table_context->committed_overdrive_table, table_context->overdrive_table,
+ sizeof(OverDriveTable_t));
+ } else {
+ smu->user_dpm_profile.committed_od = false;
+ }
break;
case PP_OD_EDIT_VDDC_CURVE:
if (!navi10_od_feature_is_supported(od_settings, SMU_11_0_ODCAP_GFXCLK_CURVE)) {
@@ -2499,6 +2513,19 @@ static int navi10_od_edit_dpm_table(struct smu_context *smu, enum PP_OD_DPM_TABL
return ret;
}
+static int navi10_restore_committed_od_settings(struct smu_context *smu)
+{
+ struct smu_table_context *table_context = &smu->smu_table;
+ void *od_table = table_context->committed_overdrive_table;
+ int ret = 0;
+
+ ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)od_table, true);
+ if (ret)
+ dev_err(smu->adev->dev, "Failed to import overdrive table!\n");
+
+ return ret;
+}
+
static int navi10_run_btc(struct smu_context *smu)
{
int ret = 0;
@@ -3262,6 +3289,7 @@ static const struct pptable_funcs navi10_ppt_funcs = {
.set_soft_freq_limited_range = smu_v11_0_set_soft_freq_limited_range,
.set_default_od_settings = navi10_set_default_od_settings,
.od_edit_dpm_table = navi10_od_edit_dpm_table,
+ .restore_committed_od_settings = navi10_restore_committed_od_settings,
.run_btc = navi10_run_btc,
.set_power_source = smu_v11_0_set_power_source,
.get_pp_feature_mask = smu_cmn_get_pp_feature_mask,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
index 0a5d46ac9ccd..28fc3f17c1b1 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
@@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct smu_context *smu)
ret = -ENOMEM;
goto err3_out;
}
+
+ smu_table->committed_overdrive_table =
+ kzalloc(tables[SMU_TABLE_OVERDRIVE].size, GFP_KERNEL);
+ if (!smu_table->committed_overdrive_table) {
+ ret = -ENOMEM;
+ goto err4_out;
+ }
+
}
return 0;
+err4_out:
+ kfree(smu_table->boot_overdrive_table);
err3_out:
kfree(smu_table->overdrive_table);
err2_out:
@@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct smu_context *smu)
struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
kfree(smu_table->gpu_metrics_table);
+ kfree(smu_table->committed_overdrive_table);
kfree(smu_table->boot_overdrive_table);
kfree(smu_table->overdrive_table);
kfree(smu_table->max_sustainable_clocks);
kfree(smu_table->driver_pptable);
kfree(smu_table->clocks_table);
smu_table->gpu_metrics_table = NULL;
+ smu_table->committed_overdrive_table = NULL;
smu_table->boot_overdrive_table = NULL;
smu_table->overdrive_table = NULL;
smu_table->max_sustainable_clocks = NULL;
--
2.29.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/amd/pm: restore user customized OD settings properly for Sienna Cichlid
2021-07-22 3:20 [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x Evan Quan
@ 2021-07-22 3:20 ` Evan Quan
2021-07-22 8:10 ` [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x Lazar, Lijo
2021-07-23 19:59 ` Matt Coffin
2 siblings, 0 replies; 11+ messages in thread
From: Evan Quan @ 2021-07-22 3:20 UTC (permalink / raw)
To: amd-gfx; +Cc: Alexander.Deucher, Evan Quan
Properly restore those committed and non-committed user customized OD
settings.
Change-Id: I25396df0b3ecdd7a0d9fc77ed220b0abf1fde020
Signed-off-by: Evan Quan <evan.quan@amd.com>
---
drivers/gpu/drm/amd/pm/inc/smu_v11_0.h | 2 ++
drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 15 +--------------
.../drm/amd/pm/swsmu/smu11/sienna_cichlid_ppt.c | 16 +++++++++++++---
drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 13 +++++++++++++
4 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
index 385b2ea5379c..7bf25efc3936 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
@@ -302,5 +302,7 @@ void smu_v11_0_interrupt_work(struct smu_context *smu);
int smu_v11_0_set_light_sbr(struct smu_context *smu, bool enable);
+int smu_v11_0_restore_committed_od_settings(struct smu_context *smu);
+
#endif
#endif
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 4752299d7f91..fbd29129550a 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
@@ -2513,19 +2513,6 @@ static int navi10_od_edit_dpm_table(struct smu_context *smu, enum PP_OD_DPM_TABL
return ret;
}
-static int navi10_restore_committed_od_settings(struct smu_context *smu)
-{
- struct smu_table_context *table_context = &smu->smu_table;
- void *od_table = table_context->committed_overdrive_table;
- int ret = 0;
-
- ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)od_table, true);
- if (ret)
- dev_err(smu->adev->dev, "Failed to import overdrive table!\n");
-
- return ret;
-}
-
static int navi10_run_btc(struct smu_context *smu)
{
int ret = 0;
@@ -3289,7 +3276,7 @@ static const struct pptable_funcs navi10_ppt_funcs = {
.set_soft_freq_limited_range = smu_v11_0_set_soft_freq_limited_range,
.set_default_od_settings = navi10_set_default_od_settings,
.od_edit_dpm_table = navi10_od_edit_dpm_table,
- .restore_committed_od_settings = navi10_restore_committed_od_settings,
+ .restore_committed_od_settings = smu_v11_0_restore_committed_od_settings,
.run_btc = navi10_run_btc,
.set_power_source = smu_v11_0_set_power_source,
.get_pp_feature_mask = smu_cmn_get_pp_feature_mask,
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 e0638dc3f617..f0a7dc1d1640 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
@@ -1957,15 +1957,16 @@ static int sienna_cichlid_set_default_od_settings(struct smu_context *smu)
int ret = 0;
ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE,
- 0, (void *)od_table, false);
+ 0, (void *)boot_od_table, false);
if (ret) {
dev_err(smu->adev->dev, "Failed to get overdrive table!\n");
return ret;
}
- memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
+ sienna_cichlid_dump_od_table(smu, boot_od_table);
- sienna_cichlid_dump_od_table(smu, od_table);
+ if (!smu->adev->in_suspend)
+ memcpy(od_table, boot_od_table, sizeof(OverDriveTable_t));
return 0;
}
@@ -2136,6 +2137,14 @@ static int sienna_cichlid_od_edit_dpm_table(struct smu_context *smu,
dev_err(smu->adev->dev, "Failed to import overdrive table!\n");
return ret;
}
+ if (memcmp(table_context->overdrive_table, table_context->boot_overdrive_table,
+ sizeof(OverDriveTable_t))) {
+ smu->user_dpm_profile.committed_od = true;
+ memcpy(table_context->committed_overdrive_table, table_context->overdrive_table,
+ sizeof(OverDriveTable_t));
+ } else {
+ smu->user_dpm_profile.committed_od = false;
+ }
break;
case PP_OD_EDIT_VDDGFX_OFFSET:
@@ -3902,6 +3911,7 @@ static const struct pptable_funcs sienna_cichlid_ppt_funcs = {
.set_soft_freq_limited_range = smu_v11_0_set_soft_freq_limited_range,
.set_default_od_settings = sienna_cichlid_set_default_od_settings,
.od_edit_dpm_table = sienna_cichlid_od_edit_dpm_table,
+ .restore_committed_od_settings = smu_v11_0_restore_committed_od_settings,
.run_btc = sienna_cichlid_run_btc,
.set_power_source = smu_v11_0_set_power_source,
.get_pp_feature_mask = smu_cmn_get_pp_feature_mask,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
index 28fc3f17c1b1..323126a7ca49 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
@@ -2113,3 +2113,16 @@ int smu_v11_0_deep_sleep_control(struct smu_context *smu,
return ret;
}
+
+int smu_v11_0_restore_committed_od_settings(struct smu_context *smu)
+{
+ struct smu_table_context *table_context = &smu->smu_table;
+ void *od_table = table_context->committed_overdrive_table;
+ int ret = 0;
+
+ ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)od_table, true);
+ if (ret)
+ dev_err(smu->adev->dev, "Failed to import overdrive table!\n");
+
+ return ret;
+}
--
2.29.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-22 3:20 [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x Evan Quan
2021-07-22 3:20 ` [PATCH 2/2] drm/amd/pm: restore user customized OD settings properly for Sienna Cichlid Evan Quan
@ 2021-07-22 8:10 ` Lazar, Lijo
2021-07-22 8:33 ` Quan, Evan
2021-07-23 19:59 ` Matt Coffin
2 siblings, 1 reply; 11+ messages in thread
From: Lazar, Lijo @ 2021-07-22 8:10 UTC (permalink / raw)
To: Evan Quan, amd-gfx; +Cc: Alexander.Deucher
On 7/22/2021 8:50 AM, Evan Quan wrote:
> The customized OD settings can be divided into two parts: those
> committed ones and non-committed ones.
> - For those changes which had been fed to SMU before S3/S4/Runpm
> suspend kicked, they are committed changes. They should be properly
> restored and fed to SMU on S3/S4/Runpm resume.
> - For those non-committed changes, they are restored only without feeding
> to SMU.
>
> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> ---
> drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 ++++
> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52 ++++++++++++++-----
> .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 12 +++++
> 4 files changed, 69 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> index 3e89852e4820..8ba53f16d2d9 100644
> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> uint32_t power_limit;
> uint32_t fan_speed_percent;
> uint32_t flags;
> + uint32_t committed_od;
>
> /* user clock state information */
> uint32_t clk_mask[SMU_CLK_COUNT];
> @@ -352,6 +353,7 @@ struct smu_table_context
>
> void *overdrive_table;
> void *boot_overdrive_table;
> + void *committed_overdrive_table;
May be rename it to user_overdrive_table - OD table with user settings?
> uint32_t gpu_metrics_table_size;
> void *gpu_metrics_table;
> @@ -623,6 +625,12 @@ struct pptable_funcs {
> enum PP_OD_DPM_TABLE_COMMAND type,
> long *input, uint32_t size);
>
> + /**
> + * @restore_committed_od_settings: Restore the customized and committed
> + * OD settings on S3/S4/Runpm resume.
> + */
> + int (*restore_committed_od_settings)(struct smu_context *smu);
> +
> /**
> * @get_clock_by_type_with_latency: Get the speed and latency of a clock
> * domain.
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index ebe672142808..5f7d98e99f76 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -416,6 +416,15 @@ static void smu_restore_dpm_user_profile(struct smu_context *smu)
> }
> }
>
> + /* Restore customized and committed OD settings */
> + if (smu->user_dpm_profile.committed_od) {
> + if (smu->ppt_funcs->restore_committed_od_settings) {
> + ret = smu->ppt_funcs->restore_committed_od_settings(smu);
> + if (ret)
> + dev_err(smu->adev->dev, "Failed to upload customized OD settings\n");
> + }
> + }
> +
Just thinking if there is a need to handle it ASIC specific. The flags
and table pointer are maintained in common structure. So can't we do the
restore of user OD settings directly in this common flow instead of
having each ASIC to implement the callback?
> /* Disable restore flag */
> smu->user_dpm_profile.flags &= ~SMU_DPM_USER_PROFILE_RESTORE;
> }
> 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 59ea59acfb00..4752299d7f91 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> @@ -2296,39 +2296,45 @@ static int navi10_set_default_od_settings(struct smu_context *smu)
> (OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
> int ret = 0;
>
> - ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)od_table, false);
> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)boot_od_table, false);
> if (ret) {
> dev_err(smu->adev->dev, "Failed to get overdrive table!\n");
> return ret;
> }
>
> - if (!od_table->GfxclkVolt1) {
> + if (!boot_od_table->GfxclkVolt1) {
> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> - &od_table->GfxclkVolt1,
> - od_table->GfxclkFreq1);
> + &boot_od_table->GfxclkVolt1,
> + boot_od_table->GfxclkFreq1);
> if (ret)
> return ret;
> }
>
> - if (!od_table->GfxclkVolt2) {
> + if (!boot_od_table->GfxclkVolt2) {
> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> - &od_table->GfxclkVolt2,
> - od_table->GfxclkFreq2);
> + &boot_od_table->GfxclkVolt2,
> + boot_od_table->GfxclkFreq2);
> if (ret)
> return ret;
> }
>
> - if (!od_table->GfxclkVolt3) {
> + if (!boot_od_table->GfxclkVolt3) {
> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> - &od_table->GfxclkVolt3,
> - od_table->GfxclkFreq3);
> + &boot_od_table->GfxclkVolt3,
> + boot_od_table->GfxclkFreq3);
> if (ret)
> return ret;
> }
>
> - memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
> + navi10_dump_od_table(smu, boot_od_table);
>
> - navi10_dump_od_table(smu, od_table);
> + /*
> + * For S3/S4/Runpm, no need to install boot od table to overdrive_table as
> + * - either they already share the same OD settings on bootup
> + * - or they have user customized OD settings
> + */
> + if (!smu->adev->in_suspend)
> + memcpy(od_table, boot_od_table, sizeof(OverDriveTable_t));
>
> return 0;
> }
> @@ -2435,6 +2441,14 @@ static int navi10_od_edit_dpm_table(struct smu_context *smu, enum PP_OD_DPM_TABL
> dev_err(smu->adev->dev, "Failed to import overdrive table!\n");
> return ret;
> }
> + if (memcmp(table_context->overdrive_table, table_context->boot_overdrive_table,
> + sizeof(OverDriveTable_t))) {
Shouldn't this be - compare against the edited settings and last
committed settings, overdrive_table vs committed_overdrive_table?
Basically, user_od_table can be initialized with boot_od settings and
the flag gives the indication that user_od table is being used or not.
Before updating, check if the edited table (overdrive_table) and the
current user_od table are different, then commit the new table.
> + smu->user_dpm_profile.committed_od = true;
> + memcpy(table_context->committed_overdrive_table, table_context->overdrive_table,
> + sizeof(OverDriveTable_t));
> + } else {
> + smu->user_dpm_profile.committed_od = false;
> + }
> break;
> case PP_OD_EDIT_VDDC_CURVE:
> if (!navi10_od_feature_is_supported(od_settings, SMU_11_0_ODCAP_GFXCLK_CURVE)) {
> @@ -2499,6 +2513,19 @@ static int navi10_od_edit_dpm_table(struct smu_context *smu, enum PP_OD_DPM_TABL
> return ret;
> }
>
> +static int navi10_restore_committed_od_settings(struct smu_context *smu)
> +{
> + struct smu_table_context *table_context = &smu->smu_table;
> + void *od_table = table_context->committed_overdrive_table;
> + int ret = 0;
> +
> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0, (void *)od_table, true);
> + if (ret)
> + dev_err(smu->adev->dev, "Failed to import overdrive table!\n");
> +
> + return ret;
> +}
As mentioned before, not sure if this is needed as callback. Even if,
this can be something common for smuv11, there is nothing ASIC specific
in this logic, right?
Thanks,
Lijo
> static int navi10_run_btc(struct smu_context *smu)
> {
> int ret = 0;
> @@ -3262,6 +3289,7 @@ static const struct pptable_funcs navi10_ppt_funcs = {
> .set_soft_freq_limited_range = smu_v11_0_set_soft_freq_limited_range,
> .set_default_od_settings = navi10_set_default_od_settings,
> .od_edit_dpm_table = navi10_od_edit_dpm_table,
> + .restore_committed_od_settings = navi10_restore_committed_od_settings,
> .run_btc = navi10_run_btc,
> .set_power_source = smu_v11_0_set_power_source,
> .get_pp_feature_mask = smu_cmn_get_pp_feature_mask,
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> index 0a5d46ac9ccd..28fc3f17c1b1 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> @@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct smu_context *smu)
> ret = -ENOMEM;
> goto err3_out;
> }
> +
> + smu_table->committed_overdrive_table =
> + kzalloc(tables[SMU_TABLE_OVERDRIVE].size, GFP_KERNEL);
> + if (!smu_table->committed_overdrive_table) {
> + ret = -ENOMEM;
> + goto err4_out;
> + }
> +
> }
>
> return 0;
>
> +err4_out:
> + kfree(smu_table->boot_overdrive_table);
> err3_out:
> kfree(smu_table->overdrive_table);
> err2_out:
> @@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct smu_context *smu)
> struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
>
> kfree(smu_table->gpu_metrics_table);
> + kfree(smu_table->committed_overdrive_table);
> kfree(smu_table->boot_overdrive_table);
> kfree(smu_table->overdrive_table);
> kfree(smu_table->max_sustainable_clocks);
> kfree(smu_table->driver_pptable);
> kfree(smu_table->clocks_table);
> smu_table->gpu_metrics_table = NULL;
> + smu_table->committed_overdrive_table = NULL;
> smu_table->boot_overdrive_table = NULL;
> smu_table->overdrive_table = NULL;
> smu_table->max_sustainable_clocks = NULL;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-22 8:10 ` [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x Lazar, Lijo
@ 2021-07-22 8:33 ` Quan, Evan
2021-07-22 9:03 ` Lazar, Lijo
0 siblings, 1 reply; 11+ messages in thread
From: Quan, Evan @ 2021-07-22 8:33 UTC (permalink / raw)
To: Lazar, Lijo, amd-gfx; +Cc: Deucher, Alexander
[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Thursday, July 22, 2021 4:10 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
>
>
> On 7/22/2021 8:50 AM, Evan Quan wrote:
> > The customized OD settings can be divided into two parts: those
> > committed ones and non-committed ones.
> > - For those changes which had been fed to SMU before S3/S4/Runpm
> > suspend kicked, they are committed changes. They should be properly
> > restored and fed to SMU on S3/S4/Runpm resume.
> > - For those non-committed changes, they are restored only without
> feeding
> > to SMU.
> >
> > Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > Signed-off-by: Evan Quan <evan.quan@amd.com>
> > ---
> > drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 ++++
> > .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> ++++++++++++++-----
> > .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 12 +++++
> > 4 files changed, 69 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > index 3e89852e4820..8ba53f16d2d9 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> > uint32_t power_limit;
> > uint32_t fan_speed_percent;
> > uint32_t flags;
> > + uint32_t committed_od;
> >
> > /* user clock state information */
> > uint32_t clk_mask[SMU_CLK_COUNT];
> > @@ -352,6 +353,7 @@ struct smu_table_context
> >
> > void *overdrive_table;
> > void *boot_overdrive_table;
> > + void *committed_overdrive_table;
>
> May be rename it to user_overdrive_table - OD table with user settings?
[Quan, Evan] Actually "overdrive_table " is the user_overdrive_table. It contains all the modification including those not committed( the commit is performed by echo "c" > /sys/class/drm/card1/device/pp_od_clk_voltage).
The new member added refers to those user customized but committed only settings. That's why it was named as " committed_overdrive_table". Any good suggestion for the naming?
>
> > uint32_t gpu_metrics_table_size;
> > void *gpu_metrics_table;
> > @@ -623,6 +625,12 @@ struct pptable_funcs {
> > enum PP_OD_DPM_TABLE_COMMAND
> type,
> > long *input, uint32_t size);
> >
> > + /**
> > + * @restore_committed_od_settings: Restore the customized and
> committed
> > + * OD settings on S3/S4/Runpm resume.
> > + */
> > + int (*restore_committed_od_settings)(struct smu_context *smu);
> > +
> > /**
> > * @get_clock_by_type_with_latency: Get the speed and latency of
> a clock
> > * domain.
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index ebe672142808..5f7d98e99f76 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -416,6 +416,15 @@ static void smu_restore_dpm_user_profile(struct
> smu_context *smu)
> > }
> > }
> >
> > + /* Restore customized and committed OD settings */
> > + if (smu->user_dpm_profile.committed_od) {
> > + if (smu->ppt_funcs->restore_committed_od_settings) {
> > + ret = smu->ppt_funcs-
> >restore_committed_od_settings(smu);
> > + if (ret)
> > + dev_err(smu->adev->dev, "Failed to upload
> customized OD settings\n");
> > + }
> > + }
> > +
>
> Just thinking if there is a need to handle it ASIC specific. The flags and table
> pointer are maintained in common structure. So can't we do the restore of
> user OD settings directly in this common flow instead of having each ASIC to
> implement the callback?
[Quan, Evan] The OD settings restoring is ASIC specific as it performed on OD table and that(OD table) is ASIC specific.
>
> > /* Disable restore flag */
> > smu->user_dpm_profile.flags &=
> ~SMU_DPM_USER_PROFILE_RESTORE;
> > }
> > 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 59ea59acfb00..4752299d7f91 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > @@ -2296,39 +2296,45 @@ static int
> navi10_set_default_od_settings(struct smu_context *smu)
> > (OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
> > int ret = 0;
> >
> > - ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> (void *)od_table, false);
> > + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> (void
> > +*)boot_od_table, false);
> > if (ret) {
> > dev_err(smu->adev->dev, "Failed to get overdrive table!\n");
> > return ret;
> > }
> >
> > - if (!od_table->GfxclkVolt1) {
> > + if (!boot_od_table->GfxclkVolt1) {
> > ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > - &od_table-
> >GfxclkVolt1,
> > - od_table-
> >GfxclkFreq1);
> > +
> &boot_od_table->GfxclkVolt1,
> > +
> boot_od_table->GfxclkFreq1);
> > if (ret)
> > return ret;
> > }
> >
> > - if (!od_table->GfxclkVolt2) {
> > + if (!boot_od_table->GfxclkVolt2) {
> > ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > - &od_table-
> >GfxclkVolt2,
> > - od_table-
> >GfxclkFreq2);
> > +
> &boot_od_table->GfxclkVolt2,
> > +
> boot_od_table->GfxclkFreq2);
> > if (ret)
> > return ret;
> > }
> >
> > - if (!od_table->GfxclkVolt3) {
> > + if (!boot_od_table->GfxclkVolt3) {
> > ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > - &od_table-
> >GfxclkVolt3,
> > - od_table-
> >GfxclkFreq3);
> > +
> &boot_od_table->GfxclkVolt3,
> > +
> boot_od_table->GfxclkFreq3);
> > if (ret)
> > return ret;
> > }
> >
> > - memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
> > + navi10_dump_od_table(smu, boot_od_table);
> >
> > - navi10_dump_od_table(smu, od_table);
> > + /*
> > + * For S3/S4/Runpm, no need to install boot od table to
> overdrive_table as
> > + * - either they already share the same OD settings on bootup
> > + * - or they have user customized OD settings
> > + */
> > + if (!smu->adev->in_suspend)
> > + memcpy(od_table, boot_od_table,
> sizeof(OverDriveTable_t));
> >
> > return 0;
> > }
> > @@ -2435,6 +2441,14 @@ static int navi10_od_edit_dpm_table(struct
> smu_context *smu, enum PP_OD_DPM_TABL
> > dev_err(smu->adev->dev, "Failed to import
> overdrive table!\n");
> > return ret;
> > }
> > + if (memcmp(table_context->overdrive_table, table_context-
> >boot_overdrive_table,
> > + sizeof(OverDriveTable_t))) {
>
> Shouldn't this be - compare against the edited settings and last committed
> settings, overdrive_table vs committed_overdrive_table?
>
> Basically, user_od_table can be initialized with boot_od settings and the flag
> gives the indication that user_od table is being used or not.
> Before updating, check if the edited table (overdrive_table) and the current
> user_od table are different, then commit the new table.
[Quan, Evan] Yes, I also considered that. But that cannot handle the case below:
1 user made some customizations -> 2 the customizations were committed -> 3 user restored to default(boot) OD settings(by "echo 'r'") and committed
Although there were some changes between 2 and 3, there is actually no real customized changes compared to default(boot) OD settings.
>
> > + smu->user_dpm_profile.committed_od = true;
> > + memcpy(table_context-
> >committed_overdrive_table, table_context->overdrive_table,
> > + sizeof(OverDriveTable_t));
> > + } else {
> > + smu->user_dpm_profile.committed_od = false;
> > + }
> > break;
> > case PP_OD_EDIT_VDDC_CURVE:
> > if (!navi10_od_feature_is_supported(od_settings,
> > SMU_11_0_ODCAP_GFXCLK_CURVE)) { @@ -2499,6 +2513,19 @@ static int
> navi10_od_edit_dpm_table(struct smu_context *smu, enum
> PP_OD_DPM_TABL
> > return ret;
> > }
> >
> > +static int navi10_restore_committed_od_settings(struct smu_context
> > +*smu) {
> > + struct smu_table_context *table_context = &smu->smu_table;
> > + void *od_table = table_context->committed_overdrive_table;
> > + int ret = 0;
> > +
> > + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> (void *)od_table, true);
> > + if (ret)
> > + dev_err(smu->adev->dev, "Failed to import overdrive
> table!\n");
> > +
> > + return ret;
> > +}
>
> As mentioned before, not sure if this is needed as callback. Even if, this can
> be something common for smuv11, there is nothing ASIC specific in this logic,
> right?
[Quan, Evan] Yes, in patch2 of the series, it was made as a SMUV11 common API.
BR
Evan
>
> Thanks,
> Lijo
>
> > static int navi10_run_btc(struct smu_context *smu)
> > {
> > int ret = 0;
> > @@ -3262,6 +3289,7 @@ static const struct pptable_funcs
> navi10_ppt_funcs = {
> > .set_soft_freq_limited_range =
> smu_v11_0_set_soft_freq_limited_range,
> > .set_default_od_settings = navi10_set_default_od_settings,
> > .od_edit_dpm_table = navi10_od_edit_dpm_table,
> > + .restore_committed_od_settings =
> > +navi10_restore_committed_od_settings,
> > .run_btc = navi10_run_btc,
> > .set_power_source = smu_v11_0_set_power_source,
> > .get_pp_feature_mask = smu_cmn_get_pp_feature_mask, diff --git
> > a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > index 0a5d46ac9ccd..28fc3f17c1b1 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > @@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct
> smu_context *smu)
> > ret = -ENOMEM;
> > goto err3_out;
> > }
> > +
> > + smu_table->committed_overdrive_table =
> > + kzalloc(tables[SMU_TABLE_OVERDRIVE].size,
> GFP_KERNEL);
> > + if (!smu_table->committed_overdrive_table) {
> > + ret = -ENOMEM;
> > + goto err4_out;
> > + }
> > +
> > }
> >
> > return 0;
> >
> > +err4_out:
> > + kfree(smu_table->boot_overdrive_table);
> > err3_out:
> > kfree(smu_table->overdrive_table);
> > err2_out:
> > @@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct
> smu_context *smu)
> > struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
> >
> > kfree(smu_table->gpu_metrics_table);
> > + kfree(smu_table->committed_overdrive_table);
> > kfree(smu_table->boot_overdrive_table);
> > kfree(smu_table->overdrive_table);
> > kfree(smu_table->max_sustainable_clocks);
> > kfree(smu_table->driver_pptable);
> > kfree(smu_table->clocks_table);
> > smu_table->gpu_metrics_table = NULL;
> > + smu_table->committed_overdrive_table = NULL;
> > smu_table->boot_overdrive_table = NULL;
> > smu_table->overdrive_table = NULL;
> > smu_table->max_sustainable_clocks = NULL;
> >
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-22 8:33 ` Quan, Evan
@ 2021-07-22 9:03 ` Lazar, Lijo
2021-07-23 6:47 ` Quan, Evan
0 siblings, 1 reply; 11+ messages in thread
From: Lazar, Lijo @ 2021-07-22 9:03 UTC (permalink / raw)
To: Quan, Evan, amd-gfx; +Cc: Deucher, Alexander
On 7/22/2021 2:03 PM, Quan, Evan wrote:
> [AMD Official Use Only]
>
>
>
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Thursday, July 22, 2021 4:10 PM
>> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
>> properly for NV1x
>>
>>
>>
>> On 7/22/2021 8:50 AM, Evan Quan wrote:
>>> The customized OD settings can be divided into two parts: those
>>> committed ones and non-committed ones.
>>> - For those changes which had been fed to SMU before S3/S4/Runpm
>>> suspend kicked, they are committed changes. They should be properly
>>> restored and fed to SMU on S3/S4/Runpm resume.
>>> - For those non-committed changes, they are restored only without
>> feeding
>>> to SMU.
>>>
>>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
>>> Signed-off-by: Evan Quan <evan.quan@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
>>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 ++++
>>> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
>> ++++++++++++++-----
>>> .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 12 +++++
>>> 4 files changed, 69 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> index 3e89852e4820..8ba53f16d2d9 100644
>>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
>>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
>>> uint32_t power_limit;
>>> uint32_t fan_speed_percent;
>>> uint32_t flags;
>>> + uint32_t committed_od;
>>>
>>> /* user clock state information */
>>> uint32_t clk_mask[SMU_CLK_COUNT];
>>> @@ -352,6 +353,7 @@ struct smu_table_context
>>>
>>> void *overdrive_table;
>>> void *boot_overdrive_table;
>>> + void *committed_overdrive_table;
>>
>> May be rename it to user_overdrive_table - OD table with user settings?
> [Quan, Evan] Actually "overdrive_table " is the user_overdrive_table. It contains all the modification including those not committed( the commit is performed by echo "c" > /sys/class/drm/card1/device/pp_od_clk_voltage).
> The new member added refers to those user customized but committed only settings. That's why it was named as " committed_overdrive_table". Any good suggestion for the naming?
As far as I understand, the problem is overdrive_table can have
intemediary settings as the edit/commit is a two-step process. At any
point we can have user_od_table keep the latest user mode settings. That
is true when user restores to default settings also; that time we can
just copy the boot settings back to user_od table and keep the flag as
false indicating that there is no custom settings.
>>
>>> uint32_t gpu_metrics_table_size;
>>> void *gpu_metrics_table;
>>> @@ -623,6 +625,12 @@ struct pptable_funcs {
>>> enum PP_OD_DPM_TABLE_COMMAND
>> type,
>>> long *input, uint32_t size);
>>>
>>> + /**
>>> + * @restore_committed_od_settings: Restore the customized and
>> committed
>>> + * OD settings on S3/S4/Runpm resume.
>>> + */
>>> + int (*restore_committed_od_settings)(struct smu_context *smu);
>>> +
>>> /**
>>> * @get_clock_by_type_with_latency: Get the speed and latency of
>> a clock
>>> * domain.
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> index ebe672142808..5f7d98e99f76 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> @@ -416,6 +416,15 @@ static void smu_restore_dpm_user_profile(struct
>> smu_context *smu)
>>> }
>>> }
>>>
>>> + /* Restore customized and committed OD settings */
>>> + if (smu->user_dpm_profile.committed_od) {
>>> + if (smu->ppt_funcs->restore_committed_od_settings) {
>>> + ret = smu->ppt_funcs-
>>> restore_committed_od_settings(smu);
>>> + if (ret)
>>> + dev_err(smu->adev->dev, "Failed to upload
>> customized OD settings\n");
>>> + }
>>> + }
>>> +
>>
>> Just thinking if there is a need to handle it ASIC specific. The flags and table
>> pointer are maintained in common structure. So can't we do the restore of
>> user OD settings directly in this common flow instead of having each ASIC to
>> implement the callback?
> [Quan, Evan] The OD settings restoring is ASIC specific as it performed on OD table and that(OD table) is ASIC specific.
I was thinking in terms of final logic that is being done. The below
structures are available in common table context and we have a common
logic to update the table. If there is no custom OD settings available,
we could handle it with the flag.
+ struct smu_table_context *table_context = &smu->smu_table;
+ void *od_table = table_context->committed_overdrive_table;
+ int ret = 0;
+
+ ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
(void *)od_table, true);
>>
>>> /* Disable restore flag */
>>> smu->user_dpm_profile.flags &=
>> ~SMU_DPM_USER_PROFILE_RESTORE;
>>> }
>>> 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 59ea59acfb00..4752299d7f91 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
>>> @@ -2296,39 +2296,45 @@ static int
>> navi10_set_default_od_settings(struct smu_context *smu)
>>> (OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
>>> int ret = 0;
>>>
>>> - ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
>> (void *)od_table, false);
>>> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
>> (void
>>> +*)boot_od_table, false);
>>> if (ret) {
>>> dev_err(smu->adev->dev, "Failed to get overdrive table!\n");
>>> return ret;
>>> }
>>>
>>> - if (!od_table->GfxclkVolt1) {
>>> + if (!boot_od_table->GfxclkVolt1) {
>>> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
>>> - &od_table-
>>> GfxclkVolt1,
>>> - od_table-
>>> GfxclkFreq1);
>>> +
>> &boot_od_table->GfxclkVolt1,
>>> +
>> boot_od_table->GfxclkFreq1);
>>> if (ret)
>>> return ret;
>>> }
>>>
>>> - if (!od_table->GfxclkVolt2) {
>>> + if (!boot_od_table->GfxclkVolt2) {
>>> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
>>> - &od_table-
>>> GfxclkVolt2,
>>> - od_table-
>>> GfxclkFreq2);
>>> +
>> &boot_od_table->GfxclkVolt2,
>>> +
>> boot_od_table->GfxclkFreq2);
>>> if (ret)
>>> return ret;
>>> }
>>>
>>> - if (!od_table->GfxclkVolt3) {
>>> + if (!boot_od_table->GfxclkVolt3) {
>>> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
>>> - &od_table-
>>> GfxclkVolt3,
>>> - od_table-
>>> GfxclkFreq3);
>>> +
>> &boot_od_table->GfxclkVolt3,
>>> +
>> boot_od_table->GfxclkFreq3);
>>> if (ret)
>>> return ret;
>>> }
>>>
>>> - memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
>>> + navi10_dump_od_table(smu, boot_od_table);
>>>
>>> - navi10_dump_od_table(smu, od_table);
>>> + /*
>>> + * For S3/S4/Runpm, no need to install boot od table to
>> overdrive_table as
>>> + * - either they already share the same OD settings on bootup
>>> + * - or they have user customized OD settings
>>> + */
>>> + if (!smu->adev->in_suspend)
>>> + memcpy(od_table, boot_od_table,
>> sizeof(OverDriveTable_t));
>>>
>>> return 0;
>>> }
>>> @@ -2435,6 +2441,14 @@ static int navi10_od_edit_dpm_table(struct
>> smu_context *smu, enum PP_OD_DPM_TABL
>>> dev_err(smu->adev->dev, "Failed to import
>> overdrive table!\n");
>>> return ret;
>>> }
>>> + if (memcmp(table_context->overdrive_table, table_context-
>>> boot_overdrive_table,
>>> + sizeof(OverDriveTable_t))) {
>>
>> Shouldn't this be - compare against the edited settings and last committed
>> settings, overdrive_table vs committed_overdrive_table?
>>
>> Basically, user_od_table can be initialized with boot_od settings and the flag
>> gives the indication that user_od table is being used or not.
>> Before updating, check if the edited table (overdrive_table) and the current
>> user_od table are different, then commit the new table.
> [Quan, Evan] Yes, I also considered that. But that cannot handle the case below:
> 1 user made some customizations -> 2 the customizations were committed -> 3 user restored to default(boot) OD settings(by "echo 'r'") and committed
> Although there were some changes between 2 and 3, there is actually no real customized changes compared to default(boot) OD settings.
On restore to default, just copy the boot_table settings to user_od and
keep the flag as false. We are using user_od as the latest user
preferred settings and overdrive_table is only used for intermediate
updates.
The flag decides whether to restore or not, but at any point we can
refer the user_od table to look upon the latest preferred user settings
(default or custom).
Thanks,
Lijo
>>
>>> + smu->user_dpm_profile.committed_od = true;
>>> + memcpy(table_context-
>>> committed_overdrive_table, table_context->overdrive_table,
>>> + sizeof(OverDriveTable_t));
>>> + } else {
>>> + smu->user_dpm_profile.committed_od = false;
>>> + }
>>> break;
>>> case PP_OD_EDIT_VDDC_CURVE:
>>> if (!navi10_od_feature_is_supported(od_settings,
>>> SMU_11_0_ODCAP_GFXCLK_CURVE)) { @@ -2499,6 +2513,19 @@ static int
>> navi10_od_edit_dpm_table(struct smu_context *smu, enum
>> PP_OD_DPM_TABL
>>> return ret;
>>> }
>>>
>>> +static int navi10_restore_committed_od_settings(struct smu_context
>>> +*smu) {
>>> + struct smu_table_context *table_context = &smu->smu_table;
>>> + void *od_table = table_context->committed_overdrive_table;
>>> + int ret = 0;
>>> +
>>> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
>> (void *)od_table, true);
>>> + if (ret)
>>> + dev_err(smu->adev->dev, "Failed to import overdrive
>> table!\n");
>>> +
>>> + return ret;
>>> +}
>>
>> As mentioned before, not sure if this is needed as callback. Even if, this can
>> be something common for smuv11, there is nothing ASIC specific in this logic,
>> right?
> [Quan, Evan] Yes, in patch2 of the series, it was made as a SMUV11 common API.
>
> BR
> Evan
>>
>> Thanks,
>> Lijo
>>
>>> static int navi10_run_btc(struct smu_context *smu)
>>> {
>>> int ret = 0;
>>> @@ -3262,6 +3289,7 @@ static const struct pptable_funcs
>> navi10_ppt_funcs = {
>>> .set_soft_freq_limited_range =
>> smu_v11_0_set_soft_freq_limited_range,
>>> .set_default_od_settings = navi10_set_default_od_settings,
>>> .od_edit_dpm_table = navi10_od_edit_dpm_table,
>>> + .restore_committed_od_settings =
>>> +navi10_restore_committed_od_settings,
>>> .run_btc = navi10_run_btc,
>>> .set_power_source = smu_v11_0_set_power_source,
>>> .get_pp_feature_mask = smu_cmn_get_pp_feature_mask, diff --git
>>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> index 0a5d46ac9ccd..28fc3f17c1b1 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> @@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct
>> smu_context *smu)
>>> ret = -ENOMEM;
>>> goto err3_out;
>>> }
>>> +
>>> + smu_table->committed_overdrive_table =
>>> + kzalloc(tables[SMU_TABLE_OVERDRIVE].size,
>> GFP_KERNEL);
>>> + if (!smu_table->committed_overdrive_table) {
>>> + ret = -ENOMEM;
>>> + goto err4_out;
>>> + }
>>> +
>>> }
>>>
>>> return 0;
>>>
>>> +err4_out:
>>> + kfree(smu_table->boot_overdrive_table);
>>> err3_out:
>>> kfree(smu_table->overdrive_table);
>>> err2_out:
>>> @@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct
>> smu_context *smu)
>>> struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
>>>
>>> kfree(smu_table->gpu_metrics_table);
>>> + kfree(smu_table->committed_overdrive_table);
>>> kfree(smu_table->boot_overdrive_table);
>>> kfree(smu_table->overdrive_table);
>>> kfree(smu_table->max_sustainable_clocks);
>>> kfree(smu_table->driver_pptable);
>>> kfree(smu_table->clocks_table);
>>> smu_table->gpu_metrics_table = NULL;
>>> + smu_table->committed_overdrive_table = NULL;
>>> smu_table->boot_overdrive_table = NULL;
>>> smu_table->overdrive_table = NULL;
>>> smu_table->max_sustainable_clocks = NULL;
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-22 9:03 ` Lazar, Lijo
@ 2021-07-23 6:47 ` Quan, Evan
2021-07-23 7:20 ` Quan, Evan
0 siblings, 1 reply; 11+ messages in thread
From: Quan, Evan @ 2021-07-23 6:47 UTC (permalink / raw)
To: Lazar, Lijo, amd-gfx; +Cc: Deucher, Alexander
[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Thursday, July 22, 2021 5:03 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
>
>
> On 7/22/2021 2:03 PM, Quan, Evan wrote:
> > [AMD Official Use Only]
> >
> >
> >
> >> -----Original Message-----
> >> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> >> Sent: Thursday, July 22, 2021 4:10 PM
> >> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> >> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> >> settings properly for NV1x
> >>
> >>
> >>
> >> On 7/22/2021 8:50 AM, Evan Quan wrote:
> >>> The customized OD settings can be divided into two parts: those
> >>> committed ones and non-committed ones.
> >>> - For those changes which had been fed to SMU before S3/S4/Runpm
> >>> suspend kicked, they are committed changes. They should be
> properly
> >>> restored and fed to SMU on S3/S4/Runpm resume.
> >>> - For those non-committed changes, they are restored only
> >>> without
> >> feeding
> >>> to SMU.
> >>>
> >>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> >>> Signed-off-by: Evan Quan <evan.quan@amd.com>
> >>> ---
> >>> drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> >>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 ++++
> >>> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> >> ++++++++++++++-----
> >>> .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 12 +++++
> >>> 4 files changed, 69 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> index 3e89852e4820..8ba53f16d2d9 100644
> >>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> >>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> >>> uint32_t power_limit;
> >>> uint32_t fan_speed_percent;
> >>> uint32_t flags;
> >>> + uint32_t committed_od;
> >>>
> >>> /* user clock state information */
> >>> uint32_t clk_mask[SMU_CLK_COUNT]; @@ -352,6 +353,7 @@ struct
> >>> smu_table_context
> >>>
> >>> void *overdrive_table;
> >>> void *boot_overdrive_table;
> >>> + void *committed_overdrive_table;
> >>
> >> May be rename it to user_overdrive_table - OD table with user settings?
> > [Quan, Evan] Actually "overdrive_table " is the user_overdrive_table. It
> contains all the modification including those not committed( the commit is
> performed by echo "c" > /sys/class/drm/card1/device/pp_od_clk_voltage).
> > The new member added refers to those user customized but committed
> only settings. That's why it was named as " committed_overdrive_table".
> Any good suggestion for the naming?
>
> As far as I understand, the problem is overdrive_table can have intemediary
> settings as the edit/commit is a two-step process. At any point we can have
> user_od_table keep the latest user mode settings. That is true when user
> restores to default settings also; that time we can just copy the boot settings
> back to user_od table and keep the flag as false indicating that there is no
> custom settings.
[Quan, Evan] For now, on Navi1x the "restores to default settings" is also a two-step process. That will make "copy the boot settings
back to user_od table and keep the flag as false" tricky. However, it seems that does not fit original design. As per original design,
the "restores to default settings" should be a one-step process. Let me correct them with new patch series and proceed further discussion then.
BR
Evan
>
> >>
> >>> uint32_t gpu_metrics_table_size;
> >>> void *gpu_metrics_table;
> >>> @@ -623,6 +625,12 @@ struct pptable_funcs {
> >>> enum PP_OD_DPM_TABLE_COMMAND
> >> type,
> >>> long *input, uint32_t size);
> >>>
> >>> + /**
> >>> + * @restore_committed_od_settings: Restore the customized and
> >> committed
> >>> + * OD settings on S3/S4/Runpm resume.
> >>> + */
> >>> + int (*restore_committed_od_settings)(struct smu_context *smu);
> >>> +
> >>> /**
> >>> * @get_clock_by_type_with_latency: Get the speed and latency of
> >> a clock
> >>> * domain.
> >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> index ebe672142808..5f7d98e99f76 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> >>> @@ -416,6 +416,15 @@ static void
> smu_restore_dpm_user_profile(struct
> >> smu_context *smu)
> >>> }
> >>> }
> >>>
> >>> + /* Restore customized and committed OD settings */
> >>> + if (smu->user_dpm_profile.committed_od) {
> >>> + if (smu->ppt_funcs->restore_committed_od_settings) {
> >>> + ret = smu->ppt_funcs-
> >>> restore_committed_od_settings(smu);
> >>> + if (ret)
> >>> + dev_err(smu->adev->dev, "Failed to upload
> >> customized OD settings\n");
> >>> + }
> >>> + }
> >>> +
> >>
> >> Just thinking if there is a need to handle it ASIC specific. The
> >> flags and table pointer are maintained in common structure. So can't
> >> we do the restore of user OD settings directly in this common flow
> >> instead of having each ASIC to implement the callback?
> > [Quan, Evan] The OD settings restoring is ASIC specific as it performed on
> OD table and that(OD table) is ASIC specific.
>
> I was thinking in terms of final logic that is being done. The below structures
> are available in common table context and we have a common logic to
> update the table. If there is no custom OD settings available, we could handle
> it with the flag.
>
> + struct smu_table_context *table_context = &smu->smu_table;
> + void *od_table = table_context->committed_overdrive_table;
> + int ret = 0;
> +
> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> (void *)od_table, true);
>
> >>
> >>> /* Disable restore flag */
> >>> smu->user_dpm_profile.flags &=
> >> ~SMU_DPM_USER_PROFILE_RESTORE;
> >>> }
> >>> 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 59ea59acfb00..4752299d7f91 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> >>> @@ -2296,39 +2296,45 @@ static int
> >> navi10_set_default_od_settings(struct smu_context *smu)
> >>> (OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
> >>> int ret = 0;
> >>>
> >>> - ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> >> (void *)od_table, false);
> >>> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> >> (void
> >>> +*)boot_od_table, false);
> >>> if (ret) {
> >>> dev_err(smu->adev->dev, "Failed to get overdrive table!\n");
> >>> return ret;
> >>> }
> >>>
> >>> - if (!od_table->GfxclkVolt1) {
> >>> + if (!boot_od_table->GfxclkVolt1) {
> >>> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> >>> - &od_table-
> >>> GfxclkVolt1,
> >>> - od_table-
> >>> GfxclkFreq1);
> >>> +
> >> &boot_od_table->GfxclkVolt1,
> >>> +
> >> boot_od_table->GfxclkFreq1);
> >>> if (ret)
> >>> return ret;
> >>> }
> >>>
> >>> - if (!od_table->GfxclkVolt2) {
> >>> + if (!boot_od_table->GfxclkVolt2) {
> >>> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> >>> - &od_table-
> >>> GfxclkVolt2,
> >>> - od_table-
> >>> GfxclkFreq2);
> >>> +
> >> &boot_od_table->GfxclkVolt2,
> >>> +
> >> boot_od_table->GfxclkFreq2);
> >>> if (ret)
> >>> return ret;
> >>> }
> >>>
> >>> - if (!od_table->GfxclkVolt3) {
> >>> + if (!boot_od_table->GfxclkVolt3) {
> >>> ret = navi10_overdrive_get_gfx_clk_base_voltage(smu,
> >>> - &od_table-
> >>> GfxclkVolt3,
> >>> - od_table-
> >>> GfxclkFreq3);
> >>> +
> >> &boot_od_table->GfxclkVolt3,
> >>> +
> >> boot_od_table->GfxclkFreq3);
> >>> if (ret)
> >>> return ret;
> >>> }
> >>>
> >>> - memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
> >>> + navi10_dump_od_table(smu, boot_od_table);
> >>>
> >>> - navi10_dump_od_table(smu, od_table);
> >>> + /*
> >>> + * For S3/S4/Runpm, no need to install boot od table to
> >> overdrive_table as
> >>> + * - either they already share the same OD settings on bootup
> >>> + * - or they have user customized OD settings
> >>> + */
> >>> + if (!smu->adev->in_suspend)
> >>> + memcpy(od_table, boot_od_table,
> >> sizeof(OverDriveTable_t));
> >>>
> >>> return 0;
> >>> }
> >>> @@ -2435,6 +2441,14 @@ static int navi10_od_edit_dpm_table(struct
> >> smu_context *smu, enum PP_OD_DPM_TABL
> >>> dev_err(smu->adev->dev, "Failed to import
> >> overdrive table!\n");
> >>> return ret;
> >>> }
> >>> + if (memcmp(table_context->overdrive_table, table_context-
> >>> boot_overdrive_table,
> >>> + sizeof(OverDriveTable_t))) {
> >>
> >> Shouldn't this be - compare against the edited settings and last
> >> committed settings, overdrive_table vs committed_overdrive_table?
> >>
> >> Basically, user_od_table can be initialized with boot_od settings and
> >> the flag gives the indication that user_od table is being used or not.
> >> Before updating, check if the edited table (overdrive_table) and the
> >> current user_od table are different, then commit the new table.
> > [Quan, Evan] Yes, I also considered that. But that cannot handle the case
> below:
> > 1 user made some customizations -> 2 the customizations were
> > committed -> 3 user restored to default(boot) OD settings(by "echo 'r'")
> and committed Although there were some changes between 2 and 3, there
> is actually no real customized changes compared to default(boot) OD settings.
>
> On restore to default, just copy the boot_table settings to user_od and keep
> the flag as false. We are using user_od as the latest user preferred settings
> and overdrive_table is only used for intermediate updates.
>
> The flag decides whether to restore or not, but at any point we can refer the
> user_od table to look upon the latest preferred user settings (default or
> custom).
>
> Thanks,
> Lijo
>
> >>
> >>> + smu->user_dpm_profile.committed_od = true;
> >>> + memcpy(table_context-
> >>> committed_overdrive_table, table_context->overdrive_table,
> >>> + sizeof(OverDriveTable_t));
> >>> + } else {
> >>> + smu->user_dpm_profile.committed_od = false;
> >>> + }
> >>> break;
> >>> case PP_OD_EDIT_VDDC_CURVE:
> >>> if (!navi10_od_feature_is_supported(od_settings,
> >>> SMU_11_0_ODCAP_GFXCLK_CURVE)) { @@ -2499,6 +2513,19 @@ static
> int
> >> navi10_od_edit_dpm_table(struct smu_context *smu, enum
> PP_OD_DPM_TABL
> >>> return ret;
> >>> }
> >>>
> >>> +static int navi10_restore_committed_od_settings(struct smu_context
> >>> +*smu) {
> >>> + struct smu_table_context *table_context = &smu->smu_table;
> >>> + void *od_table = table_context->committed_overdrive_table;
> >>> + int ret = 0;
> >>> +
> >>> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> >> (void *)od_table, true);
> >>> + if (ret)
> >>> + dev_err(smu->adev->dev, "Failed to import overdrive
> >> table!\n");
> >>> +
> >>> + return ret;
> >>> +}
> >>
> >> As mentioned before, not sure if this is needed as callback. Even if,
> >> this can be something common for smuv11, there is nothing ASIC
> >> specific in this logic, right?
> > [Quan, Evan] Yes, in patch2 of the series, it was made as a SMUV11
> common API.
> >
> > BR
> > Evan
> >>
> >> Thanks,
> >> Lijo
> >>
> >>> static int navi10_run_btc(struct smu_context *smu)
> >>> {
> >>> int ret = 0;
> >>> @@ -3262,6 +3289,7 @@ static const struct pptable_funcs
> >> navi10_ppt_funcs = {
> >>> .set_soft_freq_limited_range =
> >> smu_v11_0_set_soft_freq_limited_range,
> >>> .set_default_od_settings = navi10_set_default_od_settings,
> >>> .od_edit_dpm_table = navi10_od_edit_dpm_table,
> >>> + .restore_committed_od_settings =
> >>> +navi10_restore_committed_od_settings,
> >>> .run_btc = navi10_run_btc,
> >>> .set_power_source = smu_v11_0_set_power_source,
> >>> .get_pp_feature_mask = smu_cmn_get_pp_feature_mask, diff --git
> >>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> >>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> >>> index 0a5d46ac9ccd..28fc3f17c1b1 100644
> >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> >>> @@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct
> >> smu_context *smu)
> >>> ret = -ENOMEM;
> >>> goto err3_out;
> >>> }
> >>> +
> >>> + smu_table->committed_overdrive_table =
> >>> + kzalloc(tables[SMU_TABLE_OVERDRIVE].size,
> >> GFP_KERNEL);
> >>> + if (!smu_table->committed_overdrive_table) {
> >>> + ret = -ENOMEM;
> >>> + goto err4_out;
> >>> + }
> >>> +
> >>> }
> >>>
> >>> return 0;
> >>>
> >>> +err4_out:
> >>> + kfree(smu_table->boot_overdrive_table);
> >>> err3_out:
> >>> kfree(smu_table->overdrive_table);
> >>> err2_out:
> >>> @@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct
> >> smu_context *smu)
> >>> struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
> >>>
> >>> kfree(smu_table->gpu_metrics_table);
> >>> + kfree(smu_table->committed_overdrive_table);
> >>> kfree(smu_table->boot_overdrive_table);
> >>> kfree(smu_table->overdrive_table);
> >>> kfree(smu_table->max_sustainable_clocks);
> >>> kfree(smu_table->driver_pptable);
> >>> kfree(smu_table->clocks_table);
> >>> smu_table->gpu_metrics_table = NULL;
> >>> + smu_table->committed_overdrive_table = NULL;
> >>> smu_table->boot_overdrive_table = NULL;
> >>> smu_table->overdrive_table = NULL;
> >>> smu_table->max_sustainable_clocks = NULL;
> >>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-23 6:47 ` Quan, Evan
@ 2021-07-23 7:20 ` Quan, Evan
2021-07-23 7:39 ` Lazar, Lijo
0 siblings, 1 reply; 11+ messages in thread
From: Quan, Evan @ 2021-07-23 7:20 UTC (permalink / raw)
To: Lazar, Lijo, amd-gfx; +Cc: Deucher, Alexander
[AMD Official Use Only]
Hi Lijo,
Sorry, I doubled checked. The implementation of Navi1x is right.
The original design for "restores to default settings" is a two-step process.
That means for "case PP_OD_COMMIT_DPM_TABLE:" we must distinguish whether it's an usual customized od setting update or just restoring to defaults.
And my current implementation for that is as below. Any comments?
+ if (memcmp(table_context->overdrive_table, table_context->boot_overdrive_table,
+ sizeof(OverDriveTable_t))) {
+ smu->user_dpm_profile.committed_od = true;
+ memcpy(table_context->committed_overdrive_table, table_context->overdrive_table,
+ sizeof(OverDriveTable_t));
+ } else {
+ smu->user_dpm_profile.committed_od = false;
+ }
BR
Evan
> -----Original Message-----
> From: Quan, Evan
> Sent: Friday, July 23, 2021 2:48 PM
> To: Lazar, Lijo <Lijo.Lazar@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
> [AMD Official Use Only]
>
>
>
> > -----Original Message-----
> > From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > Sent: Thursday, July 22, 2021 5:03 PM
> > To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> > Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > settings properly for NV1x
> >
> >
> >
> > On 7/22/2021 2:03 PM, Quan, Evan wrote:
> > > [AMD Official Use Only]
> > >
> > >
> > >
> > >> -----Original Message-----
> > >> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > >> Sent: Thursday, July 22, 2021 4:10 PM
> > >> To: Quan, Evan <Evan.Quan@amd.com>; amd-
> gfx@lists.freedesktop.org
> > >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> > >> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > >> settings properly for NV1x
> > >>
> > >>
> > >>
> > >> On 7/22/2021 8:50 AM, Evan Quan wrote:
> > >>> The customized OD settings can be divided into two parts: those
> > >>> committed ones and non-committed ones.
> > >>> - For those changes which had been fed to SMU before
> S3/S4/Runpm
> > >>> suspend kicked, they are committed changes. They should be
> > properly
> > >>> restored and fed to SMU on S3/S4/Runpm resume.
> > >>> - For those non-committed changes, they are restored only
> > >>> without
> > >> feeding
> > >>> to SMU.
> > >>>
> > >>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > >>> Signed-off-by: Evan Quan <evan.quan@amd.com>
> > >>> ---
> > >>> drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > >>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 ++++
> > >>> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> > >> ++++++++++++++-----
> > >>> .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 12 +++++
> > >>> 4 files changed, 69 insertions(+), 12 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> index 3e89852e4820..8ba53f16d2d9 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> > >>> uint32_t power_limit;
> > >>> uint32_t fan_speed_percent;
> > >>> uint32_t flags;
> > >>> + uint32_t committed_od;
> > >>>
> > >>> /* user clock state information */
> > >>> uint32_t clk_mask[SMU_CLK_COUNT]; @@ -352,6 +353,7
> @@ struct
> > >>> smu_table_context
> > >>>
> > >>> void *overdrive_table;
> > >>> void *boot_overdrive_table;
> > >>> + void *committed_overdrive_table;
> > >>
> > >> May be rename it to user_overdrive_table - OD table with user settings?
> > > [Quan, Evan] Actually "overdrive_table " is the
> > > user_overdrive_table. It
> > contains all the modification including those not committed( the
> > commit is performed by echo "c" >
> /sys/class/drm/card1/device/pp_od_clk_voltage).
> > > The new member added refers to those user customized but committed
> > only settings. That's why it was named as " committed_overdrive_table".
> > Any good suggestion for the naming?
> >
> > As far as I understand, the problem is overdrive_table can have
> > intemediary settings as the edit/commit is a two-step process. At any
> > point we can have user_od_table keep the latest user mode settings.
> > That is true when user restores to default settings also; that time we
> > can just copy the boot settings back to user_od table and keep the
> > flag as false indicating that there is no custom settings.
> [Quan, Evan] For now, on Navi1x the "restores to default settings" is also a
> two-step process. That will make "copy the boot settings back to user_od
> table and keep the flag as false" tricky. However, it seems that does not fit
> original design. As per original design, the "restores to default settings"
> should be a one-step process. Let me correct them with new patch series
> and proceed further discussion then.
>
> BR
> Evan
> >
> > >>
> > >>> uint32_t gpu_metrics_table_size;
> > >>> void *gpu_metrics_table;
> > >>> @@ -623,6 +625,12 @@ struct pptable_funcs {
> > >>> enum
> PP_OD_DPM_TABLE_COMMAND
> > >> type,
> > >>> long *input, uint32_t size);
> > >>>
> > >>> + /**
> > >>> + * @restore_committed_od_settings: Restore the customized and
> > >> committed
> > >>> + * OD settings on S3/S4/Runpm resume.
> > >>> + */
> > >>> + int (*restore_committed_od_settings)(struct smu_context *smu);
> > >>> +
> > >>> /**
> > >>> * @get_clock_by_type_with_latency: Get the speed and
> latency
> > >>> of
> > >> a clock
> > >>> * domain.
> > >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > >>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > >>> index ebe672142808..5f7d98e99f76 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > >>> @@ -416,6 +416,15 @@ static void
> > smu_restore_dpm_user_profile(struct
> > >> smu_context *smu)
> > >>> }
> > >>> }
> > >>>
> > >>> + /* Restore customized and committed OD settings */
> > >>> + if (smu->user_dpm_profile.committed_od) {
> > >>> + if (smu->ppt_funcs->restore_committed_od_settings) {
> > >>> + ret = smu->ppt_funcs-
> > >>> restore_committed_od_settings(smu);
> > >>> + if (ret)
> > >>> + dev_err(smu->adev->dev, "Failed to upload
> > >> customized OD settings\n");
> > >>> + }
> > >>> + }
> > >>> +
> > >>
> > >> Just thinking if there is a need to handle it ASIC specific. The
> > >> flags and table pointer are maintained in common structure. So
> > >> can't we do the restore of user OD settings directly in this common
> > >> flow instead of having each ASIC to implement the callback?
> > > [Quan, Evan] The OD settings restoring is ASIC specific as it
> > > performed on
> > OD table and that(OD table) is ASIC specific.
> >
> > I was thinking in terms of final logic that is being done. The below
> > structures are available in common table context and we have a common
> > logic to update the table. If there is no custom OD settings
> > available, we could handle it with the flag.
> >
> > + struct smu_table_context *table_context = &smu->smu_table;
> > + void *od_table = table_context->committed_overdrive_table;
> > + int ret = 0;
> > +
> > + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > (void *)od_table, true);
> >
> > >>
> > >>> /* Disable restore flag */
> > >>> smu->user_dpm_profile.flags &=
> > >> ~SMU_DPM_USER_PROFILE_RESTORE;
> > >>> }
> > >>> 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 59ea59acfb00..4752299d7f91 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > >>> @@ -2296,39 +2296,45 @@ static int
> > >> navi10_set_default_od_settings(struct smu_context *smu)
> > >>> (OverDriveTable_t *)smu-
> >smu_table.boot_overdrive_table;
> > >>> int ret = 0;
> > >>>
> > >>> - ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > >> (void *)od_table, false);
> > >>> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > >> (void
> > >>> +*)boot_od_table, false);
> > >>> if (ret) {
> > >>> dev_err(smu->adev->dev, "Failed to get overdrive
> table!\n");
> > >>> return ret;
> > >>> }
> > >>>
> > >>> - if (!od_table->GfxclkVolt1) {
> > >>> + if (!boot_od_table->GfxclkVolt1) {
> > >>> ret =
> navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > >>> - &od_table-
> > >>> GfxclkVolt1,
> > >>> - od_table-
> > >>> GfxclkFreq1);
> > >>> +
> > >> &boot_od_table->GfxclkVolt1,
> > >>> +
> > >> boot_od_table->GfxclkFreq1);
> > >>> if (ret)
> > >>> return ret;
> > >>> }
> > >>>
> > >>> - if (!od_table->GfxclkVolt2) {
> > >>> + if (!boot_od_table->GfxclkVolt2) {
> > >>> ret =
> navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > >>> - &od_table-
> > >>> GfxclkVolt2,
> > >>> - od_table-
> > >>> GfxclkFreq2);
> > >>> +
> > >> &boot_od_table->GfxclkVolt2,
> > >>> +
> > >> boot_od_table->GfxclkFreq2);
> > >>> if (ret)
> > >>> return ret;
> > >>> }
> > >>>
> > >>> - if (!od_table->GfxclkVolt3) {
> > >>> + if (!boot_od_table->GfxclkVolt3) {
> > >>> ret =
> navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > >>> - &od_table-
> > >>> GfxclkVolt3,
> > >>> - od_table-
> > >>> GfxclkFreq3);
> > >>> +
> > >> &boot_od_table->GfxclkVolt3,
> > >>> +
> > >> boot_od_table->GfxclkFreq3);
> > >>> if (ret)
> > >>> return ret;
> > >>> }
> > >>>
> > >>> - memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
> > >>> + navi10_dump_od_table(smu, boot_od_table);
> > >>>
> > >>> - navi10_dump_od_table(smu, od_table);
> > >>> + /*
> > >>> + * For S3/S4/Runpm, no need to install boot od table to
> > >> overdrive_table as
> > >>> + * - either they already share the same OD settings on bootup
> > >>> + * - or they have user customized OD settings
> > >>> + */
> > >>> + if (!smu->adev->in_suspend)
> > >>> + memcpy(od_table, boot_od_table,
> > >> sizeof(OverDriveTable_t));
> > >>>
> > >>> return 0;
> > >>> }
> > >>> @@ -2435,6 +2441,14 @@ static int navi10_od_edit_dpm_table(struct
> > >> smu_context *smu, enum PP_OD_DPM_TABL
> > >>> dev_err(smu->adev->dev, "Failed to import
> > >> overdrive table!\n");
> > >>> return ret;
> > >>> }
> > >>> + if (memcmp(table_context->overdrive_table, table_context-
> > >>> boot_overdrive_table,
> > >>> + sizeof(OverDriveTable_t))) {
> > >>
> > >> Shouldn't this be - compare against the edited settings and last
> > >> committed settings, overdrive_table vs committed_overdrive_table?
> > >>
> > >> Basically, user_od_table can be initialized with boot_od settings
> > >> and the flag gives the indication that user_od table is being used or not.
> > >> Before updating, check if the edited table (overdrive_table) and
> > >> the current user_od table are different, then commit the new table.
> > > [Quan, Evan] Yes, I also considered that. But that cannot handle the
> > > case
> > below:
> > > 1 user made some customizations -> 2 the customizations were
> > > committed -> 3 user restored to default(boot) OD settings(by "echo
> > > 'r'")
> > and committed Although there were some changes between 2 and 3,
> there
> > is actually no real customized changes compared to default(boot) OD
> settings.
> >
> > On restore to default, just copy the boot_table settings to user_od
> > and keep the flag as false. We are using user_od as the latest user
> > preferred settings and overdrive_table is only used for intermediate
> updates.
> >
> > The flag decides whether to restore or not, but at any point we can
> > refer the user_od table to look upon the latest preferred user
> > settings (default or custom).
> >
> > Thanks,
> > Lijo
> >
> > >>
> > >>> + smu->user_dpm_profile.committed_od = true;
> > >>> + memcpy(table_context-
> > >>> committed_overdrive_table, table_context->overdrive_table,
> > >>> + sizeof(OverDriveTable_t));
> > >>> + } else {
> > >>> + smu->user_dpm_profile.committed_od = false;
> > >>> + }
> > >>> break;
> > >>> case PP_OD_EDIT_VDDC_CURVE:
> > >>> if (!navi10_od_feature_is_supported(od_settings,
> > >>> SMU_11_0_ODCAP_GFXCLK_CURVE)) { @@ -2499,6 +2513,19 @@
> static
> > int
> > >> navi10_od_edit_dpm_table(struct smu_context *smu, enum
> > PP_OD_DPM_TABL
> > >>> return ret;
> > >>> }
> > >>>
> > >>> +static int navi10_restore_committed_od_settings(struct
> > >>> +smu_context
> > >>> +*smu) {
> > >>> + struct smu_table_context *table_context = &smu->smu_table;
> > >>> + void *od_table = table_context->committed_overdrive_table;
> > >>> + int ret = 0;
> > >>> +
> > >>> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > >> (void *)od_table, true);
> > >>> + if (ret)
> > >>> + dev_err(smu->adev->dev, "Failed to import overdrive
> > >> table!\n");
> > >>> +
> > >>> + return ret;
> > >>> +}
> > >>
> > >> As mentioned before, not sure if this is needed as callback. Even
> > >> if, this can be something common for smuv11, there is nothing ASIC
> > >> specific in this logic, right?
> > > [Quan, Evan] Yes, in patch2 of the series, it was made as a SMUV11
> > common API.
> > >
> > > BR
> > > Evan
> > >>
> > >> Thanks,
> > >> Lijo
> > >>
> > >>> static int navi10_run_btc(struct smu_context *smu)
> > >>> {
> > >>> int ret = 0;
> > >>> @@ -3262,6 +3289,7 @@ static const struct pptable_funcs
> > >> navi10_ppt_funcs = {
> > >>> .set_soft_freq_limited_range =
> > >> smu_v11_0_set_soft_freq_limited_range,
> > >>> .set_default_od_settings = navi10_set_default_od_settings,
> > >>> .od_edit_dpm_table = navi10_od_edit_dpm_table,
> > >>> + .restore_committed_od_settings =
> > >>> +navi10_restore_committed_od_settings,
> > >>> .run_btc = navi10_run_btc,
> > >>> .set_power_source = smu_v11_0_set_power_source,
> > >>> .get_pp_feature_mask = smu_cmn_get_pp_feature_mask,
> diff --git
> > >>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > >>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > >>> index 0a5d46ac9ccd..28fc3f17c1b1 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > >>> @@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct
> > >> smu_context *smu)
> > >>> ret = -ENOMEM;
> > >>> goto err3_out;
> > >>> }
> > >>> +
> > >>> + smu_table->committed_overdrive_table =
> > >>> + kzalloc(tables[SMU_TABLE_OVERDRIVE].size,
> > >> GFP_KERNEL);
> > >>> + if (!smu_table->committed_overdrive_table) {
> > >>> + ret = -ENOMEM;
> > >>> + goto err4_out;
> > >>> + }
> > >>> +
> > >>> }
> > >>>
> > >>> return 0;
> > >>>
> > >>> +err4_out:
> > >>> + kfree(smu_table->boot_overdrive_table);
> > >>> err3_out:
> > >>> kfree(smu_table->overdrive_table);
> > >>> err2_out:
> > >>> @@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct
> > >> smu_context *smu)
> > >>> struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
> > >>>
> > >>> kfree(smu_table->gpu_metrics_table);
> > >>> + kfree(smu_table->committed_overdrive_table);
> > >>> kfree(smu_table->boot_overdrive_table);
> > >>> kfree(smu_table->overdrive_table);
> > >>> kfree(smu_table->max_sustainable_clocks);
> > >>> kfree(smu_table->driver_pptable);
> > >>> kfree(smu_table->clocks_table);
> > >>> smu_table->gpu_metrics_table = NULL;
> > >>> + smu_table->committed_overdrive_table = NULL;
> > >>> smu_table->boot_overdrive_table = NULL;
> > >>> smu_table->overdrive_table = NULL;
> > >>> smu_table->max_sustainable_clocks = NULL;
> > >>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-23 7:20 ` Quan, Evan
@ 2021-07-23 7:39 ` Lazar, Lijo
2021-07-23 9:11 ` Quan, Evan
0 siblings, 1 reply; 11+ messages in thread
From: Lazar, Lijo @ 2021-07-23 7:39 UTC (permalink / raw)
To: Quan, Evan, amd-gfx; +Cc: Deucher, Alexander
[Public]
Hi Evan,
In that case, using "od_edit" table for the intermediate table
During commit command, what if it's done like
1) Copy and update table if od_edit table is different than user_od table
2) Set the flag to false if od_edit table and boot_od table are different
Then current od settings may always be picked from user_od table and the flag may be used to distinguish if it's boot od settings or custom settings (for restore or other purposes).
Thanks,
Lijo
-----Original Message-----
From: Quan, Evan <Evan.Quan@amd.com>
Sent: Friday, July 23, 2021 12:51 PM
To: Lazar, Lijo <Lijo.Lazar@amd.com>; amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
[AMD Official Use Only]
Hi Lijo,
Sorry, I doubled checked. The implementation of Navi1x is right.
The original design for "restores to default settings" is a two-step process.
That means for "case PP_OD_COMMIT_DPM_TABLE:" we must distinguish whether it's an usual customized od setting update or just restoring to defaults.
And my current implementation for that is as below. Any comments?
+ if (memcmp(table_context->overdrive_table, table_context->boot_overdrive_table,
+ sizeof(OverDriveTable_t))) {
+ smu->user_dpm_profile.committed_od = true;
+ memcpy(table_context->committed_overdrive_table, table_context->overdrive_table,
+ sizeof(OverDriveTable_t));
+ } else {
+ smu->user_dpm_profile.committed_od = false;
+ }
BR
Evan
> -----Original Message-----
> From: Quan, Evan
> Sent: Friday, July 23, 2021 2:48 PM
> To: Lazar, Lijo <Lijo.Lazar@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD
> settings properly for NV1x
>
> [AMD Official Use Only]
>
>
>
> > -----Original Message-----
> > From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > Sent: Thursday, July 22, 2021 5:03 PM
> > To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> > Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > settings properly for NV1x
> >
> >
> >
> > On 7/22/2021 2:03 PM, Quan, Evan wrote:
> > > [AMD Official Use Only]
> > >
> > >
> > >
> > >> -----Original Message-----
> > >> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > >> Sent: Thursday, July 22, 2021 4:10 PM
> > >> To: Quan, Evan <Evan.Quan@amd.com>; amd-
> gfx@lists.freedesktop.org
> > >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> > >> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > >> settings properly for NV1x
> > >>
> > >>
> > >>
> > >> On 7/22/2021 8:50 AM, Evan Quan wrote:
> > >>> The customized OD settings can be divided into two parts: those
> > >>> committed ones and non-committed ones.
> > >>> - For those changes which had been fed to SMU before
> S3/S4/Runpm
> > >>> suspend kicked, they are committed changes. They should be
> > properly
> > >>> restored and fed to SMU on S3/S4/Runpm resume.
> > >>> - For those non-committed changes, they are restored only
> > >>> without
> > >> feeding
> > >>> to SMU.
> > >>>
> > >>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > >>> Signed-off-by: Evan Quan <evan.quan@amd.com>
> > >>> ---
> > >>> drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > >>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 ++++
> > >>> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> > >> ++++++++++++++-----
> > >>> .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 12 +++++
> > >>> 4 files changed, 69 insertions(+), 12 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> index 3e89852e4820..8ba53f16d2d9 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > >>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> > >>> uint32_t power_limit;
> > >>> uint32_t fan_speed_percent;
> > >>> uint32_t flags;
> > >>> + uint32_t committed_od;
> > >>>
> > >>> /* user clock state information */
> > >>> uint32_t clk_mask[SMU_CLK_COUNT]; @@ -352,6 +353,7
> @@ struct
> > >>> smu_table_context
> > >>>
> > >>> void *overdrive_table;
> > >>> void *boot_overdrive_table;
> > >>> + void *committed_overdrive_table;
> > >>
> > >> May be rename it to user_overdrive_table - OD table with user settings?
> > > [Quan, Evan] Actually "overdrive_table " is the
> > > user_overdrive_table. It
> > contains all the modification including those not committed( the
> > commit is performed by echo "c" >
> /sys/class/drm/card1/device/pp_od_clk_voltage).
> > > The new member added refers to those user customized but committed
> > only settings. That's why it was named as " committed_overdrive_table".
> > Any good suggestion for the naming?
> >
> > As far as I understand, the problem is overdrive_table can have
> > intemediary settings as the edit/commit is a two-step process. At
> > any point we can have user_od_table keep the latest user mode settings.
> > That is true when user restores to default settings also; that time
> > we can just copy the boot settings back to user_od table and keep
> > the flag as false indicating that there is no custom settings.
> [Quan, Evan] For now, on Navi1x the "restores to default settings" is
> also a two-step process. That will make "copy the boot settings back
> to user_od table and keep the flag as false" tricky. However, it seems
> that does not fit original design. As per original design, the "restores to default settings"
> should be a one-step process. Let me correct them with new patch
> series and proceed further discussion then.
>
> BR
> Evan
> >
> > >>
> > >>> uint32_t gpu_metrics_table_size;
> > >>> void *gpu_metrics_table;
> > >>> @@ -623,6 +625,12 @@ struct pptable_funcs {
> > >>> enum
> PP_OD_DPM_TABLE_COMMAND
> > >> type,
> > >>> long *input, uint32_t size);
> > >>>
> > >>> + /**
> > >>> + * @restore_committed_od_settings: Restore the customized and
> > >> committed
> > >>> + * OD settings on S3/S4/Runpm resume.
> > >>> + */
> > >>> + int (*restore_committed_od_settings)(struct smu_context *smu);
> > >>> +
> > >>> /**
> > >>> * @get_clock_by_type_with_latency: Get the speed and
> latency
> > >>> of
> > >> a clock
> > >>> * domain.
> > >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > >>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > >>> index ebe672142808..5f7d98e99f76 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > >>> @@ -416,6 +416,15 @@ static void
> > smu_restore_dpm_user_profile(struct
> > >> smu_context *smu)
> > >>> }
> > >>> }
> > >>>
> > >>> + /* Restore customized and committed OD settings */
> > >>> + if (smu->user_dpm_profile.committed_od) {
> > >>> + if (smu->ppt_funcs->restore_committed_od_settings) {
> > >>> + ret = smu->ppt_funcs-
> > >>> restore_committed_od_settings(smu);
> > >>> + if (ret)
> > >>> + dev_err(smu->adev->dev, "Failed to upload
> > >> customized OD settings\n");
> > >>> + }
> > >>> + }
> > >>> +
> > >>
> > >> Just thinking if there is a need to handle it ASIC specific. The
> > >> flags and table pointer are maintained in common structure. So
> > >> can't we do the restore of user OD settings directly in this
> > >> common flow instead of having each ASIC to implement the callback?
> > > [Quan, Evan] The OD settings restoring is ASIC specific as it
> > > performed on
> > OD table and that(OD table) is ASIC specific.
> >
> > I was thinking in terms of final logic that is being done. The below
> > structures are available in common table context and we have a
> > common logic to update the table. If there is no custom OD settings
> > available, we could handle it with the flag.
> >
> > + struct smu_table_context *table_context = &smu->smu_table;
> > + void *od_table = table_context->committed_overdrive_table;
> > + int ret = 0;
> > +
> > + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > (void *)od_table, true);
> >
> > >>
> > >>> /* Disable restore flag */
> > >>> smu->user_dpm_profile.flags &=
> > >> ~SMU_DPM_USER_PROFILE_RESTORE;
> > >>> }
> > >>> 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 59ea59acfb00..4752299d7f91 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > >>> @@ -2296,39 +2296,45 @@ static int
> > >> navi10_set_default_od_settings(struct smu_context *smu)
> > >>> (OverDriveTable_t *)smu-
> >smu_table.boot_overdrive_table;
> > >>> int ret = 0;
> > >>>
> > >>> - ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > >> (void *)od_table, false);
> > >>> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > >> (void
> > >>> +*)boot_od_table, false);
> > >>> if (ret) {
> > >>> dev_err(smu->adev->dev, "Failed to get overdrive
> table!\n");
> > >>> return ret;
> > >>> }
> > >>>
> > >>> - if (!od_table->GfxclkVolt1) {
> > >>> + if (!boot_od_table->GfxclkVolt1) {
> > >>> ret =
> navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > >>> - &od_table-
> > >>> GfxclkVolt1,
> > >>> - od_table-
> > >>> GfxclkFreq1);
> > >>> +
> > >> &boot_od_table->GfxclkVolt1,
> > >>> +
> > >> boot_od_table->GfxclkFreq1);
> > >>> if (ret)
> > >>> return ret;
> > >>> }
> > >>>
> > >>> - if (!od_table->GfxclkVolt2) {
> > >>> + if (!boot_od_table->GfxclkVolt2) {
> > >>> ret =
> navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > >>> - &od_table-
> > >>> GfxclkVolt2,
> > >>> - od_table-
> > >>> GfxclkFreq2);
> > >>> +
> > >> &boot_od_table->GfxclkVolt2,
> > >>> +
> > >> boot_od_table->GfxclkFreq2);
> > >>> if (ret)
> > >>> return ret;
> > >>> }
> > >>>
> > >>> - if (!od_table->GfxclkVolt3) {
> > >>> + if (!boot_od_table->GfxclkVolt3) {
> > >>> ret =
> navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > >>> - &od_table-
> > >>> GfxclkVolt3,
> > >>> - od_table-
> > >>> GfxclkFreq3);
> > >>> +
> > >> &boot_od_table->GfxclkVolt3,
> > >>> +
> > >> boot_od_table->GfxclkFreq3);
> > >>> if (ret)
> > >>> return ret;
> > >>> }
> > >>>
> > >>> - memcpy(boot_od_table, od_table, sizeof(OverDriveTable_t));
> > >>> + navi10_dump_od_table(smu, boot_od_table);
> > >>>
> > >>> - navi10_dump_od_table(smu, od_table);
> > >>> + /*
> > >>> + * For S3/S4/Runpm, no need to install boot od table to
> > >> overdrive_table as
> > >>> + * - either they already share the same OD settings on bootup
> > >>> + * - or they have user customized OD settings
> > >>> + */
> > >>> + if (!smu->adev->in_suspend)
> > >>> + memcpy(od_table, boot_od_table,
> > >> sizeof(OverDriveTable_t));
> > >>>
> > >>> return 0;
> > >>> }
> > >>> @@ -2435,6 +2441,14 @@ static int
> > >>> navi10_od_edit_dpm_table(struct
> > >> smu_context *smu, enum PP_OD_DPM_TABL
> > >>> dev_err(smu->adev->dev, "Failed to import
> > >> overdrive table!\n");
> > >>> return ret;
> > >>> }
> > >>> + if (memcmp(table_context->overdrive_table, table_context-
> > >>> boot_overdrive_table,
> > >>> + sizeof(OverDriveTable_t))) {
> > >>
> > >> Shouldn't this be - compare against the edited settings and last
> > >> committed settings, overdrive_table vs committed_overdrive_table?
> > >>
> > >> Basically, user_od_table can be initialized with boot_od settings
> > >> and the flag gives the indication that user_od table is being used or not.
> > >> Before updating, check if the edited table (overdrive_table) and
> > >> the current user_od table are different, then commit the new table.
> > > [Quan, Evan] Yes, I also considered that. But that cannot handle
> > > the case
> > below:
> > > 1 user made some customizations -> 2 the customizations were
> > > committed -> 3 user restored to default(boot) OD settings(by "echo
> > > 'r'")
> > and committed Although there were some changes between 2 and 3,
> there
> > is actually no real customized changes compared to default(boot) OD
> settings.
> >
> > On restore to default, just copy the boot_table settings to user_od
> > and keep the flag as false. We are using user_od as the latest user
> > preferred settings and overdrive_table is only used for intermediate
> updates.
> >
> > The flag decides whether to restore or not, but at any point we can
> > refer the user_od table to look upon the latest preferred user
> > settings (default or custom).
> >
> > Thanks,
> > Lijo
> >
> > >>
> > >>> + smu->user_dpm_profile.committed_od = true;
> > >>> + memcpy(table_context-
> > >>> committed_overdrive_table, table_context->overdrive_table,
> > >>> + sizeof(OverDriveTable_t));
> > >>> + } else {
> > >>> + smu->user_dpm_profile.committed_od = false;
> > >>> + }
> > >>> break;
> > >>> case PP_OD_EDIT_VDDC_CURVE:
> > >>> if (!navi10_od_feature_is_supported(od_settings,
> > >>> SMU_11_0_ODCAP_GFXCLK_CURVE)) { @@ -2499,6 +2513,19 @@
> static
> > int
> > >> navi10_od_edit_dpm_table(struct smu_context *smu, enum
> > PP_OD_DPM_TABL
> > >>> return ret;
> > >>> }
> > >>>
> > >>> +static int navi10_restore_committed_od_settings(struct
> > >>> +smu_context
> > >>> +*smu) {
> > >>> + struct smu_table_context *table_context = &smu->smu_table;
> > >>> + void *od_table = table_context->committed_overdrive_table;
> > >>> + int ret = 0;
> > >>> +
> > >>> + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > >> (void *)od_table, true);
> > >>> + if (ret)
> > >>> + dev_err(smu->adev->dev, "Failed to import overdrive
> > >> table!\n");
> > >>> +
> > >>> + return ret;
> > >>> +}
> > >>
> > >> As mentioned before, not sure if this is needed as callback. Even
> > >> if, this can be something common for smuv11, there is nothing
> > >> ASIC specific in this logic, right?
> > > [Quan, Evan] Yes, in patch2 of the series, it was made as a SMUV11
> > common API.
> > >
> > > BR
> > > Evan
> > >>
> > >> Thanks,
> > >> Lijo
> > >>
> > >>> static int navi10_run_btc(struct smu_context *smu)
> > >>> {
> > >>> int ret = 0;
> > >>> @@ -3262,6 +3289,7 @@ static const struct pptable_funcs
> > >> navi10_ppt_funcs = {
> > >>> .set_soft_freq_limited_range =
> > >> smu_v11_0_set_soft_freq_limited_range,
> > >>> .set_default_od_settings = navi10_set_default_od_settings,
> > >>> .od_edit_dpm_table = navi10_od_edit_dpm_table,
> > >>> + .restore_committed_od_settings =
> > >>> +navi10_restore_committed_od_settings,
> > >>> .run_btc = navi10_run_btc,
> > >>> .set_power_source = smu_v11_0_set_power_source,
> > >>> .get_pp_feature_mask = smu_cmn_get_pp_feature_mask,
> diff --git
> > >>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > >>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > >>> index 0a5d46ac9ccd..28fc3f17c1b1 100644
> > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > >>> @@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct
> > >> smu_context *smu)
> > >>> ret = -ENOMEM;
> > >>> goto err3_out;
> > >>> }
> > >>> +
> > >>> + smu_table->committed_overdrive_table =
> > >>> + kzalloc(tables[SMU_TABLE_OVERDRIVE].size,
> > >> GFP_KERNEL);
> > >>> + if (!smu_table->committed_overdrive_table) {
> > >>> + ret = -ENOMEM;
> > >>> + goto err4_out;
> > >>> + }
> > >>> +
> > >>> }
> > >>>
> > >>> return 0;
> > >>>
> > >>> +err4_out:
> > >>> + kfree(smu_table->boot_overdrive_table);
> > >>> err3_out:
> > >>> kfree(smu_table->overdrive_table);
> > >>> err2_out:
> > >>> @@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct
> > >> smu_context *smu)
> > >>> struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
> > >>>
> > >>> kfree(smu_table->gpu_metrics_table);
> > >>> + kfree(smu_table->committed_overdrive_table);
> > >>> kfree(smu_table->boot_overdrive_table);
> > >>> kfree(smu_table->overdrive_table);
> > >>> kfree(smu_table->max_sustainable_clocks);
> > >>> kfree(smu_table->driver_pptable);
> > >>> kfree(smu_table->clocks_table);
> > >>> smu_table->gpu_metrics_table = NULL;
> > >>> + smu_table->committed_overdrive_table = NULL;
> > >>> smu_table->boot_overdrive_table = NULL;
> > >>> smu_table->overdrive_table = NULL;
> > >>> smu_table->max_sustainable_clocks = NULL;
> > >>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-23 7:39 ` Lazar, Lijo
@ 2021-07-23 9:11 ` Quan, Evan
0 siblings, 0 replies; 11+ messages in thread
From: Quan, Evan @ 2021-07-23 9:11 UTC (permalink / raw)
To: Lazar, Lijo, amd-gfx; +Cc: Deucher, Alexander
[Public]
I probably get your point. Please check the update V2.
BR
Evan
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> Sent: Friday, July 23, 2021 3:39 PM
> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
> [Public]
>
> Hi Evan,
>
> In that case, using "od_edit" table for the intermediate table
>
> During commit command, what if it's done like
> 1) Copy and update table if od_edit table is different than user_od
> table
> 2) Set the flag to false if od_edit table and boot_od table are
> different
>
> Then current od settings may always be picked from user_od table and the
> flag may be used to distinguish if it's boot od settings or custom settings (for
> restore or other purposes).
>
> Thanks,
> Lijo
>
> -----Original Message-----
> From: Quan, Evan <Evan.Quan@amd.com>
> Sent: Friday, July 23, 2021 12:51 PM
> To: Lazar, Lijo <Lijo.Lazar@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
> [AMD Official Use Only]
>
> Hi Lijo,
>
> Sorry, I doubled checked. The implementation of Navi1x is right.
> The original design for "restores to default settings" is a two-step process.
> That means for "case PP_OD_COMMIT_DPM_TABLE:" we must distinguish
> whether it's an usual customized od setting update or just restoring to
> defaults.
> And my current implementation for that is as below. Any comments?
>
> + if (memcmp(table_context->overdrive_table, table_context-
> >boot_overdrive_table,
> + sizeof(OverDriveTable_t))) {
> + smu->user_dpm_profile.committed_od = true;
> + memcpy(table_context->committed_overdrive_table,
> table_context->overdrive_table,
> + sizeof(OverDriveTable_t));
> + } else {
> + smu->user_dpm_profile.committed_od = false;
> + }
>
> BR
> Evan
> > -----Original Message-----
> > From: Quan, Evan
> > Sent: Friday, July 23, 2021 2:48 PM
> > To: Lazar, Lijo <Lijo.Lazar@amd.com>; amd-gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> > Subject: RE: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > settings properly for NV1x
> >
> > [AMD Official Use Only]
> >
> >
> >
> > > -----Original Message-----
> > > From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > > Sent: Thursday, July 22, 2021 5:03 PM
> > > To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> > > Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > > settings properly for NV1x
> > >
> > >
> > >
> > > On 7/22/2021 2:03 PM, Quan, Evan wrote:
> > > > [AMD Official Use Only]
> > > >
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > > >> Sent: Thursday, July 22, 2021 4:10 PM
> > > >> To: Quan, Evan <Evan.Quan@amd.com>; amd-
> > gfx@lists.freedesktop.org
> > > >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> > > >> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD
> > > >> settings properly for NV1x
> > > >>
> > > >>
> > > >>
> > > >> On 7/22/2021 8:50 AM, Evan Quan wrote:
> > > >>> The customized OD settings can be divided into two parts: those
> > > >>> committed ones and non-committed ones.
> > > >>> - For those changes which had been fed to SMU before
> > S3/S4/Runpm
> > > >>> suspend kicked, they are committed changes. They should be
> > > properly
> > > >>> restored and fed to SMU on S3/S4/Runpm resume.
> > > >>> - For those non-committed changes, they are restored only
> > > >>> without
> > > >> feeding
> > > >>> to SMU.
> > > >>>
> > > >>> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > > >>> Signed-off-by: Evan Quan <evan.quan@amd.com>
> > > >>> ---
> > > >>> drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > > >>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 ++++
> > > >>> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 52
> > > >> ++++++++++++++-----
> > > >>> .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 12 +++++
> > > >>> 4 files changed, 69 insertions(+), 12 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > > >>> b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > > >>> index 3e89852e4820..8ba53f16d2d9 100644
> > > >>> --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > > >>> +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > > >>> @@ -231,6 +231,7 @@ struct smu_user_dpm_profile {
> > > >>> uint32_t power_limit;
> > > >>> uint32_t fan_speed_percent;
> > > >>> uint32_t flags;
> > > >>> + uint32_t committed_od;
> > > >>>
> > > >>> /* user clock state information */
> > > >>> uint32_t clk_mask[SMU_CLK_COUNT]; @@ -352,6 +353,7
> > @@ struct
> > > >>> smu_table_context
> > > >>>
> > > >>> void *overdrive_table;
> > > >>> void *boot_overdrive_table;
> > > >>> + void *committed_overdrive_table;
> > > >>
> > > >> May be rename it to user_overdrive_table - OD table with user
> settings?
> > > > [Quan, Evan] Actually "overdrive_table " is the
> > > > user_overdrive_table. It
> > > contains all the modification including those not committed( the
> > > commit is performed by echo "c" >
> > /sys/class/drm/card1/device/pp_od_clk_voltage).
> > > > The new member added refers to those user customized but
> committed
> > > only settings. That's why it was named as " committed_overdrive_table".
> > > Any good suggestion for the naming?
> > >
> > > As far as I understand, the problem is overdrive_table can have
> > > intemediary settings as the edit/commit is a two-step process. At
> > > any point we can have user_od_table keep the latest user mode settings.
> > > That is true when user restores to default settings also; that time
> > > we can just copy the boot settings back to user_od table and keep
> > > the flag as false indicating that there is no custom settings.
> > [Quan, Evan] For now, on Navi1x the "restores to default settings" is
> > also a two-step process. That will make "copy the boot settings back
> > to user_od table and keep the flag as false" tricky. However, it seems
> > that does not fit original design. As per original design, the "restores to
> default settings"
> > should be a one-step process. Let me correct them with new patch
> > series and proceed further discussion then.
> >
> > BR
> > Evan
> > >
> > > >>
> > > >>> uint32_t gpu_metrics_table_size;
> > > >>> void *gpu_metrics_table;
> > > >>> @@ -623,6 +625,12 @@ struct pptable_funcs {
> > > >>> enum
> > PP_OD_DPM_TABLE_COMMAND
> > > >> type,
> > > >>> long *input, uint32_t size);
> > > >>>
> > > >>> + /**
> > > >>> + * @restore_committed_od_settings: Restore the
> customized and
> > > >> committed
> > > >>> + * OD settings on S3/S4/Runpm resume.
> > > >>> + */
> > > >>> + int (*restore_committed_od_settings)(struct smu_context
> *smu);
> > > >>> +
> > > >>> /**
> > > >>> * @get_clock_by_type_with_latency: Get the speed and
> > latency
> > > >>> of
> > > >> a clock
> > > >>> * domain.
> > > >>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > >>> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > >>> index ebe672142808..5f7d98e99f76 100644
> > > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > >>> @@ -416,6 +416,15 @@ static void
> > > smu_restore_dpm_user_profile(struct
> > > >> smu_context *smu)
> > > >>> }
> > > >>> }
> > > >>>
> > > >>> + /* Restore customized and committed OD settings */
> > > >>> + if (smu->user_dpm_profile.committed_od) {
> > > >>> + if (smu->ppt_funcs-
> >restore_committed_od_settings) {
> > > >>> + ret = smu->ppt_funcs-
> > > >>> restore_committed_od_settings(smu);
> > > >>> + if (ret)
> > > >>> + dev_err(smu->adev->dev, "Failed to
> upload
> > > >> customized OD settings\n");
> > > >>> + }
> > > >>> + }
> > > >>> +
> > > >>
> > > >> Just thinking if there is a need to handle it ASIC specific. The
> > > >> flags and table pointer are maintained in common structure. So
> > > >> can't we do the restore of user OD settings directly in this
> > > >> common flow instead of having each ASIC to implement the callback?
> > > > [Quan, Evan] The OD settings restoring is ASIC specific as it
> > > > performed on
> > > OD table and that(OD table) is ASIC specific.
> > >
> > > I was thinking in terms of final logic that is being done. The below
> > > structures are available in common table context and we have a
> > > common logic to update the table. If there is no custom OD settings
> > > available, we could handle it with the flag.
> > >
> > > + struct smu_table_context *table_context = &smu->smu_table;
> > > + void *od_table = table_context->committed_overdrive_table;
> > > + int ret = 0;
> > > +
> > > + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> > > (void *)od_table, true);
> > >
> > > >>
> > > >>> /* Disable restore flag */
> > > >>> smu->user_dpm_profile.flags &=
> > > >> ~SMU_DPM_USER_PROFILE_RESTORE;
> > > >>> }
> > > >>> 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 59ea59acfb00..4752299d7f91 100644
> > > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > > >>> @@ -2296,39 +2296,45 @@ static int
> > > >> navi10_set_default_od_settings(struct smu_context *smu)
> > > >>> (OverDriveTable_t *)smu-
> > >smu_table.boot_overdrive_table;
> > > >>> int ret = 0;
> > > >>>
> > > >>> - ret = smu_cmn_update_table(smu,
> SMU_TABLE_OVERDRIVE, 0,
> > > >> (void *)od_table, false);
> > > >>> + ret = smu_cmn_update_table(smu,
> SMU_TABLE_OVERDRIVE, 0,
> > > >> (void
> > > >>> +*)boot_od_table, false);
> > > >>> if (ret) {
> > > >>> dev_err(smu->adev->dev, "Failed to get overdrive
> > table!\n");
> > > >>> return ret;
> > > >>> }
> > > >>>
> > > >>> - if (!od_table->GfxclkVolt1) {
> > > >>> + if (!boot_od_table->GfxclkVolt1) {
> > > >>> ret =
> > navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > > >>> -
> &od_table-
> > > >>> GfxclkVolt1,
> > > >>> -
> od_table-
> > > >>> GfxclkFreq1);
> > > >>> +
> > > >> &boot_od_table->GfxclkVolt1,
> > > >>> +
> > > >> boot_od_table->GfxclkFreq1);
> > > >>> if (ret)
> > > >>> return ret;
> > > >>> }
> > > >>>
> > > >>> - if (!od_table->GfxclkVolt2) {
> > > >>> + if (!boot_od_table->GfxclkVolt2) {
> > > >>> ret =
> > navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > > >>> -
> &od_table-
> > > >>> GfxclkVolt2,
> > > >>> -
> od_table-
> > > >>> GfxclkFreq2);
> > > >>> +
> > > >> &boot_od_table->GfxclkVolt2,
> > > >>> +
> > > >> boot_od_table->GfxclkFreq2);
> > > >>> if (ret)
> > > >>> return ret;
> > > >>> }
> > > >>>
> > > >>> - if (!od_table->GfxclkVolt3) {
> > > >>> + if (!boot_od_table->GfxclkVolt3) {
> > > >>> ret =
> > navi10_overdrive_get_gfx_clk_base_voltage(smu,
> > > >>> -
> &od_table-
> > > >>> GfxclkVolt3,
> > > >>> -
> od_table-
> > > >>> GfxclkFreq3);
> > > >>> +
> > > >> &boot_od_table->GfxclkVolt3,
> > > >>> +
> > > >> boot_od_table->GfxclkFreq3);
> > > >>> if (ret)
> > > >>> return ret;
> > > >>> }
> > > >>>
> > > >>> - memcpy(boot_od_table, od_table,
> sizeof(OverDriveTable_t));
> > > >>> + navi10_dump_od_table(smu, boot_od_table);
> > > >>>
> > > >>> - navi10_dump_od_table(smu, od_table);
> > > >>> + /*
> > > >>> + * For S3/S4/Runpm, no need to install boot od table to
> > > >> overdrive_table as
> > > >>> + * - either they already share the same OD settings on
> bootup
> > > >>> + * - or they have user customized OD settings
> > > >>> + */
> > > >>> + if (!smu->adev->in_suspend)
> > > >>> + memcpy(od_table, boot_od_table,
> > > >> sizeof(OverDriveTable_t));
> > > >>>
> > > >>> return 0;
> > > >>> }
> > > >>> @@ -2435,6 +2441,14 @@ static int
> > > >>> navi10_od_edit_dpm_table(struct
> > > >> smu_context *smu, enum PP_OD_DPM_TABL
> > > >>> dev_err(smu->adev->dev, "Failed to import
> > > >> overdrive table!\n");
> > > >>> return ret;
> > > >>> }
> > > >>> + if (memcmp(table_context->overdrive_table,
> table_context-
> > > >>> boot_overdrive_table,
> > > >>> + sizeof(OverDriveTable_t))) {
> > > >>
> > > >> Shouldn't this be - compare against the edited settings and last
> > > >> committed settings, overdrive_table vs committed_overdrive_table?
> > > >>
> > > >> Basically, user_od_table can be initialized with boot_od settings
> > > >> and the flag gives the indication that user_od table is being used or
> not.
> > > >> Before updating, check if the edited table (overdrive_table) and
> > > >> the current user_od table are different, then commit the new table.
> > > > [Quan, Evan] Yes, I also considered that. But that cannot handle
> > > > the case
> > > below:
> > > > 1 user made some customizations -> 2 the customizations were
> > > > committed -> 3 user restored to default(boot) OD settings(by "echo
> > > > 'r'")
> > > and committed Although there were some changes between 2 and 3,
> > there
> > > is actually no real customized changes compared to default(boot) OD
> > settings.
> > >
> > > On restore to default, just copy the boot_table settings to user_od
> > > and keep the flag as false. We are using user_od as the latest user
> > > preferred settings and overdrive_table is only used for intermediate
> > updates.
> > >
> > > The flag decides whether to restore or not, but at any point we can
> > > refer the user_od table to look upon the latest preferred user
> > > settings (default or custom).
> > >
> > > Thanks,
> > > Lijo
> > >
> > > >>
> > > >>> + smu->user_dpm_profile.committed_od =
> true;
> > > >>> + memcpy(table_context-
> > > >>> committed_overdrive_table, table_context->overdrive_table,
> > > >>> + sizeof(OverDriveTable_t));
> > > >>> + } else {
> > > >>> + smu->user_dpm_profile.committed_od =
> false;
> > > >>> + }
> > > >>> break;
> > > >>> case PP_OD_EDIT_VDDC_CURVE:
> > > >>> if (!navi10_od_feature_is_supported(od_settings,
> > > >>> SMU_11_0_ODCAP_GFXCLK_CURVE)) { @@ -2499,6 +2513,19 @@
> > static
> > > int
> > > >> navi10_od_edit_dpm_table(struct smu_context *smu, enum
> > > PP_OD_DPM_TABL
> > > >>> return ret;
> > > >>> }
> > > >>>
> > > >>> +static int navi10_restore_committed_od_settings(struct
> > > >>> +smu_context
> > > >>> +*smu) {
> > > >>> + struct smu_table_context *table_context = &smu-
> >smu_table;
> > > >>> + void *od_table = table_context-
> >committed_overdrive_table;
> > > >>> + int ret = 0;
> > > >>> +
> > > >>> + ret = smu_cmn_update_table(smu,
> SMU_TABLE_OVERDRIVE, 0,
> > > >> (void *)od_table, true);
> > > >>> + if (ret)
> > > >>> + dev_err(smu->adev->dev, "Failed to import
> overdrive
> > > >> table!\n");
> > > >>> +
> > > >>> + return ret;
> > > >>> +}
> > > >>
> > > >> As mentioned before, not sure if this is needed as callback. Even
> > > >> if, this can be something common for smuv11, there is nothing
> > > >> ASIC specific in this logic, right?
> > > > [Quan, Evan] Yes, in patch2 of the series, it was made as a SMUV11
> > > common API.
> > > >
> > > > BR
> > > > Evan
> > > >>
> > > >> Thanks,
> > > >> Lijo
> > > >>
> > > >>> static int navi10_run_btc(struct smu_context *smu)
> > > >>> {
> > > >>> int ret = 0;
> > > >>> @@ -3262,6 +3289,7 @@ static const struct pptable_funcs
> > > >> navi10_ppt_funcs = {
> > > >>> .set_soft_freq_limited_range =
> > > >> smu_v11_0_set_soft_freq_limited_range,
> > > >>> .set_default_od_settings = navi10_set_default_od_settings,
> > > >>> .od_edit_dpm_table = navi10_od_edit_dpm_table,
> > > >>> + .restore_committed_od_settings =
> > > >>> +navi10_restore_committed_od_settings,
> > > >>> .run_btc = navi10_run_btc,
> > > >>> .set_power_source = smu_v11_0_set_power_source,
> > > >>> .get_pp_feature_mask = smu_cmn_get_pp_feature_mask,
> > diff --git
> > > >>> a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > > >>> b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > > >>> index 0a5d46ac9ccd..28fc3f17c1b1 100644
> > > >>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > > >>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
> > > >>> @@ -422,10 +422,20 @@ int smu_v11_0_init_smc_tables(struct
> > > >> smu_context *smu)
> > > >>> ret = -ENOMEM;
> > > >>> goto err3_out;
> > > >>> }
> > > >>> +
> > > >>> + smu_table->committed_overdrive_table =
> > > >>> + kzalloc(tables[SMU_TABLE_OVERDRIVE].size,
> > > >> GFP_KERNEL);
> > > >>> + if (!smu_table->committed_overdrive_table) {
> > > >>> + ret = -ENOMEM;
> > > >>> + goto err4_out;
> > > >>> + }
> > > >>> +
> > > >>> }
> > > >>>
> > > >>> return 0;
> > > >>>
> > > >>> +err4_out:
> > > >>> + kfree(smu_table->boot_overdrive_table);
> > > >>> err3_out:
> > > >>> kfree(smu_table->overdrive_table);
> > > >>> err2_out:
> > > >>> @@ -442,12 +452,14 @@ int smu_v11_0_fini_smc_tables(struct
> > > >> smu_context *smu)
> > > >>> struct smu_dpm_context *smu_dpm = &smu->smu_dpm;
> > > >>>
> > > >>> kfree(smu_table->gpu_metrics_table);
> > > >>> + kfree(smu_table->committed_overdrive_table);
> > > >>> kfree(smu_table->boot_overdrive_table);
> > > >>> kfree(smu_table->overdrive_table);
> > > >>> kfree(smu_table->max_sustainable_clocks);
> > > >>> kfree(smu_table->driver_pptable);
> > > >>> kfree(smu_table->clocks_table);
> > > >>> smu_table->gpu_metrics_table = NULL;
> > > >>> + smu_table->committed_overdrive_table = NULL;
> > > >>> smu_table->boot_overdrive_table = NULL;
> > > >>> smu_table->overdrive_table = NULL;
> > > >>> smu_table->max_sustainable_clocks = NULL;
> > > >>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-22 3:20 [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x Evan Quan
2021-07-22 3:20 ` [PATCH 2/2] drm/amd/pm: restore user customized OD settings properly for Sienna Cichlid Evan Quan
2021-07-22 8:10 ` [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x Lazar, Lijo
@ 2021-07-23 19:59 ` Matt Coffin
2021-07-26 4:12 ` Quan, Evan
2 siblings, 1 reply; 11+ messages in thread
From: Matt Coffin @ 2021-07-23 19:59 UTC (permalink / raw)
To: Evan Quan; +Cc: amd-gfx
On 7/21/21 9:20 PM, Evan Quan wrote:
> The customized OD settings can be divided into two parts: those
> committed ones and non-committed ones.
> - For those changes which had been fed to SMU before S3/S4/Runpm
> suspend kicked, they are committed changes. They should be properly
> restored and fed to SMU on S3/S4/Runpm resume.
> - For those non-committed changes, they are restored only without feeding
> to SMU.
>
> Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> ---
It's been a while since I've been through the SMU code so I don't know
all the places that restore_uesr_profile is used, but, just to confirm,
these would still go back to default (boot) values on a GPU reset, yes?
I ask because when pushing OD settings too far, GPU resets are often
encountered, and you might be able to create a state where it would
continually reset if your OD settings are remembered even through a full
reset.
Very nice feature other than that, though, as I actually keep
`radeontop` open in a tmux session to prevent my secondary card from
resetting my OD settings, so this will be nice.
Thanks for listening, and for the driver in general,
Matt
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
2021-07-23 19:59 ` Matt Coffin
@ 2021-07-26 4:12 ` Quan, Evan
0 siblings, 0 replies; 11+ messages in thread
From: Quan, Evan @ 2021-07-26 4:12 UTC (permalink / raw)
To: Matt Coffin; +Cc: amd-gfx
[AMD Official Use Only]
Thanks for reminder. As I confirmed, the OD settings will be reset to default after gpu reset.
As smu->adev->in_suspend is 0 during gpu reset. And that will reset OD settings to default.
BR
Evan
> -----Original Message-----
> From: Matt Coffin <mcoffin13@gmail.com>
> Sent: Saturday, July 24, 2021 4:00 AM
> To: Quan, Evan <Evan.Quan@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/2] drm/amd/pm: restore user customized OD settings
> properly for NV1x
>
> On 7/21/21 9:20 PM, Evan Quan wrote:
> > The customized OD settings can be divided into two parts: those
> > committed ones and non-committed ones.
> > - For those changes which had been fed to SMU before S3/S4/Runpm
> > suspend kicked, they are committed changes. They should be properly
> > restored and fed to SMU on S3/S4/Runpm resume.
> > - For those non-committed changes, they are restored only without
> feeding
> > to SMU.
> >
> > Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440
> > Signed-off-by: Evan Quan <evan.quan@amd.com>
> > ---
>
> It's been a while since I've been through the SMU code so I don't know all
> the places that restore_uesr_profile is used, but, just to confirm, these
> would still go back to default (boot) values on a GPU reset, yes?
>
> I ask because when pushing OD settings too far, GPU resets are often
> encountered, and you might be able to create a state where it would
> continually reset if your OD settings are remembered even through a full
> reset.
>
> Very nice feature other than that, though, as I actually keep `radeontop`
> open in a tmux session to prevent my secondary card from resetting my OD
> settings, so this will be nice.
>
> Thanks for listening, and for the driver in general, Matt
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-07-26 4:12 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-22 3:20 [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x Evan Quan
2021-07-22 3:20 ` [PATCH 2/2] drm/amd/pm: restore user customized OD settings properly for Sienna Cichlid Evan Quan
2021-07-22 8:10 ` [PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x Lazar, Lijo
2021-07-22 8:33 ` Quan, Evan
2021-07-22 9:03 ` Lazar, Lijo
2021-07-23 6:47 ` Quan, Evan
2021-07-23 7:20 ` Quan, Evan
2021-07-23 7:39 ` Lazar, Lijo
2021-07-23 9:11 ` Quan, Evan
2021-07-23 19:59 ` Matt Coffin
2021-07-26 4:12 ` Quan, Evan
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.