drm/vc4: kms: Reduce firmware clock requests during atomic commits - #7498
drm/vc4: kms: Reduce firmware clock requests during atomic commits#7498popcornmix wants to merge 1 commit into
Conversation
On Raspberry Pi the core clock is managed by the firmware, so every
clk_set_min_rate() call on it results in at least one blocking
mailbox round-trip to the VPU: the clock is registered with
CLK_GET_RATE_NOCACHE, so clk_set_rate_range() always queries the
current rate via GET_CLOCK_RATE, and issues a SET_CLOCK_RATE on top
if the aggregated rate changed. All of this happens under the global
clk prepare_lock.
vc4_atomic_commit_tail() currently requests a boost of the core clock
to at least 500MHz at the start of *every* commit and settles back to
the load-derived rate at the end, even though the boost is only needed
to avoid stalling the pipeline during a full modeset. In addition the
final drm_dbg() evaluated clk_get_rate() unconditionally, hiding
another firmware query. A compositor page-flipping at 60Hz therefore
generated several mailbox transactions per frame, all of them no-ops.
Fix this in two ways:
- Only apply the 500MHz floor when one of the CRTCs in the commit
actually needs a modeset. Plane-only updates still raise the clock
to the maximum of the old and new state requirements before the
HVS is reprogrammed, since the load can change without a modeset.
- Cache the last requested minimum rate in struct vc4_hvs and skip
requests that wouldn't change it. The core and display clocks are
always requested at the same rate, so a single cached value is
enough, and the actual-rate debug query now only happens when the
request really changes.
With this, steady-state page flips no longer generate any firmware
mailbox traffic, while modesets and genuine load changes behave as
before.
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
|
@XECDesign does this help your display latency testing. |
|
The pi no longer reboot reliably, but when it does, the numbers are what you'd expect. I'll do a bit more testing to be 100% sure the patch is the problem, but it may be worth somebody else trying it as well. I'm seeing a boot failure about every 4th time. Black screen, probably not getting past the module load in initramfs. Edit: I don't think the reboot issue is caused by the patch. Still getting the same problem after reverting it. |
|
@XECDesign I assume your reboot issue was unrelated and you are happy with this PR? @6by9 care to review? I think this is important to get in - we are getting a multi-ms delay every submit, which makes it hard to get a low latency update to screen. |
Yup, LGTM. |
|
It feels wrong that the driver needs to cache the last requested rate, but if that is what is necessary then fine. |
|
The alternative would be to cache it in clk-raspberrypi. Would that be an easier sell upstream? I believe that would be safe, as the firmware (apart from first call) has to return the clock the kernel last set. We used to return a "live" clock that may be throttled or boosted outside of the kernel's knowledge but that is unworkable as kernel clock sets always do a "skip if new_clock == read_clock" and if unluckily new_clock matches a temporary clock boost/throttle the firmware never knows the new_clock was needed. Effectively the firmware maintains a very slow cache of the last set frequency by the kernel. |
|
Go with this for now. The upstreaming arguments can happen when we get around to attempting it. |
3 participants
On Raspberry Pi the core clock is managed by the firmware, so every clk_set_min_rate() call on it results in at least one blocking mailbox round-trip to the VPU: the clock is registered with CLK_GET_RATE_NOCACHE, so clk_set_rate_range() always queries the current rate via GET_CLOCK_RATE, and issues a SET_CLOCK_RATE on top if the aggregated rate changed. All of this happens under the global clk prepare_lock.
vc4_atomic_commit_tail() currently requests a boost of the core clock to at least 500MHz at the start of every commit and settles back to the load-derived rate at the end, even though the boost is only needed to avoid stalling the pipeline during a full modeset. In addition the final drm_dbg() evaluated clk_get_rate() unconditionally, hiding another firmware query. A compositor page-flipping at 60Hz therefore generated several mailbox transactions per frame, all of them no-ops.
Fix this in two ways:
Only apply the 500MHz floor when one of the CRTCs in the commit actually needs a modeset. Plane-only updates still raise the clock to the maximum of the old and new state requirements before the HVS is reprogrammed, since the load can change without a modeset.
Cache the last requested minimum rate in struct vc4_hvs and skip requests that wouldn't change it. The core and display clocks are always requested at the same rate, so a single cached value is enough, and the actual-rate debug query now only happens when the request really changes.
With this, steady-state page flips no longer generate any firmware mailbox traffic, while modesets and genuine load changes behave as before.