-
Notifications
You must be signed in to change notification settings - Fork 174
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
Stream Abstraction for amaranth.lib #317
Comments
I think it would be very useful to have cheap adapters to/from both AXI4-Stream and Avalon-ST, so it makes sense to restrict the features provided by nMigen streams to a subset supported by both. For example, I think we should not provide error reporting. One exception is that non integer multiple of octet wide streams are extremely useful, and the library primitive should not be restricted only to |
I agree with this.
I don't _dis_agree with this, but I did want to point out that it seems very possible to use AXI4-Stream's
I think I'm slightly less enthused about non-octet data streams than you are, but it seems fairly easy to build this capability on top of AXI4-Stream for an adapter if need be (it will just be a slightly more complex adapter). So, no objections. A motivating example (or more than one) seems like it would be useful for this discussion - it might help center our thinking. |
In my view, non-octet data streams are not particularly inherently valuable, but their value comes from not imposing restrictions on user code. Consider an emitter of raw 8b10b symbols from a SERDES. Such an emitter would very naturally use a 10 bit wide stream, could be connected to a 10 bit wide FIFO, or a 10→20 bit upconverter, etc. |
Couple of random thoughts - not really all that coherent;
|
If that can help, in LiteX we have something with LiteX's stream (https://github.com/enjoy-digital/litex/blob/master/litex/soc/interconnect/stream.py) that seems very similar in the idea to what you want to achieve. This is is basically a mix between AXI-4/Avalon-ST: with only
LiteX's stream can be directly mapped to AXI-4 and adapted easily to AvalonST: https://github.com/enjoy-digital/litex/blob/master/litex/soc/interconnect/avalon.py. I really don't find AvalonST convenient to use in systems due to the latency between valid/ready which make creating and and debugging cores complicated. So for nMigen's stream, you should probably reuse more from AXI-4 than AvalonST and just provide adapters to AXI-4 (which should be almost direct mapping) and AvalonST (which should only be adaptation of the valid/ready latency). |
In addition to my previous comment: I just wanted to share some feedback of what is already there in LiteX and has already been very useful: stream can simplify things a lot! With a good library of base elements: Converter/FIFOs/Mux/Demux/Gearbox/etc... + #213 on top, it could becomes very powerful! I'm not able to be heavily involved in nMigen development (due to others obligations), so just wanted to share some feedback. If you end up having something close to what is already in LiteX and better implemented, i'll probably be a user of that in the future. |
LiteX has certainly been an inspiration for this. I suspect the final design in nMigen would be close to it conceptually, with many of the changes addressing the experience of using LiteX streams for all these years. |
Taking a pass at @mithro 's comments (inline below):
It's not clear to me that streams have anything to do with resets, or indeed that they necessarily have state to be reset.
I propose that we mandate a stream's endpoints be in the same clock domain. We can use a wrapper around an AsyncFIFO to cross streams between clock domains as needed (terminate stream A in clock domain A, initiate stream B in clock domain B). As far as rates go, that's an interesting question, as is how we'd go about multiplexing multiple streams onto one faster/wider stream. This requires more thought.
I'm not sure about pipelines, but I see value in Stream-interface wrappers around FIFOs of both types. I don't think streams should inherently contain FIFOs or anything like that.
Agreed, we should support this (optionally, I would argue).
This is basically the TUSER stuff from AXI4-Stream. I think we should start without this, and if we see it as a big weakness, add it later. In general I think we should be very conservative about adding features to streams - we should start with the simplest thing that can possibly work.
This is interesting but I'm not sure it actually has much to do with streams? Unless you're saying you want typechecking on the payload types of streams which are connected together, which does sound like a good thing to have. |
I began to prototype Stream abstractions for nMigen to get a better feeling of how this could work both from a conceptual point and how the concrete ergonomics couold be like. Here I will link to some code (that you can read if you are into that or just skip) and try to sum up some of the things I learned from this. Based on the discussions in here I went with a design that has AXI-Stream like So here are some things I liked, disliked & learned from my prototype:
|
@enjoy-digital You wrote that parameters can evolve "at each start of packet" Did you mean frame instead of packet? Also, how useful have parameters been in practice? Do you think they form part of a stream specification, or would it be better to decouple them? |
Here's a list of Streams in nmigen I've found:
|
After reading through this issue I made a streams implementation as part of one of the accelerators on CFU-Playground. I'm not completely happy with it, but after using it and, and looking over other implementations, I have many opinions. Minimum Viable Stream APIA minimum stream API would include:
Most of the useful functionality built with streams uses just these three signals. It is useful to allow On the Meaning of valid and readyThere are plenty of valid that an endpoint might either ignore valid or ready or assert them constantly. For example, a video PHY may need to consume data every single clock cycle in order to generate a correct video signal. It might therefore have a sink that always asserts ready, and always reads the payload regardless of the state of valid. Endpoints like this should be explicitly allowed, so long as the behavior is well documented. On the Use of FramesLiteX includes first and last signals in its streams, which define frames. I have many opinions:
Source vs Sink is confusingIt can be difficult to write or understand code using streams. When using components with a stream interface, data goes into sink and comes out of a source, but when implementing those components the programmer sees the other side of the endpoint and, data comes out of the sink and goes into the source. To reduce this confusion, I have had some success:
I see the Luna codebase addresses some of these issues by paying careful attention to naming with Other observations
|
@alanvgreen Some things that came to mind:
I agree with your assessment, that there are plenty of cases where an something might want to use a stream like interface without obeying all of the rules. However I think one has to be very careful when doing that, as it can deadlock the whole pipeline of streams. Furthermore, in naps it was very useful to formally check that the modules using streams actually obey the rules of the stream interface:
Atleast But I think in general I agree,
This is something we tried in class StreamTruncate(Elaboratable):
def __init__(self, input: Sink, output_width: int):
self.output = Source(unsigned(output_width))
def elaborate(self, plat):
m = Module()
m.d.comb += output.connect_upstream(input, exclude=["payload"])
m.d.comb += output.payload.eq(input.payload >> (len(input) - output_width))
return m Inside a module one always wants to connect data coming from a
I think all of the discussions in this issue were about AXI4-Stream and Avalon-ST, not the "full" AXI4 and Avalon. The AXI4-Stream and Avalon-ST variants are not a lot more heavy-weight than the proposal here. |
Agreed. As per AXI streams, "
Hmmm. I think you're right here too. By default, the I'm wondering about the best mechanism for marking that behavior. Documentation only seems insufficient, but a Converter class seems overly heavy-weight.
This is a great data point. Thank you for the pointer to formal_util.py! |
I want to think more deeply about this. This "switching of directions" as you describe it was the primary motivator for both the diagram and having explicit What would you say naps does differently to LiteX that would makes naps streams easier to understand? Or is it just that you find LiteX streams simple as they are? One thing I noticed was that when using streams it is always important to understand which of the four roles the endpoint is taking:
When I lost track of which role each point was filling, I started to get confused and make mistakes. I am now wondering whether, rather than which of four roles an endpoint is taking, it might be better to think of it as only two: "coming toward" and "going away". What do you think? |
what's the motivation for this constraint? I've found this post about ready/valid signals to be a carefully worded description of the handshake protocol (mostly as per AXI4), but it does permit changing the payload while valid remains asserted, which I've used a lot for streaming outputs that are always (or almost always) valid, but only occasionally sampled. In general for such a module where data is being produced internally/periodically/from some external device like an ADC, the choice is either to discard new payloads if the currently 'valid' payload hasn't been transferred out, or discard the old payload and replace it with the new one. The module itself seems best placed to decide which approach to take, and either way it still meets all the ready/valid handshake rules with the other end. |
Defines a new Stream API - remove distinction between Source and Sink - make a StreamDescription class that can be used to document whether the Stream endpoint ignores the handshaking rules. These changes were made in light of comments on amaranth-lang/amaranth#317
@rroohhh You have convinced me that Source and Sink are not good first class concepts, and it would be better to have a single class name, with better names for variables to distinguish their role. Looking at examples such as LiteX's WishboneDMAWriter, renaming I rewrote the stream implementation that I have been working on in CFU-Playground, removing the distinction between It's currently using the name, @adamgreig @rroohhh I also thought about guarantees that endpoints should make regarding handshaking. I see the value in both loose guarantees (e.g the ADC input case or LiteX's BinaryActor) and firm guarantees, which aspertus-open-source/naps has found useful for finding bugs through formal verification. There are four qualities which seem important, any of which could be true or false: A given upstream downstream should only be allowed to connected if: (A or not B) AND (C or not D) In other words, if the upstream does not hold payload constant while valid is asserted, but the downstream relies on it, the stream should not be connected, and similarly if the downstream ignores valid when reading the payload and the upstream relies on the downstream respecting valid, then the streams should also not be connected. nap's formal_util.py - thanks for the pointer @rroohhh - checks A (The upstream holds I think it would be worthwhile allowing each stream endpoint to express which of the four properties it has, especially for streams used in common libraries. If, by default, all four properties are false then this represents the least restrictive case, and is the same as LiteX's current implementation. Codebases that rely on formal verification can set the properties as needed. I had an attempt at implementing A & C in the StreamDescription class, though I'm not happy with this, yet. |
Defines a new Stream API - remove distinction between Source and Sink - make a StreamDescription class that can be used to document whether the Stream endpoint ignores the handshaking rules. These changes were made in light of comments on amaranth-lang/amaranth#317 Signed-off-by: Alan Green <[email protected]>
Defines a new Stream API - remove distinction between Source and Sink - make a StreamDescription class that can be used to document whether the Stream endpoint ignores the handshaking rules. These changes were made in light of comments on amaranth-lang/amaranth#317 Signed-off-by: Alan Green <[email protected]>
Defines a new Stream API - remove distinction between Source and Sink - make a StreamDescription class that can be used to document whether the Stream endpoint ignores the handshaking rules. These changes were made in light of comments on amaranth-lang/amaranth#317 Signed-off-by: Alan Green <[email protected]>
Treating |
Superseded by #1244. |
There is a desire to have a generic stream abstraction in
nmigen.lib
which can be used / presented by thenmigen-stdio
cores (among others). This issue exists to capture discussion around the design of such an abstraction.Obvious places to look for inspiration here include AXI4-Stream and Avalon-ST. Wishbone, although common in OSHW designs, does not define a specific streaming mode and thus is quite complex for the streaming use case. It's also underspecified, so "Wishbone compatibility" is less meaningful than we might hope.
AXI4-Stream uses a simple ready-valid handshake for flow control, which is non-optional. It requires all transfers to be integer multiples of 8 bits, and supports both "packet" and "frame" abstractions for higher-level structure over the octet stream. It also supports sparse data streams and multiplexing multiple logical streams onto a single physical stream (see
TID
). All of these elements except for the "valid" half of the handshake are, technically speaking, optional. Anecdotally, I found the spec quite readable, although of course implementation may be another matter.Avalon-ST is quite similar to AXI4-Stream with the following notable differences:
ready
is asserted zero or more cycles before the sink is actually ready to accept data - and "ready allowance" - each assertion ofready
allows the source to send zero or more beats afterready
is deassertedAnecdotally, I found the Avalon-ST spec less readable than the AXI4-Stream spec, but only slightly.
My first-pass suggestion is a design much like Avalon-ST, but fixing the value of the
beatsPerCycle
attribute to 1 andreadyLatency
andreadyAllowance
to 0.The text was updated successfully, but these errors were encountered: