PicoDVI crashes in CircuitPython 9.2.1

[sandman72]

CircuitPython version

Adafruit CircuitPython 9.2.1 on 2024-11-20; Raspberry Pi Pico 2 with rp2350a

Code/REPL

import board
import picodvi
import framebufferio
import displayio
displayio.release_displays()
# Setup for Raspberry Pi Pico 2 - RP2350 - with DVI_Sock installed
fb = picodvi.Framebuffer(320, 240, clk_dp=board.GP14, clk_dn=board.GP15,
                                   red_dp=board.GP12, red_dn=board.GP13,
                                   green_dp=board.GP18, green_dn=board.GP19,
                                   blue_dp=board.GP16, blue_dn=board.GP17,
                                   color_depth=16)
# Boom!                                   
display = framebufferio.FramebufferDisplay(fb)

Behavior

CircuitPython core code crashed hard. Whoops!
Hard fault: memory access or instruction error.

Description

The latest PicoDVI core library crashes with CircuitPython 9.2.1 on RP2350

Running the HSTX DVI demo code (Display Ruler) works fine on CircuitPython 9.2.0, but crashes on CircuitPython 9.2.1

To replicate on Raspberry Pi Pico 2 with DVI_Sock:

• Install CircuitPython 9.2.0 - adafruit-circuitpython-raspberry_pi_pico2-en_US-9.2.0.uf2
• Upload demo code from HSTX_to_DVI_Breakout_Example.zip
• Change pins used in Framebuffer - see above code
  -> Runs fine
• Install CircuitPython 9.2.1 - adafruit-circuitpython-raspberry_pi_pico2-en_US-9.2.1.uf2
  -> Crashes hard

Additional information

No response

[dhalbert]

I did a bisect. It's f14b4c6, in #9659.

Pinging @timchinowsky for interest. picodvi uses DMA IRQ 1. #9659 added use of that IRQ for PIO: previously PIO was only using IRQ DMA 0, if I understand correctly.

The picodvi test program doesn't use PIO, but I think it's being used for the status NeoPixel.

[timchinowsky]

So is the PicoDVI IRQ handler being clobbered by the PIO handler I added? I would have thought the linker would complain about that.

[timchinowsky]

In Framebuffer_RP2350.c I'm seeing the IRQ1 handler is

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

Lines 118 to 130 in 2d2fa28

	static void __not_in_flash_func(dma_irq_handler)(void) {
	if (active_picodvi == NULL) {
	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];
	ch->al3_read_addr_trig = (uintptr_t)active_picodvi->dma_commands;
	}

while the handler for PIO is tucked into audio_dma.c:

circuitpython/ports/raspberrypi/audio_dma.c

Lines 464 to 504 in 2d2fa28

	void __not_in_flash_func(isr_dma_0)(void) {
	for (size_t i = 0; i < NUM_DMA_CHANNELS; i++) {
	uint32_t mask = 1 << i;
	if ((dma_hw->intr & mask) == 0) {
	continue;
	}
	// acknowledge interrupt early. Doing so late means that you could lose an
	// interrupt if the buffer is very small and the DMA operation
	// completed by the time callback_add() / dma_complete() returned. This
	// affected PIO continuous write more than audio.
	dma_hw->ints0 = mask;
	if (MP_STATE_PORT(playing_audio)[i] != NULL) {
	audio_dma_t *dma = MP_STATE_PORT(playing_audio)[i];
	// Record all channels whose DMA has completed; they need loading.
	dma->channels_to_load_mask |= mask;
	background_callback_add(&dma->callback, dma_callback_fun, (void *)dma);
	}
	if (MP_STATE_PORT(background_pio)[i] != NULL) {
	rp2pio_statemachine_obj_t *pio = MP_STATE_PORT(background_pio)[i];
	rp2pio_statemachine_dma_complete_write(pio, i);
	}
	}
	}
	void __not_in_flash_func(isr_dma_1)(void) {
	for (size_t i = 0; i < NUM_DMA_CHANNELS; i++) {
	uint32_t mask = 1 << i;
	if ((dma_hw->intr & mask) == 0) {
	continue;
	}
	// acknowledge interrupt early. Doing so late means that you could lose an
	// interrupt if the buffer is very small and the DMA operation
	// completed by the time callback_add() / dma_complete() returned. This
	// affected PIO continuous write more than audio.
	dma_hw->ints1 = mask;
	if (MP_STATE_PORT(background_pio)[i] != NULL) {
	rp2pio_statemachine_obj_t *pio = MP_STATE_PORT(background_pio)[i];
	rp2pio_statemachine_dma_complete_read(pio, i);
	}
	}
	}

