espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.81k stars 7.32k forks source link

esp_lcd RGB driver callback on_color_trans_done is not going to work right, (IDFGH-14039) #14860

Open kdschlosser opened 1 week ago

kdschlosser commented 1 week ago

Answers checklist.

IDF version.

current master branch

Espressif SoC revision.

ESP32-S3

Operating System used.

Linux

How did you build your project?

Command line with CMake

If you are using Windows, please specify command line type.

None

Development Kit.

N/A

Power Supply used.

USB

What is the expected behavior?

for the callback to be called after the buffer has been written to the display

What is the actual behavior?

it is being called before the buffer has been written to the display

Steps to reproduce.

OK so the purpose of that callback is to signal when the buffer has been flushed to the display. calling the callback prior to it actually being sent is NOT going to work and you will end up with tearing.

so if you register an on_vsync callback and also register the on_color_trans_done callback and you time how long it takes between each of those callbacks being made you will find that it is actually a fairly substantial amount of time.

so say you are using double buffering and the buffers are located in DMA RAM. That would mean that while one buffer is being transmitted the other buffer is going to be transmitting. If the rgb_panel_draw_bitmap function gets called just as the sending buffer starts it's transmit and say the display resolution is 800 x 480 x 24bit. that is going to take a while to transmit. The only work performed by the rgb_panel_draw_bitmap could be simply changing the current buffer index and not much more. That is going to finish really fast. Way faster than the buffer that is sending is going to be able to finish in. The purpose to the on_color_trans_done callback is to be able to signal that the buffer has finished transferring not before it has even started. So what ends up happening is the GUI code gets signaled that it is OK to write to the next buffer when it really isn't because this buffer can still be in the process of being written to the display. If the GUI code renders data to the buffer that is transmitting it is going to cause issues.

Debug Logs.

No response

More Information.

to make it work properly the following would need to be done.

to make it work properly 2 fields would need to be added to esp_rgb_panel_t

uint8_t real_fb_index;
uint8_t trans_done_index;

2 lines need to be added to the esp_lcd_new_rgb_panel function

rgb_panel->real_fb_index = 255;
rgb_panel->trans_done_index = 255;

the use of on_color_trans_done in the rgb_panel_draw_bitmap function would need to be removed

and the the rgb_lcd_default_isr_handler ISR handler needs to be written like this.

IRAM_ATTR static void rgb_lcd_default_isr_handler(void *args)
{
    esp_rgb_panel_t *rgb_panel = (esp_rgb_panel_t *)args;
    bool need_yield = false;

    // clear the interrupt status
    uint32_t intr_status = lcd_ll_get_interrupt_status(rgb_panel->hal.dev);
    lcd_ll_clear_interrupt_status(rgb_panel->hal.dev, intr_status);

    // VSYNC event happened
    if (intr_status & LCD_LL_EVENT_VSYNC_END) {
        if (rgb_panel->real_fb_index == 255) {
             rgb_panel->real_fb_index = rgb_panel->cur_fb_index;
        }
        if (rgb_panel->trans_done_index != rgb_panel->real_fb_index) {
            rgb_panel->trans_done_index = rgb_panel->real_fb_index;

            if (rgb_panel->on_color_trans_done) {
                if (rgb_panel->on_color_trans_done(&rgb_panel->base, NULL, rgb_panel->user_ctx)) {
                    need_yield = true;
                }
            }
        }
        // call user registered callback
        if (rgb_panel->on_vsync) {
            if (rgb_panel->on_vsync(&rgb_panel->base, NULL, rgb_panel->user_ctx)) {
                need_yield = true;
            }
        }

        if (rgb_panel->real_fb_index != rgb_panel->cur_fb_index) {
            rgb_panel->real_fb_index = rgb_panel->cur_fb_index;
        }
        // check whether to update the PCLK frequency, it should be safe to update the PCLK frequency in the VSYNC interrupt
        lcd_rgb_panel_try_update_pclk(rgb_panel);

        if (rgb_panel->flags.stream_mode) {
            // check whether to restart the transmission
            lcd_rgb_panel_try_restart_transmission(rgb_panel);
        }

    }
    // yield if needed
    if (need_yield) {
        portYIELD_FROM_ISR();
    }
}

The assumption I am making here is there is no way the current index would change after a vsync has finished and before the next vsync starts to run. I find it highly unlikely that this would happen but maybe it could? if it could then a mutex would be needed to guard against the current frame buffer index changing between vertical syncing of the display. I imagine the amount of time from when one vsync ends and the next starts would be incredibly small so if the application needs to be stalled the amount of time it would be stalled for would be really tiny.

The reason for the real_fb_index is that would hold the index of the frame buffer that is actually being sent instead of the index of the frame buffer that is next to be sent. because the current index is able to be changed while a buffer is transmitting is the reason why this needs to exist. The trans_done_index not matching the real index is what triggers the callback to get called. this way if the same buffer keeps on being sent over and over again the callback would only get called a single time only after the index has changed. This is written for double buffering handling and not for single buffer handling. That is something that would need to added or figured out when to call the callback. I don't believe that single buffer is going to be used due to tearing happening so it really becomes moot for adding handling of a single buffer. Unless the display is super small but in those cases an RGB bus is not going to be used with it SPI would be.