FPGA VGA Controller for 8-bit computer

[BrianHG]

Ok, change the 'always_comb' to 'always'.  I believe system verilog is expecting an 'always_ff' so that it can latch something from the 'always_comb' structure.

Also:

Code: [Select]

        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
You need to assign the unused 'pixel_colour' bits to a definite value to get rid of the warnings galore..

Also, what happened to the source:
rd_cache_col   <= colour   ;

IE, inputs:
latched_colour.
&
immediate_colour.

These inputs, now muxed to 'source_colour' should be multiplied with the 'source_word' when lower than 16 bit color.  In 16 bit color, we may change this one case so that the 'source_colour' may be a brightness or contrast setting instead of simple multiply.

Also, in the pixel_writer module, the 'PX_COPY_COLOUR' output will be latched by a clock before it feeds an output pin, however, for now, keep it as is for simulating.

Next, add the other bitplanes and correct your current code as the target is wrong and you need to define the missing bits of pixel colour & perform that multiply.

[nockieboy]

Ok, change the 'always_comb' to 'always'.  I believe system verilog is expecting an 'always_ff' so that it can latch something from the 'always_comb' structure.

Also:

Code: [Select]

        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
You need to assign the unused 'pixel_colour' bits to a definite value to get rid of the warnings galore..

Yes, I found out what the problem was - every outcome of the IF conditional needs to drive pixel_colour to a value.  It's working now as an 'always_comb'.

Also, what happened to the source:
rd_cache_col   <= colour   ;

IE, inputs:
latched_colour.
&
immediate_colour.

These inputs, now muxed to 'source_colour' should be multiplied with the 'source_word' when lower than 16 bit color.  In 16 bit color, we may change this one case so that the 'source_colour' may be a brightness or contrast setting instead of simple multiply.

Ah.  That stemmed out of confusion about what they were for.  I did ask about this in a previous post - I had reworded then to latched_word and immediate_word instead, but thinking about it, we won't be passing a word into the function, we're just reading, so I'll rename them back and hope it doesn't confuse things too much.

We're still going to need a 16-bit input into the function for it to extract the pixel data from, though, so instead of 'renaming it back', I'll just add in the latched_ and immediate_ colour inputs, if that's right?

Where does the colour value come from the read RAM?  I'll be caching the 16-bit word that's read from RAM, not any particular colour value?

Next, add the other bitplanes and correct your current code as the target is wrong and you need to define the missing bits of pixel colour & perform that multiply.

The target's wrong? How so?

As for the colour - I'm still not sure about this multiplication?  Why are we changing the read data for a copy pixel operation?  Are you saying that I should just literally multiply the source_word by the source_colour? 

[BrianHG]

Arrrrrrrrggggg...
Look at the command structure in the pixel_writer.sv
Is there not a colour value sent with every command?

Look at the simulation stimuli, is there not a cmd_color[7:0] input?

This is not the memory pixel which was read, it's the color setting provided in the cmd_color[7:0] function.

Tell me, if you are copying a 1 bit B&W color source bitmap memory onto a 256 color screen, with nothing but a dumb copy, your output will always be 0 & 1 on a 0-255 on the destination screen.  Don't you want some sort of flexibility on the output color here?

Also take a look at the address generator output here: https://www.eevblog.com/forum/fpga/fpga-vga-controller-for-8-bit-computer/msg3165360/#msg3165360

 and redo the sim with some different bitplane figures so you may correct all your 'if ( source_target[3:0] == 0 ) begin' mistakes.

HINT: Read the output pixel coordinates and read the generated address and generated target#.  You will see that all the targets bits don't always matter and they are upside-down...

[nockieboy]

Arrrrrrrrggggg...

I get that a lot.

Also take a look at the address generator output here: https://www.eevblog.com/forum/fpga/fpga-vga-controller-for-8-bit-computer/msg3165360/#msg3165360

 and redo the sim with some different bitplane figures so you may correct all your 'if ( source_target[3:0] == 0 ) begin' mistakes.

HINT: Read the output pixel coordinates and read the generated address and generated target#.  You will see that all the targets bits don't always matter and they are upside-down...

Been busy here, it's hot as well which isn't all that helpful, but I've got the following simulation output:



The PX_COPY_COLOUR output appears to be delayed by a clock - but I guess that's because I'm using the output pin which is a register output from the pixel_writer module.

[BrianHG]

Funny, my version has 0 delay, see here:



As you can see, the with the red cache miss, the same cycle the rd_data_ready comes in, the PX_COPY_COLOUR data has the right word.  During the green cache hit, the PX_COPY_COLOUR has the new correct colour value immediately during the same clock.

First few little issues, please stop using binary when we assigned numerical values to our parameters like bpp:

Code: [Select]

// these params are not final - I've just thrown them together for testing
localparam BPP_16bit = 4'd15;
localparam BPP_8bit  = 4'd7;
localparam BPP_4bit  = 4'd3;
localparam BPP_2bit  = 4'd1;
localparam BPP_1bit  = 4'd0;
Your binary figures were all wrong.

Also, this looks a little backwards:

Code: [Select]

    end else begin // assume BPP_1bit
   
pixel_colour = source_word[ ( source_target[3:0] ) ] ;  // only need to return 1 bit
   
    end

Now, you first need to figure out why your 'PX_COPY_COLOUR' seems to have a delay.  I did warn/give you a hint a few posts back with the latched and immediate.  Copy my simulation stimuli to help.

Once that works, you will need to add to you simulation stimuli the following to uncover another bug.  First only add the part I have in green an check for bugs.  There will be one.  Once fixed, add in the yellow then simulate for a full run.



I hope this teaches you that you need to work and simulate in tiny bits with FPGA code unless you are an expert.  The code you are engineering has to perform exactly otherwise the results will be all out of place and crap.  The memory bitplane color decoder will also be used by the all the pixel writer commands so it may read a pixel and edit just the correct color bits and then write that back to the same ram address.

Once your sim is done, we will add the 'cmd_color' feature to the copy pixel command.

[nockieboy]

Now, you first need to figure out why your 'PX_COPY_COLOUR' seems to have a delay.  I did warn/give you a hint a few posts back with the latched and immediate.  Copy my simulation stimuli to help.

Ok, change the 'always_comb' to 'always'.  I believe system verilog is expecting an 'always_ff' so that it can latch something from the 'always_comb' structure.

Also, in the pixel_writer module, the 'PX_COPY_COLOUR' output will be latched by a clock before it feeds an output pin, however, for now, keep it as is for simulating.

I don't see where it could be latched now?  I've specified wires for the PX_COPY_COLOUR output, so that clock delay should be removed?

Nope - I've spent an hour combing the code to find out what's causing this delay, have specified wires instead of logic, always instead of always_comb, I'm clearly missing something obvious and now my frustration is blinding me to the possible cause.   

[BrianHG]

That delay has nothing to do with the 'always_comb'.  That part you have correct.

Think carefully, what's available when either you are using data coming from the ram, or data that is in the cached byte...  When is each usable byte coming from each point valid under each circumstance?

Code: [Select]

    // set source data according to RAM read
    source_bpp    = ( ram_data_rdy ) ? immediate_bpp    : latched_bpp    ;
    source_colour = ( ram_data_rdy ) ? immediate_colour : latched_colour ;
    source_target = ( ram_data_rdy ) ? immediate_target : latched_target ;
    source_word   = ( ram_data_rdy ) ? immediate_word   : latched_word   ;

Make sure your simulation stimuli matches mine in my previous post as well.  Maybe you are seeing confusing results.

[nockieboy]

Think carefully, what's available when either you are using data coming from the ram, or data that is in the cached byte...  When is each usable byte coming from each point valid under each circumstance?

Well, rd_data_rdy_a is either HIGH or LOW.  If it's low, we should be using the latched values, if it's high we use the immediate values.  The immediate values are all piped into the function using wires, so I can't see any delay there?  I'm looking at immediate values being the issue currently, so am not overly concerned with the latched values yet.

Make sure your simulation stimuli matches mine in my previous post as well.  Maybe you are seeing confusing results.

It matches yours - except your PX_COPY_COLOUR seems to be labelled PC_COPY_COL...   

Dealing with a family bereavement now (of the pet variety) so I won't be back for a while. 

[BrianHG]

Well, rd_data_rdy_a is either HIGH or LOW.  If it's low, we should be using the latched values, if it's high we use the immediate values.  The immediate values are all piped into the function using wires, so I can't see any delay there?  I'm looking at immediate values being the issue currently, so am not overly concerned with the latched values yet.

Ok, I need you to think this through like your life depended on it...

If we are not using the 'rd_data_rdy_a', meaning that the command has immediately come in from the cmd and the memory data source is coming from the latched value, what should be your selection here?

