Hi Igor,
Thanks for the thorough experimental work and the v2 rework. The per-port
pixel rate analysis is well-reasoned -- the YCbCr 4:2:0 distinction (dclk
halved, pixel consumption rate unchanged) is a subtle point that's easy to
miss, and the three-port vs. one-port comparison that pins the condition
on per-port pixel rate rather than aggregate bandwidth is clean.
We work with RK3588 in multi-display industrial configurations (digital
signage, edge AI devices) and have independently hit the POST_BUF_EMPTY
corruption at 500 MHz AXI on high-pixel-rate modes. As Heiko noted back
in the initial VOP2 upstreaming [1], this has been a long-standing
nuisance -- good to see a proper fix in progress.
A few observations:
Threshold gap and memory contention
You noted in the initial discussion [2] that your monitor offers nothing
between 2560x1440@144 (~586 MHz) and 3840x2160@120 (1188 MHz), so the
threshold couldn't be bisected. VOP2_HIGH_BW_PIXCLK_KHZ is set to 1000000
(1 GHz), which sits in that untested gap.
In edge AI products where the NPU (6 TOPS) and VPU are active alongside
the display pipeline, the VOP2 shares memory bandwidth with other AXI
masters. Under combined display + NPU + VPU load, the effective fill rate
of the scanout FIFO may drop below what 500 MHz AXI sustains, even at
pixel rates below the current 1 GHz threshold. The binary 500/750 MHz
switch doesn't account for this.
This isn't necessarily a blocker for the initial patch -- the common case
(display-only, 4K@60 or below) is already handled correctly. But it may
be worth noting as a known limitation, or considering a device-tree
property that lets boards with heavy non-display AXI traffic lower the
trigger point.
Multi-CRTC disable race
We can corroborate the second Sashiko finding from production: when a
multi-screen advertising display reconfigures to single-screen, the
window where the AXI clock drops while other CRTCs are still scanning
out causes brief tearing on the remaining screen. The vc4
atomic_commit_setup / commit_tail approach you described -- holding
max(old, new) until drm_atomic_helper_wait_for_flip_done() -- closes
exactly this window. Endorsed.
Looking at the vc4 implementation, the key piece is that commit_setup
records a pending_commit per channel and subsequent commits wait on it
with drm_crtc_commit_wait() [3], which is what enforces ordering between
non-blocking commits that share only the private object. Without that,
v2's private state is safe within a single commit but can still be
overwritten by a stale snapshot from an earlier non-blocking commit --
exactly as Sashiko described.
Thermal note for fanless designs
On fanless RK3588J industrial enclosures (ambient 60C), sustained
750 MHz AXI raises SoC junction temperature by roughly 3-5C in our
measurements. This is within budget for our products, but the automatic
fallback to 500 MHz when no high-bandwidth port is active -- which the
patch already implements -- is essential for fanless designs. Please
retain that behavior in v3.
Error path and FRL overlap
Agreed on the Medium finding: rockchip_rgb_fini() belongs before the
err_crtcs jump.
On the HDMI FRL overlap you noted in the v1 cover letter -- it would
help to state explicitly in v3 whether this patch subsumes the
FRL-specific ACLK workaround (7e580d1cc3aa on the rockchip-3588 branch)
or coexists with it. Having two independent ACLK scaling mechanisms
could conflict if both are active.
v3 approach
Placing commit_setup/commit_tail in rockchip_mode_config_helpers
(shared by VOP and VOP2) with RK3588-only behavior is acceptable from
our perspective. The shared helpers already carry
drm_atomic_helper_commit_tail_rpm, so a thin RK3588 wrapper is the
lesser evil compared to duplicating the commit tail in the VOP2 driver.
Looking forward to v3.
[1] https://lkml.indiana.edu/2311.1/06312.html
[2] https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg626775.html
[3] https://patchwork.kernel.org/project/dri-devel/patch/20210707084745.1365390-11-maxime@cerno.tech/
Best regards,
Boardcon Embedded Design
https://www.boardcon.com