The handler for IRQ0 is multiplexed to handle both audio_dma and rp2pio interrupts. So, a few options for fixing this would be

1. Multiplex IRQ1 similarly, to handle both rp2pio and picodvi interrupts.
2. Move the picodvi interrupt to IRQ0, and multiplex it into that ISR. This gives IRQ0 a lot to do, but it dedicates IRQ0 to output and IRQ1 to input, which has some appeal.
3. Multiplex the rp2pio input IRQ into IRQ0, putting all audio and rp2pio activity into IRQ0, and leaving IRQ1 open for picodvi.

It would be nice to get all the handlers in one place, bc it looks like the linker is not smart enough to catch this.

I'm also seeing that the implementation for RP2350 is very different from RP2040. In general there's a lot here that's new to me so basically I'm saying help, @tannewt!!!

[dhalbert]

I'm looking at https://www.raspberrypi.com/documentation/pico-sdk/hardware.html.

Defining the interrupt handler explicitly in your application (e.g. by defining void isr_dma_0 will make that function the handler for the DMA_IRQ_0 on core 0, and you will not be able to change it using the above APIs at runtime). Using this method can cause link conflicts at runtime, and offers no runtime performance benefit (i.e, it should not generally be used).

audio_dma.c uses the global routines isr_dma_0m and now isr_dma_1, instead of being polite and using irq_set_exclusive_handler() or irq_add_shared_handler (the "above APIs" mentioned in the quote above). This was a mistake -- audio_dma.c should cooperate with other potential users of the DMA interrupts.

I'm not sure why rp2pio and and picodvi require an exclusive handler by using irq_set_exclusive_handler(). Maybe they could use a shared handler.

rp2pio uses DMA optionally. It sees if there are no free DMA channels, and doesn't use them if they're not available.

I think it's an accident that this used to work, because no one else was using DMA 1.

[dhalbert]

Perhaps for now we should revert #9659 and then carefully fix the use of DMA IRQ handlers so they are set up dynamically everywhere rather than using the fixed isr_dma_0 and isr_dma_1 routines.

[timchinowsky]

Well that would be a little sad 😉 but you gotta do what you gotta do. I see that RP2350 also has a DMA_IRQ_2 and DMA_IRQ_3, could RP2350 PicoDVI be tweaked to use one of those, as a less disruptive temporizing measure, until dynamic setup can be fixed for real?

[dhalbert]

I see that RP2350 also has a DMA_IRQ_2 and DMA_IRQ_3, could RP2350 PicoDVI be tweaked to use one of those, as a less disruptive temporizing measure, until dynamic setup can be fixed for real?

This also crashes on RP2040, sadly. On a Feather DVI board, 9.2.1 doesn't even start properly because the picodvi.FrameBuffer is created in board.c. 9.2.0 is fine on that board.

[timchinowsky]

Alas. So in the dynamic setup of the future, how would the different modules that want to use these resources need to coexist? If the RP2040 Feather DVI board uses PIO for both DVI and neopixel, you're not going to be able to do any I2S on there, right?

[dhalbert]

I"m wondering if they need exclusive access to the IRQ's or get away with shared access. There might glitches, but it's better than not being able to do everything at once.

[RetiredWizard]

I believe this is the crash I was running into this afternoon. In my case changing the color_depth parameter to 4 avoided the crash but then framebufferio complains about the 320,240 resolution.