Skip to content

jpeg_rle: Add missing reset to ddstrb pipeline register to prevent spurious go after reset#4118

Merged
maliberty merged 2 commits intoThe-OpenROAD-Project:masterfrom
ashnaaseth2325-oss:fix/jpeg-rle-ddstrb-reset
Apr 14, 2026
Merged

jpeg_rle: Add missing reset to ddstrb pipeline register to prevent spurious go after reset#4118
maliberty merged 2 commits intoThe-OpenROAD-Project:masterfrom
ashnaaseth2325-oss:fix/jpeg-rle-ddstrb-reset

Conversation

@ashnaaseth2325-oss
Copy link
Copy Markdown
Contributor

SUMMARY

This PR adds a missing asynchronous reset to the ddstrb pipeline register in jpeg_rle.v, preventing invalid signal propagation after reset. The change affects the jpeg_rle module and ensures stable interaction with jpeg_rle1 by eliminating unintended go pulses.


FIX

// Before
always @(posedge clk)
  ddstrb <= #1 dstrb;

// After
always @(posedge clk or negedge rst)
  if (!rst)
    ddstrb <= #1 1'b0;
  else
    ddstrb <= #1 dstrb;

VERIFICATION

Simulate with rst deasserted while dstrb is high or unknown; previously this caused an immediate transition to ac state with invalid outputs. After the fix, ddstrb resets to 0 and no spurious go pulse occurs. The state machine remains in dc until valid input arrives, preserving correct JPEG block encoding.

@maliberty
Copy link
Copy Markdown
Member

This change leads to a small QOR change. Please run update_ok for this design and commit the updated rules file. That should get the CI passing.

@ashnaaseth2325-oss
Copy link
Copy Markdown
Contributor Author

ashnaaseth2325-oss commented Apr 7, 2026

Hello @maliberty,
Thanks! This change affects QOR as expected.
I’m currently unable to run the full OpenROAD flow locally due to tool setup issues, and I also don’t have access to Jenkins artifacts.
Could you please run update_ok and commit the updated rules?
Happy to rebase if needed.

@ashnaaseth2325-oss ashnaaseth2325-oss force-pushed the fix/jpeg-rle-ddstrb-reset branch from 2be5eec to a5af43f Compare April 13, 2026 21:07
@ashnaaseth2325-oss
Copy link
Copy Markdown
Contributor Author

Hi @maliberty, I tried running update_ok again but couldn’t complete the flow locally. Could you help update the QOR reference or suggest an alternative?

Comment thread flow/designs/src/jpeg/jpeg_rle.v
@ashnaaseth2325-oss
Copy link
Copy Markdown
Contributor Author

Hello @maliberty ,
Thanks for pointing this out. This is not using both clock edges, it still runs on posedge clk. The negedge rst is only for the async reset, so ddstrb is set properly on reset. The earlier version had no reset, which caused invalid values after reset.

ashnaaseth2325-oss and others added 2 commits April 14, 2026 21:00
ddstrb had no reset, risking spurious go assertion to jpeg_rle1
immediately after reset release, corrupting the first JPEG block.

Signed-off-by: Ashnaa Seth <ashnaaseth2325@gmail.com>
Signed-off-by: ashnaaseth2325-oss <ashnaaseth2325@gmail.com>

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
designs/sky130hd/jpeg/rules-base.json updates:
| Metric                                        | Old      | New      | Type     |
| ------                                        | ---      | ---      | ----     |
| cts__timing__setup__tns                       |   -155.0 |   -170.0 | Failing  |
| globalroute__antenna_diodes_count             |      100 |      115 | Failing  |
| globalroute__timing__setup__ws                |   -0.899 |    -1.16 | Failing  |
| globalroute__timing__setup__tns               |   -201.0 |   -268.0 | Failing  |
| finish__timing__setup__ws                     |   -0.603 |   -0.943 | Failing  |
| finish__timing__setup__tns                    |    -96.0 |   -146.0 | Failing  |

designs/sky130hs/jpeg/rules-base.json updates:
| Metric                                        | Old      | New      | Type     |
| ------                                        | ---      | ---      | ----     |
| placeopt__design__instance__count__stdcell    |    56019 |    55868 | Tighten  |
| cts__design__instance__count__setup_buffer    |     4871 |     4858 | Tighten  |
| cts__design__instance__count__hold_buffer     |     4871 |     4858 | Tighten  |
| globalroute__timing__setup__tns               |   -0.809 |    -1.92 | Failing  |
| detailedroute__antenna_diodes_count           |      100 |      102 | Failing  |

Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
@maliberty maliberty force-pushed the fix/jpeg-rle-ddstrb-reset branch from 0da4a09 to 6937b58 Compare April 14, 2026 21:01
@maliberty maliberty enabled auto-merge April 14, 2026 22:16
@maliberty maliberty merged commit 43ca564 into The-OpenROAD-Project:master Apr 14, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants