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 PID argument to aie mem and reg read/write apis #8275

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rbramand-xilinx
Copy link
Collaborator

@rbramand-xilinx rbramand-xilinx commented Jul 8, 2024

Problem solved by the commit

Added PID argument to aie debug apis to uniquely identify hw ctx
Reverted PR - #8192 that removed printing PID in xrt-smi o/p to get PID info
On Linux PID info is already available but on windows implementation is different, created CR to get PID info on windows - https://jira.xilinx.com/browse/CR-1204588

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

Ignoring the PID check if it is -1 as windows flow doesn't provide PID info yet and returns -1 at present.

Risks (if any) associated the changes in the commit

Low

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

Tested the flow on both Linux and windows and the apis work as expected.

Documentation impact (if any)

Added doxygen comment for the arg added

Copy link
Collaborator

@stsoe stsoe left a comment

Choose a reason for hiding this comment

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

For pid argument, can we use pid_t? If you include core/include/types.h there is a typedef even for windows.

Copy link
Member

@hackwa hackwa 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! If our tool can grab it from xrt-smi, that will work for us.

Copy link
Collaborator

@AShivangi AShivangi left a comment

Choose a reason for hiding this comment

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

Please use this ticket to track this change

@@ -114,6 +116,7 @@ writeReport(const xrt_core::device* /*_pDevice*/,
const auto& hw_context = pt_hw_context.second;

const std::vector<std::string> entry_data = {
hw_context.get<std::string>("pid"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want to print -1 for Ryzen devices. Please do something like:

const auto device_class = xrt_core::device_query_default<xrt_core::query::device_class>(dev, xrt_core::query::device_class::type::alveo);
  switch (device_class) {
  case xrt_core::query::device_class::type::alveo:
  {
    hw_context.get<std::string>("pid"),
    break;
  }

and print this field only if it is populated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @AShivangi , I have already filed a CR to get this info on windows - https://jira.xilinx.com/browse/CR-1204588
If we dont print PID in case of Ryzen devices then users can't provide PID arg and the debug apis behavior will be different on different platforms. So added changes this way.
But that said I am holding off this PR till PID support is added in windows, there is discussion going on this and if it is difficult to provide PID in MCDM flow they will provide some unique identifier in its place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, sounds good!

@rbramand-xilinx rbramand-xilinx added the do not merge hold off on merging label Jul 9, 2024
Signed-off-by: rbramand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge hold off on merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants