[PATCH 1/2] drm/amd/pm: restore user customized OD settings properly for NV1x
Lazar, Lijo
lijo.lazar at amd.com
Thu Jul 22 08:10:07 UTC 2021
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 at 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;
>
More information about the amd-gfx
mailing list