rknpu: take the MMU force-reset out of the domain-switch path (supersedes #388) - #390
Open
mafischer wants to merge 10 commits into
Open
rknpu: take the MMU force-reset out of the domain-switch path (supersedes #388)#390mafischer wants to merge 10 commits into
mafischer wants to merge 10 commits into
Conversation
rknpu_irq_handler() touches the core's registers unconditionally: the no-job path writes RKNPU_OFFSET_INT_CLEAR and the normal path reads RKNPU_OFFSET_INT_STATUS. rknpu_power_off() runs from a deferred work item, so an interrupt that is late or spurious can be delivered after the block has already been powered down. The register access then takes an external abort and the machine dies instantly, with no console output and no recovery short of a power cycle: Unable to handle kernel paging request at virtual address ... 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 The trace was captured over netconsole with CPU 0 idle, i.e. the NPU had finished and powered down and the interrupt landed afterwards. Bail out early when the device is not powered. 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 window that occurs in practice, and an unpowered device cannot be asserting an interrupt, so returning IRQ_NONE cannot cause a level-triggered interrupt storm.
rknpu_job_free() drops the fence reference but never signals the fence. A job that completes normally is signalled from the completion path, but a job destroyed by rknpu_job_timeout_clean() or rknpu_job_abort() never gets there. Every waiter on such a fence therefore blocks until its own timeout expires rather than being woken when the job actually died, which defeats the purpose of waiting on the fence to detect a failed job. With CONFIG_ROCKCHIP_RKNPU_FENCE=y and a userspace consumer that waits on the fence fd, a single aborted job costs the full wait timeout instead of returning promptly. Signal the fence with -ETIMEDOUT before dropping the reference so waiters wake at once and can tell failure from completion with dma_fence_get_status(). The added dma_fence_is_signaled() test makes this a no-op on the normal completion path.
rknpu_iommu_domain_put() is a bare atomic_dec(), so an unbalanced put drives iommu_domain_refcount negative without bound. rknpu_iommu_domain_get_and_switch() decides a switch is safe using only if (atomic_read(&rknpu_dev->iommu_domain_refcount) == 0) so once the count has gone negative it never reads zero again. Every subsequent domain switch then waits the full RKNPU_SWITCH_DOMAIN_WAIT_TIME_MS and fails, and since rknpu_gem_object_create() switches domains, every allocation after that returns -EINVAL: RKNPU: switch iommu domain time out, failed to switch iommu domain, id: 1 RKNPU: rknpu_gem_object_create error Note this presents as MEM_CREATE failing with -EINVAL at essentially zero IOVA in use, which is easy to misread as memory exhaustion rather than a stuck reference count. Clamp at zero and log, so an unbalanced put is a bounded, visible anomaly instead of a device that needs a reboot. This is deliberately a safety net: the underlying imbalance is fixed separately.
rknpu_job_abort() releases the IOMMU domain reference unconditionally. The reference is acquired exactly once, in rknpu_job_commit(), but three teardown paths release it -- the completion path, rknpu_job_abort() and rknpu_job_timeout_clean() -- and nothing records whether a given job still holds one. A job that completes and is then aborted, or is aborted and then reaped, therefore releases twice. Because the count is device-wide rather than per job, a double release drives iommu_domain_refcount to zero while other cores are still executing. rknpu_iommu_domain_get_and_switch() decides a switch is safe using only if (atomic_read(&rknpu_dev->iommu_domain_refcount) == 0) so once the count reaches zero prematurely the IOMMU is reprogrammed underneath live work, and the result is a cascade that only a reboot clears: RKNPU: mismatch domain get from iommu_get_domain_for_dev RKNPU: failed to switch iommu domain, id: 1, ret: -22 RKNPU: rknpu_gem_get_pages: dma map 2097152 fail This only affects users of more than one IOMMU domain (rknpu_mem_create()'s iommu_domain_id); a single-domain workload cannot hit it. The premature zeros are silent: the count never goes negative, so an underflow check does not catch them. Instrumenting rknpu_iommu_domain_put() with __builtin_return_address(0) and reporting "reached zero while some core still has a job" separately from "went below zero" attributed 17 premature zeros in a single run to rknpu_job_abort(), against one underflow from rknpu_job_timeout_clean(). Track ownership per job and make every release a test-and-clear, so a job releases the reference at most once no matter which teardown path runs, and a second teardown of the same job is a no-op. With this applied the premature-zero count measured over the same workload drops to zero. Closes: rockchip-linux#387
rknpu_job_timeout_clean() detaches a timed-out job from its core and queues its cleanup work, but never releases the IOMMU domain reference the job acquired in rknpu_job_commit(). The completion path and rknpu_job_abort() both release it; this path does not. The reference is therefore leaked for every job reaped here, and since iommu_domain_refcount is device-wide and rknpu_iommu_domain_get_and_switch() proceeds only when it reads exactly zero, one leaked reference is enough to make every subsequent domain switch fail for the lifetime of the boot -- and with it every allocation that has to switch domains. Release it, guarded by the same per-job ownership bit the other two paths use, so a job reaped after it has already been aborted does not double release.
…lper Switching a master between IOMMU domains through the core -- iommu_detach_device() followed by iommu_attach_device() -- makes the attach run rk_iommu_enable(), which performs a full rk_iommu_force_reset() of every MMU bank. With work in flight on other cores that reset intermittently fails its DTE_ADDR readback verification, the attach then fails, and __iommu_attach_group() leaves the master attached to no domain at all: rk_iommu fdab9000.iommu: Error during raw reset. MMU_DTE_ADDR is not functioning iommu driver failed to attach a compatible domain WARNING: CPU: 5 PID: 7555 at drivers/iommu/iommu.c:2116 __iommu_attach_group+0x9c/0xac The hardware needs none of that to change page tables. On an already enabled MMU a page table swap is stall, write RK_MMU_DTE_ADDR on every bank, ZAP_CACHE, unstall, with paging left on throughout. rk_iommu_switch_domain() does exactly that, and also moves the rk_iommu between the two domains' iommus lists so rk_iommu_zap_iova() and the TLB flush paths still name the live domain. Because the core is not involved, iommu_get_domain_for_dev() does not track such a switch and a caller must track the live domain itself. rk_iommu_reprogram() re-establishes the MMU for the currently attached domain after a hardware reset has wiped it. It is deliberately unconditional: the domain has not changed, so a switch helper would short-circuit, but the hardware state is gone. Both are exported for the rknpu driver, which is the only in-tree user of multiple domains on one master. No behaviour changes until a caller uses them.
This driver is the only in-tree user of several IOMMU domains on one master, and it tracks which one is live itself, in rknpu_dev->iommu_domain_id. iommu_get_domain_for_dev() returns what the core last attached, which coincides with the live domain only while switching goes through the core. Introduce rknpu_iommu_live_domain() and use it everywhere a mapping, unmap or TLB flush has to target the domain the NPU is actually running in: - rknpu_iommu_dma_map_sg() / rknpu_iommu_dma_unmap_sg() - the post-map iommu_flush_iotlb_all() in rknpu_gem_get_pages() - the iommu_map()/iommu_unmap() of SRAM and NBUF cache buffers No functional change on its own: while switching still goes through the core the two answers are identical. This is preparation for the following commit, which takes the core out of the switch path -- after which they are not.
rknpu_iommu_switch_domain() changed domains with iommu_detach_device() followed by iommu_attach_device(). For rockchip-iommu the attach runs rk_iommu_enable(), i.e. a full rk_iommu_force_reset() of every MMU bank, and with work in flight on other cores that reset intermittently fails its DTE_ADDR readback. The attach then fails and the master is left attached to no domain: rk_iommu fdab9000.iommu: Error during raw reset. MMU_DTE_ADDR is not functioning iommu driver failed to attach a compatible domain WARNING: CPU: 5 PID: 7555 at drivers/iommu/iommu.c:2116 __iommu_attach_group+0x9c/0xac Use rk_iommu_switch_domain() instead, which reprograms RK_MMU_DTE_ADDR in place and never resets. Three consequences follow, all handled here: - The mismatch check against iommu_get_domain_for_dev() goes away. That function no longer tracks these switches, so the comparison could only produce a spurious failure. The driver's own iommu_domains[] is the source of truth. - A new domain gets its IOVA allocator initialised directly with init_iova_domain() + iova_domain_init_rcaches(), instead of being made to look like a DMA-API domain (setting __IOMMU_DOMAIN_DMA_API on the type and calling iommu_setup_dma_ops()) purely so that iommu_dma_init_domain() would initialise the cookie's iovad. That route depends on the core reporting this domain as the device's, which is exactly what is no longer true. The domain stays a plain unmanaged domain, which is what iommu_map_sg() and iommu_unmap() want. - The permanent overwrite of iommu_group->default_domain is removed. It existed because dma-iommu resolves every DMA-API mapping through iommu_get_dma_domain() == group->default_domain, so it was the only way to make a DMA-API mapping land in a non-default domain. Leaving the core's default pointing at a domain the driver may later free means any core path that reattaches "the default" uses it. The NPU's own buffers are mapped explicitly against the domain the driver names, so nothing here needs it. Note the private copy of struct iommu_group is retained: a *scoped* override is still required for the dma-buf attachment window, because the DMA API offers no way to name a target domain. That is added in a later commit and is restored immediately, unlike the permanent overwrite removed here. rknpu_iommu_free_domains() correspondingly drops its iommu_detach_device() -- the core never attached these domains -- and releases the IOVA allocator this function created.
A soft reset wipes the MMU, so the page table must be reprogrammed afterwards. That was done with iommu_detach_device() + iommu_attach_device() on iommu_get_domain_for_dev(), which is the group's default domain. That was only ever correct because the driver overwrote the default to follow its own switches. It no longer does, so this would re-attach domain 0 while the driver still believes domain N is live. Every IOVA the next job uses then resolves against the wrong page table: the job is committed and never completes, with no interrupt and no error, and the output buffer is left untouched. Use rk_iommu_reprogram(), which re-enables the MMU from the live domain rather than from whatever the core last attached.
An imported dma-buf's sg is mapped in two places, and neither of them named the domain the buffer would actually be used in. First, dma_buf_map_attachment() runs inside DRM core's PRIME_FD_TO_HANDLE, before any rknpu hook, and maps through the DMA API -- which always targets group->default_domain. While this driver permanently overwrote that default the mapping happened to land in the live domain; with that overwrite gone it lands in domain 0, while rknpu_gem_prime_import_sg_table() records the object as belonging to the live domain. A submit against such a buffer then resolves an IOVA that exists only in another page table: the job is committed and never completes, with no interrupt, no fault and no error, and userspace sees an untouched output buffer. Second, PRIME_FD_TO_HANDLE carries no domain at all. The target is named only by the MEM_CREATE that follows, which does not re-map an already-imported handle, so the mapping lands wherever the device happened to be. Fix both ends: - Override the core's default domain for the duration of the attachment map, in the .gem_prime_import hook, and for the matching unmap at destroy. This is scoped to one call, taken under domain_lock so a concurrent switch cannot move the live domain underneath it, and restored immediately -- unlike the permanent overwrite removed earlier in this series. - Add RKNPU_ACT_SET_DOMAIN, a switch-only action with no allocation, so userspace can make the target domain live before PRIME_FD_TO_HANDLE. An alternative to the second half would be for MEM_CREATE to re-map an imported handle when the requested domain differs from the one it was mapped in, which would avoid new uABI. That is arguably cleaner, but it is not what was implemented and measured here, so it is offered as a note rather than a claim. Native (non-imported) allocations were never affected: their MEM_CREATE performs the switch and the mapping together.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This supersedes #388. It contains those five fixes unchanged, plus five commits that take
rk_iommu_force_reset()out of the domain-switch path. Please take this one instead — #388 alone isnot sufficient for multi-domain use, and we have the A/B to show it.
Closes #387.
Why #388 alone is not enough
We built both variants and ran them on the same board, same workload, same session:
Note the targeted probes pass on both. Only the real workload separates them, which is worth
knowing if you try to reproduce this with a small test.
With #388 applied the reference-count wedge is gone (no more
switch iommu domain time out, no moremismatch domain get from iommu_get_domain_for_dev), and what remains is this:rknpu_iommu_switch_domain()switches withiommu_detach_device()+iommu_attach_device(), andfor rockchip-iommu the attach runs
rk_iommu_enable()→rk_iommu_force_reset()— a full reset ofevery MMU bank on every domain change. With work in flight on other cores that reset intermittently
fails its DTE_ADDR readback, the attach fails, and the master is left attached to nothing.
What the five new commits do
iommu/rockchip: addrk_iommu_switch_domain()andrk_iommu_reprogram()— an in-place pagetable swap (stall → write
RK_MMU_DTE_ADDR→ZAP_CACHE→ unstall, paging left on) and anunconditional re-enable for use after a hardware reset. No users yet, no behaviour change.
rknpu: resolve the live domain from the driver's own state —iommu_get_domain_for_dev()only tracks the live domain while switching goes through the core. Pure preparation; no
functional change on its own.
rknpu: switch domains without the core — adopt the primitive, initialise each domain's IOVAallocator directly instead of pretending it is a DMA-API domain, and drop the permanent overwrite
of
iommu_group->default_domain.rknpu: reprogram the live domain after a soft reset — the reset wipes the MMU, and the olddetach/attach re-established the core's default, not the live domain.
rknpu: map dma-buf imports into the domain they will be used in —dma_buf_map_attachment()runs inside DRM core's
PRIME_FD_TO_HANDLEand maps through the DMA API, i.e. intodefault_domain; andPRIME_FD_TO_HANDLEcarries no domain at all. Fixed with a scopeddefault-domain override around the attachment map/unmap, plus a switch-only
RKNPU_ACT_SET_DOMAINaction so userspace can make the target domain live first.Points a reviewer should push on
RKNPU_ACT_SET_DOMAIN). An alternative that avoids it would be forMEM_CREATEto re-map an already-imported handle when the requested domain differs. That isarguably cleaner; it is not what we implemented and measured, so we did not claim it.
struct iommu_groupis retained. The permanent overwrite is gone, but thescoped one in commit 5 still needs the field, because the DMA API offers no way to name a target
domain. If you would prefer a different mechanism there, that is the place to say so.
rockchip-iommu.cfor a driver outsidedrivers/iommu.Testing
Developed and exercised on RK3588 (Rock 5B) at 6.1.115 through a userspace NPU runtime driving
multi-domain matmul workloads, with
make test-equivalent suites and targeted multi-domain probesall passing, plus the throughput figures above.
This exact branch builds on
develop-6.1:drivers/iommu/anddrivers/rknpu/compile with noerrors or warnings, and a full
Imagelinks withrk_iommu_switch_domain/rk_iommu_reprogramcorrectly exported. To be precise about what has not been done: this branch has not been booted on
develop-6.1itself — the runtime testing was on the armbian 6.1 tree, whose rknpu driver isbyte-identical to this one (
rknpu_job.c,rknpu_gem.c,rknpu_reset.cdiffer by 0 lines,rknpu_iommu.cby 2), though itsrockchip-iommu.cdiffers by 61 lines.Two unrelated pre-existing build failures in this tree with an armbian-derived config, for the
record and not caused by this series:
drivers/input/touchscreen/focaltech_touchfails onmm_segment_t/get_fs, andmodules_checkreports a module name conflict.