Hi! Thanks for the feedback, Timothy! Highly appreciated! I've added your changes to my updated version (changed some things now to make tests easier, e.g. making timing numbers parameters so they could be changed from top level module.)
On Tuesday 17 October 2006 19:14, Timothy Miller wrote: > Ok, I've had a look at the test code that I got from Nicholas and > Simon. This is some good quality stuff, and I believe I can use it > with relatively little modification for synthesis. I just have a few > comments I'd like to make about design in general, for educational > purposes. > > First of all, I would like to ask that people try to ensure that their > name is in their code. Additionally, if you copy code, please try to > ensure that you credit those whom you're copying. While the GPL > doesn't really require this, I think it's important that people get > due credit. > For my video controller I looked at some code from Xilinx for inspiration, didn't copy anything except the timing numbers. The copyright notice for that code: // // XILINX IS PROVIDING THIS DESIGN, CODE, OR INFORMATION "AS IS" // SOLELY FOR USE IN DEVELOPING PROGRAMS AND SOLUTIONS FOR // XILINX DEVICES. BY PROVIDING THIS DESIGN, CODE, OR INFORMATION // AS ONE POSSIBLE IMPLEMENTATION OF THIS FEATURE, APPLICATION // OR STANDARD, XILINX IS MAKING NO REPRESENTATION THAT THIS // IMPLEMENTATION IS FREE FROM ANY CLAIMS OF INFRINGEMENT, // AND YOU ARE RESPONSIBLE FOR OBTAINING ANY RIGHTS YOU MAY REQUIRE // FOR YOUR IMPLEMENTATION. XILINX EXPRESSLY DISCLAIMS ANY // WARRANTY WHATSOEVER WITH RESPECT TO THE ADEQUACY OF THE // IMPLEMENTATION, INCLUDING BUT NOT LIMITED TO ANY WARRANTIES OR // REPRESENTATIONS THAT THIS IMPLEMENTATION IS FREE FROM CLAIMS OF // INFRINGEMENT, IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS // FOR A PARTICULAR PURPOSE. // // (c) Copyright 2005 Xilinx, Inc. // All rights reserved. // > Anyhow, to the code. I'm commenting on these things without actually > trying to run or synthesize them myself just yet. > > Simon did an excellent job with the test pattern generator. I think > it's going to be the basis for video test code as we go forward, > although I think Nicholas has some things worth copying. > > Here's something interesting Simon did: > > // set pixel data on negative edge, works if first part of data is read on > // rising edge. If positive edges of clock and clock_2x are aligned > // then this works for dac output too. > always @(negedge clock or posedge reset) begin > if (reset) begin > r0 <= 10'h000; > g0 <= 10'h000; > b0 <= 10'h000; > r1 <= 10'h000; > g1 <= 10'h000; > b1 <= 10'h000; > end > else begin > // horisontal red ramp test pattern > r0 <= current_pixel[9:0]; > r1 <= current_pixel[9:0]; > end > end > > Notice the comment about using negedge. In designs in general, we try > to avoid using both edges of the clock. There are stylistic reasons, > but a major reason is that it's often very hard to meet timing. > However, there's nothing actually wrong with Simon's code here. It > was an oversight on my part that video data would be misaligned to the > syncs by half a clock because we're using DDR ff's on the video data. > There are two ways to fix this. One is to do exactly what Simon did. > The other is to use DDR ff's for the syncs, even though they're not > really DDR signals (use the same data for both edges of the clock). > Unless we have trouble meeting timing, I'm going to leave it this way. > > Here's something I added: > > reg last_pixel; > > always @(posedge clock or posedge reset) begin > if (reset) begin > current_pixel <= 11'h000; > current_line <= 10'h000; > last_pixel <= 0; > end > else begin > last_pixel <= (current_pixel == `H_TOTAL - 2); > > if (last_pixel) begin > current_pixel <= 11'h000; > > if (current_line == `V_TOTAL - 1) > current_line <= 10'h000; > else > current_line <= current_line + 1; > end > else > current_pixel <= current_pixel + 1; > end > end > > I added the "last_pixel" parts. Comparison against H_TOTAL-1 was > found all over the place. While comparisons against constants aren't > too expensive, this is a prime candidate for optimization. It is > trivial to predict one cycle in advance when current_pixel==H_TOTAL-1. > Most likely, with the old code, the synthesizer would have produces > one instance of a comparitor with H_TOTAL-1, but that adds > combinatorial delay to all of the logic it's used in. I've replaced > that with a registered comparator against H_TOTAL-2. Now, last_pixel > is true exactly when current_pixel==H_TOTAL-1. The result is logic > that uses the same area (presumably), but is capable of being clocked > faster because there's less combinatorial logic between registers for > the vertical logic (and one piece of the horizontal). > > That's all. Simon's video state machine is simple, straight-forward, > and readable. > > Nicholas' video block is very good too, but it doesn't bring the video > output signals to the top-level module. It's just easier to use > Simon's. > > However, what Nicholas did do for us that was excellent is produce a > video capture block. This module acts like a monitor, watching video > signals, and spits the video frame out to a PPM file. There are only > two modifications I would suggest (or may make myself). > > One is to add the code to sample DVI data on both edges of the clock > and fix up which bits are which. First, for DVI, vid_data[5:0] can be > thrown away, because they're just extra precision for analog. Next, > master and slave should be treated as independent. The master uses > vid_data[29:18], and the slave uses vid_data[17:6]. Take > vid_data[29:18] on the riding edge of the clock, and splice it > together with vid_data[29:18] on the falling edge, and now you have a > 24-bit pixel that you can write to the file (even pixels). Do the > same for [17:6], and those are the odd pixels. I've been playing around with this the last couple of evenings, I have an analog_monitor.v together with test case working. But with the dvi I ran in to the situation where on one clock edge the same data is read into two registers that are in series. The first being the outgoing pixel data and the second one the incoming in the "monitor" code. I feel silly for not thinking about this sooner... but is it the clock or data that will be delayed? How will that be done? The de-skew feature of the SiI178 doesn't seem enough. Generally, how is this problem of pure transmission (no combinatorial delay) solved? Regards, Simon > > The other is to count frames and hack the filename each time. > > function [31:0] open_ppm_file; > input [7:0] frame_num; > reg [0:11*8-1] file_name; > begin > file_name = "frameXX.ppm"; > file_name[40:47] = frame_num/10 + 48; > file_name[48:55] = frame_num%10 + 48; > open_ppm_file = $fopen(file_name); > end > endfunction > > > Note: There are functions $display, $fdisplay, $write, and $fwrite. > The f's write to files. The display functions append \n to the line, > while the write functions do not. > > I've attached a modified version of Simon's video controller. _______________________________________________ Open-graphics mailing list [email protected] http://lists.duskglow.com/mailman/listinfo/open-graphics List service provided by Duskglow Consulting, LLC (www.duskglow.com)
