[PATCH 0/8] SI power management fixes (v2)

[PATCH 0/8] SI power management fixes (v2)

[Timur Kristóf]

This series fixes some power management issues on SI,
addressing the feedback I got for the first version,
as well as additionally dealing with the issue of higher
refresh rate displays with the non-DC display code.

I recommend backporting these commits to the stable kernel
because they fix some issues that have been there since
the beginning.

Note, I decided not to include the radeon backports in
this series and instead will tackle that separately
once this one is accepted.

Timur Kristóf (8):
  drm/amdgpu: Power up UVD 3 for FW validation (v2)
  drm/amd/pm: Disable ULV even if unsupported
  drm/amd/pm: Fix si_upload_smc_data (v2)
  drm/amd/pm: Fix si_upload_smc_data register programming (v2)
  drm/amd/pm: Treat zero vblank time as too short in si_dpm (v2)
  drm/amd/pm: Disable MCLK switching with non-DC at 120 Hz+ (v2)
  drm/amd/pm: Fix SI DPM issues with high pixel clock (v2)
  drm/amd/pm: Remove wm_low and wm_high fields from amdgpu_crtc (v2)

 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h     |  2 -
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c       |  3 +-
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c       |  3 +-
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c        |  1 -
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c        |  3 +-
 drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c        | 29 ++++++--
 drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c |  7 ++
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c   | 75 +++++++++++++++-----
 8 files changed, 93 insertions(+), 30 deletions(-)

-- 
2.50.1

[PATCH 1/8] drm/amdgpu: Power up UVD 3 for FW validation (v2)

[Timur Kristóf]

Unlike later versions, UVD 3 has firmware validation.
For this to work, the UVD should be powered up correctly.

When DPM is enabled and the display clock is off,
the SMU may choose a power state which doesn't power
the UVD, which can result in failure to initialize UVD.

v2:
Add code comments to explain about the UVD power state
and how UVD clock is turned on/off.

