Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge/stream synchronization #349

Merged
merged 5 commits into from
Oct 28, 2024
Merged

Merge/stream synchronization #349

merged 5 commits into from
Oct 28, 2024

Conversation

mcoshiro
Copy link
Contributor

@mcoshiro mcoshiro commented Oct 10, 2024

Fixes a few synchronization issues in FPGA 1 project

  • tf_merge_streamer now only read TP_bx_out when it is valid so BX 0 is synchronized with other BXs
  • tf_merge_streamer now outputs its own BX output (TP_bx_out_merged) which is synchronized with its outputs
  • tf_merge_streamer read location is now synchronized with the appropriate valid bit, which fixes occasional strange behavior

It was noticed that some of the output does not match the test vectors, but this seems to be caused by TPs upstream of the merge/stream step.

Associated with PR #64 in project_generation_scripts.

…ronize bx with output, synchronize bx 0 with other bxs, and synchronize reading of memories by tf_merge_streamer
@jasonfan393 jasonfan393 self-assigned this Oct 10, 2024
@mcoshiro
Copy link
Contributor Author

Expanding scope of PR: now added additional changes to help FPGA1 project to meet timing. Two main changes are included:

Changed TPAR memories to use URAMs

This helps with high BRAM usage since currently all the TPARs must be placed near TP_L1L2A as the done signal for all TPARs comes from this single TP. This change makes the baseline project workable, but long term, we will need to change this as additional TPARs for displaced tracking will make the current solution untenable

Modified tf_merge_streamer

Vivado synthesized the previous version of tf_merge_streamer in a way that would generate paths with 20-30 logic levels. In particular, Vivado seemed to struggle to figure out that the for loop with exit could be implemented as a small number of LUTs. I wasn't able to figure out a way that both had clean VHDL and which Vivado could figure out, so for now, I changed it to a rather ugly long if-statement, which Vivado seems to handle better. Other suggestions are welcome.

Copy link
Contributor

@jasonfan393 jasonfan393 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcoshiro looks good, I'd want to simplify the big if block if possible, but if this proves to be too annoying we can always merge this and push it off for later

IntegrationTests/common/hdl/tf_merge_streamer.vhd Outdated Show resolved Hide resolved
IntegrationTests/common/hdl/tf_merge_streamer.vhd Outdated Show resolved Hide resolved
Copy link
Contributor

@jasonfan393 jasonfan393 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! approved

@jasonfan393 jasonfan393 merged commit 7f01c3c into master Oct 28, 2024
1 check passed
@mcoshiro mcoshiro deleted the merge_stream_sync branch October 31, 2024 12:27
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