Now, if we are getting the data from the 'rd_data_rdy_a', meaning that the cmd was sent earlier, hence the original cmd controls were sent and lost to the next cmd ones, but now, the source memory data is available at this point in time immediately.  How will your selection switches look in this setup?

[nockieboy]

If we are not using the 'rd_data_rdy_a', meaning that the command has immediately come in from the cmd and the memory data source is coming from the latched value, what should be your selection here?

Now, if we are getting the data from the 'rd_data_rdy_a', meaning that the cmd was sent earlier, hence the original cmd controls were sent and lost to the next cmd ones, but now, the source memory data is available at this point in time immediately.  How will your selection switches look in this setup?

Ah daaang...   

The other cmd settings - colour, bits_per_pixel, width - are all lost when the cmd clears after the first clock.  If the module is checking the same address as previously, the cached settings are used, but if it has to read RAM and wait for the result, it uses the 'immediate' values when the RAM comes back with data.  Problem is, the colour, bits_per_pixel and width settings are all lost by this point so the PX_COPY_COLOUR is garbage.

I've switched the 'immediate' data feeds to using the cached settings instead of the 'immediate' (invalid) ones and am getting this simulation output:

[BrianHG]

What did you do?
Your simulation is still wrong.
It doesn't look anything like mine.

Why did you change these?  These were correct originally:

Code: [Select]

    .latched_word     ( rd_data_cache[15:0] ), // 16-bit word from the catch
    .latched_colour   ( rd_cache_col[7:0]   ), // 8-bit cached colour value
    .latched_bpp      ( rd_cache_bpp[3:0]   ), // cached bits-per-pixel value
    .latched_target   ( rd_cache_bit[3:0]   ), // cached target word/byte/nybble/crumb/bit
   
    .immediate_word   ( rd_data_in[15:0]    ), // 16-bit word from GPU RAM
    .immediate_colour ( rd_cache_col[7:0]   ), // current colour value
    .immediate_bpp    ( rd_cache_bpp[3:0]   ), // current bits-per-pixel value
    .immediate_target ( rd_cache_bit[3:0]   ), // current target word/byte/nybble/crumb/bit

You just made things 10x worse for you.
Why did you not concentrate on my words and this block?:

Code: [Select]

    // set source data according to RAM read
    source_bpp    = ( ram_data_rdy ) ? immediate_bpp    : latched_bpp    ;
    source_colour = ( ram_data_rdy ) ? immediate_colour : latched_colour ;
    source_target = ( ram_data_rdy ) ? immediate_target : latched_target ;
    source_word   = ( ram_data_rdy ) ? immediate_word   : latched_word   ;

     Maybe think of why I called these 'LATCHED' and 'IMMEDIATE'.  Think of what you wrote in your own post above.  Every single clue is there for you to solve.

[nockieboy]

What did you do?

I changed this:

Code: [Select]

bitplane_memory_data_to_pixel_colour get_pixel_colour_1 (
    .ram_data_rdy     ( rd_data_rdy_a       ), // use immediate values when HIGH, latched values when LOW
   
    .latched_word     ( rd_data_cache[15:0] ), // 16-bit word from the catch
    .latched_colour   ( rd_cache_col[7:0]   ), // 8-bit cached colour value
    .latched_bpp      ( rd_cache_bpp[3:0]   ), // cached bits-per-pixel value
    .latched_target   ( rd_cache_bit[3:0]   ), // cached target word/byte/nybble/crumb/bit
   
    .immediate_word   ( rd_data_in[15:0]    ), // 16-bit word from GPU RAM
    .immediate_colour ( colour[7:0]         ), // current colour value
    .immediate_bpp    ( bpp[3:0]            ), // current bits-per-pixel value
    .immediate_target ( target[3:0]         ), // current target word/byte/nybble/crumb/bit
   
    .pixel_colour     ( PX_COPY_COLOUR )       // current pixel colour value from above parameters
);

... to this:

Code: [Select]

bitplane_memory_data_to_pixel_colour get_pixel_colour_1 (
    .ram_data_rdy     ( rd_data_rdy_a       ), // use immediate values when HIGH, latched values when LOW
   
    .latched_word     ( rd_data_cache[15:0] ), // 16-bit word from the catch
    .latched_colour   ( rd_cache_col[7:0]   ), // 8-bit cached colour value
    .latched_bpp      ( rd_cache_bpp[3:0]   ), // cached bits-per-pixel value
    .latched_target   ( rd_cache_bit[3:0]   ), // cached target word/byte/nybble/crumb/bit
   
    .immediate_word   ( rd_data_in[15:0]    ), // 16-bit word from GPU RAM
    .immediate_colour ( rd_cache_col[7:0]         ), // current colour value
    .immediate_bpp    ( rd_cache_bpp[3:0]            ), // current bits-per-pixel value
    .immediate_target ( rd_cache_bit[3:0]         ), // current target word/byte/nybble/crumb/bit
   
    .pixel_colour     ( PX_COPY_COLOUR )       // current pixel colour value from above parameters
);

