rfnoc: Clarify usage of MTU vs. max payload size, remove DEFAULT_SPP

These two values where being mixed up in the code. To summarize:
- The MTU is the max CHDR packet size, including header & timestamp.
- The max payload is the total number of bytes regular payload plus
  metadata that can be fit into into a CHDR packet. It is strictly
  smaller than the MTU. For example, for 64-bit CHDR widths, if
  a timestamp is desired, the max payload is 16 bytes smaller than
  the MTU.

The other issue was that we were using a magic constant (DEFAULT_SPP)
which was causing conflicts with MTUs and max payloads.
This constant was harmful in multiple ways:
- The explanatory comment was incorrect (it stated it would cap packets
  to 1500 bytes, which it didn't)
- It imposed random, hardcoded values that interfered with an 'spp
  discovery', i.e., the ability to derive a good spp value from MTUs
- The current value capped packet sizes to 8000 bytes CHDR packets, even
  when we wanted to use bigger ones

This patch changes the following:
- noc_block_base now has improved docs for MTU, and additional APIs
  (get_max_payload_size(), get_chdr_hdr_len()) which return the
  current payload size given MTU and CHDR width, and the CHDR header
  length.
- The internally used graph nodes for TX and RX streamers also get
  equipped with the same new two API calls.
- The radio, siggen, and replay block all where doing different
  calculations for their spp/ipp values. Now, they all use the max
  payload value to calculate spp/ipp. Unit tests where adapted
  accordingly. Usage of DEFAULT_SPP was removed.
- The replay block used a hardcoded 16 bytes for header lengths, which
  was replaced by get_chdr_hdr_len()
- The TX and RX streamers where discarding the MTU value and using the
  max payload size as the MTU, which then propagated throughout the
  graph. Now, both values are stored and can be used where appropriate.

8 files changed, 98 insertions, 56 deletions

diff --git a/host/lib/rfnoc/chdr_rx_data_xport.cpp b/host/lib/rfnoc/chdr_rx_data_xport.cpp
index 7089c9071..37b3ffb89 100644
--- a/host/lib/rfnoc/chdr_rx_data_xport.cpp
+++ b/host/lib/rfnoc/chdr_rx_data_xport.cpp
@@ -5,6 +5,7 @@
 //
 
 #include <uhd/rfnoc/chdr_types.hpp>
+#include <uhd/utils/log.hpp>
 #include <uhdlib/rfnoc/chdr_rx_data_xport.hpp>
 #include <uhdlib/rfnoc/mgmt_portal.hpp>
 #include <uhdlib/rfnoc/rfnoc_common.hpp>
@@ -39,6 +40,7 @@ chdr_rx_data_xport::chdr_rx_data_xport(uhd::transport::io_service::sptr io_srv,
     const fc_params_t& fc_params,
     disconnect_callback_t disconnect)
     : _fc_state(epids, fc_params.freq)
+    , _mtu(recv_link->get_recv_frame_size())
     , _fc_sender(pkt_factory, epids)
     , _epid(epids.second)
     , _chdr_w_bytes(chdr_w_to_bits(pkt_factory.get_chdr_w()) / 8)
@@ -52,10 +54,9 @@ chdr_rx_data_xport::chdr_rx_data_xport(uhd::transport::io_service::sptr io_srv,
     _recv_packet_cb = pkt_factory.make_generic();
     _fc_sender.set_capacity(fc_params.buff_capacity);
 
-    // Calculate max payload size
-    const size_t pyld_offset =
-        _recv_packet->calculate_payload_offset(chdr::PKT_TYPE_DATA_WITH_TS);
-    _max_payload_size = recv_link->get_recv_frame_size() - pyld_offset;
+    // Calculate header size
+    _hdr_len = _recv_packet->calculate_payload_offset(chdr::PKT_TYPE_DATA_WITH_TS);
+    UHD_ASSERT_THROW(_hdr_len);
 
     // Make data transport
     auto recv_cb =
diff --git a/host/lib/rfnoc/chdr_tx_data_xport.cpp b/host/lib/rfnoc/chdr_tx_data_xport.cpp
index 7e926a163..618fffe5a 100644
--- a/host/lib/rfnoc/chdr_tx_data_xport.cpp
+++ b/host/lib/rfnoc/chdr_tx_data_xport.cpp
@@ -5,6 +5,7 @@
 //
 
 #include <uhd/rfnoc/chdr_types.hpp>
+#include <uhd/utils/log.hpp>
 #include <uhdlib/rfnoc/chdr_tx_data_xport.hpp>
 #include <uhdlib/rfnoc/mgmt_portal.hpp>
 #include <uhdlib/rfnoc/rfnoc_common.hpp>
@@ -34,6 +35,7 @@ chdr_tx_data_xport::chdr_tx_data_xport(uhd::transport::io_service::sptr io_srv,
     const fc_params_t fc_params,
     disconnect_callback_t disconnect)
     : _fc_state(fc_params.buff_capacity)
+    , _mtu(send_link->get_send_frame_size())
     , _fc_sender(pkt_factory, epids)
     , _epid(epids.first)
     , _chdr_w_bytes(chdr_w_to_bits(pkt_factory.get_chdr_w()) / 8)
@@ -48,10 +50,9 @@ chdr_tx_data_xport::chdr_tx_data_xport(uhd::transport::io_service::sptr io_srv,
     _send_packet = pkt_factory.make_generic();
     _recv_packet = pkt_factory.make_generic();
 
-    // Calculate max payload size
-    const size_t pyld_offset =
-        _send_packet->calculate_payload_offset(chdr::PKT_TYPE_DATA_WITH_TS);
-    _max_payload_size = send_link->get_send_frame_size() - pyld_offset;
+    // Calculate header length
+    _hdr_len = _send_packet->calculate_payload_offset(chdr::PKT_TYPE_DATA_WITH_TS);
+    UHD_ASSERT_THROW(_hdr_len);
 
     // Now create the send I/O we will use for data
     auto send_cb = [this](buff_t::uptr buff, transport::send_link_if* send_link) {
diff --git a/host/lib/rfnoc/noc_block_base.cpp b/host/lib/rfnoc/noc_block_base.cpp
index c2db597cb..9719e739b 100644
--- a/host/lib/rfnoc/noc_block_base.cpp
+++ b/host/lib/rfnoc/noc_block_base.cpp
@@ -316,6 +316,21 @@ size_t noc_block_base::get_mtu(const res_source_info& edge)
     return _mtu.at(edge);
 }
 
+size_t noc_block_base::get_chdr_hdr_len(const bool account_for_ts) const
+{
+    const size_t header_len_bytes = chdr_w_to_bits(_chdr_w) / 8;
+    // 64-bit CHDR requires two lines for the header if we use a timestamp,
+    // everything else requires one line
+    const size_t num_hdr_lines = (account_for_ts && _chdr_w == CHDR_W_64) ? 2 : 1;
+    return header_len_bytes * num_hdr_lines;
+}
+
+size_t noc_block_base::get_max_payload_size(
+    const res_source_info& edge, const bool account_for_ts)
+{
+    return get_mtu(edge) - get_chdr_hdr_len(account_for_ts);
+}
+
 property_base_t* noc_block_base::get_mtu_prop_ref(const res_source_info& edge)
 {
     for (size_t mtu_prop_idx = 0; mtu_prop_idx < _mtu_props.size(); mtu_prop_idx++) {
diff --git a/host/lib/rfnoc/radio_control_impl.cpp b/host/lib/rfnoc/radio_control_impl.cpp
index 3c4c28d11..697bb2549 100644
--- a/host/lib/rfnoc/radio_control_impl.cpp
+++ b/host/lib/rfnoc/radio_control_impl.cpp
@@ -139,8 +139,11 @@ radio_control_impl::radio_control_impl(make_args_ptr make_args)
     _type_in.reserve(get_num_input_ports());
     _type_out.reserve(get_num_output_ports());
     for (size_t chan = 0; chan < get_num_output_ports(); ++chan) {
+        // Default SPP is the maximum value we can fit through the edge, given our MTU
+        const int default_spp =
+            get_max_spp(get_max_payload_size({res_source_info::OUTPUT_EDGE, chan}));
         _spp_prop.push_back(
-            property_t<int>(PROP_KEY_SPP, DEFAULT_SPP, {res_source_info::USER, chan}));
+            property_t<int>(PROP_KEY_SPP, default_spp, {res_source_info::USER, chan}));
         _samp_rate_in.push_back(property_t<double>(
             PROP_KEY_SAMP_RATE, get_tick_rate(), {res_source_info::INPUT_EDGE, chan}));
         _samp_rate_out.push_back(property_t<double>(
@@ -166,17 +169,15 @@ radio_control_impl::radio_control_impl(make_args_ptr make_args)
             {&_spp_prop.back()},
             [this, chan, &spp = _spp_prop.back()]() {
                 RFNOC_LOG_TRACE("Calling resolver for spp@" << chan);
-                // MTU is max payload size, header with timestamp is already
-                // accounted for
-                const int mtu =
-                    static_cast<int>(get_mtu({res_source_info::OUTPUT_EDGE, chan}));
-                const int mtu_samps       = mtu / (_samp_width / 8);
-                const int max_spp_per_mtu = mtu_samps - (mtu_samps % _spc);
-                if (spp.get() > max_spp_per_mtu) {
-                    RFNOC_LOG_DEBUG("spp value " << spp.get() << " exceeds MTU of "
-                                                   << mtu << "! Coercing to "
-                                                   << max_spp_per_mtu);
-                    spp = max_spp_per_mtu;
+                const size_t max_pyld =
+                    get_max_payload_size({res_source_info::OUTPUT_EDGE, chan});
+                const int max_spp = get_max_spp(max_pyld);
+                if (spp.get() > max_spp) {
+                    RFNOC_LOG_DEBUG("spp value "
+                                    << spp.get() << " exceeds MTU of "
+                                    << get_mtu({res_source_info::OUTPUT_EDGE, chan})
+                                    << "! Coercing to " << max_spp);
+                    spp = max_spp;
                 }
                 if (spp.get() % _spc) {
                     spp = spp.get() - (spp.get() % _spc);
@@ -185,7 +186,7 @@ radio_control_impl::radio_control_impl(make_args_ptr make_args)
                         << spp.get());
                 }
                 if (spp.get() <= 0) {
-                    spp = DEFAULT_SPP;
+                    spp = max_spp;
                     RFNOC_LOG_WARNING(
                         "spp must be greater than zero! Coercing to " << spp.get());
                 }
@@ -198,7 +199,6 @@ radio_control_impl::radio_control_impl(make_args_ptr make_args)
                 chan,
                 &samp_rate_in  = _samp_rate_in.at(chan),
                 &samp_rate_out = _samp_rate_out.at(chan)]() {
-                const auto UHD_UNUSED(log_chan) = chan;
                 RFNOC_LOG_TRACE("Calling resolver for samp_rate@" << chan);
                 samp_rate_in  = coerce_rate(samp_rate_in.get());
                 samp_rate_out = samp_rate_in.get();
@@ -1109,3 +1109,9 @@ void radio_control_impl::async_message_handler(
                 boost::format("Received async message to invalid addr 0x%08X!") % addr));
     }
 }
+
+int radio_control_impl::get_max_spp(const size_t bytes)
+{
+    const int packet_samps = bytes / (_samp_width / 8);
+    return packet_samps - (packet_samps % _spc);
+}
diff --git a/host/lib/rfnoc/replay_block_control.cpp b/host/lib/rfnoc/replay_block_control.cpp
index e2d2523da..79e7446e4 100644
--- a/host/lib/rfnoc/replay_block_control.cpp
+++ b/host/lib/rfnoc/replay_block_control.cpp
@@ -124,7 +124,7 @@ public:
             _replay_reg_iface.poke64(
                 REG_PLAY_BUFFER_SIZE_LO_ADDR, _play_size.at(port).get(), port);
             _replay_reg_iface.poke32(REG_PLAY_WORDS_PER_PKT_ADDR,
-                (_packet_size.at(port).get() - CHDR_MAX_LEN_HDR) / _word_size,
+                (_packet_size.at(port).get() - get_chdr_hdr_len()) / _word_size,
                 port);
         }
     }
@@ -226,7 +226,7 @@ public:
 
     uint32_t get_max_items_per_packet(const size_t port) const override
     {
-        return (_packet_size.at(port).get() - CHDR_MAX_LEN_HDR)
+        return (_packet_size.at(port).get() - get_chdr_hdr_len())
                / get_play_item_size(port);
     }
 
@@ -274,7 +274,7 @@ public:
 
     void set_max_items_per_packet(const uint32_t ipp, const size_t port) override
     {
-        set_max_packet_size(CHDR_MAX_LEN_HDR + ipp * get_play_item_size(port), port);
+        set_max_packet_size(get_chdr_hdr_len() + ipp * get_play_item_size(port), port);
     }
 
     void set_max_packet_size(const uint32_t size, const size_t port) override
@@ -460,17 +460,26 @@ private:
 
     void _set_packet_size(const uint32_t packet_size, const size_t port)
     {
-        // MTU is max payload size, header with timestamp is already accounted for
         const size_t mtu               = get_mtu({res_source_info::OUTPUT_EDGE, port});
-        const uint32_t item_size       = get_play_item_size(port);
-        const uint32_t mtu_payload     = mtu - CHDR_MAX_LEN_HDR;
-        const uint32_t mtu_items       = mtu_payload / item_size;
-        const uint32_t ipc             = _word_size / item_size; // items per cycle
-        const uint32_t max_ipp_per_mtu = mtu_items - (mtu_items % ipc);
-        const uint32_t payload_size    = packet_size - CHDR_MAX_LEN_HDR;
-        uint32_t ipp                   = payload_size / item_size;
-        if (ipp > max_ipp_per_mtu) {
-            ipp = max_ipp_per_mtu;
+        uint32_t requested_packet_size = packet_size;
+        if (requested_packet_size > mtu) {
+            requested_packet_size = mtu;
+            RFNOC_LOG_WARNING("Requested packet size exceeds MTU! Coercing to "
+                              << requested_packet_size);
+        }
+        const size_t max_payload_bytes =
+            get_max_payload_size({res_source_info::OUTPUT_EDGE, port});
+        const uint32_t item_size = get_play_item_size(port);
+        const uint32_t ipc       = _word_size / item_size; // items per cycle
+        const uint32_t max_items = max_payload_bytes / item_size;
+        const uint32_t max_ipp   = max_items - (max_items % ipc);
+        const uint32_t requested_payload_size =
+            requested_packet_size - (mtu - max_payload_bytes);
+        uint32_t ipp = requested_payload_size / item_size;
+        if (ipp > max_ipp) {
+            RFNOC_LOG_DEBUG("ipp value " << ipp << " exceeds MTU of " << mtu
+                                         << "! Coercing to " << max_ipp);
+            ipp = max_ipp;
         }
         if ((ipp % ipc) != 0) {
             ipp = ipp - (ipp % ipc);
@@ -478,7 +487,7 @@ private:
                 "ipp must be a multiple of the block bus width! Coercing to " << ipp);
         }
         if (ipp <= 0) {
-            ipp = DEFAULT_SPP;
+            ipp = max_ipp;
             RFNOC_LOG_WARNING("ipp must be greater than zero! Coercing to " << ipp);
         }
         // Packet size must be a multiple of word size
diff --git a/host/lib/rfnoc/rfnoc_rx_streamer.cpp b/host/lib/rfnoc/rfnoc_rx_streamer.cpp
index 7acdf357d..d57b8aab2 100644
--- a/host/lib/rfnoc/rfnoc_rx_streamer.cpp
+++ b/host/lib/rfnoc/rfnoc_rx_streamer.cpp
@@ -164,11 +164,15 @@ void rfnoc_rx_streamer::connect_channel(
 {
     UHD_ASSERT_THROW(channel < _mtu_in.size());
 
-    // Update MTU property based on xport limits
-    const size_t mtu = xport->get_max_payload_size();
-    set_property<size_t>(PROP_KEY_MTU, mtu, {res_source_info::INPUT_EDGE, channel});
+    // Stash away the MTU before we lose access to xports
+    const size_t mtu = xport->get_mtu();
 
     rx_streamer_impl<chdr_rx_data_xport>::connect_channel(channel, std::move(xport));
+
+    // Update MTU property based on xport limits. We need to do this after
+    // connect_channel(), because that's where the chdr_rx_data_xport object
+    // learns its header size.
+    set_property<size_t>(PROP_KEY_MTU, mtu, {res_source_info::INPUT_EDGE, channel});
 }
 
 void rfnoc_rx_streamer::_register_props(const size_t chan, const std::string& otw_format)
diff --git a/host/lib/rfnoc/rfnoc_tx_streamer.cpp b/host/lib/rfnoc/rfnoc_tx_streamer.cpp
index b4aea202d..969c41ae6 100644
--- a/host/lib/rfnoc/rfnoc_tx_streamer.cpp
+++ b/host/lib/rfnoc/rfnoc_tx_streamer.cpp
@@ -125,9 +125,8 @@ void rfnoc_tx_streamer::connect_channel(
 {
     UHD_ASSERT_THROW(channel < _mtu_out.size());
 
-    // Update MTU property based on xport limits
-    const size_t mtu = xport->get_max_payload_size();
-    set_property<size_t>(PROP_KEY_MTU, mtu, {res_source_info::OUTPUT_EDGE, channel});
+    // Stash away the MTU before we lose access to xports
+    const size_t mtu = xport->get_mtu();
 
     xport->set_enqueue_async_msg_fn(
         [this, channel](
@@ -145,6 +144,11 @@ void rfnoc_tx_streamer::connect_channel(
         });
 
     tx_streamer_impl<chdr_tx_data_xport>::connect_channel(channel, std::move(xport));
+
+    // Update MTU property based on xport limits. We need to do this after
+    // connect_channel(), because that's where the chdr_tx_data_xport object
+    // learns its header size.
+    set_property<size_t>(PROP_KEY_MTU, mtu, {res_source_info::OUTPUT_EDGE, channel});
 }
 
 bool rfnoc_tx_streamer::recv_async_msg(
diff --git a/host/lib/rfnoc/siggen_block_control.cpp b/host/lib/rfnoc/siggen_block_control.cpp
index 2628d66ab..8dcc68319 100644
--- a/host/lib/rfnoc/siggen_block_control.cpp
+++ b/host/lib/rfnoc/siggen_block_control.cpp
@@ -154,9 +154,12 @@ private:
                 PROP_KEY_CONSTANT_Q, 1.0, {res_source_info::USER, port}});
             _prop_phase_inc.emplace_back(property_t<double>{
                 PROP_KEY_SINE_PHASE_INC, 1.0, {res_source_info::USER, port}});
+            const int default_spp =
+                static_cast<int>(
+                    get_max_payload_size({res_source_info::OUTPUT_EDGE, port}))
+                / uhd::convert::get_bytes_per_item(_prop_type_out.at(port).get());
             _prop_spp.emplace_back(property_t<int>{
-                PROP_KEY_SPP, DEFAULT_SPP, {res_source_info::USER, port}});
-
+                PROP_KEY_SPP, default_spp, {res_source_info::USER, port}});
             register_property(&_prop_enable.back(), [this, port]() {
                 _siggen_reg_iface.poke32(REG_ENABLE_OFFSET,
                     uint32_t(_prop_enable.at(port).get() ? 1 : 0),
@@ -256,21 +259,20 @@ private:
                     get_mtu_prop_ref({res_source_info::OUTPUT_EDGE, port})},
                 {&_prop_spp.back()},
                 [this, port]() {
-                    // MTU is max payload size, header with timestamp is already
-                    // accounted for
-                    int spp = _prop_spp.at(port).get();
-                    const int mtu =
-                        static_cast<int>(get_mtu({res_source_info::OUTPUT_EDGE, port}));
-                    const int mtu_samps =
-                        mtu
+                    int spp               = _prop_spp.at(port).get();
+                    const int max_payload = static_cast<int>(
+                        get_max_payload_size({res_source_info::OUTPUT_EDGE, port}));
+                    const int max_samps =
+                        max_payload
                         / uhd::convert::get_bytes_per_item(_prop_type_out.at(port).get());
-                    if (spp > mtu_samps) {
-                        RFNOC_LOG_WARNING("spp value " << spp << " exceeds MTU of " << mtu
-                                                       << "! Coercing to " << mtu_samps);
-                        spp = mtu_samps;
+                    if (spp > max_samps) {
+                        RFNOC_LOG_WARNING("spp value " << spp << " exceeds MTU of "
+                                                       << max_payload << "! Coercing to "
+                                                       << max_samps);
+                        spp = max_samps;
                     }
                     if (spp <= 0) {
-                        spp = DEFAULT_SPP;
+                        spp = max_samps;
                         RFNOC_LOG_WARNING(
                             "spp must be greater than zero! Coercing to " << spp);
                     }