frontend: Fix reg next_id launch deadlock (non-blocking) - #154
Merged
Conversation
Make next_id/status/done_id internal (non-external) PeakRDL registers and launch a transfer on the next_id rd_swacc read strobe. The config read now completes in the same cycle it is issued (non-blocking): it no longer waits on the arbiter grant, so it can never head-of-line-block the config bus. Previously next_id was an external register whose read-ack was gated on the arbiter accepting the launch. When the backend stalled (req_ready_i low), the read-ack was withheld, the reg block raised external_pending, and that stall masked every further config-bus request -- deadlocking the block whenever a next_id read's launch was not granted in its single active cycle. The rd_swacc strobe pulses for one cycle, so capture it into a per-port launch_pending latch that holds arb_valid and the request payload stable until the arbiter accepts the launch (arb_ready implies req_ready_i), honoring rr_arb_tree LockIn so a launch is never dropped when the grant is a cycle late. The change lives in the CPUIF-agnostic launch/hwif layer, below the per-CPUIF packing, so it holds for apb4-flat, obi-flat and axi4-lite-flat.
…heck Self-checking testbench for the idma_reg32_3d register frontend that gates the non-blocking next_id launch contract (NumStreams 1 and 2, apb4-flat): - Test 2 asserts the next_id read completes within a bounded latency (2 config cycles) even while req_ready_i is held low. On a blocking readback (where the read-ack waits on the arbiter grant) this read hangs, so the per-read watchdog trips -- the test distinguishes the non-blocking read from a blocking one. - Test 2b (launch integrity) holds req_ready_i low for several cycles after a single next_id read, then releases it, and scoreboards that the launch fires exactly once. This is the no-dropped-launch guarantee of the pending latch. - Test 3 (NumStreams=2) checks stream_idx_o stays pinned to the launching stream until the grant lands. - A per-read watchdog and a global timeout catch any read or run that hangs. Because the non-blocking read no longer stalls, the driver waits for the launch to be accepted (id-advance) before re-launching, modeling the SW poll. Wired into Bender (idma_test) and idma.mk (idma_sim_tb_idma_reg_frontend).
stream_idx_o was driven by a last-wins scan over launch_pending, decoupled from the rr_arb_tree grant. With NumRegs>1 and multiple config ports launching different streams concurrently, the arbiter grants one port's dma_req_o while stream_idx_o points at another port's stream, so the transfer is tracked and completed under the wrong stream. Drive stream_idx_o from the arbiter's winning index (rr_arb_tree idx_o), gated by req_valid_o, so it always matches dma_req_o. NumRegs=1 was unaffected, which is why the existing tests missed it. Add a NumRegs=2 directed test: two ports launch on distinct streams, checking stream_idx_o matches the arbitrated port's stream. Fails pre-fix, passes after. Flagged by da-gazzi (#150).
Collaborator
Author
|
Folded in the |
da-gazzi
reviewed
Jul 9, 2026
da-gazzi
reviewed
Jul 9, 2026
da-gazzi
reviewed
Jul 9, 2026
Contributor
|
Mostly LGTM, you could avoid some duplicate TB logic by using test infrastructure from the APB repo |
DanielKellerM
commented
Jul 10, 2026
Rename the registered launch-latch signals (launch_pending, held_stream, arb_dma_req) to the _q convention and collapse the verbose block comments to a single line per the review.
Group tb_idma_reg_frontend.sv with the other frontend testbenches in test/frontend/ and its Bender entry accordingly, and remove the verbose sim-target comment per the review.
Collaborator
Author
|
Addressed the review in two new commits ( Fixed
Deferred (follow-ups)
|
This was referenced Jul 10, 2026
Replace the hand-rolled strict-APB4 tasks with the standard apb_test::apb_driver from the apb dep. Bridge one APB_DV interface per config port to the packed dma_ctrl_req_i/rsp_o via the apb assign macros and give each port its own driver so the multi-port arbitration test can drive two ports concurrently. Recover the bounded read latency by timing the blocking driver.read and subtracting its fixed framing; the per-read watchdog still fatals a read that never returns.
Collaborator
Author
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
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.
What
Fixes the register-frontend
next_idlaunch deadlock (introduced by the SystemRDL/PeakRDL migration in #73, inherited by #148) by making the launch non-blocking. Supersedes #153.Why non-blocking, not just deadlock-free
Reading
next_idlaunches a transfer. Withnext_idmodelled as a PeakRDLexternalregister, that read stalls the config bus until the backend arbiter grants — which has two problems:external_pending, masks the CPUIF request, and the read-ack can then never fire, so the whole reg block freezes.next_idread back-pressures unrelated memory transactions — a real hang on integrated systems, not a corner case.#153 fixed (1) but kept the blocking contract, so it still hits (2). A non-blocking read avoids both.
The fix
next_idbecomes an internal register with anrd_swaccread-strobe, so the config read completes in the same cycle (non-blocking — no HOL-block). A launch-pending latch then holds the arbiter request asserted until the backend grants, so a launch is never dropped when the grant lands a cycle after the (now-immediate) read.status/done_idare likewise internal, which also removes a latent freeze on reads of unused per-stream channel registers.Template-only (
idma_reg.rdl+idma_reg.sv.tpl); CPUIF-agnostic (apb/obi/axi4-lite).Software contract
The read no longer blocks — it returns immediately with the launch id. To serialise launches, software polls
next_iduntil it advances (the launch was accepted) before programming the next transfer, instead of relying on the read to block.Test
New
test/tb_idma_reg_frontend.sv(non-blocking contract) — the first directed reg-frontend testbench. It asserts thenext_idread completes within a bounded latency even under backpressure (the non-blocking property), and that each returned id yields exactly one backend launch with no drop (the latch guarantee). Verified in Questa (NumStreams 1 and 2). Non-vacuity: the same testbench run against a blocking template fails the bounded-latency check (the read never completes under backpressure), so it genuinely distinguishes the contract.Supersedes / subsumes
status/done_id) — this rewrite makes them internal.