Re: [PATCH v2] drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs

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

Hi Igor,

On 8/13/2026 6:13 PM, Igor Paunovic wrote:
> The review bot found three things on v2 and all three are right. I am
> answering rather than sending a v3 straight away, because the fix for
> the two High ones is a single change that touches a file shared by
> every Rockchip SoC, and I would rather ask about that than guess.
> 
> Both High findings come from the same shortcut. v2 keeps the
> requirement in a global atomic state object, which I still think is the
> right container, but it applies the rate from vop2_crtc_atomic_enable()
> and _disable() rather than from the commit tail:
> 
>   - Out of order commits. Two non-blocking commits on different CRTCs
>     share only the private object, and nothing orders them, so a commit
>     that took its snapshot before another CRTC raised the rate can land
>     after it and lower it again.
> 
>   - Multi-CRTC disable. atomic_disable() runs once per CRTC, and the
>     first one already sees a state in which every participating CRTC is
>     off, so the rate drops while the others are still scanning out and
>     waiting for dsp_hold_completion.
> 
> vc4 solves both of these for its core clock, and what I did was take
> half of that pattern instead of all of it:
> 
>   - vc4_atomic_commit_setup() records a pending commit per channel in
>     the private state and the next commit waits on it with
>     drm_crtc_commit_wait(). That is the ordering v2 has no equivalent
>     of.
> 
>   - vc4_atomic_commit_tail() holds max(old, new) for the length of the
>     commit and only drops to the new rate after
>     drm_atomic_helper_wait_for_flip_done(). That is exactly the window
>     the second finding describes.
> 
> Hence the question. Doing the same in rockchip means adding both
> .atomic_commit_setup and .atomic_commit_tail to
> rockchip_mode_config_helpers in rockchip_drm_fb.c, which today carries
> only .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm and is
> shared by every SoC this driver supports, VOP as well as VOP2. The
> commit tail would be a thin wrapper around the rpm helper with the
> clock work on either side of it, and both hooks would do nothing on
> anything that is not RK3588.
>

As you said, placing it in atomic_commit_tail is precisely because
atomic_flush is insufficient to handle multiple CRTC cases. 
So it's okay for me. Let's see if others have any comments. 

> Is that acceptable, or would you rather this stayed inside vop2 in some
> other shape? I am happy to write it either way, but I would rather find
> that out before than after.
> 
> The Medium finding needs no discussion: if
> drm_atomic_private_obj_init() fails, the jump to err_crtcs does not
> undo rockchip_rgb_init(). It is also new in this patch, since before it
> nothing after rockchip_rgb_init() could fail, so it is mine and it will
> be fixed in the next version whatever shape the rest takes.
> 
> Igor
> 
> 

-- 
Best, 
Chaoyi