[DjDeveloperr]

Summary

Every T8140 CPU frequency request spun until the cluster reported the new state, about 50 µs per transition. Because of that verification, the T8140 also never used schedutil's fast-switch path. Every change therefore went through the governor kthread, which runs at deadline priority and busy-waited on whichever CPU it landed on.

Under light GPU work the P-cluster changes state roughly a thousand times a second. Each GPU completion is handled by the mailbox interrupt thread, a real-time task that raises the cluster to its top state, and the cluster drops back once the thread sleeps. The spin cost about 7% of a P-core, and it landed between a completion and the next submission.

This change checks each request when the next one is issued. That request still waits for BUSY to clear and still stops all requests if the accepted state differs from the last one written, so an unexpected owner is still caught, one request later. With the wait gone, T8140 enables fast switch and the governor thread disappears. Requests through ->target_index, from suspend and the static governors, still wait for their own transition. The fast-switch callback resolves its table index from the frequency it is given rather than reading policy->cached_resolved_idx, which a concurrent limits update can rewrite.

Measurements

Neo (A18 Pro); same boot, alternating with the previous module; gpubench, µs per submission:

row	before	after
compute, depth 2	58.8–59.7	46.4–47.3
compute, depth 1	107.8–108.4	102.6–102.7
render, depth 2	47.5–47.8	45.8–45.9
render depth 32 / compute depth 64	unchanged	unchanged

Checks on Neo

• Governor thread. It is gone with this change (sugov count 0). schedutil runs fast switch on both policies.
• Errors. No DVFS busy or completion errors were logged through about 50k transitions in the latency runs, nor during all-core load.
• Thermal behaviour is unchanged. Under 20 s of load on all six cores, alternating with the stock module, both step the P-cluster down through 2196/1620/1260/744 MHz at the same temperatures (77–106 °C). The fast die-temperature limit still applies through its QoS request.

Scope

T8140 only, behind a new lazy_verify flag. T8142 and T8152 keep blocking verification until they are measured.

Base

The same change on 7.2.11; it applies unchanged. The measurements above were taken on Neo with the 7.3 kernel. This branch builds cleanly on 7.2.11 with the Neo config; a Neo boot test of the 7.2.11 build follows.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable new issues were found.

Summary

T8140 now checks the previous frequency request before issuing another, rather than waiting after each write.

• T8140 checks frequency changes on the next request.

No actionable new issues were found.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[T8140 frequency request] --> B[Wait for BUSY to clear]
  B --> C{Accepted state matches previous request?}
  C -- No --> D[Stop requests and return an error]
  C -- Yes --> E{Already at requested state?}
  E -- Yes --> F[Return success]
  E -- No --> G[Write new request]
  G --> H[Save expected state]
  H --> F

Loading

_{Reviews (2) · Last reviewed commit: "cpufreq: apple: verify T8140 transitions..." · Reviewed by Greptile}

[maralcbr]

Thanks @DjDeveloperr, this is a nice win. Keeping the check in front of every write means an unexpected owner still never gets a second request, and the gpubench numbers and the sugov count are convincing. One fix before merge:

• The fast-switch callback reads the cached index twice. apple_soc_cpufreq_fast_switch() (drivers/cpufreq/apple-soc-cpufreq.c:431 and :434) programs policy->cached_resolved_idx and then reads it again for the return value. cpufreq_set_policy() rewrites that cache when it resolves new limits (drivers/cpufreq/cpufreq.c:487, :2664-2672), and nothing serialises that against the scheduler path. If a QoS update lands in between, the cluster runs at one state while policy->cur and the frequency scale record another. Schedutil skips the callback while its own target is unchanged (kernel/sched/cpufreq_schedutil.c:125-139), so that can last until the next real change. The same helper already runs on the other SoCs, but this PR is what moves T8140 off the slow path, which programs and reports one captured index, and T8140 is the SoC whose fast die-temperature limit moves the QoS most often. Please look the index up locally: int idx = cpufreq_table_find_index_l(policy, target_freq, false);, then pass idx to apple_soc_cpufreq_set_target() and return policy->freq_table[idx].frequency. target_freq is already clamped by cpufreq_driver_fast_switch() and the table is sorted. Reading the cache once would only fix the mismatch, not a pick made from someone else's resolution.

Smaller things:

