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

Add issue_token attribute to IpuDmaMemcpyNdOp #1209

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

jtuyls
Copy link
Collaborator

@jtuyls jtuyls commented Apr 10, 2024

This PR adds an issue_token field to IpuDmaMemcpyNdOp. This allows more fine-grained control of which DMA operations should issue tokens and enables synchronization on MM2S DMA operations in addition to the S2MM DMA operations which were implicitly enabled earlier. The new issue_token field is set to false by default for backward compatibility with the implicit issuance of tokens for S2MM DMA operations only.

cc @fifield @jgmelber @AndraBisca Could you help review?

@makslevental
Copy link
Contributor

enables synchronization on MM2S DMA operations

I'm curious - when do you want to do this?

Copy link
Contributor

@makslevental makslevental left a comment

Choose a reason for hiding this comment

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

LGTM

test/Conversion/DmaToIpu/dma_to_ipu_issue_token.mlir Outdated Show resolved Hide resolved
@makslevental
Copy link
Contributor

The test fails are flakes (something about caching the ccache dir)

@jtuyls
Copy link
Collaborator Author

jtuyls commented Apr 10, 2024

enables synchronization on MM2S DMA operations

I'm curious - when do you want to do this?

If you have a sequence of bd writes using the same bd_id, to avoid overwriting a bd before fully executed.

@makslevental
Copy link
Contributor

If you have a sequence of bd writes using the same bd_id, to avoid overwriting a bd before fully executed.

Makes sense

@jtuyls
Copy link
Collaborator Author

jtuyls commented Apr 10, 2024

@makslevental Could you help with approving the workflow again?

@makslevental
Copy link
Contributor

@makslevental Could you help with approving the workflow again?

Sure but I pinged you on slack about a longer-term solution...

@makslevental makslevental force-pushed the ipu-dma-add-issue-token branch from 0f8b591 to 22289fc Compare April 10, 2024 20:20
@makslevental makslevental merged commit 066ce8f into Xilinx:main Apr 10, 2024
54 checks passed
@jtuyls jtuyls deleted the ipu-dma-add-issue-token branch April 10, 2024 21:09
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.

3 participants