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

Remove Hardcoded register space 64KB in shim and zocl #8300

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

ManojTakasi
Copy link
Collaborator

Problem solved by the commit

CR-1185445 XRT has hardcoded the register space for read/write to 64KB.

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

In the zocl side, we are hardcoding CU size as 64KB size, mmap is failing for IP's which has more than 64KB size .

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

Removed Hardcoded check 0x1000 from the user side and zocl side.

Risks (if any) associated the changes in the commit

NA

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

Tested on vck190

Documentation impact (if any)

NA

@ManojTakasi ManojTakasi changed the title Remove Hardcoded register space Remove Hardcoded register space 64KB in shim and zocl Jul 22, 2024
Comment on lines 268 to 280
try {
for (auto& xml_inst : xml_kernel) {
if (xml_inst.first != "instance")
continue;
for (auto& xml_remap : xml_inst.second) {
if (xml_remap.first != "addrRemap")
continue;
address_range = convert(xml_remap.second.get<std::string>("<xmlattr>.range"));
if (address_range != 0)
return address_range;
else
return default_address_range;
}
Copy link
Collaborator

@stsoe stsoe Jul 22, 2024

Choose a reason for hiding this comment

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

This is not the same behavior. The value returned is not the same after this change. There are xclbins with kernel attribute "range=0xValue" but same xclbins do not have range attribute. I will vaguer that you cannot make this change.

How can RISK be N/A when you are changing the behavior of an existing function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you Soren for your inputs and for pointing that out. I haven't considered the case where xclbins might not have the range attribbute. I will make the necessary adjustments to ensure the behaviour remains consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi Manoj,
As discussed offline, Please remove this change and ask users to have their xclbin's updated with proper range information of slave,

Copy link
Collaborator Author

@ManojTakasi ManojTakasi Jul 25, 2024

Choose a reason for hiding this comment

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

Hi Vamshi, Soren,

The Changes have been made as suggested. could you pelase review them once again? Please let me know if any further changes are required.

@chvamshi-xilinx chvamshi-xilinx merged commit c0126fd into Xilinx:master Aug 5, 2024
16 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.

3 participants