• apple-soc-cpufreq.c:243-244 still says every transition is verified. The last request is now only checked when the next one is issued, and under a static governor or before the policy exits there may be no next one. Suggest: "each request is verified when the next one is issued".
• With fast switch, a lower fast die-temperature limit takes effect on the cluster's next scheduler update rather than straight away from sugov_limits(). That's fine on a busy cluster, but a CPU under nohz_full= or isolcpus=nohz can go a long time without one. Worth a sentence near :110-112, where the sampling side already handles nohz_full.
• Optional: keep the blocking wait on the ->target_index path and defer only in ->fast_switch. It costs nothing there, and requests from suspend and the static governors would be checked when they're made.

CI was still running when I looked: compile, KUnit, package and both host-test jobs were pending, so I'll check ci / result again with the fix. For release (not the source merge): the Neo boot of the 7.2.11 build you mentioned, on the final head, with the sugov count and no DVFS errors.

[maralcbr]

nit: "every transition is verified" is no longer quite true for the last request. Suggest: "each request is verified when the next one is issued".

[DjDeveloperr]

Fixed in 54b6358: the comment now says requests from the fast-switch path are verified when the next one is issued, and requests through ->target_index are still waited for.

[DjDeveloperr]

Thanks, all four are in 54b6358:

• Fast-switch index. apple_soc_cpufreq_fast_switch() now resolves its own index with cpufreq_table_find_index_l(policy, target_freq, false), then programs and reports that same entry. It no longer reads policy->cached_resolved_idx.
• Wait on ->target_index. I took the optional suggestion. The body is now apple_soc_cpufreq_request(policy, index, wait): ->target_index always waits for its own transition (suspend and the static governors), and only ->fast_switch defers the check on T8140.
• T8140 comment. It now reads "each request is verified", and explains which path defers.
• nohz_full note. Added next to the nohz_full sampling text: with fast switch, a lower limit reaches the cluster at its next scheduler frequency update, which a nohz_full CPU with its tick stopped may not have for a long time.

It builds cleanly on 7.2.11 with the Neo config. I'll post the Neo boot of the 7.2.11 build on the final head with the sugov count and the DVFS error check.

[chriskearney]

Checked 54b635838d4c32c4b52fb822b283d19dec8fca39: Marcelo's cached-index concern is resolved. Fast switch now resolves one local index from target_freq, programs it and returns that same entry's frequency. The ordinary target path retains blocking completion verification; only T8140 fast switch defers it.

This revision also changes the shared fast-switch helper used on M1/M2/M3, so those machines provide relevant regression coverage even though they cannot validate Neo's lazy-verification gains.

For Neo qualification, keep the final 7.2.11 boot and a suspend/resume cycle with no DVFS errors. The documented QoS-limit delay until the next scheduler update, particularly with nohz_full, remains a qualification limit rather than something a clean M3 run can settle.

Validation here: source review of the request/fast-switch paths and schedutil limits handling. No independent build or physical boot of this head yet; its new CI run is still pending.

[maralcbr]

Reviewed at 0c4dceb (source only; no build or boot on my side).

The motivation is clear and the numbers are good, but two things need changing before this can go in.

1. Fast-switch now spins in scheduler context. With fast_switch_possible set, apple_soc_cpufreq_set_target() runs from schedutil with the runqueue lock held and IRQs off. Its first step is still the readq_poll_timeout_atomic() waiting for BUSY to clear, with T8140's 400 µs timeout. Whenever a request arrives while the previous ~50 µs transition is still in progress (likely at ~1000 transitions/s), that wait moves from the sugov kthread into the scheduler hot path.

Simply returning early when BUSY is set doesn't work: schedutil stores next_freq before calling ->fast_switch(), ignores its return value, and suppresses an identical next target, so a skipped request would never be retried. Any skip needs explicit retry handling, or the overlap has to be bounded some other way.

2. Lazy verification also covers the slow and suspend paths. lazy_verify is checked inside the shared set_target(), so ->target_index() loses synchronous verification too. cpufreq_generic_suspend() stops the governor and then requests suspend_freq; nothing may follow to verify that request was accepted. Please keep synchronous verification on the non-fast-switch path (including suspend), and make only the fast-switch path lazy.

Also: the measurements were taken on 7.3, and the 7.2.11 Neo boot is still to come. Please post it with the result of a combined run with #226 and #228 on the same base, since all three affect the same utilisation and frequency loop.

[DjDeveloperr]

Thanks both. This review was at 0c4dceb.

• Lazy verification on the slow and suspend paths. Already fixed at 54b6358. ->target_index always waits for its own transition, so cpufreq_generic_suspend()'s request is verified synchronously. Only ->fast_switch defers.

