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 <[email protected]> > --- > 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
