FPGA VGA Controller for 8-bit computer

[nockieboy]

Latest code attached.  I'm not sure what should be going in the CMD_IN_PXCOPY case if a valid address and cache hit is found. It seems it shouldn't be doing anything as the cache values will already have the necessary data in them already?  Shouldn't that data then be written out somewhere?

Sorry.  I've been away from this for about six hours and I've already lost my thread. 

[BrianHG]

Ok, you just about got it.

1) Always store a copy of all the pixel parameters like bits-pixel, bit position and color data.  Even if the cache address is a valid match (cache hit), the bits/pixel, sub-pixel location and color function may change within the same address.  IE reading the next adjacent pixel within the same memory address.

2) If the read address is different, then also store the cache copy of the new read address and wait for the 'rd_data_rdy_a' flag to come in.  Once it comes in, set the rd_cache_valid flag, latch the read ram data and clear the stop_fifo_read.

hmmmm, 'stop_fifo_read'.   I don't like that.  Let's change it to 'read_wait_a' and for the pixel writer we will make a 'read_wait_b' registers.  For the fifo control, we will use for the shift_out something like:

cmd_shift_out = ( (  !(read_wait_a && !rd_data_rdy_a) && !(read_wait_b && !rd_data_rdy_b) ) && pixel_cmd_rdy ) ;

The first 2 if set stop the fifo reading while the second optional two '!rd_data_rdy_a/b' will begin the fifo once again 1 clock early before the 'read_wait_a/b' get cleared by the 'rd_data_rdy_a/b'.  Reading the fifo will only begin if only if there is a new 'pixel_cmd_rdy'.

I just realized that if a rd_data_rdy_a/b never comes after a rd_req_a/b, EG something wrong with the command to the ram controller, the pixel write may stay frozen until a reset.  You will need to decide if we should include some sort of recovery timeout, out, you can just do this in the Z80 if the geometry plotter fifo fill up and never empties.

[nockieboy]

1) Always store a copy of all the pixel parameters like bits-pixel, bit position and color data.  Even if the cache address is a valid match (cache hit), the bits/pixel, sub-pixel location and color function may change within the same address.  IE reading the next adjacent pixel within the same memory address.

Okay, I've done this by setting the colour, bpp and target caches outside of the check for a cache miss, so they get set every time CMD_IN_PXCOPY is valid.

This is in the cache-miss conditional now, as I didn't want to overwrite the read colour data from RAM when the read completes, but I'm caching it in a different register now so it's not an issue.  Should work fine as it is, though?

2) If the read address is different, then also store the cache copy of the new read address and wait for the 'rd_data_rdy_a' flag to come in.  Once it comes in, set the rd_cache_valid flag, latch the read ram data and clear the stop_fifo_read.

Okay, if there's a cache miss I'm updating the cache address with the new one, setting rd_wait_a and rd_req_a and resetting rd_cache_valid.  I hope I'm using the right method to wait for the data to return, though:

Code: [Select]

end else if ( pixel_cmd_rdy & ~rd_wait_a & ~rd_wait_b ) begin
The above check should stop the case from executing if either rd_wait_a or _b are high.

Now I'm a little lost with what to do with the data that's been returned from RAM.  There's a clock delay whilst rd_wait_a is reset, cache_valid is set and the read data from RAM is cached.  Then CMD_IN_PXCOPY executes a second time, branching into the empty half of the IF..conditional to do something with the read data?  Is that right?  Seems like I'm using 2 clocks when you're going to tell me it can all be done in zero... 

I just realized that if a rd_data_rdy_a/b never comes after a rd_req_a/b, EG something wrong with the command to the ram controller, the pixel write may stay frozen until a reset.  You will need to decide if we should include some sort of recovery timeout, out, you can just do this in the Z80 if the geometry plotter fifo fill up and never empties.

I think a timeout will make it more robust?

EDIT: Seems there a problem with the HDL as it is - I'm getting these two errors:

Error (10200): Verilog HDL Conditional Statement error at geo_pixel_writer.sv(133): cannot match operand(s) in the condition to the corresponding edges in the enclosing event control of the always construct
Error (10200): Verilog HDL Conditional Statement error at geo_pixel_writer.sv(166): cannot match operand(s) in the condition to the corresponding edges in the enclosing event control of the always construct

[BrianHG]

Ok...

1) always_ff @( posedge clk or posedge reset or posedge collision_rd_rst or posedge collision_wr_rst ) begin

This means that all the flipflops inside will have 4 clock edge sources.  Wont that slow down/delay all these flipflops with so many edge conditions?

What's wrong with this?
always_ff @( posedge clk) begin

2) These guys:

Code: [Select]

    if ( collision_rd_rst ) rd_px_collision_counter <= 8'b0 ;   // reset the COPY/READ PIXEL COLLISION counter
    if ( collision_wr_rst ) wr_px_collision_counter <= 8'b0 ;   // reset the WRITE PIXEL COLLISION counter
   
    if ( rd_data_rdy_a ) begin  // valid data from RAM

rd_cache_valid <= 1'b1 ;
rd_wait_a      <= 1'b0 ;
rd_data_cache  <= rd_data_in ;

end
Why do they have higher priority than the system reset line?
And if the reset is set, and a 'rd_data_rdy_a' comes in at the same time, wont 'rd_cache_valid' be assigned a 1 and 0 simultaneously?
Shouldn't these be set after the else of 'reset'?

3)    end else if ( pixel_cmd_rdy & !rd_wait_a & !rd_wait_b ) begin
Wouldn't it be better to write this like this?
       end else begin
Now, you may have functions after the reset which may run both during the 'exec_cmd' and also separately during the '!exec_cmd' while the system waits for a response.

4) if ( pixel_cmd_rdy & !rd_wait_a & !rd_wait_b )
Single '&' instead of '&&'.  If I were you, just change it to:
    if ( exec_cmd )

5) I said to assign these 3:

Code: [Select]

rd_cache_col   <= colour   ;
rd_cache_bpp   <= bpp      ;
rd_cache_bit   <= target   ;
Whether of not there is a cache hit, so why are they inside the if:

Code: [Select]

                if ( !rd_cache_valid | !rd_addr_valid ) begin  // check for cache miss on read address
Also you use | instead or ||.

Now, there is also a new 'end else begin' at the bottom since we want some actions during the '!exec_cmd', like maybe the:

Code: [Select]

    if ( rd_data_rdy_a ) begin  // valid data from RAM

rd_cache_valid <= 1'b1 ;
rd_wait_a      <= 1'b0 ;
rd_data_cache  <= rd_data_in ;

end

Give this a try.

[nockieboy]

Darn typos.

Compiling fine now.  As for the triggers in the always_ff @(), I forgot to remove those when I was trying to find the source of the compile errors. D'oh.

All makes a bit more sense now. 

[BrianHG]

Again:

   if ( !rd_cache_valid || !rd_addr_valid ) begin  // check for cache miss on read address
   
      rd_cache_col   <= colour   ;
      rd_cache_bpp   <= bpp      ;
      rd_cache_bit   <= target   ;
      rd_cache_addr  <= ram_addr ; // cache new address
      rd_wait_a      <= 1'b1     ; // hold everything while we wait for data from RAM
      rd_req_a       <= 1'b1     ; // send 'rd_req_a' pulse
      rd_cache_valid <= 1'b0     ; // clear cache valid flag in case it wasn't already cleared
   
   end else begin

The guys in red are always set on a 'CMD_IN_PXCOPY', whether there is a cache hit or miss.
The guy in purple should only be set once the new data read is valid, however, the system as a whole should still be functional where you have it right now.  This one you can leave where it is.

Now, show me a complete simulation of multiple pixel reads.

[nockieboy]