• Spinning in scheduler context. Agreed: the initial BUSY wait still runs from schedutil with the runqueue lock held. A related gap, raised on the twin PR elsewhere: a final deferred request with no successor is never verified. I'm reworking the T8140 fast path so it never spins and never leaves a request unchecked:

  • A request that arrives while BUSY is set is stored as pending, so a suppressed identical target can't lose it.
  • A per-policy hard hrtimer completes it once BUSY clears.
  • Every deferred write, including the last one, is verified by that timer within the existing transition timeout, and a mismatch or stuck BUSY latches the failure as today.
  • ->target_index serialises with the timer and keeps full blocking verification.

  It will come with Neo runs: the latency rows, transition rate with zero DVFS errors, an injected refused or stuck transition proving the latch, thermal transitions, and suspend/resume.

• Qualification. The 7.2.11 Neo boot will be one combined run with soc: apple: clpc: raise the T8140 fabric floor for GPU work #226 and irqchip/apple-aic: Report the effective affinity of AICv2+ IRQs #228, including a suspend/resume cycle with no DVFS errors, as you both asked. The shared fast-switch helper change also reaches M1/M2/M3. I'll note which of those I can cover.

[DjDeveloperr]

The fast-switch rework is in 7298b9b ("verify T8140 fast-switch requests from a timer"):

• No spinning in scheduler context. A request that finds the cluster busy is kept as pending. It is neither spun on nor dropped, so schedutil's suppressed identical target can't lose it. A per-policy hard hrtimer writes it once the transition finishes.
• Every request is verified, including the last of a burst. The timer checks the last written request within 1 ms (0.5 ms slack, so it can share an interrupt). A busy timeout or a wrong accepted state latches transition_failed as before, and is reported from the timer, not the scheduler.
• ->target_index cancels the timer, drops any pending request and keeps blocking verification (suspend, static governors). ->exit cancels the timer. Only T8140 (lazy_verify) uses any of this.

On Neo, against the previous head, same boot, alternating, 3 rounds:

• compute d1 102.67 → 102.38 µs and d2 46.70 → 46.61; render d2 45.81 → 45.76; 5–6% more timer interrupts, and transition counts identical.
• About 570 transitions/s under mixed load with 0 DVFS errors.
• Static governors and a pm_test=devices suspend/resume ran clean, and all-core load throttles to the same frequencies.
• A diag-only build (not in the change) showed the timer alone catching a refused write with no later request, and a stuck BUSY on both policies.

The combined 7.2.11 run with #226 and #228 is next.

[maralcbr]

Follow-up at 7298b9b (source only).

First, a correction: my previous review was posted against 0c4dceb after you had already pushed 54b6358, so the slow and suspend path point was already fixed when I posted it. Sorry for the noise.

The timer rework fixes the scheduler-context spin well. Keeping the latest request as pending means a suppressed identical target can't be lost, every write gets a verifier, including the last of a burst, and I found no lock-order problem (rq lock / sugov update_lock → priv->lock → hrtimer base). Re-arming the timer from its own callback is fine, and so is HARD mode on PREEMPT_RT.

One remaining issue: generic suspend can skip the driver. fast_switch() returns the requested frequency even when the request is only pending, and the core stores it in policy->cur (cpufreq.c:2209). If that frequency equals suspend_freq, cpufreq_generic_suspend() → __cpufreq_driver_target() takes the target_freq == policy->cur shortcut (cpufreq.c:2370) and never reaches set_target(). So the cancel, drain and synchronous verification are bypassed. Two ways to get there:

1. A transition is BUSY, schedutil asks for the lowest frequency (stored as pending), and suspend arrives before the timer writes it.
2. Without any tight timing: the timer finds the previous transition failed and drops the pending low request (apple_soc_cpufreq_fail() clears pending_pstate). Hardware never gets it, but policy->cur is already low, so a later suspend skips the driver and its failure check.

CPUFREQ_NEED_UPDATE_LIMITS alone wouldn't cover it, because __target_index() has its own equality shortcut. I'd suggest a driver ->suspend that always cancels the timer, drains any pending or in-flight request, verifies it synchronously, and reports a latched failure, then calls the generic suspend. Please exercise "pending at suspend" and "failed pending at suspend" (your diag build looks like the right tool).

Smaller points, not blocking:

