Merge pull request #10116 from eightycc/framebuffer2

Fixes to RP2350 Framebuffer and handling of flash writes

Author: tannewt

ports/raspberrypi/bindings/picodvi/Framebuffer.c

@@ -51,11 +51,11 @@
 //|         The framebuffer pixel format varies depending on color_depth:
 //|
 //|         * 1 - Each bit is a pixel. Either white (1) or black (0).
-//|         * 2 - Each 2 bits is a pixels. Grayscale between white (0x3) and black (0x0).
-//|         * 4 - Each nibble is a pixels in RGB format. The fourth bit is ignored. (RP2350 only)
-//|         * 8 - Each byte is a pixels in RGB332 format.
-//|         * 16 - Each two bytes are a pixel in RGB565 format.
-//|         * 32 - Each four bytes are a pixel in RGB888 format. The top byte is ignored.
+//|         * 2 - Each two bits is a pixel. Grayscale between white (0x3) and black (0x0).
+//|         * 4 - Each nibble is a pixel in RGB format. The fourth bit is ignored. (RP2350 only)
+//|         * 8 - Each byte is a pixel in RGB332 format.
+//|         * 16 - Each two bytes is a pixel in RGB565 format.
+//|         * 32 - Each four bytes is a pixel in RGB888 format. The top byte is ignored.
 //|
 //|         Output resolution support varies between the RP2040 and RP2350.
 //|

ports/raspberrypi/common-hal/microcontroller/__init__.c

