From c8f086fd9c117fccc51036cc47ffbd554ac692a3 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Mon, 13 Jul 2026 18:07:27 -0400 Subject: [PATCH] WIP --- GUIDE.md | 9 ++ deploy.sh | 22 +++- docs/spi_lcd_timing.md | 37 ++++++- tests/test_spi_timing.py | 145 ++++++++++++++++++++++++ tools/bench_adc_contention.py | 202 ++++++++++++++++++++++++++++++++++ tools/bench_lcd_device.py | 10 +- tools/bench_pack_variants.py | 138 +++++++++++++++-------- uilib/lcd_ili9341.py | 32 +++--- uilib/panel.py | 6 +- uilib/spi_timing.py | 118 ++++++++++++++++---- 10 files changed, 632 insertions(+), 87 deletions(-) create mode 100644 tests/test_spi_timing.py create mode 100644 tools/bench_adc_contention.py diff --git a/GUIDE.md b/GUIDE.md index bfd1e49af..cbdb7b7ae 100644 --- a/GUIDE.md +++ b/GUIDE.md @@ -68,6 +68,15 @@ Things that cost hours if you don't know them. None are derivable from reading t for alpha, or `depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)` for opaque RGB (bit-identical to the device; `depth=24` differs in AA rounding). +- **`PanelStack`'s root surface must stay opaque.** `LcdIli9341.update` quantises it to + RGB565 with an SDL convert-blit; give that blit an `SRCALPHA` source and SDL silently + switches to its per-pixel *alpha-blending* blitter — ~7x slower, and it lands on every + LCD push. The root is a blend *destination*, so it needs 32-bit for blend precision + but gains nothing from a dest alpha channel (a dimmer over black yields the same + `(128,128,128)` either way). Panel surfaces (`ShroudedPanel`, `RoundedPanel`) are blit + *sources* and do still need `RGBA`. Benchmark the pack path with + `tools/bench_pack_variants.py`. + - **Snapshot loads broadcast only deltas** against mod-ui's own cache; pedalboard loads and connect dumps rebroadcast unconditionally. Reselecting a *board* is a full resync. Reselecting a *snapshot* is not. diff --git a/deploy.sh b/deploy.sh index 058a97665..66a9f157c 100755 --- a/deploy.sh +++ b/deploy.sh @@ -10,15 +10,35 @@ REMOTE_DIR="/home/pistomp/pi-stomp" echo "==> Deploying to ${TARGET}" -rsync -az --delete --exclude='__pycache__' --exclude='*.pyc' \ +rsync -az --delete \ + --exclude='.venv' --exclude='__pycache__' --exclude='*.pyc' \ + --exclude='.pytest_cache' --exclude='.ruff_cache' --exclude='.coverage' \ + --exclude='.DS_Store' --exclude='.claude' --exclude='.github' \ + --exclude='typings' \ --exclude='.git' --exclude='.gitignore' --exclude='tests' \ --exclude='setup' --exclude='*.md' --exclude='*.yml' --exclude='*.yaml' \ --exclude='*.json' --exclude='*.toml' --exclude='*.lock' \ --exclude='*.png' --exclude='*.jpg' --exclude='*.svg' \ + --filter='protect .git-meta' \ ./ \ "${TARGET}:${REMOTE_DIR}/" echo "==> Restarting mod-ala-pi-stomp" ssh "${TARGET}" 'sudo systemctl restart mod-ala-pi-stomp' +echo "==> Service starting, showing logs..." +echo "----------------------------------------" +ssh "${TARGET}" "timeout 2 sudo journalctl -u mod-ala-pi-stomp -f --since '1 second ago' 2>/dev/null || true" +echo "----------------------------------------" + +if ssh "${TARGET}" 'sudo systemctl is-active --quiet mod-ala-pi-stomp'; then + echo "==> Service started successfully." +else + echo "ERROR: Service failed to start!" + echo "----------------------------------------" + echo "Service status:" + ssh "${TARGET}" 'sudo systemctl status mod-ala-pi-stomp' + exit 1 +fi + echo "==> Done" diff --git a/docs/spi_lcd_timing.md b/docs/spi_lcd_timing.md index ebc016af4..2bec5d254 100644 --- a/docs/spi_lcd_timing.md +++ b/docs/spi_lcd_timing.md @@ -92,6 +92,31 @@ dominates and the PCIe stall is near-zero at 50 MHz. --- +## The pack + +`LcdIli9341.update()` quantises the composed surface to RGB565 with a **single SDL +convert-blit** into a preallocated 16-bit staging surface — not a numpy +channel-mask pipeline. Measured full-frame on this board: + +| Term | Value | +|---|---| +| `fixed_ms` | 0.2727 ms | +| `pipeline_ms_per_px` | 3.83e-6 ms/px (**0.29 ms** full frame) | +| `bits_per_pixel` | 16.03 — no framing overhead | + +The blit source **must be opaque**; `PanelStack`'s root is XRGB for exactly this +reason. An `SRCALPHA` source silently takes SDL's per-pixel alpha-blending path +instead of a format convert and is *slower than the numpy pack it replaced*. See +CLAUDE.md "Traps" and `tools/bench_pack_variants.py`. + +At 50 MHz the pack is now **~1%** of a full-frame push (0.29 ms of 25 ms) — wire +time is overwhelmingly the cost, and dirty-rect culling remains the only lever +that matters. The pack is *not* negligible on v2, where it is 6.2× dearer; the +cost model is per-SoC for that reason. See +[`spi_lcd_timing_pi3.md`](spi_lcd_timing_pi3.md). + +--- + ## The PCIe DMA stall At 100 MHz SPI clock (BAUDR=2), the TX FIFO drains 16 bytes in **1.28 µs**. @@ -147,7 +172,7 @@ poll loop (10 ms cadence) _do_update() # runs on worker thread lock.acquire() - numpy pack + rot90 (~1 ms) + SDL 565 convert-blit (~0.3 ms) # no rot90: panel is landscape-native os.write(spidev_fd, data) # blocks ~25 ms on worker thread lock.release() ``` @@ -215,9 +240,8 @@ runs in a DRM worker thread. lcd-splash; the stamp mechanism would need redesigning. **Python-side changes**: Replace `LcdIli9341` with a thin writer that opens -`/dev/fb0`, packs RGB565, and calls `write()`. Rotation must be pre-applied -in numpy (same as today) or configured via `MADCTL` in the firmware blob to -make the panel landscape-native (eliminating the `rot90` step entirely). +`/dev/fb0`, packs RGB565, and calls `write()`. Rotation needs no work — the +driver already sets `MADCTL` in `__init__` so the panel is landscape-native. **When to prefer this over the background thread**: If DRM integration is wanted for other reasons (console on the display, compositor, hardware @@ -270,7 +294,10 @@ optimization and requires no kernel or boot changes. | ≥ 100 MHz requested → actual | **100 MHz** (BAUDR=2) — **display garbled** (ILI9341 max 66.7 MHz) | | 66.7 MHz achievable? | **No** — requires BAUDR=3 (odd, forbidden) | | Safe maximum setting | Any value 56–99 MHz (all give 50 MHz actual) | -| Full-frame time @ 50 MHz | ~25 ms (24.6 ms wire + ~0 PCIe stall + ~0.4 ms overhead) | +| Configured in `pistomptre.py` | `spi_speed_hz=50_000_000` | +| Full-frame time @ 50 MHz | **25.19 ms** measured (24.6 ms wire + ~0 PCIe stall + 0.56 ms pack/overhead) | +| RGB565 pack | SDL convert-blit; source **must be opaque**. 0.29 ms/full frame | +| Cost-model constants | `spi_timing.PUSH_PROFILE["brcm,bcm2712"]` — per-SoC; v2's differ 6.2× | | DMA/IRQ crossover | 128 bytes (FIFO depth × 2-byte words) | | Optimal `spidev.bufsiz` | ≥153,600 (set to 163,840 in `pistomp-arch/files/cmdline.txt`) | | Primary optimization lever | Dirty-rect culling (linear in pixel count) | diff --git a/tests/test_spi_timing.py b/tests/test_spi_timing.py new file mode 100644 index 000000000..af64e8be0 --- /dev/null +++ b/tests/test_spi_timing.py @@ -0,0 +1,145 @@ +import pytest + +from uilib.spi_timing import ( + DEFAULT_PROFILE, + DEFAULT_SOURCE_HZ, + PUSH_PROFILE, + SPI_SOURCE_HZ, + actual_spi_hz, + push_profile, + spi_source_hz, + transfer_ms, +) + +PI3 = 400_000_000 # v2 — BCM2837 VPU core clock +PI5 = 200_000_000 # v3 — RP1 CLK_SYS + + +@pytest.mark.parametrize( + "requested, expected_hz", + [ + # Measured on a Pi 3A+ (docs/spi_lcd_timing_pi3.md): every achievable + # clock is 400 MHz / even divisor. + (12_500_000, 12_500_000), # cdiv 32 + (20_000_000, 20_000_000), # cdiv 20 + (25_000_000, 25_000_000), # cdiv 16 + (28_571_428, 25_000_000), # cdiv 15 -> 16 + (33_333_333, 400_000_000 / 14), # cdiv 13 -> 14 + (40_000_000, 40_000_000), # cdiv 10 + (44_444_444, 40_000_000), # cdiv 9 -> 10 + (50_000_000, 50_000_000), # cdiv 8 (production) + (57_142_857, 50_000_000), # cdiv 7 -> 8 + (80_000_000, 400_000_000 / 6), # cdiv 5 -> 6 + (99_000_000, 400_000_000 / 6), # cdiv 5 -> 6 + (100_000_000, 100_000_000), # cdiv 4 — garbles the ILI9341 + ], +) +def test_pi3_divisor_rule(requested, expected_hz): + assert actual_spi_hz(requested, PI3) == pytest.approx(expected_hz) + + +def test_pi3_one_hertz_below_a_divisor_point_costs_a_full_step(): + # 400e6 / 6 == 66_666_666.67, so 66_666_666 rounds the divisor to 7 -> 8. + assert actual_spi_hz(66_666_666, PI3) == pytest.approx(50_000_000) + assert actual_spi_hz(66_666_667, PI3) == pytest.approx(400_000_000 / 6) + + +@pytest.mark.parametrize( + "requested, expected_hz", + [ + (81_000_000, 50_000_000), # BAUDR 3 -> 4 + (50_000_000, 50_000_000), # BAUDR 4 + (99_000_000, 50_000_000), # BAUDR 3 -> 4 + (100_000_000, 100_000_000), # BAUDR 2 + ], +) +def test_pi5_divisor_rule(requested, expected_hz): + assert actual_spi_hz(requested, PI5) == pytest.approx(expected_hz) + + +def test_pi5_cannot_reach_66mhz_but_pi3_can(): + # 200/6 needs BAUDR 3 (odd); 400/6 is even, so v2 out-runs v3 here. + assert actual_spi_hz(70_000_000, PI5) == pytest.approx(50_000_000) + assert actual_spi_hz(70_000_000, PI3) == pytest.approx(400_000_000 / 6) + + +def test_actual_never_exceeds_request(): + for source in (PI3, PI5): + for requested in range(1_000_000, 100_000_000, 997_000): + assert actual_spi_hz(requested, source) <= requested + + +def test_divisor_floors_at_two(): + assert actual_spi_hz(10_000_000_000, PI3) == pytest.approx(PI3 / 2) + + +def test_rejects_nonpositive(): + with pytest.raises(ValueError): + actual_spi_hz(0, PI3) + + +def test_source_from_device_tree(monkeypatch, tmp_path): + dt = tmp_path / "compatible" + dt.write_bytes(b"raspberrypi,3-model-b-plus\0brcm,bcm2837\0") + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", dt) + assert spi_source_hz() == PI3 + + +def test_source_defaults_when_device_tree_absent(monkeypatch, tmp_path): + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", tmp_path / "nope") + assert spi_source_hz() == DEFAULT_SOURCE_HZ + + +def test_source_defaults_on_unknown_soc(monkeypatch, tmp_path): + dt = tmp_path / "compatible" + dt.write_bytes(b"acme,widget\0") + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", dt) + assert spi_source_hz() == DEFAULT_SOURCE_HZ + + +def test_every_known_soc_has_a_push_profile(): + # push_profile() indexes PUSH_PROFILE with whatever soc_key() returns, and + # soc_key() answers from SPI_SOURCE_HZ. A SoC in one dict but not the other + # is a KeyError on that board and nowhere else. + assert SPI_SOURCE_HZ.keys() == PUSH_PROFILE.keys() + + +def test_pi3_push_costs_more_than_pi5(tmp_path, monkeypatch): + # The CPU-bound terms are why the profile is per-SoC at all: an A53 pack is + # ~7x an A76's, and treating them alike over-admits inline pushes on v2. + dt = tmp_path / "compatible" + + dt.write_bytes(b"brcm,bcm2837\0") + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", dt) + pi3 = push_profile() + + dt.write_bytes(b"brcm,bcm2712\0") + pi5 = push_profile() + + assert pi3.pipeline_ms_per_px > 5 * pi5.pipeline_ms_per_px + assert pi3.fixed_ms > pi5.fixed_ms + + +def test_unknown_soc_on_a_real_board_gets_the_slow_profile(tmp_path, monkeypatch): + # A Pi 2, or a Pi 6 we've never benched: guessing Pi 5's costs here would + # over-admit inline pushes and stall the poll loop. + dt = tmp_path / "compatible" + dt.write_bytes(b"brcm,bcm9999\0") + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", dt) + assert push_profile() == PUSH_PROFILE["brcm,bcm2837"] + + +def test_off_device_gets_the_default_profile(tmp_path, monkeypatch): + # No device tree at all (mac, emulator, CI) — pair with DEFAULT_SOURCE_HZ. + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", tmp_path / "absent") + assert push_profile() == DEFAULT_PROFILE + + +def test_transfer_ms_scales_with_clock(): + slow = transfer_ms(76800, 50_000_000) + fast = transfer_ms(76800, 400_000_000 / 6) + assert slow > fast + # Wire time dominates a full frame: 66.67 MHz should be meaningfully cheaper. + # The floor is the pure wire ratio, 50/66.67 = 0.75; the CPU-bound terms are + # what hold it above that, so this tracks the host's push profile. + assert fast / slow == pytest.approx(0.76, abs=0.03) diff --git a/tools/bench_adc_contention.py b/tools/bench_adc_contention.py new file mode 100644 index 000000000..d79a01eee --- /dev/null +++ b/tools/bench_adc_contention.py @@ -0,0 +1,202 @@ +#!/usr/bin/env python3 +"""Does an in-flight LCD push stall the ADC on the shared SPI bus? + +The LCD (CE0, Blinka) and the MCP3008 (CE1, raw spidev @1MHz) sit on the same +SPI master. Today they never overlap: poll_lcd_updates() and poll_controls() +both run on the UI thread, so the kernel sees one transfer at a time. + +Moving the LCD push to a worker thread (docs/spi_lcd_timing_pi5.md) would remove +a 19-25ms UI-thread stall -- but only if it doesn't simply relocate that stall +into poll_controls(). With spidev.bufsiz=163840 a full frame is ONE kernel SPI +message, and the spi core serializes messages per master: an ADC read issued +mid-frame may wait for the whole payload to clock out. + +This measures ADC xfer2 latency with the LCD idle vs. with a worker thread +pushing frames continuously, and sweeps the LCD payload chunk size -- chunking +releases the bus between writes, letting the ADC interleave, at the cost of more +syscalls. + +Verdict this is looking for: p95/max ADC latency under contention. The UI tick is +10ms; an ADC read that blocks for ~19ms is a worse bug than the one we set out to +fix. + + sudo systemctl stop mod-ala-pi-stomp + PYTHONPATH=/opt/pistomp/pi-stomp python tools/bench_adc_contention.py [baud] +""" + +from __future__ import annotations + +import os +import statistics +import sys +import threading +import time +from pathlib import Path + +# tools/ lands on sys.path[0], not the repo root -- without this, `import uilib` +# finds the copy in the venv's site-packages and benches the packaged driver. +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +os.environ.setdefault("SDL_VIDEODRIVER", "dummy") + +import pygame # noqa: E402 + +pygame.init() +pygame.display.set_mode((1, 1)) + +import board # noqa: E402 +import digitalio # noqa: E402 +import spidev # noqa: E402 + +from uilib.box import Box # noqa: E402 +from uilib.lcd_ili9341 import LcdIli9341 # noqa: E402 + +BAUD = int(sys.argv[1]) if len(sys.argv) > 1 else 66_666_667 +ADC_CHANNEL = 0 + + +def adc_open() -> spidev.SpiDev: + """Exactly what hardware.init_spi() does.""" + spi = spidev.SpiDev() + spi.open(0, 1) # bus 0, CE1 + spi.max_speed_hz = 1_000_000 + return spi + + +def adc_read(spi: spidev.SpiDev) -> int: + """One MCP3008 single-ended read -- analogcontrol.readChannel().""" + adc = spi.xfer2([1, (8 + ADC_CHANNEL) << 4, 0]) + return ((adc[1] & 3) << 8) + adc[2] + + +def sample_adc(spi: spidev.SpiDev, duration_s: float) -> list[float]: + """Poll the ADC at the real 10ms tick cadence for `duration_s`.""" + out: list[float] = [] + deadline = time.perf_counter() + duration_s + while time.perf_counter() < deadline: + t0 = time.perf_counter_ns() + adc_read(spi) + out.append((time.perf_counter_ns() - t0) / 1e6) + time.sleep(0.010) + return out + + +def report(label: str, samples: list[float]) -> None: + s = sorted(samples) + p = lambda q: s[min(len(s) - 1, int(q * len(s)))] # noqa: E731 + over = sum(1 for v in s if v > 10.0) + print( + f" {label:<34} n={len(s):4d} med={statistics.median(s):6.3f} " + f"p95={p(0.95):7.3f} p99={p(0.99):7.3f} max={max(s):7.3f} ms" + f" >10ms: {over} ({100*over/len(s):.1f}%)" + ) + + +def main() -> int: + lcd = LcdIli9341( + board.SPI(), + digitalio.DigitalInOut(board.CE0), + digitalio.DigitalInOut(board.D6), + digitalio.DigitalInOut(board.D5), + BAUD, + True, + ) + surf = pygame.Surface((320, 240), depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)) + for y in range(240): + pygame.draw.line(surf, (y, 255 - y, (y * 3) % 256), (0, y), (319, y)) + full = Box(0, 0, 320, 240) + + adc = adc_open() + + print(f"LCD @ {lcd.baudrate/1e6:.2f} MHz actual | ADC @ 1 MHz, CE1 | tick = 10 ms\n") + + print("ADC latency, LCD idle (baseline):") + report("idle", sample_adc(adc, 2.0)) + + # --- LCD pushing continuously on a worker thread --- + stop = threading.Event() + frames = [0] + + def pusher(): + while not stop.is_set(): + lcd.update(surf, full) + frames[0] += 1 + + print("\nADC latency, worker thread pushing full frames continuously:") + t = threading.Thread(target=pusher, daemon=True) + t.start() + time.sleep(0.2) # let it get going + contended = sample_adc(adc, 4.0) + stop.set() + t.join(timeout=5) + report("contended (one 153.6kB write)", contended) + print(f" (worker pushed {frames[0]} frames)") + + # --- same, but chunk the payload so the bus is released between writes --- + orig_block = lcd.disp._block + + for chunk_kb in (64, 16, 4): + chunk = chunk_kb * 1024 + + def chunked(x0, y0, x1, y1, data=None, _c=chunk): + if data is None: + return orig_block(x0, y0, x1, y1, data) + for i in range(0, len(data), _c): + # Only the first call sets the address window; the panel + # auto-increments GRAM across subsequent payload writes. + if i == 0: + orig_block(x0, y0, x1, y1, data[i : i + _c]) + else: + _raw_payload(lcd, data[i : i + _c]) + return None + + lcd.disp._block = chunked + stop = threading.Event() + frames = [0] + + def pusher2(): + while not stop.is_set(): + lcd.update(surf, full) + frames[0] += 1 + + t = threading.Thread(target=pusher2, daemon=True) + t.start() + time.sleep(0.2) + s = sample_adc(adc, 3.0) + stop.set() + t.join(timeout=5) + report(f"contended ({chunk_kb}kB chunks)", s) + print(f" (worker pushed {frames[0]} frames)") + + lcd.disp._block = orig_block + adc.close() + return 0 + + +def _raw_payload(lcd: LcdIli9341, data: bytes) -> None: + """Continuation payload write: no address window, DC high, CS asserted.""" + disp = lcd.disp + spi_dev = disp.spi_device + spi = spi_dev.spi + cs = spi_dev.chip_select + dc = disp.dc_pin + fd = spi._spi._spi.handle + + while not spi.try_lock(): + time.sleep(0) + try: + spi.configure( + baudrate=spi_dev.baudrate, polarity=spi_dev.polarity, phase=spi_dev.phase + ) + if cs: + cs.value = spi_dev.cs_active_value + dc.value = 1 + os.write(fd, data) + finally: + if cs: + cs.value = not spi_dev.cs_active_value + spi.unlock() + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/bench_lcd_device.py b/tools/bench_lcd_device.py index 09e7c504e..f5d9f53b7 100644 --- a/tools/bench_lcd_device.py +++ b/tools/bench_lcd_device.py @@ -12,6 +12,12 @@ import sys import time import statistics +from pathlib import Path + +# Running as tools/bench_lcd_device.py puts tools/ on sys.path[0], not the repo +# root -- so `import uilib` finds the copy installed in the venv's site-packages +# and silently benches the *packaged* driver instead of the working tree. +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) os.environ.setdefault("SDL_VIDEODRIVER", "dummy") @@ -35,7 +41,9 @@ True, ) -surf = pygame.Surface((320, 240)) +# Opaque XRGB, matching PanelStack's root. update()'s 565 convert-blit is ~7x +# slower off an SRCALPHA source, so the fit is only valid on an opaque one. +surf = pygame.Surface((320, 240), depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)) for y in range(240): pygame.draw.line(surf, (y, 255 - y, (y * 3) % 256), (0, y), (319, y)) diff --git a/tools/bench_pack_variants.py b/tools/bench_pack_variants.py index 32cb6dd6a..efef28bff 100644 --- a/tools/bench_pack_variants.py +++ b/tools/bench_pack_variants.py @@ -1,13 +1,23 @@ #!/usr/bin/env python3 """Benchmark RGB565 pack variants for the LCD update path. -Compares the current pygame.image.tobytes() approach against pygame.surfarray -alternatives (pixels3d / array3d) at clip sizes the tuner strobe actually -generates: one stripe edge (~5px), one stripe span (~53px), the unioned strobe -region (~270px), and the full strobe widget (320px). +Compares the production SDL staging-surface blit (LcdIli9341.update) against the +numpy packs it replaced, at clip sizes the tuner strobe actually generates: one +stripe edge (~5px), one stripe span (~53px), the unioned strobe region (~270px), +and the full strobe widget (320px). -Source surface is RGBA (the PanelStack surface format when use_dimming=True). -Both reading and output buffer allocation strategies are tested. +Each size is run against both source formats, because that is the whole question +for the SDL path: + + opaque RGB source (PanelStack's root) — a straight convert blit. ~2.5x faster + than the best numpy pack. + + RGBA source — blitting this into a 565 surface hits SDL's per-pixel + *alpha-blending* blitter, not a format convert, and is ~7x slower than the + opaque blit. This is why the root must stay opaque; see CLAUDE.md "Traps". + +SDL output is native little-endian; the ILI9341 wants high byte first, so the +byteswap is part of the measured cost, not an optimisation to factor out. Run: uv run python tools/bench_pack_variants.py @@ -98,6 +108,35 @@ def pack_pixels3d_colmajor(sub: pygame.Surface, out_col: np.ndarray) -> bytes: return pix.tobytes() # wrong byte order for LCD — for timing only +_staging: dict[tuple[int, int], pygame.Surface] = {} + + +def _stage_565(w: int, h: int) -> pygame.Surface: + """Preallocated RGB565 staging surface, as the driver would hold.""" + s = _staging.get((w, h)) + if s is None: + # masks spelled out — never a bare Surface. See CLAUDE.md "Traps". + s = pygame.Surface((w, h), depth=16, masks=(0xF800, 0x07E0, 0x001F, 0)) + _staging[(w, h)] = s + return s + + +def pack_sdl_blit(sub: pygame.Surface, out: np.ndarray) -> bytes: + """Production path: blit into a 565 staging surface, byteswap its pixels. + + Slicing the sub-rect out of the staging array sidesteps SDL's 4-byte row + padding, which an odd width would otherwise hit. Surface pixels are native + little-endian; the panel wants high byte first. + """ + sw, sh = sub.get_size() + s565 = _stage_565(sw, sh) + s565.blit(sub, (0, 0)) + staged = pygame.surfarray.pixels2d(s565).T + out_bytes = staged[:sh, :sw].byteswap().tobytes() + del staged # unlock the surface + return out_bytes + + def pack_pixels2d(sub: pygame.Surface, out: np.ndarray) -> bytes: """pixels2d: lock the surface as a (width, height) array of mapped pixels. @@ -125,30 +164,27 @@ def pack_pixels2d(sub: pygame.Surface, out: np.ndarray) -> bytes: def _check_all_match() -> None: - rng = np.random.default_rng(7) - # Use a size that's not a power-of-2 to catch stride bugs + # Odd width, non-power-of-2 height: catches stride bugs and the SDL row pad. w, h = 13, 47 - rgba = rng.integers(0, 256, (h, w, 4), dtype=np.uint8) - # Build an RGBA surface from random data - surf = pygame.Surface((w, h), pygame.SRCALPHA) - pygame.surfarray.blit_array(surf, rgba[:, :, :3].transpose(1, 0, 2).copy()) - out = np.empty((h, w, 2), dtype=np.uint8) + for label, surf in (("RGBA", make_rgba_surface(w, h)), ("opaque", make_opaque_surface(w, h))): + out = np.empty((h, w, 2), dtype=np.uint8) + ref = pack_tobytes(surf, out.copy()) - ref = pack_tobytes(surf, out.copy()) - - variants = [ - ("pixels3d_transpose", pack_pixels3d_transpose(surf, out.copy())), - ("pixels3d_contig", pack_pixels3d_contig(surf, out.copy())), - ("array3d", pack_array3d(surf, out.copy())), - ("pixels2d", pack_pixels2d(surf, out.copy())), - ] - for name, result in variants: - assert result == ref, ( - f"{name} mismatch at ({w}x{h}):\n" - f" ref[0:8]={list(ref[:8])}\n got[0:8]={list(result[:8])}" - ) - print("Correctness check passed: all variants produce identical bytes.\n") + variants = [ + ("pixels3d_transpose", pack_pixels3d_transpose(surf, out.copy())), + ("pixels3d_contig", pack_pixels3d_contig(surf, out.copy())), + ("array3d", pack_array3d(surf, out.copy())), + ("pixels2d", pack_pixels2d(surf, out.copy())), + ("sdl_blit", pack_sdl_blit(surf, out.copy())), + ] + for name, result in variants: + assert result == ref, ( + f"{name} mismatch on {label} source at ({w}x{h}):\n" + f" ref[0:8]={list(ref[:8])}\n got[0:8]={list(result[:8])}" + ) + print("Correctness check passed: all variants produce identical bytes " + "(both source formats, odd width).\n") # --------------------------------------------------------------------------- @@ -208,16 +244,26 @@ def _print(r: Result, baseline_us: float | None = None) -> None: # --------------------------------------------------------------------------- -def make_rgba_surface(w: int, h: int) -> pygame.Surface: - """RGBA surface with random pixel data (matches PanelStack format).""" +def _fill_random(surf: pygame.Surface, w: int, h: int) -> pygame.Surface: rng = np.random.default_rng(42) - surf = pygame.Surface((w, h), pygame.SRCALPHA) - data = rng.integers(0, 256, (h, w, 4), dtype=np.uint8) - # blit_array expects (w, h, 3) for RGB - pygame.surfarray.blit_array(surf, data[:, :, :3].transpose(1, 0, 2).copy()) + data = rng.integers(0, 256, (h, w, 3), dtype=np.uint8) + pygame.surfarray.blit_array(surf, data.transpose(1, 0, 2).copy()) return surf +def make_rgba_surface(w: int, h: int) -> pygame.Surface: + """Today's PanelStack root format (use_dimming=True).""" + return _fill_random(pygame.Surface((w, h), pygame.SRCALPHA), w, h) + + +def make_opaque_surface(w: int, h: int) -> pygame.Surface: + """Opaque RGB root: what the SDL blit path would need. Same 8-bit blend + precision as RGBA — a dest alpha channel buys nothing for alpha blending.""" + return _fill_random( + pygame.Surface((w, h), depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)), w, h + ) + + def run(sizes: list[tuple[int, int]], iters: int) -> None: MAX_W = max(w for w, h in sizes) MAX_H = max(h for w, h in sizes) @@ -225,19 +271,23 @@ def run(sizes: list[tuple[int, int]], iters: int) -> None: out_col = np.empty((MAX_W, MAX_H, 2), dtype=np.uint8) # col-major (w,h,2) for w, h in sizes: - surf = make_rgba_surface(w, h) - px = w * h - print(f"\n{'='*72}") + px = w * h + print(f"\n{'='*78}") print(f" {w}x{h} ({px:,} px, {px*2:,} B RGB565)") - print(f"{'='*72}") + print(f"{'='*78}") + + rgba = make_rgba_surface(w, h) + opaque = make_opaque_surface(w, h) results = [ - bench("tobytes (baseline)", lambda s=surf: pack_tobytes(s, out), iters), - bench("pixels3d + transpose", lambda s=surf: pack_pixels3d_transpose(s, out), iters), - bench("pixels3d + ascontiguousarray", lambda s=surf: pack_pixels3d_contig(s, out), iters), - bench("array3d + transpose", lambda s=surf: pack_array3d(s, out), iters), - bench("pixels2d + transpose", lambda s=surf: pack_pixels2d(s, out), iters), - bench("pixels3d no-transpose [WRONG]",lambda s=surf: pack_pixels3d_colmajor(s, out_col), iters), + bench("pixels3d + transpose (was PROD)", lambda s=rgba: pack_pixels3d_transpose(s, out), iters), + bench("tobytes", lambda s=rgba: pack_tobytes(s, out), iters), + bench("pixels3d + ascontiguousarray", lambda s=rgba: pack_pixels3d_contig(s, out), iters), + bench("array3d + transpose", lambda s=rgba: pack_array3d(s, out), iters), + bench("pixels2d + transpose", lambda s=rgba: pack_pixels2d(s, out), iters), + bench("sdl blit [RGBA src, as doc]", lambda s=rgba: pack_sdl_blit(s, out), iters), + bench("sdl blit [opaque src] (PROD)", lambda s=opaque: pack_sdl_blit(s, out), iters), + bench("pixels3d no-transpose [WRONG]",lambda s=rgba: pack_pixels3d_colmajor(s, out_col), iters), ] baseline = results[0].median_us @@ -245,7 +295,7 @@ def run(sizes: list[tuple[int, int]], iters: int) -> None: _print(r, baseline_us=None if r is results[0] else baseline) print() - print(f" Speedup summary vs tobytes baseline ({baseline:.1f} µs median):") + print(f" Speedup summary vs the old numpy pack ({baseline:.1f} µs median):") for r in results[1:]: delta = baseline - r.median_us sign = "faster" if delta > 0 else "SLOWER" diff --git a/uilib/lcd_ili9341.py b/uilib/lcd_ili9341.py index d3b6029c9..0846dcb14 100644 --- a/uilib/lcd_ili9341.py +++ b/uilib/lcd_ili9341.py @@ -14,7 +14,6 @@ # along with pi-stomp. If not, see . import pygame -import numpy as np from uilib.panel import LcdBase, Box from uilib.spi_timing import transfer_ms as spi_transfer_ms @@ -67,7 +66,10 @@ def __init__(self, spi, cs_pin, dc_pin, reset_pin, baudrate, flip=True): # Landscape dimensions presented to the UI (matches the panel post-MADCTL). self.width = self.disp.height # 320 self.height = self.disp.width # 240 - self._pixels = np.empty((self.height, self.width, 2), dtype=np.uint8) + # 565 staging surface: SDL converts RGB888->RGB565 in one C blit, far + # cheaper than packing it in numpy. Masks spelled out -- a bare Surface + # inherits the display format. See CLAUDE.md "Traps". + self._565 = pygame.Surface((self.width, self.height), depth=16, masks=(0xF800, 0x07E0, 0x001F, 0)) def _block_fast(self, x0, y0, x1, y1, data=None): """Bypass adafruit_rgb_display's write method to perform a block write @@ -158,9 +160,13 @@ def clear(self): def update(self, image: pygame.Surface, box=None): """Push (a sub-rect of) the composed pygame surface to the LCD. - Converts surface → RGB888 bytes → packed RGB565, writing via Display._block - to bypass PIL. The panel runs landscape-native (MADCTL set in __init__) so - no rotation is needed.""" + Quantises to RGB565 via an SDL convert-blit, writing via Display._block to + bypass PIL. The panel runs landscape-native (MADCTL set in __init__) so no + rotation is needed. + + `image` must be opaque: blitting an SRCALPHA source takes SDL's per-pixel + alpha-blending blitter instead of a straight convert, and is ~7x slower + than the numpy pack this replaced. PanelStack's root is opaque XRGB.""" if self.lock.locked(): logging.debug("LCD update was locked by another thread") self.lock.acquire() @@ -181,14 +187,14 @@ def update(self, image: pygame.Surface, box=None): # Landscape-native: surface coords map straight to the panel address # window, so the RGB565 sub-rect ships row-major with no rotation. sw, sh = sub.get_size() - with profiling.measure("lcd.update:pack"): - arr = pygame.surfarray.pixels3d(sub).transpose(1, 0, 2) - - pix = self._pixels[:sh, :sw] - g = arr[:, :, 1] - pix[:, :, 0] = (arr[:, :, 0] & 0xF8) | (g >> 5) - pix[:, :, 1] = ((g & 0x1C) << 3) | (arr[:, :, 2] >> 3) - pixels_bytes = pix.tobytes() + self._565.blit(sub, (0, 0)) + + # Staging is full-width, so slicing the sub-rect out of it sidesteps + # SDL's 4-byte row padding, which an odd-width rect would otherwise hit. + # Surface pixels are native little-endian; the panel wants high byte first. + staged = pygame.surfarray.pixels2d(self._565).T + pixels_bytes = staged[:sh, :sw].byteswap().tobytes() + del staged # unlock the surface with profiling.measure("lcd.update:_block(SPI)"): self.disp._block(x1, y1, x1 + sw - 1, y1 + sh - 1, pixels_bytes) diff --git a/uilib/panel.py b/uilib/panel.py index 146c61e63..bdf5b779c 100644 --- a/uilib/panel.py +++ b/uilib/panel.py @@ -353,10 +353,12 @@ def __init__( # and the offset remains 0,0 (don't try to scroll) if box is None: box = Box((0, 0), lcd.dimensions()) + # The root stays opaque even when dimming. It is a blend *destination*: + # 32-bit keeps full 8-bit blend precision, but a dest alpha channel buys + # nothing, and an SRCALPHA root would force the LCD's 565 convert-blit + # down SDL's alpha-blending path (~7x slower). if image_format is None: image_format = lcd.default_format() - if use_dimming: - image_format = "RGBA" trace(self, "Panel stack initializing with box=", box) super(PanelStack, self).__init__(box=box, image_format=image_format) diff --git a/uilib/spi_timing.py b/uilib/spi_timing.py index be0ec6a36..a019441a8 100644 --- a/uilib/spi_timing.py +++ b/uilib/spi_timing.py @@ -20,35 +20,111 @@ emulator's transfer-time simulation, and lcd320x240's poll_divisor. """ -# Device target: BAUDR=4 → 50.00 MHz actual (Pi 4/5; see pistomp-arch firstboot.sh). -DEVICE_SPI_HZ: float = 50_000_000.0 +import math +from pathlib import Path +from typing import NamedTuple, Optional + +_DT_COMPATIBLE = Path("/proc/device-tree/compatible") + +# SPI controller source clock by SoC. spi-bcm2835 (Pi <=4) divides the VPU core +# clock; spi-dw (Pi 5, inside RP1) divides RP1_CLK_SYS. +SPI_SOURCE_HZ: dict[str, int] = { + "brcm,bcm2837": 400_000_000, # Pi 3 (v2) + "brcm,bcm2711": 500_000_000, # Pi 4 + "brcm,bcm2712": 200_000_000, # Pi 5 (v3) +} + +# Off-device (tests, emulator). +DEFAULT_SOURCE_HZ: int = 200_000_000 + + +def soc_key() -> Optional[str]: + """Device-tree `compatible` entry naming the running SoC, if we know it.""" + try: + raw = _DT_COMPATIBLE.read_bytes() + except OSError: + return None + for entry in raw.decode("ascii", errors="replace").split("\0"): + if entry in SPI_SOURCE_HZ: + return entry + return None + + +def spi_source_hz() -> int: + """SPI controller source clock for the running host.""" + soc = soc_key() + return DEFAULT_SOURCE_HZ if soc is None else SPI_SOURCE_HZ[soc] + + +def actual_spi_hz(requested_hz: float, source_hz: Optional[int] = None) -> float: + """The clock the SPI controller will really run at for `requested_hz`. + + Both drivers take ceil(source / requested) then round that divisor up to an + even number, so the achieved clock lands on source/even and never exceeds + the request. One hertz below an exact divisor point costs a whole step: on a + Pi 3, 66_666_666 gives 50 MHz and 66_666_667 gives 66.67 MHz. + """ + if requested_hz <= 0: + raise ValueError(f"requested_hz must be positive, got {requested_hz}") + source = spi_source_hz() if source_hz is None else source_hz + + # Integer DIV_ROUND_UP; float division rounds the wrong way at divisor points. + divisor = -(-source // math.floor(requested_hz)) + if divisor < 2: + divisor = 2 + elif divisor % 2: + divisor += 1 + return source / divisor + -# Constants fit from on-device timing of LcdIli9341.update() at 20 MHz, 33.3 MHz, -# and 50 MHz actual (Pi 5 / Python 3.14, tools/bench_lcd_device.py). -# -# DW APB SSI BAUDR rounding means only these speeds are achievable: -# request 20 MHz (BAUDR=10) → 20.00 MHz actual -# request 34–49 MHz (BAUDR=6) → 33.33 MHz actual -# request 50–99 MHz (BAUDR=4) → 50.00 MHz actual -# # The push cost is affine: # fixed per-call + clock-independent per-pixel + bits-on-the-wire / clock # -# Fit holds within ~1% across all three clocks and the full size range. +# fixed and per-pixel are CPU-bound, so they are *per-SoC* -- an A53 at 1.2 GHz is +# nothing like an A76 at 2.4 GHz, and v2's per-pixel cost is 6.2x v3's. Sharing one +# global would let v3's cheap per-pixel cost admit a 31.7k-px clip inline on v2 that +# really costs 9.6 ms, against an 8 ms budget on a 10 ms tick. Underestimating is the +# unsafe direction -- transfer_ms gates inline pushes onto the UI thread +# (panel.py INLINE_BUDGET_MS). +# +# Refit with tools/bench_lcd_device.py, on each board, whenever the pack path changes. + + +class PushProfile(NamedTuple): + fixed_ms: float # address-window commands + Python/driver call overhead + pipeline_ms_per_px: float # 565 convert-blit + driver; no cheaper on a faster clock + bits_per_pixel: float # ~16 (RGB565); the fit converging there means no framing + + +# Fit from on-device timing of LcdIli9341.update() across three clocks per board +# (Python 3.13, SDL convert-blit pack). Errors: v3 rms 0.4%, v2 rms 3.1%. +_PI3 = PushProfile(1.2008, 2.3647e-05, 15.923) # Pi 3A+ (v2), fit @ 20/40/66.7 MHz + +PUSH_PROFILE: dict[str, PushProfile] = { + "brcm,bcm2837": _PI3, + "brcm,bcm2711": _PI3, # Pi 4 unmeasured; the slower board is the safe guess + "brcm,bcm2712": PushProfile(0.2727, 3.8290e-06, 16.032), # Pi 5 (v3) @ 20/33.3/50 MHz +} + +# Off-device (tests, emulator) — matches DEFAULT_SOURCE_HZ's Pi 5 assumption. +DEFAULT_PROFILE = PUSH_PROFILE["brcm,bcm2712"] -# Wire bits per pixel: 16 (RGB565). The fit converges very close to 16, -# confirming negligible framing overhead at these clock speeds. -BITS_PER_PIXEL = 16.0071 -# Clock-independent per-pixel cost: numpy 565 packing + driver. Doesn't get -# cheaper with a faster clock, so it floors how fast large pushes can go. -PIPELINE_MS_PER_PX = 5.856e-05 +def push_profile() -> PushProfile: + """Affine push-cost model for the running host. -# Fixed per-call cost: address-window commands + Python/driver call overhead. -FIXED_MS = 0.7117 + An unrecognised SoC on a *real* board (a Pi 2, a future Pi 6) gets the slowest + profile we have, not the default: overestimating only defers a push to the + worker, while underestimating stalls the poll loop. + """ + soc = soc_key() + if soc is not None: + return PUSH_PROFILE[soc] + return DEFAULT_PROFILE if not _DT_COMPATIBLE.exists() else _PI3 def transfer_ms(pixels: int, spi_hz: float) -> float: """Estimated milliseconds to push `pixels` to an SPI display at `spi_hz`.""" - wire_ms = pixels * BITS_PER_PIXEL / spi_hz * 1000 - return FIXED_MS + pixels * PIPELINE_MS_PER_PX + wire_ms + p = push_profile() + wire_ms = pixels * p.bits_per_pixel / spi_hz * 1000 + return p.fixed_ms + pixels * p.pipeline_ms_per_px + wire_ms