On 7/22/2021 2:03 PM, Quan, Evan wrote:
[AMD Official Use Only]-----Original Message----- From: Lazar, Lijo <Lijo.Lazar@xxxxxxx> Sent: Thursday, July 22, 2021 4:10 PM To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx> 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 withoutfeedingto SMU. Change-Id: Iea7cf7908dfcd919a4d0205e10bff91b1149a440 Signed-off-by: Evan Quan <evan.quan@xxxxxxx> --- 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_COMMANDtype,long *input, uint32_t size); + /** + * @restore_committed_od_settings: Restore the customized andcommitted+ * 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 ofa 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(structsmu_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 uploadcustomized 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 intnavi10_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 tooverdrive_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(structsmu_context *smu, enum PP_OD_DPM_TABLdev_err(smu->adev->dev, "Failed to importoverdrive 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 intnavi10_od_edit_dpm_table(struct smu_context *smu, enum PP_OD_DPM_TABLreturn 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 overdrivetable!\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 EvanThanks, Lijostatic int navi10_run_btc(struct smu_context *smu) { int ret = 0; @@ -3262,6 +3289,7 @@ static const struct pptable_funcsnavi10_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(structsmu_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(structsmu_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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
