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

Fix bug in patching args in multiple runs #8660

Merged
merged 2 commits into from
Dec 17, 2024

Conversation

rbramand-xilinx
Copy link
Collaborator

@rbramand-xilinx rbramand-xilinx commented Dec 15, 2024

Problem solved by the commit

When kernel is run multiple times and in some of the runs if arg is updated using set_arg the present code patches the arg address on top of existing already patched value this will result in test failures. This PR fixes this issue

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

How problem was solved, alternative solutions (if any) and why they were rejected

Storing BD data ptr entries in patch info and marking it dirty after first patching.
When second or subsequent run is made with set_arg the original BD data ptr values stored are restored and we patch on top of it which is proper.

Risks (if any) associated the changes in the commit

Moderate as this code changes xrt code critical path

What has been tested and how, request additional testing if necessary

Tested xrt elf flow test on aie2p by calling set_arg in multiple kernel runs and test passes
Also tested vadd application on aie2ps by modifying host application accordingly.
Thorough testing is needed to make sure this change doesn't break all the existing flows

Documentation impact (if any)

NA

Signed-off-by: rbramand <rbramand@amd.com>
Copy link
Collaborator

@chvamshi-xilinx chvamshi-xilinx left a comment

Choose a reason for hiding this comment

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

I know, this file is not following other XRT user space style of coding, Please update name of variable to lower case. "max_bd_words"

Signed-off-by: rbramand <rbramand@amd.com>
Copy link
Collaborator

@larry9523 larry9523 left a comment

Choose a reason for hiding this comment

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

This fix introduces a new dependency between xrt_core and AIE architecture. Ideally, all patching knowledges should come from ELF.

But given the plan that we will move all the patching service out of XRT to aiebu, I think this temporal solution is OK.

@larry9523
Copy link
Collaborator

@rbramand-xilinx please provide a list of tests we have done before we are confident enough to merge this PR

@rbramand-xilinx
Copy link
Collaborator Author

@rbramand-xilinx please provide a list of tests we have done before we are confident enough to merge this PR

Hi @larry9523 , I have tested xrt flow test on aie2p and vadd test on aie2ps by modifying host application and test passes on both of them.

@rbramand-xilinx
Copy link
Collaborator Author

@rbramand-xilinx please provide a list of tests we have done before we are confident enough to merge this PR

Hi @larry9523 , I have tested xrt flow test on aie2p and vadd test on aie2ps by modifying host application and test passes on both of them.

@larry9523 I have verified non control packet , control packet elf flow tests and also full elf (xclbin2elf) flows and patching args in multiple runs tests passes only after applying this change.
@sayyanna is verifying preemption test, if that test passes I guess we are good to go.

@sayyanna
Copy link
Collaborator

@rbramand-xilinx please provide a list of tests we have done before we are confident enough to merge this PR

Hi @larry9523 , I have tested xrt flow test on aie2p and vadd test on aie2ps by modifying host application and test passes on both of them.

@larry9523 I have verified non control packet , control packet elf flow tests and also full elf (xclbin2elf) flows and patching args in multiple runs tests passes only after applying this change. @sayyanna is verifying preemption test, if that test passes I guess we are good to go.

Verified preemption test with patching args multiple times and tests passes only after applying this change.

@larry9523 larry9523 merged commit 4042c06 into Xilinx:master Dec 17, 2024
20 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.

6 participants