From 6b6b9472d75ba63444de14b74258b9a374f74095 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 28 Aug 2026 09:20:23 -0500 Subject: [PATCH 01/15] feat(m5stack-tab5): use display-driver (MADCTL) rotation for the ST7121 variant All three Tab5 display variants previously rotated every non-zero LVGL orientation at flush time via the ESP32-P4 PPA (with an lv_draw_sw_rotate fallback); the espp::Display rotation_callback was left null, so the display driver's set_rotation() (MADCTL) support was never used. Route rotation through the espp::Display machinery for the ST7121: - Wire the Display rotation_callback to a new on_display_rotation() which, for the ST7121, forwards 0/180 to the driver's set_rotation() (MADCTL GS/SS scan-direction flip done by the panel itself) and restores the natural scan direction for 90/270. - Skip the PPA/software buffer rotation in flush() when the panel handles the current rotation (new panel_handles_rotation()); the logical frame is then written unrotated and the panel flips it at scan-out, which lands partial areas exactly where LVGL's rotated mapping expects them. - 90/270 keep the PPA path: the Tab5 panels are MIPI-DSI DPI (video mode) panels streaming a fixed 720x1280 raster, so the panel cannot swap axes. The ILI9881 and ST7123 variants keep their existing PPA rotation path, and the touch transform is unchanged: touchpad_convert() maps physical to logical coordinates from the LVGL rotation, which is independent of how the display rotation is implemented. Co-Authored-By: Claude Fable 5 --- .../m5stack-tab5/include/m5stack-tab5.hpp | 8 +++ components/m5stack-tab5/src/video.cpp | 50 ++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/components/m5stack-tab5/include/m5stack-tab5.hpp b/components/m5stack-tab5/include/m5stack-tab5.hpp index 669ecffb30..19ae1cab30 100644 --- a/components/m5stack-tab5/include/m5stack-tab5.hpp +++ b/components/m5stack-tab5/include/m5stack-tab5.hpp @@ -952,6 +952,14 @@ class M5StackTab5 : public BaseComponent { esp_err_t (*original_panel_init_)(esp_lcd_panel_t *panel){nullptr}; void flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_map); + // Called by espp::Display (LV_EVENT_RESOLUTION_CHANGED) whenever the LVGL + // display rotation changes; routes the rotation to the display driver + // (MADCTL) when the active panel can honor it in hardware. + void on_display_rotation(const DisplayRotation &rotation); + // Whether the active display controller applies the given LVGL rotation in + // panel hardware (via the display driver's set_rotation()/MADCTL), making + // buffer rotation (PPA / software) in flush() unnecessary. + bool panel_handles_rotation(lv_display_rotation_t rotation) const; static bool notify_lvgl_flush_ready(esp_lcd_panel_handle_t panel, esp_lcd_dpi_panel_event_data_t *edata, void *user_ctx); diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index 719efe1092..51bb568769 100755 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -388,7 +388,11 @@ bool M5StackTab5::initialize_display(size_t pixel_buffer_size) { Display::LvglConfig{.width = display_width_, .height = display_height_, .flush_callback = std::bind_front(&M5StackTab5::flush, this), - .rotation_callback = nullptr, // DisplayDriver::rotate, + // Forward LVGL rotation changes to the display driver so + // panels that can rotate in hardware (ST7121) do so via + // MADCTL instead of the PPA / software rotation in flush(). + .rotation_callback = + std::bind_front(&M5StackTab5::on_display_rotation, this), .rotation = rotation}, Display::OledConfig{ .set_brightness_callback = @@ -483,6 +487,42 @@ size_t M5StackTab5::rotated_display_height() const { } } +bool M5StackTab5::panel_handles_rotation(lv_display_rotation_t rotation) const { + // Only the ST7121 variant routes rotation to the panel (the ILI9881 and + // ST7123 keep the historical PPA/flush-time rotation path). The Tab5 panels + // are MIPI-DSI DPI (video mode) panels: the host streams a fixed 720x1280 + // raster, so the panel cannot swap axes for 90/270 (no MADCTL MV/GRAM + // addressing in video mode) and those still need the frame rotated into the + // framebuffer (PPA, or software fallback). The gate/source scan-direction + // flips (MADCTL GS/SS) do work, so 0 and 180 are applied by the panel + // itself through the espp display driver's set_rotation(). + if (display_controller_ != DisplayController::ST7121) { + return false; + } + return rotation == LV_DISPLAY_ROTATION_0 || rotation == LV_DISPLAY_ROTATION_180; +} + +void M5StackTab5::on_display_rotation(const DisplayRotation &rotation) { + if (!display_driver_ || display_controller_ != DisplayController::ST7121) { + // Other variants keep the PPA/flush-time rotation path; nothing to do. + return; + } + // Runs on the LVGL thread (RESOLUTION_CHANGED event from + // lv_display_set_rotation), i.e. strictly ordered with subsequent flush() + // calls, so the MADCTL state below is always consistent with the rotation + // decision flush() makes via panel_handles_rotation(). + if (rotation == DisplayRotation::LANDSCAPE || rotation == DisplayRotation::LANDSCAPE_INVERTED) { + // 0 / 180: the panel applies the rotation itself (scan-direction flip via + // MADCTL); flush() writes unrotated buffers at unrotated coordinates. + display_driver_->set_rotation(rotation); + } else { + // 90 / 270: a DPI video-mode panel cannot swap axes, so restore the + // natural scan direction and let the PPA rotation in flush() do the full + // transform. + display_driver_->set_rotation(DisplayRotation::LANDSCAPE); + } +} + void M5StackTab5::write_lcd_lines(int xs, int ys, int xe, int ye, const uint8_t *data, uint32_t user_data) { (void)user_data; @@ -539,7 +579,13 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m // own thread. flush() runs on the LVGL (GUI) thread, so reading it here is // safe; the camera task reads the cached atomic instead. camera_display_rotation_.store(static_cast(rotation), std::memory_order_relaxed); - if (rotation > LV_DISPLAY_ROTATION_0 && third_buffer != nullptr) { + // When the panel itself applies the rotation (ST7121, 180 degrees via the + // display driver's MADCTL scan flip - see on_display_rotation()), the + // logical frame is written to the framebuffer unrotated and at unrotated + // coordinates; the panel flips the whole frame at scan-out, which lands each + // partial area exactly where LVGL's rotated mapping expects it. + if (rotation > LV_DISPLAY_ROTATION_0 && !panel_handles_rotation(rotation) && + third_buffer != nullptr) { int32_t ww = lv_area_get_width(area); int32_t hh = lv_area_get_height(area); lv_color_format_t cf = lv_display_get_color_format(disp); From d16e9092c4ffa9508e1a2c1b267b971f0bb43904 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 28 Aug 2026 14:35:05 -0500 Subject: [PATCH 02/15] =?UTF-8?q?fix(m5stack-tab5):=20address=20rotation?= =?UTF-8?q?=20review=20=E2=80=94=20unify=20rotation=20enums,=20apply=20ini?= =?UTF-8?q?tial=20rotation,=20document=20MADCTL=20ordering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Pin the espp::DisplayRotation <-> lv_display_rotation_t value correspondence with a static_assert and route all conversions through explicit to_lv_rotation()/to_display_rotation() helpers; the panel-vs-PPA decision in on_display_rotation() now uses the exact same panel_handles_rotation() predicate flush() uses. - Explicitly (re)apply the current rotation at the end of initialize_lcd() so the panel MADCTL matches flush()'s expectation from the first frame even if initialize_display() was called before initialize_lcd() (the normal order is already covered: LVGL sends LV_EVENT_RESOLUTION_CHANGED synchronously from lv_display_set_rotation() in Display's constructor, before any flush can run). - Document why the runtime MADCTL write is safe w.r.t. the DPI pipeline: it travels on the DSI generic/DBI command channel, hardware-arbitrated against the video stream and independent of the framebuffer/DMA path; worst case is a single transient frame during a rotation change, which is stated and accepted. Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/src/video.cpp | 58 ++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 5 deletions(-) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index 51bb568769..331ca0c1cc 100755 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -21,6 +21,25 @@ using namespace std::chrono_literals; namespace espp { +// espp::DisplayRotation and lv_display_rotation_t enumerate the same four +// quarter-turns in the same order, and espp::Display itself converts between +// them by value (static_cast, see display.hpp). The rotation logic below leans +// on that correspondence — flush() keys off the LVGL enum while +// on_display_rotation() receives the espp one — so pin the mapping down +// explicitly and convert through these helpers only. +static_assert(static_cast(DisplayRotation::LANDSCAPE) == LV_DISPLAY_ROTATION_0 && + static_cast(DisplayRotation::PORTRAIT) == LV_DISPLAY_ROTATION_90 && + static_cast(DisplayRotation::LANDSCAPE_INVERTED) == + LV_DISPLAY_ROTATION_180 && + static_cast(DisplayRotation::PORTRAIT_INVERTED) == LV_DISPLAY_ROTATION_270, + "espp::DisplayRotation and lv_display_rotation_t must stay value-compatible"); +static constexpr lv_display_rotation_t to_lv_rotation(DisplayRotation rotation) { + return static_cast(rotation); +} +static constexpr DisplayRotation to_display_rotation(lv_display_rotation_t rotation) { + return static_cast(rotation); +} + M5StackTab5::DisplayController M5StackTab5::detect_display_controller() { auto &i2c = internal_i2c(); @@ -366,6 +385,22 @@ bool M5StackTab5::initialize_lcd() { return false; } + // Program the panel's initial scan direction so the rotation decision + // flush() makes via panel_handles_rotation() is valid from the very first + // frame. In the documented init order (initialize_lcd() before + // initialize_display()) this is already guaranteed: espp::Display's + // constructor calls lv_display_set_rotation() with the initial rotation, + // which synchronously fires the LV_EVENT_RESOLUTION_CHANGED handler (in + // LVGL, update_resolution() sends the event as a direct call) and thus + // on_display_rotation() before any flush can run. But if + // initialize_display() ran first, that initial callback was dropped because + // display_driver_ did not exist yet — so (re)apply the current rotation here + // now that the driver is up. Harmless when redundant: it rewrites the MADCTL + // value the driver init just programmed. + on_display_rotation( + display_ ? to_display_rotation(lv_display_get_rotation(display_->get_lvgl_display())) + : rotation); + logger_.info("M5Stack Tab5 LCD initialization completed successfully"); return true; } @@ -507,11 +542,24 @@ void M5StackTab5::on_display_rotation(const DisplayRotation &rotation) { // Other variants keep the PPA/flush-time rotation path; nothing to do. return; } - // Runs on the LVGL thread (RESOLUTION_CHANGED event from - // lv_display_set_rotation), i.e. strictly ordered with subsequent flush() - // calls, so the MADCTL state below is always consistent with the rotation - // decision flush() makes via panel_handles_rotation(). - if (rotation == DisplayRotation::LANDSCAPE || rotation == DisplayRotation::LANDSCAPE_INVERTED) { + // Ordering: this runs on the LVGL thread — LV_EVENT_RESOLUTION_CHANGED is + // sent synchronously from inside lv_display_set_rotation() — so it strictly + // precedes the invalidation-driven flush() calls for the new orientation, + // and the MADCTL state below is always consistent with the decision flush() + // makes via panel_handles_rotation() (the same predicate, keyed on the same + // rotation value via to_lv_rotation()). + // + // Synchronization with the display pipeline: the MADCTL write goes out on + // the DSI generic/DBI command channel (esp_lcd_panel_io_tx_param -> + // mipi_dsi_hal_host_gen_write_dcs_command), which the DSI host peripheral + // arbitrates against the DPI video stream in hardware. It never touches the + // DPI framebuffer or its DMA, so it cannot corrupt a previous flush()'s + // in-flight draw_bitmap copy. The panel may latch the new scan direction + // mid scan-out, which can show as (at most) a single transient frame; that + // is accepted here, since a rotation change is a full-screen visual + // discontinuity anyway and LVGL follows it immediately with a full + // invalidate/redraw of the new orientation. + if (panel_handles_rotation(to_lv_rotation(rotation))) { // 0 / 180: the panel applies the rotation itself (scan-direction flip via // MADCTL); flush() writes unrotated buffers at unrotated coordinates. display_driver_->set_rotation(rotation); From 33eb080d0456f6c9553a425a0236cb09904b9276 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 28 Aug 2026 15:59:16 -0500 Subject: [PATCH 03/15] fix(m5stack-tab5): gate ST7121 panel-side rotation behind an experimental Kconfig (default off) Hardware testing showed the MADCTL GS/SS 180-degree scan flip renders corrupted on (at least some) ST7121 units. Root-cause analysis: the 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, producing interleaved garbage. (The bit positions themselves match Espressif's esp_lcd_st7121 mirror implementation, whose generic template was likely never validated on this glass either; ESPHome's mipi_dsi driver documents the same 0/180-only constraint for DSI panels generally, so the design was sound - this panel's init tables are what make runtime flips unsafe.) Default behavior is now EXACTLY the pre-branch known-good path: PPA/software rotation for all orientations and ZERO runtime MADCTL writes (the rotation callback is a full no-op), which also makes A/B comparison for unrelated display artifacts clean. The panel-side path remains available for experimentation via CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION, and the rest of the branch (init-order hardening, single-predicate rotation decision, enum unification, documented DSI command-channel safety) stands. Both configurations build clean. Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/Kconfig | 14 ++++++++++++++ components/m5stack-tab5/src/video.cpp | 24 ++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/components/m5stack-tab5/Kconfig b/components/m5stack-tab5/Kconfig index b76c0eade4..fe8beb41a4 100644 --- a/components/m5stack-tab5/Kconfig +++ b/components/m5stack-tab5/Kconfig @@ -27,4 +27,18 @@ menu "M5Stack Tab5 Configuration" help Size of the stack used for the audio processing task. + config M5STACK_TAB5_ST7121_HW_ROTATION + bool "ST7121: apply 0/180 rotation in the panel (EXPERIMENTAL)" + default n + help + Route 0/180-degree display rotation to the ST7121 panel itself (MADCTL + GS/SS scan-direction flip) instead of the PPA/software rotation in the + flush path. EXPERIMENTAL: verified NOT working on at least some ST7121 + Tab5 units - the 180-degree orientation renders corrupted, most likely + because the TDDI gate/source mux tables programmed at init (command + 0xAC block) are matched to the normal scan direction and a MADCTL GS + flip alone reorders gate scanning without swapping them. Leave disabled + (the default) to keep the known-good PPA rotation for all orientations; + enable only to experiment on your unit. + endmenu diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index 331ca0c1cc..ea13651969 100755 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -523,21 +523,41 @@ size_t M5StackTab5::rotated_display_height() const { } bool M5StackTab5::panel_handles_rotation(lv_display_rotation_t rotation) const { +#if !CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION + // Panel-side rotation is DISABLED by default: hardware testing showed the + // 180-degree MADCTL GS/SS scan flip renders corrupted on (at least some) + // ST7121 units - the TDDI gate/source mux tables programmed at init (the + // 0xAC block) are matched to the normal scan direction, and a MADCTL GS + // flip alone reorders gate scanning without swapping them. All orientations + // therefore use the PPA/flush-time rotation (the pre-existing, known-good + // path). Enable CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION to experiment. + (void)rotation; + return false; +#else // Only the ST7121 variant routes rotation to the panel (the ILI9881 and // ST7123 keep the historical PPA/flush-time rotation path). The Tab5 panels // are MIPI-DSI DPI (video mode) panels: the host streams a fixed 720x1280 // raster, so the panel cannot swap axes for 90/270 (no MADCTL MV/GRAM // addressing in video mode) and those still need the frame rotated into the // framebuffer (PPA, or software fallback). The gate/source scan-direction - // flips (MADCTL GS/SS) do work, so 0 and 180 are applied by the panel - // itself through the espp display driver's set_rotation(). + // flips (MADCTL GS/SS) are applied by the panel itself through the espp + // display driver's set_rotation() for 0 and 180 - EXPERIMENTAL, see the + // Kconfig help. if (display_controller_ != DisplayController::ST7121) { return false; } return rotation == LV_DISPLAY_ROTATION_0 || rotation == LV_DISPLAY_ROTATION_180; +#endif } void M5StackTab5::on_display_rotation(const DisplayRotation &rotation) { +#if !CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION + // Panel-side rotation disabled (see panel_handles_rotation()): make this a + // complete no-op so NO runtime MADCTL write ever reaches the panel - the + // scan state stays exactly as the init sequence programmed it. + (void)rotation; + return; +#endif if (!display_driver_ || display_controller_ != DisplayController::ST7121) { // Other variants keep the PPA/flush-time rotation path; nothing to do. return; From 513f565d2d0fa9c6de8605b66c7b6e3c2b0e63a8 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 28 Aug 2026 16:32:25 -0500 Subject: [PATCH 04/15] fix(m5stack-tab5): reduce PSRAM contention that underruns the DSI scan-out (display streaking) The Tab5's DPI (video mode) panel continuously scans its PSRAM framebuffer at ~140 MB/s via DW-GDMA; when concurrent PSRAM/AXI traffic starves that read stream the DSI bridge FIFO underruns and the panel shows streaks along the physical scan-line axis. The flush path was generating far more PSRAM traffic than needed: - On ESP-IDF 6.0 the esp_lcd_dpi_panel_config_t::flags.use_dma2d flag no longer exists (replaced by esp_lcd_dpi_panel_enable_dma2d()), so every esp_lcd_panel_draw_bitmap silently regressed to a CPU memcpy through the cache (~1.8 MB read + ~1.8 MB write + ~1.8 MB writeback per full frame). Call esp_lcd_dpi_panel_enable_dma2d() so copies run on the 2D-DMA engine again (matches M5Stack's UserDemo and esp-bsp's m5stack_tab5). - The PPA rotation path rotated into a PSRAM scratch buffer and then draw_bitmap-copied that into the framebuffer. The PPA output supports placing a block inside a larger picture, so rotate directly into the DPI framebuffer at the rotated offset instead, halving that path's PSRAM traffic. The CPU software-rotate fallback keeps the scratch buffer. - Register both PPA clients (display rotation and camera preview) with data_burst_length = 64 instead of the default 128: full-length PPA bursts are known to starve the DSI scan-out DMA even with 200 MHz hex PSRAM (lvgl/lvgl#9590; LVGL exposes the same knob as LV_PPA_BURST_LENGTH). Co-Authored-By: Claude Fable 5 --- .../m5stack-tab5/include/m5stack-tab5.hpp | 9 ++ components/m5stack-tab5/src/camera.cpp | 6 + components/m5stack-tab5/src/video.cpp | 126 ++++++++++++++---- 3 files changed, 114 insertions(+), 27 deletions(-) diff --git a/components/m5stack-tab5/include/m5stack-tab5.hpp b/components/m5stack-tab5/include/m5stack-tab5.hpp index 19ae1cab30..cd23962698 100644 --- a/components/m5stack-tab5/include/m5stack-tab5.hpp +++ b/components/m5stack-tab5/include/m5stack-tab5.hpp @@ -944,6 +944,15 @@ class M5StackTab5 : public BaseComponent { esp_lcd_panel_handle_t panel{nullptr}; // color handle } lcd_handles_{}; + // The DPI panel's (PSRAM) framebuffer, queried from esp_lcd once the panel + // is created. flush() uses it to rotate LVGL draw buffers directly into the + // scanned-out framebuffer with the PPA, skipping the intermediate scratch + // buffer + draw_bitmap copy (which doubles the PSRAM traffic and can starve + // the DSI scan-out DMA into FIFO underruns / on-screen streaking). Null when + // unavailable, in which case flush() falls back to the scratch-buffer path. + void *dpi_framebuffer_{nullptr}; + size_t dpi_framebuffer_bytes_{0}; + // Display controller detection DisplayController display_controller_{DisplayController::UNKNOWN}; diff --git a/components/m5stack-tab5/src/camera.cpp b/components/m5stack-tab5/src/camera.cpp index 276cae81e8..7fe01fb50f 100644 --- a/components/m5stack-tab5/src/camera.cpp +++ b/components/m5stack-tab5/src/camera.cpp @@ -245,6 +245,12 @@ bool M5StackTab5::initialize_camera(const camera_frame_callback_t &callback, // hardware pass. The callback receives this preview buffer. ppa_client_config_t ppa_cfg = {}; ppa_cfg.oper_type = PPA_OPERATION_SRM; + // Throttle the PPA's AXI bursts (default 128 bytes): full-length PPA bursts + // against PSRAM are known to starve the DSI panel's continuous framebuffer + // scan-out DMA and underrun its FIFO, streaking the display (see the display + // PPA client in video.cpp and lvgl/lvgl#9590). The camera PPA runs every + // frame concurrently with display flushes, so keep its bursts short too. + ppa_cfg.data_burst_length = PPA_DATA_BURST_LENGTH_64; if (ppa_register_client(&ppa_cfg, &camera_ppa_client_) != ESP_OK) { logger_.error("Could not register the camera PPA client"); stop_camera(); diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index ea13651969..5dc09c4e42 100755 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -385,6 +385,50 @@ bool M5StackTab5::initialize_lcd() { return false; } +#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(6, 0, 0) + // Use the 2D-DMA engine for esp_lcd_panel_draw_bitmap copies into the DPI + // framebuffer. On ESP-IDF < 6.0 this was requested with the + // esp_lcd_dpi_panel_config_t::flags.use_dma2d flag (set above); IDF 6.0 + // removed the flag in favor of this explicit call. Without it every + // draw_bitmap is a CPU memcpy through the cache — for a full 720x1280 RGB565 + // frame that is ~1.8 MB read + ~1.8 MB written + a ~1.8 MB cache writeback + // per flush, all PSRAM traffic that competes with the DPI panel's continuous + // ~140 MB/s framebuffer scan-out DMA. Starving that scan-out DMA underruns + // the DSI bridge FIFO, which shows up as streaks/tears along the panel's + // scan-line axis (the driver logs "underrun happens" when it detects this). + // The DMA2D copy runs without the CPU touching the data and completes via + // the same on_color_trans_done callback registered above. + ret = esp_lcd_dpi_panel_enable_dma2d(lcd_handles_.panel); + if (ret != ESP_OK) { + // Not fatal: draw_bitmap falls back to the (slower) CPU copy. + logger_.warn("Could not enable DMA2D for DPI draw_bitmap ({}); using CPU copies", + esp_err_to_name(ret)); + } +#endif + + // Cache the DPI framebuffer address so flush() can rotate directly into it + // with the PPA (see flush()). The panel scans this buffer out continuously; + // the espp draw/flush path itself never writes it with the CPU. + { + void *fb = nullptr; + if (esp_lcd_dpi_panel_get_frame_buffer(lcd_handles_.panel, 1, &fb) == ESP_OK && fb != nullptr) { + const size_t fb_bytes = static_cast(display_width_) * display_height_ * sizeof(Pixel); + // The PPA requires its output buffer pointer and size to be aligned to + // the (128-byte) cache line; the esp_lcd driver allocates the DPI + // framebuffer DMA-aligned, and 720*1280*2 is a multiple of 128, but + // verify rather than assume — if it does not hold, flush() simply keeps + // the scratch-buffer path. + if ((reinterpret_cast(fb) % 128) == 0 && (fb_bytes % 128) == 0) { + dpi_framebuffer_ = fb; + dpi_framebuffer_bytes_ = fb_bytes; + } else { + logger_.warn("DPI framebuffer not cache-line aligned; PPA will rotate via scratch buffer"); + } + } else { + logger_.warn("Could not query the DPI framebuffer; PPA will rotate via scratch buffer"); + } + } + // Program the panel's initial scan direction so the rotation decision // flush() makes via panel_handles_rotation() is valid from the very first // frame. In the documented init order (initialize_lcd() before @@ -452,6 +496,15 @@ bool M5StackTab5::initialize_display(size_t pixel_buffer_size) { if (g_ppa_client == nullptr) { ppa_client_config_t ppa_cfg = {}; ppa_cfg.oper_type = PPA_OPERATION_SRM; + // Throttle the PPA's AXI bursts (default PPA_DATA_BURST_LENGTH_128). The + // DPI panel continuously scans its PSRAM framebuffer at ~140 MB/s; on the + // ESP32-P4 a PPA client running full-length bursts against the same PSRAM + // is known to starve that scan-out DMA and underrun the DSI bridge FIFO, + // which appears as streaks along the panel's scan-line axis even with + // 200 MHz hex PSRAM (see lvgl/lvgl#9590 - shorter PPA bursts fix the + // artifacts at a small cost in PPA throughput; LVGL's own PPA integration + // exposes the same knob as LV_PPA_BURST_LENGTH). + ppa_cfg.data_burst_length = PPA_DATA_BURST_LENGTH_64; esp_err_t perr = ppa_register_client(&ppa_cfg, &g_ppa_client); if (perr != ESP_OK) { logger_.warn("Could not register PPA client ({}); rotation will fall back to software", @@ -657,25 +710,34 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m int32_t ww = lv_area_get_width(area); int32_t hh = lv_area_get_height(area); lv_color_format_t cf = lv_display_get_color_format(disp); - bool rotated = false; - if (g_ppa_client != nullptr) { - // Hardware rotation via the PPA. The LVGL rotation maps directly onto the - // PPA rotation angle (LVGL 90 -> PPA 90, 180 -> 180, 270 -> 270); this is - // the mapping verified on hardware. For 90/270 the output picture - // width/height are swapped. If a different panel comes out turned the - // wrong way, swap the 90 and 270 cases here. + // Map the logical (LVGL) area to physical panel coordinates up front: the + // direct-to-framebuffer PPA path needs the rotated destination offsets + // before the PPA runs, and the fallback paths need them for draw_bitmap. + lv_display_rotate_area(disp, const_cast(area)); + offsetx1 = area->x1; + offsetx2 = area->x2; + offsety1 = area->y1; + offsety2 = area->y2; + if (g_ppa_client != nullptr && dpi_framebuffer_ != nullptr) { + // Hardware rotation via the PPA, writing the rotated block DIRECTLY into + // the DPI panel's framebuffer at the rotated offset (the PPA output + // supports placing a block inside a larger picture). This halves the + // PSRAM traffic of the previous scratch-buffer approach (PPA write + + // draw_bitmap read + write), which matters because the DSI scan-out DMA + // is reading the same PSRAM continuously and underruns - visible as + // streaks - when the flush path hogs the bandwidth. + // + // The LVGL rotation maps directly onto the PPA rotation angle (LVGL 90 + // -> PPA 90, 180 -> 180, 270 -> 270); this is the mapping verified on + // hardware. If a different panel comes out turned the wrong way, swap + // the 90 and 270 cases here. ppa_srm_rotation_angle_t angle = PPA_SRM_ROTATION_ANGLE_0; - uint32_t out_w = ww, out_h = hh; if (rotation == LV_DISPLAY_ROTATION_90) { angle = PPA_SRM_ROTATION_ANGLE_90; - out_w = hh; - out_h = ww; } else if (rotation == LV_DISPLAY_ROTATION_180) { angle = PPA_SRM_ROTATION_ANGLE_180; } else if (rotation == LV_DISPLAY_ROTATION_270) { angle = PPA_SRM_ROTATION_ANGLE_270; - out_w = hh; - out_h = ww; } ppa_srm_oper_config_t srm = {}; srm.in.buffer = px_map; @@ -684,22 +746,37 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m srm.in.block_w = ww; srm.in.block_h = hh; srm.in.srm_cm = PPA_SRM_COLOR_MODE_RGB565; - srm.out.buffer = third_buffer; - srm.out.buffer_size = third_buffer_bytes; - srm.out.pic_w = out_w; - srm.out.pic_h = out_h; + srm.out.buffer = dpi_framebuffer_; + srm.out.buffer_size = dpi_framebuffer_bytes_; + srm.out.pic_w = display_width_; + srm.out.pic_h = display_height_; + srm.out.block_offset_x = offsetx1; + srm.out.block_offset_y = offsety1; srm.out.srm_cm = PPA_SRM_COLOR_MODE_RGB565; srm.rotation_angle = angle; srm.scale_x = 1.0f; srm.scale_y = 1.0f; srm.mode = PPA_TRANS_MODE_BLOCKING; - // On failure third_buffer holds stale/partial data; leave rotated=false so - // we fall through to the software rotation below rather than flushing it. - rotated = (ppa_do_scale_rotate_mirror(g_ppa_client, &srm) == ESP_OK); + if (ppa_do_scale_rotate_mirror(g_ppa_client, &srm) == ESP_OK) { + // The rotated pixels are already in the scanned-out framebuffer and + // the PPA driver performed the cache maintenance (write-back of the + // source window, invalidate of the destination window). The espp + // flush path never dirties the framebuffer with the CPU (draw_bitmap + // copies run on the DMA2D engine, and its CPU fallback writes back + // the cache before returning), so there is nothing left to sync and + // no draw_bitmap call is needed: signal LVGL directly. + lv_display_flush_ready(disp); + return; + } + // On failure fall through to the scratch-buffer software rotation: the + // framebuffer may hold a partial block, but the fallback redraws the + // full area at the same destination. } - if (!rotated) { - // Software fallback: the PPA client failed to register or the PPA - // operation failed. Rotates into third_buffer, fully overwriting it. + { + // Fallback: the PPA client failed to register, the framebuffer could + // not be queried, or the PPA operation failed. Rotate on the CPU into + // the scratch buffer, fully overwriting it, and hand it to draw_bitmap + // below. uint32_t w_stride = lv_draw_buf_width_to_stride(ww, cf); uint32_t h_stride = lv_draw_buf_width_to_stride(hh, cf); if (rotation == LV_DISPLAY_ROTATION_180) { @@ -714,11 +791,6 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m } } px_map = reinterpret_cast(third_buffer); - lv_display_rotate_area(disp, const_cast(area)); - offsetx1 = area->x1; - offsetx2 = area->x2; - offsety1 = area->y1; - offsety2 = area->y2; } // pass the draw buffer to the DPI panel driver From 0f4c38e68b5f93e6363d4e6e161b38088669b47c Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 28 Aug 2026 16:40:14 -0500 Subject: [PATCH 05/15] doc(m5stack-tab5): document thread-safety of the init-path on_display_rotation() call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit on_display_rotation()'s ordering comment claimed it runs on the LVGL thread, but initialize_lcd() also calls it directly on the init thread. Scope the LVGL-thread claim to the rotation-callback invocation and spell out at the call site why the direct init-path call is safe in either init order: in the documented order display_ is still null (no LVGL display, flush callback, or handler task exists, and the Display constructor re-fires the callback synchronously before any flush); in the reversed order the MADCTL write rides the DSI command channel, which never touches the DPI framebuffer or its DMA, so at worst one transient frame — and with CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION off (default) the call is a no-op entirely. Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/src/video.cpp | 44 +++++++++++++++++++-------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index 5dc09c4e42..bef4f47c44 100755 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -431,16 +431,30 @@ bool M5StackTab5::initialize_lcd() { // Program the panel's initial scan direction so the rotation decision // flush() makes via panel_handles_rotation() is valid from the very first - // frame. In the documented init order (initialize_lcd() before - // initialize_display()) this is already guaranteed: espp::Display's - // constructor calls lv_display_set_rotation() with the initial rotation, - // which synchronously fires the LV_EVENT_RESOLUTION_CHANGED handler (in - // LVGL, update_resolution() sends the event as a direct call) and thus - // on_display_rotation() before any flush can run. But if - // initialize_display() ran first, that initial callback was dropped because - // display_driver_ did not exist yet — so (re)apply the current rotation here - // now that the driver is up. Harmless when redundant: it rewrites the MADCTL - // value the driver init just programmed. + // frame. NOTE: unlike the normal invocation of on_display_rotation() — the + // LVGL rotation event, delivered synchronously on the LVGL thread — this is + // a direct call on the caller's (init) thread. It is safe in either init + // order: + // - Documented order (initialize_lcd() before initialize_display()): + // display_ is still null here, so no LVGL display or flush callback + // exists yet and espp::Display does not run an LVGL handler task of its + // own — nothing can race this call. It is also redundant-but-harmless in + // this order: the espp::Display constructor will call + // lv_display_set_rotation() with the initial rotation, which synchronously + // fires the LV_EVENT_RESOLUTION_CHANGED handler (in LVGL, + // update_resolution() sends the event as a direct call) and thus + // on_display_rotation() again before any flush can run. + // - Reversed order (initialize_display() first): the constructor-time + // callback above was dropped because display_driver_ did not exist yet, + // so (re)apply the current LVGL rotation now that the driver is up. Even + // if the application is already pumping LVGL on another thread, this + // cannot corrupt an in-flight flush: the MADCTL write travels on the DSI + // command channel, which never touches the DPI framebuffer or its DMA and + // is arbitrated against the video stream in hardware (see + // on_display_rotation()). The worst case is one transient frame scanned + // out with the new direction — the same as any runtime rotation change. + // (With CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION disabled — the default — + // on_display_rotation() is a no-op and this call does nothing at all.) on_display_rotation( display_ ? to_display_rotation(lv_display_get_rotation(display_->get_lvgl_display())) : rotation); @@ -615,12 +629,16 @@ void M5StackTab5::on_display_rotation(const DisplayRotation &rotation) { // Other variants keep the PPA/flush-time rotation path; nothing to do. return; } - // Ordering: this runs on the LVGL thread — LV_EVENT_RESOLUTION_CHANGED is - // sent synchronously from inside lv_display_set_rotation() — so it strictly + // Ordering: in its normal invocation — the espp::Display rotation callback — + // this runs on the LVGL thread: LV_EVENT_RESOLUTION_CHANGED is sent + // synchronously from inside lv_display_set_rotation(), so it strictly // precedes the invalidation-driven flush() calls for the new orientation, // and the MADCTL state below is always consistent with the decision flush() // makes via panel_handles_rotation() (the same predicate, keyed on the same - // rotation value via to_lv_rotation()). + // rotation value via to_lv_rotation()). It is additionally called once + // directly from initialize_lcd() (an init-thread call, not the LVGL thread) + // to program the initial scan direction; see the safety analysis at that + // call site. // // Synchronization with the display pipeline: the MADCTL write goes out on // the DSI generic/DBI command channel (esp_lcd_panel_io_tx_param -> From 94ab211da96dcb3f031f0f76700edb07483ab60a Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 28 Aug 2026 20:08:08 -0500 Subject: [PATCH 06/15] fix(m5stack-tab5): synchronize LCD-state publication against a concurrently flushing LVGL thread initialize_lcd() writes lcd_handles_.panel early (before panel->init(), DMA2D enable, and caching the dpi_framebuffer_/dpi_framebuffer_bytes_ pair), while flush() gated only on a plain null-check of that pointer. With the reversed init order (initialize_display() first, the app already pumping LVGL on another thread), flush() could race those plain writes: undefined behavior, and observably a not-yet-initialized panel or a torn framebuffer pointer/size pair. Publish the LCD state through a single std::atomic lcd_initialized_ gate: initialize_lcd() clears it on entry, writes every field (lcd_handles_, dpi_framebuffer_ + bytes, display_driver_, display_controller_), and store-releases it true as its final step; flush(), write_lcd_lines() and on_display_rotation() load-acquire it and no-op until it is true, so every plain write happens-before any read that observes the gate open. The init-path rotation call runs after the release store, and a rotation event that arrives while the gate is closed is harmless: initialize_lcd() re-applies the current LVGL rotation once the driver is up. Also restructure on_display_rotation()'s Kconfig-disabled early-return into #if/#else/#endif, fixing the cppcheck unreachableCode finding from static analysis. Co-Authored-By: Claude Fable 5 --- .../m5stack-tab5/include/m5stack-tab5.hpp | 13 ++++ components/m5stack-tab5/src/video.cpp | 65 ++++++++++++++++--- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/components/m5stack-tab5/include/m5stack-tab5.hpp b/components/m5stack-tab5/include/m5stack-tab5.hpp index cd23962698..534b3cd38f 100644 --- a/components/m5stack-tab5/include/m5stack-tab5.hpp +++ b/components/m5stack-tab5/include/m5stack-tab5.hpp @@ -944,6 +944,19 @@ class M5StackTab5 : public BaseComponent { esp_lcd_panel_handle_t panel{nullptr}; // color handle } lcd_handles_{}; + // Publication gate for the LCD state that flush() / write_lcd_lines() / + // on_display_rotation() read from other threads (lcd_handles_, + // dpi_framebuffer_ + dpi_framebuffer_bytes_, display_driver_, + // display_controller_). Those fields are plain (non-atomic) and are written + // by initialize_lcd() on the init thread; if the LVGL display already exists + // (initialize_display() called first) the LVGL thread can be flushing + // concurrently, so the readers must not touch them until they are all + // written. initialize_lcd() clears this flag on entry, writes every field, + // and store-releases it true as its final publication step; the readers + // load-acquire it and bail out while it is false. The release/acquire pair + // makes all of the writes happen-before any read that observes true. + std::atomic lcd_initialized_{false}; + // The DPI panel's (PSRAM) framebuffer, queried from esp_lcd once the panel // is created. flush() uses it to rotate LVGL draw buffers directly into the // scanned-out framebuffer with the PPA, skipping the intermediate scratch diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index bef4f47c44..e4c192ec29 100755 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -111,6 +111,17 @@ M5StackTab5::DisplayController M5StackTab5::detect_display_controller() { bool M5StackTab5::initialize_lcd() { logger_.info("Initializing M5Stack Tab5 LCD (MIPI-DSI, {}x{})", display_width_, display_height_); + // Close the publication gate while the LCD state below (lcd_handles_, + // dpi_framebuffer_ + dpi_framebuffer_bytes_, display_driver_, + // display_controller_) is being (re)built. If the LVGL display already + // exists (initialize_display() called first) its thread may already be + // pumping flush(); flush()/write_lcd_lines()/on_display_rotation() all + // load-acquire this flag and no-op while it is false, so none of them can + // observe a partially initialized panel or a torn framebuffer-pointer/size + // pair. It is store-released true as the final step of this function, after + // every field has been written. + lcd_initialized_.store(false, std::memory_order_release); + if (!ioexp_0x43_) { if (!initialize_io_expanders()) { logger_.error("Failed to init IO expanders for LCD reset"); @@ -429,12 +440,26 @@ bool M5StackTab5::initialize_lcd() { } } + // Publish the fully initialized LCD state. Every field the cross-thread + // readers touch (lcd_handles_, dpi_framebuffer_ + dpi_framebuffer_bytes_, + // display_driver_, display_controller_) has been written above, so the + // release store here makes all of those writes visible to any + // flush()/write_lcd_lines()/on_display_rotation() call that load-acquires + // the flag as true. Until this store, those readers no-op (flush() just + // signals lv_display_flush_ready()), which is what closes the + // reversed-init-order race: with initialize_display() called first and the + // application already pumping LVGL on another thread, a concurrent flush() + // can no longer pass a plain lcd_handles_.panel null-check mid-build and + // read a not-yet-init'd panel or a torn framebuffer-pointer/size pair. + lcd_initialized_.store(true, std::memory_order_release); + // Program the panel's initial scan direction so the rotation decision // flush() makes via panel_handles_rotation() is valid from the very first // frame. NOTE: unlike the normal invocation of on_display_rotation() — the // LVGL rotation event, delivered synchronously on the LVGL thread — this is - // a direct call on the caller's (init) thread. It is safe in either init - // order: + // a direct call on the caller's (init) thread. It runs after the release + // store above (same thread, so its own acquire load sees true) and is safe + // in either init order: // - Documented order (initialize_lcd() before initialize_display()): // display_ is still null here, so no LVGL display or flush callback // exists yet and espp::Display does not run an LVGL handler task of its @@ -444,10 +469,10 @@ bool M5StackTab5::initialize_lcd() { // fires the LV_EVENT_RESOLUTION_CHANGED handler (in LVGL, // update_resolution() sends the event as a direct call) and thus // on_display_rotation() again before any flush can run. - // - Reversed order (initialize_display() first): the constructor-time - // callback above was dropped because display_driver_ did not exist yet, - // so (re)apply the current LVGL rotation now that the driver is up. Even - // if the application is already pumping LVGL on another thread, this + // - Reversed order (initialize_display() first): any earlier rotation + // callback no-op'd on the closed gate (and display_driver_ did not exist + // yet), so (re)apply the current LVGL rotation now that the driver is up. + // Even if the application is already pumping LVGL on another thread, this // cannot corrupt an in-flight flush: the MADCTL write travels on the DSI // command channel, which never touches the DPI framebuffer or its DMA and // is arbitrated against the video stream in hardware (see @@ -623,8 +648,16 @@ void M5StackTab5::on_display_rotation(const DisplayRotation &rotation) { // complete no-op so NO runtime MADCTL write ever reaches the panel - the // scan state stays exactly as the init sequence programmed it. (void)rotation; - return; -#endif +#else + // Acquire-load the publication gate (paired with the release store in + // initialize_lcd()) before reading display_driver_ / display_controller_: + // with initialize_display() called first, LVGL rotation events can arrive + // while initialize_lcd() is still writing those fields. Bailing out here is + // harmless — initialize_lcd() re-applies the current LVGL rotation via its + // direct call once the gate is open. + if (!lcd_initialized_.load(std::memory_order_acquire)) { + return; + } if (!display_driver_ || display_controller_ != DisplayController::ST7121) { // Other variants keep the PPA/flush-time rotation path; nothing to do. return; @@ -660,12 +693,17 @@ void M5StackTab5::on_display_rotation(const DisplayRotation &rotation) { // transform. display_driver_->set_rotation(DisplayRotation::LANDSCAPE); } +#endif } void M5StackTab5::write_lcd_lines(int xs, int ys, int xe, int ye, const uint8_t *data, uint32_t user_data) { (void)user_data; - if (lcd_handles_.panel == nullptr || data == nullptr) { + // Acquire-load the publication gate (paired with the release store in + // initialize_lcd()) instead of a plain lcd_handles_.panel null-check: this + // runs on the caller's thread (e.g. the camera task) and must not race the + // init thread's writes or draw into a panel that is not fully initialized. + if (!lcd_initialized_.load(std::memory_order_acquire) || data == nullptr) { return; } if (xs < 0 || ys < 0 || xe < xs || ye < ys) { @@ -703,7 +741,14 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m // interrupt is handled separately in notify_lvgl_flush_ready(). Blocking work // (the PPA rotation and esp_lcd_panel_draw_bitmap) is therefore safe here. - if (lcd_handles_.panel == nullptr) { + // Acquire-load the publication gate before touching any LCD state. This + // pairs with the release store at the end of initialize_lcd(): observing + // true guarantees lcd_handles_ (incl. a fully init'd, DMA2D-enabled panel), + // the dpi_framebuffer_/dpi_framebuffer_bytes_ pair, display_driver_ and + // display_controller_ are all completely written and will not be written + // again. Until then (LCD not yet initialized, or mid re-init) just tell + // LVGL the flush is done and drop the frame. + if (!lcd_initialized_.load(std::memory_order_acquire)) { lv_display_flush_ready(disp); return; } From 18d7e74742299e3b59c3c545508744c917adb436 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Fri, 28 Aug 2026 22:57:40 -0500 Subject: [PATCH 07/15] fix(m5stack-tab5): address PPA flush-path review feedback - flush(): rotate a local copy of the LVGL area instead of mutating the const area pointer via const_cast (lv_display_rotate_area on a copy) - Tie the direct-to-framebuffer PPA rotation to the single-framebuffer panel configuration: name the num_fbs value (kNumDpiFramebuffers) and static_assert it is 1 where the framebuffer pointer is cached, since with multiple DPI framebuffers the cached pointer could target a non-scanned buffer - Replace the hard-coded 128-byte cache-line alignment with the value the PPA driver itself validates against, queried via esp_cache_get_alignment(MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA) (used for both the framebuffer alignment check and the scratch-buffer allocation); add esp_mm to the component requirements for the header Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/CMakeLists.txt | 2 +- components/m5stack-tab5/src/video.cpp | 88 ++++++++++++++++++++------ 2 files changed, 69 insertions(+), 21 deletions(-) mode change 100755 => 100644 components/m5stack-tab5/src/video.cpp diff --git a/components/m5stack-tab5/CMakeLists.txt b/components/m5stack-tab5/CMakeLists.txt index 7d91d55ad7..1ff161b1aa 100755 --- a/components/m5stack-tab5/CMakeLists.txt +++ b/components/m5stack-tab5/CMakeLists.txt @@ -1,6 +1,6 @@ idf_component_register( INCLUDE_DIRS "include" SRC_DIRS "src" - REQUIRES driver esp_driver_i2s esp_driver_ppa esp_driver_sdmmc esp_driver_spi esp_lcd esp_video esp_cam_sensor fatfs base_component bmi270 codec display display_drivers gt911 i2c ina226 input_drivers interrupt pi4ioe5v rx8130ce st7123touch task touch + REQUIRES driver esp_driver_i2s esp_driver_ppa esp_driver_sdmmc esp_driver_spi esp_lcd esp_mm esp_video esp_cam_sensor fatfs base_component bmi270 codec display display_drivers gt911 i2c ina226 input_drivers interrupt pi4ioe5v rx8130ce st7123touch task touch REQUIRED_IDF_TARGETS "esp32p4" ) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp old mode 100755 new mode 100644 index e4c192ec29..8ac8b91376 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -16,6 +16,7 @@ #include #include #include +#include using namespace std::chrono_literals; @@ -40,6 +41,36 @@ static constexpr DisplayRotation to_display_rotation(lv_display_rotation_t rotat return static_cast(rotation); } +// Number of framebuffers the DPI panel is created with (esp_lcd_dpi_panel_config_t::num_fbs, +// used for every controller variant below). The direct-to-framebuffer PPA +// rotation in flush() caches THE single framebuffer pointer at init and writes +// into it unconditionally; with more than one framebuffer the driver flips +// which buffer is scanned out, and rotating into a fixed one would +// intermittently update a non-visible buffer. flush() therefore only enables +// that optimization when this is 1 (see initialize_lcd()); if you raise this, +// the code falls back to the scratch-buffer path (or teach it to track the +// active framebuffer). +static constexpr uint8_t kNumDpiFramebuffers = 1; + +// Alignment the PPA requires for its output buffer in external (PSRAM) memory: +// both the buffer pointer and the buffer size must be multiples of the data +// cache line size. Query it from the cache driver rather than hard-coding the +// ESP32-P4's 128-byte L2 line — this is the exact value the PPA driver itself +// validates against (ppa_check_buffer_alignment() checks +// s_platform.buf_alignment_size, which it obtains via +// esp_cache_get_alignment(MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA, ...)). +static size_t ppa_out_buffer_alignment() { + size_t alignment = 0; + if (esp_cache_get_alignment(MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA, &alignment) != ESP_OK || + alignment == 0) { + // Should not happen (the call only fails on invalid arguments); fall back + // to the largest cache line on any current target so alignment is never + // under-estimated. + alignment = 128; + } + return alignment; +} + M5StackTab5::DisplayController M5StackTab5::detect_display_controller() { auto &i2c = internal_i2c(); @@ -242,7 +273,7 @@ bool M5StackTab5::initialize_lcd() { #else dpi_cfg.pixel_format = LCD_COLOR_PIXEL_FORMAT_RGB565; #endif - dpi_cfg.num_fbs = 1; + dpi_cfg.num_fbs = kNumDpiFramebuffers; dpi_cfg.video_timing.h_size = display_width_; dpi_cfg.video_timing.v_size = display_height_; dpi_cfg.video_timing.hsync_back_porch = 140; @@ -270,7 +301,7 @@ bool M5StackTab5::initialize_lcd() { #else dpi_cfg.pixel_format = LCD_COLOR_PIXEL_FORMAT_RGB565; #endif - dpi_cfg.num_fbs = 1; + dpi_cfg.num_fbs = kNumDpiFramebuffers; dpi_cfg.video_timing.h_size = display_width_; dpi_cfg.video_timing.v_size = display_height_; dpi_cfg.video_timing.hsync_back_porch = 40; @@ -295,7 +326,7 @@ bool M5StackTab5::initialize_lcd() { #else dpi_cfg.pixel_format = LCD_COLOR_PIXEL_FORMAT_RGB565; #endif - dpi_cfg.num_fbs = 1; + dpi_cfg.num_fbs = kNumDpiFramebuffers; dpi_cfg.video_timing.h_size = display_width_; dpi_cfg.video_timing.v_size = display_height_; dpi_cfg.video_timing.hsync_back_porch = 40; @@ -420,16 +451,29 @@ bool M5StackTab5::initialize_lcd() { // Cache the DPI framebuffer address so flush() can rotate directly into it // with the PPA (see flush()). The panel scans this buffer out continuously; // the espp draw/flush path itself never writes it with the CPU. + // + // This optimization is only valid with a single DPI framebuffer: caching one + // fixed pointer assumes the panel scans that same buffer forever. With + // num_fbs > 1 the driver flips between buffers and the PPA could rotate into + // one that is not being scanned out, so the guard below keeps the (always + // correct) scratch-buffer path instead. + static_assert(kNumDpiFramebuffers == 1, + "The direct-to-framebuffer PPA rotation caches a single framebuffer pointer; " + "with multiple DPI framebuffers it must track the active one instead"); { void *fb = nullptr; - if (esp_lcd_dpi_panel_get_frame_buffer(lcd_handles_.panel, 1, &fb) == ESP_OK && fb != nullptr) { + if (esp_lcd_dpi_panel_get_frame_buffer(lcd_handles_.panel, kNumDpiFramebuffers, &fb) == + ESP_OK && + fb != nullptr) { const size_t fb_bytes = static_cast(display_width_) * display_height_ * sizeof(Pixel); // The PPA requires its output buffer pointer and size to be aligned to - // the (128-byte) cache line; the esp_lcd driver allocates the DPI - // framebuffer DMA-aligned, and 720*1280*2 is a multiple of 128, but - // verify rather than assume — if it does not hold, flush() simply keeps - // the scratch-buffer path. - if ((reinterpret_cast(fb) % 128) == 0 && (fb_bytes % 128) == 0) { + // the data cache line (128 bytes on the ESP32-P4; queried, not assumed — + // see ppa_out_buffer_alignment()). The esp_lcd driver allocates the DPI + // framebuffer DMA-aligned, and 720*1280*2 is a multiple of any such + // line size, but verify rather than assume — if it does not hold, + // flush() simply keeps the scratch-buffer path. + const size_t cache_align = ppa_out_buffer_alignment(); + if ((reinterpret_cast(fb) % cache_align) == 0 && (fb_bytes % cache_align) == 0) { dpi_framebuffer_ = fb; dpi_framebuffer_bytes_ = fb_bytes; } else { @@ -551,12 +595,13 @@ bool M5StackTab5::initialize_display(size_t pixel_buffer_size) { g_ppa_client = nullptr; } } - // Align to 128 bytes: the PPA output buffer in external (PSRAM) memory must - // be aligned to the L1 and L2 cache line size, and the ESP32-P4's L2 line is - // 128 bytes. Both the pointer and the size must be a multiple of it. - static constexpr size_t kCacheAlign = 128; + // The PPA output buffer in external (PSRAM) memory must be aligned to the + // data cache line size (128 bytes on the ESP32-P4): both the pointer and the + // size must be a multiple of it. Query the requirement instead of + // hard-coding it (see ppa_out_buffer_alignment()). + const size_t cache_align = ppa_out_buffer_alignment(); size_t required_bytes = pixel_buffer_size * sizeof(uint16_t); - required_bytes = (required_bytes + kCacheAlign - 1) / kCacheAlign * kCacheAlign; + required_bytes = (required_bytes + cache_align - 1) / cache_align * cache_align; // Reuse the existing scratch buffer if it is already the right size; otherwise // free it first so a repeated initialize_display() call (e.g. re-init with a @@ -567,7 +612,7 @@ bool M5StackTab5::initialize_display(size_t pixel_buffer_size) { third_buffer = nullptr; } third_buffer_bytes = required_bytes; - third_buffer = (uint16_t *)heap_caps_aligned_alloc(kCacheAlign, third_buffer_bytes, + third_buffer = (uint16_t *)heap_caps_aligned_alloc(cache_align, third_buffer_bytes, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT); if (third_buffer == nullptr) { // The scratch buffer is required for display rotation - both the PPA path @@ -776,11 +821,14 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m // Map the logical (LVGL) area to physical panel coordinates up front: the // direct-to-framebuffer PPA path needs the rotated destination offsets // before the PPA runs, and the fallback paths need them for draw_bitmap. - lv_display_rotate_area(disp, const_cast(area)); - offsetx1 = area->x1; - offsetx2 = area->x2; - offsety1 = area->y1; - offsety2 = area->y2; + // Rotate a local copy — LVGL handed us a const pointer, so do not mutate + // its area in place. + lv_area_t rotated_area = *area; + lv_display_rotate_area(disp, &rotated_area); + offsetx1 = rotated_area.x1; + offsetx2 = rotated_area.x2; + offsety1 = rotated_area.y1; + offsety2 = rotated_area.y2; if (g_ppa_client != nullptr && dpi_framebuffer_ != nullptr) { // Hardware rotation via the PPA, writing the rotated block DIRECTLY into // the DPI panel's framebuffer at the rotated offset (the PPA output From b85dd725af04c965a9a81770f417103e40ea9058 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sat, 29 Aug 2026 14:59:34 -0500 Subject: [PATCH 08/15] fix(m5stack-tab5): gate DPI DMA2D per controller and harden the LCD publication-gate lifecycle - Skip esp_lcd_dpi_panel_enable_dma2d() on the ILI9881 variant: the repo documents (esp32-p4-function-ev-board, esp32-p4-nano) that the DMA2D draw_bitmap path corrupts RGB565 channel order on ILI9881C-family DSI panels; keep it for the ST7121/ST7123 TDDI variants (no corruption reported there, the underrun fix targets them, and M5Stack's own demo ships DMA2D-enabled on mostly-ST71xx units). Update the stale cross-reference in the function-ev-board note. - Apply the initial panel rotation BEFORE store-releasing lcd_initialized_: with the reversed init order an already-running LVGL thread could flush in the window between the gate opening and the MADCTL apply, skipping PPA rotation with the panel still in its old scan direction. The init path now uses a gate-free apply_panel_rotation() helper (same thread as the state writes) and the gate opens last. - Refuse LCD re-initialization once the gate has opened: clearing the atomic would not wait for readers that already observed true, so a concurrent flush()/write_lcd_lines() could race the re-init writes; the gate is now monotonic (false -> true, never back). Co-Authored-By: Claude Fable 5 --- .../esp32-p4-function-ev-board/src/video.cpp | 8 +- .../m5stack-tab5/include/m5stack-tab5.hpp | 19 +- components/m5stack-tab5/src/video.cpp | 176 ++++++++++++------ 3 files changed, 138 insertions(+), 65 deletions(-) diff --git a/components/esp32-p4-function-ev-board/src/video.cpp b/components/esp32-p4-function-ev-board/src/video.cpp index c8f4beb65a..e1d998490a 100644 --- a/components/esp32-p4-function-ev-board/src/video.cpp +++ b/components/esp32-p4-function-ev-board/src/video.cpp @@ -148,9 +148,11 @@ bool Esp32P4FunctionEvBoard::initialize_lcd() { // (esp_lcd_panel_draw_bitmap) through it corrupts the RGB565 channel order on // this board — colors come out brighter/greener and alpha blends render // wrong, while the bytes in the frame buffer are correct. The plain CPU copy - // path renders correctly and matches the m5stack-tab5 BSP, which also does - // not enable DMA2D on IDF >= 6. (An earlier "blank screen without DMA2D" was - // actually the RST_LCD/PWM jumper wiring, not DMA2D.) + // path renders correctly and matches the m5stack-tab5 BSP's ILI9881 variant, + // which skips DMA2D on IDF >= 6 for the same reason (its ST71xx variants + // enable it without issue, as does the esp32-p4-nano's JD9365). (An earlier + // "blank screen without DMA2D" was actually the RST_LCD/PWM jumper wiring, + // not DMA2D.) } // Send the panel controller's vendor init sequence over DBI (command mode), diff --git a/components/m5stack-tab5/include/m5stack-tab5.hpp b/components/m5stack-tab5/include/m5stack-tab5.hpp index 534b3cd38f..d9cc6f99cb 100644 --- a/components/m5stack-tab5/include/m5stack-tab5.hpp +++ b/components/m5stack-tab5/include/m5stack-tab5.hpp @@ -951,10 +951,13 @@ class M5StackTab5 : public BaseComponent { // by initialize_lcd() on the init thread; if the LVGL display already exists // (initialize_display() called first) the LVGL thread can be flushing // concurrently, so the readers must not touch them until they are all - // written. initialize_lcd() clears this flag on entry, writes every field, - // and store-releases it true as its final publication step; the readers - // load-acquire it and bail out while it is false. The release/acquire pair - // makes all of the writes happen-before any read that observes true. + // written. initialize_lcd() only runs while this flag is false (it refuses + // to re-initialize once the gate has opened — clearing the flag would not + // wait for readers that already observed true), writes every field, applies + // the initial panel rotation, and store-releases it true as its final + // publication step; the readers load-acquire it and bail out while it is + // false. The release/acquire pair makes all of the writes happen-before any + // read that observes true, and the flag never transitions true -> false. std::atomic lcd_initialized_{false}; // The DPI panel's (PSRAM) framebuffer, queried from esp_lcd once the panel @@ -978,6 +981,14 @@ class M5StackTab5 : public BaseComponent { // display rotation changes; routes the rotation to the display driver // (MADCTL) when the active panel can honor it in hardware. void on_display_rotation(const DisplayRotation &rotation); + // Gate-free core of on_display_rotation(): routes the rotation to the + // display driver (MADCTL) when the active panel honors it in hardware. + // Callers must guarantee display_driver_ / display_controller_ are safe to + // read: on_display_rotation() does so via its lcd_initialized_ acquire + // load; initialize_lcd() calls this directly on the init thread (which + // wrote those fields) BEFORE opening the gate, so the initial scan + // direction is programmed before any flush() can skip the PPA rotation. + void apply_panel_rotation(const DisplayRotation &rotation); // Whether the active display controller applies the given LVGL rotation in // panel hardware (via the display driver's set_rotation()/MADCTL), making // buffer rotation (PPA / software) in flush() unnecessary. diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index 8ac8b91376..b8e3203052 100644 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -142,16 +142,30 @@ M5StackTab5::DisplayController M5StackTab5::detect_display_controller() { bool M5StackTab5::initialize_lcd() { logger_.info("Initializing M5Stack Tab5 LCD (MIPI-DSI, {}x{})", display_width_, display_height_); - // Close the publication gate while the LCD state below (lcd_handles_, - // dpi_framebuffer_ + dpi_framebuffer_bytes_, display_driver_, - // display_controller_) is being (re)built. If the LVGL display already - // exists (initialize_display() called first) its thread may already be - // pumping flush(); flush()/write_lcd_lines()/on_display_rotation() all - // load-acquire this flag and no-op while it is false, so none of them can - // observe a partially initialized panel or a torn framebuffer-pointer/size - // pair. It is store-released true as the final step of this function, after - // every field has been written. - lcd_initialized_.store(false, std::memory_order_release); + // Re-initialization is NOT supported once the publication gate has opened: + // clearing lcd_initialized_ here would not wait for readers that already + // passed their acquire-load of true — a concurrent flush() or + // write_lcd_lines() could then race the rebuild of lcd_handles_ / + // display_driver_ / the framebuffer state it is still using. Rather than + // add reader/writer synchronization to the hot flush path for a re-init + // this BSP never needs (the panel hardware is fixed at boot), refuse. + if (lcd_initialized_.load(std::memory_order_acquire)) { + logger_.warn("LCD already initialized; re-initialization is not supported — skipping"); + return true; + } + + // The publication gate is provably closed here: this is either the first + // call or a retry after a failed attempt, and a failed attempt never + // reaches the final store-release. No reader can therefore have observed + // true, so the LCD state below (lcd_handles_, dpi_framebuffer_ + + // dpi_framebuffer_bytes_, display_driver_, display_controller_) can be + // built without racing them: even if the LVGL display already exists + // (initialize_display() called first) and its thread is already pumping + // flush(), flush()/write_lcd_lines()/on_display_rotation() all load-acquire + // this flag and no-op while it is false, so none of them can observe a + // partially initialized panel or a torn framebuffer-pointer/size pair. The + // gate is store-released true as the final step of this function, after + // every field has been written and the initial panel rotation applied. if (!ioexp_0x43_) { if (!initialize_io_expanders()) { @@ -440,11 +454,31 @@ bool M5StackTab5::initialize_lcd() { // scan-line axis (the driver logs "underrun happens" when it detects this). // The DMA2D copy runs without the CPU touching the data and completes via // the same on_color_trans_done callback registered above. - ret = esp_lcd_dpi_panel_enable_dma2d(lcd_handles_.panel); - if (ret != ESP_OK) { - // Not fatal: draw_bitmap falls back to the (slower) CPU copy. - logger_.warn("Could not enable DMA2D for DPI draw_bitmap ({}); using CPU copies", - esp_err_to_name(ret)); + // + // Gate it per controller, though: DMA2D is a color-processing engine, not a + // plain copy, and this repository documents (from hardware testing) that + // routing draw_bitmap through it corrupts the RGB565 channel order on + // ILI9881C-family DSI panels — colors render brighter/greener and alpha + // blends come out wrong even though the framebuffer bytes are correct (see + // components/esp32-p4-function-ev-board/src/video.cpp, ILI9881C/EK79007, + // and components/esp32-p4-nano/src/video.cpp, which enables DMA2D only for + // its JD9365 panel for the same reason). The Tab5's original revision uses + // that same ILI9881 family, so it keeps the always-correct CPU copy path + // and accepts the extra PSRAM traffic. The ST7121/ST7123 TDDI variants keep + // DMA2D: no such corruption has been reported for them, they are the units + // on which the underrun streaking this call addresses was reproduced, and + // M5Stack's own Tab5 demo firmware ships with DMA2D enabled on production + // units that are predominantly ST71xx. + if (display_controller_ != DisplayController::ILI9881) { + ret = esp_lcd_dpi_panel_enable_dma2d(lcd_handles_.panel); + if (ret != ESP_OK) { + // Not fatal: draw_bitmap falls back to the (slower) CPU copy. + logger_.warn("Could not enable DMA2D for DPI draw_bitmap ({}); using CPU copies", + esp_err_to_name(ret)); + } + } else { + logger_.info("ILI9881 variant: keeping CPU draw_bitmap copies (DMA2D corrupts RGB565 " + "channel order on ILI9881C-family panels)"); } #endif @@ -484,26 +518,20 @@ bool M5StackTab5::initialize_lcd() { } } - // Publish the fully initialized LCD state. Every field the cross-thread - // readers touch (lcd_handles_, dpi_framebuffer_ + dpi_framebuffer_bytes_, - // display_driver_, display_controller_) has been written above, so the - // release store here makes all of those writes visible to any - // flush()/write_lcd_lines()/on_display_rotation() call that load-acquires - // the flag as true. Until this store, those readers no-op (flush() just - // signals lv_display_flush_ready()), which is what closes the - // reversed-init-order race: with initialize_display() called first and the - // application already pumping LVGL on another thread, a concurrent flush() - // can no longer pass a plain lcd_handles_.panel null-check mid-build and - // read a not-yet-init'd panel or a torn framebuffer-pointer/size pair. - lcd_initialized_.store(true, std::memory_order_release); - - // Program the panel's initial scan direction so the rotation decision - // flush() makes via panel_handles_rotation() is valid from the very first - // frame. NOTE: unlike the normal invocation of on_display_rotation() — the - // LVGL rotation event, delivered synchronously on the LVGL thread — this is - // a direct call on the caller's (init) thread. It runs after the release - // store above (same thread, so its own acquire load sees true) and is safe - // in either init order: + // Program the panel's initial scan direction BEFORE publishing the LCD + // state, so the rotation decision flush() makes via + // panel_handles_rotation() is valid from the very first frame that can + // reach the panel. Ordering matters in the reversed init order + // (initialize_display() first): an already-running LVGL thread may flush + // the instant the gate opens, and if the gate opened before this call such + // a flush could skip the PPA rotation (panel_handles_rotation() true) while + // the panel's MADCTL still held the old scan direction. Applying the + // rotation first closes that window. on_display_rotation() itself no-ops + // while the gate is closed, so the init path calls the gate-free + // apply_panel_rotation() helper directly — safe, because this is the same + // thread that wrote display_driver_/display_controller_ above (no + // synchronization needed against itself) and any concurrent flush() still + // no-ops on the closed gate. Per init order: // - Documented order (initialize_lcd() before initialize_display()): // display_ is still null here, so no LVGL display or flush callback // exists yet and espp::Display does not run an LVGL handler task of its @@ -520,14 +548,28 @@ bool M5StackTab5::initialize_lcd() { // cannot corrupt an in-flight flush: the MADCTL write travels on the DSI // command channel, which never touches the DPI framebuffer or its DMA and // is arbitrated against the video stream in hardware (see - // on_display_rotation()). The worst case is one transient frame scanned - // out with the new direction — the same as any runtime rotation change. + // apply_panel_rotation()). // (With CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION disabled — the default — - // on_display_rotation() is a no-op and this call does nothing at all.) - on_display_rotation( + // apply_panel_rotation() is a no-op and this call does nothing at all.) + apply_panel_rotation( display_ ? to_display_rotation(lv_display_get_rotation(display_->get_lvgl_display())) : rotation); + // Publish the fully initialized LCD state — the gate opens LAST. Every + // field the cross-thread readers touch (lcd_handles_, dpi_framebuffer_ + + // dpi_framebuffer_bytes_, display_driver_, display_controller_) has been + // written above and the panel's initial MADCTL rotation has been applied, + // so the release store here makes all of that visible to any + // flush()/write_lcd_lines()/on_display_rotation() call that load-acquires + // the flag as true. Until this store, those readers no-op (flush() just + // signals lv_display_flush_ready()), which is what closes the + // reversed-init-order races: with initialize_display() called first and the + // application already pumping LVGL on another thread, a concurrent flush() + // can no longer pass a plain lcd_handles_.panel null-check mid-build and + // read a not-yet-init'd panel, a torn framebuffer-pointer/size pair, or a + // panel whose scan direction does not yet match panel_handles_rotation(). + lcd_initialized_.store(true, std::memory_order_release); + logger_.info("M5Stack Tab5 LCD initialization completed successfully"); return true; } @@ -698,25 +740,40 @@ void M5StackTab5::on_display_rotation(const DisplayRotation &rotation) { // initialize_lcd()) before reading display_driver_ / display_controller_: // with initialize_display() called first, LVGL rotation events can arrive // while initialize_lcd() is still writing those fields. Bailing out here is - // harmless — initialize_lcd() re-applies the current LVGL rotation via its - // direct call once the gate is open. + // harmless — initialize_lcd() applies the current LVGL rotation via + // apply_panel_rotation() before it opens the gate, so the panel state is + // already consistent for the first flush() that observes the gate open. if (!lcd_initialized_.load(std::memory_order_acquire)) { return; } + apply_panel_rotation(rotation); +#endif +} + +void M5StackTab5::apply_panel_rotation(const DisplayRotation &rotation) { +#if !CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION + // Panel-side rotation disabled (see panel_handles_rotation()): complete + // no-op so NO MADCTL write ever reaches the panel. + (void)rotation; +#else if (!display_driver_ || display_controller_ != DisplayController::ST7121) { // Other variants keep the PPA/flush-time rotation path; nothing to do. return; } - // Ordering: in its normal invocation — the espp::Display rotation callback — - // this runs on the LVGL thread: LV_EVENT_RESOLUTION_CHANGED is sent - // synchronously from inside lv_display_set_rotation(), so it strictly - // precedes the invalidation-driven flush() calls for the new orientation, - // and the MADCTL state below is always consistent with the decision flush() - // makes via panel_handles_rotation() (the same predicate, keyed on the same - // rotation value via to_lv_rotation()). It is additionally called once - // directly from initialize_lcd() (an init-thread call, not the LVGL thread) - // to program the initial scan direction; see the safety analysis at that - // call site. + // Ordering: in its normal invocation — the espp::Display rotation callback, + // via on_display_rotation() — this runs on the LVGL thread: + // LV_EVENT_RESOLUTION_CHANGED is sent synchronously from inside + // lv_display_set_rotation(), so it strictly precedes the + // invalidation-driven flush() calls for the new orientation, and the MADCTL + // state below is always consistent with the decision flush() makes via + // panel_handles_rotation() (the same predicate, keyed on the same rotation + // value via to_lv_rotation()). It is additionally called once directly from + // initialize_lcd() (an init-thread call, not the LVGL thread, deliberately + // BEFORE the lcd_initialized_ gate opens) to program the initial scan + // direction; see the safety analysis at that call site. This helper is + // gate-free: callers must guarantee display_driver_/display_controller_ are + // safe to read (on_display_rotation() does so via its acquire load; the + // init path wrote them on the same thread). // // Synchronization with the display pipeline: the MADCTL write goes out on // the DSI generic/DBI command channel (esp_lcd_panel_io_tx_param -> @@ -788,11 +845,12 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m // Acquire-load the publication gate before touching any LCD state. This // pairs with the release store at the end of initialize_lcd(): observing - // true guarantees lcd_handles_ (incl. a fully init'd, DMA2D-enabled panel), - // the dpi_framebuffer_/dpi_framebuffer_bytes_ pair, display_driver_ and - // display_controller_ are all completely written and will not be written - // again. Until then (LCD not yet initialized, or mid re-init) just tell - // LVGL the flush is done and drop the frame. + // true guarantees lcd_handles_ (incl. a fully init'd panel, DMA2D-enabled + // on the ST71xx variants), the dpi_framebuffer_/dpi_framebuffer_bytes_ + // pair, display_driver_ and display_controller_ are all completely written + // and will not be written again (the gate never closes once open — + // initialize_lcd() refuses to re-run). Until then (LCD not yet initialized) + // just tell LVGL the flush is done and drop the frame. if (!lcd_initialized_.load(std::memory_order_acquire)) { lv_display_flush_ready(disp); return; @@ -873,8 +931,10 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m // the PPA driver performed the cache maintenance (write-back of the // source window, invalidate of the destination window). The espp // flush path never dirties the framebuffer with the CPU (draw_bitmap - // copies run on the DMA2D engine, and its CPU fallback writes back - // the cache before returning), so there is nothing left to sync and + // copies run on the DMA2D engine on the ST71xx variants, and the CPU + // copy path — used by the ILI9881 variant and as the DMA2D fallback — + // writes back the cache before returning), so there is nothing left + // to sync and // no draw_bitmap call is needed: signal LVGL directly. lv_display_flush_ready(disp); return; From 74a7e6b2f22ceae0776e99cd10d6ba6eceeeab72 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sat, 29 Aug 2026 19:55:16 -0500 Subject: [PATCH 09/15] fix(m5stack-tab5): replace private esp_cache_private.h with the public Kconfig cache-line constant The PPA output-buffer alignment was queried at runtime via esp_cache_get_alignment(), which is only declared in the private esp_private/esp_cache_private.h header and can move or break across IDF releases. On the ESP32-P4 that call just reads back the L2 cache line size that cpu_start.c programs into the cache HAL from CONFIG_CACHE_L2_CACHE_LINE_SIZE (64 or 128), so use that public compile-time constant directly (with a conservative 128-byte fallback if the symbol is ever absent). This also removes the component's only use of esp_mm, so drop the esp_mm REQUIRES again. Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/CMakeLists.txt | 2 +- components/m5stack-tab5/src/video.cpp | 57 ++++++++++++++------------ 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/components/m5stack-tab5/CMakeLists.txt b/components/m5stack-tab5/CMakeLists.txt index 1ff161b1aa..7d91d55ad7 100755 --- a/components/m5stack-tab5/CMakeLists.txt +++ b/components/m5stack-tab5/CMakeLists.txt @@ -1,6 +1,6 @@ idf_component_register( INCLUDE_DIRS "include" SRC_DIRS "src" - REQUIRES driver esp_driver_i2s esp_driver_ppa esp_driver_sdmmc esp_driver_spi esp_lcd esp_mm esp_video esp_cam_sensor fatfs base_component bmi270 codec display display_drivers gt911 i2c ina226 input_drivers interrupt pi4ioe5v rx8130ce st7123touch task touch + REQUIRES driver esp_driver_i2s esp_driver_ppa esp_driver_sdmmc esp_driver_spi esp_lcd esp_video esp_cam_sensor fatfs base_component bmi270 codec display display_drivers gt911 i2c ina226 input_drivers interrupt pi4ioe5v rx8130ce st7123touch task touch REQUIRED_IDF_TARGETS "esp32p4" ) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index b8e3203052..be303252ae 100644 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -11,12 +11,14 @@ #include #include +#include + #include +#include #include #include #include #include -#include using namespace std::chrono_literals; @@ -54,22 +56,25 @@ static constexpr uint8_t kNumDpiFramebuffers = 1; // Alignment the PPA requires for its output buffer in external (PSRAM) memory: // both the buffer pointer and the buffer size must be multiples of the data -// cache line size. Query it from the cache driver rather than hard-coding the -// ESP32-P4's 128-byte L2 line — this is the exact value the PPA driver itself -// validates against (ppa_check_buffer_alignment() checks -// s_platform.buf_alignment_size, which it obtains via -// esp_cache_get_alignment(MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA, ...)). -static size_t ppa_out_buffer_alignment() { - size_t alignment = 0; - if (esp_cache_get_alignment(MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA, &alignment) != ESP_OK || - alignment == 0) { - // Should not happen (the call only fails on invalid arguments); fall back - // to the largest cache line on any current target so alignment is never - // under-estimated. - alignment = 128; - } - return alignment; -} +// cache line size. On the ESP32-P4 external memory sits behind the L2 cache, +// whose line size is a Kconfig choice (CONFIG_CACHE_L2_CACHE_LINE_SIZE, 64 or +// 128 bytes) that cpu_start.c programs into the cache HAL at boot — so this +// public compile-time constant equals exactly the value the PPA driver itself +// validates against at runtime (ppa_check_buffer_alignment() checks +// s_platform.buf_alignment_size, which it obtains from that same cache HAL via +// the private esp_cache_get_alignment(MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA, +// ...)). Using the Kconfig constant keeps this file off the private +// esp_private/esp_cache_private.h header, which can move or break across IDF +// releases. +#if defined(CONFIG_CACHE_L2_CACHE_LINE_SIZE) && (CONFIG_CACHE_L2_CACHE_LINE_SIZE > 0) +static constexpr size_t kPpaOutBufferAlignment = CONFIG_CACHE_L2_CACHE_LINE_SIZE; +#else +// Fallback for IDF configurations that do not expose the symbol (it has +// shipped with ESP32-P4 support from the start, so this is belt-and-braces): +// use the largest L2 cache line the P4 supports so the alignment is never +// under-estimated — over-aligning is always safe here. +static constexpr size_t kPpaOutBufferAlignment = 128; +#endif M5StackTab5::DisplayController M5StackTab5::detect_display_controller() { auto &i2c = internal_i2c(); @@ -501,12 +506,11 @@ bool M5StackTab5::initialize_lcd() { fb != nullptr) { const size_t fb_bytes = static_cast(display_width_) * display_height_ * sizeof(Pixel); // The PPA requires its output buffer pointer and size to be aligned to - // the data cache line (128 bytes on the ESP32-P4; queried, not assumed — - // see ppa_out_buffer_alignment()). The esp_lcd driver allocates the DPI - // framebuffer DMA-aligned, and 720*1280*2 is a multiple of any such - // line size, but verify rather than assume — if it does not hold, - // flush() simply keeps the scratch-buffer path. - const size_t cache_align = ppa_out_buffer_alignment(); + // the data cache line (see kPpaOutBufferAlignment). The esp_lcd driver + // allocates the DPI framebuffer DMA-aligned, and 720*1280*2 is a + // multiple of any such line size, but verify rather than assume — if it + // does not hold, flush() simply keeps the scratch-buffer path. + const size_t cache_align = kPpaOutBufferAlignment; if ((reinterpret_cast(fb) % cache_align) == 0 && (fb_bytes % cache_align) == 0) { dpi_framebuffer_ = fb; dpi_framebuffer_bytes_ = fb_bytes; @@ -638,10 +642,9 @@ bool M5StackTab5::initialize_display(size_t pixel_buffer_size) { } } // The PPA output buffer in external (PSRAM) memory must be aligned to the - // data cache line size (128 bytes on the ESP32-P4): both the pointer and the - // size must be a multiple of it. Query the requirement instead of - // hard-coding it (see ppa_out_buffer_alignment()). - const size_t cache_align = ppa_out_buffer_alignment(); + // data cache line size: both the pointer and the size must be a multiple of + // it (see kPpaOutBufferAlignment). + const size_t cache_align = kPpaOutBufferAlignment; size_t required_bytes = pixel_buffer_size * sizeof(uint16_t); required_bytes = (required_bytes + cache_align - 1) / cache_align * cache_align; From 60972be7424b2e2abfc321a685b92820c63733d4 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sat, 29 Aug 2026 22:23:53 -0500 Subject: [PATCH 10/15] fix(m5stack-tab5): suppress cppcheck config-exploration false positives The static-analysis CI flagged the new cache-line alignment checks (video.cpp:514): cppcheck's --force explores a preprocessor configuration where CONFIG_CACHE_L2_CACHE_LINE_SIZE folds to 1, trivializing the modulo checks (knownConditionTrueFalse/moduloofone); on real configurations the constant is 64 or 128 and both checks are meaningful. Also suppressed the flush()-path knownConditionTrueFalse where panel_handles_rotation() is a compile-time false in the (default) HW-rotation-disabled configuration. Inline suppressions with justifications; both build configurations still compile clean and local cppcheck reports no findings at either site. Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/src/video.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index be303252ae..a3adbdab4c 100644 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -511,6 +511,10 @@ bool M5StackTab5::initialize_lcd() { // multiple of any such line size, but verify rather than assume — if it // does not hold, flush() simply keeps the scratch-buffer path. const size_t cache_align = kPpaOutBufferAlignment; + // cppcheck-suppress [knownConditionTrueFalse, moduloofone] // cache_align + // is a Kconfig compile-time constant (64 or 128); cppcheck's --force + // explores a configuration where the macro folds to 1, trivializing the + // checks. On real configurations both checks are meaningful. if ((reinterpret_cast(fb) % cache_align) == 0 && (fb_bytes % cache_align) == 0) { dpi_framebuffer_ = fb; dpi_framebuffer_bytes_ = fb_bytes; @@ -874,6 +878,9 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m // logical frame is written to the framebuffer unrotated and at unrotated // coordinates; the panel flips the whole frame at scan-out, which lands each // partial area exactly where LVGL's rotated mapping expects it. + // cppcheck-suppress knownConditionTrueFalse // panel_handles_rotation() is + // a compile-time constant false only in the (default) configuration with + // CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION disabled, which cppcheck analyzes. if (rotation > LV_DISPLAY_ROTATION_0 && !panel_handles_rotation(rotation) && third_buffer != nullptr) { int32_t ww = lv_area_get_width(area); From 2d69b61b4f4f5444569746ea1224404540b74220 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sat, 29 Aug 2026 22:35:20 -0500 Subject: [PATCH 11/15] fix(m5stack-tab5): harden LCD retry state and enforce the RGB565-only video path Address the remaining PR #738 review feedback on video.cpp: - Reset dpi_framebuffer_/dpi_framebuffer_bytes_ at the start of every initialize_lcd() attempt: a retry after a failed attempt that then cannot re-cache the framebuffer (query or alignment failure) could otherwise open the publication gate with a stale pointer from the previous attempt still cached, and flush()'s direct-to-framebuffer PPA path would write through it. - Make the kNumDpiFramebuffers commentary match the static_assert: the single-framebuffer assumption is an intentional compile-time guard (raising the count requires teaching the PPA path to track the active framebuffer), not a runtime fallback as the old comment claimed. - Enforce the RGB565-only video path at compile time: the DPI panel config, the PPA rotation color modes, and the buffer sizing all hard-code RGB565, so static_assert LV_COLOR_DEPTH == 16 (and a 2-byte Pixel) instead of corrupting output if LVGL were configured with a different color depth, and size the rotation scratch buffer with sizeof(Pixel) rather than a bare sizeof(uint16_t). Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/src/video.cpp | 47 +++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 7 deletions(-) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index a3adbdab4c..b5c2773006 100644 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -43,15 +43,31 @@ static constexpr DisplayRotation to_display_rotation(lv_display_rotation_t rotat return static_cast(rotation); } +// The entire video path is RGB565-only: the DPI panel is configured for +// RGB565 in initialize_lcd(), flush()'s PPA rotation uses +// PPA_SRM_COLOR_MODE_RGB565 for both its input and output, and every buffer +// is sized in sizeof(Pixel) = 2-byte pixels. LVGL must therefore render +// RGB565 as well (LV_COLOR_DEPTH 16) — with any other color depth the flush +// callback would receive pixels these fixed color modes and sizes +// misinterpret (wrong colors at best, buffer overruns at worst). Refuse to +// build such a configuration instead of failing at runtime; supporting +// another depth means plumbing the active format through the DPI config, the +// PPA color modes, and the buffer sizing together. +static_assert(LV_COLOR_DEPTH == 16 && sizeof(M5StackTab5::Pixel) == sizeof(uint16_t), + "The Tab5 video path (DPI panel config, PPA rotation color modes, buffer " + "sizing) is RGB565-only; configure LVGL with LV_COLOR_DEPTH 16"); + // Number of framebuffers the DPI panel is created with (esp_lcd_dpi_panel_config_t::num_fbs, // used for every controller variant below). The direct-to-framebuffer PPA // rotation in flush() caches THE single framebuffer pointer at init and writes // into it unconditionally; with more than one framebuffer the driver flips // which buffer is scanned out, and rotating into a fixed one would -// intermittently update a non-visible buffer. flush() therefore only enables -// that optimization when this is 1 (see initialize_lcd()); if you raise this, -// the code falls back to the scratch-buffer path (or teach it to track the -// active framebuffer). +// intermittently update a non-visible buffer. That optimization is therefore +// only implemented for exactly one framebuffer: raising this value requires +// first teaching the direct-to-framebuffer path to track the active +// framebuffer (or removing it in favor of the always-correct scratch-buffer +// path), and a static_assert in initialize_lcd() enforces that at compile +// time rather than letting the stale-pointer bug ship. static constexpr uint8_t kNumDpiFramebuffers = 1; // Alignment the PPA requires for its output buffer in external (PSRAM) memory: @@ -172,6 +188,20 @@ bool M5StackTab5::initialize_lcd() { // gate is store-released true as the final step of this function, after // every field has been written and the initial panel rotation applied. + // Start every attempt from a clean slate for the state that is otherwise + // only assigned on success paths below. A failed attempt never opened the + // gate, so no reader has seen these, but it may still have left them set — + // and a retry does not necessarily rewrite them (the framebuffer caching + // below deliberately leaves them untouched when the query or alignment + // check fails). Without this reset such a retry could open the gate with a + // stale framebuffer pointer from the previous attempt still cached, and + // flush()'s direct-to-framebuffer PPA path would then write through it. + // (display_driver_ / display_controller_ need no reset here: display_driver_ + // is .reset() unconditionally below and both are rewritten together before + // the gate can open.) + dpi_framebuffer_ = nullptr; + dpi_framebuffer_bytes_ = 0; + if (!ioexp_0x43_) { if (!initialize_io_expanders()) { logger_.error("Failed to init IO expanders for LCD reset"); @@ -494,8 +524,11 @@ bool M5StackTab5::initialize_lcd() { // This optimization is only valid with a single DPI framebuffer: caching one // fixed pointer assumes the panel scans that same buffer forever. With // num_fbs > 1 the driver flips between buffers and the PPA could rotate into - // one that is not being scanned out, so the guard below keeps the (always - // correct) scratch-buffer path instead. + // one that is not being scanned out. Multi-framebuffer operation is + // intentionally unsupported until this path learns to track the active + // framebuffer, and the static_assert below turns raising + // kNumDpiFramebuffers without doing that work into a compile-time error + // instead of an intermittent visual glitch. static_assert(kNumDpiFramebuffers == 1, "The direct-to-framebuffer PPA rotation caches a single framebuffer pointer; " "with multiple DPI framebuffers it must track the active one instead"); @@ -649,7 +682,7 @@ bool M5StackTab5::initialize_display(size_t pixel_buffer_size) { // data cache line size: both the pointer and the size must be a multiple of // it (see kPpaOutBufferAlignment). const size_t cache_align = kPpaOutBufferAlignment; - size_t required_bytes = pixel_buffer_size * sizeof(uint16_t); + size_t required_bytes = pixel_buffer_size * sizeof(Pixel); required_bytes = (required_bytes + cache_align - 1) / cache_align * cache_align; // Reuse the existing scratch buffer if it is already the right size; otherwise From df3cf9ea4b2fc83c3e687fb96ea0cb434a355cff Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 30 Aug 2026 11:30:37 -0500 Subject: [PATCH 12/15] fix(m5stack-tab5): PPA-correct scratch allocation caps, single RGB565 config site, Pixel-typed buffer Round-8 review fixes + static-analysis repair: - The rotation scratch buffer is allocated with MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA - it is a PPA (DMA) output target and these are exactly the caps kPpaOutBufferAlignment is derived for (was SPIRAM|8BIT). - The DPI pixel-format selection is consolidated to ONE shared site for all three controller variants, with a comment noting the preprocessor branch only picks IDF-version field names for the single supported RGB565 format (the file-level static_assert already enforces RGB565-only). - third_buffer is typed as M5StackTab5::Pixel* to match the sizeof(Pixel) sizing introduced earlier. - Removed the now-stale knownConditionTrueFalse inline suppression on the flush() rotation gate: CI's cppcheck no longer emits that finding there, so the suppression itself failed the static-analysis run as unmatchedSuppression (cppcheck version divergence; same repair as the coredump component). Both Kconfig states build clean. Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/src/video.cpp | 46 +++++++++++++-------------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index b5c2773006..3f723b81f4 100644 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -316,12 +316,6 @@ bool M5StackTab5::initialize_lcd() { dpi_cfg.virtual_channel = 0; dpi_cfg.dpi_clk_src = MIPI_DSI_DPI_CLK_SRC_DEFAULT; dpi_cfg.dpi_clock_freq_mhz = 60; -#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(6, 0, 0) - dpi_cfg.in_color_format = LCD_COLOR_FMT_RGB565; - dpi_cfg.out_color_format = LCD_COLOR_FMT_RGB565; -#else - dpi_cfg.pixel_format = LCD_COLOR_PIXEL_FORMAT_RGB565; -#endif dpi_cfg.num_fbs = kNumDpiFramebuffers; dpi_cfg.video_timing.h_size = display_width_; dpi_cfg.video_timing.v_size = display_height_; @@ -344,12 +338,6 @@ bool M5StackTab5::initialize_lcd() { // (M5Stack/esp-bsp value) shrinks the blanking window and desyncs the touch // scan, so the panel shows but touch never reports. Keep this at 70 MHz. dpi_cfg.dpi_clock_freq_mhz = 70; -#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(6, 0, 0) - dpi_cfg.in_color_format = LCD_COLOR_FMT_RGB565; - dpi_cfg.out_color_format = LCD_COLOR_FMT_RGB565; -#else - dpi_cfg.pixel_format = LCD_COLOR_PIXEL_FORMAT_RGB565; -#endif dpi_cfg.num_fbs = kNumDpiFramebuffers; dpi_cfg.video_timing.h_size = display_width_; dpi_cfg.video_timing.v_size = display_height_; @@ -369,12 +357,6 @@ bool M5StackTab5::initialize_lcd() { // against the pixel clock; use the M5GFX reference 70 MHz and porch set // (they differ from the ST7123's: VBP 24 / VPW 20 / VFP 200). dpi_cfg.dpi_clock_freq_mhz = 70; -#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(6, 0, 0) - dpi_cfg.in_color_format = LCD_COLOR_FMT_RGB565; - dpi_cfg.out_color_format = LCD_COLOR_FMT_RGB565; -#else - dpi_cfg.pixel_format = LCD_COLOR_PIXEL_FORMAT_RGB565; -#endif dpi_cfg.num_fbs = kNumDpiFramebuffers; dpi_cfg.video_timing.h_size = display_width_; dpi_cfg.video_timing.v_size = display_height_; @@ -390,6 +372,18 @@ bool M5StackTab5::initialize_lcd() { } if (lcd_handles_.panel == nullptr) { + // The DPI pixel format is set here - in exactly one place, shared by all + // controller variants - and is always RGB565: the whole video path is + // RGB565-only (see the static_assert at the top of this file), and no + // Kconfig or build flag selects any other panel format. The preprocessor + // branch below only picks the IDF-version-specific field names for that + // one format, never a different depth. +#if ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(6, 0, 0) + dpi_cfg.in_color_format = LCD_COLOR_FMT_RGB565; + dpi_cfg.out_color_format = LCD_COLOR_FMT_RGB565; +#else + dpi_cfg.pixel_format = LCD_COLOR_PIXEL_FORMAT_RGB565; +#endif logger_.info("Creating DPI panel with resolution {}x{}", dpi_cfg.video_timing.h_size, dpi_cfg.video_timing.v_size); ret = esp_lcd_new_panel_dpi(lcd_handles_.mipi_dsi_bus, &dpi_cfg, &lcd_handles_.panel); @@ -618,7 +612,7 @@ bool M5StackTab5::initialize_lcd() { // Scratch buffer that holds the hardware-rotated frame produced by the PPA // before it is handed to the panel (see flush()). Kept in PSRAM and aligned to // the data-cache line size, which the PPA requires for its output buffer. -static uint16_t *third_buffer = nullptr; +static M5StackTab5::Pixel *third_buffer = nullptr; static size_t third_buffer_bytes = 0; // PPA (Pixel Processing Accelerator) client used to rotate the frame in // hardware instead of on the CPU (lv_draw_sw_rotate). CPU rotation of a @@ -694,8 +688,15 @@ bool M5StackTab5::initialize_display(size_t pixel_buffer_size) { third_buffer = nullptr; } third_buffer_bytes = required_bytes; - third_buffer = (uint16_t *)heap_caps_aligned_alloc(cache_align, third_buffer_bytes, - MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT); + // Request MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA: the buffer is a PPA (DMA) + // output target, and these are exactly the caps kPpaOutBufferAlignment is + // derived for. The heap layer supports this combination for external + // memory (esp_heap_adjust_alignment_to_hw() applies the cache-line + // alignment/size the caps require, then maps the request onto the PSRAM + // heap), so on the ESP32-P4 - whose PSRAM is DMA-capable - it yields a + // DMA-usable PSRAM allocation. + third_buffer = static_cast(heap_caps_aligned_alloc( + cache_align, third_buffer_bytes, MALLOC_CAP_SPIRAM | MALLOC_CAP_DMA)); if (third_buffer == nullptr) { // The scratch buffer is required for display rotation - both the PPA path // and the software fallback rotate into it - so without it a non-zero @@ -911,9 +912,6 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m // logical frame is written to the framebuffer unrotated and at unrotated // coordinates; the panel flips the whole frame at scan-out, which lands each // partial area exactly where LVGL's rotated mapping expects it. - // cppcheck-suppress knownConditionTrueFalse // panel_handles_rotation() is - // a compile-time constant false only in the (default) configuration with - // CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION disabled, which cppcheck analyzes. if (rotation > LV_DISPLAY_ROTATION_0 && !panel_handles_rotation(rotation) && third_buffer != nullptr) { int32_t ww = lv_area_get_width(area); From 550999d04d9052a3d602313f080be8aba776c0e1 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Sun, 30 Aug 2026 23:02:33 -0500 Subject: [PATCH 13/15] fix(m5stack-tab5): avoid cross-thread LVGL read when applying the initial panel rotation initialize_lcd()'s init-path apply_panel_rotation() called lv_display_get_rotation() from the init thread when display_ already existed (reversed init order) - reading LVGL state without the app's LVGL lock while the GUI task may be running (LV_USE_OS == LV_OS_NONE). Use the configured initial 'rotation' instead; runtime rotation changes are still applied by on_display_rotation() (the LVGL rotation callback, on the GUI task). Both Kconfig states build clean. Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/src/video.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index 3f723b81f4..4aac26d77c 100644 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -586,9 +586,14 @@ bool M5StackTab5::initialize_lcd() { // apply_panel_rotation()). // (With CONFIG_M5STACK_TAB5_ST7121_HW_ROTATION disabled — the default — // apply_panel_rotation() is a no-op and this call does nothing at all.) - apply_panel_rotation( - display_ ? to_display_rotation(lv_display_get_rotation(display_->get_lvgl_display())) - : rotation); + // Use the configured initial `rotation` rather than querying LVGL here: + // in the reversed init order the LVGL update task may already be running, + // and lv_display_get_rotation() from this (init) thread would read LVGL + // state without the application's LVGL lock (LV_USE_OS == LV_OS_NONE). Any + // rotation the app sets later is applied by on_display_rotation() (the LVGL + // rotation callback, which runs on the GUI task), so the panel still tracks + // runtime changes; this call only establishes the initial MADCTL. + apply_panel_rotation(rotation); // Publish the fully initialized LCD state — the gate opens LAST. Every // field the cross-thread readers touch (lcd_handles_, dpi_framebuffer_ + From 236f42b49c6846066233e3758a87c7d87ea7e322 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Mon, 31 Aug 2026 09:18:36 -0500 Subject: [PATCH 14/15] fix(m5stack-tab5): flush/geometry use the BSP-managed display, not the global LVGL default flush() and rotated_display_width()/height() read the rotation from lv_display_get_default() rather than the display actually being used (the flush callback's disp argument / the BSP's own display). Correct in a single-display setup but wrong if another LVGL display is the default; matches the fix already applied to the sibling P4 BSPs. Builds clean. Co-Authored-By: Claude Fable 5 --- components/m5stack-tab5/src/video.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index 4aac26d77c..2ceef99e6f 100644 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -718,7 +718,8 @@ bool M5StackTab5::initialize_display(size_t pixel_buffer_size) { } size_t M5StackTab5::rotated_display_width() const { - auto rotation = lv_display_get_rotation(lv_display_get_default()); + auto rotation = + lv_display_get_rotation(display_ ? display_->get_lvgl_display() : lv_display_get_default()); switch (rotation) { // swap case LV_DISPLAY_ROTATION_90: @@ -733,7 +734,8 @@ size_t M5StackTab5::rotated_display_width() const { } size_t M5StackTab5::rotated_display_height() const { - auto rotation = lv_display_get_rotation(lv_display_get_default()); + auto rotation = + lv_display_get_rotation(display_ ? display_->get_lvgl_display() : lv_display_get_default()); switch (rotation) { // swap case LV_DISPLAY_ROTATION_90: @@ -907,7 +909,8 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m int offsety1 = area->y1; int offsety2 = area->y2; - auto rotation = lv_display_get_rotation(lv_display_get_default()); + // Use the display actually being flushed, not the global default display. + auto rotation = lv_display_get_rotation(disp); // Cache the rotation for the camera task, which must not call LVGL from its // own thread. flush() runs on the LVGL (GUI) thread, so reading it here is // safe; the camera task reads the cached atomic instead. From cfdbd8d9585ec172fbd0423676fc88e5f44f9fe4 Mon Sep 17 00:00:00 2001 From: William Emfinger Date: Mon, 31 Aug 2026 09:47:46 -0500 Subject: [PATCH 15/15] fix(m5stack-tab5): serialize panel draws so async DMA2D draw_bitmap is single-flight-safe With the IDF6 DMA2D hook, esp_lcd_panel_draw_bitmap() is asynchronous and single-flight (a second call while one is in flight returns ESP_ERR_INVALID_STATE). Both flush() and the public cross-thread write_lcd_lines() issued draws without serialization or checking the return, so a concurrent direct write could make an LVGL flush's draw fail with no on_color_trans_done -> LVGL waits forever; and a direct write's completion could be taken for an LVGL flush completion. Now every panel draw goes through draw_and_wait() under panel_op_mutex_: the ISR only gives a completion semaphore (no longer signals LVGL); each draw holds the mutex across the draw and its completion wait (only one transfer in flight); draw_and_wait() checks the return and skips the wait on failure, and flush() ALWAYS calls lv_display_flush_ready() afterwards so a failed draw never hangs LVGL (frame dropped); the PPA direct-to-FB path takes the same mutex around its framebuffer write. Both Kconfig states build clean. Co-Authored-By: Claude Fable 5 --- .../m5stack-tab5/include/m5stack-tab5.hpp | 18 +++++ components/m5stack-tab5/src/video.cpp | 70 ++++++++++++++++--- 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/components/m5stack-tab5/include/m5stack-tab5.hpp b/components/m5stack-tab5/include/m5stack-tab5.hpp index d9cc6f99cb..9aa3a247e8 100644 --- a/components/m5stack-tab5/include/m5stack-tab5.hpp +++ b/components/m5stack-tab5/include/m5stack-tab5.hpp @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -285,6 +286,10 @@ class M5StackTab5 : public BaseComponent { /// \note This method queues the panel transfer asynchronously and may return /// before the write has completed. void write_lcd_lines(int xs, int ys, int xe, int ye, const uint8_t *data, uint32_t user_data); + // Issue one draw_bitmap and block until the DPI copy completes (or a + // bounded timeout). Caller must hold panel_op_mutex_. Returns false if the + // draw was rejected/failed (no completion will arrive). + bool draw_and_wait(int x1, int y1, int x2, int y2, const void *data); ///////////////////////////////////////////////////////////////////////////// // Audio System @@ -960,6 +965,19 @@ class M5StackTab5 : public BaseComponent { // read that observes true, and the flag never transitions true -> false. std::atomic lcd_initialized_{false}; + // Serializes every panel draw. esp_lcd_panel_draw_bitmap() is asynchronous and + // single-flight when the DMA2D hook is enabled (a second call while one is in + // flight returns ESP_ERR_INVALID_STATE), and flush() and the public, + // cross-thread write_lcd_lines() both issue draws. Holding this mutex across + // the draw AND its completion wait means only one transfer is ever in flight, + // so a direct write cannot make an LVGL flush's draw fail (which would leave + // LVGL waiting forever) and a direct write's completion cannot be mistaken for + // an LVGL flush completion. + std::mutex panel_op_mutex_; + // Signalled from the on_color_trans_done ISR for each completed draw; the + // issuing draw (under panel_op_mutex_) waits on it, making draws synchronous. + SemaphoreHandle_t draw_done_sem_{nullptr}; + // The DPI panel's (PSRAM) framebuffer, queried from esp_lcd once the panel // is created. flush() uses it to rotate LVGL draw buffers directly into the // scanned-out framebuffer with the PPA, skipping the intermediate scratch diff --git a/components/m5stack-tab5/src/video.cpp b/components/m5stack-tab5/src/video.cpp index 2ceef99e6f..afdb5d47b1 100644 --- a/components/m5stack-tab5/src/video.cpp +++ b/components/m5stack-tab5/src/video.cpp @@ -464,6 +464,15 @@ bool M5StackTab5::initialize_lcd() { .on_color_trans_done = &M5StackTab5::notify_lvgl_flush_ready, .on_refresh_done = nullptr, }; + // Completion semaphore for the serialized synchronous draws (see + // draw_and_wait()); created before the callback that gives it can fire. + if (draw_done_sem_ == nullptr) { + draw_done_sem_ = xSemaphoreCreateBinary(); + if (draw_done_sem_ == nullptr) { + logger_.error("Failed to create the draw-completion semaphore"); + return false; + } + } ret = esp_lcd_dpi_panel_register_event_callbacks(lcd_handles_.panel, &cbs, this); if (ret != ESP_OK) { logger_.error("Failed to register panel event callback: {}", esp_err_to_name(ret)); @@ -846,6 +855,27 @@ void M5StackTab5::apply_panel_rotation(const DisplayRotation &rotation) { #endif } +bool M5StackTab5::draw_and_wait(int x1, int y1, int x2, int y2, const void *data) { + // Caller holds panel_op_mutex_. Drain any stale completion first (defensive: + // a prior transfer that timed out could leave the binary semaphore signalled), + // issue the draw, then block until on_color_trans_done gives the semaphore. + if (draw_done_sem_ != nullptr) { + xSemaphoreTake(draw_done_sem_, 0); + } + esp_err_t err = esp_lcd_panel_draw_bitmap(lcd_handles_.panel, x1, y1, x2, y2, data); + if (err != ESP_OK) { + // Rejected (e.g. ESP_ERR_INVALID_STATE: a prior draw still in flight) or + // failed - no completion callback will arrive, so do not wait for one. + logger_.warn_rate_limited("draw_bitmap failed: {}", esp_err_to_name(err)); + return false; + } + if (draw_done_sem_ != nullptr) { + // Bounded wait so a missing completion cannot wedge the caller forever. + xSemaphoreTake(draw_done_sem_, pdMS_TO_TICKS(200)); + } + return true; +} + void M5StackTab5::write_lcd_lines(int xs, int ys, int xe, int ye, const uint8_t *data, uint32_t user_data) { (void)user_data; @@ -860,7 +890,8 @@ void M5StackTab5::write_lcd_lines(int xs, int ys, int xe, int ye, const uint8_t logger_.error("write_lcd_lines: Bad region: ({},{}) to ({},{})", xs, ys, xe, ye); return; } - esp_lcd_panel_draw_bitmap(lcd_handles_.panel, xs, ys, xe + 1, ye + 1, data); + std::lock_guard lock(panel_op_mutex_); + draw_and_wait(xs, ys, xe + 1, ye + 1, data); } void M5StackTab5::brightness(float brightness) { @@ -975,7 +1006,14 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m srm.scale_x = 1.0f; srm.scale_y = 1.0f; srm.mode = PPA_TRANS_MODE_BLOCKING; - if (ppa_do_scale_rotate_mirror(g_ppa_client, &srm) == ESP_OK) { + // Serialize the framebuffer write with draw_bitmap (write_lcd_lines / + // the non-PPA flush tail) so two engines never write the panel FB at once. + esp_err_t ppa_err; + { + std::lock_guard lock(panel_op_mutex_); + ppa_err = ppa_do_scale_rotate_mirror(g_ppa_client, &srm); + } + if (ppa_err == ESP_OK) { // The rotated pixels are already in the scanned-out framebuffer and // the PPA driver performed the cache maintenance (write-back of the // source window, invalidate of the destination window). The espp @@ -1013,10 +1051,16 @@ void M5StackTab5::flush(lv_display_t *disp, const lv_area_t *area, uint8_t *px_m px_map = reinterpret_cast(third_buffer); } - // pass the draw buffer to the DPI panel driver - esp_lcd_panel_draw_bitmap(lcd_handles_.panel, offsetx1, offsety1, offsetx2 + 1, offsety2 + 1, - px_map); - // For DPI panels, the notification will come through the callback + // Pass the draw buffer to the DPI panel driver. Serialized + synchronous (see + // draw_and_wait): only one panel transfer is ever in flight, so a concurrent + // write_lcd_lines() cannot make this draw fail. Always signal LVGL afterwards + // - even if the draw was rejected/failed - so LVGL never waits forever for a + // completion that will not come. + { + std::lock_guard lock(panel_op_mutex_); + draw_and_wait(offsetx1, offsety1, offsetx2 + 1, offsety2 + 1, px_map); + } + lv_display_flush_ready(disp); } bool IRAM_ATTR M5StackTab5::notify_lvgl_flush_ready(esp_lcd_panel_handle_t panel, @@ -1027,12 +1071,16 @@ bool IRAM_ATTR M5StackTab5::notify_lvgl_flush_ready(esp_lcd_panel_handle_t panel return false; } - // This is called from ISR context, so we need to be careful about what we do - // Just notify LVGL that the flush is ready - avoid logging or other complex operations - if (tab5->display_) { - tab5->display_->notify_flush_ready(); + // ISR context: just release the draw-completion semaphore. The draw that + // issued this transfer (flush() or write_lcd_lines(), holding + // panel_op_mutex_) is waiting on it and will signal LVGL itself. This keeps + // a direct write_lcd_lines() completion from being mistaken for an LVGL + // flush completion. + BaseType_t higher_priority_task_woken = pdFALSE; + if (tab5->draw_done_sem_ != nullptr) { + xSemaphoreGiveFromISR(tab5->draw_done_sem_, &higher_priority_task_woken); } - return false; + return higher_priority_task_woken == pdTRUE; } void M5StackTab5::dsi_write_command(uint8_t cmd, std::span params,