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

Axi4Stream: userWidth tied to dataWidth #1252

Open
oletf opened this issue Nov 27, 2023 · 16 comments
Open

Axi4Stream: userWidth tied to dataWidth #1252

oletf opened this issue Nov 27, 2023 · 16 comments

Comments

@oletf
Copy link
Contributor

oletf commented Nov 27, 2023

I'm using a Vivado IP with AXI4Stream interface I have set up like this :
image

In my case my io is defined as :

val m_axis  = master(Axi4Stream(Axi4StreamConfig(dataWidth = 3, useLast = true)))

I can currently ignore the user bits and keep them unconnected, so it's not a problem, but if I were to need to use them,
I have no way to build a config which matches the IP data and user width,
as Axi4StreamBundle defines here :

user = Bits(config.userWidth*config.dataWidth bit)

while the docstring mentions :

dataWidth Width of the bus in BYTES
userWidth Width of the User field in bits

but in the end user's actual width ends up being dataWidth*userWidth bits
the userbit adaptation also uses dataWidth here,
it makes sense together if the dataWidth*userWidth bits size is actually needed,
but I don't get why it's the case.

Is there an explicit dataWidth to userWidth ratio requirement in the Axi4 specification,
or it's some kind of refactor typo ?

I have made a minimal change here : oletf@6313695, which made sense to me,
I can PR if this change makes sense and I didn't miss any important part related to AXI4Stream specification.

@likewise
Copy link
Collaborator

likewise commented Nov 27, 2023

The width of the TUSER (user) field can be any number of bits, according to the AMBA AXI 4 Stream specification.

Thus there is no implicit correlation between the data width and the user width.

Quoting the specification: TUSER width. Recommended number of bits is an integer multiple of the width of the interface in bytes.(https://documentation-service.arm.com/static/642583d7314e245d086bc8c9?token=)

The data width is typically specified in bytes; the specification requires this to be integer multiple of bytes.

In case we only want to allow the recommendation, the user width is a multiple integer number of bits for each byte in the data width.

Ideally though, we allow the number of user bits to be anything.

@oletf
Copy link
Contributor Author

oletf commented Nov 27, 2023

ok so no I just got to an IP which has this :
image

So maybe it should be better to keep userWidth as bits,
keep the that.config.userWidth <= this.config.userWidth assert,
let the subdivideIn make slices being bits, and let the pairing make sure that.config.userWidth is a multiple of this.config.userWidth ?

@oletf
Copy link
Contributor Author

oletf commented Nov 27, 2023

Thanks a lot for the clarification @likewise,
so as I get it,
the current choice has been to only allow the recommendation then.

Would an additional Boolean like userWidthIntMultiple true as default, to keep current code fully compatible,
but which could allow to opt-out of the recommendation for cases like the one in my first message (being 3:2),
be conceivable ?

@andreasWallner
Copy link
Collaborator

I'd be for allowing an arbitrary number of bits, I have seen a bunch of components that did not conform to the ARM recommendation.
If we allow something that is not an integer multiple of the data width then I don't see a reason why we'd limit it to bytes?

W.r.t. connecting: we could pre-pad with zeros in case the master is narrower then the slave, and create an error in case the master is too wide for the slave? (a user can then still manually resize if they know it's ok to throw bits away?)

The only question: how to do the change w/o breaking older code?

@likewise
Copy link
Collaborator

For user bits, I totally agree.

And yes, we have the problem that the current components are not spec compliant, so we either break old code or keep being incompatible with the spec.

I am not sure if there is a way to fix this. Maybe we can first change the implementation to deal with arbitrary user widths also, then change the API with new signatures?

I think the spec has higher priority and our implementation is incompliant.

(I have not used the SpinalHDL AXI Streaming components from the library for reasons like this.)

(I would even consider to allow for an arbitrary number of bits in the data width. For me it makes perfect sense to stream 12-bit video or image samples across AXI Stream.)

@MrLiujiefei
Copy link

I've had the same problem, what time can update !

Dolu1990 added a commit that referenced this issue Jan 22, 2024
@Dolu1990
Copy link
Member

TUSER width. Recommended number of bits is an integer multiple of the width of the interface in bytes.

I think the reason why they recommend this is to allow width adapter to also "adapt" the user signal in the same way than they adapt the byte.

So the user signal having data for each byte individualy.

What is the purpose of the user signal in practice ?

(I would even consider to allow for an arbitrary number of bits in the data width. For me it makes perfect sense to stream 12-bit video or image samples across AXI Stream.)

AxiStream is realy realy realy much byte oriented, maybe instead of having an arbitrary number of data bits, the best would be to have an arbitrary parametrable bits per "byte", meaning "hoo one byte is now 12 bits". This would allow to keep all the "byte" related stuff consistant in for the bridges and adapters.

@Readon
Copy link
Collaborator

Readon commented Jan 28, 2024

One possible way is to define an alternative interface which accept BitCount but Int as parameter, so that there definition could be different and remains the old code works either.

On other hand, the width itself is better to be defined as BitCount though.

@Readon
Copy link
Collaborator

Readon commented Jan 28, 2024

Or even more, we could have a user define Bundle for user data which could be placed at the wires that userWidth originaly specified.

@Dolu1990
Copy link
Member

Hmm
Fondamentaly, there could be 2 kinds of user data :

  • The ones which are per byte, which need to be handled by the width adapter properly (mux)
  • The ones which are per transaction, which pass through the width adapter without any alteration

So, maybe the best would just be to have an additional signal for the "per transaction" user data. That would make things very clear.
So yes, something as Readon, adding another signal. But not sure if that should be a Bundle or just an Bits signal (enforcing people to specify a type parameter vs the few time this feature is used may be counter productive) ?

@dokleina
Copy link
Contributor

I might be too late to add an opinion here…

Would it make sense to add some kind of metadata to AXI Streams to signal how User data should be handled? This could be an a trait or something applied to an AXI Stream to indicate consumers should interact or adapt User data.

I could come up with a working example if there’s any interest.

@Dolu1990
Copy link
Member

@dokleina

Would it make sense to add some kind of metadata to AXI Streams to signal how User data should be handled? This could be an a trait or something applied to an AXI Stream to indicate consumers should interact or adapt User data.

Saying for instance "this range of user data is byte related" "this range of user data is transaction related" ?

@dokleina
Copy link
Contributor

@Dolu1990

Yeah, something like that.

I was thinking an AXI Stream's User data interactions could be grouped into two categories:

  • Propagation: Transfer level, packet level, and any other conceivable modes.
  • Association: Data byte association (as the AXI Stream spec recommends), or lack of association.

A stream could mix any Propagation and Association together: e.g., Transfer-level with an independent width.

Components like the Axi4StreamWidthAdapter would then consider the Association of the stream to know if User data and Data byte association needs to be preserved.

A resolution strategy would need to be specified for mismatched Association and Propagation types. I'm not sure how that would look.

@Dolu1990
Copy link
Member

hmm
I feel like the simplest would just be to have the two signals, one for Data byte association and one for transaction level. that would be backward compatible and just "simple" ? I'm afraid that otherwise it may make things quite verbose / complicated ?

@dokleina
Copy link
Contributor

I look a look at your attempt commit and I like where it's going.

When you say "two signals" it looks like you meant "two parameters"?

I'd just like to be able to query Axi4StreamConfig for whether its User data is per byte or independently sized unambiguously.

@Dolu1990
Copy link
Member

Dolu1990 commented Mar 4, 2024

My attempt isn't good.
Now i mean, two parameters and two signals. No more merge of any sort

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

No branches or pull requests

7 participants