Fixes: b38f3e80ecec ("drm amdgpu: SI UVD v3_1 (v2)")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c | 29 +++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
index 5dbaebb592b3..2e79a3afc774 100644
--- a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
@@ -623,7 +623,22 @@ static void uvd_v3_1_enable_mgcg(struct amdgpu_device *adev,
  *
  * @ip_block: Pointer to the amdgpu_ip_block for this hw instance.
  *
- * Initialize the hardware, boot up the VCPU and do some testing
+ * Initialize the hardware, boot up the VCPU and do some testing.
+ *
+ * On SI, the UVD is meant to be used in a specific power state,
+ * or alternatively the driver can manually enable its clock.
+ * In amdgpu we use the dedicated UVD power state when DPM is enabled.
+ * Calling amdgpu_dpm_enable_uvd makes DPM select the UVD power state
+ * for the SMU and afterwards enables the UVD clock.
+ * This is automatically done by amdgpu_uvd_ring_begin_use when work
+ * is submitted to the UVD ring. Here, we have to call it manually
+ * in order to power up UVD before firmware validation.
+ *
+ * Note that we must not disable the UVD clock here, as that would
+ * cause the ring test to fail. However, UVD is powered off
+ * automatically after the ring test: amdgpu_uvd_ring_end_use calls
+ * the UVD idle work handler which will disable the UVD clock when
+ * all fences are signalled.
  */
 static int uvd_v3_1_hw_init(struct amdgpu_ip_block *ip_block)
 {
@@ -633,6 +648,15 @@ static int uvd_v3_1_hw_init(struct amdgpu_ip_block *ip_block)
 	int r;
 
 	uvd_v3_1_mc_resume(adev);
+	uvd_v3_1_enable_mgcg(adev, true);
+
+	/* Make sure UVD is powered during FW validation.
+	 * It's going to be automatically powered off after the ring test.
+	 */
+	if (adev->pm.dpm_enabled)
+		amdgpu_dpm_enable_uvd(adev, true);
+	else
+		amdgpu_asic_set_uvd_clocks(adev, 53300, 40000);
 
 	r = uvd_v3_1_fw_validate(adev);
 	if (r) {
@@ -640,9 +664,6 @@ static int uvd_v3_1_hw_init(struct amdgpu_ip_block *ip_block)
 		return r;
 	}
 
-	uvd_v3_1_enable_mgcg(adev, true);
-	amdgpu_asic_set_uvd_clocks(adev, 53300, 40000);
-
 	uvd_v3_1_start(adev);
 
 	r = amdgpu_ring_test_helper(ring);
-- 
2.50.1

[PATCH 2/8] drm/amd/pm: Disable ULV even if unsupported

[Timur Kristóf]

This commit fixes some instability on Tahiti.

Sometimes UVD initialization would fail when using DC.
I suspect this is because DC doesn't immediately turn on the
display clock, so it changes how DPM behaves.

Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index 52e732be59e3..33b9d4beec84 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -5639,10 +5639,13 @@ static int si_disable_ulv(struct amdgpu_device *adev)
 {
 	struct si_power_info *si_pi = si_get_pi(adev);
 	struct si_ulv_param *ulv = &si_pi->ulv;
+	PPSMC_Result r;
 
+	r = amdgpu_si_send_msg_to_smc(adev, PPSMC_MSG_DisableULV);
+
+	/* Only care about SMC reply when ULV is supported. */
 	if (ulv->supported)
-		return (amdgpu_si_send_msg_to_smc(adev, PPSMC_MSG_DisableULV) == PPSMC_Result_OK) ?
-			0 : -EINVAL;
+		return (r == PPSMC_Result_OK) ? 0 : -EINVAL;
 
 	return 0;
 }
-- 
2.50.1

[PATCH 3/8] drm/amd/pm: Fix si_upload_smc_data (v2)

[Timur Kristóf]

The si_upload_smc_data function uses si_write_smc_soft_register
to set some register values in the SMC, and expects the result
to be PPSMC_Result_OK which is 1.

The PPSMC_Result_OK / PPSMC_Result_Failed values are used for
checking the result of a command sent to the SMC.
However, the si_write_smc_soft_register actually doesn't send
any commands to the SMC and returns zero on success,
so this check was incorrect.

Fix that by not checking the return value, just like other
calls to si_write_smc_soft_register.
This seems to fix an SMC hang that happens in si_set_sw_state.

Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 26 +++++++++++-----------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index 33b9d4beec84..b16009d342c3 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -5834,23 +5834,23 @@ static int si_upload_smc_data(struct amdgpu_device *adev)
 	if (amdgpu_crtc == NULL)
 		return 0;
 
-	if (amdgpu_crtc->line_time <= 0)
-		return 0;
+	int first_crtc_id = amdgpu_crtc->crtc_id;
+	int first_crtc_line_time = amdgpu_crtc->line_time;
 
-	if (si_write_smc_soft_register(adev,
-				       SI_SMC_SOFT_REGISTER_crtc_index,
-				       amdgpu_crtc->crtc_id) != PPSMC_Result_OK)
+	if (first_crtc_line_time <= 0)
 		return 0;
 
-	if (si_write_smc_soft_register(adev,
-				       SI_SMC_SOFT_REGISTER_mclk_change_block_cp_min,
-				       amdgpu_crtc->wm_high / amdgpu_crtc->line_time) != PPSMC_Result_OK)
-		return 0;
+	si_write_smc_soft_register(adev,
+		SI_SMC_SOFT_REGISTER_crtc_index,
+		first_crtc_id);
 
-	if (si_write_smc_soft_register(adev,
-				       SI_SMC_SOFT_REGISTER_mclk_change_block_cp_max,
-				       amdgpu_crtc->wm_low / amdgpu_crtc->line_time) != PPSMC_Result_OK)
-		return 0;
+	si_write_smc_soft_register(adev,
+		SI_SMC_SOFT_REGISTER_mclk_change_block_cp_min,
+		amdgpu_crtc->wm_high / first_crtc_line_time);
+
+	si_write_smc_soft_register(adev,
+		SI_SMC_SOFT_REGISTER_mclk_change_block_cp_max,
+		amdgpu_crtc->wm_low / first_crtc_line_time);
 
 	return 0;
 }
-- 
2.50.1

[PATCH 4/8] drm/amd/pm: Fix si_upload_smc_data register programming (v2)

[Timur Kristóf]

Based on some comments in dm_pp_display_configuration
above the crtc_index and line_time fields, these values
are programmed to the SMC to work around an SMC hang
when it switches MCLK.

According to Alex, the Windows driver programs them to:
mclk_change_block_cp_min = 200 / line_time
mclk_change_block_cp_max = 100 / line_time
Let's use the same for the sake of consistency.

Previously we used the watermark values, but it seemed buggy
as the code was mixing up low/high and A/B watermarks, and
was not saving a low watermark value on DCE 6, so
mclk_change_block_cp_max would be always zero previously.

Split this change off from the previous si_upload_smc_data
to make it easier to bisect, in case it causes any issues.

Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index b16009d342c3..db46fc0817a7 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -5846,11 +5846,11 @@ static int si_upload_smc_data(struct amdgpu_device *adev)
 
 	si_write_smc_soft_register(adev,
 		SI_SMC_SOFT_REGISTER_mclk_change_block_cp_min,
-		amdgpu_crtc->wm_high / first_crtc_line_time);
+		200 / first_crtc_line_time);
 
 	si_write_smc_soft_register(adev,
 		SI_SMC_SOFT_REGISTER_mclk_change_block_cp_max,
-		amdgpu_crtc->wm_low / first_crtc_line_time);
+		100 / first_crtc_line_time);
 
 	return 0;
 }