• On lazy SoCs set_target() holds the raw priv->lock with IRQs off across both BUSY polls, up to about 800 µs of polling budget. After hrtimer_cancel(), the governor contract already keeps fast switch out of ->target_index, so the lock (and the "late fast-switch request" comment) could go, or be dropped around the polls.
• hrtimer_cancel() in set_target() can cancel a queued failure report, leaving failure_unreported set and never printed. Report it while draining.
• With BUSY and a pending request, the timer re-arms every 4 µs until the 400 µs deadline: roughly 90 hard-IRQ callbacks for a stuck transition. Fine as a failure-path cost; just noting it.
• ->exit cancels the timer without draining, so "every write is verified" doesn't strictly hold across teardown. Worth a word in the comment.

Keeping changes requested for the suspend path only.

[DjDeveloperr]

Thanks, and good catch on the policy->cur == suspend_freq shortcut. Both paths are real: a pending low request at suspend, and a failed pending request that the timer dropped. I'm adding a driver ->suspend that cancels the timer, drains any pending or in-flight request, verifies it synchronously and reports a latched failure before the generic suspend. It'll be exercised with the diag build for "pending at suspend" and "failed pending at suspend". I'll also take the smaller points:

• drop the lock around the set_target() polls;
• report a failure whose timer was cancelled;
• document or drain ->exit.

[DjDeveloperr]

Suspend path fixed in 979d4f1 ("cpufreq: apple: settle T8140 lazy requests before suspend").

• policy->cur == suspend_freq shortcut. The driver now has its own ->suspend. On lazy_verify SoCs it cancels the verify timer, takes the pending request (or the last expected state), writes it and waits for it, verifying both it and the transition before it. Only then does it call cpufreq_generic_suspend(). The cluster then matches policy->cur, so the core's shortcut is correct again. A latched failure makes the driver suspend return -EIO, which the core logs.
• Raw lock across the ->target_index polls. Removed. Waited-for requests now cancel the timer and drain first. The comment explains that the core stops a fast-switching governor before any waited-for request, so the two never run together.
• Failure cancelled by hrtimer_cancel(). The drain step prints a report the timer had not yet made.
• ->exit. Uses the same drain step, and the comment says so.

Tested on T8140:

• A pm_test=devices suspend cycle produced no DVFS or driver suspend errors, and transitions continued afterwards.
• With a diagnostic build (not part of the change), a request left pending for the lowest frequency, equal to both policy->cur and suspend_freq, was written and verified at suspend. Before the fix, the core skipped the driver and left the cluster at the older state.
• With the same diagnostic build, a stuck BUSY that dropped the pending request, with the timer's report held back, was reported at suspend and failed the driver suspend. The system still suspended and resumed.
• Light-submit cost is unchanged: compute d1 102.25 → 102.12 µs, compute d2 36.93 → 36.47 µs, render d2 48.24 → 48.45 µs.

The combined 7.2.11 run with this head is in progress, and I'll post its results on #226, #227 and #228.

[DjDeveloperr]

Results from the combined 7.2.11 Neo run with this head (979d4f1), together with #226 and #228:

• Suspend/resume: two pm_test (freeze) cycles, one at devices and one at platform, which adds the late and noirq phases. Real s2idle does not resume on Neo yet. Both cycles entered and exited cleanly with no DVFS lines and no driver suspend errors. Wi-Fi re-associated and transferred data after each.
• Whole run: no DVFS lines in the kernel log at boot or at the end. The run covered idle, 1080p video, light browsing, a single-thread CPU leg in the top quartile and 90 s of sustained GPU load. There are no sugov kthreads, as expected with fast switching.

[maralcbr]

Approving at 979d4f1.

The driver ->suspend closes the gap: it runs after the governor has stopped, settles any pending request so the cluster matches policy->cur before the core's equal-frequency shortcut, and reports a withheld failure. The other points are addressed too: no IRQs-off polling on the slow path, the lost failure report is printed, and ->exit drains. The combined run is clean: pm_test freeze at devices and platform, and no DVFS lines across the whole run.

One check is deferred, not waived. Real s2idle doesn't resume on Neo yet, and the driver keeps expected_pstate across suspend without reconciling it on resume. That assumption predates this PR, but if firmware changes DVFS_CMD during a real s2idle, the first request after resume will latch transition_failed for good. Once real s2idle resumes on Neo, please check that both policies still change frequency under load afterwards (frequencies actually moving, not just a clean log) and post the result here. I'm tracking it on our side.

[DjDeveloperr]

Thanks. Agreed on the s2idle check: once real s2idle resumes on Neo, I'll put both policies under load afterwards, confirm that their frequencies actually move, and post the result here.

