On Thu, Mar 9, 2023 at 6:54 AM Lazar, Lijo <lijo.la...@amd.com> wrote:
>
>
>
> On 3/9/2023 8:11 AM, Quan, Evan wrote:
> > [AMD Official Use Only - General]
> >
> >
> >
> >> -----Original Message-----
> >> From: Deucher, Alexander <alexander.deuc...@amd.com>
> >> Sent: Wednesday, March 8, 2023 11:20 PM
> >> To: amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Błażej Szczygieł
> >> <mumei6...@gmail.com>; Quan, Evan <evan.q...@amd.com>
> >> Subject: [PATCH 2/2] drm/amd/pm: Fix navi10 incorrect OD volage after
> >> resume
> >>
> >> Always setup overdrive tables after resume. Preserve only some
> >> user-defined settings in user_overdrive_table if they're set.
> >>
> >> Copy restored user_overdrive_table into od_table to get correct
> >> values.
> >>
> >> On cold boot, BTC was triggered and GfxVfCurve was calibrated. We
> >> got VfCurve settings (a). On resuming back, BTC will be triggered
> >> again and GfxVfCurve will be recalibrated. VfCurve settings (b)
> >> got may be different from those of cold boot. So if we reuse
> >> those VfCurve settings (a) got on cold boot on suspend, we can
> >> run into discrepencies.
> >>
> >> Based on the sienna cichlid patch from Błażej Szczygieł
> >> <mumei6...@gmail.com>
> >>
> >> Cc: Błażej Szczygieł <mumei6...@gmail.com>
> >> Cc: Evan Quan <evan.q...@amd.com>
> >> Signed-off-by: Alex Deucher <alexander.deuc...@amd.com>
> >> ---
> >> .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 47
> >> +++++++++++++++----
> >> 1 file changed, 37 insertions(+), 10 deletions(-)
> >>
> >> 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 95da6dd1cc65..68201d8e1c72 100644
> >> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> >> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
> >> @@ -2510,16 +2510,9 @@ static int navi10_set_default_od_settings(struct
> >> smu_context *smu)
> >> (OverDriveTable_t *)smu->smu_table.boot_overdrive_table;
> >> OverDriveTable_t *user_od_table =
> >> (OverDriveTable_t *)smu->smu_table.user_overdrive_table;
> >> + OverDriveTable_t user_od_table_bak;
> >> int ret = 0;
> >>
> >> - /*
> >> - * 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");
> >> @@ -2553,7 +2546,27 @@ static int navi10_set_default_od_settings(struct
> >> smu_context *smu)
> >> navi10_dump_od_table(smu, boot_od_table);
> >>
> >> memcpy(od_table, boot_od_table, sizeof(OverDriveTable_t));
> >> - memcpy(user_od_table, boot_od_table, sizeof(OverDriveTable_t));
> >> +
> >> + /*
> >> + * For S3/S4/Runpm resume, we need to setup those overdrive
> >> tables again,
> >> + * but we have to preserve user defined values in "user_od_table".
> >> + */
> >> + if (!smu->adev->in_suspend) {
> >> + memcpy(user_od_table, boot_od_table,
> >> sizeof(OverDriveTable_t));
> >> + smu->user_dpm_profile.user_od = false;
> >> + } else if (smu->user_dpm_profile.user_od) {
> >> + memcpy(&user_od_table_bak, user_od_table,
> >> sizeof(OverDriveTable_t));
> >> + memcpy(user_od_table, boot_od_table,
> >> sizeof(OverDriveTable_t));
> >> + user_od_table->GfxclkFmin =
> >> user_od_table_bak.GfxclkFmin;
> >> + user_od_table->GfxclkFmax =
> >> user_od_table_bak.GfxclkFmax;
> >> + user_od_table->UclkFmax = user_od_table_bak.UclkFmax;
> >> + user_od_table->GfxclkFreq1 =
> >> user_od_table_bak.GfxclkFreq1;
> >> + user_od_table->GfxclkVolt1 =
> >> user_od_table_bak.GfxclkVolt1;
> >> + user_od_table->GfxclkFreq2 =
> >> user_od_table_bak.GfxclkFreq2;
> >> + user_od_table->GfxclkVolt2 =
> >> user_od_table_bak.GfxclkVolt2;
> >> + user_od_table->GfxclkFreq3 =
> >> user_od_table_bak.GfxclkFreq3;
> >> + user_od_table->GfxclkVolt3 =
> >> user_od_table_bak.GfxclkVolt3;
> >> + }
> > Thing is a little tricky for navi10...
> > For navi2x, the vfcurve settings(GfxVfCurve.a, GfxVfCurve.b, GfxVfCurve.c)
> > are not configurable by user. We do not expose them to user.
> > So, we can just load the new vfcurve settings on resuming back without
> > worry about overriding user's settings.
> >
> > Unlike navi2x, user can customize the vfcurve settings(by setting
> > GfxclkFreq/GfxVolt pairs) on navi10. More specifically:
> > - On cold boot, btc was triggered and vfcurve line was calibrated
> > - Driver calculated the target voltage(via
> > navi10_overdrive_get_gfx_clk_base_voltage) for the point
> > frequencies(GfxclkFreq1, GfxclkFreq2, GfxclkFreq3) and expose them to user
> > - e.g. point1 frequency/voltage: 500Mhz/ 0.75v
> > - Then user customized the vfcurve line by setting a new target voltage for
> > the point frequency.
> > - e.g. 500Mhz / 0.76v --> 10mv added
> > - On resuming back, the vfcurve line was recalibrated. The target voltage
> > for the point1 frequency may be changed to for example 0.745v(for 500Mhz).
> > Under such scenario, if we just restore user's settings(0.76v which will
> > add 15mv), that might not fit user's original intention.
> >
> > So, for this case:
> > 1. Either we add some new variables to record the voltage offset(10mv) from
> > user's settings. And we restore the target voltage with new vfcurve line
> > and voltage offset(that is 0.745v + 10mv = 0.755v for the case above). But
> > still we cannot know whether it truely reflects user's original intention.
> > 2. Or we just restore back to the new vfcurve line calibrated and remind
> > user that original setting was dropped and they need to set new ones.
> >
>
> As per the current driver logic, user may choose to override (voltage,
> frequency) points to define a new curve. Since the user is defining the
> curve, it's better to restore the same curve.
>
> BTW, this patch doesn't seem to have any effect as the curve will be
> restored properly otherwise also.
>
I'll drop this patch then.
Thanks,
Alex
> Thanks,
> Lijo
>
> > BR
> > Evan
> >>
> >> return 0;
> >> }
> >> @@ -2733,6 +2746,20 @@ static int navi10_od_edit_dpm_table(struct
> >> smu_context *smu, enum PP_OD_DPM_TABL
> >> return ret;
> >> }
> >>
> >> +static int navi10_restore_user_od_settings(struct smu_context *smu)
> >> +{
> >> + struct smu_table_context *table_context = &smu->smu_table;
> >> + OverDriveTable_t *od_table = table_context->overdrive_table;
> >> + OverDriveTable_t *user_od_table = table_context-
> >>> user_overdrive_table;
> >> + int res;
> >> +
> >> + res = smu_v11_0_restore_user_od_settings(smu);
> >> + if (res == 0)
> >> + memcpy(od_table, user_od_table,
> >> sizeof(OverDriveTable_t));
> >> +
> >> + return res;
> >> +}
> >> +
> >> static int navi10_run_btc(struct smu_context *smu)
> >> {
> >> int ret = 0;
> >> @@ -3560,7 +3587,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,
> >> + .restore_user_od_settings = navi10_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,
> >> --
> >> 2.39.2
