[PATCH 2/2] drm/amd/pm: Fix navi10 incorrect OD volage after resume
Lazar, Lijo
lijo.lazar at amd.com
Thu Mar 9 11:54:42 UTC 2023
On 3/9/2023 8:11 AM, Quan, Evan wrote:
> [AMD Official Use Only - General]
>
>
>
>> -----Original Message-----
>> From: Deucher, Alexander <Alexander.Deucher at amd.com>
>> Sent: Wednesday, March 8, 2023 11:20 PM
>> To: amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Błażej Szczygieł
>> <mumei6102 at gmail.com>; Quan, Evan <Evan.Quan at 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ł
>> <mumei6102 at gmail.com>
>>
>> Cc: Błażej Szczygieł <mumei6102 at gmail.com>
>> Cc: Evan Quan <evan.quan at amd.com>
>> Signed-off-by: Alex Deucher <alexander.deucher at 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.
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
More information about the amd-gfx
mailing list