One more commit is coming to this branch. A review on the aurora-silicon twin found a gap: with fast switching, a lowered fast die-temperature limit reaches the cluster only at schedutil's next frequency update. A nohz_full CPU with its tick stopped may not have one for a long time. Before lazy verification, ->target_index applied the limit at once, so the gap comes from this PR.

The fix is "cpufreq: apple: apply a lowered T8140 fast die limit at once". It builds with W=1 and is checkpatch-clean on 7.2.11. It's being qualified on Neo now:

• forced-limit delivery on a normal boot and on a nohz_full boot;
• a limit that drops and recovers quickly;
• thermal load;
• static governors;
• teardown;
• light rows.

I'll push it with those results, so the branch will need another look.

[DjDeveloperr]

Pushed 29ce152, "cpufreq: apple: apply a lowered T8140 fast die limit at once", on top of the commit you approved.

The commit fixes a gap a review on the aurora-silicon twin found. With fast switching, a lowered fast die-temperature limit reached the cluster only at schedutil's next frequency update, which a nohz_full CPU with its tick stopped may not have for a long time. Before lazy verification, ->target_index applied the limit at once. Now the driver applies the limit inside the sample that sets it, and clamps later fast-switch requests to it. If the limit recovers before the QoS request changes, the driver moves the cluster back to the scheduler's last request.

Measured on Neo:

• Forced limit, spinner on a P-core: written inside the sample. Before, it took about 1 ms on a normal boot and 0.02 to 2.5 ms with nohz_full on that CPU.
• Drop and restore within one sample: returns to the scheduler's request.
• Real thermal load: frequency at or below the limit in every sample.
• Other paths: static governor switches, unbind and rebind, and pm_test=devices are clean.
• Light GPU rows: unchanged.

The full table is on aurora-silicon/linux#443. Only lazy_verify SoCs run the new code.

[maralcbr]

Reviewed at 29ce152 (source only). This replaces my approval of 979d4f1.

Applying a lowered limit inside the sample is the right fix for the nohz_full gap, and most of it checks out: the fd->lock → priv->lock → hrtimer base nesting is valid on PREEMPT_RT, sampling runs from softirq, setting waited before hrtimer_cancel() stops a sample re-arming the timer during the drain, init sets up the lock and cap before apple_fast_die_init(), and T8140 opp-levels rise with frequency, so min() over P-states is sound.

One hole: lazy_cap() moves the cluster without updating policy->cur, which reopens the suspend shortcut.

1. schedutil asks for H while the cap is L, and L equals suspend_freq. lazy_request() clamps it and returns L, so policy->cur = L and asked_pstate = H.
2. The cap rises before the QoS work has applied the drop. lazy_cap() restores min(asked, cap) = H in hardware, but policy->cur stays L, and schedutil, which already holds next_freq = H, won't ask again.
3. At suspend, apple_soc_cpufreq_suspend() drains and verifies H, then cpufreq_generic_suspend() sees suspend_freq == policy->cur and returns without calling the driver. The cluster suspends at H.

Two simple ways out: have the lazy suspend path write and verify the suspend P-state itself (via apple_soc_cpufreq_set_pstate()) instead of relying on cpufreq_generic_suspend(), or set policy->cur to the settled frequency before calling it. It would also help to keep policy->cur honest when lazy_cap() moves the cluster, since scaling_cur_freq and stats read it. Please exercise this sequence with your diag build (low cap equal to suspend_freq, cap rises, then suspend).

Everything else from the previous rounds still stands.

[DjDeveloperr]

Thanks. The fix for the policy->cur hole you described at 29ce152 is written ("keep T8140 policy->cur in step with the cluster"):

• The suspend step records the state it settled the cluster in as policy->cur before cpufreq_generic_suspend(), so the suspend frequency is written whenever the cluster is elsewhere.
• When the fast die limit moves the cluster, policy->cur follows, for scaling_cur_freq and the statistics.

It's being exercised on Neo now with the diag build, before and after the fix: limit at suspend_freq with a spinner, then the limit rises, then a pm_test=devices suspend. I'll push it as one more commit on this branch once that passes, so please hold the merge until then.

[DjDeveloperr]

This merged before the policy->cur fix landed, so I'll open it as a follow-up PR on integration/aurora ("cpufreq: apple: keep T8140 policy->cur in step with the cluster") once the Neo check of the sequence you described passes. The same follow-up will go to aurora-silicon, where AsahiLinux#443 carries the same code.