rtl8733b: port USB TX aggregation (send_packets)

docs/aggregation.md

@@ -28,10 +28,25 @@ never lands on an exact bulk-MPS multiple (the sync bulk path has no ZLP).
 
 Per-family hardware rules:
 
-- **HalMAC (8822B/8821C/8822C/8822E)**: at most **3 descriptors per bulk
+- **HalMAC 88xx (8822B/8821C/8822C/8822E)**: at most **3 descriptors per bulk
   transfer** (mainline rtw88 `usb_tx_agg_desc_num` / halmac `BLK_DESC_NUM`) —
   the library clamps. Layout is rtw88-parity: no first-block reserve; the
   8-byte PKT_OFFSET shim is inserted only to escape a bulk-boundary total.
+- **RTL8733B (HALMAC 87xx)**: the same 3-descriptor HalMAC rule and the same
+  no-reserve layout, with a 40-byte descriptor. Two things differ from the
+  88xx siblings. `DMA_TXAGG_NUM` sits at the same `dword7[31:24]`, but this
+  family's checksum is folded *inside* `fill_tx_desc_8733b`, so the count is
+  built in rather than patched on and re-checksummed — byte `0x1f` is inside
+  the checksummed span (the fold skips only `0x1c-0x1d`), so writing it after
+  the checksum yields a descriptor the chip rejects. And `BLK_DESC_NUM = 3`
+  was already programmed by MAC init before this knob existed, so no
+  bring-up change was needed to enable it.
+  **This is the family where batching actually pays.** On the CV610 craft one
+  bulk-OUT submission costs ~248 µs of CPU against ~22 µs on x86, and ~87% of
+  that is the kernel USB submit/completion path — so packing 3:1 measured
+  **248 → 148 µs per frame, 43.0% → 26.7% of one core at ~1750 fps**, with
+  the frame rate unchanged. On x86 the same A/B moves 21.5 → 10.6 µs. Where
+  the host is small, this knob is worth roughly ten points of a core.
 - **Jaguar1 (8812A/8811A/8821A/8814A)**: vendor-parity — the first block
   carries the 8-byte PKT_OFFSET reserve (dropped at a boundary total), and the
   OQT guard caps descriptor STARTS per bulk window (8812A = 1, 8814A = 3,

docs/logging.md

@@ -109,7 +109,7 @@ Emitters: L = library, RX/TX/... = demo. Optional fields in [brackets];
 |---|---|---|
 | `tx.frame` | TX | n, rc — precoder demo variant: n, ok |
 | `tx.stats` | TX | submitted, failed, was_timeout, last_rc |
-| `tx.agg` | L (`DEVOURER_TX_USB_AGG`, send_packets) | frames, bytes, shim, ok — one per multi-frame bulk-OUT URB |
+| `tx.agg` | L (`DEVOURER_TX_USB_AGG`, send_packets) | frames, bytes, shim, ok — one per multi-frame bulk-OUT URB. RTL8733B also emits `sent` (bytes actually transferred) and sets `ok` only on a FULL write, so `ok=false` with `0 <= sent < bytes` is a short write, not a transport error |
 | `tx.report` | L (`DEVOURER_TX_REPORT`, CCX decode) | t, state (0=delivered, 1=retry-drop), ok, retries, final_rate, queue_time_raw, bmc, macid, fmt ("8812"\|"halmac"); halmac adds tag (SW_DEFINE echo), rts_retries, missed (fw-stuffed constant on Jaguar3 — tag gaps are the drop signal; `tests/txrpt_coverage_attrib.py`) — t is the achieved-report-rate timebase (the CCX emission ceiling is reports/s) |
 | `tx.status` | RX, duplex (C2H TX_RPT decode) | hoff, queue, retry, airtime_us, rate |
 | `tx.receipt` | TX (its RX thread, `DEVOURER_TX_RECEIPTS`) | t, fresh, total, covered, receipts, tlv hex — one event per absorbed windowed RX receipt (src/cell/RxReceipt.h), WITH the raw TLV so `tests/receipt_verify.py` can replay the merge and compare frame-exactly against the receiver's rx.seq ledger; decimating would break that comparison |

docs/rtl8733b.md

@@ -99,6 +99,41 @@ every frame. The witness decoded all eight `rate_hw` values on air with valid
 FCS, every frame at 20 MHz with STBC and LDPC clear, matching the forced-BCC
 contract.
 
+USB **TX** aggregation is ported and is the one host-side lever that matters on
+a small SoC. `send_packets` packs up to three `[txdesc][frame]` blocks into one
+bulk-OUT URB (`DEVOURER_TX_USB_AGG`, default 0 = off = byte-identical), the
+HalMAC 3-descriptor rule that MAC init had already programmed as
+`BLK_DESC_NUM` in `DWBCN0_CTRL[7:4]`. The block count rides the first
+descriptor's `dword7[31:24]` and is written *before* the checksum, which folds
+over byte `0x1f`.
+
+The cost it removes is host CPU, not air time. One bulk submission measured
+**~248 µs of CPU on the CV610 craft** against **~22 µs on x86** — the same
+transfer, an 11× ratio — and a profile attributes ~87% of it to the kernel USB
+submit/completion path, with the per-byte term below noise on ARM. Packing 3:1
+therefore removes two submissions in three: on the craft, **248 → 148 µs per
+frame and 43.0% → 26.7% of one core at ~1750 fps**, frame rate unchanged
+(1733 → 1800). On x86 the same A/B reads 21.5 → 10.6 µs.
+
+The failure mode this was checked against is the 8822BU's: wrong packing makes
+the TXDMA re-air block 1 `agg_num` times, which frame counts cannot see. Each
+frame was therefore stamped (`DEVOURER_TX_QOS_DATA`) and the stamps counted at
+the witness (`DEVOURER_RX_PCTR`): **31552 receptions, 31551 distinct counters,
+ratio 1.00** where re-airing would read 3.00. Note `GetTxStats().submitted`
+counts URBs rather than frames, so an aggregated session reports about a third
+— accounting, not throughput.
+
+The **boundary shim** is verified separately, because a fixed-payload sweep
+never reaches it. When a packed total lands on an exact bulk-MPS multiple the
+planner inserts the 8-byte first-block `PKT_OFFSET` reserve (the sync bulk path
+has no ZLP), and on the 8822BU a reserved first block is precisely what made
+the TXDMA re-air block 1. For three blocks at USB HS that happens when the MPDU
+length is ≡ 472 mod 512 — measured exactly, `urb_bytes` 1544 / 3080 / 4616 at
+payloads 472 / 984 / 1496, all `n x 512 + 8`. Stamped at the witness with the
+shim engaged: **15027 shim URBs, 44032 receptions, 44032 distinct, ratio 1.00**
+against 1.00 for the no-shim control. This part's block walker does account the
+reserve.
+
 RX aggregation is capped at 12 KiB (`rtl8733b::kRxAggregateBytes8733b`) because
 the vendor default of 20 KiB exceeds one bulk-IN URB: a 60-second high-traffic
 receive run had xHCI split such an aggregate, leaving a descriptor tail and its

src/rtl8733b/CLAUDE.md

@@ -108,6 +108,31 @@ unrelated register map.
   The pre-change binary measured **flat in the same session and geometry**
   (0.3 dB across the same 64 qdB) — the do-nothing control that makes the
   14 dB readable.
+- **USB TX aggregation is ported, and this is the family it matters most on.**
+  The mechanism (block count placement, the checksum-ordering constraint, the
+  URB-vs-frame accounting) is doc-commented at
+  `TxDescriptor8733b.h`'s `agg_num` and `Rtl8733bDevice::send_packets` — read
+  it there. What only lives here:
+  - **Bring-up needed no change.** MAC init *already* programmed
+    `BLK_DESC_NUM = 3` into `DWBCN0_CTRL[7:4]` (0x0208), the same field and
+    value the 88xx siblings use. The port was two things — a descriptor field
+    and the packer — because of that.
+  - **Why it is worth having here specifically.** One bulk submission costs
+    **~248 µs of CPU on the CV610 craft** against ~22 µs on x86, ~87% of it
+    the kernel USB path. Craft A/B at ~1750 fps: **248 → 148 µs per frame,
+    43.0 → 26.7% of one core**, frame rate unchanged. The counterparts, in the
+    same breath: it buys **nothing on air** — same frames, same airtime, purely
+    host CPU; it buys nothing at all unless the caller uses `send_packets`
+    (waybeam-link does not); and the win shrinks with the host, being only
+    ~11 µs/frame on x86.
+  - **Frame counts cannot verify it.** The 8822BU precedent is that wrong
+    packing makes the TXDMA re-air block 1 `agg_num` times, and `rx_hits` is
+    identical either way — it was 23900 in both arms here. Only per-frame
+    stamps discriminate (`DEVOURER_TX_QOS_DATA` + `DEVOURER_RX_PCTR`, count
+    distinct `pctr`): **ratio 1.00 = distinct, ~3.00 = re-airing**. Verified
+    1.00 with the boundary shim both off and on — and the shim needs a payload
+    chosen for it (MPDU length ≡ 472 mod 512 at USB HS), because a fixed-payload
+    sweep never reaches it.
 - **The thermal table is chosen once per channel set, not per frame.** The CCK
   and OFDM/HT variants of the thermal-compensation table are different tables.
   `configure_tx_power` picks one from the configured TX mode and leaves it,

src/rtl8733b/Rtl8733bDevice.cpp

@@ -8,12 +8,13 @@
 #include <vector>
 
 #include "RateDefinitions.h"
-#include "RadiotapPeek.h"
+#include "RadiotapPeek.h" /* send_packets batch pre-parse */
 #include "RadiotapTxFlags.h"
 #include "RxParseAbort.h" /* rx.parse_abort — abandoned-aggregate event */
 #include "rtl8733b/Rtl8733bUsbIds.h"
 #include "rtl8733b/TxDescriptor8733b.h"
 #include "SignalStop.h"
+#include "TxAggPlan.h" /* shared USB TX aggregation layout planner */
 
 extern "C" {
 #include "ieee80211_radiotap.h"
@@ -484,9 +485,146 @@ SelectedChannel Rtl8733bDevice::GetSelectedChannel() {
   return _channel;
 }
 
+size_t Rtl8733bDevice::send_packets(const TxPacketView *pkts, size_t count) {
+  const unsigned agg = _cfg.tx.usb_agg_max;
+  if (agg <= 1 || !_device.is_usb() || count == 0)
+    return IRtlDevice::send_packets(pkts, count);
+
+  std::lock_guard<std::recursive_mutex> lock(_reg_mu);
+  if (!_phy_ready || !_mac_ready || !_tx_ready) {
+    _logger->error("RTL8733B TX rejected before InitWrite");
+    return 0;
+  }
+
+  devourer::TxAggLimits lim;
+  lim.desc_size = rtl8733b::kTxDescSize;
+  lim.bulk_size = _device.speed() >= devourer::kUsbSpeedSuper  ? 1024
+                  : _device.speed() >= devourer::kUsbSpeedHigh ? 512
+                                                               : 64;
+  /* BLK_DESC_NUM: MAC init already programs 3 into DWBCN0_CTRL[7:4]
+   * (Halmac8733bMac.cpp) — the same field and value the HalMAC 88xx siblings
+   * use — so 3 descriptors per bulk transfer is what this TXDMA parses.
+   * Layout is rtw88/HalMAC parity: no first-block PKT_OFFSET reserve. */
+  lim.max_frames = std::min<unsigned>(agg, 3u);
+  lim.descs_per_bulk = 0;
+  lim.first_reserve = false;
+
+  size_t done = 0, ok = 0;
+  while (done < count) {
+    /* Collect the contiguous run for ONE URB. A frame carrying a radiotap
+     * CHANNEL other than the session channel ends the run — this backend does
+     * not retune mid-submission, so build_tx_block refuses such a frame
+     * outright and it must not be packed beside frames that would have
+     * aired. */
+    std::vector<size_t> lens;
+    for (size_t i = done; i < count && lens.size() < lim.max_frames; ++i) {
+      /* A null view is treated exactly like a malformed one: it ends the run
+       * and, if it led, is skipped per the IRtlDevice::send_packets
+       * contract. */
+      const uint16_t rlen =
+          pkts[i].data == nullptr
+              ? uint16_t{0}
+              : devourer::radiotap_hdr_len(pkts[i].data, pkts[i].len);
+      if (rlen == 0)
+        break;
+      const int want =
+          devourer::radiotap_peek_channel(pkts[i].data, pkts[i].len);
+      if (want > 0 && want != _channel.Channel) {
+        /* This backend retunes for nobody mid-submission — build_tx_block
+         * refuses an off-channel frame outright. A LEADING one still has to
+         * enter the run alone, so the single-frame path below refuses it and
+         * `done` moves past it; ending the run empty here instead would spin
+         * this loop forever on the same entry, holding _reg_mu. */
+        if (lens.empty())
+          lens.push_back(pkts[i].len - rlen);
+        break;
+      }
+      lens.push_back(pkts[i].len - rlen);
+    }
+    if (lens.empty()) {
+      /* The leading view was null or malformed. This is also the loop's
+       * termination guarantee: `done` advances on every iteration whatever
+       * the collector above decided, so no future run rule can reintroduce a
+       * non-advancing path. */
+      ++done;
+      continue;
+    }
+
+    const devourer::TxAggPlan plan =
+        devourer::plan_tx_agg(lens.data(), lens.size(), lim);
+    if (plan.frames() <= 1) {
+      /* One block (or a frame the URB cap refuses): the classic single-frame
+       * path is byte-identical and uncapped. */
+      if (send_packet(pkts[done].data, pkts[done].len))
+        ++ok;
+      ++done;
+      continue;
+    }
+
+    /* The block count rides the FIRST descriptor, and it must be in place
+     * before that descriptor is checksummed — hence built in, not patched on
+     * afterwards the way the 8822C does it (its checksum is recomputable in
+     * isolation; this one is folded inside fill_tx_desc_8733b). */
+    std::vector<uint8_t> urb(plan.total, 0);
+    size_t built = 0;
+    for (size_t k = 0; k < plan.frames(); ++k) {
+      const uint8_t poff = (k == 0 && plan.shim) ? 1 : 0;
+      const uint8_t anum =
+          k == 0 ? static_cast<uint8_t>(plan.frames()) : uint8_t{0};
+      if (build_tx_block(pkts[done + k].data, pkts[done + k].len,
+                         urb.data() + plan.blocks[k].offset, poff, anum) == 0)
+        break; /* pre-validated, so only a defensive bail */
+      ++built;
+    }
+    if (built != plan.frames()) {
+      for (size_t k = 0; k < plan.frames(); ++k, ++done)
+        if (send_packet(pkts[done].data, pkts[done].len))
+          ++ok;
+      continue;
+    }
+
+    const int rc = _device.bulk_send_sync_ep(_device.first_bulk_out_ep(),
+                                             urb.data(), urb.size(),
+                                             /*timeout_ms=*/100);
+    /* bulk_send_sync_ep returns BYTES SUBMITTED, so `rc >= 0` also covers a
+     * short write. A truncated URB means the chip got a prefix — some
+     * trailing block is partial or absent — and there is no way to say which
+     * frames aired, so none of them may be reported as submitted. The
+     * single-frame path already refuses a short write; the aggregated one
+     * must not be the looser of the two in the same backend. */
+    const bool sent_all = rc == static_cast<int>(urb.size());
+    if (rc >= 0 && !sent_all)
+      _logger->error("RTL8733B aggregated TX short on EP 0x{:02x}: {}/{} "
+                     "({} frames dropped)",
+                     _device.first_bulk_out_ep(), rc, urb.size(),
+                     plan.frames());
+    devourer::Ev(_logger->events(), "tx.agg")
+        .f("frames", (unsigned long long)plan.frames())
+        .f("bytes", (unsigned long long)urb.size())
+        .f("sent", (long long)rc)
+        .f("shim", plan.shim)
+        .f("ok", sent_all);
+    if (sent_all) {
+      ok += plan.frames();
+      /* Same one-shot latch send_packet uses — a session whose very first TX
+       * is aggregated must still say so once, or the "first TX accepted"
+       * breadcrumb goes missing exactly when the packing is what is on
+       * trial. It counts URB acceptances, not frames; it is a latch, not a
+       * meter. */
+      if (_tx_submits.fetch_add(1) == 0)
+        _logger->info(
+            "RTL8733B first TX accepted (aggregated): EP=0x{:02x} frames={} "
+            "bytes={} shim={}",
+            _device.first_bulk_out_ep(), plan.frames(), urb.size(), plan.shim);
+    }
+    done += plan.frames();
+  }
+  return ok;
+}
+
 size_t Rtl8733bDevice::build_tx_block(const uint8_t *packet, size_t length,
-                                      uint8_t *out,
-                                      uint8_t packet_offset) {
+                                      uint8_t *out, uint8_t packet_offset,
+                                      uint8_t agg_num) {
   if (packet == nullptr || out == nullptr)
     return 0;
   const uint16_t radiotap_length =
@@ -595,6 +733,7 @@ size_t Rtl8733bDevice::build_tx_block(const uint8_t *packet, size_t length,
       cfg.rate_hw, cfg.bandwidth, _channel.Channel > 14);
   cfg.data_sc = data_sc;
   cfg.packet_offset = packet_offset;
+  cfg.agg_num = agg_num;
   cfg.retry_limit = static_cast<uint8_t>(
       std::clamp(_cfg.tx.retry_limit, 0, 63));
   cfg.short_gi = false;

src/rtl8733b/Rtl8733bDevice.h

@@ -39,6 +39,20 @@ class Rtl8733bDevice : public IRtlDevice {
    * interface declaration. */
   void FastRetune(uint8_t channel, bool cache_rf) override;
   bool send_packet(const uint8_t *packet, size_t length) override;
+  /* USB TX aggregation (cfg.tx.usb_agg_max / DEVOURER_TX_USB_AGG): pack
+   * consecutive frames into one bulk-OUT URB. Measured on the CV610 craft,
+   * this is the lever that matters on an embedded host — a single submission
+   * costs ~248 us of CPU there against ~22 us on x86, and ~87% of that is the
+   * kernel USB submit/completion path, so folding three frames into one URB
+   * removes two of every three (measured: 248 -> 148 us per frame on the
+   * craft, frame rate unchanged). Knob off / non-USB falls back to the
+   * interface-default per-frame loop, byte-identical.
+   *
+   * Note when reading TX stats against this: GetTxStats().submitted counts
+   * bulk-OUT transfers, so an aggregated session reports roughly frames/3 —
+   * the same accounting Jaguar1/2/3 have, not a throughput drop. The
+   * per-URB `tx.agg` event carries the real frame count. */
+  size_t send_packets(const TxPacketView *pkts, size_t count) override;
   void SetTxMode(const devourer::TxMode &mode) override;
   void ClearTxMode() override;
   SelectedChannel GetSelectedChannel() override;
@@ -72,8 +86,12 @@ class Rtl8733bDevice : public IRtlDevice {
 private:
   void bring_up_to_phy();
   bool configure_tx_power(SelectedChannel channel);
+  /* Fill one [txdesc][frame] block at `out`. `agg_num` is the USB TX
+   * aggregation block count and belongs on the FIRST descriptor of a packed
+   * URB only; 0 everywhere else, which is what keeps the single-frame path
+   * byte-identical. */
   size_t build_tx_block(const uint8_t *packet, size_t length, uint8_t *out,
-                        uint8_t packet_offset);
+                        uint8_t packet_offset, uint8_t agg_num = 0);
 
   RtlAdapter _device;
   Logger_t _logger;

src/rtl8733b/TxDescriptor8733b.h

@@ -114,6 +114,20 @@ struct TxDescConfig {
   uint8_t bandwidth = 0; // 0=20 MHz, 1=40 MHz
   uint8_t data_sc = 0;
   uint8_t packet_offset = 0; // 8-byte USB boundary shim count
+  /* USB TX aggregation block count, set on the FIRST descriptor of a packed
+   * bulk-OUT URB and left 0 on every other descriptor and on every
+   * single-frame transfer (0 and 1 are both "one block" to the TXDMA, and 0
+   * keeps the single-frame path byte-identical to before this field existed).
+   * HALMAC DMA_TXAGG_NUM, dword7[31:24] — the same placement the 8822C
+   * carries it at, alongside the checksum in the same dword's low half.
+   * Capped by BLK_DESC_NUM = 3, which MAC init already programs.
+   *
+   * ORDERING: byte 0x1f is INSIDE the checksummed span (the fold covers 32
+   * bytes and skips only 0x1c-0x1d, the checksum field itself), so this must
+   * be written BEFORE the checksum. fill_tx_desc_8733b does that by
+   * construction; the 8822C's patch-then-recompute shape does not port here,
+   * because this family folds the checksum inside the fill. */
+  uint8_t agg_num = 0;
   uint8_t retry_limit = 0;
   bool short_gi = false;
   bool ldpc = false;
@@ -127,7 +141,7 @@ inline bool valid_tx_desc_config(const TxDescConfig &cfg) {
   return cfg.packet_size != 0 && cfg.sequence <= 0x0fff &&
          (legacy_cck || legacy_ofdm || ht_1ss) && cfg.rate_id <= 0x1f &&
          cfg.bandwidth <= 1 && cfg.data_sc <= 0x0f &&
-         cfg.packet_offset <= 1 &&
+         cfg.packet_offset <= 1 && cfg.agg_num <= 3 &&
          cfg.retry_limit <= 0x3f &&
          (!legacy_cck || cfg.bandwidth == 0) && !cfg.ldpc &&
          (!(legacy_cck || legacy_ofdm) || !cfg.short_gi);
@@ -169,6 +183,11 @@ inline bool fill_tx_desc_8733b(uint8_t *desc, size_t desc_len,
   txdesc_set_bits(desc + 0x14, 7, 1, cfg.ldpc ? 1 : 0);
   txdesc_set_bits(desc + 0x20, 15, 1, 0); // preserve caller sequence
   txdesc_set_bits(desc + 0x24, 12, 12, cfg.sequence);
+  /* DMA_TXAGG_NUM before the checksum, not after: the fold covers 32 bytes
+   * skipping only 0x1c-0x1d (the checksum field itself), so byte 0x1f is
+   * INSIDE the checksummed span. Writing the count afterwards would leave a
+   * descriptor the chip rejects. */
+  txdesc_set_bits(desc + 0x1c, 24, 8, cfg.agg_num);
   txdesc_set_bits(desc + 0x1c, 0, 16, txdesc_checksum_8733b(desc));
   return true;
 }