rgb_lcd: workaround auto-next frame bug

Closes https://github.com/espressif/esp-idf/issues/8620

components/esp_lcd/CMakeLists.txt

@@ -14,4 +14,5 @@ set(priv_requires "driver")
 
 idf_component_register(SRCS ${srcs}
                        INCLUDE_DIRS ${includes}
-                       PRIV_REQUIRES ${priv_requires})
+                       PRIV_REQUIRES ${priv_requires}
+                       LDFRAGMENTS linker.lf)

components/esp_lcd/Kconfig

@@ -6,5 +6,16 @@ menu "LCD and Touch Panel"
             help
                 LCD driver allocates an internal buffer to transform the data into a proper format, because of
                 the endian order mismatch. This option is to set the size of the buffer, in bytes.
+
+        config LCD_RGB_ISR_IRAM_SAFE
+            depends on IDF_TARGET_ESP32S3
+            bool "RGB LCD ISR IRAM-Safe"
+            default n
+            select GDMA_CTRL_FUNC_IN_IRAM # need to restart GDMA in the LCD ISR
+            help
+                Ensure the LCD interrupt is IRAM-Safe by allowing the interrupt handler to be
+                executable when the cache is disabled (e.g. SPI Flash write).
+                If you want the LCD driver to keep flushing the screen even when cache ops disabled,
+                you can enable this option. Note, this will also increase the IRAM usage.
     endmenu
 endmenu

components/esp_lcd/include/esp_lcd_panel_rgb.h