... for the reasons outlined in my post.  I haven't had a lot of time this weekend to spend on this and I feel like I've been fumbling for solutions and going round in circles, wasting time.

Your simulation is still wrong.
It doesn't look anything like mine.

Yep, I realise that now that I've compared the two.  Back when I ran the simulation earlier today I saw the delay had gone and thought, "Eureka!"  Clearly it was premature and I should have waited to check the results in more detail, but I didn't have time.

Why did you change these?  These were correct originally:

Code: [Select]

    .latched_word     ( rd_data_cache[15:0] ), // 16-bit word from the catch
    .latched_colour   ( rd_cache_col[7:0]   ), // 8-bit cached colour value
    .latched_bpp      ( rd_cache_bpp[3:0]   ), // cached bits-per-pixel value
    .latched_target   ( rd_cache_bit[3:0]   ), // cached target word/byte/nybble/crumb/bit
   
    .immediate_word   ( rd_data_in[15:0]    ), // 16-bit word from GPU RAM
    .immediate_colour ( rd_cache_col[7:0]   ), // current colour value
    .immediate_bpp    ( rd_cache_bpp[3:0]   ), // current bits-per-pixel value
    .immediate_target ( rd_cache_bit[3:0]   ), // current target word/byte/nybble/crumb/bit

You just made things 10x worse for you.

