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.

Sign up for GitHub

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

Jump to bottom

hw: Add native iDMA 2D capabilities, tune interconnect #73

Merged
merged 2 commits into from
Sep 19, 2023
Merged

hw: Add native iDMA 2D capabilities, tune interconnect #73

merged 2 commits into from
Sep 19, 2023

Conversation

thommythomaso
Copy link
Collaborator

No description provided.

@thommythomaso thommythomaso self-assigned this Sep 12, 2023
@thommythomaso thommythomaso added the enhancement New feature or request label Sep 12, 2023
@thommythomaso thommythomaso force-pushed the 2d-tbenz branch 3 times, most recently from 4fb6739 to 6021bda Compare September 14, 2023 14:37
alex96295
alex96295 previously approved these changes Sep 14, 2023
View reviewed changes
Copy link
Collaborator

@alex96295 alex96295 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM for me.

Just one minor: can you please add the missing parameters that are not 0 by default also in the xilinx wrapper? We can size them differently, but if we do not add them, they will be tied to 0 because of the default keyword

@paulsc96 paulsc96 changed the title iDMA: Add native 2D capabilities, tune the interconnect hw: Add native iDMA 2D capabilities, tune interconnect Sep 14, 2023
paulsc96
paulsc96 previously approved these changes Sep 14, 2023
View reviewed changes
Copy link
Collaborator

@paulsc96 paulsc96 left a comment

Choose a reason for hiding this comment

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

LGTM.

If I may nitpick, the DIF header looks a bit verbose, boiler-platey, and maintenance-unfriendly for my taste. Since it can be prefixed through macros, maybe it makes sense to include it in and get it from the iDMA repo directly?

Not a pressing issue though.

@thommythomaso
Copy link
Collaborator Author

LGTM.

If I may nitpick, the DIF header looks a bit verbose, boiler-platey, and maintenance-unfriendly for my taste. Since it can be prefixed through macros, maybe it makes sense to include it in and get it from the iDMA repo directly?

Not a pressing issue though.

Wilco. I will move as much as I can from the systems into the iDMA with the next release.

@thommythomaso thommythomaso dismissed stale reviews from alex96295 and paulsc96 via 8907d0c September 15, 2023 08:52
@thommythomaso thommythomaso force-pushed the 2d-tbenz branch 2 times, most recently from 8907d0c to 6df9ace Compare September 15, 2023 08:59
@thommythomaso @alex96295
iDMA: Add 2D support in hardware, tune interconnect
3499784 
---------

Co-authored-by: Alessandro Ottaviano <[email protected]>
paulsc96
paulsc96 approved these changes Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@paulsc96 paulsc96 left a comment

Choose a reason for hiding this comment

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

I cleaned up some stuff (parameter ordering) and extended the Xilinx wrapper. Automerge engaged :)

@paulsc96 paulsc96 merged commit f7568e2 into main Sep 19, 2023
18 checks passed
@paulsc96 paulsc96 deleted the 2d-tbenz branch September 19, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex96295 alex96295 alex96295 approved these changes

@paulsc96 paulsc96 paulsc96 approved these changes

@micprog micprog Awaiting requested review from micprog

Assignees

@thommythomaso thommythomaso

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thommythomaso @paulsc96 @alex96295