raspberrypi / pico-extras

BSD 3-Clause "New" or "Revised" License
480 stars 120 forks source link

Unexpected result in SCANVIDEO if sync pins below colour pins #36

Open Memotech-Bill opened 2 years ago

Memotech-Bill commented 2 years ago

I am trying to build a VGA interface using only 8 GPIO pins. Two pins for sync and two pins each for red, green and blue.

This is not currently working for my board.

If I have the following definitions in my board header file:

#define PICO_SCANVIDEO_SYNC_PIN_BASE 6
#define PICO_SCANVIDEO_HSYNC_PIN 6
#define PICO_SCANVIDEO_VSYNC_PIN 7
#define PICO_SCANVIDEO_ALPHA_PIN -1
#define PICO_SCANVIDEO_COLOR_PIN_BASE 0
#define PICO_SCANVIDEO_COLOR_PIN_COUNT 6
#define PICO_SCANVIDEO_PIXEL_RSHIFT 0
#define PICO_SCANVIDEO_PIXEL_GSHIFT 2
#define PICO_SCANVIDEO_PIXEL_BSHIFT 4
#define PICO_SCANVIDEO_PIXEL_RCOUNT 2
#define PICO_SCANVIDEO_PIXEL_GCOUNT 2
#define PICO_SCANVIDEO_PIXEL_BCOUNT 2

Then, loading this onto the Pico I measure the expected 31.25kHz on GPIO 6 and 60Hz on GPIO 7.

However, my board has the pins in a different order. If I compile with the following board definition:

#define PICO_SCANVIDEO_SYNC_PIN_BASE 0
#define PICO_SCANVIDEO_HSYNC_PIN 0
#define PICO_SCANVIDEO_VSYNC_PIN 1
#define PICO_SCANVIDEO_ALPHA_PIN -1
#define PICO_SCANVIDEO_COLOR_PIN_BASE 2
#define PICO_SCANVIDEO_COLOR_PIN_COUNT 6
#define PICO_SCANVIDEO_PIXEL_RSHIFT 0
#define PICO_SCANVIDEO_PIXEL_GSHIFT 2
#define PICO_SCANVIDEO_PIXEL_BSHIFT 4
#define PICO_SCANVIDEO_PIXEL_RCOUNT 2
#define PICO_SCANVIDEO_PIXEL_GCOUNT 2
#define PICO_SCANVIDEO_PIXEL_BCOUNT 2

and load the resulting executable onto the Pico, I get the expected 60Hz on GPIO 1, but GPIO 0 measures 25MHz!

So it appears that, for some reason, if the sync pins are below the colour pins, what should be HSYNC is actually the pixel clock.

Any obvious reason for this?

Memotech-Bill commented 2 years ago

As a further experiment, I tried the following board definition:

#define PICO_SCANVIDEO_SYNC_PIN_BASE 2
#define PICO_SCANVIDEO_HSYNC_PIN 2
#define PICO_SCANVIDEO_VSYNC_PIN 3
#define PICO_SCANVIDEO_ALPHA_PIN -1
#define PICO_SCANVIDEO_COLOR_PIN_BASE 4
#define PICO_SCANVIDEO_COLOR_PIN_COUNT 6
#define PICO_SCANVIDEO_PIXEL_RSHIFT 0
#define PICO_SCANVIDEO_PIXEL_GSHIFT 2
#define PICO_SCANVIDEO_PIXEL_BSHIFT 4
#define PICO_SCANVIDEO_PIXEL_RCOUNT 2
#define PICO_SCANVIDEO_PIXEL_GCOUNT 2
#define PICO_SCANVIDEO_PIXEL_BCOUNT 2

This gives the expected 31.25kHz on GPIO 2. So it looks as the problem is only if the HSYNC is on GPIO 0.

kilograham commented 2 years ago

The setup code in scanvideo,c is definitely not correct for certain pin combinations - there are a few PRs from people to fix very scenarios, but you may want to look at the code.

Memotech-Bill commented 2 years ago

Adding some diagnostics and building with my board configuration with PICO_SCANVIDEO_SYNC_PIN_BASE = 0 and PICO_SCANVIDEO_COLOR_PIN_BASE = 2 gives:

State machine 0: execctrl = 0x0002D580, shiftctrl = 0x400E0000, pinctrl = 0x00600002
pinctrl: in: base = 0, out: base = 2, count = 6, set: base = 0, count = 0, sideset: base = 0, count = 0
State machine 1: execctrl = 0x0001F000, shiftctrl = 0x000C0000, pinctrl = 0x14000000
pinctrl: in: base = 0, out: base = 0, count = 0, set: base = 0, count = 5, sideset: base = 0, count = 0
State machine 2: execctrl = 0x0001F000, shiftctrl = 0x000C0000, pinctrl = 0x14000000
pinctrl: in: base = 0, out: base = 0, count = 0, set: base = 0, count = 5, sideset: base = 0, count = 0
State machine 3: execctrl = 0x0001FD80, shiftctrl = 0x000E0000, pinctrl = 0x20200000
pinctrl: in: base = 0, out: base = 0, count = 2, set: base = 0, count = 0, sideset: base = 0, count = 1

So the timing SM (3) has a sideset count of 1, even though PICO_SCANVIDEO_ENABLE_CLOCK_PIN = 0. Looking at "timing.pio.h" reveals:

static inline pio_sm_config video_htiming_program_get_default_config(uint offset) {
    pio_sm_config c = pio_get_default_sm_config();
    sm_config_set_wrap(&c, offset + video_htiming_wrap_target, offset + video_htiming_wrap);
    sm_config_set_sideset(&c, 1, false, false);
    return c;
}

So the PIO compiler is generating a default configuration which unconditionally sets a sideset count of 1. However, since PICO_SCANVIDEO_ENABLE_CLOCK_PIN = 0, the sideset pin base is being left undefined (and is defaulting to zero).

It is suggested that routine setup_sm in pico-extras/src/rp2_common/pico_scanvideo_dpi/scanvideo.c should be revised as follows

void setup_sm(int sm, uint offset) {
#ifndef NDEBUG
    printf("Setting up SM %d\n", sm);
#endif

    pio_sm_config config = is_scanline_sm(sm) ? video_mode.pio_program->configure_pio(video_pio, sm, offset) :
                           video_htiming_program_get_default_config(offset);

#if PICO_SCANVIDEO_ENABLE_VIDEO_CLOCK_DOWN
    sm_config_set_clkdiv_int_frac(&config, video_clock_down_times_2 / 2, (video_clock_down_times_2 & 1u) << 7u);
#endif

    if (!is_scanline_sm(sm)) {
        // enable auto-pull
        sm_config_set_out_shift(&config, true, true, 32);
        const uint BASE = PICO_SCANVIDEO_SYNC_PIN_BASE; // hsync and vsync are +0 and +1, clock is +2
        uint pin_count;
#if PICO_SCANVIDEO_ENABLE_DEN_PIN
        pin_count = 3;
        // 3 OUT pins and maybe 1 sideset pin following them
#else
        // 2 OUT pins and 1 sideset pin following them
        pin_count = 2;
#endif
        sm_config_set_out_pins(&config, BASE, pin_count);
#if PICO_SCANVIDEO_ENABLE_CLOCK_PIN
        // side set pin as well
        sm_config_set_sideset_pins(&config, BASE + pin_count);
        pin_count++;
#else
        sm_config_set_sideset(&config, 0, false, false);
#endif
        pio_sm_set_consecutive_pindirs(video_pio, sm, BASE, pin_count, true);
    }

    pio_sm_init(video_pio, sm, offset, &config); // now paused
}
lurch commented 2 years ago

It is suggested that...

If you've tested that as working, you should probably submit a PR ?

Memotech-Bill commented 2 years ago

I have now tested it, and it does not work :(

Changing the sideset count changes the sidesest data in the PIO program into delay data, wrecking the timing.

Three options:

Edit: Since the SCANVIDEO code already dynamically edits the "timing.pio" program for the polarity of the clock signal, making the same code dynamically remove the sideset if there is no clock is probably the way to go.

I have partially tested this, and the sync frequencies are now correct. I have yet to check that I now have a working display signal.

Memotech-Bill commented 2 years ago

I have now tested my revisions to dynamically edit the timing PIO program to remove the pixel clock side set if PICO_SCANVIDEO_ENABLE_CLOCK_PIN is zero (disabled) and confirmed that I now get a working VGA signal.

I have created a pull request with my changes: https://github.com/raspberrypi/pico-extras/pull/37/files