@@ -55,6 +55,7 @@ void common_hal_mcu_enable_interrupts(void) {
 #define PICO_ELEVATED_IRQ_PRIORITY (0x60) // between PICO_DEFAULT and PIOCO_HIGHEST_IRQ_PRIORITY
 static uint32_t oldBasePri = 0; // 0 (default) masks nothing, other values mask equal-or-larger priority values
 void common_hal_mcu_disable_interrupts(void) {
+    uint32_t my_interrupts = save_and_disable_interrupts();
     if (nesting_count == 0) {
         // We must keep DMA_IRQ_1 (reserved for pico dvi) enabled at all times,
         // including during flash writes. Do this by setting the priority mask (BASEPRI
@@ -66,6 +67,7 @@ void common_hal_mcu_disable_interrupts(void) {
         __isb(); // Instruction synchronization barrier
     }
     nesting_count++;
+    restore_interrupts(my_interrupts);
 }
 
 void common_hal_mcu_enable_interrupts(void) {

ports/raspberrypi/common-hal/nvm/ByteArray.c

@@ -27,21 +27,16 @@ static void write_page(uint32_t page_addr, uint32_t offset, uint32_t len, uint8_
     // Write a whole page to flash, buffering it first and then erasing and rewriting it
     // since we can only write a whole page at a time.
     if (offset == 0 && len == FLASH_PAGE_SIZE) {
-        // disable interrupts to prevent core hang on rp2040
-        common_hal_mcu_disable_interrupts();
         supervisor_flash_pre_write();
         flash_range_program(RMV_OFFSET(page_addr), bytes, FLASH_PAGE_SIZE);
         supervisor_flash_post_write();
-        common_hal_mcu_enable_interrupts();
     } else {
         uint8_t buffer[FLASH_PAGE_SIZE];
         memcpy(buffer, (uint8_t *)page_addr, FLASH_PAGE_SIZE);
         memcpy(buffer + offset, bytes, len);
-        common_hal_mcu_disable_interrupts();
         supervisor_flash_pre_write();
         flash_range_program(RMV_OFFSET(page_addr), buffer, FLASH_PAGE_SIZE);
         supervisor_flash_post_write();
-        common_hal_mcu_enable_interrupts();
     }
 
 }

ports/raspberrypi/common-hal/picodvi/Framebuffer_RP2350.c

@@ -165,12 +165,11 @@ static void __not_in_flash_func(dma_irq_handler)(void) {
         return;
     }
     uint ch_num = active_picodvi->dma_pixel_channel;
-    dma_channel_hw_t *ch = &dma_hw->ch[ch_num];
     dma_hw->intr = 1u << ch_num;
 
     // Set the read_addr back to the start and trigger the first transfer (which
     // will trigger the pixel channel).
-    ch = &dma_hw->ch[active_picodvi->dma_command_channel];
+    dma_channel_hw_t *ch = &dma_hw->ch[active_picodvi->dma_command_channel];
     ch->al3_read_addr_trig = (uintptr_t)active_picodvi->dma_commands;
 }
 
@@ -224,6 +223,9 @@ void common_hal_picodvi_framebuffer_construct(picodvi_framebuffer_obj_t *self,
         mp_raise_ValueError_varg(MP_ERROR_TEXT("Invalid %q and %q"), MP_QSTR_width, MP_QSTR_height);
     }
 
+    self->dma_command_channel = -1;
+    self->dma_pixel_channel = -1;
+
     if (width % 160 == 0) {
         self->output_width = 640;
     } else {
@@ -258,24 +260,18 @@ void common_hal_picodvi_framebuffer_construct(picodvi_framebuffer_obj_t *self,
 
     self->width = width;
     self->height = height;
-    self->pitch = (self->width * color_depth) / 8;
     self->color_depth = color_depth;
-    // Align each row to words.
-    if (self->pitch % sizeof(uint32_t) != 0) {
-        self->pitch += sizeof(uint32_t) - (self->pitch % sizeof(uint32_t));
-    }
-    self->pitch /= sizeof(uint32_t);
+    // Pitch is number of 32-bit words per line. We round up pitch_bytes to the nearest word
+    // so that each scanline begins on a natural 32-bit word boundary.
+    size_t pitch_bytes = (self->width * color_depth) / 8;
+    self->pitch = (pitch_bytes + sizeof(uint32_t) - 1) / sizeof(uint32_t);
     size_t framebuffer_size = self->pitch * self->height;
 
     // We check that allocations aren't in PSRAM because we haven't added XIP
     // streaming support.
     self->framebuffer = (uint32_t *)port_malloc(framebuffer_size * sizeof(uint32_t), true);
     if (self->framebuffer == NULL || ((size_t)self->framebuffer & 0xf0000000) == 0x10000000) {
-        if (self->framebuffer != NULL) {
-            // Return the memory in PSRAM.
-            port_free(self->framebuffer);
-            self->framebuffer = NULL;
-        }
+        common_hal_picodvi_framebuffer_deinit(self);
         m_malloc_fail(framebuffer_size * sizeof(uint32_t));
         return;
     }
@@ -296,26 +292,26 @@ void common_hal_picodvi_framebuffer_construct(picodvi_framebuffer_obj_t *self,
         self->dma_commands_len = (MODE_720_V_FRONT_PORCH + MODE_720_V_SYNC_WIDTH + MODE_720_V_BACK_PORCH + 2 * MODE_720_V_ACTIVE_LINES + 1) * dma_command_size;
     }
     self->dma_commands = (uint32_t *)port_malloc(self->dma_commands_len * sizeof(uint32_t), true);
-    if (self->dma_commands == NULL || ((size_t)self->framebuffer & 0xf0000000) == 0x10000000) {
-        port_free(self->framebuffer);
+    if (self->dma_commands == NULL || ((size_t)self->dma_commands & 0xf0000000) == 0x10000000) {
+        common_hal_picodvi_framebuffer_deinit(self);
         m_malloc_fail(self->dma_commands_len * sizeof(uint32_t));
         return;
     }
 
-    int dma_pixel_channel_maybe = dma_claim_unused_channel(false);
-    if (dma_pixel_channel_maybe < 0) {
-        mp_raise_RuntimeError(MP_ERROR_TEXT("Internal resource(s) in use"));
-        return;
-    }
+    // The command channel and the pixel channel form a pipeline that feeds combined HSTX
+    // commands and pixel data to the HSTX FIFO. The command channel reads a pre-computed
+    // list of control/status words from the dma_commands buffer and writes them to the
+    // pixel channel's control/status registers. Under control of the command channel, the
+    // pixel channel smears/swizzles pixel data from the framebuffer and combines
+    // it with HSTX commands, forwarding the combined stream to the HSTX FIFO.
 
-    int dma_command_channel_maybe = dma_claim_unused_channel(false);
-    if (dma_command_channel_maybe < 0) {
-        dma_channel_unclaim((uint)dma_pixel_channel_maybe);
+    self->dma_pixel_channel = dma_claim_unused_channel(false);
+    self->dma_command_channel = dma_claim_unused_channel(false);
+    if (self->dma_pixel_channel < 0 || self->dma_command_channel < 0) {
+        common_hal_picodvi_framebuffer_deinit(self);
         mp_raise_RuntimeError(MP_ERROR_TEXT("Internal resource(s) in use"));
         return;
     }
-    self->dma_pixel_channel = dma_pixel_channel_maybe;
-    self->dma_command_channel = dma_command_channel_maybe;
 
     size_t command_word = 0;
     size_t frontporch_start;
@@ -582,7 +578,10 @@ void common_hal_picodvi_framebuffer_construct(picodvi_framebuffer_obj_t *self,
     dma_irq_handler();
 }
 
-static void _turn_off_dma(uint8_t channel) {
+static void _turn_off_dma(int channel) {
+    if (channel < 0) {
+        return;
+    }
     dma_channel_config c = dma_channel_get_default_config(channel);
     channel_config_set_enable(&c, false);
     dma_channel_set_config(channel, &c, false /* trigger */);
@@ -605,6 +604,8 @@ void common_hal_picodvi_framebuffer_deinit(picodvi_framebuffer_obj_t *self) {
 
     _turn_off_dma(self->dma_pixel_channel);
     _turn_off_dma(self->dma_command_channel);
+    self->dma_pixel_channel = -1;
+    self->dma_command_channel = -1;
 
     active_picodvi = NULL;
 

ports/raspberrypi/common-hal/picodvi/Framebuffer_RP2350.h

@@ -39,6 +39,6 @@ typedef struct {
     mp_uint_t output_width;
     uint16_t pitch; // Number of words between rows. (May be more than a width's worth.)
     uint8_t color_depth;
-    uint8_t dma_pixel_channel;
-    uint8_t dma_command_channel;
+    int dma_pixel_channel;
+    int dma_command_channel;
 } picodvi_framebuffer_obj_t;

ports/raspberrypi/supervisor/internal_flash.c

@@ -40,41 +40,26 @@
 static uint8_t _cache[SECTOR_SIZE];
 static uint32_t _cache_lba = NO_CACHE;
 static uint32_t _flash_size = 0;
-
-#ifdef PICO_RP2350
-static uint32_t m1_rfmt;
-static uint32_t m1_timing;
+#if CIRCUITPY_AUDIOCORE
+static uint32_t _audio_channel_mask;
 #endif
 
-static void __no_inline_not_in_flash_func(save_psram_settings)(void) {
-    #ifdef PICO_RP2350
-    // We're about to invalidate the XIP cache, clean it first to commit any dirty writes to PSRAM
-    // From https://forums.raspberrypi.com/viewtopic.php?t=378249#p2263677
-    // Perform clean-by-set/way on all lines
-    for (uint32_t i = 0; i < 2048; ++i) {
-        // Use the upper 16k of the maintenance space (0x1bffc000 through 0x1bffffff):
-        *(volatile uint8_t *)(XIP_SRAM_BASE + (XIP_MAINTENANCE_BASE - XIP_BASE) + i * 8u + 0x1u) = 0;
-    }
-
-    m1_timing = qmi_hw->m[1].timing;
-    m1_rfmt = qmi_hw->m[1].rfmt;
-    #endif
-}
-
-static void __no_inline_not_in_flash_func(restore_psram_settings)(void) {
-    #ifdef PICO_RP2350
-    qmi_hw->m[1].timing = m1_timing;
-    qmi_hw->m[1].rfmt = m1_rfmt;
-    __compiler_memory_barrier();
-    #endif
-}
-
 void supervisor_flash_pre_write(void) {
-    save_psram_settings();
+    // Disable interrupts. XIP accesses will fault during flash writes.
+    common_hal_mcu_disable_interrupts();
+    #if CIRCUITPY_AUDIOCORE
+    // Pause audio DMA to avoid noise while interrupts are disabled.
+    _audio_channel_mask = audio_dma_pause_all();
+    #endif
 }
 
 void supervisor_flash_post_write(void) {
-    restore_psram_settings();
+    #if CIRCUITPY_AUDIOCORE
+    // Unpause audio DMA.
+    audio_dma_unpause_mask(_audio_channel_mask);
+    #endif
+    // Re-enable interrupts.
+    common_hal_mcu_enable_interrupts();
 }
 
 void supervisor_flash_init(void) {
@@ -91,11 +76,9 @@ void supervisor_flash_init(void) {
     // Read the RDID register to get the flash capacity.
     uint8_t cmd[] = {0x9f, 0, 0, 0};
     uint8_t data[4];
-    common_hal_mcu_disable_interrupts();
     supervisor_flash_pre_write();
     flash_do_cmd(cmd, data, 4);
     supervisor_flash_post_write();
-    common_hal_mcu_enable_interrupts();
     uint8_t power_of_two = FLASH_DEFAULT_POWER_OF_TWO;
     // Flash must be at least 2MB (1 << 21) because we use the first 1MB for the
     // CircuitPython core. We validate the range because Adesto Tech flash chips
@@ -119,21 +102,11 @@ void port_internal_flash_flush(void) {
     if (_cache_lba == NO_CACHE) {
         return;
     }
-    // Make sure we don't have an interrupt while we do flash operations.
-    common_hal_mcu_disable_interrupts();
-    // and audio DMA must be paused as well
-    #if CIRCUITPY_AUDIOCORE
-    uint32_t channel_mask = audio_dma_pause_all();
-    #endif
     supervisor_flash_pre_write();
     flash_range_erase(CIRCUITPY_CIRCUITPY_DRIVE_START_ADDR + _cache_lba, SECTOR_SIZE);
     flash_range_program(CIRCUITPY_CIRCUITPY_DRIVE_START_ADDR + _cache_lba, _cache, SECTOR_SIZE);
-    supervisor_flash_post_write();
     _cache_lba = NO_CACHE;
-    #if CIRCUITPY_AUDIOCORE
-    audio_dma_unpause_mask(channel_mask);
-    #endif
-    common_hal_mcu_enable_interrupts();
+    supervisor_flash_post_write();
 }
 
 mp_uint_t supervisor_flash_read_blocks(uint8_t *dest, uint32_t block, uint32_t num_blocks) {