[PATCH 1/2] clk: rockchip: rk3399: add 85.5 MHz rate to PLL rate table

[PATCH 1/2] clk: rockchip: rk3399: add 85.5 MHz rate to PLL rate table

[Vasily Khoruzhick]

85.5 MHz is the pixel clock of the standard DMT 1366x768@60 mode,
which is the native mode of many cheap HDMI panels.

Use a 1368 MHz VCO (24 MHz * 57, already used by the existing
1368 MHz entry) with postdiv1 = postdiv2 = 4 to hit 85.5 MHz
exactly in integer mode.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/clk/rockchip/clk-rk3399.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index c2b243d7a5e2..db241d2df625 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -98,6 +98,7 @@ static struct rockchip_pll_rate_table rk3399_pll_rates[] = {
 	RK3036_PLL_RATE( 148500000, 1, 99, 4, 4, 1, 0),
 	RK3036_PLL_RATE( 106500000, 1, 71, 4, 4, 1, 0),
 	RK3036_PLL_RATE(  96000000, 1, 64, 4, 4, 1, 0),
+	RK3036_PLL_RATE(  85500000, 1, 57, 4, 4, 1, 0),
 	RK3036_PLL_RATE(  74250000, 2, 99, 4, 4, 1, 0),
 	RK3036_PLL_RATE(  65000000, 1, 65, 6, 4, 1, 0),
 	RK3036_PLL_RATE(  54000000, 1, 54, 6, 4, 1, 0),
-- 
2.55.0

[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@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

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

[Chaoyi Chen]

Hello Vasily,

On 9/2/2026 7:42 AM, Vasily Khoruzhick wrote:
> 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.
> 

Why did vop_crtc_mode_fixup() run first? Within drm_atomic_helper_check_modeset(), 
mode_valid() is executed before mode_fixup().

> 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.
>

Did you designate VPLL as the parent clock of the VOP dclk in the DTS?

> 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@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;
> 

What about RK3328? It uses hdmi->hdmiphy_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;
>  }
> 

What about other display interfaces? For example, the .mode_fixup 
corresponding to cdn-dp will not be called.

> @@ -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;
>  }

-- 
Best, 
Chaoyi

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

[Vasily Khoruzhick]

