drm/rockchip: vop2: Scale the AXI clock to the bandwidth the mode needs

VOP2 fetches the scanout data for all of its video ports over a single AXI clock. On RK3588 that clock is pinned to 500 MHz by the device tree and nothing ever raises it, so a mode whose pixel rate outruns what the AXI clock can deliver underruns the internal scanout FIFO. The hardware reports this as POST_BUF_EMPTY and the picture is corrupted.

3840x2160@120 over DisplayPort is such a mode. Measured on an Orange Pi 5 Plus by moving the rate at runtime while the mode was up, with dclk unchanged at 594 MHz throughout, so that the AXI rate was the only variable:

500 MHz: ~594000 suppressed vop2_isr callbacks per 5 s, corrupted 750 MHz: no POST_BUF_EMPTY at all for the 42 s the phase lasted, clean 500 MHz: ~607000 suppressed callbacks per 5 s, corrupted again

Both transitions are immediate. Heiko Stuebner reports the same starvation on different hardware [1].

The requirement follows each port's pixel rate rather than its interface clock, and it is per port rather than aggregate. Two measurements at 500 MHz pin that down:

- a single port scanning out 3840x2160@120 underruns, while the same

    composed pixel rate spread over three ports - 3840x2160@60 on one
    and 3840x2160@30 on two others - is clean for a minute with no
    underrun on any of them.  The totals are equal to the pixel:
    3840*2160*120 == 3840*2160*(60+30+30).

- 3840x2160@60 is clean where 3840x2160@120 is not, although both run dclk at 594 MHz on this board: the 120 Hz link is YCbCr 4:2:0, which halves dclk without halving the rate at which the port consumes pixels.

So the condition belongs on each video port's own crtc_clock. Summing across ports would be wrong, and keying on dclk would miss 4:2:0 entirely. The threshold sits between the measured points: 3840x2160@60 (594000 kHz) and 2560x1440@144 (about 586000 kHz) are both clean at the default rate, 3840x2160@120 (1188000 kHz) is not.

Track the requirement as a global atomic state object: a CRTC that is being modeset records what its port needs during its own atomic check, and the rate applied is the maximum over the ports, never below the rate the platform set up. Because the clock is shared, it is programmed from the commit tail rather than from the per-CRTC enable and disable
hooks: it is held at the rate both the old and the new configuration need from before the first CRTC is touched until every CRTC has moved over, and only then settled at what the new configuration needs. Commits that share nothing but this object are ordered through it, the way vc4 orders commits that share its core clock: each modeset commit records itself per port and the next one waits for those before it touches the clock. The settle write goes out before commit_hw_done(), which is all the next commit waits for, so it cannot land after that commit's own. Page flips do not take the object and are not held up by it.

rockchip_drm_private carries a pointer to the hooks, and only VOP2 on RK3588 sets it, so the commit tail is unchanged for every other Rockchip SoC.

Tested on the same board on a drm-misc-next based tree plus the dw-dp and Rockchip USBDP PHY series, which DisplayPort Alt Mode needs in order to come up at all, with three outputs up (DisplayPort and two HDMI):
3840x2160@120 on the DisplayPort output selects 750 MHz, 3840x2160@60 returns the clock to 500 MHz and going back raises it again; disabling both HDMI outputs at once while the DisplayPort one runs 3840x2160@120 keeps 750 MHz throughout and re-enabling them keeps it; the same sequence at 3840x2160@60 stays at 500 MHz; no POST_BUF_EMPTY in any of it.

---
v3:
- Program the clock from the commit tail instead of the per-CRTC enable and disable hooks, and order commits that share only the private object through it - the full vc4 pattern rather than half of it. The Sashiko review of v2 [2] found that without an atomic_commit_setup hook two non-blocking commits sharing only the private object can apply their rates out of order, and that a commit disabling several CRTCs lowered the clock from the first atomic_disable while the other ports were still scanning out. Chaoyi Chen was fine with placing it in atomic_commit_tail [3]. The settle write is issued before drm_atomic_helper_commit_hw_done(), since the next commit only waits for that.
- Only modeset commits take the private object, so a page flip on one port no longer waits for the previous commit on every other port.
- Free the RGB encoder when drm_atomic_private_obj_init() fails [2].
- Initialise state->obj in create_state as every other private object does; v2 left it NULL and the debugfs state dump dereferences it.
- Report a failed clk_set_rate() instead of ignoring it.
- Retested with three outputs, including disabling two of them at once; see the commit message. The out-of-order case is not one I can provoke from userspace: it is covered by the ordering, not by a measurement.
- Boardcon reports tearing during multi-CRTC reconfiguration in production multi-display use on their RK3588 board (dual HDMI plus DP, three screens to one), a sustained 3-5 C junction increase at 750 MHz on a fanless RK3588J, and offered to test [4]; Cc'd.
- Mainline has no other code touching this clock. A tree that carries the downstream "Scale ACLK rate up for RK3588 FRL display modes" change should not carry both: this patch keys on the pixel rate, which is what the FIFO sees, and the two must not fight over the clock.
- Rebased on drm-misc-next 0878e6053d01d.

v2: https://lore.kernel.org/all/[email protected]/
- Each CRTC records its own requirement in a global atomic state object instead of the rate being computed by walking the CRTC list, which read state the commit holds no lock for (Sashiko review of v1).

