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

[Chaoyi Chen <chaoyi.chen@rock-chips.com>]

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