[-- Attachment #1: Type: text/plain, Size: 5076 bytes --]

On Tue, Sep 1, 2026 at 8:50 PM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:
>
> Hello Vasily,

Hi Chaoyi,

> On 9/2/2026 7:42 AM, Vasily Khoruzhick wrote:
> > 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.
> >
>
> Why did vop_crtc_mode_fixup() run first? Within drm_atomic_helper_check_modeset(),
> mode_valid() is executed before mode_fixup().

Sorry, ambiguous wording on my part - "first" meant before the
encoder's mode_set() programs the VPLL, not before mode_valid(). The
order is as you say, and that's exactly the problem: mode_valid()
rounds the pixel clock on the ref clock (the VPLL itself), whose rate
table can produce 85.5 MHz, so the mode is accepted. mode_fixup()
however rounds on the dclk composite - a different clock - which
evaluates its mux parents at their current rates. During check the
VPLL still runs at the previous mode's rate, so GPLL/7 = 84.857 MHz
wins and gets stored in adjusted_mode->clock. mode_set() then programs
the VPLL to that corrupted value in the commit phase. I can reword the
commit message to make this clearer.

> > 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.
> >
>
> Did you designate VPLL as the parent clock of the VOP dclk in the DTS?

No, and it isn't needed: in the commit phase the encoder's
atomic_mode_set() sets the VPLL to exactly the pixel clock before
vop_crtc_atomic_enable() sets the dclk (mode-set runs before enables
in the atomic helpers), so by then the dclk composite finds vpll/1 as
an exact match and the mux selects VPLL by itself. Verified via
clk_summary on the patched kernel: vpll = 85500000 feeding dclk_vop0 =
85500000. Pinning the parent in DT also wouldn't have fixed the bug -
without CLK_SET_RATE_PARENT the check-phase clk_round_rate() against
the still-stale VPLL would return the same wrong value.

See clk_summary for broken and working cases attached.

> > @@ -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;
> >
>
> What about RK3328? It uses hdmi->hdmiphy_clk.

RK3328 clock ownership is the inverse of RK3399's. hdmiphy_clk is only
ever consulted (in mode_valid()); the encoder's mode_set() programs
just ref_clk, which is NULL on RK3328, so the encoder never sets the
PHY PLL - the promise dclk_exact expresses ("the encoder will program
this rate itself at mode_set time") wouldn't be true there.

> >  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;
> >  }
> >
>
> What about other display interfaces? For example, the .mode_fixup
> corresponding to cdn-dp will not be called.

The rounding still runs for cdn-dp and all other interfaces: it moved
into vop_crtc_atomic_check(), which is a CRTC callback and is invoked
by drm_atomic_helper_check() regardless of the attached encoder - same
as the old CRTC-level .mode_fixup was.

Regards,
Vasily

[-- Attachment #2: clk_summary_1366x768_broken.gz --]
[-- Type: application/gzip, Size: 6534 bytes --]

[-- Attachment #3: clk_summary_1366x768_working.gz --]
[-- Type: application/gzip, Size: 6517 bytes --]

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

[Chaoyi Chen]

Hello Vasily,

On 9/4/2026 6:19 AM, Vasily Khoruzhick wrote:
> On Tue, Sep 1, 2026 at 8:50 PM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:
>>
>> Hello Vasily,
> 
> Hi Chaoyi,
> 
>> On 9/2/2026 7:42 AM, Vasily Khoruzhick wrote:
>>> 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.
>>>
>>
>> Why did vop_crtc_mode_fixup() run first? Within drm_atomic_helper_check_modeset(),
>> mode_valid() is executed before mode_fixup().
> 
> Sorry, ambiguous wording on my part - "first" meant before the
> encoder's mode_set() programs the VPLL, not before mode_valid(). The
> order is as you say, and that's exactly the problem: mode_valid()
> rounds the pixel clock on the ref clock (the VPLL itself), whose rate
> table can produce 85.5 MHz, so the mode is accepted. mode_fixup()
> however rounds on the dclk composite - a different clock - which
> evaluates its mux parents at their current rates. During check the
> VPLL still runs at the previous mode's rate, so GPLL/7 = 84.857 MHz
> wins and gets stored in adjusted_mode->clock. mode_set() then programs
> the VPLL to that corrupted value in the commit phase. I can reword the
> commit message to make this clearer.
> 

I think I understand your point now. The encoder's
mode_fixup() yields 85.5 MHz, while the CRTC's mode_fixup() yields 
84.857 MHz. The patch effectively bypasses the CRTC mode_fixup(), 
so that both the encoder's mode_set() and the CRTC's atomic_enable() 
clk_set_rate() end up using 85.5 MHz. I think it's worth mentioning
this in the commit message.

>>> 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.
>>>
>>
>> Did you designate VPLL as the parent clock of the VOP dclk in the DTS?
> 
> No, and it isn't needed: in the commit phase the encoder's
> atomic_mode_set() sets the VPLL to exactly the pixel clock before
> vop_crtc_atomic_enable() sets the dclk (mode-set runs before enables
> in the atomic helpers), so by then the dclk composite finds vpll/1 as
> an exact match and the mux selects VPLL by itself. Verified via
> clk_summary on the patched kernel: vpll = 85500000 feeding dclk_vop0 =
> 85500000. Pinning the parent in DT also wouldn't have fixed the bug -
> without CLK_SET_RATE_PARENT the check-phase clk_round_rate() against
> the still-stale VPLL would return the same wrong value.
> 
> See clk_summary for broken and working cases attached.
> 
>>> @@ -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;
>>>
>>
>> What about RK3328? It uses hdmi->hdmiphy_clk.
> 
> RK3328 clock ownership is the inverse of RK3399's. hdmiphy_clk is only
> ever consulted (in mode_valid()); the encoder's mode_set() programs
> just ref_clk, which is NULL on RK3328, so the encoder never sets the
> PHY PLL - the promise dclk_exact expresses ("the encoder will program
> this rate itself at mode_set time") wouldn't be true there.
>

Given the complexity of clock trees across different platforms,
I think a better approach would be to implement functions like
rockchip_drm_dclk_round_rate() and rockchip_drm_dclk_set_rate(), 
which properly configure the parent clock to meet the frequency
requirements. Of course, the current approach is okay for me. :)

>>>  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;
>>>  }
>>>
>>
>> What about other display interfaces? For example, the .mode_fixup
>> corresponding to cdn-dp will not be called.
> 
> The rounding still runs for cdn-dp and all other interfaces: it moved
> into vop_crtc_atomic_check(), which is a CRTC callback and is invoked
> by drm_atomic_helper_check() regardless of the attached encoder - same
> as the old CRTC-level .mode_fixup was.
>

I still don't quite understand why we need to move this to CRTC atomic_check().
What's wrong with checking !s->dclk_exact in CRTC mode_fixup()? Thanks.

> Regards,
> Vasily

-- 
Best, 
Chaoyi

Re: [PATCH 1/2] clk: rockchip: rk3399: add 85.5 MHz rate to PLL rate table

[Alexey Charkov]

Hi Vasily,

On Wed, Sep 2, 2026 at 3:42 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> 85.5 MHz is the pixel clock of the standard DMT 1366x768@60 mode,
> which is the native mode of many cheap HDMI panels.
>
> Use a 1368 MHz VCO (24 MHz * 57, already used by the existing
> 1368 MHz entry) with postdiv1 = postdiv2 = 4 to hit 85.5 MHz
> exactly in integer mode.
>
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  drivers/clk/rockchip/clk-rk3399.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> index c2b243d7a5e2..db241d2df625 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -98,6 +98,7 @@ static struct rockchip_pll_rate_table rk3399_pll_rates[] = {
>         RK3036_PLL_RATE( 148500000, 1, 99, 4, 4, 1, 0),
>         RK3036_PLL_RATE( 106500000, 1, 71, 4, 4, 1, 0),
>         RK3036_PLL_RATE(  96000000, 1, 64, 4, 4, 1, 0),
> +       RK3036_PLL_RATE(  85500000, 1, 57, 4, 4, 1, 0),

There is already an entry for 1368000000, which is 16x your rate, and
the 16x divisor should fit comfortably into the downstream clock's
8-bit divisor field. Do you really need a separate PLL rate? Have you
checked what the hardware arrives at with the unmodified PLL table -
e.g. via /sys/kernel/debug/clk/clk_summary?

Best regards,
Alexey

Re: [PATCH 1/2] clk: rockchip: rk3399: add 85.5 MHz rate to PLL rate table

[Vasily Khoruzhick]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

On Wed, Sep 2, 2026 at 1:43 AM Alexey Charkov <alchark@flipper.net> wrote:
>
> Hi Vasily,

Hey Alexey,

> > diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> > index c2b243d7a5e2..db241d2df625 100644
> > --- a/drivers/clk/rockchip/clk-rk3399.c
> > +++ b/drivers/clk/rockchip/clk-rk3399.c
> > @@ -98,6 +98,7 @@ static struct rockchip_pll_rate_table rk3399_pll_rates[] = {
> >         RK3036_PLL_RATE( 148500000, 1, 99, 4, 4, 1, 0),
> >         RK3036_PLL_RATE( 106500000, 1, 71, 4, 4, 1, 0),
> >         RK3036_PLL_RATE(  96000000, 1, 64, 4, 4, 1, 0),
> > +       RK3036_PLL_RATE(  85500000, 1, 57, 4, 4, 1, 0),
>
> There is already an entry for 1368000000, which is 16x your rate, and
> the 16x divisor should fit comfortably into the downstream clock's
> 8-bit divisor field. Do you really need a separate PLL rate? Have you
> checked what the hardware arrives at with the unmodified PLL table -
> e.g. via /sys/kernel/debug/clk/clk_summary?

See arch/arm64/boot/dts/rockchip/rk3399-base.dtsi, hdmi ref clock is
wired directly to VPLL, and dw_hdmi-rockchip calls clk_set_rate() with
pixel clock for ref clock, see
dw_hdmi_rockchip_encoder_atomic_mode_set(). So at least in the rk3399
case VPLL is supposed to support the required pixel clock. Without the
first patch 1366x768 mode with 85.5MHz pixel clock is just rejected.

Please see clk_summary attached for 1280x800 (no patches applied),
broken 1366x768 (just the first patch applied), and working 1366x768
(both patches applied)

Regards,
Vasily

[-- Attachment #2: clk_summary_1280x800.gz --]
[-- Type: application/gzip, Size: 6512 bytes --]

[-- Attachment #3: clk_summary_1366x768_broken.gz --]
[-- Type: application/gzip, Size: 6534 bytes --]

[-- Attachment #4: clk_summary_1366x768_working.gz --]
[-- Type: application/gzip, Size: 6517 bytes --]

Re: [PATCH 1/2] clk: rockchip: rk3399: add 85.5 MHz rate to PLL rate table

[Alexey Charkov]

On Fri, Sep 4, 2026 at 1:59 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> On Wed, Sep 2, 2026 at 1:43 AM Alexey Charkov <alchark@flipper.net> wrote:
> >
> > Hi Vasily,
>
> Hey Alexey,
>
> > > diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> > > index c2b243d7a5e2..db241d2df625 100644
> > > --- a/drivers/clk/rockchip/clk-rk3399.c
> > > +++ b/drivers/clk/rockchip/clk-rk3399.c
> > > @@ -98,6 +98,7 @@ static struct rockchip_pll_rate_table rk3399_pll_rates[] = {
> > >         RK3036_PLL_RATE( 148500000, 1, 99, 4, 4, 1, 0),
> > >         RK3036_PLL_RATE( 106500000, 1, 71, 4, 4, 1, 0),
> > >         RK3036_PLL_RATE(  96000000, 1, 64, 4, 4, 1, 0),
> > > +       RK3036_PLL_RATE(  85500000, 1, 57, 4, 4, 1, 0),
> >
> > There is already an entry for 1368000000, which is 16x your rate, and
> > the 16x divisor should fit comfortably into the downstream clock's
> > 8-bit divisor field. Do you really need a separate PLL rate? Have you
> > checked what the hardware arrives at with the unmodified PLL table -
> > e.g. via /sys/kernel/debug/clk/clk_summary?
>
> See arch/arm64/boot/dts/rockchip/rk3399-base.dtsi, hdmi ref clock is
> wired directly to VPLL, and dw_hdmi-rockchip calls clk_set_rate() with
> pixel clock for ref clock, see
> dw_hdmi_rockchip_encoder_atomic_mode_set(). So at least in the rk3399
> case VPLL is supposed to support the required pixel clock. Without the
> first patch 1366x768 mode with 85.5MHz pixel clock is just rejected.

I strongly suspect that the dtsi doesn't describe the real clock usage
here. The TRM for RK3399 doesn't show any "ref" clock for HDMI, and no
TRM-documented VPLL users look like anything that could connect
directly to the HDMI controller.

I believe something is hardcoding the divisor (or leaving it at the
power-on default) in the actual DCLK of a VOP which the HDMI
controller uses, instead of modelling it properly as a mux (frac/div)
feeding off VPLL via another mux - both perfectly representable in the
clock framework and already envisaged in the clock driver, making the
PLL table patching unnecessary.

Can you please try re-pointing the "ref" clock at DCLK_VOP0 (or 1,
depending on which one your HDMI controller uses)? Your clock summary
already shows that something is assigning its parent to DCLK_VOPx_DIV
and the latter's parent to PLL_VPLL, so rate changes the current
driver code does on VPLL propagate to the muxed and divided downstream
consumer as a side-effect rather than as an actual rate request.

Best regards,
Alexey