Regression Bug in AXI-Stream slave when upgrading from v4.0.8

[FranzForstmayr]

Hi,
I triggered a regression bug when upgrading VUnit to v4.2.0 from v4.0.8.
With git bisect I found the offinding commit 3e098f5, merged in #429 .

When using the axi-stream slave, the ready signal starts toggling.

Same issue is in the axi_stream_slave, although the tests are passing, just need approximately twice as long. The offending line is https://github.com/VUnit/vunit/blob/master/vunit/vhdl/verification_components/src/axi_stream_slave.vhd#L95, however I haven't digged deep enough to find out why this change was made.

[umarcor]

FTR, other changes to AXI-Stream a few days before #429, introduced another partial regression as explained in #494. Does GitHub allow to easily compare v4.0.8 and v4.2.0 of subdir https://github.com/VUnit/vunit/tree/master/vunit/vhdl/verification_components/src only?

[eschmidscs]

We've hit something similar some time ago.
You probably use the pop function with direct response variables or the check function with blocking=>True.
In this case, the slave takes one message, handles it and goes back to idle. Going back to idle causes ready to go low for one cycle.
You either have to use pop with references or check with blocking=>False.

That was different, probably before the merge you mentioned. But the process structure in the slave was changed.

[eine]

I created a synthetic example in https://github.com/eine/vunit/tree/axis/synthetic/examples/vhdl/array_axis_vcs/src/test, in an attempt to better understand this issue. According to previous comments here and in #673, the blocking behaviour is produced by pop_axi_stream in https://github.com/eine/vunit/blob/axis/synthetic/examples/vhdl/array_axis_vcs/src/test/tb_axis_loop.vhd#L113 (that is, by the API of the AXI Stream Slave verification component).

In #673, @FranzForstmayr proposed using a queue (https://github.com/VUnit/vunit/pull/673/files#diff-f2d4d6a3240eda80ddc8a14823d070f532d73aa493d7680a449d846445014477R113-R121), which implies using pop_axi_stream with references and an intermediate queue. That was suggested by Emanual above. Hence, Franz's solution does not really fix the issue, but works around it by introducing a "throughput" adapter. I.e., the queue is effectively a FIFO.

On the one hand, I am concerned about the apparent inconsistency between push_axi_stream and pop_axi_stream. The former does not introduce a cycle, while the latter does it. I think it is acceptable for both of them to introduce a cycle (should there be technical constraints that require it), but inconsistency is confusing. @olafvandenberg, since you implemented #429, do you mind providing some insight?

On the other hand, new_axi_stream_slave and new_axi_stream_master accept actor as an optional argument. This can be used for embedding queues into the verification components:

  constant master_axi_stream : axi_stream_master_t := new_axi_stream_master (data_length => data_width, actor => new_actor(inbox_size => 64, outbox_size => 64));
  constant slave_axi_stream  : axi_stream_slave_t  := new_axi_stream_slave  (data_length => data_width, actor => new_actor(inbox_size => 64, outbox_size => 64));

Therefore, using an external queue for using pop_axi_stream by reference feels non idiomatic to me. If necessary, that should be internal to the VC.

/cc @LarsAsplund

[olafvandenberg]

I'm not sure I fully understand the issue.
The pop_axi_stream with reference can be fully queued up, without any cycles in between.
When using the pop with a direct await reply, there is indeed a low cycle, so back-to-back transfers like this don't work I guess..
Is that the issue?

The changes (if I remember correctly) at that time were made to align to the clock input of the VC. Before the changes it could occur that the VC became asynchronous when using a wait for time message i think.
The wait for time I'm using a lot to generate randomized / bursty style of data traffic.

The queue being outside of the vc, was already the case I think in the original version.
It follows the same style as the stream slave vc, i guess if the different style (with passing the actor some sizes) is going to be used, it should be adapted in every vc that uses this style.

[eine]

I'm not sure I fully understand the issue.
The pop_axi_stream with reference can be fully queued up, without any cycles in between.
When using the pop with a direct await reply, there is indeed a low cycle, so back-to-back transfers like this don't work I guess..
Is that the issue?

The issue is the different behaviour of pop_axi_stream, depending on the overload:

• vunit/vunit/vhdl/verification_components/src/axi_stream_pkg.vhd

  Lines 256 to 261 in 710f68d

  	procedure pop_axi_stream(
  	signal net : inout network_t;
  	axi_stream : axi_stream_slave_t;
  	variable tdata : out std_logic_vector;
  	variable tlast : out std_logic
  	);

  forces a low cycle.

• vunit/vunit/vhdl/verification_components/src/axi_stream_pkg.vhd

  Lines 264 to 266 in 710f68d

  	procedure pop_axi_stream(signal net : inout network_t;
  	axi_stream : axi_stream_slave_t;
  	variable reference : inout axi_stream_reference_t);

  does not force a low cycle.

My understanding is that the two await_pop_axi_stream_reply variants are the ones that should introduce a low cycle:

• vunit/vunit/vhdl/verification_components/src/axi_stream_pkg.vhd

  Lines 269 to 279 in 710f68d

  	procedure await_pop_axi_stream_reply(
  	signal net : inout network_t;
  	variable reference : inout axi_stream_reference_t;
  	variable tdata : out std_logic_vector;
  	variable tlast : out std_logic;
  	variable tkeep : out std_logic_vector;
  	variable tstrb : out std_logic_vector;
  	variable tid : out std_logic_vector;
  	variable tdest : out std_logic_vector;
  	variable tuser : out std_logic_vector
  	);

• vunit/vunit/vhdl/verification_components/src/axi_stream_pkg.vhd

  Lines 281 to 286 in 710f68d

  	procedure await_pop_axi_stream_reply(
  	signal net : inout network_t;
  	variable reference : inout axi_stream_reference_t;
  	variable tdata : out std_logic_vector;
  	variable tlast : out std_logic
  	);

But the three pop_axi_stream should behave equally, and not introduce a low cycle.

So, the question is whether the different behaviour of pop_axi_stream corresponds to some limitation of how you implemented the stall feature, or was it an unwanted side effect.

[olafvandenberg]

Ok I get it..
It is an unwanted side effect of the change needed for the clock re-alignment.
I'll have a look today to see if I can think of a proper fix for it.

[olafvandenberg]

So I had a closer look, and I can't seem to reproduce this behavior.
There is a small glitch on ready if you do back-to-back transfers while using the blocking version of the pop_axi_stream procedure.
This we can fix easily by a small change, if necessary.
The low cycle I do see when there is no new message in the queue before the reply is send, so not truly back-to-back in that case.
For example:

for i in 3 to 14 loop
    if i = 14 then
          last := '1';
    end if;
    pop_axi_stream(  net, slave_axi_stream,
                     tdata => data,
                     tlast => last,
                     tkeep => keep,
                     tstrb => strb,
                     tid   => id,
                     tdest => dest,
                     tuser => user
                  );
end loop;

The code above will not give low cycles, only a small glitch on ready which can be removed by lowering ready after sending the reply and only if the queue is empty.

In the picture above, the first set of transfers is using the check_axi_stream, which is non-blocking.
The second set of transfers uses the blocking pop_axi_stream.

The code below does give a low cycle, but as stated is not truly back-to-back:

for i in 3 to 14 loop
        if i = 14 then
          last := '1';
        end if;
        pop_axi_stream(  net, slave_axi_stream,
                         tdata => data,
                         tlast => last,
                         tkeep => keep,
                         tstrb => strb,
                         tid   => id,
                         tdest => dest,
                         tuser => user
                      );

        wait for 1 ps;
      end loop;

Removing the glitches by moving the tready <= '0' in the axistreaming slave:

[eschmidscs]

Sorry for mixing in again here, but this looks really interesting. I am pretty sure that we did not have this behaviour here.
What simulator are you using?
And which testbench? Can I try to run that one here?
What happens if you replace the "wait for 1 ps" in the 50%-duty-cycle example with "wait for 0ps" (i.e. communication with vunit)?

[olafvandenberg]