@@ -59,7 +59,7 @@ typedef struct {
     unsigned int hsync_front_porch; /*!< Horizontal front porch, number of PCLK between the end of active data and the next hsync */
     unsigned int vsync_pulse_width; /*!< Vertical sync width, unit: number of lines */
     unsigned int vsync_back_porch;  /*!< Vertical back porch, number of invalid lines between vsync and start of frame */
-    unsigned int vsync_front_porch; /*!< Vertical front porch, number of invalid lines between then end of frame and the next vsync */
+    unsigned int vsync_front_porch; /*!< Vertical front porch, number of invalid lines between the end of frame and the next vsync */
     struct {
         unsigned int hsync_idle_low: 1;  /*!< The hsync signal is low in IDLE state */
         unsigned int vsync_idle_low: 1;  /*!< The vsync signal is low in IDLE state */

components/esp_lcd/linker.lf

@@ -0,0 +1,6 @@
+[mapping:esp_lcd]
+archive: libesp_lcd.a
+entries:
+    if LCD_RGB_ISR_IRAM_SAFE = y:
+        esp_lcd_common: lcd_com_mount_dma_data (noflash)
+        esp_lcd_rgb_panel: lcd_rgb_panel_start_transmission (noflash)

components/esp_lcd/src/esp_lcd_common.h

@@ -1,13 +1,16 @@
 /*
- * SPDX-FileCopyrightText: 2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
 #pragma once
 
 #include <stddef.h>
+#include "sdkconfig.h"
 #include "soc/soc_caps.h"
 #include "hal/dma_types.h"
+#include "esp_intr_alloc.h"
+#include "esp_heap_caps.h"
 #if SOC_LCDCAM_SUPPORTED
 #include "hal/lcd_hal.h"
 #endif
@@ -16,6 +19,9 @@
 extern "C" {
 #endif
 
+#define LCD_I80_INTR_ALLOC_FLAGS     ESP_INTR_FLAG_INTRDISABLED
+#define LCD_I80_MEM_ALLOC_CAPS       MALLOC_CAP_DEFAULT
+
 #define LCD_PERIPH_CLOCK_PRE_SCALE (2) // This is the minimum divider that can be applied to LCD peripheral
 
 #if SOC_LCDCAM_SUPPORTED

components/esp_lcd/src/esp_lcd_rgb_panel.c

@@ -16,8 +16,6 @@
 #include "freertos/semphr.h"
 #include "esp_attr.h"
 #include "esp_check.h"
-#include "esp_intr_alloc.h"
-#include "esp_heap_caps.h"
 #include "esp_pm.h"
 #include "esp_lcd_panel_interface.h"
 #include "esp_lcd_panel_rgb.h"
@@ -39,6 +37,12 @@
 #include "hal/lcd_hal.h"
 #include "hal/lcd_ll.h"
 
+#if CONFIG_LCD_RGB_ISR_IRAM_SAFE
+#define LCD_RGB_INTR_ALLOC_FLAGS     (ESP_INTR_FLAG_IRAM | ESP_INTR_FLAG_INTRDISABLED)
+#else
+#define LCD_RGB_INTR_ALLOC_FLAGS     ESP_INTR_FLAG_INTRDISABLED
+#endif
+
 static const char *TAG = "lcd_panel.rgb";
 
 typedef struct esp_rgb_panel_t esp_rgb_panel_t;
@@ -58,7 +62,8 @@ static esp_err_t rgb_panel_disp_off(esp_lcd_panel_t *panel, bool off);
 static esp_err_t lcd_rgb_panel_select_periph_clock(esp_rgb_panel_t *panel, lcd_clock_source_t clk_src);
 static esp_err_t lcd_rgb_panel_create_trans_link(esp_rgb_panel_t *panel);
 static esp_err_t lcd_rgb_panel_configure_gpio(esp_rgb_panel_t *panel, const esp_lcd_rgb_panel_config_t *panel_config);
-static IRAM_ATTR void lcd_default_isr_handler(void *args);
+static void lcd_rgb_panel_start_transmission(esp_rgb_panel_t *rgb_panel);
+static void lcd_default_isr_handler(void *args);
 
 struct esp_rgb_panel_t {
     esp_lcd_panel_t base;  // Base class of generic lcd panel
@@ -77,9 +82,6 @@ struct esp_rgb_panel_t {
     size_t resolution_hz;    // Peripheral clock resolution
     esp_lcd_rgb_timing_t timings;   // RGB timing parameters (e.g. pclk, sync pulse, porch width)
     gdma_channel_handle_t dma_chan; // DMA channel handle
-    int new_frame_id;               // ID for new frame, we use ID to identify whether the frame content has been updated
-    int cur_frame_id;               // ID for current transferring frame
-    SemaphoreHandle_t done_sem;     // Binary semaphore, indicating if the new frame has been flushed to LCD
     esp_lcd_rgb_panel_frame_trans_done_cb_t on_frame_trans_done; // Callback, invoked after frame trans done
     void *user_ctx;                // Reserved user's data of callback functions
     int x_gap;                      // Extra gap in x coordinate, it's used when calculate the flush window
@@ -87,7 +89,6 @@ struct esp_rgb_panel_t {
     struct {
         unsigned int disp_en_level: 1; // The level which can turn on the screen by `disp_gpio_num`
         unsigned int stream_mode: 1;   // If set, the LCD transfers data continuously, otherwise, it stops refreshing the LCD when transaction done
-        unsigned int new_frame: 1;     // Whether the frame we're going to flush is a new one
         unsigned int fb_in_psram: 1;   // Whether the frame buffer is in PSRAM
     } flags;
     dma_descriptor_t dma_nodes[]; // DMA descriptor pool of size `num_dma_nodes`
@@ -100,6 +101,16 @@ esp_err_t esp_lcd_new_rgb_panel(const esp_lcd_rgb_panel_config_t *rgb_panel_conf
     ESP_GOTO_ON_FALSE(rgb_panel_config && ret_panel, ESP_ERR_INVALID_ARG, err, TAG, "invalid parameter");
     ESP_GOTO_ON_FALSE(rgb_panel_config->data_width == 16, ESP_ERR_NOT_SUPPORTED, err, TAG,
                       "unsupported data width %d", rgb_panel_config->data_width);
+
+#if CONFIG_LCD_RGB_ISR_IRAM_SAFE
+    if (rgb_panel_config->on_frame_trans_done) {
+        ESP_RETURN_ON_FALSE(esp_ptr_in_iram(rgb_panel_config->on_frame_trans_done), ESP_ERR_INVALID_ARG, TAG, "on_frame_trans_done callback not in IRAM");
+    }
+    if (rgb_panel_config->user_ctx) {
+        ESP_RETURN_ON_FALSE(esp_ptr_internal(rgb_panel_config->user_ctx), ESP_ERR_INVALID_ARG, TAG, "user context not in internal RAM");
+    }
+#endif
+
     // calculate the number of DMA descriptors
     size_t fb_size = rgb_panel_config->timings.h_res * rgb_panel_config->timings.v_res * rgb_panel_config->data_width / 8;
     size_t num_dma_nodes = fb_size / DMA_DESCRIPTOR_BUFFER_MAX_SIZE;
@@ -107,7 +118,7 @@ esp_err_t esp_lcd_new_rgb_panel(const esp_lcd_rgb_panel_config_t *rgb_panel_conf
         num_dma_nodes++;
     }
     // DMA descriptors must be placed in internal SRAM (requested by DMA)
-    rgb_panel = heap_caps_calloc(1, sizeof(esp_rgb_panel_t) + num_dma_nodes * sizeof(dma_descriptor_t), MALLOC_CAP_DMA);
+    rgb_panel = heap_caps_calloc(1, sizeof(esp_rgb_panel_t) + num_dma_nodes * sizeof(dma_descriptor_t), MALLOC_CAP_DMA | MALLOC_CAP_INTERNAL);
     ESP_GOTO_ON_FALSE(rgb_panel, ESP_ERR_NO_MEM, err, TAG, "no mem for rgb panel");
     rgb_panel->num_dma_nodes = num_dma_nodes;
     rgb_panel->panel_id = -1;
@@ -117,6 +128,7 @@ esp_err_t esp_lcd_new_rgb_panel(const esp_lcd_rgb_panel_config_t *rgb_panel_conf
     rgb_panel->panel_id = panel_id;
     // enable APB to access LCD registers
     periph_module_enable(lcd_periph_signals.panels[panel_id].module);
+    periph_module_reset(lcd_periph_signals.panels[panel_id].module);
     // alloc frame buffer
     bool alloc_from_psram = false;
     // fb_in_psram is only an option, if there's no PSRAM on board, we still alloc from SRAM
@@ -140,17 +152,13 @@ esp_err_t esp_lcd_new_rgb_panel(const esp_lcd_rgb_panel_config_t *rgb_panel_conf
     rgb_panel->sram_trans_align = sram_trans_align;
     rgb_panel->fb_size = fb_size;
     rgb_panel->flags.fb_in_psram = alloc_from_psram;
-    // semaphore indicates new frame trans done
-    rgb_panel->done_sem = xSemaphoreCreateBinary();
-    ESP_GOTO_ON_FALSE(rgb_panel->done_sem, ESP_ERR_NO_MEM, err, TAG, "create done sem failed");
-    xSemaphoreGive(rgb_panel->done_sem); // initialize the semaphore count to 1
     // initialize HAL layer, so we can call LL APIs later
     lcd_hal_init(&rgb_panel->hal, panel_id);
     // set peripheral clock resolution
     ret = lcd_rgb_panel_select_periph_clock(rgb_panel, rgb_panel_config->clk_src);
     ESP_GOTO_ON_ERROR(ret, err, TAG, "select periph clock failed");
     // install interrupt service, (LCD peripheral shares the interrupt source with Camera by different mask)
-    int isr_flags = ESP_INTR_FLAG_SHARED;
+    int isr_flags = LCD_RGB_INTR_ALLOC_FLAGS | ESP_INTR_FLAG_SHARED;
     ret = esp_intr_alloc_intrstatus(lcd_periph_signals.panels[panel_id].irq_id, isr_flags,
                                     (uint32_t)lcd_ll_get_interrupt_status_reg(rgb_panel->hal.dev),
                                     LCD_LL_EVENT_VSYNC_END, lcd_default_isr_handler, rgb_panel, &rgb_panel->intr);
@@ -184,7 +192,7 @@ esp_err_t esp_lcd_new_rgb_panel(const esp_lcd_rgb_panel_config_t *rgb_panel_conf
     rgb_panel->base.set_gap = rgb_panel_set_gap;
     // return base class
     *ret_panel = &(rgb_panel->base);
-    ESP_LOGD(TAG, "new rgb panel(%d) @%p, fb_size=%zu", rgb_panel->panel_id, rgb_panel, rgb_panel->fb_size);
+    ESP_LOGD(TAG, "new rgb panel(%d) @%p, fb @%p, size=%zu", rgb_panel->panel_id, rgb_panel, rgb_panel->fb, rgb_panel->fb_size);
     return ESP_OK;
 
 err:
@@ -196,9 +204,6 @@ err:
         if (rgb_panel->fb) {
             free(rgb_panel->fb);
         }
-        if (rgb_panel->done_sem) {
-            vSemaphoreDelete(rgb_panel->done_sem);
-        }
         if (rgb_panel->dma_chan) {
             gdma_disconnect(rgb_panel->dma_chan);
             gdma_del_channel(rgb_panel->dma_chan);
@@ -218,14 +223,12 @@ err:
 static esp_err_t rgb_panel_del(esp_lcd_panel_t *panel)
 {
     esp_rgb_panel_t *rgb_panel = __containerof(panel, esp_rgb_panel_t, base);
-    xSemaphoreTake(rgb_panel->done_sem, portMAX_DELAY); // wait for last flush done
     int panel_id = rgb_panel->panel_id;
     gdma_disconnect(rgb_panel->dma_chan);
     gdma_del_channel(rgb_panel->dma_chan);
     esp_intr_free(rgb_panel->intr);
     periph_module_disable(lcd_periph_signals.panels[panel_id].module);
     lcd_com_remove_device(LCD_COM_DEVICE_TYPE_RGB, rgb_panel->panel_id);
-    vSemaphoreDelete(rgb_panel->done_sem);
     free(rgb_panel->fb);
     if (rgb_panel->pm_lock) {
         esp_pm_lock_release(rgb_panel->pm_lock);
@@ -280,10 +283,16 @@ static esp_err_t rgb_panel_init(esp_lcd_panel_t *panel)
     lcd_ll_enable_output_hsync_in_porch_region(rgb_panel->hal.dev, true);
     // generate the hsync at the very begining of line
     lcd_ll_set_hsync_position(rgb_panel->hal.dev, 0);
-    // starting sending next frame automatically
+    // restart flush by hardware has some limitation, instead, the driver will restart the flush in the VSYNC end interrupt by software
-    lcd_ll_enable_auto_next_frame(rgb_panel->hal.dev, rgb_panel->flags.stream_mode);
+    lcd_ll_enable_auto_next_frame(rgb_panel->hal.dev, false);
     // trigger interrupt on the end of frame
     lcd_ll_enable_interrupt(rgb_panel->hal.dev, LCD_LL_EVENT_VSYNC_END, true);
+    // enable intr
+    esp_intr_enable(rgb_panel->intr);
+    // start transmission
+    if (rgb_panel->flags.stream_mode) {
+        lcd_rgb_panel_start_transmission(rgb_panel);
+    }
     ESP_LOGD(TAG, "rgb panel(%d) start, pclk=%uHz", rgb_panel->panel_id, rgb_panel->timings.pclk_hz);
 err:
     return ret;
@@ -303,7 +312,7 @@ static esp_err_t rgb_panel_draw_bitmap(esp_lcd_panel_t *panel, int x_start, int
     x_end = MIN(x_end, rgb_panel->timings.h_res);
     y_start = MIN(y_start, rgb_panel->timings.v_res);
     y_end = MIN(y_end, rgb_panel->timings.v_res);
-    xSemaphoreTake(rgb_panel->done_sem, portMAX_DELAY); // wait for last transaction done
+
     // convert the frame buffer to 3D array
     int bytes_per_pixel = rgb_panel->data_width / 8;
     int pixels_per_line = rgb_panel->timings.h_res;
@@ -321,20 +330,12 @@ static esp_err_t rgb_panel_draw_bitmap(esp_lcd_panel_t *panel, int x_start, int
         // CPU writes data to PSRAM through DCache, data in PSRAM might not get updated, so write back
         Cache_WriteBack_Addr((uint32_t)&to[y_start][0][0], (y_end - y_start) * rgb_panel->timings.h_res * bytes_per_pixel);
     }
-    // we don't care the exact frame ID, as long as it's different from the previous one
+
-    rgb_panel->new_frame_id++;
+    // restart the new transmission
     if (!rgb_panel->flags.stream_mode) {
-        // in one-off mode, the "new frame" flag is controlled by this API
+        lcd_rgb_panel_start_transmission(rgb_panel);
-        rgb_panel->cur_frame_id = rgb_panel->new_frame_id;
-        rgb_panel->flags.new_frame = 1;
-        // reset FIFO of DMA and LCD, incase there remains old frame data
-        gdma_reset(rgb_panel->dma_chan);
-        lcd_ll_stop(rgb_panel->hal.dev);
-        lcd_ll_fifo_reset(rgb_panel->hal.dev);
-        gdma_start(rgb_panel->dma_chan, (intptr_t)rgb_panel->dma_nodes);
     }
-    // start LCD engine
+
-    lcd_ll_start(rgb_panel->hal.dev);
     return ESP_OK;
 }
 
@@ -439,7 +440,8 @@ static esp_err_t lcd_rgb_panel_configure_gpio(esp_rgb_panel_t *panel, const esp_
 static esp_err_t lcd_rgb_panel_select_periph_clock(esp_rgb_panel_t *panel, lcd_clock_source_t clk_src)
 {
     esp_err_t ret = ESP_OK;
-    lcd_ll_set_group_clock_src(panel->hal.dev, clk_src, LCD_PERIPH_CLOCK_PRE_SCALE, 1, 0);
+    // force to use integer division, as fractional division might lead to clock jitter
+    lcd_ll_set_group_clock_src(panel->hal.dev, clk_src, LCD_PERIPH_CLOCK_PRE_SCALE, 0, 0);
     switch (clk_src) {
     case LCD_CLK_SRC_PLL160M:
         panel->resolution_hz = 160000000 / LCD_PERIPH_CLOCK_PRE_SCALE;
@@ -469,12 +471,8 @@ static esp_err_t lcd_rgb_panel_create_trans_link(esp_rgb_panel_t *panel)
         panel->dma_nodes[i].dw0.owner = DMA_DESCRIPTOR_BUFFER_OWNER_CPU;
         panel->dma_nodes[i].next = &panel->dma_nodes[i + 1];
     }
-    // fix the last DMA descriptor according to whether the LCD works in stream mode
+    // one-off DMA chain
-    if (panel->flags.stream_mode) {
+    panel->dma_nodes[panel->num_dma_nodes - 1].next = NULL;
-        panel->dma_nodes[panel->num_dma_nodes - 1].next = &panel->dma_nodes[0]; // chain into a circle
-    } else {
-        panel->dma_nodes[panel->num_dma_nodes - 1].next = NULL; // one-off DMA chain
-    }
     // mount the frame buffer to the DMA descriptors
     lcd_com_mount_dma_data(panel->dma_nodes, panel->fb, panel->fb_size);
     // alloc DMA channel and connect to LCD peripheral
@@ -496,32 +494,37 @@ err:
     return ret;
 }
 
+static void lcd_rgb_panel_start_transmission(esp_rgb_panel_t *rgb_panel)
+{
+    // reset FIFO of DMA and LCD, incase there remains old frame data
+    gdma_reset(rgb_panel->dma_chan);
+    lcd_ll_stop(rgb_panel->hal.dev);
+    lcd_ll_fifo_reset(rgb_panel->hal.dev);
+    gdma_start(rgb_panel->dma_chan, (intptr_t)rgb_panel->dma_nodes);
+    // delay 1us is sufficient for DMA to pass data to LCD FIFO
+    // in fact, this is only needed when LCD pixel clock is set too high
+    esp_rom_delay_us(1);
+    // start LCD engine
+    lcd_ll_start(rgb_panel->hal.dev);
+}
+
 IRAM_ATTR static void lcd_default_isr_handler(void *args)
 {
-    esp_rgb_panel_t *panel = (esp_rgb_panel_t *)args;
+    esp_rgb_panel_t *rgb_panel = (esp_rgb_panel_t *)args;
     bool need_yield = false;
-    BaseType_t high_task_woken = pdFALSE;
 
-    uint32_t intr_status = lcd_ll_get_interrupt_status(panel->hal.dev);
+    uint32_t intr_status = lcd_ll_get_interrupt_status(rgb_panel->hal.dev);
-    lcd_ll_clear_interrupt_status(panel->hal.dev, intr_status);
+    lcd_ll_clear_interrupt_status(rgb_panel->hal.dev, intr_status);
     if (intr_status & LCD_LL_EVENT_VSYNC_END) {
-        if (panel->flags.new_frame) { // the finished one is a new frame
+        // call user registered callback
-            if (panel->on_frame_trans_done) {
+        if (rgb_panel->on_frame_trans_done) {
-                if (panel->on_frame_trans_done(&panel->base, NULL, panel->user_ctx)) {
+            if (rgb_panel->on_frame_trans_done(&rgb_panel->base, NULL, rgb_panel->user_ctx)) {
-                    need_yield = true;
-                }
-            }
-            xSemaphoreGiveFromISR(panel->done_sem, &high_task_woken);
-            if (high_task_woken == pdTRUE) {
                 need_yield = true;
             }
         }
-        // in stream mode, the "new frame" flag is controlled by comparing "new frame id" and "cur frame id"
+        // to restart the transmission
-        if (panel->flags.stream_mode) {
+        if (rgb_panel->flags.stream_mode) {
-            // new_frame_id is only modified in `rgb_panel_draw_bitmap()`, fetch first and use below to avoid inconsistent
+            lcd_rgb_panel_start_transmission(rgb_panel);
-            int new_frame_id = panel->new_frame_id;
-            panel->flags.new_frame = (panel->cur_frame_id != new_frame_id);
-            panel->cur_frame_id = new_frame_id;
         }
     }
     if (need_yield) {

components/esp_lcd/test/test_rgb_panel.c

@@ -5,6 +5,7 @@
 #include "esp_lcd_panel_rgb.h"
 #include "esp_lcd_panel_ops.h"
 #include "soc/soc_caps.h"
+#include "esp_attr.h"
 
 #define TEST_LCD_H_RES         (480)
 #define TEST_LCD_V_RES         (272)
@@ -29,20 +30,23 @@
 #define TEST_LCD_DATA13_GPIO   (16) // R2
 #define TEST_LCD_DATA14_GPIO   (17) // R3
 #define TEST_LCD_DATA15_GPIO   (18) // R4
-#define TEST_LCD_DISP_EN_GPIO  (39)
+#define TEST_LCD_DISP_EN_GPIO  (-1)
+
+#define TEST_LCD_PIXEL_CLOCK_HZ (10 * 1000 * 1000)
 
 #if SOC_LCD_RGB_SUPPORTED
 // RGB driver consumes a huge memory to save frame buffer, only test it with PSRAM enabled
 #if CONFIG_SPIRAM_USE_MALLOC
-TEST_CASE("lcd rgb lcd panel", "[lcd]")
-{
-#define TEST_IMG_SIZE (100 * 100 * sizeof(uint16_t))
-    uint8_t *img = malloc(TEST_IMG_SIZE);
-    TEST_ASSERT_NOT_NULL(img);
 
+#define TEST_IMG_SIZE (100 * 100 * sizeof(uint16_t))
+
+static esp_lcd_panel_handle_t test_rgb_panel_initialization(bool stream_mode, esp_lcd_rgb_panel_frame_trans_done_cb_t cb, void *user_data)
+{
     esp_lcd_panel_handle_t panel_handle = NULL;
     esp_lcd_rgb_panel_config_t panel_config = {
         .data_width = 16,
+        .psram_trans_align = 64,
+        .clk_src = LCD_CLK_SRC_PLL160M,
         .disp_gpio_num = TEST_LCD_DISP_EN_GPIO,
         .pclk_gpio_num = TEST_LCD_PCLK_GPIO,
         .vsync_gpio_num = TEST_LCD_VSYNC_GPIO,
@@ -67,37 +71,79 @@ TEST_CASE("lcd rgb lcd panel", "[lcd]")
             TEST_LCD_DATA15_GPIO,
         },
         .timings = {
-            .pclk_hz = 12000000,
+            .pclk_hz = TEST_LCD_PIXEL_CLOCK_HZ,
             .h_res = TEST_LCD_H_RES,
             .v_res = TEST_LCD_V_RES,
-            .hsync_back_porch = 43,
+            .hsync_back_porch = 68,
-            .hsync_front_porch = 2,
+            .hsync_front_porch = 20,
-            .hsync_pulse_width = 1,
+            .hsync_pulse_width = 5,
-            .vsync_back_porch = 12,
+            .vsync_back_porch = 18,
-            .vsync_front_porch = 1,
+            .vsync_front_porch = 4,
             .vsync_pulse_width = 1,
         },
-        .flags.fb_in_psram = 1,
+        .on_frame_trans_done = cb,
+        .user_ctx = user_data,
+        .flags.fb_in_psram = 1, // allocate frame buffer in PSRAM
+        .flags.relax_on_idle = !stream_mode,
     };
-    // Test stream mode and one-off mode
-    for (int i = 0; i < 2; i++) {
-        panel_config.flags.relax_on_idle = i;
-        TEST_ESP_OK(esp_lcd_new_rgb_panel(&panel_config, &panel_handle));
-        TEST_ESP_OK(esp_lcd_panel_reset(panel_handle));
-        TEST_ESP_OK(esp_lcd_panel_init(panel_handle));
 
-        for (int i = 0; i < 200; i++) {
+    TEST_ESP_OK(esp_lcd_new_rgb_panel(&panel_config, &panel_handle));
-            uint8_t color_byte = esp_random() & 0xFF;
+    TEST_ESP_OK(esp_lcd_panel_reset(panel_handle));
-            int x_start = esp_random() % (TEST_LCD_H_RES - 100);
+    TEST_ESP_OK(esp_lcd_panel_init(panel_handle));
-            int y_start = esp_random() % (TEST_LCD_V_RES - 100);
-            memset(img, color_byte, TEST_IMG_SIZE);
-            esp_lcd_panel_draw_bitmap(panel_handle, x_start, y_start, x_start + 100, y_start + 100, img);
-        }
 
-        TEST_ESP_OK(esp_lcd_panel_del(panel_handle));
+    return panel_handle;
+}
+
+TEST_CASE("lcd_rgb_panel_stream_mode", "[lcd]")
+{
+    uint8_t *img = malloc(TEST_IMG_SIZE);
+    TEST_ASSERT_NOT_NULL(img);
+
+    printf("initialize RGB panel with stream mode\r\n");
+    esp_lcd_panel_handle_t panel_handle = test_rgb_panel_initialization(true, NULL, NULL);
+    printf("flush random color block\r\n");
+    for (int i = 0; i < 200; i++) {
+        uint8_t color_byte = esp_random() & 0xFF;
+        int x_start = esp_random() % (TEST_LCD_H_RES - 100);
+        int y_start = esp_random() % (TEST_LCD_V_RES - 100);
+        memset(img, color_byte, TEST_IMG_SIZE);
+        esp_lcd_panel_draw_bitmap(panel_handle, x_start, y_start, x_start + 100, y_start + 100, img);
     }
+    printf("delete RGB panel\r\n");
+    TEST_ESP_OK(esp_lcd_panel_del(panel_handle));
+    free(img);
+}
+
+static IRAM_ATTR bool test_rgb_panel_trans_done(esp_lcd_panel_handle_t panel, esp_lcd_rgb_panel_event_data_t *edata, void *user_ctx)
+{
+    TaskHandle_t task_to_notify = (TaskHandle_t)user_ctx;
+    BaseType_t high_task_wakeup;
+    vTaskNotifyGiveFromISR(task_to_notify, &high_task_wakeup);
+    return high_task_wakeup == pdTRUE;
+}
+
+TEST_CASE("lcd_rgb_panel_one_shot_mode", "[lcd]")
+{
+    uint8_t *img = malloc(TEST_IMG_SIZE);
+    TEST_ASSERT_NOT_NULL(img);
+    TaskHandle_t cur_task = xTaskGetCurrentTaskHandle();
+
+    printf("initialize RGB panel with ont-shot mode\r\n");
+    esp_lcd_panel_handle_t panel_handle = test_rgb_panel_initialization(false, test_rgb_panel_trans_done, cur_task);
+    printf("flush random color block\r\n");
+    for (int i = 0; i < 200; i++) {
+        uint8_t color_byte = esp_random() & 0xFF;
+        int x_start = esp_random() % (TEST_LCD_H_RES - 100);
+        int y_start = esp_random() % (TEST_LCD_V_RES - 100);
+        memset(img, color_byte, TEST_IMG_SIZE);
+        esp_lcd_panel_draw_bitmap(panel_handle, x_start, y_start, x_start + 100, y_start + 100, img);
+        // wait for flush done
+        TEST_ASSERT_NOT_EQUAL(0, ulTaskNotifyTake(pdFALSE, pdMS_TO_TICKS(1000)));
+    }
+
+    printf("delete RGB panel\r\n");
+    TEST_ESP_OK(esp_lcd_panel_del(panel_handle));
     free(img);
-#undef TEST_IMG_SIZE
 }
 
 // The following test shows a porting example of LVGL GUI library

components/hal/esp32s3/include/hal/lcd_ll.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -34,7 +34,11 @@ static inline void lcd_ll_enable_clock(lcd_cam_dev_t *dev, bool en)
 static inline void lcd_ll_set_group_clock_src(lcd_cam_dev_t *dev, lcd_clock_source_t src, int div_num, int div_a, int div_b)
 {
     // lcd_clk = module_clock_src / (div_num + div_b / div_a)
-    HAL_ASSERT(div_num >= 2);
+    HAL_ASSERT(div_num >= 2 && div_num <= 256);
+    // dic_num == 0 means 256 divider in hardware
+    if (div_num >= 256) {
+        div_num = 0;
+    }
     HAL_FORCE_MODIFY_U32_REG_FIELD(dev->lcd_clock, lcd_clkm_div_num, div_num);
     dev->lcd_clock.lcd_clkm_div_a = div_a;
     dev->lcd_clock.lcd_clkm_div_b = div_b;
@@ -42,10 +46,15 @@ static inline void lcd_ll_set_group_clock_src(lcd_cam_dev_t *dev, lcd_clock_sour
     case LCD_CLK_SRC_PLL160M:
         dev->lcd_clock.lcd_clk_sel = 3;
         break;
+    case LCD_CLK_SRC_PLL240M:
+        dev->lcd_clock.lcd_clk_sel = 2;
+        break;
     case LCD_CLK_SRC_XTAL:
         dev->lcd_clock.lcd_clk_sel = 1;
         break;
     default:
+        // disble LCD clock source
+        dev->lcd_clock.lcd_clk_sel = 0;
         HAL_ASSERT(false && "unsupported clock source");
         break;
     }
@@ -60,7 +69,6 @@ static inline void lcd_ll_set_clock_idle_level(lcd_cam_dev_t *dev, bool level)
 __attribute__((always_inline))
 static inline void lcd_ll_set_pixel_clock_edge(lcd_cam_dev_t *dev, bool active_on_neg)
 {
-    dev->lcd_clock.lcd_clk_equ_sysclk = 0; // if we want to pixel_clk == lcd_clk, just make clkcnt = 0
     dev->lcd_clock.lcd_ck_out_edge = active_on_neg;
 }
 
@@ -68,7 +76,15 @@ __attribute__((always_inline))
 static inline void lcd_ll_set_pixel_clock_prescale(lcd_cam_dev_t *dev, uint32_t prescale)
 {
     // Formula: pixel_clk = lcd_clk / (1 + clkcnt_n)
-    dev->lcd_clock.lcd_clkcnt_n = prescale - 1;
+    // clkcnt_n can't be zero
+    uint32_t scale = 1;
+    if (prescale == 1) {
+        dev->lcd_clock.lcd_clk_equ_sysclk = 1;
+    } else {
+        dev->lcd_clock.lcd_clk_equ_sysclk = 0;
+        scale = prescale - 1;
+    }
+    dev->lcd_clock.lcd_clkcnt_n = scale;
 }
 
 static inline void lcd_ll_enable_rgb_yuv_convert(lcd_cam_dev_t *dev, bool en)
@@ -97,14 +113,10 @@ static inline void lcd_ll_set_blank_cycles(lcd_cam_dev_t *dev, uint32_t fk_cycle
 
 static inline void lcd_ll_set_data_width(lcd_cam_dev_t *dev, uint32_t width)
 {
+    HAL_ASSERT(width == 8 || width == 16);
     dev->lcd_user.lcd_2byte_en = (width == 16);
 }
 
-static inline uint32_t lcd_ll_get_data_width(lcd_cam_dev_t *dev)
-{
-    return dev->lcd_user.lcd_2byte_en ? 16 : 8;
-}
-
 static inline void lcd_ll_enable_output_always_on(lcd_cam_dev_t *dev, bool en)
 {
     dev->lcd_user.lcd_always_out_en = en;
@@ -117,6 +129,7 @@ static inline void lcd_ll_start(lcd_cam_dev_t *dev)
     dev->lcd_user.lcd_start = 1;
 }
 
+__attribute__((always_inline))
 static inline void lcd_ll_stop(lcd_cam_dev_t *dev)
 {
     dev->lcd_user.lcd_start = 0;
@@ -125,33 +138,35 @@ static inline void lcd_ll_stop(lcd_cam_dev_t *dev)
 
 static inline void lcd_ll_reset(lcd_cam_dev_t *dev)
 {
-    dev->lcd_user.lcd_reset = 1;
+    dev->lcd_user.lcd_reset = 1; // self clear
-    dev->lcd_user.lcd_reset = 0;
 }
 
 __attribute__((always_inline))
-static inline void lcd_ll_reverse_data_bit_order(lcd_cam_dev_t *dev, bool en)
+static inline void lcd_ll_reverse_bit_order(lcd_cam_dev_t *dev, bool en)
 {
     // whether to change LCD_DATA_out[N:0] to LCD_DATA_out[0:N]
     dev->lcd_user.lcd_bit_order = en;
 }
 
 __attribute__((always_inline))
-static inline void lcd_ll_reverse_data_byte_order(lcd_cam_dev_t *dev, bool en)
+static inline void lcd_ll_swap_byte_order(lcd_cam_dev_t *dev, uint32_t width, bool en)
 {
-    dev->lcd_user.lcd_byte_order = en;
+    HAL_ASSERT(width == 8 || width == 16);
+    if (width == 8) {
+        // {B0}{B1}{B2}{B3} => {B1}{B0}{B3}{B2}
+        dev->lcd_user.lcd_8bits_order = en;
+        dev->lcd_user.lcd_byte_order = 0;
+    } else if (width == 16) {
+        // {B1,B0},{B3,B2} => {B0,B1}{B2,B3}
+        dev->lcd_user.lcd_byte_order = en;
+        dev->lcd_user.lcd_8bits_order = 0;
+    }
 }
 
 __attribute__((always_inline))
-static inline void lcd_ll_reverse_data_8bits_order(lcd_cam_dev_t *dev, bool en)
-{
-    dev->lcd_user.lcd_8bits_order = en;
-}
-
 static inline void lcd_ll_fifo_reset(lcd_cam_dev_t *dev)
 {
-    dev->lcd_misc.lcd_afifo_reset = 1;
+    dev->lcd_misc.lcd_afifo_reset = 1; // self clear
-    dev->lcd_misc.lcd_afifo_reset = 0;
 }
 
 __attribute__((always_inline))
@@ -171,6 +186,7 @@ static inline void lcd_ll_set_dc_delay_ticks(lcd_cam_dev_t *dev, uint32_t delay)
 __attribute__((always_inline))
 static inline void lcd_ll_set_command(lcd_cam_dev_t *dev, uint32_t data_width, uint32_t command)
 {
+    HAL_ASSERT(data_width == 8 || data_width == 16);
     // if command phase has two cycles, in the first cycle, command[15:0] is sent out via lcd_data_out[15:0]
     // in the second cycle, command[31:16] is sent out via lcd_data_out[15:0]
     if (data_width == 8) {

components/hal/include/hal/lcd_types.h

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2021 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2021-2022 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -19,6 +19,8 @@ extern "C" {
  * +=====================+=========================+============================+
  * | LCD_CLK_SRC_PLL160M | High resolution         | ESP_PM_APB_FREQ_MAX lock   |
  * +---------------------+-------------------------+----------------------------+
+ * | LCD_CLK_SRC_PLL240M | High resolution         | ESP_PM_APB_FREQ_MAX lock   |
+ * +---------------------+-------------------------+----------------------------+
  * | LCD_CLK_SRC_APLL    | Configurable resolution | ESP_PM_NO_LIGHT_SLEEP lock |
  * +---------------------+-------------------------+----------------------------+
  * | LCD_CLK_SRC_XTAL    | Medium resolution       | No PM lock                 |
@@ -27,6 +29,7 @@ extern "C" {
  */
 typedef enum {
     LCD_CLK_SRC_PLL160M, /*!< Select PLL160M as the source clock */
+    LCD_CLK_SRC_PLL240M, /*!< Select PLL240M as the source clock */
     LCD_CLK_SRC_APLL,    /*!< Select APLL as the source clock */
     LCD_CLK_SRC_XTAL,    /*!< Select XTAL as the source clock */
 } lcd_clock_source_t;