fix: rt_midend choice FIFO sync + testbench for mixed traffic#108
Open
DanielKellerM wants to merge 5 commits into
Open
fix: rt_midend choice FIFO sync + testbench for mixed traffic#108DanielKellerM wants to merge 5 commits into
DanielKellerM wants to merge 5 commits into
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.
tb_idma_rt_midend previously tied off the bypass nd_req input, hiding the choice-FIFO alignment bug (Flavien Solt, PR #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.
There was a problem hiding this comment.
Pull request overview
This PR fixes a response-demux misrouting issue in idma_rt_midend by aligning the choice FIFO push/pop conditions with the unified downstream request/response handshakes (so every issued request is tracked and every returned response consumes one choice entry). It also updates the RT midend testbench to drive mixed internal (counter) and bypass traffic so the previously untested interleaving path is exercised in simulation, and adds a Make target to run that testbench under Questa/ModelSim.
Changes:
- Fix
idma_rt_midendchoice FIFO bookkeeping to push on downstream request handshake and pop on downstream response handshake. - Extend
tb_idma_rt_midendto generate bypass traffic interleaved with internal events and add an end-of-test routing consistency check. - Add
idma_sim_tb_idma_rt_midendMake target and introduceVSIMtool variable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/midend/idma_rt_midend.sv |
Corrects choice FIFO push/pop handshake signals to prevent response demux drift under mixed traffic. |
test/midend/tb_idma_rt_midend.sv |
Drives bypass + internal traffic concurrently and checks bypass response count matches bypass request count. |
idma.mk |
Adds VSIM variable and a Questa/ModelSim target to compile and run tb_idma_rt_midend. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // would either lose bypass responses (they go to | ||
| // the internal sink) or duplicate them. | ||
| assert (bypass_rsp_seen == bypass_req_issued) else begin | ||
| $error("[tb_idma_rt_midend] routing mismatch: bypass_req_issued=%0d bypass_rsp_seen=%0d", |
Comment on lines
+174
to
+177
| bypass_req_issued = '0; | ||
| bypass_rsp_seen = '0; | ||
| internal_rsp_seen = '0; | ||
|
|
DanielKellerM
force-pushed
the
fix/rt-midend-choice-fifo-tests
branch
from
00d2898 to
de87a9c
Compare
DanielKellerM
force-pushed
the
fix/rt-midend-choice-fifo-tests
branch
from
255537b to
fa042fb
Compare
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.
Summary
Lands @flaviens's
rt_midendchoice FIFO fix (originally PR #96, which is stuck in a ghost-merged state and cannot be reopened, see the comment thread there), plus two follow-up testbench commits that exercise the previously-untested mixed-traffic interleaving path.Commits
idma_rt_midend: track every arbiter handshake in the choice FIFO(Flavien Solt) — fixes the choice FIFO push/pop conditions that only matched the external side of the handshake, causing the response demux to drift out of sync when internal and external events interleave. Original PR: idma_rt_midend: track every arbiter handshake in the choice FIFO #96.test: Drop redundant counter init from main initial block(Daniel Keller) — small cleanuptest: Drive rt_midend bypass non-trivially; add Make target(Daniel Keller) — exercises the previously-untied bypass path so the bug above would be caught in CI going forward.Test plan
make idma_hw_allregenerates cleanlytb_idma_rt_midendexercises mixed ext/int traffic; expected to pass with the choice FIFO fix