I'm using Modelsim PE 10.7b.
For the test I did a small modification in the tb_axi_stream.vhd.
the test I modified is "test back-to-back passing check", I've simply copied the test in total, and replaced the check_axistream with a pop_axi_stream:

    elsif run("test back-to-back passing check") then
      wait until rising_edge(aclk);
      timestamp := now;

      last := '0';
      for i in 3 to 14 loop
        if i = 14 then
          last := '1';
        end if;
        push_axi_stream(net, master_axi_stream,
                        tdata => std_logic_vector(to_unsigned(i, 8)),
                        tlast => last,
                        tkeep => "1",
                        tstrb => "1",
                        tid => std_logic_vector(to_unsigned(42, id'length)),
                        tdest => std_logic_vector(to_unsigned(i+1, dest'length)),
                        tuser => std_logic_vector(to_unsigned(i*2, user'length)));
      end loop;

      last := '0';
      for i in 3 to 14 loop
        if i = 14 then
          last := '1';
        end if;
        check_axi_stream(net, slave_axi_stream,
                         expected => std_logic_vector(to_unsigned(i, 8)),
                         tlast => last,
                         tkeep => "1",
                         tstrb => "1",
                         tid => std_logic_vector(to_unsigned(42, id'length)),
                         tdest => std_logic_vector(to_unsigned(i+1, dest'length)),
                         tuser => std_logic_vector(to_unsigned(i*2, user'length)),
                         msg  => "check blocking",
                         blocking  => false);
      end loop;

      check_equal(now, timestamp, result(" setting up transaction stalled"));

      wait_until_idle(net, as_sync(slave_axi_stream));
      check_equal(now, timestamp + (12+1)*10 ns, " transaction time incorrect");

      -- Repeat while using blocking pop_axi_stream to check issue #573
      wait until rising_edge(aclk);
      timestamp := now;

      last := '0';
      for i in 3 to 14 loop
        if i = 14 then
          last := '1';
        end if;
        push_axi_stream(net, master_axi_stream,
                        tdata => std_logic_vector(to_unsigned(i, 8)),
                        tlast => last,
                        tkeep => "1",
                        tstrb => "1",
                        tid => std_logic_vector(to_unsigned(42, id'length)),
                        tdest => std_logic_vector(to_unsigned(i+1, dest'length)),
                        tuser => std_logic_vector(to_unsigned(i*2, user'length)));
      end loop;

      last := '0';
      for i in 3 to 14 loop
        if i = 14 then
          last := '1';
        end if;
        pop_axi_stream(  net, slave_axi_stream,
                         tdata => data,
                         tlast => last,
                         tkeep => keep,
                         tstrb => strb,
                         tid   => id,
                         tdest => dest,
                         tuser => user
                      );
      end loop;

      check_equal(now, timestamp, result(" setting up transaction stalled"));

      wait_until_idle(net, as_sync(slave_axi_stream));
      check_equal(now, timestamp + (12+1)*10 ns, " transaction time incorrect");

note that the check on time will fail, since the blocking pop will consume time..

[eine]

So I had a closer look, and I can't seem to reproduce this behavior.

Can you please try the array_axis_vcs example? The following waveform was generated with GHDL (python3 run.py -g) on Windows (MSYS2/MINGW64).

Note: that waveform also shows 5% stalls for both the master and the slave.

The modification in https://github.com/VUnit/vunit/pull/673/files#diff-f2d4d6a3240eda80ddc8a14823d070f532d73aa493d7680a449d846445014477 shows that the UUT can handle back-to-back transfers. However, the current example using a single for loop does introduce dead cycles.

The code above will not give low cycles, only a small glitch on ready which can be removed by lowering ready after sending the reply and only if the queue is empty.

Maybe this is a different behaviour between ModelSim and GHDL? So, where ModelSim is showing a glitch, GHDL shows a cycle?

Or is it because of https://github.com/VUnit/vunit/blob/master/examples/vhdl/array_axis_vcs/src/test/tb_axis_loop.vhd#L122 ?

[eschmidscs]

I tried to run the same test as @olafvandenberg in questasim 2019.2_2. The result is obviously 50%-DC:

This starts to be scary...

[eschmidscs]

... on the other hand: If the result is 100% or 50% DC depends on the loop exit in axi_stream_slave. It exits if the queue is empty - and then it waits for the next clock edge, causing 50% DC.
The question if the queue is empty depends on the message sequence. And this sequence depends in the end on delta-cycle order, which is AFAIK not specified by IEEE. So you end up with simulator dependent behaviour.