I'm at the end of my time today - I'm not likely to get anything done tomorrow as I'm in meetings all day. 

Here's the current test setup for pixel_writer.  I've set up the schematic as best I can, but I've missed something obvious somewhere as the simulation isn't running properly (no data is returned from RAM). I'll maybe have some time tomorrow evening to take a look at what's up. 

[BrianHG]

Well, it seems to simulate for me.
Though, as you can see, I decomposed your command input so I can read what command I'm sending:



Now, when simulating, as you can see, the read_req_a is sent out, but, it stays on indefinitely and has a delay:



Now, as you can see, I've added output pins to tap the memory and read rdy flags.

In the attached .zip, I've also added the 'SDC1.sdc' file to tell Quartus that we need the design to at least function at 125MHz so the timing report will be accurate instead of all red.

In the simulation, I've changed your clock to 100MHz exact so it is easy to read and edit the stimulus inputs.

Your next task is to fix the logic of the 'read_req_a' so it has 0 delay and stays on only for 1 clock.

Oh, BTW, did I ever say we needed a 'next_cmd' output?  I don't remember.
Get rid of it unless for now you tie it to 'exec_cmd' just to see what it's doing.

[nockieboy]

Is this what you're after?



I suspect I'll need to add a few more constraints to the rd_req_a signal, some of the pixel commands won't require a read before every write in every screen mode, surely?

[BrianHG]

Ok, it's a start but this is your :

 rd_req_a       = exec_cmd && !last_exec_cmd  ;

What if there are 2 sequential reads?
What if the command is not a 'CMD_IN_PXCOPY'?
What if there is a valid cache hit?  (IE, no read req should be sent, the stored data should just be used)
What if we are in a reset state?

You need a few additional conditions on that line, then it should work.

    rd_cache_hit  = ( ram_addr == rd_cache_addr ) && rd_cache_valid ;
    wr_cache_hit  = ( ram_addr == wr_cache_addr ) && wr_cache_valid ;
    rd_req_a       = exec_cmd && !rd_cache_hit && (pixel_cmd[3:0] == CMD_IN_PXCOPY) && !reset;

[nockieboy]

What if there are 2 sequential reads?

No idea.

What if the command is not a 'CMD_IN_PXCOPY'?

See my previous comment:

I suspect I'll need to add a few more constraints to the rd_req_a signal, some of the pixel commands won't require a read before every write in every screen mode, surely?

I can't see why any of the pixel writes or copies wouldn't require a read before a write, unless we're operating in 16-bit colour mode when the entire 16-bit word would just be overwritten by the new word (but even then, only unless it's not writing in mask mode).

The NOP, RST (and SETARGB?) commands won't need a read either.  What's the neatest way to include those conditions in the assignment, though?  It's going to be a long line of AND's otherwise.  I guess I could create another signal that goes high if the command is one of those that don't need a read...

    rd_cache_hit  = ( ram_addr == rd_cache_addr ) && rd_cache_valid ;
    wr_cache_hit  = ( ram_addr == wr_cache_addr ) && wr_cache_valid ;
    rd_req_a       = exec_cmd && !rd_cache_hit && (pixel_cmd[3:0] == CMD_IN_PXCOPY) && !reset;

Ah - you hid the extra bit above and I didn't see it until I was writing my reply.  My question still stands, though - surely there's more commands that would need a read before write other than PXCOPY?

[BrianHG]

Go ahead and try this: (I'm sorry, but you must engineer this an inch at a time.  Getting 1 thing wrong means things going awry and you'll be in debugging nightmare and never get it to work right without creating code which generated numerous consistent wasted clock cycles slowing everything down by more than 50%.)



If your cache works, the second read command will do nothing.
Add another read at a new address and it should activate another read_req.

Now, there is 1 other thing the read command needs to do, generate a 16bit color pixel value which an adjacent potential 'CMD_IN_PXPASTE' / 'CMD_IN_PXPASTE_M' command will need to use to draw the copied color.

