Le mardi 22 septembre 2026 à 10:01 +0200, Igor Paunovic a écrit : > The rocket driver runs the NPU at whatever rate the devicetree pinned > it to: 200 MHz on the RK3588, out of the 1 GHz the hardware reaches. > This series adds devfreq, the OPP table and the thermal plumbing that > go with it. > > v1: https://lore.kernel.org/r/[email protected] > > As Tomeu asked [1], the slot-search fix now leads the series, with the > two rocket fixes this builds on and, as patch 4, 2/2 of my July > lifecycle series [2], without which v2 oopsed when one core was unbound, > bound and unbound again. KASAN found that v1 4/7 (now 8) kept a clk > handle that unbinding freed; the notes under each --- list the rest.
In future series or update, can you use common formatting in the future, I find your changelog hard to read. Do something like: Since v1: - Changed patch X with action Z - Fix blabla - ... Its actually good to be more concise in your communication, "wall of text" are difficult to cope with for maintainers. cheers, Nicolas > > Hardware constraint > ------------------- > > The three cores share one clock and one supply. The clock comes from a > PVTPLL inside the NPU power islands: for a PVTPLL rate the firmware > programs NPU GRF registers inside the islands, for the 200 MHz boot rate > it only switches CRU selectors to GPLL (clk_npu_set_rate() in > plat/rockchip/rk3588/drivers/scmi/rk3588_clk.c, TF-A v2.12.0, which > Nicolas pointed me to). An island powered up while the clock is above > the boot rate never acknowledges the power-on, and the next register > access into it takes an asynchronous SError. Returning to the boot rate > is safe at any time; raising the clock is not, so while it is above the > boot rate every core is held runtime resumed and no island can > transition. Unbinding any core takes devfreq down until all are bound > again. > > The firmware accepts only the nine rates of its rk3588_npu_pvtpll_table, > 300 to 1000 MHz in 100 MHz steps plus 200 MHz, and refuses any other. > The refusal never reaches the caller: clk_change_rate() ignores what > ->set_rate() returns, so clk_set_rate() reports success and the clock > stays put. Hence patch 7 names exactly those nine. > > The table is on all three cores with opp-shared (Nicolas's review); > the one devfreq device sits on rknn_core_0, the lowest devicetree index. > Patch 9 explains the maximum over the cores and why runtime suspend > does not call devfreq_suspend_device(). > > Testing > ------- > > Orange Pi 5 Plus, 7.3.0-rc2 (drm-misc-next 3afbeb6b99dd) plus this > series, in-tree rocket, BL31 v2.12.0-10-g70d814213: the edk2-rk3588 TF-A > branch with upstream d2d6928641ba, so that it reports the PVTPLL rate it > measures. MobileNetV1 through Teflon, 3 x 20 s per arm, one thread > pinned to one CPU, CPUs on the performance governor, a bit-exact oracle > on every inference, polling_interval at the driver's 50 ms, core 0's > interrupt on a CPU with its deep idle state disabled. > > cur_freq is only the driver's request and scmi_clk_npu on the GPLL path > echoes the last accepted rate, so the CRU selector is the witness > (CLKSEL_CON74 bit 0 = PVTPLL, CLKSEL_CON73 = GPLL divider), read once a > second: > > 200 MHz OPP, userspace 91.15 inf/s CRU GPLL/6 = 198 MHz, 700 mV > 1000 MHz OPP, userspace 234.45 inf/s CRU PVTPLL (BL31 1 GHz), 850 mV > simple_ondemand 232.88 inf/s OPPs 200-1000 MHz > 200 MHz OPP again 90.87 inf/s drift 0.997 > > That is 2.57x pinned and 2.55x under the governor, 0.7 % below pinned; > the oracle and the interrupts per inference were the same in all four > arms. > > Three clients, each with its own oracle, all three cores computing > together, output bit-exact and identical to a single-client control: > > OPP one client three clients total > 900 MHz, 800 mV 234.42 199.33 + 198.95 + 199.19 597.47 inf/s > 1000 MHz, 850 mV 240.49 203.16 + 203.01 + 203.46 609.63 inf/s > > After 25 s of load the governor is at the 200 MHz OPP from the first > sample (0.24 s apart) through the 60 s idle window: CRU 198 MHz in all > 60 one-second samples, rail 700 mV. The islands were power-cycled five > times at 700 mV and powered down once more after a raise to 1000 MHz. > All six bind orders give bit-exact output, with devfreq and cooling back > on rknn_core_0 after every rebind. > > Unbind and reload: 25 rounds of all three cores, 4 of a single core with > an inference while it was gone, 5 rmmod rounds, and 3 unbinds plus 1 > rmmod with the clock raised (CRU on the PVTPLL before each). Without > patch 4 the single-core round oopses, as its commit message shows. > > On a KASAN, kmemleak and PROVE_LOCKING build of the same tree the unbind > (no client submitting), reload, three-client and island tests gave no > report and kmemleak nothing from this driver, and a lockdep run through > the governor, cooling, three clients, and unbind and rmmod with the > clock raised to the 1 GHz OPP left lockdep enabled. > > Build: the series was rebased onto drm-misc-next 8ef59ee79407 to send > it; apart from one comment corrected in 9 (rocket_devfreq.o is > byte-identical), the patches are the ones tested above. On that base, > arm64, gcc 13.3: each patch that touches drivers/accel/rocket builds > with W=1 and no warning, and sparse reports nothing in > drivers/accel/rocket; 9, 10 and 11 also build with W=1 and no warning > with CONFIG_DEVFREQ_THERMAL=n. dt_binding_check at 6 and 11, and > CHECK_DTBS on rk3588-orangepi-5-plus, rk3588s-orangepi-5 and > rk3588-edgeble-neu6b-io at the base, 7 and 11, give no schema warning. > A full Image, modules and dtbs build at 11 completes with no warning > from drivers/accel/rocket. checkpatch --strict flags only the 75-column > oops line in 4 and MAINTAINERS for the new files in 9, which > F: drivers/accel/rocket/ covers. > > Two corrections to my reply of 9 September [3]: the supply could have > come down to the 200 MHz voltage even had the clock change failed, since > the refusal is not reported, so the CRU selector is the witness now; and > the v1 governor arm was the governor running, not limits pinned by hand. > > Not done > -------- > > - Unbinding a core while clients submit is still unsafe. Patch 3 notes > that an open file keeps an entity on the departed core's scheduler; > on the KASAN build (three clients running, then as root > "echo fdac0000.npu > /sys/bus/platform/drivers/rocket/unbind") that > is a use-after-free: > > BUG: KASAN: slab-use-after-free in _raw_spin_lock+0xb4/0x118 > drm_sched_rq_remove_entity+0x94/0x2b0 [gpu_sched] > drm_sched_entity_select_rq+0x1f4/0x698 [gpu_sched] > drm_sched_job_arm+0x50/0x258 [gpu_sched] > rocket_job_push+0x2f8/0x6b8 [rocket] > rocket_ioctl_submit+0x888/0xd28 [rocket] > Freed by: drm_sched_fini <- rocket_job_fini <- rocket_core_fini <- > rocket_remove > > then a soft lockup: drm_sched_fini() frees the run queues while the > entity still points at one. drm-misc-next has the same path (read, > not run): rocket_job_open() builds the entity over every core, and > rocket_remove() ignores open files. The fixes I see are policy, > suppress_bind_attrs (undoing patch 4) or unplugging the DRM device > when any core goes. Tomeu, which would you prefer? I will write it. > - Probe and remove are not serialised against open, and a file opened > while a core probes can see its scheduler uninitialised; both predate > this series. > - Lifecycle 1/2 [2] (a devres leak that moves the accel minor up after > every full unbind and rebind, invisible to kmemleak) builds on > Guangshuo Li's "accel/rocket: clear rdev on device init failure" [4], > which is not in drm-misc-next. Sashiko's other pre-existing findings > on the standalone slot-search patch, which I agreed with [5], stand > unless patch 4 covers them. > - Not exercised: a devicetree without the OPP table or without > assigned-clock-rates (code-read only), throttling at 85 degrees (not > reachable here), system suspend (s2idle does not resume on this board, > for reasons outside the NPU), kexec from a raised clock, and any other > board. No milliwatt figure, no energy model. > > Routing > ------- > > Patches 1-4 are fixes with Cc: stable and 9 builds on 1, 2 and 4, so the > simplest route is all but the two dts patches through drm-misc-next. The > binding (6) must land before those (7 and 11, Heiko's tree), which I can > resend once the rest is in. 1-3 supersede their standalone postings > (links under each ---); 5 is also 01/14 of Jiaxing Hu's RK3576 series, > and whichever lands first, the other drops it. I have not rebased onto > that series. > > Questions > --------- > > Q1. DT maintainers: is opp-shared on all three cores the right > description of one clock and one supply? v2 leaves opp-suspend out; > Nicolas found it "not 100% fit" for 200 MHz but not unfit either, > and asked for your view [6]. > Q2. Maximum over the cores, or a summed busy count? And no OPP table > means no devfreq and no error, as in panfrost and lima: the policy > you want? > Q3. assigned-clock-rates stays on all three nodes. Jiaxing reports that > on the RK3576 it hangs the board before the console when set on the > SCMI clock, and that the vendor driver never writes that rate from > DT [7]. I have not booted this board without it. Keep it, or drop it > in a patch on top? > > Nicolas also floated the component framework; I have not tried it. > > Credits > ------- > > Nicolas Dufresne: review, opp-shared, the TF-A pointer, and a proof of > concept with the same rates and voltages. Tomeu Vizoso agreed to the > full-range table [8]. Jiaxing Hu: the RK3576 multi-core corruption the > three-client test answers, a test and a review. Sidong Yang: the report > behind patch 3 and a test. > > Jonas Karlman pointed me at his GPU approach, a .config_clks() that > checks pm_runtime_suspended() [9][10], and warned that "just reading > clk_summary to read SCMI clock rate can issue SError or a freeze" [11]. > From the code, not a test: ->get_cur_freq() returns the last request, > probe reads the boot rate with the domain on, and with the in-tree > devicetree every set_rate that reaches the firmware, re-read included, > runs with the cores held. > > Hüseyin BIYIK wrote TF-A commit d2d6928641ba, which makes BL31 report > the PVTPLL rate it measures rather than the one asked for, and guessed > early that the clock framework ignores what SCMI answers [12]; for a > refused set_rate it does. > > The Assisted-by: LLM tags are Claude (Fable 5.1 and Opus 5; the v1 > cover named only Opus 5, though Fable 5.1 worked on v1 too). We wrote > the code and these messages together, including the v3 changes to patch > 4; it also found the silent refusal with me in the clock, OPP and TF-A > code, wrote and adversarially reviewed the test scripts, and analysed > the crash patch 4 fixes and the one under Not done. I threw out plenty > of what it proposed. The board, every boot and measurement, the decision > to send this and the responsibility for it are mine. > > [1] > https://lore.kernel.org/r/capsqs2qxqda2ftbejb_-qeva9ay4+4ysdputj8javbyasyi...@mail.gmail.com > [2] https://lore.kernel.org/r/[email protected] > [3] https://lore.kernel.org/r/[email protected] > [4] > https://lore.kernel.org/r/[email protected] > [5] https://lore.kernel.org/r/[email protected] > [6] > https://lore.kernel.org/r/[email protected] > [7] https://lore.kernel.org/r/[email protected] > [8] > https://lore.kernel.org/r/capsqs2rruyg1vz5nm7990yd0hckr5e-swhbt0ishavyvcqq...@mail.gmail.com > [9] https://lore.kernel.org/r/[email protected] > [10] > https://github.com/Kwiboo/linux-rockchip/commits/next-20260619-rk35xx-scmi-gpu-clk/ > [11] https://lore.kernel.org/r/[email protected] > [12] https://lore.kernel.org/r/[email protected] > > Igor Paunovic (11): > accel/rocket: search every core slot when a core is removed > accel/rocket: number the cores by devicetree position, not bind order > accel/rocket: search every core slot when looking up a scheduler > accel/rocket: keep core slots stable across unbind and rebind > accel/rocket: request the core clocks by name > dt-bindings: npu: rockchip: allow DVFS and thermal properties > arm64: dts: rockchip: rk3588: add an OPP table for the NPU > accel/rocket: restore the NPU clock boot rate before powering the > cores down > accel/rocket: add devfreq support > accel/rocket: register a devfreq cooling device > arm64: dts: rockchip: rk3588: add passive cooling to the NPU thermal > zone > > .../npu/rockchip,rk3588-rknn-core.yaml | 10 + > arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 17 +- > arch/arm64/boot/dts/rockchip/rk3588-opp.dtsi | 54 ++ > drivers/accel/rocket/Kconfig | 2 + > drivers/accel/rocket/Makefile | 1 + > drivers/accel/rocket/rocket_core.c | 14 + > drivers/accel/rocket/rocket_core.h | 16 + > drivers/accel/rocket/rocket_devfreq.c | 525 ++++++++++++++++++ > drivers/accel/rocket/rocket_devfreq.h | 67 +++ > drivers/accel/rocket/rocket_device.c | 2 + > drivers/accel/rocket/rocket_device.h | 21 + > drivers/accel/rocket/rocket_drv.c | 192 ++++++- > drivers/accel/rocket/rocket_job.c | 47 +- > 13 files changed, 945 insertions(+), 23 deletions(-) > create mode 100644 drivers/accel/rocket/rocket_devfreq.c > create mode 100644 drivers/accel/rocket/rocket_devfreq.h > > > base-commit: 8ef59ee794076e2b58cff357b12de2ba5d441271
signature.asc
Description: This is a digitally signed message part