-- 
2.50.1

[PATCH 5/8] drm/amd/pm: Treat zero vblank time as too short in si_dpm (v2)

[Timur Kristóf]

Some parts of the code base expect that MCLK switching is turned
off when the vblank time is set to zero.

According to pp_pm_compute_clocks the non-DC code has issues
with MCLK switching with refresh rates over 120 Hz.

Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index db46fc0817a7..1e2aeea0b552 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -3082,8 +3082,8 @@ static bool si_dpm_vblank_too_short(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 	u32 vblank_time = amdgpu_dpm_get_vblank_time(adev);
-	/* we never hit the non-gddr5 limit so disable it */
-	u32 switch_limit = adev->gmc.vram_type == AMDGPU_VRAM_TYPE_GDDR5 ? 450 : 0;
+	/* we never hit the non-gddr5 limit so disable it (but treat 0 as too short) */
+	u32 switch_limit = adev->gmc.vram_type == AMDGPU_VRAM_TYPE_GDDR5 ? 450 : 1;
 
 	if (vblank_time < switch_limit)
 		return true;
-- 
2.50.1

[PATCH 6/8] drm/amd/pm: Disable MCLK switching with non-DC at 120 Hz+ (v2)

[Timur Kristóf]

According to pp_pm_compute_clocks the non-DC display code
has "issues with mclk switching with refresh rates over 120 hz".
The workaround is to disable MCLK switching in this case.

Do the same for legacy DPM.

Fixes: 6ddbd37f1074 ("drm/amd/pm: optimize the amdgpu_pm_compute_clocks() implementations")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c
index 42efe838fa85..2d2d2d5e6763 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm_internal.c
@@ -66,6 +66,13 @@ u32 amdgpu_dpm_get_vblank_time(struct amdgpu_device *adev)
 					(amdgpu_crtc->v_border * 2));
 
 				vblank_time_us = vblank_in_pixels * 1000 / amdgpu_crtc->hw_mode.clock;
+
+				/* we have issues with mclk switching with
+				 * refresh rates over 120 hz on the non-DC code.
+				 */
+				if (drm_mode_vrefresh(&amdgpu_crtc->hw_mode) > 120)
+					vblank_time_us = 0;
+
 				break;
 			}
 		}
-- 
2.50.1

[PATCH 7/8] drm/amd/pm: Fix SI DPM issues with high pixel clock (v2)

[Timur Kristóf]

We define "high pixelclock" for SI as higher than necessary
for 4K 30Hz. For example, 4K 60Hz and 1080p 144Hz fall into
this category.