For now, make an output 'PX_COPY_COLOR[15:0]' and then you will need to engineer the necessary function to create that register.

Now, that function will need to operate in 2 ways, if there is a cache hit, it's source will be taken from the 'rd_data_cache' and computed immediately.  If there isn't a cache hit, the function will need to wait until the 'rd_data_rdy_a' comes in and it will generate the 'PX_COPY_COLOR' from the 'rd_data_in' instead since you would be wasting an additional clock cycle for the 'rd_data_cache' to update before you could compute the result.

Let's see how you solve this one...

[BrianHG]

Now, when I say 'Function', yes, I mean add another separate module inside the pixel_writer since you will be using this logic twice, once for the read/copy, and once again for all the different pixel write/paste commands.

Give the function a name like this:  bitplane_memory_data_to_pixel_color

It will be all combinational logic, no clocks, no reset, just a raw funct with a bunch of inputs and one 16 bit color output.

Inputs Hint:
ram_data_rdy
latched_color
latched_bpp
latched_target
immediate_color
immediate_bpp
immediate_target

Output:
pixel_color

[BrianHG]

Ah - you hid the extra bit above and I didn't see it until I was writing my reply.  My question still stands, though - surely there's more commands that would need a read before write other than PXCOPY?

No, not really.  Usually after a PXCOPY, a pixel paste/paste_m command would be next.
But were aren't there yet.  First finish the PXCOPY.

And I hid that extra red bit since your were supposed to figure that one out on your own, the:
&& (pixel_cmd[3:0] == CMD_IN_PXCOPY) && !reset;
Portion which would prevent the 'read_req_a' if the incoming command wasn't a PXCOPY or during a reset.

[nockieboy]

Go ahead and try this: (I'm sorry, but you must engineer this an inch at a time.  Getting 1 thing wrong means things going awry and you'll be in debugging nightmare and never get it to work right without creating code which generated numerous consistent wasted clock cycles slowing everything down by more than 50%.)

If your cache works, the second read command will do nothing.
Add another read at a new address and it should activate another read_req.

How's this?

Now, there is 1 other thing the read command needs to do, generate a 16bit color pixel value which an adjacent potential 'CMD_IN_PXPASTE' / 'CMD_IN_PXPASTE_M' command will need to use to draw the copied color.

For now, make an output 'PX_COPY_COLOR[15:0]' and then you will need to engineer the necessary function to create that register.

Now, that function will need to operate in 2 ways, if there is a cache hit, it's source will be taken from the 'rd_data_cache' and computed immediately.  If there isn't a cache hit, the function will need to wait until the 'rd_data_rdy_a' comes in and it will generate the 'PX_COPY_COLOR' from the 'rd_data_in' instead since you would be wasting an additional clock cycle for the 'rd_data_cache' to update before you could compute the result.

Let's see how you solve this one...

Working on this now...

[BrianHG]

 
So far so good!

[nockieboy]

Quick question - do I need to get rd_cache_valid set at the same time as rd_data_rdy_a goes high (instead of currently a clock later when it goes low again)?  Will need another wire and an AND gate if I do, I think.

Actually no, looks like it's better as it is as I can use the immediate values to return a colour from the bitplane_memory_data_to_pixel_colour module all the time rd_data_rdy_a is high, and the latched values when rd_cache_valid is high, and nothing at any other time?

[nockieboy]

