Possible race condition in the sprite rendering state machine

[gyurco]

Hi!

First thanks for this new core! I'm trying to make it work on MiST-derivate (SiDi128). As it doesn't have a DDR3 RAM, I wrote an emulation module using SDR SDRAM. I noticed in some cases, the last 4 pixels were missing on sprites. It turned out that the last (80th) write to the framebuffer was missing (see the attached trace).
It turned out that in the ST_DRAW_TILE3 state where the shifter is advanced to the last row, if busy is active even for one cycle, then the shifter_done status will be active, without giving a chance to write the last state to the framebuffer. The attached patch solved it for me. Maybe on the Cyclone V DDR3 controller, busy is never activated for writes?
(Sorry for not sending a PR, but I don't have a DE10-nano to test with MiSTer).

diff --git a/rtl/tc0200obj.sv b/rtl/tc0200obj.sv
index 9c16aee..c46c3c5 100644
--- a/rtl/tc0200obj.sv
+++ b/rtl/tc0200obj.sv
@@ -135,7 +135,8 @@ typedef enum
     ST_READ_TILE_WAIT,
     ST_DRAW_TILE1,
     ST_DRAW_TILE2,
-    ST_DRAW_TILE3
+    ST_DRAW_TILE3,
+    ST_DRAW_END
 } draw_state_t;
 
 draw_state_t obj_state = ST_IDLE;
@@ -562,7 +563,6 @@ always @(posedge clk) begin
         ST_DRAW_TILE2: begin
             ddr_obj.acquire <= 1;
             if (~ddr_obj.busy) begin
-                shifter_read <= 1;
                 ddr_obj.addr <= OBJ_FB_DDR_BASE + {13'd0, draw_buffer, shifter_addr, 3'd0 };
                 ddr_obj.burstcnt <= 1;
                 ddr_obj.wdata <= shifter_data;
@@ -571,7 +571,12 @@ always @(posedge clk) begin
 
                 fb_dirty_set_addr <= fb_dirty_draw_addr;
                 fb_dirty_set <= 1;
-                obj_state <= ST_DRAW_TILE3;
+                if (shifter_done)
+                  obj_state <= ST_DRAW_END;
+                else begin
+                  obj_state <= ST_DRAW_TILE3;
+                  shifter_read <= 1;
+                end
             end
         end
 
@@ -581,11 +586,16 @@ always @(posedge clk) begin
             shifter_read <= 0;
             if (~ddr_obj.busy ) begin
                 ddr_obj.write <= 0;
-                if (shifter_done) begin
-                    obj_state <= ST_READ_START;
-                end else begin
-                    obj_state <= ST_DRAW_TILE1;
-                end
+                obj_state <= ST_DRAW_TILE1;
+            end
+        end
+
+        ST_DRAW_END: begin
+            ddr_obj.acquire <= 1;
+            fb_dirty_set <= 0;
+            if (~ddr_obj.busy ) begin
+                ddr_obj.write <= 0;
+                obj_state <= ST_READ_START;
             end
         end

[wickerwaka]

Thanks, I'll incorporate this. Yeah, the DE-10's DDR controller rarely (if ever?) returns busy for writes. Is the SiDi128 handling all these systems in SDRAM okay? I decided to go with DDR for the sprites early on because it seemed like a good candidate because any latency could be hidden pretty easily. It has worked out well and I haven't really had to worry about any SDRAM bandwidth issues.

[gyurco]

SiDi128 has 2 SDRAM chips, so I used the second one exclusively for DDR-emulation. Here's my simple emulation module:
https://github.com/gyurco/Mist_FPGA/blob/F2/Arcade_MiST/Taito%20F2%20Hardware/rtl/sdram_ddremu.sv
I tried to copy the expected semantics of the MiSTer's DDR controller, but busy is usually asserted for every request for a small time.

I also used my bank-interleaving SDRAM controller for the other things, you can find it in that repo, too. The change needed in F2.sv is minimal, and it gives much more usable bandwidth, so I could move the sound ROM to SDRAM (as BRAM is a little less on this FPGA).