-
Notifications
You must be signed in to change notification settings - Fork 10
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
Minimal stream interface #61
Conversation
fb43d18
to
bfcce17
Compare
bfcce17
to
ba92e3b
Compare
|
||
The protocol in this proposal and the Avalon-ST protocol are interoperable if the Avalon-ST interface is configured with parameters `readyLatency = readyAllowance = 0` and includes `data`, `ready`, and `valid` signals only (i.e. beats, packets, or channels are not used). In addition, `data` must be renamed to `payload` and a posedge clock domain must be used in Amaranth. | ||
|
||
The Avalon-ST protocol is widely used in the industry, especially in the Altera (Intel (Altera)) ecosystem, and the ability to define components interoperable with it is highly desirable. While it could be used as-is in the Amaranth ecosystem, it is valuable to reconsider whether the inclusion of some of the rarely used features is worth the support and compatibility burden, since all generic interconnect would have to be aware of them. Splitting of `data` into symbols or bytes seems particularly unnecessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💙
ad85a38
to
d6e0978
Compare
I've read through the RFC and it looks quite good to me. It captures well the simplest subset of AXI-Stream that is commonly used to move data around. I'm curious about why asserting valid combinatorially is allowed in the following:
Specially in combination with
The AXI specification (not AXI-Stream!), mentions the following in Section A3.1 Clock and Reset:
However, I checked the AXI-Stream specification and it is true that it doesn't include this restriction. This sentence does not appear in Section 2.8 Clock and Reset, nor anywhere else in the document. In this sense, AXI handshaking is slightly more strict that AXI-Stream handshaking (I learned this today, as I thought that AXI-Stream also forbade combinatorial paths). I think I've never seen an AXI-Stream consumer that makes TREADY depend combinatorially on TVALID. In any case, I think that for this RFC allowing |
I've asked around and I got a number of responses indicating that interconnect which allocates buffer space when an input stream becomes ready may need to do it combinatorially. That seemed like a good reason to allow it. Compatibility with AXI-Stream is also, in itself, fairly compelling, as it is by far the most popular prior art. |
|
d6e0978
to
ddd83ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this lays good groundwork for a useful streams abstraction.
text/0061-minimal-streams.md
Outdated
4. Once the transmitter asserts `valid`, it must not change the contents of `payload` until a transfer is performed. | ||
5. The transmitter must not assert `valid` in response to the receiver asserting `ready` (combinatorially or not). | ||
6. Once the receiver asserts `ready` it may deassert it at any time. | ||
7. The receiver may assert `ready` in response to the transmitter asserting `valid` (including combinatorially). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found rules 5 and 7 a little difficult to parse, although I believe I understand the intent. Some possible alternate formulations:
5. The transmitter must not condition the assertion of valid
on the state of ready
from the receiver
7. The receiver may condition the state of ready
on the state of valid
(or the contents of payload
)(including combinatorially)
(In addition to the previously discussed case of buffer allocation, another case where ready may depend on both valid and data would be a block implementing routing or demultiplexing based on a tdest
type signal in the payload)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Condition the assertion" and "condition the state" are more confusing to me (introduces a new term used nowhere else in the spec; also I don't generally recall reading a lot of text using this terminology).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AXI-Stream uses this wording:
A Transmitter is not permitted to wait until TREADY is asserted before asserting TVALID. Once TVALID is
asserted, it must remain asserted until the handshake occurs.A Receiver is permitted to wait for TVALID to be asserted before asserting TREADY. It is permitted that a
Receiver asserts and deasserts TREADY without TVALID being asserted.
Maybe "wait for" is clearer than "in response to" or "condition". The main reason why these conditions are in place is to avoid a deadlock if the producer waits for ready
and the consumer waits for valid
, so one of them is not supposed to wait potentially indefinitely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And while thinking more about this, I realize that maybe the current wording of 5 is perhaps not what is wanted here. In AXI-Stream it would be perfectly fine for a producer to assert valid
in response to seeing ready
asserted, provided that the producer also has a way in which it is guaranteed to assert valid
even if the consumer never asserts ready
. Is this the rule that is intended here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the rule that is intended here?
The intent is to exclude (a) comb feedback of valid
from ready
(which results in not-well-formed programs) and (b) in general valid
being dependent on ready
(which results in deadlocks).
I agree that these two are separate and would have to think about it more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case I think that your last version 1c92187 forbids combinatorial loops clearly enough, so the things I'm wondering are more about AXI-Stream than about this RFC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why the spec would forbid this, …
The issue is that if you have a source that under some conditions waits for ready
before setting valid
and a sink that under some conditions waits for valid
before setting ready
and connect them to each other, you've got a potential for a deadlock when they both come into the condition where they're waiting for the other end to make the first move.
To eliminate the possibility of deadlock, one of these must be forbidden, and having valid
depend on the state of ready
is the less useful one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zyp I think @daniestevez's example doesn't have a deadlock. In it, the transmitter always makes forward progress, just at different speed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah. I can't come up with a practical use for code that resembles the example, but since it neither has potential for deadlocks nor combinatorial loops, I don't see a specific reason to disallow it.
Any form of feedback from
ready
tovalid
is prohibited.
This wording is too strict, as deassertion of valid
very much depends on feedback from ready
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah. I can't come up with a practical use for code that resembles the example, but since it neither has potential for deadlocks nor combinatorial loops, I don't see a specific reason to disallow it.
I think having streams be push-only makes understanding the systems they are a part of easier, which to me is significant here.
Any form of feedback from
ready
tovalid
is prohibited.This wording is too strict, as _de_assertion of
valid
very much depends on feedback fromready
.
Fixed, thanks. Now it's just the posedge that needs to happen without feedback.
[omitted-features]: #omitted-features | ||
|
||
This proposal intentionally omits the following features: | ||
1. packetization (often implemented using `first` and/or `last` control signals) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's wrong to exclude this, but I do think that it is an important feature to get to quickly. Many use cases need packetizetion, and if it's not provided may invent their own version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An issue that I discovered is that there's no consensus on even the structure of trivial packets. Some people want first
only, some last
only, some both first
and last
. And this is before going into interaction of packetization with other features.
I'm actually fairly OK with people inventing their own versions if we can't all agree on a common protocol anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the fact that AXI-Stream uses last
is a strong case for using last
. I also find it easier to deal with than first
. If the protocol has last
, then when you see it asserted, you usually must reset things for the start of the next packet, and you can do it sequentially. A first
, instead, usually forces you to deal with this situation combinationally, or to stop processing for one cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the fact that AXI-Stream uses
last
is a strong case for usinglast
.
Not really. It is important to remain interoperable to some extent with existing protocols, yes, but the fact that AXI-Stream did something is not in itself a motivation for us doing something the same way. There are plenty of decisions in AMBA we should absolutely not replicate, e.g. the byte-oriented nature of TDATA is a big one.
If the protocol has
last
, then when you see it asserted, you usually must reset things for the start of the next packet, and you can do it sequentially. Afirst
, instead, usually forces you to deal with this situation combinationally, or to stop processing for one cycle.
Other people (@zyp) argued for allowing first
without last
for cases where you don't know if you have a last byte, e.g. COBS. This is what I'm referring to as a lack of consensus on what the protocol should be.
1c92187
to
4e4db25
Compare
We have discussed this RFC on the 2024-03-25 core subsystem meeting. The disposition was to merge. |
4e4db25
to
f6b41f4
Compare
Rendered