I think I explained "why" pretty well in my previous post?   I clearly don't understand enough about the hardware or HDL subtleties to be able to see the problem.  How long have you worked with this stuff?  You must be qualified and have gone to university and studied EE or something similar, not to mention however many years working in the industry?  I obviously don't have that training or experience (other than what you've been patient enough to teach me). 

Why did you not concentrate on my words and this block?:

Code: [Select]

    // set source data according to RAM read
    source_bpp    = ( ram_data_rdy ) ? immediate_bpp    : latched_bpp    ;
    source_colour = ( ram_data_rdy ) ? immediate_colour : latched_colour ;
    source_target = ( ram_data_rdy ) ? immediate_target : latched_target ;
    source_word   = ( ram_data_rdy ) ? immediate_word   : latched_word   ;

     Maybe think of why I called these 'LATCHED' and 'IMMEDIATE'.  Think of what you wrote in your own post above.  Every single clue is there for you to solve.

Well that block - to me - is a closed book.  ram_data_rdy is a wire (rd_data_rdy_a, in fact) - there's no delay on it that I'm aware of.  So the source_ values are assigned instantly to either their immediate values or their latched values, depending on whether ram_data_rdy is HIGH or LOW.

According to my limited understanding of the subject and my internal logic, there is an issue with the immediate values - not the latched values.  That could be my first mistake, but I consider it a fair assumption based on what I know.  My previous thinking was that there is no guarantee that the immediate_colour, target or bpp will have valid values by the time ram_data_rdy goes high as they all become invalid once cmd_rdy goes LOW.  In fact, all their values go to zero. 

In fact, this appears to be what is happening in the simulation - when rd_data_rdy_a goes HIGH, the immediate values (all zero) for color, target and bpp are used on the incoming data word from RAM, and a valid value isn't returned by the function until rd_data_rdy_a goes LOW and the inputs are switched over to the latched values instead, which have all the correct settings saved and thus a valid value is returned in PX_COPY_COLOUR.

I'm still not 100% on why you say my previous change was wrong, because of my thinking above.

Now I'm clearly missing a connection or I'm not making a link because I can't see any further than that.  I'm just staring at waveforms and numbers and code and it's all just staring back at me.

I'm going to bed.  Maybe sleeping on the problem (again) will help.

[BrianHG]

Ok, I give up, see if you can figure out and explain to me why this works.

Code: [Select]

    // set source data according to RAM read
    source_bpp    = ( !ram_data_rdy )  ? immediate_bpp    : latched_bpp    ;
    source_colour = ( !ram_data_rdy )  ? immediate_colour : latched_colour ;
    source_target = ( !ram_data_rdy )  ? immediate_target : latched_target ;
    source_word   = (  ram_data_rdy )  ? immediate_word   : latched_word   ;
Once you understand that, next copy the green part of my stimuli on my second sim setup with the green and yellow patch.  There is a minor bug there and once you solve it, adding the yellow section running 6 adjacent commands should simulate fine.


BTW, I'm self taught.  No schooling in this field.  However, yes, I've been working in electronics since the mid 80's when I began it as a hobby.

[nockieboy]

Ok, I give up, see if you can figure out and explain to me why this works.

Code: [Select]

    // set source data according to RAM read
    source_bpp    = ( !ram_data_rdy )  ? immediate_bpp    : latched_bpp    ;
    source_colour = ( !ram_data_rdy )  ? immediate_colour : latched_colour ;
    source_target = ( !ram_data_rdy )  ? immediate_target : latched_target ;
    source_word   = (  ram_data_rdy )  ? immediate_word   : latched_word   ;

For crying out loud...   Why do I always go straight to the complicated solutions?  I wasn't even considering changing the polarity of the ram_data_rdy signal.  The solution is always obvious when you can see it.

Once you understand that, next copy the green part of my stimuli on my second sim setup with the green and yellow patch.  There is a minor bug there and once you solve it, adding the yellow section running 6 adjacent commands should simulate fine.

Right.  Let's get off on the right foot with this one.  Treat me as if I'm a complete imbecile as I'm not even sure what the bug is!   I've worked out the values on paper for PX_COPY_COLOUR for the green section and I'm getting results of 4 and 5 for the first two read requests, as they are output in the simulation when rd_data_rdy_a goes high.  So what's the issue?  Is it because the function is outputting garbage the rest of the time or because rd_req_a is extended?

I need to know I'm focusing my full cognitive ability (like a bag of cats) on the bug and not some harmless (or intended) feature that I think is a bug because I'm going stir crazy looking at simulation outputs.

[BrianHG]

Yep, next step, change this line:

Code: [Select]

    exec_cmd       = ( ( !(rd_wait_a && !rd_data_rdy_a) && !(rd_wait_b && !rd_data_rdy_b) ) && pixel_cmd_rdy ) ;
to this:

Code: [Select]

    exec_cmd       = ( ( !(rd_wait_a ) && !(rd_wait_b ) ) && pixel_cmd_rdy ) ;
The '!rd_data_rdy_a' was causing the fifo to begin reading too early.

Do a simulate with first my green, then yellow added stimuli.  The cache hits should now stream out with valid pixel data right after the read.

Now, I think we should add a new output to the 'bitplane_memory_data_to_pixel_colour' called:
color_eq_0

the equation is simple:
color_eq_0 = ( pixel_colour == 0 );


Now, before the 'end // else reset' we want to add a smart register for the 'PX_COPY_COLOUR' and the new 'PX_COPY_COLOUR_eq0' , do this:  (We need to hold onto the results for when a PXPASTE/_M commands comes in)

Code: [Select]

          if ( !rd_data_rdy_a || (exec_cmd && (pixel_cmd[3:0]==CMD_IN_PXCOPY) && rd_cache_hit) ) begin  // if pixel color data came in from ram or was immediately available when the command came in and there was a cache hit
                            PX_COPY_COLOUR_reg     <= PX_COPY_COLOUR ;     // Store pixel color
                            PX_COPY_COLOUR_eq0_reg <= PX_COPY_COLOUR_eq0 ; // store the color =0
          end


Last, we need to add the color translation/conversion function to the 'bitplane_memory_data_to_pixel_colour' module.

The formula is simple, output pixel_colour should now equal:

pixel_colour = pixel_colour * source_colour; // give the option fot the pixel copy command to change the read color to a new larger color.

However, you cannot do this with combinational logic.  Without any latch, pixel_colour would be assigned twice, so, rename and replace all the other 'pixel_colour' in that section with 'int_pixel_colour' and add a:

logic [15:0] int_pixel_colour;

and

pixel_colour = int_pixel_colour * source_colour; // give the option fot the pixel copy command to change the read color to a new larger color.


With the other logic s in that module.

Redoo the simulations, and check that the new PX_COPY_COLOUR_reg holds the read and that the changing the source CMD_color from 1 to other values alters the read pixel value.

If everything is good, we have 1 new addition to the 'bitplane_memory_data_to_pixel_colour' for another color transformation from multiply to XOR.  Then, we will do the pixel write command.

[nockieboy]

Redoo the simulations, and check that the new PX_COPY_COLOUR_reg holds the read and that the changing the source CMD_color from 1 to other values alters the read pixel value.

Is this right?  I've highlighted the PX_COPY_COLOUR values that have changed compared to the previous simulation results (i.e. with cmd_colour set to 1 all the way through):

[BrianHG]

Does this help you see it:



If you used a CMD_Colour of always 1, it may have been easier to see the output colors...

Now, output the 'PX_COPY_COLOUR_reg' instead.  It should be delayed by an additional clock.

Next, all of the pixel writing routines.

(1 weird thing, why it you 'CMD_COLOR' input showing a single digit instead of 01, 04, 02,...)

[BrianHG]

There was a typo in my above line#263.

 if ( !rd_data_rdy_a || ( exec_cmd && ( pixel_cmd[3:0] == CMD_IN_PXCOPY ) && rd_cache_hit ) ) begin

Get rid of that !, the line should read:

if ( rd_data_rdy_a || ( exec_cmd && ( pixel_cmd[3:0] == CMD_IN_PXCOPY ) && rd_cache_hit ) ) begin

Now if you output your 'PX_COPY_COLOUR_reg', it will have the right data at the right time.

Arrrg, now we hit an FMAX issue where this module only wants to operate at 100MHz max.  I've got a little bit of work to do to find the best fix.

[BrianHG]

Ok, found the FMAX bottleneck, it's line #324:

    pixel_colour  = int_pixel_colour * source_colour ; // give the option for the pixel copy command to change the read color to a new larger color

For now, change it to :

    pixel_colour  = int_pixel_colour ; //  * source_colour ; // We will move this function's feature into the pixel writer to increase the FMAX.

And now you will need to add an output to the 'bitplane_memory_data_to_pixel_colour' called:
    output logic [7:0] source_color;

And add the appropriate initiation of the function + add this new green line here:

      if ( !rd_data_rdy_a || ( exec_cmd && ( pixel_cmd[3:0] == CMD_IN_PXCOPY ) && rd_cache_hit ) ) begin
         // if pixel color data came in from RAM or was immediately available when the command came in and there was a cache hit
         PX_COPY_COLOUR_reg <= PX_COPY_COLOUR ;     // Store pixel color
         PX_COPY_COLOUR_eq0 <= PX_COPY_COLOUR_eq0 ; // store the color =0
         PX_COPY_COLOUR_opt_reg  <= PX_COPY_COLOUR_opt ; // store the CMD_COLOR option
      end

Send me your updated .sv to verify.
Also now pass out the 'PX_COPY_COLOUR_reg' instead of the regular 'PX_COPY_COLOUR'.

[nockieboy]

(1 weird thing, why it you 'CMD_COLOR' input showing a single digit instead of 01, 04, 02,...)

Erm... probably because I have it set to 'unsigned decimal'.

Ok, found the FMAX bottleneck, it's line #324:

    pixel_colour  = int_pixel_colour * source_colour ; // give the option for the pixel copy command to change the read color to a new larger color

For now, change it to :

    pixel_colour  = int_pixel_colour ; //  * source_colour ; // We will move this function's feature into the pixel writer to increase the FMAX.

And now you will need to add an output to the 'bitplane_memory_data_to_pixel_colour' called:
    output logic [7:0] source_color;

And add the appropriate initiation of the function + add this new green line here:

      if ( !rd_data_rdy_a || ( exec_cmd && ( pixel_cmd[3:0] == CMD_IN_PXCOPY ) && rd_cache_hit ) ) begin
         // if pixel color data came in from RAM or was immediately available when the command came in and there was a cache hit
         PX_COPY_COLOUR_reg <= PX_COPY_COLOUR ;     // Store pixel color
         PX_COPY_COLOUR_eq0 <= PX_COPY_COLOUR_eq0 ; // store the color =0
         PX_COPY_COLOUR_opt_reg  <= PX_COPY_COLOUR_opt ; // store the CMD_COLOR option
      end

Send me your updated .sv to verify.
Also now pass out the 'PX_COPY_COLOUR_reg' instead of the regular 'PX_COPY_COLOUR'.

Okay, I got a little lost in these instructions so I've attached the entire project, just in case. 

EDIT:  Oh jeez, I've just scanned the code again and realised that source_colour is already a wire in the system - I've now created it as an input at your request - will that cause issues or was that intended?  I've also had to infer that PX_COPY_COLOUR_opt is connected to source_colour from the function.  Seems like there could be a number of errors in that project I posted.

[BrianHG]

Ok, I fixed up a few minor glitches on your part.

I made these outputs:

Code: [Select]

    output logic [15:0] PX_COPY_COLOUR,
    output logic [15:0] PX_COPY_COLOUR_reg,
    output logic [7:0]  PX_COPY_COLOUR_opt_reg
Also, this is 8 bits, not 16 bits:

Code: [Select]

logic [7:0] PX_COPY_COLOUR_opt     ;
Fixed the simulation and grouped the individual 'BIT' buses at the bottom.
I've attached the update.

Next, writing pixels.  For this one, I'll have a good explanation in an hour or 2...

[BrianHG]

Ok, all the pixel write CMD.

Step 1, they must read the current CMD memory address unless it's a cache hit, and check if the current chosen pixel was not a 0, otherwise you need to increment the 'rd_px_collision_counter' and you might need to increment the 'wr_px_collision_counter' if you have incremented the rd collision and the color you are writing isn't a transparent color 0 when the transparent mask '_M' CMD feature is enabled.

Step 2, change the appropriate bits in the write word cache and send out a write req & new write data.

Step 2.5 Now, if the copy pixel cache address and the write pixel cache address matches, when a write is done, the copy pixel cache will need to be updated with a copy of the data written to the ram.

The 3 above steps needs to appropriately set and clear the write busy flag so that a write may take place without a read.


Once we get this simulating and working, you will need to combine this module with the geometry unit in a new project under a single .sv call geometry_processor.sv which calls all 3 sub geometry units.  That  geometry_processor.sv will be wired into your GPU project and you will need to write some software to test the first graphic line.

[BrianHG]

Step A, like the module 'bitplane_memory_data_to_pixel_colour', you now need a another module:

memory_pixel_bits_editor

Notes:

Code: [Select]

Inputs:

    input logic         ram_data_rdy,
    input logic  [15:0] latched_word,
    input logic  [7:0]  latched_colour,
    input logic  [3:0]  latched_bpp,
    input logic  [3:0]  latched_target,
    input logic  [15:0] immediate_word,
    input logic  [7:0]  immediate_colour,
    input logic  [3:0]  immediate_bpp,
    input logic  [3:0]  immediate_target,

    input logic         paste_enable,
    input logic  [15:0] paste_colour,
    input logic         transparent_mask_enable,


Outputs

    output logic        collision_rd_inc,
    output logic        collision_wr_inc,
    output logic [15:0] output_word

Local params:
// these params are not final - I've just thrown them together for testing
localparam BPP_16bit = 4'd15;
localparam BPP_8bit  = 4'd7;
localparam BPP_4bit  = 4'd3;
localparam BPP_2bit  = 4'd1;
localparam BPP_1bit  = 4'd0;

Other logic...
logic [3:0]  source_bpp       ;
logic [15:0] source_colour    ;
logic [3:0]  source_target    ;
logic [15:0] source_word      ;
logic [15:0] target_colour    ;
logic        target_colour_0  ;
logic [15:0] edited_word ;

you must already know this part:

source_color[15:8] = 0 ; // Assign 0 to the upper 8 bits of the source color.

    // set source data according to RAM read
    source_bpp            = ( !ram_data_rdy ) ? immediate_bpp    : latched_bpp    ;
    source_colour[7:0] = ( !ram_data_rdy ) ? immediate_colour : latched_colour ;
    source_target         = ( !ram_data_rdy ) ? immediate_target : latched_target ;
    source_word          = (  ram_data_rdy ) ? immediate_word   : latched_word   ;

    target_colour        =  ( paste_enable )  ?   paste_colour : source_colour ;
    target_colour_0    =   (target_colour == 0) ;
    collision_wr_inc   = collision_rd_inc && (!target_colour_0 || !transparent_mask_enable);

    output_word        = (transparent_mask_enable && target_colour_0) ? source_word : edited_word ; // choose output word based on transparency mask.


and then:

    if ( source_bpp[3:0] == BPP_16bit ) begin
       
                                    edited_word        = target_colour            ;
                                    collision_rd_inc   = (source_word[15:0]  !=0) ;

    end else if ( source_bpp[3:0] == BPP_8bit ) begin
   
       if (source_target[0]) begin                                                  // place target color into the first 8 bits and retain the upper 8 bits.
                                    edited_word[15:8]  = target_colour[7:0]       ;
                                    edited_word[7:0]   = source_word[7:0]         ;
                                    collision_rd_inc   = (source_word[15:8]  !=0) ;

       end else begin                                                               // place target color into the upper 8 bits and retain the lower 8 bits.
                                    edited_word[7:0]   = target_colour[7:0]       ;
                                    edited_word[15:8]  = source_word[15:8]        ;
                                    collision_rd_inc   = (source_word[7:0]   !=0) ;
       end

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

if ( source_target[1:0] == 0 ) begin        // place target color into the first 4 bits and retain the upper 12 bits.
                                    edited_word[15:12] = target_colour[3:0]       ;
                                    edited_word[11:0]  = source_word[11:0]        ;
                                    collision_rd_inc   = (source_word[15:12] !=0) ;

    end else if ( source_target[1:0] == 1 ) begin        // place target color into the second 4 bits and retain the upper 8 bits and lower 4 bits.
                                    edited_word[11:8]  = target_colour[3:0]      ;
                                    edited_word[15:12] = source_word[15:12]      ;
                                    edited_word[7:0]   = source_word[7:0]        ;
                                    collision_rd_inc   = (source_word[11:8] !=0) ;


You should be able to figure out the rest...

Send me an updated code with the inserted pixel write CMD as far as you can work out.

[nockieboy]

Okay, I'm posting the 'code so far' so you can pull me up if I'm heading in the wrong direction.

Quick question about the new module - BPP_1bit is clearly a special case, I'm not sure if I'm handling it correctly.  I haven't tried to synthesise yet as the rest of the code is incomplete, but SystemVerilog seems to have a bit of a fit when it comes to variable bit-selection.

What I've done is set the target bit in the edited word - that's easy enough - but I need to set the bits above and below the targeted bit to the same value as their counterpart in the source word.  If the targeted bit is 15 or 0, a special case arises, so I've handled it like this:

Code: [Select]

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

edited_word[ ( ~source_target[3:0] ) ] = target_colour[0] ;
collision_rd_inc = ( source_word[ ( ~source_target[3:0] ) ] != 0 ) ;

if ( ~source_target[3:0] != 15 ) edited_word[ 15 : ( ~source_target[3:0] + 1 ) ] = source_word[ 15 : ( ~source_target[3:0] + 1 ) ] ;
if ( ~source_target[3:0] != 0 )  edited_word[ ( ~source_target[3:0] - 1 ) : 0 ]  = source_word[ ( ~source_target[3:0] - 1 ) : 0 ]  ;

end
I'm hoping it's okay, but I'm expecting errors. 

[BrianHG]

Ok, your gonna flip after I simplified over a weeks of work down to 4 lines, plus 2 look up tables.
The 4 lines:

Code: [Select]

wr_pix_c_miss = rd_data_in & (16'hFFFF ^ (LUT_mask[wc_bpp] << LUT_shift[{wc_bpp,wc_target}])) | ( (wc_colour & LUT_mask[wc_bpp]) << LUT_shift[{wc_bpp,wc_target}] ) ; // Separate out the PX_COPY_COLOUR
wr_pix_c_hit  = wcd        & (16'hFFFF ^ (LUT_mask[bpp   ] << LUT_shift[{bpp   ,target   }])) | ( (colour    & LUT_mask[bpp   ]) << LUT_shift[{bpp   ,target   }] ) ; // Separate out the PX_COPY_COLOUR

rd_pix_c_miss = ( rd_data_in >> LUT_shift[{rc_bpp,rc_target}]) & LUT_mask[rc_bpp] ; // Separate out the PX_COPY_COLOUR
rd_pix_c_hit  = ( rcd        >> LUT_shift[{bpp   ,target   }]) & LUT_mask[bpp   ] ; // Separate out the PX_COPY_COLOUR
Oh well....  The entire code is now 1/4 the size and the 2 added modules at the end are now non-existent.

The pixel write command seems to work fine.  I've removed a the fancy copy, mask and collision counter features, we should test this with software before I add more stuff.

Here is a simulation.  The red line denotes a cache miss (IE, new write pixel address) and the green lines denotes a cache hit (IE, same address, but different pixel on the bitplane).

I attached the source Quartus simulation project.
It is time to merge this code with the geometry code like I said in the previous post and tie it into the GPU project.

You should be able to draw lines, boxes and filled boxes on 256, 16, 4 & 2 color screens.
Lets see some results as I comment the code changes I made on my end.