Make nesting_count update atomic by disabling interrupts.

Fix miscellaneous memory leaks.
Minor code cleanup in RP2350 framebuffer.
Factor teardown of partially constructed Framebuffer object.

Author: eightycc

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/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;
     }
@@ -297,25 +293,25 @@ void common_hal_picodvi_framebuffer_construct(picodvi_framebuffer_obj_t *self,
     }
     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);
+        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;