[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
[4] https://lore.kernel.org/all/[email protected]/

 drivers/gpu/drm/rockchip/rockchip_drm_drv.h  |  24 ++
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c   |  54 ++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 227 +++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  25 ++
 4 files changed, 329 insertions(+), 1 deletion(-)

index 4705dc6b8bd7d..a7e20a076145f 100644
@@ -61,18 +61,42 @@ struct rockchip_crtc_state {
 #define to_rockchip_crtc_state(s) \
 		container_of(s, struct rockchip_crtc_state, base)
 
+/*
+ * A VOP applies its hardware state one CRTC at a time.  A VOP with a resource
+ * that all of its CRTCs share, such as the AXI clock on RK3588, needs to see
+ * the commit as a whole in order to program it: commits that only share that
+ * resource have to be ordered, and the resource has to be held at the level
+ * both the old and the new configuration need until every CRTC has moved
+ * over.  The VOP points rockchip_drm_private.commit_hooks at its hooks at
+ * bind time, and the atomic commit helpers call them.  All three are
+ * required.
+ */
+struct rockchip_drm_commit_hooks {
+	/* From drm_mode_config_helper_funcs.atomic_commit_setup. */
+	int (*setup)(struct rockchip_drm_commit_hooks *hooks,
+		     struct drm_atomic_commit *state);
+	/* Start of the commit tail, before any CRTC is touched. */
+	void (*tail_begin)(struct rockchip_drm_commit_hooks *hooks,
+			   struct drm_atomic_commit *state);
+	/* After every CRTC has moved over, before commit_hw_done(). */
+	void (*tail_end)(struct rockchip_drm_commit_hooks *hooks,
+			 struct drm_atomic_commit *state);
+};
+
 /*
  * Rockchip drm private structure.
  *
  * @crtc: array of enabled CRTCs, used to map from "pipe" to drm_crtc.
  * @num_pipe: number of pipes for this device.
  * @mm_lock: protect drm_mm on multi-threads.
+ * @commit_hooks: set by a VOP that has to see whole commits, or NULL.
  */
 struct rockchip_drm_private {
 	struct iommu_domain *domain;
 	struct device *iommu_dev;
 	struct mutex mm_lock;
 	struct drm_mm mm;
+	struct rockchip_drm_commit_hooks *commit_hooks;
 };
 
 struct rockchip_encoder {

index ffcb39c130aa1..cfca58d6c7403 100644
@@ -8,6 +8,7 @@
 #define _ROCKCHIP_DRM_VOP2_H
 
 #include <linux/regmap.h>
+#include <drm/drm_atomic.h>
 #include <drm/drm_modes.h>
 #include <dt-bindings/soc/rockchip,vop2.h>
 #include "rockchip_drm_drv.h"

@@ -286,6 +287,26 @@ struct vop2_data {
 	unsigned int soc_id;
 };
 
+/*
+ * The AXI clock is shared by every video port, so the rate it has to run at is
+ * a property of the device rather than of one CRTC.  Track it as a global
+ * atomic state object: each CRTC records its own requirement during atomic
+ * check, and the rate applied is the maximum over the ports.  Going through
+ * the atomic state is what makes this safe - a commit never reads the state of
+ * a CRTC it does not hold a lock for.
+ */
+struct vop2_aclk_state {
+	struct drm_private_state base;
+	unsigned long vp_rate[ROCKCHIP_MAX_CRTC];
+	/*
+	 * The commit that last touched each video port.  The next commit to
+	 * that port waits for it before it changes the clock.
+	 */
+	struct drm_crtc_commit *pending_commit[ROCKCHIP_MAX_CRTC];
+};
+
+#define to_vop2_aclk_state(x) container_of(x, struct vop2_aclk_state, base)
+
 struct vop2 {
 	u32 version;
 	struct device *dev;

@@ -326,6 +347,10 @@ struct vop2 {
 	unsigned int enable_count;
 	struct clk *hclk;
 	struct clk *aclk;
+	/* AXI clock rate set up by the platform, used as the lower bound. */
+	unsigned long aclk_rate_normal;
+	struct drm_private_obj aclk_obj;
+	struct rockchip_drm_commit_hooks commit_hooks;
 	struct clk *pclk;
 	struct clk *pll_hdmiphy0;
 	struct clk *pll_hdmiphy1;

base-commit: 0878e6053d01d87c75bb1d00b0187e8a2e105ead

2.43.0

Both findings rest on that premise, and it does not hold. The commit tail runs in process context: drm_atomic_helper_commit() calls commit_tail() on the caller's stack for a blocking commit and queues it on a workqueue for a nonblocking one. commit_tail() then calls drm_atomic_helper_wait_for_fences() and drm_atomic_helper_wait_for_dependencies() - three loops of drm_crtc_commit_wait(), the very function flagged here - immediately before the ->atomic_commit_tail hook. The default hooks block in drm_atomic_helper_wait_for_vblanks(), and the CRTC atomic_enable and atomic_disable helpers run inside them.

The wait itself is what the core asks for. From the drm_private_obj documentation in drm_atomic.h: "Drivers should store (and get a reference to) the &drm_crtc_commit structure in our private state in &drm_mode_config_helper_funcs.atomic_commit_setup, and then wait for that commit to complete as the first step of &drm_mode_config_helper_funcs.atomic_commit_tail, similar to drm_atomic_helper_wait_for_dependencies()." vc4_atomic_commit_tail() does exactly that, followed by clk_set_min_rate(), which takes the same clk prepare_lock as clk_set_rate(). The wait is on the previous commits' drm_crtc_commit, never on the current one, so it cannot wait on itself.

The pre-existing note answers itself: vop2_crtc_atomic_disable() is called from that same commit tail, and the @atomic_commit_tail documentation says "When disabling a CRTC this hook _must_ stall for the commit to complete." It also calls clk_set_parent() and clk_disable_unprepare() there, unchanged.

I suspect the two senses of "atomic" got conflated: an atomic commit is an all-or-nothing modeset update, not in_atomic() context. There is no might_sleep() or context restriction on this path.

Thanks,
Igor