[finger563]

Summary

Audit of how the M5Stack Tab5's three display variants (ILI9881 / ST7123 / ST7121) handle rotation, plus fixes for DSI scan-out underrun (display streaking) found while testing on hardware.

The branch originally routed 0°/180° rotation on the ST7121 TDDI variant to the panel itself (MADCTL GS/SS scan-direction flip) instead of the ESP32-P4 PPA. Hardware testing showed the 180° MADCTL flip renders corrupted on (at least some) ST7121 units — the init sequence programs full TDDI gate/source mux tables (the 0xAC block) matched to the normal scan direction, and a MADCTL GS flip alone reorders gate scanning without swapping them. Panel-side rotation is therefore experimental and opt-in, gated behind CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION (default off). The default build keeps the pre-branch known-good behavior: PPA rotation for all orientations and zero runtime MADCTL writes.

What landed

Rotation audit + plumbing (default behavior unchanged)

• Audit conclusions documented in code: these are MIPI-DSI video-mode (DPI) panels — a fixed 720x1280 raster with no axis swap — so panel-side rotation could only ever cover 0/180; 90/270 must stay on the PPA. And on the ST7121 specifically, the init-time gate/source mux tables make even runtime 0/180 MADCTL flips unsafe.
• Wired Display::LvglConfig::rotation_callback to a new M5StackTab5::on_display_rotation(); the rotation decision is centralized in a single predicate (panel_handles_rotation()) shared by the flush path and the callback.
• Enum unification (DisplayRotation <-> lv_display_rotation_t mapping pinned down with explicit converters) and init-order hardening: the current rotation is (re)applied when the display driver comes up, correct in either initialize_lcd() / initialize_display() order, with the thread-safety of the init-path call documented at the call site.
• Experimental opt-in: CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION (default n) with the corruption warning in its Kconfig help. When enabled, the ST7121 applies 0/180 via the display driver's set_rotation() (MADCTL) and flush() skips the PPA for those orientations.
• ILI9881 / ST7123 behavior unchanged; touch mapping (touchpad_convert) is invariant to the transform mechanism; camera PPA pass unaffected.

DSI underrun / streaking fixes (active by default)

Streaking traced to DSI bridge FIFO underrun from PSRAM read contention against the DPI panel's continuous ~140 MB/s framebuffer scan-out. Three traffic reductions:

• DMA2D re-enabled for draw_bitmap: IDF 6.0 removed flags.use_dma2d in favor of esp_lcd_dpi_panel_enable_dma2d(), which was never called — every flush had silently regressed to a CPU memcpy (~5.4 MB of PSRAM/cache traffic per full frame).
• PPA rotates directly into the DPI framebuffer (output block placement) instead of PPA -> scratch buffer -> draw_bitmap copy, halving the rotation path's PSRAM traffic.
• PPA burst length 128 -> 64 on both PPA clients (display + camera); full-length PPA bursts are known to starve the DSI scan-out even at 200 MHz hex PSRAM ((draw/ppa) cause DSI underrun under heavy load on ESP32-P4 lvgl/lvgl#9590).

Hardware testing

• ST7121 unit: 180° MADCTL scan flip renders corrupted -> panel-side rotation gated off by default (see PR discussion for the full root-cause analysis and the streaking diagnosis / escalation ladder).

Testing

• Tab5 example builds clean in both configurations (CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION off and on) with ESP-IDF 6.0, esp32p4.

🤖 Generated with Claude Code

[github-actions]

✅Static analysis result - no issues found! ✅

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR updates M5Stack Tab5 rotation handling so the ST7121 (TDDI) panel uses the panel’s MADCTL scan-direction flip for 0°/180° rotation, avoiding PPA/software rotation artifacts with partial flush areas.

Changes:

• Wire Display::LvglConfig::rotation_callback to M5StackTab5::on_display_rotation() to forward 0°/180° rotation to the ST7121 driver.
• Add panel_handles_rotation() to decide when to skip PPA/software rotation.
• Update flush() to bypass rotation when the panel applies it (ST7121, 180°).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
components/m5stack-tab5/src/video.cpp	Adds rotation callback handling, panel-rotation decision logic, and adjusts flush rotation path accordingly.
components/m5stack-tab5/include/m5stack-tab5.hpp	Declares the new rotation callback handler and helper used by the updated flush logic.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[finger563]

Hardware test result (ST7121 unit): the MADCTL 180° scan flip renders corrupted — addressed in 33eb080.

Root cause analysis: the ST7121 init sequence programs full TDDI gate/source mux tables (the 0xAC block), which vendor inits pair to a specific scan direction. A MADCTL GS flip alone reorders gate scanning without swapping those tables → interleaved garbage. Corroborating evidence: espp's GS/SS bit positions match Espressif's official esp_lcd_st7121 mirror implementation (generic template code, likely never validated on this glass), and ESPHome's mipi_dsi driver documents the same 0/180-only constraint for DSI panels generally — the design was sound; this panel's init tables are what make runtime flips unsafe.

Resolution: panel-side rotation is now gated behind CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION (default off). The default build is exactly the pre-branch known-good behavior — PPA rotation for all orientations and zero runtime MADCTL writes. The branch's remaining value stands: the audit conclusion (documented in code), init-order hardening, the single-predicate rotation decision, and enum unification. Both configurations build clean.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

[finger563]

Display streaking: root cause + fixes (513f565)

The streaking is consistent with DSI bridge FIFO underrun from PSRAM read contention: the DPI (video-mode) panel scans its PSRAM framebuffer continuously at ~140 MB/s via DW-GDMA, and when competing PSRAM/AXI traffic starves that stream the bridge discards pixels, which renders as streaks along the physical scan-line axis in any orientation. This matches "worse with HW_ROTATION off" (that config runs the PPA + an extra full-frame copy on every flush). Note: on this board's pre-v3.0 P4 silicon the driver's underrun happens error log may never fire (the underrun interrupt is only supported on rev v3.0 per IDF v5.5.2 release notes), so absence of that log does not rule underrun out.

Three traffic reductions landed in 513f565:

1. DMA2D re-enabled for draw_bitmap — IDF 6.0 removed flags.use_dma2d in favor of esp_lcd_dpi_panel_enable_dma2d(), which we never called, so every flush had silently regressed to a CPU memcpy (~5.4 MB of cache/PSRAM traffic per full frame). M5Stack's UserDemo and esp-bsp's m5stack_tab5 both use DMA2D here.
2. PPA rotates directly into the DPI framebuffer (output block placement) instead of PPA -> scratch buffer -> draw_bitmap copy — halves the rotation path's PSRAM traffic.
3. PPA burst length 128 -> 64 on both PPA clients (display + camera) — full-length PPA bursts are known to starve the DSI scan-out even at 200 MHz hex PSRAM ((draw/ppa) cause DSI underrun under heavy load on ESP32-P4 lvgl/lvgl#9590).

If streaks persist, please try in this order

1. Rule out the camera: comment out initialize_camera() in the example. The CSI/ISP pipeline writes ~100+ MB/s into PSRAM; camera-induced DSI underrun at max PSRAM config is documented ((draw/ppa) cause DSI underrun under heavy load on ESP32-P4 lvgl/lvgl#9590). If this fixes it, the remaining fix is camera-side (lower preview rate/resolution).
2. PPA burst 32: change PPA_DATA_BURST_LENGTH_64 -> PPA_DATA_BURST_LENGTH_32 in video.cpp and camera.cpp.
3. Larger L2 cache: in example/sdkconfig set CONFIG_CACHE_L2_CACHE_512KB=y (keep CONFIG_CACHE_L2_CACHE_LINE_128B=y) — fewer CPU misses to PSRAM.
4. Disable power management: set CONFIG_PM_ENABLE=n (M5Stack's UserDemo ships without PM; the DPI driver pins CPU freq, but PM still adds latency/jitter elsewhere).
5. XIP experiment: toggle CONFIG_SPIRAM_XIP_FROM_PSRAM (currently y). It removes flash-cache stalls but adds instruction-fetch traffic to PSRAM; Espressif's own P4 external-RAM guide says to profile both ways.
6. Lower DPI clock: dpi_clock_freq_mhz 70 -> 65/60 in video.cpp (ST7121/ST7123 branches). Caveat: the TDDI touch scan is timed against the pixel clock, so verify touch still works.
7. AXI-ICM QoS (last resort, experimental): IDF ships hal/axi_icm_ll.h with per-master token-bucket regulators (axi_icm_ll_set_qos_burstiness(AXI_ICM_MASTER_CPU/DMA2D, ..., AXI_ICM_ACCESS_READ)) that could throttle the CPU/DMA2D masters so the DW-GDMA scan-out wins arbitration — nothing in IDF programs it yet, so this would be a hand-rolled experiment.

If a serial console is attached during streaking, any lcd.dsi.dpi: ... underrun happens message would confirm the diagnosis outright (only possible on rev v3 silicon).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

components/m5stack-tab5/src/video.cpp:498

• Publishing true here exposes the panel before its initial MADCTL state is applied below. In the supported reversed-init order, an already-running LVGL thread can flush in this window; with experimental 180° rotation, panel_handles_rotation() skips PPA even though the panel is still in its old scan direction. Apply the initial panel rotation before opening the gate (using an init-only helper that does not require the gate).

  lcd_initialized_.store(true, std::memory_order_release);

components/m5stack-tab5/src/video.cpp:154

• Clearing an atomic flag does not wait for readers that already observed the previous true. A concurrent flush() or write_lcd_lines() can pass its acquire-load, then race the re-init writes to display_driver_, display_controller_, or framebuffer state despite the stated guarantee. Either prohibit re-initialization while readers run, or use reader/writer synchronization that waits for in-flight operations before rebuilding this state.

  lcd_initialized_.store(false, std::memory_order_release);

[finger563]

Addressed the two suppressed comments from the latest Copilot review in b85dd72 (same commit as the DMA2D gating fix).

1. video.cpp:498 — gate published before the initial MADCTL apply

Valid: in the reversed init order (initialize_display() first), an already-running LVGL thread could flush in the window between lcd_initialized_.store(true) and the initial on_display_rotation() call that followed it — panel_handles_rotation() would skip PPA rotation while the panel's MADCTL still held its old scan direction.

Fixed by restructuring so the gate opens last:

• The MADCTL/rotation core moved into a gate-free private helper, apply_panel_rotation(). on_display_rotation() (the LVGL rotation callback) is now just the lcd_initialized_ acquire-load check followed by the helper.
• initialize_lcd() calls apply_panel_rotation() directly before the store-release. This bypasses the gate, which is safe: it runs on the same init thread that just wrote display_driver_/display_controller_ (no synchronization needed against itself), and any concurrent flush() still no-ops on the still-closed gate. So by the time any flush can observe the gate open, the panel scan direction already matches panel_handles_rotation()'s decision.
• The per-init-order safety analysis at the call site (documented order: display_ null, nothing can race, redundant-but-harmless vs the espp::Display ctor's synchronous rotation event; reversed order: MADCTL rides the DSI command channel, arbitrated in hardware against the video stream) was kept and updated for the new ordering.

2. video.cpp:154 — clearing the gate does not wait for in-flight readers

Also valid: lcd_initialized_.store(false) on re-entry cannot un-publish state from a flush()/write_lcd_lines() that already passed its acquire-load of true; rebuilding lcd_handles_/display_driver_/the framebuffer pair underneath such a reader would be a race despite the gate.

Of the two options offered, I took "prohibit re-initialization": initialize_lcd() now refuses to run once the gate has opened (warn + early-return true), rather than adding reader/writer synchronization (e.g. an rwlock or RCU-style quiescence wait) to the per-frame flush hot path for a re-init this BSP never performs — the panel hardware is fixed at boot, and no espp BSP supports LCD re-init. Consequences, now documented on the flag:

• The gate is monotonic: false -> true, never back. The store(false) is gone; on entry the gate is provably closed (first call, or retry after a failed attempt that never reached the final store-release), so no reader can have observed true while the state is being built — retry-after-failure stays supported.
• The stale "clears this flag on entry" / "mid re-init" wording in the header and flush() comments was updated to match.

Both CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION states build cleanly (default off, and =y).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

components/m5stack-tab5/src/video.cpp:794

• With the experimental option enabled, this drops every rotation event that occurs after initialize_display() but before the LCD gate opens. initialize_lcd() then applies the compile-time initial rotation, not the current/pending value; for example, if LVGL changes to 180° during the documented reversed-init window, the panel is programmed to 0°, while the first flush observes 180° and panel_handles_rotation() skips PPA rotation. Preserve pending rotations and synchronize the final apply/publication step so an event cannot be lost between applying MADCTL and opening the gate.

  if (!lcd_initialized_.load(std::memory_order_acquire)) {
    return;
  }

[Alaining]

Hi @finger563, thanks for this PR. I built extra functionality on top of it. Please check #847

New functionality: Rotated screen remains running at high FPS (within 5FPS wrt non-rotated) and screen tearing was fixed. This was achieved by using more than one DPI frame buffer.

Thanks!

[Alaining]

A correction to my earlier comment: please ignore what I said about #847 increasing the FPS.

• feat(m5stack-tab5): tear-free rendering with two DPI frame buffers #847 is now just optional tear-free rendering, off by default: 2 frame buffers, LVGL draws into
  the hidden one, and the flush switches buffers and waits for frame-done. The numbers are in its
  description.
• The price of a tear-free UI: 1.2 MB more PSRAM, no display rotation in that mode, and heavy
  animations run at half the panel rate (29 FPS) instead of tearing.

[finger563]

Hardware results on a Tab5 (ST71xx panel, ESP-IDF v6.1)

Measured with a small display benchmark added locally to the m5stack-tab5 example. It runs before the GUI starts, sets LVGL's refresh timer to 1 ms, and runs each phase for 8 s at 0° and again at 90°:

• small: a counter label changes every frame;
• full: the whole screen changes colour every frame;
• bar: a full-height bar sweeps across the screen (tearing check);
• heavy: 24 gradient boxes move every frame.

DSI underruns were counted from the driver's "underrun happens" log lines. These are single runs. Branches tested: espp main (base), #738, and #847, both PRs with main merged in locally (see the note below).

Phase	base 0°	base 90°	#738 0°	#738 90°	#847 (1 FB) 0°	#847 (1 FB) 90°	#847 (2 FBs, direct) 0°
full	25.0 fps	10.1 fps	35.9 fps	13.1 fps	35.9 fps	13.1 fps	57.8 fps
heavy	14.8 fps	7.9 fps	18.2 fps	9.7 fps	18.2 fps	9.7 fps	19.2 fps
bar	181 fps	167 fps	283 fps	224 fps	283 fps	224 fps	57.7 fps
small	996 fps	1000 fps	1000 fps	1000 fps	1000 fps	1000 fps	57.7 fps
DSI underruns	0	0	0	0	0	0	0
PSRAM free at start	24.5 MB		24.5 MB		24.5 MB		28.3 MB

small and bar above about 58 fps only show how fast LVGL renders and flushes a small dirty area; the panel refreshes at about 58 Hz. In 2-FB direct mode each frame waits for a panel refresh, so those phases cap at the panel rate. The 2-FB mode can't rotate, so it has no 90° column. Its 3.7 MB of extra free PSRAM is because the example's full-frame LVGL draw buffers aren't needed in that mode.

Takeaways:

• fix(m5stack-tab5): rotation audit, experimental opt-in ST7121 panel rotation (default off), and DSI underrun fixes #738: DMA2D gives about 45% faster full redraws at 0° and about 30% at 90°, with no underruns and no colour problems. The camera, colours and touch look right.
• feat(m5stack-tab5): tear-free rendering with two DPI frame buffers #847 with 1 FB performs identically to fix(m5stack-tab5): rotation audit, experimental opt-in ST7121 panel rotation (default off), and DSI underrun fixes #738.
• feat(m5stack-tab5): tear-free rendering with two DPI frame buffers #847 with 2 FBs: no tearing in the bar phase, and full redraws run at the panel rate. Rotation does nothing in that mode, as documented. Rotation, touch and the camera tab work at every rotation with 1 FB.
• Rotated flushes are much slower than unrotated ones: about 76 ms per full frame through the PPA, against 28 ms without rotation (about 100 ms on main). We're looking into the PPA path next.
• Not tested: ILI9881 and ST7121, and the opt-in ST7121 hardware rotation.

Build note: neither PR builds as-is on ESP-IDF v6.1. v6.1 added esp_lcd_dpi_panel_event_callbacks_t::on_vsync, so the designated initializer in video.cpp (cbs = {.on_color_trans_done = ..., .on_refresh_done = nullptr}) fails with -Werror=missing-field-initializers. main already zero-initializes the struct and assigns the callbacks. Rebasing onto main fixes it; the only conflict is that initializer, and keeping main's form plus the PRs' additions resolves it. The rebase also brings in BSP APIs that downstream users rely on (touch_state(), touch_state_convert(), sdcard_component()).

🤖 Generated with Claude Code

[finger563]

Updated the branch so it merges cleanly and closes the one correctness hole from the review:

• Merged main (the only conflict was the zero-initialised esp_lcd_dpi_panel_event_callbacks_t from fix(bsp): initialize new DPI panel event callback members for IDF v6.1 #782 vs. the draw-completion semaphore; resolved keeping both).
• draw_and_wait() no longer reports a timed-out transfer as success (33d836c). It returned true after its 200 ms bounded wait expired, so a wedged DMA2D copy that might still be reading the caller's buffer looked finished: the LVGL flush handed the draw buffer back for re-rendering and write_lcd_lines() callers reused theirs, silently. It now returns false and warns (rate-limited); the late completion, if it ever arrives, is still drained by the next call instead of being paired with it. The stale "asynchronous" note on write_lcd_lines() is gone too.

Hardware check on a Tab5 (ST71xx, ESP-IDF v6.1): display benchmark phases at 0° and 90° (small updates, full-screen redraws, sweeping bar, render-heavy animation), 60 s total, no draw timeouts, no DSI underruns, no errors; then the example GUI came up normally.

phase	0°	90° (PPA)
small	1000 fps	514 fps
full	35.5 fps	12.0 fps
bar	246 fps	170 fps
heavy	13.1 fps	6.8 fps

Left as-is, for a follow-up if wanted: the flush is now synchronous on the GUI task (before, the ISR signalled LVGL so a DMA2D copy overlapped rendering of the next area). A pending_op_ recorded under panel_op_mutex_ would let the ISR call lv_display_flush_ready() for LVGL draws and give the semaphore for direct writes, keeping the pipelining while still fixing the completion mix-up this PR addresses.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

[finger563]

Pushed 250a7ef: the LVGL flush is asynchronous again, without giving up what the serialization fixed.

• flush() issues its draw_bitmap and returns; the transfer-done ISR signals LVGL (lv_display_flush_ready) and clears an in-flight flag. Only flush() sets lvgl_flush_pending_, so a direct write's completion never reaches LVGL.
• Every panel operation (LVGL draw, write_lcd_lines(), the PPA rotation into the framebuffer, the software rotation into the scratch buffer) first waits for the transfer in flight under panel_op_mutex_ (wait_for_transfer_idle()), so only one transfer is ever in flight and no engine writes a buffer another is still reading. A timed-out transfer is given up on and LVGL released instead of stalling every later draw.
• write_lcd_lines() stays synchronous, so callers may reuse their buffer on return.

Same Tab5 (ST71xx, ESP-IDF v6.1), same build and benchmark as the table above, no DSI underruns, no draw timeouts:

phase	synchronous (before)	asynchronous (now)
small, 0°	1000 fps	993 fps
full, 0°	35.5 fps	38.4 fps
bar, 0°	246 fps	345 fps
heavy, 0°	13.1 fps	15.7 fps
90° phases (PPA)	514 / 12.0 / 170 / 6.8	571 / 12.0 / 171 / 6.8

The gain lands on mid-sized partial flushes, the usual LVGL workload. Rotated output is unchanged because the PPA call still blocks; making that one non-blocking too would be a separate change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU

[Copilot]

🟡 Changes recommended

Critical DMA completion races and timeout handling can release source buffers while transfers remain active, allowing unsafe memory reuse and misattributed completions.

4 open issues

• Do not release source buffers or clear busy state on timeout · New
• Keep transfer in flight until its completion token is consumed · New
• Retain rotation requests received before the initialization gate opens · New
• Dispatch rotation events through the emitting display · New

1 resolved since last review

• Enabling the IDF 6 DMA2D hook makes draw_bitmap asynchronous and single-flight: the hook returns…

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Critical concurrency risks involving overlapping DMA2D/PPA transfers and mismatched panel rotation during initialization require human assessment.

No open issues 🎉

4 resolved since last review

• Keep transfer in flight until its completion token is consumed
• Do not release source buffers or clear busy state on timeout
• Dispatch rotation events through the emitting display
• Retain rotation requests received before the initialization gate opens

_{Review effort: Balanced}