[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Thursday, July 22, 2021 5:03 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 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
> >>> without
> >> feeding
> >>> to 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.
[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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
