Re: [PATCH 2/2] drm/amd/pm: Fix navi10 incorrect OD volage after resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 3/9/2023 8:11 AM, Quan, Evan wrote:
[AMD Official Use Only - General]



-----Original Message-----
From: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
Sent: Wednesday, March 8, 2023 11:20 PM
To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Błażej Szczygieł
<mumei6102@xxxxxxxxx>; Quan, Evan <Evan.Quan@xxxxxxx>
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@xxxxxxxxx>

Cc: Błażej Szczygieł <mumei6102@xxxxxxxxx>
Cc: Evan Quan <evan.quan@xxxxxxx>
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
---
  .../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



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux