diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi index 677afe4c5..db7af7e9d 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi @@ -3510,6 +3510,7 @@ pvtm@4 { rknpu: npu@fdab0000 { compatible = "rockchip,rk3588-rknpu"; + rockchip,sram = <&rkvdec0_sram>; reg = <0x0 0xfdab0000 0x0 0x10000>, <0x0 0xfdac0000 0x0 0x10000>, <0x0 0xfdad0000 0x0 0x10000>; @@ -4280,7 +4281,6 @@ rkvdec0: rkvdec-core@fdc38000 { rockchip,core-mask = <0x00010001>; rockchip,task-capacity = <16>; rockchip,taskqueue-node = <9>; - rockchip,sram = <&rkvdec0_sram>; /* rcb_iova: start and size 1M@4095M */ rockchip,rcb-iova = <0xFFF00000 0x100000>; rockchip,rcb-info = <136 24576>, <137 49152>, <141 90112>, <140 49152>, @@ -4334,7 +4334,6 @@ rkvdec1: rkvdec-core@fdc48000 { rockchip,core-mask = <0x00020002>; rockchip,task-capacity = <16>; rockchip,taskqueue-node = <9>; - rockchip,sram = <&rkvdec1_sram>; /* rcb_iova: start and size 1M@4094M */ rockchip,rcb-iova = <0xFFE00000 0x100000>; rockchip,rcb-info = <136 24576>, <137 49152>, <141 90112>, <140 49152>, @@ -6941,10 +6940,7 @@ syssram: sram@ff001000 { ranges = <0x0 0x0 0xff001000 0xef000>; /* start address and size should be 4k algin */ rkvdec0_sram: rkvdec-sram@0 { - reg = <0x0 0x78000>; - }; - rkvdec1_sram: rkvdec-sram@78000 { - reg = <0x78000 0x77000>; + reg = <0x0 0xef000>; }; }; diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index c48af6b2d..934838b56 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -30,6 +30,23 @@ /** MMU register offsets */ #define RK_MMU_DTE_ADDR 0x00 /* Directory table address */ + +/* #patch29 (diagnostic): rk_iommu_attach_device() returns SUCCESS without touching hardware + * when the IOMMU is not runtime-active -- the domain is recorded in software but + * RK_MMU_DTE_ADDR still points at the PREVIOUS page table. rk_iommu_resume() is supposed to + * program it later. If a master (the NPU) issues DMA in the window between the two, it walks + * the WRONG table; because sibling domains allocate overlapping IOVAs the access RESOLVES + * instead of faulting, so the failure is completely silent. Count both halves so the window + * can be correlated with NPU dispatch stalls. */ +unsigned long rk_dbg_attach_deferred; +unsigned long rk_dbg_attach_programmed; +unsigned long rk_dbg_resume_programmed; +EXPORT_SYMBOL(rk_dbg_attach_deferred); +EXPORT_SYMBOL(rk_dbg_attach_programmed); +EXPORT_SYMBOL(rk_dbg_resume_programmed); +module_param_named(dbg_attach_deferred, rk_dbg_attach_deferred, ulong, 0444); +module_param_named(dbg_attach_programmed, rk_dbg_attach_programmed, ulong, 0444); +module_param_named(dbg_resume_programmed, rk_dbg_resume_programmed, ulong, 0444); #define RK_MMU_STATUS 0x04 #define RK_MMU_COMMAND 0x08 #define RK_MMU_PAGE_FAULT_ADDR 0x0C /* IOVA of last page fault */ @@ -1339,9 +1356,16 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, rk_domain->shootdown_entire = iommu->shootdown_entire; ret = pm_runtime_get_if_in_use(iommu->dev); - if (!ret || WARN_ON_ONCE(ret < 0)) + if (!ret || WARN_ON_ONCE(ret < 0)) { + /* #patch29: HW page table NOT reprogrammed; deferred to rk_iommu_resume() */ + rk_dbg_attach_deferred++; + pr_warn_ratelimited( + "rk_iommu: attach DEFERRED (not runtime-active), DTE_ADDR still stale (n=%lu)\n", + rk_dbg_attach_deferred); return 0; + } + rk_dbg_attach_programmed++; ret = rk_iommu_enable(iommu); if (ret) rk_iommu_detach_device(iommu->domain, dev); @@ -1777,6 +1801,7 @@ static int __maybe_unused rk_iommu_resume(struct device *dev) if (iommu->dlr_disable) return 0; + rk_dbg_resume_programmed++; /* #patch29 */ return rk_iommu_enable(iommu); } diff --git a/drivers/rknpu/include/rknpu_drv.h b/drivers/rknpu/include/rknpu_drv.h index 3f3804ad7..478c30a8a 100644 --- a/drivers/rknpu/include/rknpu_drv.h +++ b/drivers/rknpu/include/rknpu_drv.h @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -100,6 +101,11 @@ struct rknpu_subcore_data { struct rknpu_job *job; int64_t task_num; struct rknpu_timer timer; + /* #patch: progress watchdog. wd_last = last PC completed-task counter seen, + * wd_flat = consecutive samples with no advance. */ + void *wd_job; /* #patch44: job the wd_flat run belongs to */ + u32 wd_last; + u32 wd_flat; }; /** @@ -159,6 +165,11 @@ struct rknpu_device { struct rknpu_debugger debugger; struct hrtimer timer; ktime_t kt; + /* #patch: progress watchdog timer (armed on submit, self-disarms when idle) */ + struct hrtimer wd_timer; + struct work_struct wd_work; + ktime_t wd_kt; + int wd_on; phys_addr_t sram_start; phys_addr_t sram_end; phys_addr_t nbuf_start; @@ -186,4 +197,6 @@ int rknpu_power_get(struct rknpu_device *rknpu_dev); int rknpu_power_put(struct rknpu_device *rknpu_dev); int rknpu_power_put_delay(struct rknpu_device *rknpu_dev); +void rknpu_wd_arm(struct rknpu_device *rknpu_dev); + #endif /* __LINUX_RKNPU_DRV_H_ */ diff --git a/drivers/rknpu/include/rknpu_gem.h b/drivers/rknpu/include/rknpu_gem.h index 61e8fa22c..f7dc952b2 100644 --- a/drivers/rknpu/include/rknpu_gem.h +++ b/drivers/rknpu/include/rknpu_gem.h @@ -121,7 +121,12 @@ static inline void rknpu_gem_object_put(struct drm_gem_object *obj) /* * get rknpu drm object from gem handle, this function could be used for * other drivers such as 2d/3d acceleration drivers. - * with this function call, gem object reference count would be increased. + * + * #patch37 WARNING: this does NOT give you a reference. It looks the object up (which takes one) + * and immediately puts it back, so the returned pointer is kept alive ONLY by the handle table and + * is valid only while that handle cannot be closed concurrently. Do NOT hold it across anything + * that sleeps -- see rknpu_gem_destroy_ioctl(), which used to hold it across an ~18 s domain-switch + * wait. The previous version of this comment claimed the refcount "would be increased"; it does not. */ static inline struct rknpu_gem_object * rknpu_gem_object_find(struct drm_file *filp, unsigned int handle) diff --git a/drivers/rknpu/include/rknpu_ioctl.h b/drivers/rknpu/include/rknpu_ioctl.h index f4e3b83bf..c49e5c9eb 100644 --- a/drivers/rknpu/include/rknpu_ioctl.h +++ b/drivers/rknpu/include/rknpu_ioctl.h @@ -134,6 +134,30 @@ enum e_rknpu_action { RKNPU_GET_FREE_SRAM_SIZE = 23, RKNPU_GET_IOMMU_DOMAIN_ID = 24, RKNPU_SET_IOMMU_DOMAIN_ID = 25, + + /* #patch: per-core reset, the symmetric counterpart of RKNPU_ACT_RESET (which is device-wide). + * args->value = core index. Value 0x100 is deliberately outside the vendor's range so a future + * vendor action cannot collide with it. + * + * Does per-core AXI+AHB reset AND the IOMMU re-attach: the re-attach is REQUIRED, because + * rk_iommu shares the NPU reset domain, so resetting a core wipes its MMU programming and any + * subsequent submit has a valid IOVA with no translation behind it. Measured: reset alone leaves + * the job unrecoverable (deficit 2.0/stall); reset + re-attach recovers it (1.0/stall). */ + RKNPU_ACT_RESET_CORE = 0x100, + /* #patch61: tear down and rebuild ALL iommu domain state — an explicit shortcut for the + * reboot that is currently the only way to clear it. Outside the vendor range (0-25). + * + * WHY: unbalanced domain puts accumulate across runs (rknpu_job_abort drives the refcount to + * zero while other cores still hold work) until every switch fails with + * "mismatch domain get from iommu_get_domain_for_dev" and then every allocation fails with + * "rknpu_gem_get_pages: dma map ... fail". Nothing clears it short of a reboot (~90 s, and it + * destroys any in-RAM diagnostics). + * + * MANUAL and EXPLICIT — not fired automatically. The driver refuses with -EBUSY if any core + * still has a job, so it cannot run underneath live work. CALLER CONTRACT: this changes the + * IOVAs, so every existing buffer AND every regcmd built against those IOVAs is invalid + * afterwards and must be rebuilt. */ + RKNPU_ACT_REINIT = 0x101, }; /** diff --git a/drivers/rknpu/include/rknpu_iommu.h b/drivers/rknpu/include/rknpu_iommu.h index 225176650..e4e62c79b 100644 --- a/drivers/rknpu/include/rknpu_iommu.h +++ b/drivers/rknpu/include/rknpu_iommu.h @@ -58,3 +58,5 @@ int iommu_get_dma_cookie(struct iommu_domain *domain); #endif #endif + +int rknpu_iommu_reinit(struct rknpu_device *rknpu_dev); /* #patch61 */ diff --git a/drivers/rknpu/include/rknpu_job.h b/drivers/rknpu/include/rknpu_job.h index b4d40d96f..78acd0b22 100644 --- a/drivers/rknpu/include/rknpu_job.h +++ b/drivers/rknpu/include/rknpu_job.h @@ -19,6 +19,11 @@ #define RKNPU_JOB_DONE (1 << 0) #define RKNPU_JOB_ASYNC (1 << 1) +/* #patch41: the progress watchdog has declared this job stalled. INTERNAL to job->flags (which is + * never assigned from the uABI args->flags), so it cannot collide with RKNPU_JOB_PC/NONBLOCK/etc. */ +#define RKNPU_JOB_STALLED (1 << 8) + +/* #patch47 profiler fields (filled at commit; free -- derived from existing timestamps) */ #define RKNPU_JOB_DETACHED (1 << 2) #define RKNPU_CORE_AUTO_MASK 0x00 @@ -49,6 +54,19 @@ struct rknpu_job { ktime_t hw_elapse_time; atomic_t submit_count[RKNPU_MAX_CORES]; int iommu_domain_id; + s64 prof_gap_us; /* #patch47: since previous commit on this core */ + s64 prof_queue_us; + unsigned int prof_since_switch; + s64 prof_tsince_us; /* #patch52: us since the last domain switch */ /* #patch51 */ /* #patch47: submit -> commit */ + /* #patch (experiment): PC register snapshot taken at commit — what we WROTE and what + * reads BACK immediately after. Reported by the progress watchdog when a job stalls, so + * a stalled commit can be compared against a healthy one with no per-commit logging. */ + uint32_t dbg_blk[17]; /* #patch: full PC/INT register block sampled after the start pulse */ + ktime_t dbg_kick_t; /* #patch: when recovery kicked this job (0 = never) */ + uint32_t dbg_seq; /* #patch: userspace task-descriptor stamp seen at commit */ + uint32_t dbg_wr[6]; + uint32_t dbg_rd[7]; + bool dbg_valid; }; irqreturn_t rknpu_core0_irq_handler(int irq, void *data); @@ -79,3 +97,5 @@ int rknpu_get_rw_amount(struct rknpu_device *rknpu_dev, uint32_t *dt_wr, int rknpu_get_total_rw_amount(struct rknpu_device *rknpu_dev, uint32_t *amount); #endif /* __LINUX_RKNPU_JOB_H_ */ + +void rknpu_reap_all_cores(struct rknpu_device *rknpu_dev); /* #patch54 */ diff --git a/drivers/rknpu/include/rknpu_reset.h b/drivers/rknpu/include/rknpu_reset.h index 5e3094871..dead4f74b 100644 --- a/drivers/rknpu/include/rknpu_reset.h +++ b/drivers/rknpu/include/rknpu_reset.h @@ -15,4 +15,8 @@ int rknpu_reset_get(struct rknpu_device *rknpu_dev); int rknpu_soft_reset(struct rknpu_device *rknpu_dev); +int rknpu_soft_reset_core(struct rknpu_device *rknpu_dev, int core); + +int rknpu_reset_core_and_remap(struct rknpu_device *rknpu_dev, int core); + #endif diff --git a/drivers/rknpu/rknpu_drv.c b/drivers/rknpu/rknpu_drv.c index 4090344b4..01504a779 100644 --- a/drivers/rknpu/rknpu_drv.c +++ b/drivers/rknpu/rknpu_drv.c @@ -37,6 +37,7 @@ #include #endif +#include #include "rknpu_ioctl.h" #include "rknpu_reset.h" #include "rknpu_fence.h" @@ -417,6 +418,19 @@ static int rknpu_action(struct rknpu_device *rknpu_dev, break; case RKNPU_SET_VOLT: break; + case RKNPU_ACT_RESET_CORE: { + /* #patch: per-core counterpart of RKNPU_ACT_RESET. Cheaper than the device-wide path — no + * msleep(100), only two reset lines — but it still needs the same quiesce for the IOMMU + * re-attach, which is unsafe asynchronously (it raced rknpu_iommu_dma_map_sg and panicked). */ + int _core = (int)args->value; + if (_core < 0 || _core >= rknpu_dev->config->num_irqs) + return -EINVAL; + ret = rknpu_reset_core_and_remap(rknpu_dev, _core); + break; + } + case RKNPU_ACT_REINIT: /* #patch61 */ + ret = rknpu_iommu_reinit(rknpu_dev); + break; case RKNPU_ACT_RESET: ret = rknpu_soft_reset(rknpu_dev); break; @@ -778,17 +792,454 @@ static enum hrtimer_restart hrtimer_handler(struct hrtimer *timer) return HRTIMER_RESTART; } +/* ================= PROGRESS WATCHDOG (#patch) ========================================== + * WHY. rknpu_job_timeout_clean is not a watchdog: it runs only when ANOTHER submit arrives on + * the same core, so a stalled job with no follow-up submit is never noticed (which is why + * userspace has to fire "reap dummy" submits). It also measures TOTAL JOB AGE, meaningless for + * a PC-chained job whose task list is extensible at runtime. "No task completed in N samples" + * is well defined for a chain of any length. + * + * CONFIRMED FINDING: on a doorbell miss the PC completed-task counter reads 0 with the job many + * ms old -- the job is accepted and NEVER DISPATCHES. (Control: at an 8 ms threshold a healthy + * ~30 ms job does NOT report, so the counter genuinely advances and the offset is right.) + * + * POWER SAFETY -- the hard part, and v1/v2 got it wrong. + * v1 read NPU MMIO straight from the hrtimer. A register read on a powered-down block is an + * external abort => instant HARD hang (no ping, power cycle required). It hung the board on + * its first NPU workload. + * v2 guarded with pm_runtime_get_if_in_use(rknpu_dev->dev). STILL WRONG on RK3588: this driver + * keeps its OWN refcount (power_refcount/power_lock) and rknpu_power_off() puts the three + * PER-CORE genpds (genpd_dev_npu0/1/2) directly. A reference on the parent does not hold those + * up, so core i's domain can still drop between the check and the readl on base[i]. + * v3 (this): sample in PROCESS context and hold power_lock across the read. rknpu_power_off() + * takes the same mutex, so the genpds cannot go down while we read. We deliberately do NOT call + * rknpu_power_get(): that would POWER THE NPU ON just to read a counter and pin it up forever. + * mutex_trylock means the watchdog can never delay a submit; a missed sample is simply skipped + * rather than counted as a no-advance. + * + * The hrtimer only decides cadence and queues the work; all MMIO happens in the worker. */ +/* DEFAULT OFF. This watchdog has destabilised the board three times: v1 hard-hung it (unguarded MMIO, + * fixed), and even the power_lock-guarded v3 COLD-RESET it twice mid-run (instant SoC reset, no panic, + * no console, ramoops cleared). Discriminator, same workload: wd_period_us=1000 -> reset; + * wd_period_us=0 -> 4000 reps clean, same boot_id. It is a DIAGNOSTIC: set wd_period_us=1000 only for + * a deliberate investigation, accepting that the board may reset. */ +/* #patch40: the progress watchdog is ON BY DEFAULT. + * + * It was opt-in because two earlier versions killed the board (v1 read NPU MMIO straight from the + * hrtimer; v2's pm_runtime_get_if_in_use on the parent missed the per-core genpds). v3 samples in + * process context holding power_lock and has since run clean. The other reason -- "it caused board + * resets" -- was RETRACTED; that was the vendor IRQ bug fixed as change 6. + * + * Opt-in made it useless in practice: the one time it was needed, it was enabled by hand and + * recorded wd_ticks=0 across 8 stalls, because the arm site was NONBLOCK-only (fixed in #patch39). + * A stalled BLOCKING submit otherwise sits in rknpu_job_wait() for its whole timeout -- 60.6 s + * measured -- with nothing reporting it. Detection at ~120 ms is a ~500x cut in time-to-visibility. + * + * Cost, measured A/B on this board (2000-rep int4 probe, ndom=1, interleaved x3, warmup discarded): + * OFF 133/133/133 commits/s ON 131/133/129 commits/s => ~1.5% + * Paid on every submit since #patch39. Judged worth it: the stalls it exposes cost ~23% of 27B + * prefill and were previously invisible. + * + * Detection latency is wd_samples * wd_period_us ~= 120 ms. Do NOT drop the period to 1000 without + * also dropping wd_samples: 1 kHz sampling costs ~100x throughput (2.8 vs 283-490 commits/s). */ +static unsigned int rknpu_wd_period_us = 10000; +module_param_named(wd_period_us, rknpu_wd_period_us, uint, 0644); +MODULE_PARM_DESC(wd_period_us, "progress watchdog sample period in us (0 disables)"); + +/* diagnostics: is the watchdog actually sampling, and why not when it isn't */ +static unsigned long rknpu_wd_ticks, rknpu_wd_seen_job, rknpu_wd_pm_skip, rknpu_wd_stalls; +module_param_named(wd_ticks, rknpu_wd_ticks, ulong, 0444); +module_param_named(wd_seen_job, rknpu_wd_seen_job, ulong, 0444); +module_param_named(wd_pm_skip, rknpu_wd_pm_skip, ulong, 0444); +module_param_named(wd_stalls, rknpu_wd_stalls, ulong, 0444); + +/* START-CONFIRM-AND-RETRY (wd_kick=1). + * A stalled job is committed with byte-identical registers, a valid mapping, a fresh descriptor and + * verified-correct regcmd bytes in DRAM — it simply never begins, and the completed-task counter stays 0. + * The registers are still correct at that point, so rather than the current recovery (userspace waits out + * a ~614 ms window, then issues a DEVICE-WIDE ACT_RESET that destroys warm state on all three cores, then + * resubmits — ~3.6 s per miss), just RE-PULSE the start trigger for that one core. + * + * Cheap and targeted: two register writes, no reset, no warm-state loss, no resubmit. If the PC merely + * missed its start, this restarts it; if it does not, we have lost nothing and learned that the fault is + * not a missed start trigger. Fires ONCE per stall episode, on the same condition as the report, and only + * with the counter still reading 0 (i.e. nothing has executed, so re-pulsing cannot disturb work in + * flight). Opt-in while under evaluation. */ +static unsigned long rknpu_cnt_kick; +module_param_named(cnt_kick, rknpu_cnt_kick, ulong, 0444); + +static unsigned int rknpu_wd_samples = 12; +/* #patch41: FAST-ABORT. When a BLOCKING submit stalls, its caller sits in rknpu_job_wait() for + * args->timeout x 3 -- 60.8 s measured on a real 1.5B-Q8 run -- before getting -ETIMEDOUT and + * self-healing. Waking it as soon as the stall is established turns that into ~wd_abort_ms. + * + * DELIBERATELY NOT the logging threshold. The PC counter advances per COMPLETED TASK, so a single + * long-running task is indistinguishable from a stall by this signal alone; aborting at the ~120 ms + * log threshold would kill slow-but-healthy jobs. This is a separate, much larger bound: still + * ~30x faster than 60 s while leaving any plausible single task room to finish. + * + * 0 disables. OPT-IN on purpose -- the previous automatic stall intervention (wd_kick) was deleted + * for having no measurable benefit, so this one earns its default only once measured. */ +static unsigned int rknpu_wd_abort_ms; +module_param_named(wd_abort_ms, rknpu_wd_abort_ms, uint, 0644); +MODULE_PARM_DESC(wd_abort_ms, "fail a stalled BLOCKING submit after this many ms (0 = off)"); +/* #patch47: opt-in timing profiler. prof_mask != 0 enables it (bit0 = per-stall breakdown, + * bit1 = also log healthy commits). Everything it reports is derived from timestamps the driver + * already takes, so the cost when disabled is one branch and when enabled is one printk. */ +/* #patch49: CAUSAL TEST for the change-16 correlation. + * + * Every stalled job was committed 0.24-0.9 ms after the previous commit on the same core, against a + * healthy mean three orders of magnitude larger. That is correlation. This enforces a MINIMUM spacing + * between commits on a core so the hypothesis can be tested directly: if stalls vanish as this rises, + * back-to-back commits cause them; if they persist, the short gap is a symptom of something else. + * + * 0 = off (default). Clamped to 5000 us because this MUST busy-wait: rknpu_job_next() can be reached + * from the completion IRQ, where sleeping is illegal. A diagnostic knob, not a shipping fix -- a real + * fix would reshape submission rather than burn CPU in udelay(). */ +static unsigned int rknpu_min_commit_gap_us; +module_param_named(min_commit_gap_us, rknpu_min_commit_gap_us, uint, 0644); +MODULE_PARM_DESC(min_commit_gap_us, "enforce >= this gap between commits on a core (0=off, max 5000)"); +unsigned long rknpu_gap_enforced_n, rknpu_gap_enforced_us; +module_param_named(gap_enforced_n, rknpu_gap_enforced_n, ulong, 0444); +module_param_named(gap_enforced_us_sum, rknpu_gap_enforced_us, ulong, 0444); +unsigned int rknpu_min_commit_gap_get(void) { return rknpu_min_commit_gap_us; } + +/* #patch55: SUB-BLOCK register dump — is the NPU stuck mid-read, or did it never issue one? + * + * At stall time the PC completed-task counter reads 0x0 and all four MMU banks read IDLE + * (0x19 = paging|idle|replay-empty). Those two facts pull in opposite directions: MMU-idle says no + * translation is outstanding, i.e. nothing was ever requested; but the job WAS triggered, so + * something should have been. The missing instrument is the NPU's own sub-blocks — we only ever + * dumped the PC block. + * + * CNA 0x1000 does the convolution feeder think it is running? + * DPU 0x4000 output stage state + * CDMA 0x5000 IS A DMA READ OUTSTANDING? <- the discriminator + * + * CDMA busy while the MMU is idle == a transaction stuck UPSTREAM of translation (a hung read). + * CDMA idle too == the trigger never took and nothing was issued. + * + * Deliberately dumped BLIND rather than by name: mainline drivers/accel/rocket names these, but it + * is a much newer kernel than this tree, and we do not need names to diff a stalled snapshot against + * a healthy one. Names can follow once we know which words move. + * + * Safety: each core maps 0x10000 (rk3588s.dtsi), so 0x1000/0x4000/0x5000 are in range; and this runs + * only from the watchdog worker, which is process context holding power_lock with power_refcount > 0 + * (patch06: an MMIO read of a powered-down block kills the machine instantly). Capped so a stall + * storm cannot flood the log. */ +/* #patch58: does the PC completed-task counter EVER read non-zero while a job is in flight? + * + * The cnt>0 healthy control never fired across a whole run despite tens of thousands of samples. + * If cnt is always 0 for in-flight jobs then `cnt == wd_last` is trivially true and the watchdog is + * not a progress detector at all — it is a "job present for > wd_samples*period" duration timer, and + * the "counter 0x0" in every stall report is NOT evidence that the hardware never started, because + * healthy jobs would report it too. That would invalidate the framing this whole investigation has + * carried. Record the maximum ever observed and settle it. */ +static unsigned int rknpu_wd_max_cnt; +module_param_named(wd_max_cnt, rknpu_wd_max_cnt, uint, 0444); +static unsigned long rknpu_wd_cnt_nonzero; +module_param_named(wd_cnt_nonzero, rknpu_wd_cnt_nonzero, ulong, 0444); + +static unsigned int rknpu_prof_blk_cap = 4; +module_param_named(prof_blk_cap, rknpu_prof_blk_cap, uint, 0644); +static unsigned int rknpu_prof_blk_done; +static unsigned int rknpu_prof_blk_healthy_done; + +static void rknpu_blk_dump(void __iomem *base, int core, const char *tag) +{ + static const u32 blk[] = { 0x1000, 0x4000, 0x5000 }; + static const char *nm[] = { "CNA", "DPU", "CDMA" }; + int b, i; + + /* #patch57: 16 words per block, not 8. The first 8 were byte-identical between healthy and + * stalled, which may simply mean they are static config. Widen so a word that actually moves + * has a chance to appear. */ + for (b = 0; b < 3; b++) { + u32 v[16]; + int h; + + for (i = 0; i < 16; i++) + v[i] = readl(base + blk[b] + i * 4); + for (h = 0; h < 2; h++) + LOG_ERROR("RKNPU: BLK[%s] core%d %-4s +0x%04x: %08x %08x %08x %08x %08x %08x %08x %08x\n", + tag, core, nm[b], blk[b] + h * 32, + v[h*8+0], v[h*8+1], v[h*8+2], v[h*8+3], + v[h*8+4], v[h*8+5], v[h*8+6], v[h*8+7]); + } +} + +static unsigned int rknpu_prof_mask; +module_param_named(prof_mask, rknpu_prof_mask, uint, 0644); +MODULE_PARM_DESC(prof_mask, "timing profiler: 0=off, 1=stall breakdown, 3=+healthy commits"); +/* #patch50: shape-axis profiling. bit1 = log every stall with its shape (already on), + * bit2 = ALSO log every commit with its shape, so the HEALTHY distribution can be built. + * ~1100 lines per run — trivial for the persistent disk log, and it keeps all the binning + * offline in awk instead of hard-coding a histogram in the kernel. */ +unsigned int rknpu_prof_mask_get(void) { return rknpu_prof_mask; } +/* #patch48: non-static — rknpu_job_done() (other TU) accumulates the healthy side */ +unsigned long rknpu_prof_ok_n, rknpu_prof_ok_gap, rknpu_prof_ok_queue; +module_param_named(prof_ok_n, rknpu_prof_ok_n, ulong, 0444); +module_param_named(prof_ok_gap_us_sum, rknpu_prof_ok_gap, ulong, 0444); +module_param_named(prof_ok_queue_us_sum, rknpu_prof_ok_queue, ulong, 0444); +static unsigned long rknpu_prof_bad_n, rknpu_prof_bad_gap, rknpu_prof_bad_queue; +module_param_named(prof_bad_n, rknpu_prof_bad_n, ulong, 0444); +module_param_named(prof_bad_gap_us_sum, rknpu_prof_bad_gap, ulong, 0444); +module_param_named(prof_bad_queue_us_sum, rknpu_prof_bad_queue, ulong, 0444); + +static unsigned long rknpu_wd_queued_aborts; +module_param_named(wd_queued_aborts, rknpu_wd_queued_aborts, ulong, 0444); +static unsigned long rknpu_wd_aborts; +module_param_named(wd_aborts, rknpu_wd_aborts, ulong, 0444); /* #patch40: 12 x 10 ms ~= 120 ms detection */ +module_param_named(wd_samples, rknpu_wd_samples, uint, 0644); +MODULE_PARM_DESC(wd_samples, "consecutive no-advance samples before reporting a stall"); + +/* Process context: safe to take power_lock, so safe to touch MMIO. */ +static void rknpu_wd_work_fn(struct work_struct *work) +{ + struct rknpu_device *rknpu_dev = + container_of(work, struct rknpu_device, wd_work); + struct rknpu_subcore_data *subcore_data = NULL; + struct rknpu_job *job = NULL; + unsigned long flags; + int i; + + /* never block a submit: power_lock is held by rknpu_power_get/put on the hot path */ + if (!mutex_trylock(&rknpu_dev->power_lock)) { + rknpu_wd_pm_skip++; + return; + } + /* powered off => nothing is running and MMIO is unsafe; skip without penalising wd_flat */ + if (atomic_read(&rknpu_dev->power_refcount) <= 0) { + rknpu_wd_pm_skip++; + mutex_unlock(&rknpu_dev->power_lock); + return; + } + + for (i = 0; i < rknpu_dev->config->num_irqs; i++) { + void __iomem *rknpu_core_base = rknpu_dev->base[i]; + u32 cnt; + + subcore_data = &rknpu_dev->subcore_datas[i]; + + spin_lock_irqsave(&rknpu_dev->irq_lock, flags); + job = subcore_data->job; + spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); + + if (!job) { + subcore_data->wd_flat = 0; + subcore_data->wd_job = NULL; /* #patch44 */ + continue; + } + rknpu_wd_seen_job++; + + cnt = readl(rknpu_core_base + + rknpu_dev->config->pc_task_status_offset) & + rknpu_dev->config->pc_task_number_mask; + + /* #patch44: a stall run must be attributed to a JOB, not just to a counter value. + * + * wd_flat only reset when the PC counter CHANGED. Consecutive jobs on one core each + * start their counter at 0, so across a job boundary the counter looks unchanged and + * wd_flat keeps climbing. Both the stall report and the fast-abort fire on EQUALITY + * (== wd_samples / == an), so only the FIRST stall in a run of them was ever seen and + * every later one was silently absorbed. + * + * Measured before this fix: 10 timed-out waits, 5 detected+aborted at 2.0 s, 5 missed + * entirely at ~61 s -- exactly 50% coverage, with the misses in a clean second mode. + * Reset the run whenever the job identity changes. */ + if (subcore_data->wd_job != job) { + subcore_data->wd_job = job; + subcore_data->wd_last = cnt; + subcore_data->wd_flat = 0; + } + + if (cnt > rknpu_wd_max_cnt) + rknpu_wd_max_cnt = cnt; /* #patch58 */ + if (cnt) + rknpu_wd_cnt_nonzero++; + + /* #patch57: a VALID healthy control. + * + * The previous baseline fired at wd_flat == 0, which only means RECENTLY COMMITTED — + * plausibly the identical register state to a job that is about to be reported + * stalled. Comparing that against a stall compares a state with itself, and indeed + * every word matched. Gate on cnt > 0 instead: the PC completed-task counter has + * advanced, so this job has provably executed at least one task and cannot be in the + * failure state (whose defining symptom is counter stuck at 0x0). Also far more + * catchable than requiring the counter to CHANGE between two 10 ms samples. */ + if ((rknpu_prof_mask & 4) && cnt > 0 && + rknpu_prof_blk_healthy_done < rknpu_prof_blk_cap) { + rknpu_prof_blk_healthy_done++; + rknpu_blk_dump(rknpu_core_base, i, "OK"); + } + + if (cnt == subcore_data->wd_last) { + /* fire ONCE per stall episode (equality, not >=) */ + if (++subcore_data->wd_flat == rknpu_wd_samples) { + rknpu_wd_stalls++; + if ((rknpu_prof_mask & 4) && + rknpu_prof_blk_done < rknpu_prof_blk_cap) { + rknpu_prof_blk_done++; + rknpu_blk_dump(rknpu_core_base, i, "STALL"); /* #patch55 */ + } + /* #patch47: attribute this stall's timeline */ + rknpu_prof_bad_n++; + rknpu_prof_bad_gap += (unsigned long)job->prof_gap_us; + rknpu_prof_bad_queue += (unsigned long)job->prof_queue_us; + if (rknpu_prof_mask & 1) + LOG_ERROR("RKNPU: PROF stalled core %d: tn=%u cm=%#x dom=%d sinceswitch=%u tsince=%lldus gap_since_prev_commit=%lldus queue(submit->commit)=%lldus | healthy mean gap=%luus queue=%luus over n=%lu\n", + i, job->args->task_number, + job->args->core_mask, + job->iommu_domain_id, + job->prof_since_switch, + job->prof_tsince_us, + job->prof_gap_us, job->prof_queue_us, + rknpu_prof_ok_n ? rknpu_prof_ok_gap / rknpu_prof_ok_n : 0UL, + rknpu_prof_ok_n ? rknpu_prof_ok_queue / rknpu_prof_ok_n : 0UL, + rknpu_prof_ok_n); + /* #patch: report COMPLETED vs SUBMITTED so the line is self-interpreting. + * The kernel has both: the PC counter (completed) and the submit's task + * counts. "0/21" says the job never started; "17/21" would say it stalled + * partway. core_mask is echoed so this pairs with the userspace + * ORK_KMSG_SUBMIT stamp (`ork: sub#N dom=D cm=0xM tasks=T`). */ + LOG_ERROR("RKNPU: core %d NO TASK PROGRESS for %u samples (%u us), tasks=%u/%u done (counter %#x), cm=0x%x dom=%u, job age %lld us\n", + i, subcore_data->wd_flat, + subcore_data->wd_flat * rknpu_wd_period_us, + cnt, + (job->args->subcore_task[i].task_number ? + job->args->subcore_task[i].task_number : + job->args->task_number), + cnt, + job->args->core_mask, + job->args->iommu_domain_id, + ktime_us_delta(ktime_get(), job->timestamp)); + if (job->dbg_valid) + LOG_ERROR("RKNPU: commit snapshot: seq=%u WROTE data_addr=%#x amount=%#x int_mask=%#x task_ctrl=%#x dma_base=%#x tasks=%u | READBACK data_addr=%#x amount=%#x int_mask=%#x task_ctrl=%#x dma_base=%#x status(pre)=%#x status(post)=%#x\n", + job->dbg_seq, job->dbg_wr[0], job->dbg_wr[1], job->dbg_wr[2], + job->dbg_wr[3], job->dbg_wr[4], job->dbg_wr[5], + job->dbg_rd[0], job->dbg_rd[1], job->dbg_rd[2], + job->dbg_rd[3], job->dbg_rd[4], job->dbg_rd[5], + job->dbg_rd[6]); + LOG_ERROR("RKNPU: PCBLK %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x | en=%08x\n", + job->dbg_blk[0], job->dbg_blk[1], job->dbg_blk[2], job->dbg_blk[3], + job->dbg_blk[4], job->dbg_blk[5], job->dbg_blk[6], job->dbg_blk[7], + job->dbg_blk[8], job->dbg_blk[9], job->dbg_blk[10], job->dbg_blk[11], + job->dbg_blk[12], job->dbg_blk[13], job->dbg_blk[14], job->dbg_blk[15], + job->dbg_blk[16]); + } + + /* #patch41: fast-abort a stalled BLOCKING submit. ASYNC jobs have no + * waiter, so there is nothing to wake. Equality fires it once. */ + if (rknpu_wd_abort_ms && !(job->flags & RKNPU_JOB_ASYNC)) { + unsigned int an = (rknpu_wd_abort_ms * 1000u) / + (rknpu_wd_period_us ? + rknpu_wd_period_us : 1u); + + if (an && subcore_data->wd_flat == an) { + struct rknpu_job *entry = NULL; + unsigned int qn = 0; + + spin_lock_irqsave(&rknpu_dev->irq_lock, flags); + job->flags |= RKNPU_JOB_STALLED; + /* #patch43: also abort everything QUEUED behind this head. + * + * This is the detection-coverage gap: the sampler only ever + * inspects subcore_data->job, the COMMITTED job, so a submit + * waiting in todo_list is invisible and its waiter burns the + * whole timeout for a job that never even started. Measured: + * 14 userspace submit failures produced only 2 watchdog + * episodes, and fast-abort therefore only helped 2 of 14. + * + * Safe to declare these stalled: rknpu_job_next() promotes + * from todo_list ONLY when subcore_data->job is NULL, so while + * this head is stuck nothing behind it can run. A woken job + * that never committed has last_task == NULL, and + * rknpu_job_wait() already handles exactly that -- it unlinks + * itself from todo_list and returns. No new path needed. */ + list_for_each_entry(entry, + &subcore_data->todo_list, + head[i]) { + if (entry->flags & RKNPU_JOB_ASYNC) + continue; + entry->flags |= RKNPU_JOB_STALLED; + qn++; + } + spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); + /* one wake serves the whole queue: every waiter sleeps on this + * same job_done_wq and re-tests its OWN job->flags */ + wake_up(&subcore_data->job_done_wq); + rknpu_wd_aborts++; + rknpu_wd_queued_aborts += qn; + LOG_ERROR("RKNPU: core %d FAST-ABORT after %u ms, +%u queued behind it (would have waited %u ms x3)\n", + i, rknpu_wd_abort_ms, qn, + job->args->timeout); + } + } + } else { + subcore_data->wd_last = cnt; + subcore_data->wd_flat = 0; + } + } + + mutex_unlock(&rknpu_dev->power_lock); +} + +/* Cadence only — NO MMIO here. Self-disarms once no core has an outstanding job. */ +static enum hrtimer_restart rknpu_wd_handler(struct hrtimer *timer) +{ + struct rknpu_device *rknpu_dev = + container_of(timer, struct rknpu_device, wd_timer); + unsigned long flags; + int i, active = 0; + + rknpu_wd_ticks++; + + spin_lock_irqsave(&rknpu_dev->irq_lock, flags); + for (i = 0; i < rknpu_dev->config->num_irqs; i++) + if (rknpu_dev->subcore_datas[i].job) + active++; + spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); + + if (!active) { + rknpu_dev->wd_on = 0; + return HRTIMER_NORESTART; + } + /* a no-op if the previous sample is still queued, which self-throttles */ + queue_work(system_wq, &rknpu_dev->wd_work); + + hrtimer_forward_now(timer, rknpu_dev->wd_kt); + return HRTIMER_RESTART; +} + +/* Armed from the submit path; self-disarms in the handler once no core has a job. Re-arming an + * already-running hrtimer is harmless, so the wd_on race is benign. */ +void rknpu_wd_arm(struct rknpu_device *rknpu_dev) +{ + if (!rknpu_wd_period_us || rknpu_dev->wd_on) + return; + rknpu_dev->wd_on = 1; + rknpu_dev->wd_kt = ktime_set(0, (u64)rknpu_wd_period_us * 1000); + hrtimer_start(&rknpu_dev->wd_timer, rknpu_dev->wd_kt, HRTIMER_MODE_REL); +} + static void rknpu_init_timer(struct rknpu_device *rknpu_dev) { rknpu_dev->kt = ktime_set(0, RKNPU_LOAD_INTERVAL); hrtimer_init(&rknpu_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); rknpu_dev->timer.function = hrtimer_handler; hrtimer_start(&rknpu_dev->timer, rknpu_dev->kt, HRTIMER_MODE_REL); + /* #patch: watchdog timer is initialised but NOT started — armed on submit */ + hrtimer_init(&rknpu_dev->wd_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + rknpu_dev->wd_timer.function = rknpu_wd_handler; + rknpu_dev->wd_on = 0; + INIT_WORK(&rknpu_dev->wd_work, rknpu_wd_work_fn); } static void rknpu_cancel_timer(struct rknpu_device *rknpu_dev) { hrtimer_cancel(&rknpu_dev->timer); + hrtimer_cancel(&rknpu_dev->wd_timer); /* #patch */ + cancel_work_sync(&rknpu_dev->wd_work); /* #patch */ } static bool rknpu_is_iommu_enable(struct device *dev) diff --git a/drivers/rknpu/rknpu_gem.c b/drivers/rknpu/rknpu_gem.c index f1346f98f..c9dbc2768 100644 --- a/drivers/rknpu/rknpu_gem.c +++ b/drivers/rknpu/rknpu_gem.c @@ -878,10 +878,25 @@ int rknpu_gem_create_ioctl(struct drm_device *drm, void *data, { struct rknpu_mem_create *args = data; struct rknpu_gem_object *rknpu_obj = NULL; + struct drm_gem_object *obj = NULL; int ret = -EINVAL; - rknpu_obj = rknpu_gem_object_find(file_priv, args->handle); - if (!rknpu_obj) { + /* #patch38: hold a real reference, same reasoning as #patch37 in + * rknpu_gem_destroy_ioctl(). rknpu_gem_object_find() is lookup-then-put and returns an + * UNREFERENCED pointer kept alive only by the handle table, so every dereference below + * raced a concurrent close of that handle. The window here is far shorter than the + * destroy path's (nothing sleeps between the lookup and the last use), which is why this + * one is a hardening fix rather than a suspected crash source -- but the object is read + * four times after the lookup, and correctness should not rest on "probably not + * preempted there". + * + * Both branches below leave exactly ONE reference owned by us, dropped at the end. Net + * refcount across the ioctl is unchanged from before: the handle keeps the object alive + * afterwards, exactly as it always did. */ + obj = drm_gem_object_lookup(file_priv, args->handle); + if (obj) { + rknpu_obj = to_rknpu_obj(obj); + } else { rknpu_obj = rknpu_gem_object_create(drm, args->flags, args->size, args->sram_size, args->iommu_domain_id, @@ -895,15 +910,19 @@ int rknpu_gem_create_ioctl(struct drm_device *drm, void *data, rknpu_gem_object_destroy(rknpu_obj); return ret; } + /* rknpu_gem_handle_create() dropped the allocation reference (the handle owns it + * now), so take our own to match the lookup branch. */ + obj = &rknpu_obj->base; + drm_gem_object_get(obj); } - // rknpu_gem_object_get(&rknpu_obj->base); - args->size = rknpu_obj->size; args->sram_size = rknpu_obj->sram_size; args->obj_addr = (__u64)(uintptr_t)rknpu_obj; args->dma_addr = rknpu_obj->dma_addr; + rknpu_gem_object_put(obj); + return 0; } @@ -927,12 +946,33 @@ int rknpu_gem_destroy_ioctl(struct drm_device *drm, void *data, struct rknpu_device *rknpu_dev = drm->dev_private; struct rknpu_gem_object *rknpu_obj = NULL; struct rknpu_mem_destroy *args = data; + struct drm_gem_object *obj = NULL; int ret = 0; int wait_count = 0; - rknpu_obj = rknpu_gem_object_find(file_priv, args->handle); - if (!rknpu_obj) + /* #patch37: hold a REAL reference for the whole ioctl. + * + * rknpu_gem_object_find() is lookup-then-put -- it takes a reference via + * drm_gem_object_lookup() and drops it again before returning, so the caller receives an + * UNREFERENCED pointer kept alive only by the handle table. (Its kerneldoc, which claims + * the refcount "would be increased", is wrong; that comment is how this was missed.) + * + * Survivable for a short ioctl. Not for this one: it then blocks in + * rknpu_iommu_domain_get_and_switch() for up to 6 s per attempt, three attempts -- ~18 s -- + * and dereferences rknpu_obj->iommu_domain_id inside the loop. If the owning process dies + * in that window (SIGTERM -> DRM file close -> every handle released -> objects freed) the + * pointer dangles and we walk freed memory. + * + * The window is wide open in practice, not theoretical: a stalled NPU job is precisely what + * makes the domain switch time out, and a stuck run is precisely when the process is killed. + * Suspected source of the "BUG: Bad page cache ... still mapped when deleted" corruption + * (rknpu maps GEM shmem pages with vm_insert_page() and clears VM_PFNMAP, and vma->vm_file + * stays the DRM node -- so the shmem inode's i_mmap tree cannot find those VMAs to unmap at + * evict, and the GEM refcount is the ONLY thing preventing a premature free). */ + obj = drm_gem_object_lookup(file_priv, args->handle); + if (!obj) return -EINVAL; + rknpu_obj = to_rknpu_obj(obj); do { ret = rknpu_iommu_domain_get_and_switch( @@ -941,12 +981,18 @@ int rknpu_gem_destroy_ioctl(struct drm_device *drm, void *data, if (ret && ++wait_count >= 3) { LOG_DEV_ERROR(rknpu_dev->dev, "failed to destroy memory\n"); + rknpu_gem_object_put(obj); return ret; } } while (ret); ret = rknpu_gem_handle_destroy(file_priv, args->handle); + /* Drop ours BEFORE the domain put: if this was the last reference, the object's teardown + * runs here with the domain still held -- the same ordering the original code had, where + * handle_destroy itself could free the object at exactly this point. */ + rknpu_gem_object_put(obj); + rknpu_iommu_domain_put(rknpu_dev); return ret; @@ -1422,6 +1468,7 @@ rknpu_gem_prime_import_sg_table(struct drm_device *dev, struct sg_table *sgt) { struct rknpu_gem_object *rknpu_obj = NULL; + struct rknpu_device *rknpu_dev = dev->dev_private; int npages = 0; int ret = -EINVAL; @@ -1432,6 +1479,9 @@ rknpu_gem_prime_import_sg_table(struct drm_device *dev, } rknpu_obj->dma_addr = sg_dma_address(sgt->sgl); + /* #patch: the sg above was mapped against the CURRENTLY attached domain. + * Record it, or destroy unmaps from domain 0 and strands this IOVA. */ + rknpu_obj->iommu_domain_id = rknpu_dev->iommu_domain_id; npages = rknpu_obj->size >> PAGE_SHIFT; rknpu_obj->pages = rknpu_gem_alloc_page(npages); diff --git a/drivers/rknpu/rknpu_iommu.c b/drivers/rknpu/rknpu_iommu.c index 3e05fd5e5..4f843191a 100644 --- a/drivers/rknpu/rknpu_iommu.c +++ b/drivers/rknpu/rknpu_iommu.c @@ -10,19 +10,128 @@ #include "rknpu_iommu.h" +/* #patch30 diagnostics */ +unsigned long rknpu_dbg_switch_inflight; +unsigned long rknpu_dbg_switch_idle; +int rknpu_dbg_switch_verbose; +module_param_named(dbg_switch_inflight, rknpu_dbg_switch_inflight, ulong, 0444); +module_param_named(dbg_switch_idle, rknpu_dbg_switch_idle, ulong, 0444); +module_param_named(dbg_switch_verbose, rknpu_dbg_switch_verbose, int, 0644); + #define RKNPU_SWITCH_DOMAIN_WAIT_TIME_MS 6000 +/* #patch59: what RATCHETS across runs? + * + * The mismatch wedge is cumulative — it appears on roughly the 4th run of a boot and clears only on + * reboot, so something is persistent kernel state. dbg_domain_underflow stays 0, so it is not the + * refcount going negative. The two remaining candidates are a leaked domain REFERENCE (refcount + * never returns to 0, so every switch has to wait) and a leaked MAPPING (IOVA space exhausted, so + * dma map eventually fails — which is the observed proximate cause, + * "rknpu_gem_get_pages: dma map fail"). + * + * Sample all three after each run: whichever monotonically climbs is the leak. refcount_now is a + * shadow of the atomic, updated wherever it is touched, so it can be read from sysfs. */ +unsigned long rknpu_dbg_refcnt_now; +module_param_named(dbg_refcnt_now, rknpu_dbg_refcnt_now, ulong, 0444); +unsigned long rknpu_dbg_map_n, rknpu_dbg_unmap_n; +module_param_named(dbg_map_n, rknpu_dbg_map_n, ulong, 0444); +module_param_named(dbg_unmap_n, rknpu_dbg_unmap_n, ulong, 0444); + + +/* #patch51: distinguish "this DOMAIN is bad" from "the first commit after a SWITCH is bad". + * Per-domain stall rate alone cannot separate them: a domain that is visited briefly has a much + * larger FRACTION of its commits sitting immediately after a switch, so it would look bad either + * way. Reset on every switch, incremented per commit; logged per commit so the two can be + * disentangled by controlling for it. */ +unsigned int rknpu_commits_since_switch; +/* #patch52: COUNT vs TIME. sinceswitch alone cannot tell "the first N commits are poisoned" from + * "there is a settling window of T microseconds", because the first jobs into a new domain arrive in + * a burst -- the two describe the same events. Stamp the switch so each commit can also record how + * long ago it happened; then the stall boundary can be checked against both axes independently. + * They imply different fixes: a count means prime/quiesce, a time means a cooldown. */ +ktime_t rknpu_last_switch_time; +/* #patch53 */ +/* #patch54: ESCALATION for the domain-switch livelock. + * + * get_and_switch() only proceeds when iommu_domain_refcount reads 0. A job that never completes + * pins that reference, so every switch then times out after RKNPU_SWITCH_DOMAIN_WAIT_TIME_MS (6 s) + * and returns -EINVAL -- and userspace simply retries, forever. Nothing breaks the loop: + * rknpu_job_timeout_clean() is only invoked when a NEW submit arrives, which is exactly what cannot + * happen while allocation is failing. + * + * Measured cost of having no bound: a run wedged the board hard enough to need a power cycle, with + * "switch iommu domain time out" repeating every 6 s and one job abort at 184 s. None of the kernel's + * own detectors can see it -- panic_on_oops needs a fault, softlockup needs a spinning CPU, and + * hung_task needs CONTINUOUS uninterruptible sleep whereas this loop wakes every 6-20 s. It is a + * livelock, invisible by construction. + * + * So bound it here: after N consecutive timeouts, reap the stuck jobs, which releases the references + * they hold. Reaping (not forcing the refcount to 0) is deliberate -- if a job really is still live, + * zeroing the count would let a switch happen underneath it and corrupt its translations. */ +static unsigned int rknpu_switch_escalate = 3; +module_param_named(switch_escalate, rknpu_switch_escalate, uint, 0644); +MODULE_PARM_DESC(switch_escalate, "consecutive domain-switch timeouts before reaping stuck jobs (0=off)"); +static unsigned int rknpu_switch_consec; +unsigned long rknpu_switch_escalations; +module_param_named(switch_escalations, rknpu_switch_escalations, ulong, 0444); +void rknpu_reap_all_cores(struct rknpu_device *rknpu_dev); + +unsigned int rknpu_dom_cooldown_us; +module_param_named(dom_cooldown_us, rknpu_dom_cooldown_us, uint, 0644); +MODULE_PARM_DESC(dom_cooldown_us, "settle this many us after an iommu domain switch (0=off)"); +unsigned long rknpu_dom_cooldowns; +module_param_named(dom_cooldowns, rknpu_dom_cooldowns, ulong, 0444); + +/* #patch33: never dereference a NULL iommu domain. + * + * rknpu_iommu_switch_domain() does detach(src) then attach(dst); if the attach fails it tries to + * re-attach src, and if THAT fails too the device is left attached to NO domain. The switch does + * fail in practice: a stalled job cannot be drained, so the switch times out + * ("switch iommu domain time out") and the abort path runs. The next GEM allocation then called + * iommu_get_domain_for_dev() and dereferenced the NULL result one line later: + * + * Unable to handle kernel NULL pointer dereference at virtual address 0000000000000040 + * pc : rknpu_iommu_dma_map_sg+0x48 Comm: i4_widek_stall_ + * rknpu_gem_object_create / rknpu_gem_create_ioctl + * + * which kills the board (this is a strong candidate for the long-standing "NPU wedge needs a + * power cycle"). Turn an unrecoverable panic into an ordinary error return. */ +static struct rknpu_iommu_dma_cookie *rknpu_iommu_cookie(struct device *dev, + const char *who) +{ + struct iommu_domain *domain = iommu_get_domain_for_dev(dev); + + if (!domain) { + dev_err_ratelimited(dev, "%s: NPU attached to NO iommu domain (a domain switch left it detached)\n", + who); + return NULL; + } + if (!domain->iova_cookie) { + dev_err_ratelimited(dev, "%s: iommu domain has no iova cookie\n", who); + return NULL; + } + return (struct rknpu_iommu_dma_cookie *)domain->iova_cookie; +} + + dma_addr_t rknpu_iommu_dma_alloc_iova(struct iommu_domain *domain, size_t size, u64 dma_limit, struct device *dev, bool size_aligned) { - struct rknpu_iommu_dma_cookie *cookie = (void *)domain->iova_cookie; - struct iova_domain *iovad = &cookie->iovad; + struct rknpu_iommu_dma_cookie *cookie; + struct iova_domain *iovad; unsigned long shift, iova_len, iova = 0; unsigned long limit_pfn; struct iova *new_iova = NULL; bool alloc_fast = size_aligned; + if (!domain || !domain->iova_cookie) { /* #patch34 */ + dev_err_ratelimited(dev, "%s: no iommu domain/cookie\n", __func__); + return 0; + } + cookie = (struct rknpu_iommu_dma_cookie *)domain->iova_cookie; + iovad = &cookie->iovad; + shift = iova_shift(iovad); iova_len = size >> shift; @@ -209,8 +318,9 @@ int rknpu_iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, bool iova_aligned) { struct iommu_domain *domain = iommu_get_domain_for_dev(dev); - struct rknpu_iommu_dma_cookie *cookie = (void *)domain->iova_cookie; - struct iova_domain *iovad = &cookie->iovad; + struct rknpu_iommu_dma_cookie *cookie = + rknpu_iommu_cookie(dev, __func__); /* #patch33 */ + struct iova_domain *iovad; struct scatterlist *s = NULL, *prev = NULL; int prot = rknpu_dma_info_to_prot(dir, dev_is_dma_coherent(dev)); dma_addr_t iova; @@ -219,6 +329,11 @@ int rknpu_iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, ssize_t ret = -EINVAL; int i = 0; + if (!cookie) + return 0; /* #patch33: 0 == mapping failed, per this function's convention */ + rknpu_dbg_map_n++; /* #patch59 */ + iovad = &cookie->iovad; + if (iova_aligned) return dma_map_sg(dev, sg, nents, dir); @@ -299,8 +414,9 @@ void rknpu_iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, bool iova_aligned) { struct iommu_domain *domain = iommu_get_domain_for_dev(dev); - struct rknpu_iommu_dma_cookie *cookie = (void *)domain->iova_cookie; - struct iova_domain *iovad = &cookie->iovad; + struct rknpu_iommu_dma_cookie *cookie = + rknpu_iommu_cookie(dev, __func__); /* #patch34 */ + struct iova_domain *iovad; size_t iova_off = 0; dma_addr_t end = 0, start = 0; struct scatterlist *tmp = NULL; @@ -308,6 +424,11 @@ void rknpu_iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, size_t size = 0; int i = 0; + if (!cookie) + return; /* #patch34: nothing mapped through a domain that is gone */ + rknpu_dbg_unmap_n++; /* #patch59 */ + iovad = &cookie->iovad; + if (iova_aligned) return dma_unmap_sg(dev, sg, nents, dir); @@ -443,6 +564,31 @@ int rknpu_iommu_switch_domain(struct rknpu_device *rknpu_dev, int domain_id) return 0; } + /* #patch30 (diagnostic): a domain switch calls iommu_attach_device(), and for + * rockchip-iommu that runs rk_iommu_enable() -> enable_stall + FORCE_RESET of every + * MMU bank + ZAP_CACHE + paging off/on. If an NPU job is still in flight, its + * descriptor/regcmd fetches are cut mid-dispatch: the job is committed but the PC + * completed-task counter never advances, and nothing faults. Count switches that + * race in-flight work so it can be correlated with the observed dispatch stalls. */ + { + int _i, _busy = 0; + unsigned long _f; + spin_lock_irqsave(&rknpu_dev->irq_lock, _f); + for (_i = 0; _i < rknpu_dev->config->num_irqs; _i++) + if (rknpu_dev->subcore_datas[_i].job) + _busy++; + spin_unlock_irqrestore(&rknpu_dev->irq_lock, _f); + if (_busy) { + rknpu_dbg_switch_inflight++; + if (rknpu_dbg_switch_verbose) + LOG_WARN("switch domain %d->%d with %d job(s) IN FLIGHT (n=%lu)\n", + src_domain_id, domain_id, _busy, + rknpu_dbg_switch_inflight); + } else { + rknpu_dbg_switch_idle++; + } + } + src_domain = iommu_get_domain_for_dev(rknpu_dev->dev); if (src_domain != rknpu_dev->iommu_domains[src_domain_id]) { LOG_DEV_ERROR( @@ -461,10 +607,32 @@ int rknpu_iommu_switch_domain(struct rknpu_device *rknpu_dev, int domain_id) "failed to attach dst iommu domain, id: %d, ret: %d\n", domain_id, ret); if (iommu_attach_device(src_domain, rknpu_dev->dev)) { + int _d; + LOG_DEV_ERROR( rknpu_dev->dev, "failed to reattach src iommu domain, id: %d\n", src_domain_id); + /* #patch34: detach(src) already happened, so the device is now + * attached to NO domain. Every later map/unmap dereferences + * iommu_get_domain_for_dev() == NULL and panics the box, seconds + * later and far from here. Attach ANYTHING still valid rather + * than return with the device unattached. */ + for (_d = 0; _d < RKNPU_MAX_IOMMU_DOMAIN_NUM; _d++) { + if (!rknpu_dev->iommu_domains[_d]) + continue; + if (!iommu_attach_device( + rknpu_dev->iommu_domains[_d], + rknpu_dev->dev)) { + rknpu_dev->iommu_domain_id = _d; + rknpu_dev->iommu_group->default_domain = + rknpu_dev->iommu_domains[_d]; + LOG_DEV_ERROR(rknpu_dev->dev, + "recovered onto iommu domain %d\n", + _d); + break; + } + } } return ret; } @@ -505,6 +673,33 @@ int rknpu_iommu_switch_domain(struct rknpu_device *rknpu_dev, int domain_id) // reset default iommu domain rknpu_dev->iommu_group->default_domain = dst_domain; + /* #patch53: COOLDOWN after a domain switch. + * + * Measured, replicated across two unrelated code paths (blocking colsplit on a 1.5B model, + * and the async int4 doorbell probe): EVERY observed stall was a commit landing within + * ~1 ms of an IOMMU domain switch, and there were ZERO stalls beyond that window across + * ~10,000 commits. Being early by COUNT is not the predictor -- 9004 commits at position + * 0-2 produced 5 stalls, all of them in the sub-1 ms subset while 8993 early-but->=1ms + * commits were clean. Time is the operative variable. + * + * Sleeping here is legal and correct: rknpu_iommu_switch_domain() is only ever reached from + * process context (gem create/destroy/sync ioctls and the submit path), never from the + * completion IRQ. That is why this can usleep_range() where the earlier per-commit spacing + * experiment was forced into udelay() -- and it is charged once per SWITCH, not once per + * commit, so the cost is ~1 ms x the switch count (~56 in a 1.5B run) rather than per job. + * + * NOT a claim about mechanism: the window is necessary but not sufficient (inside it the + * rate ranges 0.2%-90.9% by workload), so something else modulates it. This removes the + * necessary condition, which is enough to eliminate every stall observed so far. + * + * 0 = off (default, opt-in until measured). */ + if (rknpu_dom_cooldown_us) { + usleep_range(rknpu_dom_cooldown_us, rknpu_dom_cooldown_us + 100); + rknpu_dom_cooldowns++; + } + + rknpu_commits_since_switch = 0; /* #patch51 */ + rknpu_last_switch_time = ktime_get(); /* #patch52 -- stamp AFTER the cooldown */ LOG_INFO("switch iommu domain from %d to %d\n", src_domain_id, domain_id); @@ -523,7 +718,9 @@ int rknpu_iommu_domain_get_and_switch(struct rknpu_device *rknpu_dev, mutex_lock(&rknpu_dev->domain_lock); if (domain_id == rknpu_dev->iommu_domain_id) { + rknpu_switch_consec = 0; /* #patch54 */ atomic_inc(&rknpu_dev->iommu_domain_refcount); + rknpu_dbg_refcnt_now = atomic_read(&rknpu_dev->iommu_domain_refcount); /* #patch59 */ mutex_unlock(&rknpu_dev->domain_lock); break; } @@ -538,7 +735,9 @@ int rknpu_iommu_domain_get_and_switch(struct rknpu_device *rknpu_dev, mutex_unlock(&rknpu_dev->domain_lock); return ret; } + rknpu_switch_consec = 0; /* #patch54 */ atomic_inc(&rknpu_dev->iommu_domain_refcount); + rknpu_dbg_refcnt_now = atomic_read(&rknpu_dev->iommu_domain_refcount); /* #patch59 */ mutex_unlock(&rknpu_dev->domain_lock); break; } @@ -551,6 +750,16 @@ int rknpu_iommu_domain_get_and_switch(struct rknpu_device *rknpu_dev, rknpu_dev->dev, "switch iommu domain time out, failed to switch iommu domain, id: %d\n", domain_id); + /* #patch54: break the livelock rather than returning into an infinite retry */ + if (rknpu_switch_escalate && + ++rknpu_switch_consec >= rknpu_switch_escalate) { + rknpu_switch_consec = 0; + rknpu_switch_escalations++; + LOG_DEV_ERROR(rknpu_dev->dev, + "ESCALATING after %u consecutive switch timeouts: reaping stuck jobs to release the domain reference\n", + rknpu_switch_escalate); + rknpu_reap_all_cores(rknpu_dev); + } return -EINVAL; } } @@ -558,9 +767,139 @@ int rknpu_iommu_domain_get_and_switch(struct rknpu_device *rknpu_dev, return 0; } +/* #patch45 diagnostics: the live refcount, and how often a put tried to take it negative. */ +unsigned long rknpu_dbg_premature_zero; +module_param_named(dbg_premature_zero, rknpu_dbg_premature_zero, ulong, 0444); +unsigned long rknpu_dbg_domain_underflow; +module_param_named(dbg_domain_underflow, rknpu_dbg_domain_underflow, ulong, 0444); + int rknpu_iommu_domain_put(struct rknpu_device *rknpu_dev) { - atomic_dec(&rknpu_dev->iommu_domain_refcount); + /* #patch45: NEVER let the refcount go negative. + * + * rknpu_iommu_domain_get_and_switch() proceeds only when this reads exactly 0 + * (`if (atomic_read(&iommu_domain_refcount) == 0)`). A bare atomic_dec() therefore turns a + * single unbalanced put into a PERMANENT wedge: the count never reads 0 again, every domain + * switch burns its 6 s RKNPU_SWITCH_DOMAIN_WAIT_TIME_MS and fails, and because + * rknpu_gem_object_create() switches domains, EVERY allocation then fails with -EINVAL + * ("rknpu_gem_object_create error") until reboot. + * + * Unbalanced puts are a live risk here, not a hypothetical: our own kernel change 02 ADDED a + * put to rknpu_job_timeout_clean() to reclaim a leaked reference, which double-puts on any + * path that already released it. That is precisely the wedge seen repeatedly on this board + * (59 'mismatch domain'/'switch iommu domain time out' lines in a single 300 s run), and it + * invalidated several measurements before it was understood. + * + * Clamping is strictly better than wedging: an over-put becomes a counted anomaly instead of + * a dead NPU. The clamp is racy against a concurrent get, but the alternative it replaces is + * an unrecoverable device, so the trade is not close. dbg_domain_underflow != 0 means a real + * refcount bug remains to be found -- the clamp treats the symptom, not the cause. */ + /* #patch60: NAME the over-putting caller. + * + * Six runs showed dbg_domain_underflow reaching 5 — excess puts — but the counter alone + * cannot say WHICH of the ~10 call sites is unbalanced, and change 02 (our added put in + * rknpu_job_timeout_clean) is only a suspect, not a finding. %pS resolves the return address + * to a symbol, so the offending site names itself. + * + * Two conditions are reported, and the SECOND is the one that actually causes damage: + * - underflow: the count went below zero. Loud, but already too late. + * - premature zero: the count hit exactly 0 while jobs are still IN FLIGHT. No underflow, + * no warning, but rknpu_iommu_domain_get_and_switch() tests only `refcount == 0`, so it + * will now switch domains out from under live work — which is the mismatch-domain + * signature. These are invisible to the underflow counter and must be more numerous. */ + { + void *caller = __builtin_return_address(0); + int after; + + rknpu_dbg_refcnt_now = atomic_read(&rknpu_dev->iommu_domain_refcount) - 1; + after = atomic_dec_return(&rknpu_dev->iommu_domain_refcount); + if (after < 0) { + atomic_set(&rknpu_dev->iommu_domain_refcount, 0); + rknpu_dbg_refcnt_now = 0; + rknpu_dbg_domain_underflow++; + LOG_DEV_ERROR(rknpu_dev->dev, + "domain_put UNDERFLOW (clamped) from %pS\n", caller); + } else if (after == 0) { + unsigned long f; + int k, busy = 0; + + spin_lock_irqsave(&rknpu_dev->irq_lock, f); + for (k = 0; k < rknpu_dev->config->num_irqs; k++) + if (rknpu_dev->subcore_datas[k].job) + busy++; + spin_unlock_irqrestore(&rknpu_dev->irq_lock, f); + if (busy) { + rknpu_dbg_premature_zero++; + LOG_DEV_ERROR(rknpu_dev->dev, + "domain refcount hit 0 with %d job(s) IN FLIGHT — put from %pS\n", + busy, caller); + } + } + } + + return 0; +} + +/* #patch61: explicit state reset — the software equivalent of the reboot. + * + * Refuses while any core has a job, because freeing a domain out from under live work would strand + * its translations. Beyond freeing the domains it also zeroes the refcount and the diagnostic + * counters, so a fresh run starts from a known state rather than inheriting a skewed one. Note the + * caller must treat every existing buffer AND every regcmd it has built as invalid afterwards: the + * IOVAs change, and userspace bakes IOVAs into its programs. */ +int rknpu_iommu_reinit(struct rknpu_device *rknpu_dev) +{ + unsigned long flags; + int i, busy = 0; + + spin_lock_irqsave(&rknpu_dev->irq_lock, flags); + for (i = 0; i < rknpu_dev->config->num_irqs; i++) + if (rknpu_dev->subcore_datas[i].job) + busy++; + spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); + /* #patch62: ABORT in-flight work rather than refusing. + * + * The first cut returned -EBUSY whenever any core had a job. That made the call useless for + * its entire purpose: a device in the bad state almost always HAS a stuck job — that is what + * being stuck means — so REINIT refused with -EBUSY at exactly the moment it was needed + * (measured: idle device -> ok; wedged device -> -EBUSY). The guard was protecting the caller + * from the case the caller was trying to fix. + * + * So reap first, then re-check. rknpu_reap_all_cores() runs the driver's own timeout_clean + * path, which aborts the jobs and releases the references they hold. If a core STILL reports a + * job after that, something is wedged beyond what this call can safely unpick, and -EBUSY is + * then a truthful answer rather than an unhelpful one. */ + if (busy) { + LOG_INFO("RKNPU: REINIT — %d job(s) in flight, reaping before teardown\n", busy); + rknpu_reap_all_cores(rknpu_dev); + + busy = 0; + spin_lock_irqsave(&rknpu_dev->irq_lock, flags); + for (i = 0; i < rknpu_dev->config->num_irqs; i++) + if (rknpu_dev->subcore_datas[i].job) + busy++; + spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); + + if (busy) { + LOG_DEV_ERROR(rknpu_dev->dev, + "REINIT: %d job(s) survived the reap — refusing\n", + busy); + return -EBUSY; + } + } + + LOG_INFO("RKNPU: REINIT — freeing all iommu domains and resetting state (was: refcnt=%d underflow=%lu premature_zero=%lu)\n", + atomic_read(&rknpu_dev->iommu_domain_refcount), + rknpu_dbg_domain_underflow, rknpu_dbg_premature_zero); + + rknpu_iommu_free_domains(rknpu_dev); + atomic_set(&rknpu_dev->iommu_domain_refcount, 0); + rknpu_dbg_refcnt_now = 0; + rknpu_dbg_domain_underflow = 0; + rknpu_dbg_premature_zero = 0; + rknpu_dbg_map_n = 0; + rknpu_dbg_unmap_n = 0; + rknpu_switch_consec = 0; return 0; } diff --git a/drivers/rknpu/rknpu_job.c b/drivers/rknpu/rknpu_job.c index 57d67d81f..96f46fadf 100644 --- a/drivers/rknpu/rknpu_job.c +++ b/drivers/rknpu/rknpu_job.c @@ -1,3 +1,4 @@ +/* #patch40: the wd_kick auto-recovery was deleted -- see the note in rknpu_drv.c. */ // SPDX-License-Identifier: GPL-2.0 /* * Copyright (C) Rockchip Electronics Co., Ltd. @@ -15,9 +16,54 @@ #include "rknpu_gem.h" #include "rknpu_fence.h" #include "rknpu_mem.h" +#include #include "rknpu_iommu.h" #include "rknpu_job.h" +/* #patch47 */ +ktime_t rknpu_prof_last_commit[3]; +/* #patch48 */ +extern unsigned long rknpu_prof_ok_n, rknpu_prof_ok_gap, rknpu_prof_ok_queue; +/* #patch49 */ +extern unsigned long rknpu_gap_enforced_n, rknpu_gap_enforced_us; +unsigned int rknpu_min_commit_gap_get(void); +unsigned int rknpu_prof_mask_get(void); /* #patch50 */ +extern unsigned int rknpu_commits_since_switch; /* #patch51 */ +extern ktime_t rknpu_last_switch_time; /* #patch52 */ + + + +/* #patch (experiment): does the QUEUE stall behind a lost completion interrupt? + * rknpu_job_next() only runs from the completion path, so a lost IRQ leaves subcore_data->job + * set and the next submit sits on todo_list, never committed -> PC never starts -> the + * "accepted but never dispatched" doorbell miss. A deficit of completions vs commits proves it. + * commit = rknpu_job_subcore_commit() calls (jobs actually handed to the PC) + * irq = rknpu_irq_handler() entries (interrupts that reached the handler) + * nojob = handler found no job (spurious / already-reaped) + * unpow = handler declined, block unpowered (Change 6 guard) + * done = rknpu_job_done() final completion (interrupt_count reached 0) + * Healthy steady state: commit == done, and irq >= commit. */ +static unsigned int rknpu_dbg_healthy_n; /* #patch: matched-control samples logged */ +/* #patch: how often rknpu_job_timeout_clean SOFT-RESETS the NPU and drops the RUNNING job. + * It compares ktime_us_delta(now, job->timestamp) >= args->timeout -- MICROseconds against a value every + * other site treats as MILLIseconds -- so our 1500 ("ms") is really a 1.5 ms threshold while our jobs run + * a median 5.7 ms. Any job still executing when the next submit lands on its core is therefore older than + * the threshold, gets the whole NPU reset under it, and is dropped: committed, never completed, no + * interrupt. That predicts commit-done == stall count, which is what we measure. + * treap = timeout_clean reset+dropped a job + * treap_age = age (us) of the job it killed, last occurrence */ +static unsigned long rknpu_cnt_treap; +static unsigned long rknpu_cnt_treap_age; +module_param_named(cnt_treap, rknpu_cnt_treap, ulong, 0444); +module_param_named(cnt_treap_age, rknpu_cnt_treap_age, ulong, 0444); +static unsigned long rknpu_cnt_commit, rknpu_cnt_irq, rknpu_cnt_nojob, + rknpu_cnt_unpow, rknpu_cnt_done; +module_param_named(cnt_commit, rknpu_cnt_commit, ulong, 0444); +module_param_named(cnt_irq, rknpu_cnt_irq, ulong, 0444); +module_param_named(cnt_nojob, rknpu_cnt_nojob, ulong, 0444); +module_param_named(cnt_unpow, rknpu_cnt_unpow, ulong, 0444); +module_param_named(cnt_done, rknpu_cnt_done, ulong, 0444); + #define _REG_READ(base, offset) readl(base + (offset)) #define _REG_WRITE(base, value, offset) writel(value, base + (offset)) @@ -101,8 +147,20 @@ static void rknpu_job_free(struct rknpu_job *job) rknpu_gem_object_put(&task_obj->base); #endif - if (job->fence) + if (job->fence) { + /* #patch: a job torn down WITHOUT completing (rknpu_job_timeout_clean / + * rknpu_job_abort) never reaches the RKNPU_JOB_DONE path, so its fence is + * never signalled and every waiter blocks until its OWN timeout expires -- + * which defeats the point of waiting on a fence to detect a stalled job. + * Signal it with an error instead, so a waiter wakes immediately and can + * tell a stall from a completion via dma_fence_get_status(). No-op on the + * success path, where the completion IRQ already signalled it. */ + if (!dma_fence_is_signaled(job->fence)) { + dma_fence_set_error(job->fence, -ETIMEDOUT); + dma_fence_signal(job->fence); + } dma_fence_put(job->fence); + } if (job->args_owner) kfree(job->args); @@ -192,10 +250,21 @@ static inline int rknpu_job_wait(struct rknpu_job *job) do { ret = wait_event_timeout(subcore_data->job_done_wq, - job->flags & RKNPU_JOB_DONE || + job->flags & (RKNPU_JOB_DONE | + RKNPU_JOB_STALLED) || rknpu_dev->soft_reseting, msecs_to_jiffies(args->timeout)); + /* #patch41 (fast-abort): the watchdog woke us because this job has made no + * progress. Force ret=0 and leave the retry loop so we fall into the SAME + * `if (ret <= 0)` path a natural timeout takes -- it samples the PC counter, + * logs, and returns -ETIMEDOUT. Identical outcome to the old behaviour, just + * without waiting args->timeout x 3 for it (measured: 60.6 s). */ + if (job->flags & RKNPU_JOB_STALLED) { + ret = 0; + break; + } + if (++wait_count >= 3) break; @@ -294,6 +363,8 @@ static inline int rknpu_job_subcore_commit_pc(struct rknpu_job *job, int pc_task_number_bits = rknpu_dev->config->pc_task_number_bits; int i = 0; int submit_index = atomic_read(&job->submit_count[core_index]); + + rknpu_cnt_commit++; /* #patch */ int max_submit_number = rknpu_dev->config->max_submit_number; unsigned long flags; @@ -368,9 +439,42 @@ static inline int rknpu_job_subcore_commit_pc(struct rknpu_job *job, job->last_task = last_task; job->int_mask[core_index] = last_task->int_mask; + /* #patch: snapshot what we WROTE vs what reads BACK, before and after the start pulse. + * Distinguishes "the write never landed" (readback zero/garbage -> ordering, barrier or + * gated clocks) from "it landed and the PC ignored it" (hardware state machine). */ + job->dbg_wr[0] = first_task->regcmd_addr; + job->dbg_wr[1] = (first_task->regcfg_amount + RKNPU_PC_DATA_EXTRA_AMOUNT + + pc_data_amount_scale - 1) / pc_data_amount_scale - 1; + job->dbg_wr[2] = last_task->int_mask; + job->dbg_wr[3] = ((0x6 | task_pp_en) << pc_task_number_bits) | task_number; + job->dbg_wr[4] = (uint32_t)args->task_base_addr; + job->dbg_wr[5] = task_number; + /* #patch: the sequence userspace stamped into this descriptor (rknpu_task.op_idx, a field the + * driver otherwise ignores). If a STALLED commit reports an OLDER stamp than the ones healthy + * commits are reporting, the kernel read a STALE task array through its kernel mapping. */ + job->dbg_seq = first_task->op_idx; + job->dbg_rd[0] = REG_READ(RKNPU_OFFSET_PC_DATA_ADDR); + job->dbg_rd[1] = REG_READ(RKNPU_OFFSET_PC_DATA_AMOUNT); + job->dbg_rd[2] = REG_READ(RKNPU_OFFSET_INT_MASK); + job->dbg_rd[3] = REG_READ(RKNPU_OFFSET_PC_TASK_CONTROL); + job->dbg_rd[4] = REG_READ(RKNPU_OFFSET_PC_DMA_BASE_ADDR); + job->dbg_rd[5] = REG_READ(rknpu_dev->config->pc_task_status_offset); + REG_WRITE(0x1, RKNPU_OFFSET_PC_OP_EN); REG_WRITE(0x0, RKNPU_OFFSET_PC_OP_EN); + job->dbg_rd[6] = REG_READ(rknpu_dev->config->pc_task_status_offset); + /* #patch: full PC/INT block AFTER the start pulse. Map (vendor rknpu_ioctl.h): + * 0x00 VERSION 0x04 VERSION_NUM 0x08 PC_OP_EN 0x10 PC_DATA_ADDR + * 0x14 PC_DATA_AMT 0x20 INT_MASK 0x24 INT_CLEAR 0x28 INT_STATUS + * 0x2c INT_RAW 0x30 PC_TASK_CTRL 0x34 PC_DMA_BASE 0x3c PC_TASK_STATUS + * plus ENABLE_MASK at 0xf008. Everything we WRITE is already verified identical between a + * healthy and a stalled commit; this samples the registers we do NOT write, to see whether the + * PC's own state differs. If nothing differs, the fault is not visible at this level. */ + { int _i; for (_i = 0; _i < 16; _i++) job->dbg_blk[_i] = REG_READ(_i * 4); + job->dbg_blk[16] = REG_READ(RKNPU_OFFSET_ENABLE_MASK); } + job->dbg_valid = true; + return 0; } @@ -426,6 +530,7 @@ static void rknpu_job_commit(struct rknpu_job *job) } } + static void rknpu_job_next(struct rknpu_device *rknpu_dev, int core_index) { struct rknpu_job *job = NULL; @@ -450,9 +555,52 @@ static void rknpu_job_next(struct rknpu_device *rknpu_dev, int core_index) list_del_init(&job->head[core_index]); subcore_data->job = job; job->hw_commit_time = ktime_get(); + /* #patch47 (profiler): record the gap since the PREVIOUS commit on this core, so a stalled + * job's timeline can be compared against a healthy one's. Two questions this answers: + * does a stall correlate with back-to-back commits (too little settle between jobs), or + * with a long queue delay (submit -> commit)? Both are free here -- job->timestamp and + * hw_commit_time already exist; only the per-core previous-commit time is new. */ + job->prof_gap_us = rknpu_prof_last_commit[core_index] ? + ktime_us_delta(job->hw_commit_time, + rknpu_prof_last_commit[core_index]) : 0; + job->prof_queue_us = ktime_us_delta(job->hw_commit_time, job->timestamp); + rknpu_prof_last_commit[core_index] = job->hw_commit_time; job->hw_recoder_time = job->hw_commit_time; spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); + /* #patch50: shape axis. The timing hypothesis failed its causal test (enforcing spacing + * gave no dose-response and inverted at 1500us), so the working theory is that a short gap + * is a SYMPTOM of which op is running. Log the shape of every commit so the stalled and + * healthy shape distributions can be compared directly. */ + job->prof_since_switch = rknpu_commits_since_switch++; /* #patch51 */ + job->prof_tsince_us = rknpu_last_switch_time ? /* #patch52 */ + ktime_us_delta(job->hw_commit_time, rknpu_last_switch_time) : -1; + if (rknpu_prof_mask_get() & 2) + LOG_ERROR("RKNPU: PROF commit core=%d tn=%u cm=%#x dom=%d sinceswitch=%u tsince=%lldus gap=%lldus\n", + core_index, job->args->task_number, + job->args->core_mask, job->iommu_domain_id, + job->prof_since_switch, job->prof_tsince_us, job->prof_gap_us); + + /* #patch49: enforce a minimum spacing between commits on this core. + * + * Placed here so the enforcement matches where #patch47 MEASURES the gap -- same path, same + * core, apples to apples. udelay() not usleep_range(): this can be reached from the + * completion IRQ. Clamped, and skipped entirely when the knob is 0. */ + { + unsigned int minus = rknpu_min_commit_gap_get(); + + if (minus && job->prof_gap_us >= 0 && + job->prof_gap_us < (s64)minus) { + unsigned int need = minus - (unsigned int)job->prof_gap_us; + + if (need > 5000) + need = 5000; + udelay(need); + rknpu_gap_enforced_n++; + rknpu_gap_enforced_us += need; + } + } + if (atomic_dec_and_test(&job->run_count)) rknpu_job_commit(job); } @@ -485,9 +633,48 @@ static void rknpu_job_done(struct rknpu_job *job, int ret, int core_index) if (atomic_dec_and_test(&job->interrupt_count)) { int use_core_num = job->use_core_num; + rknpu_cnt_done++; /* #patch */ + if (job->dbg_kick_t) + LOG_ERROR("RKNPU: KICKED JOB COMPLETED %lldus after the kick\n", + ktime_us_delta(ktime_get(), job->dbg_kick_t)); + + /* #patch (experiment control): dump a SUCCESSFUL job's commit snapshot every 2000th + * completion, so the stalled-job dump has a same-run baseline to compare against. + * Without this the stalled readback is uninterpretable — we cannot tell an abnormal + * value from simply how the register reads back. */ + /* MATCHED control: only sample successful jobs with the SAME signature as the stalled + * ones (task_ctrl 0x7002 = ping-pong on, 2 tasks). An unmatched control compared a + * 1-task pp-off job against a 2-task pp-on stall and was uninterpretable. */ + /* Log the FIRST few matching successes, never a modulo. Sampling every 500th + * completion aliased: the workload repeats a 4-job cycle (one real 0x7002 op plus + * three 0x6001 drain dummies) and 500 %% 4 == 0, so the sampler locked to one phase + * and captured ZERO matching jobs. */ + if (job->dbg_valid && job->dbg_wr[3] == 0x7002 && rknpu_dbg_healthy_n < 3 && + ++rknpu_dbg_healthy_n) { + LOG_ERROR("RKNPU: HEALTHY commit snapshot: WROTE data_addr=%#x amount=%#x int_mask=%#x task_ctrl=%#x dma_base=%#x tasks=%u | READBACK data_addr=%#x amount=%#x int_mask=%#x task_ctrl=%#x dma_base=%#x status(pre)=%#x status(post)=%#x\n", + job->dbg_wr[0], job->dbg_wr[1], job->dbg_wr[2], + job->dbg_wr[3], job->dbg_wr[4], job->dbg_wr[5], + job->dbg_rd[0], job->dbg_rd[1], job->dbg_rd[2], + job->dbg_rd[3], job->dbg_rd[4], job->dbg_rd[5], + job->dbg_rd[6]); + LOG_ERROR("RKNPU: HEALTHY PCBLK %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x | en=%08x\n", + job->dbg_blk[0], job->dbg_blk[1], job->dbg_blk[2], job->dbg_blk[3], + job->dbg_blk[4], job->dbg_blk[5], job->dbg_blk[6], job->dbg_blk[7], + job->dbg_blk[8], job->dbg_blk[9], job->dbg_blk[10], job->dbg_blk[11], + job->dbg_blk[12], job->dbg_blk[13], job->dbg_blk[14], job->dbg_blk[15], + job->dbg_blk[16]); + } + rknpu_iommu_domain_put(rknpu_dev); job->flags |= RKNPU_JOB_DONE; + /* #patch48: healthy baseline for the profiler — same two quantities the stall path + * records, so the comparison is like-for-like. */ + if (!(job->flags & RKNPU_JOB_STALLED)) { + rknpu_prof_ok_n++; + rknpu_prof_ok_gap += (unsigned long)job->prof_gap_us; + rknpu_prof_ok_queue += (unsigned long)job->prof_queue_us; + } job->ret = ret; if (job->fence) @@ -573,10 +760,26 @@ static void rknpu_job_abort(struct rknpu_job *job) for (i = 0; i < rknpu_dev->config->num_irqs; i++) { if (job->args->core_mask & rknpu_core_mask(i)) { subcore_data = &rknpu_dev->subcore_datas[i]; - if (job == subcore_data->job && !job->irq_entry[i]) { + if (job == subcore_data->job) { + /* #patch35: clear UNCONDITIONALLY. + * + * Upstream gates this on !job->irq_entry[i], but rknpu_job_cleanup() + * below frees the job either way -- so whenever irq_entry[i] is set + * this left a DANGLING pointer in subcore_data->job. The IRQ handler, + * rknpu_job_next() and the progress watchdog all read that field, so + * the next reader touches freed slab. The resulting corruption panics + * somewhere unrelated seconds later (observed: + * __memcg_kmem_charge_page via fork from dropbear, and + * sg_free_table/__free_pages), which is why it never looked like an + * NPU bug. Reached whenever an aborted job is stuck -- e.g. after + * "switch iommu domain time out". + * + * Keep the task_num adjustment gated as before so the accounting is + * unchanged; only the freed-pointer publication is fixed. */ + if (!job->irq_entry[i]) + subcore_data->task_num -= + rknpu_get_task_number(job, i); subcore_data->job = NULL; - subcore_data->task_num -= - rknpu_get_task_number(job, i); } } } @@ -601,7 +804,20 @@ static void rknpu_job_abort(struct rknpu_job *job) job->timestamp)); } } - rknpu_soft_reset(rknpu_dev); + /* #patch42: a FAST-ABORTed job (#patch41) also lands here with -ETIMEDOUT, but a + * device-wide rknpu_soft_reset() is the wrong hammer for it. Fast-abort fires + * ~30x more often than the natural 60 s timeout it replaces, and each device-wide + * reset disrupts healthy jobs on the OTHER cores. Measured: turning fast-abort on + * cut submit failures 14 -> 2 but pushed the worst in-kernel wait 61.5 s -> 184.3 s, + * because the extra resets stalled everyone else. Reset only the cores this job + * actually owns; the device-wide path is kept for genuine timeouts. */ + if (job->flags & RKNPU_JOB_STALLED) { + for (i = 0; i < rknpu_dev->config->num_irqs; i++) + if (job->args->core_mask & rknpu_core_mask(i)) + rknpu_soft_reset_core(rknpu_dev, i); + } else { + rknpu_soft_reset(rknpu_dev); + } } else { LOG_ERROR( "job abort, flags: %#x, ret: %d, elapsed time: %lldus\n", @@ -646,11 +862,38 @@ static inline irqreturn_t rknpu_irq_handler(int irq, void *data, int core_index) uint32_t status = 0; unsigned long flags; + /* #patch: NEVER touch NPU MMIO when the block is powered down. + * + * Both paths below access registers unconditionally -- the no-job path does + * REG_WRITE(RKNPU_INT_CLEAR), and the normal path does REG_READ(INT_STATUS). + * rknpu_power_off() is driven by a DEFERRED work item (rknpu_power_off_delay_work), + * so a late or spurious interrupt can arrive after power has gone. The register read + * then takes an external abort and the machine dies instantly -- no console output, + * no ping, requiring a power cycle. + * + * CAPTURED VIA NETCONSOLE 2026-08-26 (this is what that panic looked like): + * pc : readl+0x4/0x20 + * lr : rknpu_irq_handler.isra.0+0x94/0x2f0 + * Call trace: readl / rknpu_core0_irq_handler / __handle_irq_event_percpu + * ... el1_interrupt / cpuidle_enter / do_idle + * CPU 0 was IDLE -- i.e. the NPU had finished and powered down, and the IRQ landed after. + * + * power_refcount is an atomic, so unlike power_lock (a mutex) it is safe to read from + * hard-IRQ context. The check is racy in principle but closes the real window, which is + * an interrupt arriving well after power-off. If we are unpowered the NPU cannot be + * asserting anything, so IRQ_NONE is correct and cannot cause a level-IRQ storm. */ + if (atomic_read(&rknpu_dev->power_refcount) <= 0) { + rknpu_cnt_unpow++; /* #patch */ + return IRQ_NONE; + } + rknpu_cnt_irq++; /* #patch */ + subcore_data = &rknpu_dev->subcore_datas[core_index]; spin_lock_irqsave(&rknpu_dev->irq_lock, flags); job = subcore_data->job; if (!job) { + rknpu_cnt_nojob++; /* #patch */ spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); REG_WRITE(RKNPU_INT_CLEAR, RKNPU_OFFSET_INT_CLEAR); rknpu_job_next(rknpu_dev, core_index); @@ -711,6 +954,9 @@ static void rknpu_job_timeout_clean(struct rknpu_device *rknpu_dev, if (job && ktime_us_delta(ktime_get(), job->timestamp) >= job->args->timeout) { + rknpu_cnt_treap++; /* #patch */ + rknpu_cnt_treap_age = + ktime_us_delta(ktime_get(), job->timestamp); rknpu_soft_reset(rknpu_dev); spin_lock_irqsave(&rknpu_dev->irq_lock, flags); @@ -718,6 +964,7 @@ static void rknpu_job_timeout_clean(struct rknpu_device *rknpu_dev, spin_unlock_irqrestore(&rknpu_dev->irq_lock, flags); + rknpu_iommu_domain_put(rknpu_dev); /* #patch: reap the domain ref this timed-out job leaked (job_done/job_abort put; timeout_clean forgot) -> fixes multi-domain switch wedge */ do { schedule_work(&job->cleanup_work); @@ -743,6 +990,22 @@ static void rknpu_job_timeout_clean(struct rknpu_device *rknpu_dev, } } +/* #patch54: reap every core's timed-out job, releasing the iommu domain references they hold. + * + * Exported for the domain-switch escalation in rknpu_iommu.c. rknpu_job_timeout_clean() is not a + * watchdog -- it only runs when ANOTHER submit arrives -- so when a stuck job pins the domain + * refcount above zero there is nothing that will ever reap it, and every subsequent switch burns + * its full 6 s timeout forever. */ +void rknpu_reap_all_cores(struct rknpu_device *rknpu_dev) +{ + uint32_t mask = 0; + int i; + + for (i = 0; i < rknpu_dev->config->num_irqs; i++) + mask |= rknpu_core_mask(i); + rknpu_job_timeout_clean(rknpu_dev, mask); +} + static int rknpu_submit(struct rknpu_device *rknpu_dev, struct rknpu_submit *args) { @@ -820,6 +1083,23 @@ static int rknpu_submit(struct rknpu_device *rknpu_dev, #endif } + /* #patch39: arm the progress watchdog for EVERY submit, not just NONBLOCK ones. + * + * The arm used to live inside the NONBLOCK branch below, so a BLOCKING submit was never + * observed by the watchdog at all. That is not a corner case: ork_dyn_colsplit() issues + * blocking submits by default ("NO barrier + BLOCKING submit -- EXACTLY the mcworker", + * src/npu/core/colsplit.c:48) and only goes NONBLOCK under ORK_F16_SENTINEL. Its + * ork_dyn_ prefix makes it look like the doorbell path; it is not. + * + * Measured consequence: a 1.5B-Q8 run that stalled 8 times with wd_period_us=1000 recorded + * wd_ticks=0 -- the hrtimer never started once, so the instrumentation built to catch these + * stalls could not see the very path that stalls. A stalled blocking submit then sits in + * rknpu_job_wait() for its full timeout (observed: 60.6 s) with nothing sampling it. + * + * A blocking job is in flight exactly like an async one, so there is no reason to treat it + * differently; the handler still self-disarms once no core has a job. */ + rknpu_wd_arm(rknpu_dev); + if (args->flags & RKNPU_JOB_NONBLOCK) { job->flags |= RKNPU_JOB_ASYNC; rknpu_job_timeout_clean(rknpu_dev, job->args->core_mask); diff --git a/drivers/rknpu/rknpu_reset.c b/drivers/rknpu/rknpu_reset.c index 65142368c..4e891dceb 100644 --- a/drivers/rknpu/rknpu_reset.c +++ b/drivers/rknpu/rknpu_reset.c @@ -95,6 +95,89 @@ static int rknpu_reset_deassert(struct reset_control *rst) } #endif +/* #patch: PER-CORE soft reset. rknpu_soft_reset() asserts ALL SIX DT reset lines + * ("srst_a0","srst_a1","srst_a2","srst_h0","srst_h1","srst_h2" — AXI+AHB per core), + * sleeps 100 ms, and sets the global soft_reseting flag that makes rknpu_job_next() + * bail for EVERY core. Recovering one stalled core therefore resets the whole NPU and + * destroys the other two cores' warm state — most of the ~3.6 s a doorbell miss costs. + * + * The lines are per core, so reset only core N: indices N (AXI) and N+num_irqs (AHB). + * No msleep, no global flag, no IOMMU re-attach — the other cores keep running. */ +int rknpu_soft_reset_core(struct rknpu_device *rknpu_dev, int core) +{ +#ifndef FPGA_PLATFORM + int ret = 0, ia, ih; + + if (rknpu_dev->bypass_soft_reset) + return 0; + + ia = core; + ih = core + rknpu_dev->config->num_irqs; + if (core < 0 || ih >= rknpu_dev->num_srsts) + return -EINVAL; + + ret |= rknpu_reset_assert(rknpu_dev->srsts[ia]); + ret |= rknpu_reset_assert(rknpu_dev->srsts[ih]); + udelay(10); + ret |= rknpu_reset_deassert(rknpu_dev->srsts[ia]); + ret |= rknpu_reset_deassert(rknpu_dev->srsts[ih]); + udelay(10); + + wake_up(&rknpu_dev->subcore_datas[core].job_done_wq); + return ret; +#else + return 0; +#endif +} + +/* #patch: per-core reset + IOMMU re-programming — the minimal sequence measured to actually recover a + * stalled core. Shared by the RKNPU_ACT_RESET_CORE ioctl and the progress watchdog so both paths behave + * identically, mirroring how rknpu_soft_reset serves the device-wide action. */ +int rknpu_reset_core_and_remap(struct rknpu_device *rknpu_dev, int core) +{ +#ifndef FPGA_PLATFORM + struct iommu_domain *domain; + int ret; + + ret = rknpu_soft_reset_core(rknpu_dev, core); + if (ret) + return ret; + + if (!rknpu_dev->iommu_en) + return 0; + + /* the re-attach must not race a concurrent dma_map; quiesce as rknpu_soft_reset does */ + if (!mutex_trylock(&rknpu_dev->reset_lock)) + return -EBUSY; + rknpu_dev->soft_reseting = true; + domain = iommu_get_domain_for_dev(rknpu_dev->dev); + if (domain) { + /* #patch46: serialize against domain switching. + * + * rknpu_soft_reset() holds reset_lock; rknpu_iommu_domain_get_and_switch() holds + * domain_lock. Different locks, so this detach/attach had NO mutual exclusion against + * a concurrent switch -- and a switch that reads iommu_get_domain_for_dev() while we + * are detached sees a domain that does not match rknpu_dev->iommu_domains[], bails + * with "mismatch domain get from iommu_get_domain_for_dev", and fails the caller. + * + * Measured: 1600 mismatch lines in a single 300 s run once the fast-abort raised the + * reset rate ~30x. Low reset rates hid it; it was always latent. + * + * Ordering is reset_lock -> domain_lock. Safe: no path takes reset_lock while holding + * domain_lock (rknpu_iommu_switch_domain does not reset). */ + mutex_lock(&rknpu_dev->domain_lock); + iommu_detach_device(domain, rknpu_dev->dev); + iommu_attach_device(domain, rknpu_dev->dev); + mutex_unlock(&rknpu_dev->domain_lock); + } + rknpu_dev->soft_reseting = false; + mutex_unlock(&rknpu_dev->reset_lock); + return 0; +#else + return 0; +#endif +} + int rknpu_soft_reset(struct rknpu_device *rknpu_dev) { #ifndef FPGA_PLATFORM @@ -107,8 +190,15 @@ int rknpu_soft_reset(struct rknpu_device *rknpu_dev) return 0; } + /* #patch: a failed trylock used to `return 0` — reporting SUCCESS without resetting anything. + * The caller (RKNPU_ACT_RESET from userspace recovery) then believes the NPU was reset when it + * was not, and spins on a still-wedged core. That was harmless while this was the only resetter; + * it stopped being harmless once the per-core recovery path also takes reset_lock, at which point + * userspace recovery silently became a no-op and the workload ran ~10x slower. + * Wait for the in-flight reset instead, so an ACT_RESET always means a reset actually happened. + * Legal: this function already msleep()s, so it is process context. */ if (!mutex_trylock(&rknpu_dev->reset_lock)) - return 0; + mutex_lock(&rknpu_dev->reset_lock); rknpu_dev->soft_reseting = true;