[Public]
Hi Guchun,
Currently, the smu->user_dpm_profile.flags seems only used to flag whether we are in a process restoring the user customized settings(power limit/fan settings/od settings etc). We could either drop it or expand its usage. Let me consider more and make a follow-up patch for this.
BR
Evan
> -----Original Message-----
> From: Chen, Guchun <Guchun.Chen@xxxxxxx>
> Sent: Friday, July 23, 2021 9:45 PM
> To: Lazar, Lijo <Lijo.Lazar@xxxxxxx>; Quan, Evan <Evan.Quan@xxxxxxx>;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Subject: RE: [PATCH V2 1/2] drm/amd/pm: restore user customized OD
> settings properly for NV1x
>
> [Public]
>
> Just curious that in the patch, it adds a variable *user_od* serving the check
> of applying user customized OD setting. Instead of this, does it make sense
> to use the *flag* in struct smu_user_dpm_profile for this? As we have below
> bit in pm/inc/amdgpu_smu.h, can we add another bit for OD restore? This
> will help unify the usage of *flag* in SMU.
>
> #define SMU_DPM_USER_PROFILE_RESTORE (1 << 0)
>
> Regards,
> Guchun
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Lazar, Lijo
> Sent: Friday, July 23, 2021 6:09 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Subject: Re: [PATCH V2 1/2] drm/amd/pm: restore user customized OD
> settings properly for NV1x
>
> The series looks good to me, though I prefer to use a common logic to
> restore od settings so that smuv12,smuv13 gets the restore feature by
> default once they add the user table logic. Don't have strong argument for it
> unless Alex, Kenneth or others have some comments.
>
> Anyway, the series is
> Reviewed-by: Lijo Lazar <lijo.lazar@xxxxxxx>
>
> On 7/23/2021 2:39 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@xxxxxxx>
> > --
> > v1->v2
> > - better naming and logic revised for checking OD setting
> > update(Lijo)
> > ---
> > drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 8 +++
> > drivers/gpu/drm/amd/pm/inc/smu_v11_0.h | 2 +
> > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 9 +++
> > .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 55
> +++++++++++++------
> > .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 25 +++++++++
> > 5 files changed, 82 insertions(+), 17 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..c2c201b8e3cf 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 user_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 *user_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_user_od_settings: Restore the user customized
> > + * OD settings on S3/S4/Runpm resume.
> > + */
> > + int (*restore_user_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/inc/smu_v11_0.h
> > b/drivers/gpu/drm/amd/pm/inc/smu_v11_0.h
> > index 385b2ea5379c..1e42aafbb9fd 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_user_od_settings(struct smu_context *smu);
> > +
> > #endif
> > #endif
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index ebe672142808..8ca7337ea5fc 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 user customized OD settings */
> > + if (smu->user_dpm_profile.user_od) {
> > + if (smu->ppt_funcs->restore_user_od_settings) {
> > + ret = smu->ppt_funcs-
> >restore_user_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..d7722c229ddd 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> > @@ -2294,41 +2294,52 @@ static int
> navi10_set_default_od_settings(struct smu_context *smu)
> > (OverDriveTable_t *)smu->smu_table.overdrive_table;
> > OverDriveTable_t *boot_od_table =
> > (OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
> > + OverDriveTable_t *user_od_table =
> > + (OverDriveTable_t *)smu->smu_table.user_overdrive_table;
> > int ret = 0;
> >
> > - ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> (void *)od_table, false);
> > + /*
> > + * For S3/S4/Runpm resume, no need to setup those overdrive
> tables again as
> > + * - either they already have the default OD settings got during cold
> bootup
> > + * - or they have some user customized OD settings which cannot be
> overwritten
> > + */
> > + if (smu->adev->in_suspend)
> > + return 0;
> > +
> > + 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);
> > + memcpy(od_table, boot_od_table, sizeof(OverDriveTable_t));
> > + memcpy(user_od_table, boot_od_table, sizeof(OverDriveTable_t));
> >
> > return 0;
> > }
> > @@ -2429,11 +2440,20 @@ static int navi10_od_edit_dpm_table(struct
> smu_context *smu, enum PP_OD_DPM_TABL
> > memcpy(table_context->overdrive_table, table_context-
> >boot_overdrive_table, sizeof(OverDriveTable_t));
> > break;
> > case PP_OD_COMMIT_DPM_TABLE:
> > - navi10_dump_od_table(smu, od_table);
> > - 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;
> > + if (memcmp(od_table, table_context->user_overdrive_table,
> sizeof(OverDriveTable_t))) {
> > + navi10_dump_od_table(smu, od_table);
> > + 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;
> > + }
> > + memcpy(table_context->user_overdrive_table,
> od_table, sizeof(OverDriveTable_t));
> > + smu->user_dpm_profile.user_od = true;
> > +
> > + if (!memcmp(table_context->user_overdrive_table,
> > + table_context->boot_overdrive_table,
> > + sizeof(OverDriveTable_t)))
> > + smu->user_dpm_profile.user_od = false;
> > }
> > break;
> > case PP_OD_EDIT_VDDC_CURVE:
> > @@ -3262,6 +3282,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_user_od_settings = smu_v11_0_restore_user_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..7aa47dbba8d8 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->user_overdrive_table =
> > + kzalloc(tables[SMU_TABLE_OVERDRIVE].size,
> GFP_KERNEL);
> > + if (!smu_table->user_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->user_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->user_overdrive_table = NULL;
> > smu_table->boot_overdrive_table = NULL;
> > smu_table->overdrive_table = NULL;
> > smu_table->max_sustainable_clocks = NULL; @@ -2101,3 +2113,16
> @@
> > int smu_v11_0_deep_sleep_control(struct smu_context *smu,
> >
> > return ret;
> > }
> > +
> > +int smu_v11_0_restore_user_od_settings(struct smu_context *smu) {
> > + struct smu_table_context *table_context = &smu->smu_table;
> > + void *user_od_table = table_context->user_overdrive_table;
> > + int ret = 0;
> > +
> > + ret = smu_cmn_update_table(smu, SMU_TABLE_OVERDRIVE, 0,
> (void *)user_od_table, true);
> > + if (ret)
> > + dev_err(smu->adev->dev, "Failed to import overdrive
> table!\n");
> > +
> > + return ret;
> > +}
> >
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.
> freedesktop.org%2Fmailman%2Flistinfo%2Famd-
> gfx&data=04%7C01%7Cguchun.chen%40amd.com%7Cad0c1951c4f54d7
> a88c508d94dc1f9d6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C
> 637626317788023084%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM
> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata
> =tq9QDYaQmRxqBNRpFZgRQ0DPBSO6AI3YFN033RQUgOI%3D&reserve
> d=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
