idma_rt_midend: track every arbiter handshake in the choice FIFO#96
Merged
DanielKellerM merged 1 commit intoMay 21, 2026
Merged
Conversation
The RT midend funnels two request sources -- internal counter-driven
events (nd_req_valid_int / nd_req_ready_int) and external bypass
events (nd_req_valid_i / nd_req_ready_o) -- into one downstream port
through stream_arbiter_bypass. Each outgoing request is tagged with a
1-bit `choice` (1 = ext, 0 = int) which is supposed to be pushed into
i_stream_fifo so the response demux can route returning bursts back
to the bypass user (choice=1) or discard them at int_valid (choice=0,
internal RT events have no consumer).
The current push/pop conditions only match the *external* side of the
handshake:
.valid_i ( nd_req_valid_i & nd_req_ready_o )
.ready_i ( burst_rsp_valid_o & burst_rsp_ready_i )
So an internal RT event passes through stream_arbiter_bypass and
reaches the downstream port (nd_req_valid_o), but nothing is pushed
into the FIFO. Likewise, when the corresponding response returns and
the demux routes it to int_valid, the FIFO is not popped. The FIFO is
therefore systematically out of sync with the response stream
whenever ext and int events interleave: it contains only ext entries
while burst_rsp_valid_i carries a mix.
Replace the conditions with the arbiter-output and demux-input
handshakes so every outgoing request pushes once and every incoming
response pops once:
.valid_i ( nd_req_valid_o & nd_req_ready_i )
.ready_i ( burst_rsp_valid_i & burst_rsp_ready_o )
Reproducer (Verilator 5.046, NumEvents=1, NumOutstanding=8): release
reset, enable event 0 with event_counts=5, issue an ext bypass, wait
for the internal counter to fire, issue a second ext bypass. Send
three downstream burst responses tagged 1, 0, 1 (the expected source
of each event in issue order). Without the fix, the demux delivers
the int-tagged response to burst_rsp_o (corrupting the bypass user's
response stream) and silently routes one of the ext-tagged responses
to int_valid (lost). With the fix, all three responses arrive at
their declared source.
The existing testbench (tb_idma_rt_midend.sv) ties the bypass input
off (nd_req_valid_i = 1'b0) and leaves burst_rsp_o /
burst_rsp_valid_o unconnected, so the mixed-traffic interleaving and
the demux output side are never exercised; the bug therefore did not
surface in CI.
There was a problem hiding this comment.
Pull request overview
This PR fixes response-routing misalignment in idma_rt_midend by ensuring the “choice” FIFO tracks every downstream request/response handshake (not just bypass handshakes), so mixed internal (event-driven) and external (bypass) traffic can’t desynchronize the FIFO and misroute or drop responses.
Changes:
- Push the choice FIFO on the unified downstream request handshake (
nd_req_valid_o && nd_req_ready_i). - Pop the choice FIFO on the unified downstream response handshake (
burst_rsp_valid_i && burst_rsp_ready_o).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+209
to
+213
| .valid_i ( nd_req_valid_o & nd_req_ready_i ), | ||
| .ready_o ( /* HACK: NC */ ), | ||
| .data_o ( choice_head ), | ||
| .valid_o ( /* HACK: NC */ ), | ||
| .ready_i ( burst_rsp_valid_o & burst_rsp_ready_i ) | ||
| .ready_i ( burst_rsp_valid_i & burst_rsp_ready_o ) |
| .usage_o ( /* NC */ ), | ||
| .data_i ( choice ), | ||
| .valid_i ( nd_req_valid_i & nd_req_ready_o ), | ||
| .valid_i ( nd_req_valid_o & nd_req_ready_i ), |
Collaborator
|
Hi @flaviens, sorry about the noise here. We accidentally rebase-merged this PR earlier and had to force-push devel back as recovery. Your rt_midend fix is on our list to land. We're folding it together with PR #80 and some test work we've been carrying locally, so it will come back as a fresh PR soon. We'll cherry-pick your commit so the authorship trailer stays. If you'd rather be the PR author on the resubmit, happy to coordinate. |
This was referenced May 21, 2026
Author
|
Hi! Please do the simplest. Thanks! |
DanielKellerM
added a commit
to DanielKellerM/iDMA
that referenced
this pull request
Jun 10, 2026
tb_idma_rt_midend previously tied off the bypass nd_req input, hiding the choice-FIFO alignment bug (Flavien Solt, PR pulp-platform#96): on master the FIFO records only bypass handshakes while responses pop on any downstream completion, so internal-vs-bypass routing drifts as soon as both streams are active. Extending the smoke TB to drive bypass and count expected routing turns it into a real regression guard. Adds idma_sim_tb_idma_rt_midend Make target for repeatable invocation.
DanielKellerM
added a commit
that referenced
this pull request
Jun 10, 2026
) Track every arbiter handshake in the choice FIFO so the response demux stays in sync when internal and external events interleave, and add a mixed-traffic testbench (vsim + vcs, blocking) that catches the bug. Original fix: #96. Co-authored-by: Flavien Solt <flavien.solt97@gmail.com>
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.
Hi!
If I understand correctly, the FIFO currently records only external bypass handshakes, but downstream responses can correspond to either external or internal requests. When the two interleave, the FIFO loses alignment and responses may be misrouted or dropped. This changes FIFO push/pop to use the unified downstream request and response handshakes, so every issued request pushes once and every returned response pops once. The bug was missed because the existing testbench ties off bypass input and does not exercise mixed traffic.
Please double check.
Thanks!
Flavien