Katia median filter#9
Conversation
| @@ -0,0 +1,25 @@ | |||
| module bram #( | |||
There was a problem hiding this comment.
I tried to avoid nitpicks in the review but lmk if you want me to add them
| localparam int ADDR_H = $clog2(IMAGE_HEIGHT); | ||
| localparam int PIXEL_W = 24; | ||
|
|
||
| function automatic logic [7:0] max4_u8( |
There was a problem hiding this comment.
The way you find the min and max is inefficient.
Here, you evaluate a > b and a < b in the next function. For our purposes, !(a < b) = a > b. If you restructured your code, you could have saved 2 8-bit comparisons
| input logic [7:0] a, | ||
| input logic [7:0] b, | ||
| input logic [7:0] c, | ||
| input logic [7:0] d |
There was a problem hiding this comment.
Only going to say this once, but no magic numbers
| end | ||
| endfunction | ||
|
|
||
| function automatic pixel_t to_pixel_fn(input logic [23:0] a); |
There was a problem hiding this comment.
These functions don't depend on module parameters. You can move them to a package to clean them up
| bram #( | ||
| .BRAM_ADDR_WIDTH (ADDR_W), | ||
| .BRAM_DATA_WIDTH (PIXEL_W) | ||
| ) line_buffer_bram ( |
| pixel_t input_pipe1_q; | ||
| pixel_t input_pipe2_d; | ||
| pixel_t input_pipe2_q; | ||
| logic input_pipe1_valid_d; |
There was a problem hiding this comment.
Nit: Don't use the same name over and over ie: input_pipe1_valid_q, input_pipe2_valid_q, input_pipe3_valid_q. If they are just the same signal, just one cycle delayed, make it a shift register.
|
|
||
| // pack input pixel to BRAM data | ||
| logic [23:0] bram_input; | ||
| always_comb bram_input = from_pixel_fn(pixel_valid_if_i.pixel); |
| green_total = d0.green + d1.green + d2.green + d3.green; | ||
| blue_total = d0.blue + d1.blue + d2.blue + d3.blue; | ||
|
|
||
| red_total = (red_total - max_red - min_red + 1) >> 1; |
There was a problem hiding this comment.
Here, you add all the red pixels together, then subtract the min and the max. It would be easier to find the two median values and then add those together and take the average and it would be far less expensive
DonnyC123
left a comment
DonnyC123
left a comment
There was a problem hiding this comment.
I noticed that you were missing your test bench code.
There are some optimizations/changes to the code that I would like to see if we were using this in our project, but overall, it looked good for a first version. It was also great to see that you used BRAM in your design. Your code is somewhat verbose, but it was reasonably easy to read. Overall, good job!
2 participants
works