[kmac,dv] Check FIFO empty interrupt #23942
Conversation
martin-velay
requested review from
rswarbrick and
vogelpi
and removed request for
a team
martin-velay
force-pushed
the
kmac_fifo_empty
branch
2 times, most recently
from
d777849
to
f1560b6
Compare
martin-velay
force-pushed
the
kmac_fifo_empty
branch
4 times, most recently
from
2b700a8
to
af37ff6
Compare
|
Hi @rswarbrick, @andreaskurth and @vogelpi, Thanks in advance! |
There was a problem hiding this comment.
Thanks @martin-velay for your work on this! The PR looks good to me. I think I can follow and understand the code and it looks right to me. But I am obviously no DV expert.
Great catch also on the Xcelium-specific fixes btw!
| fork begin | ||
| predict_fifo_empty_intr(); | ||
| end join_none |
There was a problem hiding this comment.
If I understand correctly, this will be firing off a new process on each negedge of the clock. Can we restructure slightly so that predict_fifo_empty_intr gets split into two pieces? The first could return instantly. This would be enough for updating intr_fifo_empty_allowed (which is what we really need in the loop here).
Then we could fire off the second half and update fifo_empty outside of this loop?
I think this will also get rid of the second wait_clks line in the loop.
There was a problem hiding this comment.
I'll need time to re-run the simulation over the night then, to be sure it's not breaking anything as this was a bit tricky to get it to work
There was a problem hiding this comment.
@rswarbrick
When you suggest to move the "second half and update fifo_empty outside of this loop", I guess you are referring to the do...while(...) loop? If yes, this will cause to not check the FIFO full interrupt when it's actually the most important. I think I should keep the check on each clock cycles when intr_fifo_empty_allowed or msgfifo_access are high.
If that's OK, this PR could be merged now as the other comments have been already addressed.
There was a problem hiding this comment.
I'm not personally a fan of the approach this code takes. In particular, I really don't like the background process we fire off when updating intr_fifo_empty (because I worry that it ends up hard to reason about - what happens if there's a reset?) I'm willing to believe the code works, and I'm glad. And I hope I never have to maintain it :-)
Let's forget my restructuring notes: it's your code, not mine! I'm happy that the code looks correct: let's merge it.
- Linked to this issue lowRISC#22341 Signed-off-by: Martin Velay <[email protected]>
- Use SV string function "len" outside of the constraints
- Wait EDN request without using DV_SPINWAIT macro, as get() method
from UVM TLM interface is blocking by natur
Signed-off-by: Martin Velay <[email protected]>
martin-velay
force-pushed
the
kmac_fifo_empty
branch
from
af37ff6
to
66d54be
Compare
|
Merging despite the CI failure: it's pretty clearly an IT issue and not caused by this PR. |
As of lowRISC#23942, the FIFO empty status-type interrupt is predicted and verified which unfortunately negatively impacts the simulation performance in some cases. Most tests are not notably impacted but for some rather long running tests, the maximum observed job runtime increased by up to 50%. As a result, we started seeing timeouts for some of these tests in the nightly regressions after merging the FIFO empty interrupt verification PR. Therefore, this commit increases the timeout for those long running tests where we observed timeouts. This resolves lowRISC#24225. Signed-off-by: Pirmin Vogel <[email protected]>
As of #23942, the FIFO empty status-type interrupt is predicted and verified which unfortunately negatively impacts the simulation performance in some cases. Most tests are not notably impacted but for some rather long running tests, the maximum observed job runtime increased by up to 50%. As a result, we started seeing timeouts for some of these tests in the nightly regressions after merging the FIFO empty interrupt verification PR. Therefore, this commit increases the timeout for those long running tests where we observed timeouts. This resolves #24225. Signed-off-by: Pirmin Vogel <[email protected]>
As of lowRISC#23942, the FIFO empty status-type interrupt is predicted and verified which unfortunately negatively impacts the simulation performance in some cases. Most tests are not notably impacted but for some rather long running tests, the maximum observed job runtime increased by up to 50%. As a result, we started seeing timeouts for some of these tests in the nightly regressions after merging the FIFO empty interrupt verification PR. Therefore, this commit increases the timeout for those long running tests where we observed timeouts. This resolves lowRISC#24225. Signed-off-by: Pirmin Vogel <[email protected]>
As of lowRISC#23942, the FIFO empty status-type interrupt is predicted and verified which unfortunately negatively impacts the simulation performance in some cases. Most tests are not notably impacted but for some rather long running tests, the maximum observed job runtime increased by up to 50%. As a result, we started seeing timeouts for some of these tests in the nightly regressions after merging the FIFO empty interrupt verification PR. Therefore, this commit increases the timeout for those long running tests where we observed timeouts. This resolves lowRISC#24225. This is a cherry pick of commit dff10e5 to branch earlgrey_1.0.0. Signed-off-by: Pirmin Vogel <[email protected]>
As of #23942, the FIFO empty status-type interrupt is predicted and verified which unfortunately negatively impacts the simulation performance in some cases. Most tests are not notably impacted but for some rather long running tests, the maximum observed job runtime increased by up to 50%. As a result, we started seeing timeouts for some of these tests in the nightly regressions after merging the FIFO empty interrupt verification PR. Therefore, this commit increases the timeout for those long running tests where we observed timeouts. This resolves #24225. This is a cherry pick of commit dff10e5 to branch earlgrey_1.0.0. Signed-off-by: Pirmin Vogel <[email protected]>
Linked to this issue #22341
I am not sure it works well yet. I have only checked it for one test, and it seems good from what I've seen.
This check is not modeling the FIFO level, instead it is using the FIFO empty/full from the DUT through a backdoor access. Then it does a prediction on the FIFO empty interrupt as described in the KMAC register doc.