When a high pixel clock display is connected, always disable
memory clock switching to solve some flickering that can be
observed on 4K 60Hz displays. (It's very subtle but noticeable
on Pitcairn and Tahiti.)

When two high pixel clock displays are connected to Oland,
additionally disable shader clock switching, which results in
a higher voltage, thereby addressing some visible flickering.

Ideally, these issues would be solved by introducing something
like dce_calcs, but we don't have that for old GPUs, so
ad-hoc fixes become necessary.

v2:
Add more comments.

Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index 1e2aeea0b552..3fe6fa564e71 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -3443,12 +3443,14 @@ static void si_apply_state_adjust_rules(struct amdgpu_device *adev,
 {
 	struct  si_ps *ps = si_get_ps(rps);
 	struct amdgpu_clock_and_voltage_limits *max_limits;
+	struct amdgpu_connector *conn;
 	bool disable_mclk_switching = false;
 	bool disable_sclk_switching = false;
 	u32 mclk, sclk;
 	u16 vddc, vddci, min_vce_voltage = 0;
 	u32 max_sclk_vddc, max_mclk_vddci, max_mclk_vddc;
 	u32 max_sclk = 0, max_mclk = 0;
+	u32 high_pixelclock_count = 0;
 	int i;
 
 	if (adev->asic_type == CHIP_HAINAN) {
@@ -3476,6 +3478,42 @@ static void si_apply_state_adjust_rules(struct amdgpu_device *adev,
 		}
 	}
 
+	/* We define "high pixelclock" for SI as higher than necessary for 4K 30Hz.
+	 * For example, 4K 60Hz and 1080p 144Hz fall into this category.
+	 * Find number of such displays connected.
+	 */
+	for (i = 0; i < adev->mode_info.num_crtc; i++) {
+		if (!(adev->pm.dpm.new_active_crtcs & (1 << i)) ||
+			!adev->mode_info.crtcs[i]->enabled)
+			continue;
+
+		conn = to_amdgpu_connector(adev->mode_info.crtcs[i]->connector);
+
+		if (conn->pixelclock_for_modeset > 297000)
+			high_pixelclock_count++;
+	}
+
+	/* These are some ad-hoc fixes to some issues observed with SI GPUs.
+	 * They are necessary because we don't have something like dce_calcs
+	 * for these GPUs to calculate bandwidth requirements.
+	 */
+	if (high_pixelclock_count) {
+		/* High pixelclock displays need higher memory bandwidth.
+		 * Disable memory clock switching in order to:
+		 * - Solve visible display flickering
+		 * - Avoid starving other clients of the memory controller
+		 */
+		disable_mclk_switching = true;
+
+		/* On Oland, we observe some flickering when two 4K 60Hz
+		 * displays are connected, possibly because voltage is too low.
+		 * Raise the voltage by requiring a higher SCLK.
+		 * (Voltage cannot be adjusted independently without also SCLK.)
+		 */
+		if (high_pixelclock_count > 1 && adev->asic_type == CHIP_OLAND)
+			disable_sclk_switching = true;
+	}
+
 	if (rps->vce_active) {
 		rps->evclk = adev->pm.dpm.vce_states[adev->pm.dpm.vce_level].evclk;
 		rps->ecclk = adev->pm.dpm.vce_states[adev->pm.dpm.vce_level].ecclk;
-- 
2.50.1

[PATCH 8/8] drm/amd/pm: Remove wm_low and wm_high fields from amdgpu_crtc (v2)

[Timur Kristóf]

These fields were only used by si_dpm and are not necessary
anymore. They also may have been incorrect because:
- wm_high was set to the LOW_WATERMARK field of watermark A.
- wm_low was not set on DCE 6 and was always zero.

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 2 --
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c   | 3 +--
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c   | 3 +--
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c    | 1 -
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c    | 3 +--
 5 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 6da4f946cac0..20460cfd09bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -496,8 +496,6 @@ struct amdgpu_crtc {
 	struct drm_connector *connector;
 	/* for dpm */
 	u32 line_time;
-	u32 wm_low;
-	u32 wm_high;
 	u32 lb_vblank_lead_lines;
 	struct drm_display_mode hw_mode;
 	/* for virtual dce */
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index bf7c22f81cda..c7b104222a97 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1141,8 +1141,7 @@ static void dce_v10_0_program_watermarks(struct amdgpu_device *adev,
 
 	/* save values for DPM */
 	amdgpu_crtc->line_time = line_time;
-	amdgpu_crtc->wm_high = latency_watermark_a;
-	amdgpu_crtc->wm_low = latency_watermark_b;
+
 	/* Save number of lines the linebuffer leads before the scanout */
 	amdgpu_crtc->lb_vblank_lead_lines = lb_vblank_lead_lines;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 47e05783c4a0..fe8a4f70414a 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1173,8 +1173,7 @@ static void dce_v11_0_program_watermarks(struct amdgpu_device *adev,
 
 	/* save values for DPM */
 	amdgpu_crtc->line_time = line_time;
-	amdgpu_crtc->wm_high = latency_watermark_a;
-	amdgpu_crtc->wm_low = latency_watermark_b;
+
 	/* Save number of lines the linebuffer leads before the scanout */
 	amdgpu_crtc->lb_vblank_lead_lines = lb_vblank_lead_lines;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 276c025c4c03..8ab1f08afcef 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1034,7 +1034,6 @@ static void dce_v6_0_program_watermarks(struct amdgpu_device *adev,
 
 	/* save values for DPM */
 	amdgpu_crtc->line_time = line_time;
-	amdgpu_crtc->wm_high = latency_watermark_a;
 
 	/* Save number of lines the linebuffer leads before the scanout */
 	amdgpu_crtc->lb_vblank_lead_lines = lb_vblank_lead_lines;
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index e62ccf9eb73d..47ed5f1840fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -1096,8 +1096,7 @@ static void dce_v8_0_program_watermarks(struct amdgpu_device *adev,
 
 	/* save values for DPM */
 	amdgpu_crtc->line_time = line_time;
-	amdgpu_crtc->wm_high = latency_watermark_a;
-	amdgpu_crtc->wm_low = latency_watermark_b;
+
 	/* Save number of lines the linebuffer leads before the scanout */
 	amdgpu_crtc->lb_vblank_lead_lines = lb_vblank_lead_lines;
 }
-- 
2.50.1

Re: [PATCH 5/8] drm/amd/pm: Treat zero vblank time as too short in si_dpm (v2)

[Alex Deucher]

On Mon, Aug 25, 2025 at 5:46 PM Timur Kristóf <timur.kristof@gmail.com> wrote:
>
> Some parts of the code base expect that MCLK switching is turned
> off when the vblank time is set to zero.
>
> According to pp_pm_compute_clocks the non-DC code has issues
> with MCLK switching with refresh rates over 120 Hz.
>
> Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>  drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> index db46fc0817a7..1e2aeea0b552 100644
> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> @@ -3082,8 +3082,8 @@ static bool si_dpm_vblank_too_short(void *handle)
>  {
>         struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>         u32 vblank_time = amdgpu_dpm_get_vblank_time(adev);
> -       /* we never hit the non-gddr5 limit so disable it */
> -       u32 switch_limit = adev->gmc.vram_type == AMDGPU_VRAM_TYPE_GDDR5 ? 450 : 0;
> +       /* we never hit the non-gddr5 limit so disable it (but treat 0 as too short) */
> +       u32 switch_limit = adev->gmc.vram_type == AMDGPU_VRAM_TYPE_GDDR5 ? 450 : 1;

Took me a while to wrap my head around this.  It might be clearer to
just return early if the vblank_time is 0.  That said, if there are no
displays attached there is no reason to not enable mclk switching.

Alex

>
>         if (vblank_time < switch_limit)
>                 return true;
> --
> 2.50.1
>

Re: [PATCH 5/8] drm/amd/pm: Treat zero vblank time as too short in si_dpm (v2)

[Timur Kristóf]

On Tue, 2025-08-26 at 16:05 -0400, Alex Deucher wrote:
> On Mon, Aug 25, 2025 at 5:46 PM Timur Kristóf
> <timur.kristof@gmail.com> wrote:
> > 
> > Some parts of the code base expect that MCLK switching is turned
> > off when the vblank time is set to zero.
> > 
> > According to pp_pm_compute_clocks the non-DC code has issues
> > with MCLK switching with refresh rates over 120 Hz.
> > 
> > Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> >  drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > index db46fc0817a7..1e2aeea0b552 100644
> > --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > @@ -3082,8 +3082,8 @@ static bool si_dpm_vblank_too_short(void
> > *handle)
> >  {
> >         struct amdgpu_device *adev = (struct amdgpu_device
> > *)handle;
> >         u32 vblank_time = amdgpu_dpm_get_vblank_time(adev);
> > -       /* we never hit the non-gddr5 limit so disable it */
> > -       u32 switch_limit = adev->gmc.vram_type ==
> > AMDGPU_VRAM_TYPE_GDDR5 ? 450 : 0;
> > +       /* we never hit the non-gddr5 limit so disable it (but
> > treat 0 as too short) */
> > +       u32 switch_limit = adev->gmc.vram_type ==
> > AMDGPU_VRAM_TYPE_GDDR5 ? 450 : 1;
> 
> Took me a while to wrap my head around this.  It might be clearer to
> just return early if the vblank_time is 0. 

Sure, I can do that if you think that makes it easier to read.

> That said, if there are no
> displays attached there is no reason to not enable mclk switching.
> 

The function is only called when there are displays attached.
Should I mention that in a comment?

> 
> > 
> >         if (vblank_time < switch_limit)
> >                 return true;
> > --
> > 2.50.1
> > 

Re: [PATCH 5/8] drm/amd/pm: Treat zero vblank time as too short in si_dpm (v2)

[Alex Deucher]

On Wed, Aug 27, 2025 at 10:27 AM Timur Kristóf <timur.kristof@gmail.com> wrote:
>
> On Tue, 2025-08-26 at 16:05 -0400, Alex Deucher wrote:
> > On Mon, Aug 25, 2025 at 5:46 PM Timur Kristóf
> > <timur.kristof@gmail.com> wrote:
> > >
> > > Some parts of the code base expect that MCLK switching is turned
> > > off when the vblank time is set to zero.
> > >
> > > According to pp_pm_compute_clocks the non-DC code has issues
> > > with MCLK switching with refresh rates over 120 Hz.
> > >
> > > Fixes: 841686df9f7d ("drm/amdgpu: add SI DPM support (v4)")
> > > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > > ---
> > >  drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > > b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > > index db46fc0817a7..1e2aeea0b552 100644
> > > --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > > +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > > @@ -3082,8 +3082,8 @@ static bool si_dpm_vblank_too_short(void
> > > *handle)
> > >  {
> > >         struct amdgpu_device *adev = (struct amdgpu_device
> > > *)handle;
> > >         u32 vblank_time = amdgpu_dpm_get_vblank_time(adev);
> > > -       /* we never hit the non-gddr5 limit so disable it */
> > > -       u32 switch_limit = adev->gmc.vram_type ==
> > > AMDGPU_VRAM_TYPE_GDDR5 ? 450 : 0;
> > > +       /* we never hit the non-gddr5 limit so disable it (but
> > > treat 0 as too short) */
> > > +       u32 switch_limit = adev->gmc.vram_type ==
> > > AMDGPU_VRAM_TYPE_GDDR5 ? 450 : 1;
> >
> > Took me a while to wrap my head around this.  It might be clearer to
> > just return early if the vblank_time is 0.
>
> Sure, I can do that if you think that makes it easier to read.
>
> > That said, if there are no
> > displays attached there is no reason to not enable mclk switching.
> >
>
> The function is only called when there are displays attached.
> Should I mention that in a comment?
>

Sure that sounds good.  Thanks!

Alex

> >
> > >
> > >         if (vblank_time < switch_limit)
> > >                 return true;
> > > --
> > > 2.50.1
> > >