[PATCH 2/2] drm/rockchip: vop: don't round the pixel clock when the encoder owns the PLL

[Vasily Khoruzhick]

On RK3399 the HDMI reference clock is VPLL, a dedicated PLL that is a
parent of the VOP dclk. dw_hdmi_rockchip_mode_valid() accepts a mode
only if VPLL can produce its pixel clock, and encoder mode_set() then
programs VPLL to that rate. However vop_crtc_mode_fixup() ran first,
in the check phase, and rounded adjusted_mode->clock through
clk_round_rate() on the dclk. At that point VPLL still sits at its
previous rate, so the dclk composite picks whichever of VPLL/CPLL/GPLL
gets closest at its *current* rate and stores that inexact value.

For 1366x768 (85.5 MHz) this yields GPLL/7 = 84.857 MHz. mode_set()
then requests 84.857 MHz from VPLL, which the PLL rate table snaps
down to 74.25 MHz, and the VOP ends up on GPLL/7. The panel receives a
timing 0.75% slow, which some monitors misdetect (e.g. as 1195x768)
and display distorted. Only modes whose clock happens to be an exact
GPLL or CPLL fraction (74.25, 148.5, 297 MHz, ...) were unaffected.

Let the encoder tell the CRTC, via a new rockchip_crtc_state flag set
in its atomic_check, that it will program a dedicated dclk parent to
exactly the requested pixel clock. Move the rounding from mode_fixup
to atomic_check, which runs after the encoder's atomic_check as
recommended by the DRM documentation, and skip it when the flag is
set. With VPLL then set to the exact rate before the VOP enables,
clk_set_rate() on the dclk finds an exact match on VPLL.

The flag is only meaningful within the check that sets it and is
cleared when the state is duplicated, so it cannot leak into a later
modeset on the same CRTC with a different encoder. Behaviour for
encoders without a dedicated PLL is unchanged.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Vasily Khoruzhick <anarsoul at gmail.com>
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  8 ++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |  9 ++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 25 +++++++++++++++------
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index b6e154c35e7c..ece44c6ec95c 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -300,8 +300,8 @@ dw_hdmi_rockchip_encoder_atomic_check(struct drm_encoder *encoder,
 				      struct drm_crtc_state *crtc_state,
 				      struct drm_connector_state *conn_state)
 {
-	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
 	struct rockchip_hdmi *hdmi = to_rockchip_hdmi(encoder);
+	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
 	union phy_configure_opts opts = {};
 	u32 bus_format;
 
@@ -327,6 +327,12 @@ dw_hdmi_rockchip_encoder_atomic_check(struct drm_encoder *encoder,
 
 	s->output_type = DRM_MODE_CONNECTOR_HDMIA;
 	s->bus_format = bus_format;
+	/*
+	 * The reference clock (e.g. VPLL on RK3399) is a parent of the VOP
+	 * dclk, and mode_set() programs it to the pixel clock, which
+	 * mode_valid() already guaranteed it can produce.
+	 */
+	s->dclk_exact = !!hdmi->ref_clk;
 
 	if (!hdmi->phy || !conn_state->hdmi.tmds_char_rate)
 		return 0;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 4705dc6b8bd7..8cb828ae9af6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -57,6 +57,15 @@ struct rockchip_crtc_state {
 	u32 bus_format;
 	u32 bus_flags;
 	int color_space;
+	/*
+	 * Set by an encoder's atomic_check when it owns a dedicated PLL that
+	 * feeds the CRTC's dclk and will program it to exactly
+	 * adjusted_mode->clock at mode_set time. The CRTC must then not
+	 * round the pixel clock against the current clock tree, which does
+	 * not reflect that PLL's future rate. Only valid within one check,
+	 * it is cleared when the state is duplicated.
+	 */
+	bool dclk_exact;
 };
 #define to_rockchip_crtc_state(s) \
 		container_of(s, struct rockchip_crtc_state, base)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 0090d8ff0c79..73a92ccbcb94 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1207,11 +1207,9 @@ static enum drm_mode_status vop_crtc_mode_valid(struct drm_crtc *crtc,
 	return MODE_OK;
 }
 
-static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
-				const struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode)
+static void vop_crtc_adjust_clock(struct vop *vop,
+				  struct drm_display_mode *adjusted_mode)
 {
-	struct vop *vop = to_vop(crtc);
 	unsigned long rate;
 
 	/*
@@ -1245,8 +1243,6 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
 		rate = clk_round_rate(vop->dclk,
 				      adjusted_mode->clock * 1000 + 999);
 	adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
-
-	return true;
 }
 
 static bool vop_dsp_lut_is_enabled(struct vop *vop)
@@ -1558,6 +1554,19 @@ static int vop_crtc_atomic_check(struct drm_crtc *crtc,
 	s = to_rockchip_crtc_state(crtc_state);
 	s->enable_afbc = afbc_planes > 0;
 
+	/*
+	 * Round the pixel clock to what the dclk can really produce, unless
+	 * the encoder will program a dedicated dclk parent PLL to exactly
+	 * this rate at mode_set time. In that case the clock tree seen here
+	 * (with that PLL still at its old rate) would pick a worse, inexact
+	 * source and bake that rate into adjusted_mode, defeating the PLL.
+	 *
+	 * Same condition the atomic helpers use for the mode_fixup callback.
+	 */
+	if ((crtc_state->mode_changed || crtc_state->connectors_changed) &&
+	    !s->dclk_exact)
+		vop_crtc_adjust_clock(vop, &crtc_state->adjusted_mode);
+
 	return 0;
 }
 
@@ -1623,7 +1632,6 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 
 static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
 	.mode_valid = vop_crtc_mode_valid,
-	.mode_fixup = vop_crtc_mode_fixup,
 	.atomic_check = vop_crtc_atomic_check,
 	.atomic_begin = vop_crtc_atomic_begin,
 	.atomic_flush = vop_crtc_atomic_flush,
@@ -1643,6 +1651,9 @@ static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
 	if (!rockchip_state)
 		return NULL;
 
+	/* Only valid within the check phase that sets it. */
+	rockchip_state->dclk_exact = false;
+
 	__drm_atomic_helper_crtc_duplicate_state(crtc, &rockchip_state->base);
 	return &rockchip_state->base;
 }
-- 
2.55.0