[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.la...@amd.com>; amd-gfx@lists.freedesktop.org > Cc: Deucher, Alexander <alexander.deuc...@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.la...@amd.com> > > Sent: Thursday, July 22, 2021 5:03 PM > > To: Quan, Evan <evan.q...@amd.com>; amd-gfx@lists.freedesktop.org > > Cc: Deucher, Alexander <alexander.deuc...@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.la...@amd.com> > > >> Sent: Thursday, July 22, 2021 4:10 PM > > >> To: Quan, Evan <evan.q...@amd.com>; amd- > g...@lists.freedesktop.org > > >> Cc: Deucher, Alexander <alexander.deuc...@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.q...@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