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

[Vasily Khoruzhick]

On Tue, Sep 1, 2026 at 8:50 PM Chaoyi Chen <chaoyi.chen at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clk_summary_1366x768_broken.gz
Type: application/gzip
Size: 6534 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-rockchip/attachments/20260903/ececcfa7/attachment-0002.gz>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clk_summary_1366x768_working.gz
Type: application/gzip
Size: 6517 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-rockchip/attachments/20260903/ececcfa7/attachment-0003.gz>