sy2002 / QNICE-FPGA

QNICE-FPGA is a 16-bit computer system for recreational programming built as a fully-fledged System-on-a-Chip in portable VHDL.
http://qnice-fpga.com
Other
69 stars 15 forks source link

VGA_SCAN_LINE not always incrementing by 1 #181

Closed MJoergen closed 3 years ago

MJoergen commented 3 years ago

The VGA scan line register VGA_SCAN_LINE is expected to increment by one in the range 0 to 524 inclusive.

Occasionally, spurious values are returned.

The program test_vga_scan_line.c can be used to reproduce the error.

Sample output is:

Suspicious:
521
522
523
524
4
524
0
1
2
3

In the above, the value 4 is spurious.

I believe the root cause is incorrect Clock Domain Crossing of the signal vga_pixel_y in the file vga_multicolor.vhd.

MJoergen commented 3 years ago

Fixed and verified.

MJoergen commented 3 years ago

Just wanted to add some clarification: After building the hardware using Vivado it is possible to generate various reports. One such report is called CDC and deals exactly with Clock Domain Crossings. Running this tool gives a Critical Warning regarding precisely the signal vga_pixel_y in the file vga_multicolor.vhd.

The warning says that all signals before a CDC must come directly from a register and not combinatorial logic. The reason is that we are sampling an array of bits, and it is important that all bits change simultaneously. In this particular case, due to combinatorial logic, some of the bits would experience some transient toggling before settling on the correct value. But due to the asynchronous nature of a CDC, the signal would be sampled while it was toggling, leading to the incorrect behavior seen.

The fix was easy: Simply add a register to the result of the combinatorial calculation.

sy2002 commented 3 years ago

@MJoergen WOW - thank you - this is a cool finding. I always love to learn new things so thank you for sheding some light on this very interessting topic. And P.S.: Maybe CDC problems are also the reason for some strange HyperRAM behaviours on the MEGA65! I will try to use the CDC report that you are talking about there, too.