Preliminary files attached for the new module (it's name is too long to keep writing ) and its instantiation in the pixel_writer module.

I've tweaked the name of a couple of the inputs to the new module - instead of latched_colour and immediate_colour, I've called them latched_word and immediate_word as I feel it's more descriptive.

Right, so now I've got to come up with some combinational logic to return the word/byte/nybble/crumb/bit from the supplied word, based on the bpp value.

The returned value will have to be 16 bits wide to accommodate returning whole words in true colour mode, for example.

Will go away and have a think about this for a bit.

[nockieboy]

Okay, is this along the right lines?

Code: [Select]

pixel_colour = ( ram_data_rdy ) ? immediate_word[( 15 - immediate_target ) : ( ( 15 - immediate_target ) - immediate_bpp )] : latched_word[( 15 - latched_target ) :( ( 15 - latched_target ) - latched_bpp )] ;

Assuming I'm on the right lines, the only issue with the above code that I'm aware of is the bit-width of the resultant value - pixel_colour is a 16-bit output and I'd need to pad the computed value with zeroes somehow, unless I'm completely off the table and into someone's pint of beer.

EDIT: Appears I'm in the beer.  Having real trouble making the code above work without errors - looks like Verilog doesn't like having non-constants on either side of the : when selecting a bit range... 

Simplified the above code to this (assuming I understand the -: properly):

Code: [Select]

pixel_colour = ( ram_data_rdy ) ? immediate_word[ immediate_target -: immediate_bpp ] : latched_word[ latched_target -: latched_bpp ] ;


[BrianHG]

I could not download your 'geo_pixel_writer.sv.txt '.  It said 'Atachment Not Found'.
However you have done it, perhaps in this case, maybe you should use 2 dimensional IF statements.
IE

IF (bpp[3:0]==BPP_16bit) begin

end else IF (bpp[3:0]==BPP_8bit) begin

end else IF (bpp[3:0]==BPP_4bit) begin

end else IF (bpp[3:0]==BPP_2bit) begin

end else IF (bpp[3:0]==BPP_1bit) begin

end

Inside each one IF (bpp[3:0]), make another IF (target[ x:x ] == ##) begin end else if (target[ x:x ] == ##)... exc ...
Remember, you pay attention to different x:x bits depending on the bpp size.

Also, when generating the color output, we want to multiply the colour[] input with the isolated pixel value so we shift the 1 bpp color pixel value to a selected new screen color if the output screen has 8bpp.

Take a look at 'bitplane_to_raster.v' in your main project.

[BrianHG]

Also, you are allowed to have multiple modules defined inside one xxxx.sv file.  This module could have been placed inside the 'geo_pixel_writer.sv' just after the line 'endmodule'.

In fact, for the geometry unit, we were going to place all the 'xxxx.sv' together into a single 'geometry_processor.sv' so you have a single complete module to interface with in Quartus.

[BrianHG]

Okay, is this along the right lines?

Code: [Select]

pixel_colour = ( ram_data_rdy ) ? immediate_word[( 15 - immediate_target ) : ( ( 15 - immediate_target ) - immediate_bpp )] : latched_word[( 15 - latched_target ) :( ( 15 - latched_target ) - latched_bpp )] ;

Assuming I'm on the right lines, the only issue with the above code that I'm aware of is the bit-width of the resultant value - pixel_colour is a 16-bit output and I'd need to pad the computed value with zeroes somehow, unless I'm completely off the table and into someone's pint of beer.

EDIT: Appears I'm in the beer.  Having real trouble making the code above work without errors - looks like Verilog doesn't like having non-constants on either side of the : when selecting a bit range... 

Simplified the above code to this (assuming I understand the -: properly):

Code: [Select]

pixel_colour = ( ram_data_rdy ) ? immediate_word[ immediate_target -: immediate_bpp ] : latched_word[ latched_target -: latched_bpp ] ;


Ok, you are attacking the problem again in a different way than what I was expecting.

You are correct that depending on the (ram_data_rdy), there is a selection of which source data becomes relevant. 

However, depending if you have already noticed, there is a little trick inside here.  We will see what happens when you simulate and output the read pixel color.

Ok, now, look at my 'IF' code above, since I want the code to look like that which narrows down the window and ignores erroneous bpp settings, you will need some intermediate variables from the above 'muxs' to feed this section to generate the 'read pixel color' output.


Your graphic in this above post https://www.eevblog.com/forum/fpga/fpga-vga-controller-for-8-bit-computer/msg3176276/#msg3176276 is now corrupt.  Re-uplodd it if you still have it.

[nockieboy]

Have re-uploaded the graphic you said was corrupted.  Should be working now.

Also re-uploading my most recent geo_pixel_writer and bitplane_memory_data_to_pixel_colour modules.

[nockieboy]

Also, you are allowed to have multiple modules defined inside one xxxx.sv file.  This module could have been placed inside the 'geo_pixel_writer.sv' just after the line 'endmodule'.

In fact, for the geometry unit, we were going to place all the 'xxxx.sv' together into a single 'geometry_processor.sv' so you have a single complete module to interface with in Quartus.

Aha, yes, I was getting a little trigger-happy with the new modules.    Have moved it into geo_pixel_writer.sv module now. 

EDIT: Seems the forum is hiccuping and not allowing uploads... here's the latest code from my pixel_writer module:

Code: [Select]

module bitplane_memory_data_to_pixel_colour (

// *** INPUTS
    input logic         ram_data_rdy,
    input logic  [15:0] latched_word,
    input logic  [3:0]  latched_bpp,
    input logic  [3:0]  latched_target,
    input logic  [15:0] immediate_word,
    input logic  [3:0]  immediate_bpp,
    input logic  [3:0]  immediate_target,

// *** OUTPUTS
    output logic [15:0] pixel_colour
   
);

// these params are not final - I've just thrown them together for testing
localparam BPP_16bit = 4'b1000;
localparam BPP_8bit  = 4'b0100;
localparam BPP_4bit  = 4'b0010;
localparam BPP_2bit  = 4'b0001;
localparam BPP_1bit  = 4'b0000;

logic [3:0]  source_bpp    ;
logic [3:0]  source_target ;
logic [15:0] source_word   ;

always_comb begin

    // output colour from latched values all the time ram_data_rdy is low
    // otherwise output colour from immediate values
   
    //pixel_colour = ( ram_data_rdy ) ? immediate_word[ immediate_target -: immediate_bpp ] : latched_word[ latched_target -: latched_bpp ] ;
   
    // set source data according to RAM read
    source_bpp    = ( ram_data_rdy ) ? immediate_bpp    : latched_bpp    ;
    source_target = ( ram_data_rdy ) ? immediate_target : latched_target ;
    source_word   = ( ram_data_rdy ) ? immediate_word   : latched_word   ;
   
    if ( source_bpp[3:0] == BPP_16bit ) begin
   
        if ( source_target[3:0] == 1'b0 ) begin
       
            pixel_colour = source_word ;
       
        end

    end else if ( source_bpp[3:0] == BPP_8bit ) begin
   
        if ( source_target[3:0] == 0 ) begin
       
            pixel_colour = source_word[15:8] ;
       
        end else if ( source_target[3:0] == 1 ) begin
       
            pixel_colour = source_word[7:0]  ;
       
        end

    end else if ( source_bpp[3:0] == BPP_4bit ) begin

    end else if ( source_bpp[3:0] == BPP_2bit ) begin

    end else if ( source_bpp[3:0] == BPP_1bit ) begin

    end
   
end

endmodule
However, this doesn't compile.  I'm getting the following error:

Error (10166): SystemVerilog RTL Coding error at geo_pixel_writer.sv(312): always_comb construct does not infer purely combinational logic

[nockieboy]

Latest pixel_writer.  Hopefully the colour module is progressing along the right lines.  I'm not sure I completely understand what you're after here:

Also, when generating the color output, we want to multiply the colour[] input with the isolated pixel value so we shift the 1 bpp color pixel value to a selected new screen color if the output screen has 8bpp.

Take a look at 'bitplane_to_raster.v' in your main project.

Had a look at bitplane_to_raster, but unless you smack me in the mouth with it, it's not springing out at me.