alexforencich / verilog-axis

Verilog AXI stream components for FPGA implementation
MIT License
722 stars 222 forks source link

AXIS Mux Missing first tvalid? #19

Open polyee13 opened 2 years ago

polyee13 commented 2 years ago

@alexforencich I ran stimulus (two AXI Stream interfaces) through the axis_mux.v module and it seems to be missing the tvalid on the first beat on the m_axis output. So, if i transmit send it a transaction of 16 beats, I only get 15 tvalid's out. tdata is correct though. Can you confirm this? axis_mux_missing_first_beat

polyee13 commented 2 years ago

@alexforencich The other issue I see is that if a change occurs on the select input, the first tdata of the next frame is still from s_axis[0] and not the selected s_axis stream and vice versa, when switched back to the original stream, the first beat of the output is from the previous stream. Please confirm this as well.

alexforencich commented 2 years ago

I need to see exactly how you're wiring things up. Both of these odd behaviors could be expected if the AXI stream interface is not properly connected, resulting in the handshaking protocol being violated. Part of your trace makes no sense - tready should not be continuously high on BOTH inputs at the same time. And exactly which module are you using, axis_mux or axis_arb_mux?

Gatherer commented 2 years ago

Just saw this one. Sorry to maybe to interfere but the capture do not make any sense. On bot s_axis_if

alexforencich commented 2 years ago

Oh, I missed that. Yep, this trace definitely make no sense. More information about the rest of the design is required to figure out what's actually going on here.

polyee13 commented 2 years ago

I'm currently not using my interfaces's (aclk and arstn), I drive a clk, and rst into your axis_mux module.

I agree with you on tready, but the s_axis[i].tready I'm driving that from my own version of the axi stream mux, which I configure to either provide no backpressure at all or backpressure.

Essentially I'm sending two axi streams to your "golden" module and my own module. Obviously, both modules can't drive tready. They both behave in the same manner (before correcting my version), so I know my verilog to systemverilog conversion is ok.

Below is how I'm connecting your module.

` axis_mux_ver

( . S_COUNT ( S_COUNT )

 , . DATA_WIDTH   ( DATA_WIDTH   )
 //, . KEEP_ENABLE  ( KEEP_ENABLE  )
 , . KEEP_WIDTH   ( KEEP_WIDTH   )
 , . ID_ENABLE   ( ID_ENABLED   )
 , . ID_WIDTH     ( ID_WIDTH     )
 , . DEST_ENABLE ( DEST_ENABLED )
 , . DEST_WIDTH   ( DEST_WIDTH   )
 , . USER_ENABLE ( USER_ENABLED )
 , . USER_WIDTH   ( USER_WIDTH   )
)
UUT_golden
( .clk
, .rst
, .s_axis_tdata   ({s_axis_if[1].tdata, s_axis_if[0].tdata})
, .s_axis_tkeep   ('1)
, .s_axis_tvalid  ({s_axis_if[1].tvalid, s_axis_if[0].tvalid})
, .s_axis_tready  ()
, .s_axis_tlast   ({s_axis_if[1].tlast, s_axis_if[0].tlast})
, .s_axis_tid     ({s_axis_if[1].tid, s_axis_if[0].tid})
, .s_axis_tdest   ({s_axis_if[1].tdest, s_axis_if[0].tdest})
, .s_axis_tuser   ('1)
, .m_axis_tdata   (m_axis_ver_if.tdata)
, .m_axis_tkeep   ()
, .m_axis_tvalid  (m_axis_ver_if.tvalid)
, .m_axis_tready  (1'b1)
, .m_axis_tlast   (m_axis_ver_if.tlast)
, .m_axis_tid     (m_axis_ver_if.tid)
, .m_axis_tdest   (m_axis_ver_if.tdest)
, .m_axis_tuser   ()
, .enable
, .select

);`

I'm able to get the expected behavior with a few tweaks to my version.

Here are some screenshots of your module vs. mine. The first screen shot is the first "packet" with select input equal to 0. Notice that I switch the select input in the middle of the packet to test the "frame-awareness" aspect of the mux. The 2nd screen shot is the 2nd packet. As you can see it's also missing the first tvalid on the 1st beat, and the value of tdata is from the other interface, not the selected interface.
Let me know if it's clear after viewing these screenshots.

axis_mux_compare_sel_eq0 axis_mux_compare_sel_eq1

alexforencich commented 2 years ago

You're violating the tready/tvalid handshaking protocol. Specifically, once you set tvalid = 1, you cannot change anything until tready = 1. So, in both of the traces that you just posted, you are dropping tvalid and changing tdata when tready = 0. You need to hold both of those signals high for one additional cycle.

polyee13 commented 2 years ago

Ok, I'm sure I would have noticed this by adding some formal properties to check the protocol, and with the mod to my own module, we would have caught it when we dropped it into the top level, which uses the axi protocol checker from Xilinx. So, this very likely a testbench issue I will fix this on my end. Thanks for clarifying. Let's close the Issue.

polyee13 commented 2 years ago

I think my use case for this axis mux is not applicable. I need to switch between two always active AXIS Streams (i..e no backpressure), but it's not necessarily a real-time switch in the middle of a frame. The select is either s_axis stream and static for the rest of the operation until the FPGA is reset. Therefore, I cannot throttle either AXI Stream subordinate interface, which is why in the first screen shot you saw both always ready, because I essentially tied both tready's to the m_axis tready.

alexforencich commented 2 years ago

That's a good point, quite a few of the cores in here are packet- or frame-oriented, with no configuration options to ignore tlast. I'll look in to adding an option to enable/disable framing support, I think it would be a pretty simple change for many of the modules. I also need to look in to supporting "interleaved" transfers, where the core might need to check both tlast and tid/tdest/etc.

polyee13 commented 2 years ago

Ok, I'll try and modify on my end (my svlog version) to see if I can get the expected behavior. I first have to fix my stimulus generation. Thanks again and keep up the good work.

milos-ponos commented 10 months ago

I have another problem with the implementation of your axis_mux @alexforencich. It drops m_axis_tvalid at the end of each frame which can significantly impact throughput for small frame sizes without any good reason. It seems just a bad design. Unfortunately, we discovered this the harder way.