Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional interleaving / deinterleaving in Axis #62

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ollie-etl
Copy link

@ollie-etl ollie-etl commented May 6, 2023

Adds interleaving support to AxisStreamSource. Packets may be interleaved on tid, tdest, the combination, or neither. The suggestion of tuser was ignored, because I don't believe that is semantically well defined. The maximum interleave depth can
be set, to limit the number of concurrent packets streams.

AxisStreamSink and AxisStreamMonitor can deinterleave streams, an return packets on recv ordered by tlast occurance on packets in the stream.

The implementation maintains a single concurrent queue, but maintains a set of in-flight frames in the _run implmentation of each class.

Where the dut may output interleaved transactions, the current sink
returns frames delimited by tlast, but contvain data from multiple
transactions.

This commit adds an opt in paramater which allows transactions to be
sorted, prior to insertion in the rx queue. This causes recv() / read()
calls to return frames containing deinterleaved transations, with the
return order determined by occurance of tlast within each transaction.

If tlast is not presence, this has no effect
@ollie-etl
Copy link
Author

@alexforencich Another feature for your consideration. I think its generally applicable and useful, hence the upstream PR.

README.md Outdated Show resolved Hide resolved
cocotbext/axi/axis.py Outdated Show resolved Hide resolved
@alexforencich
Copy link
Owner

I think this is a really good idea, and I think it should probably be extended by supporting interleaving on the source side as well.

I'm thinking it might make sense to have an interleave depth parameter as well, default 1 probably, to control the interleaving on TX and check and warn on RX if the depth is exceeded.

I also think it would make sense to be able to specify any combination of tid, tdest, and tuser for interleaving/de-interleaving. So perhaps replace the default value with an empty set or list, then the interleaving can be specified as something like {'tid', 'tdest'} and the options checked with in. Although I suppose the fun (or maybe not so fun) part about python is that in would also work if someone specified "tid tdest".

Also I have run in to problems before with pre-allocating for all possible ID values, so I think it makes sense to do something similar to the AXI models, where the storage is dynamically allocated based on the IDs that are active at a given point in time. And this is probably going to be required anyway when multiple signals are used, anyway. I'm thinking you can do something simple like use (tid, tdest, tuser) as a dict key to look up the frame, and then delete the key when the frame is complete. I think you can also check the number of entries in the dict against the interleave depth to see if it was exceeded.

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (e816d6a) 75.96% compared to head (8859e22) 75.79%.
Report is 6 commits behind head on master.

❗ Current head 8859e22 differs from pull request most recent head b00c88a. Consider uploading reports for the commit b00c88a to get more accurate results

Files Patch % Lines
cocotbext/axi/axis.py 44.44% 4 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   75.96%   75.79%   -0.17%     
==========================================
  Files          23       23              
  Lines        3703     3719      +16     
  Branches      600      608       +8     
==========================================
+ Hits         2813     2819       +6     
- Misses        614      618       +4     
- Partials      276      282       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ollie-etl
Copy link
Author

I think this is a really good idea, and I think it should probably be extended by supporting interleaving on the source side as well.

The thought occurred to me, but the implementation was less obvious. I'll give it some thought.

@alexforencich
Copy link
Owner

Well, we can set the source-side interleaving aside for now and just implement de-interleaving on the receive side.

@ollie-etl ollie-etl changed the title Optional deinterleaving in AxisStreamSink Optional interleaving / deinterleaving in AxisStreamSink Nov 16, 2023
@ollie-etl
Copy link
Author

@alexforencich Sorry for the huge delay here. Have updated to generalise over AxisStreamSource, AxisStreamMonitor and AxisStreamSink

tests/axis/Makefile Outdated Show resolved Hide resolved
@ollie-etl ollie-etl changed the title Optional interleaving / deinterleaving in AxisStreamSink Optional interleaving / deinterleaving in Axis Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants