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

CR 1217061 : xrt-smi advanced --help prints Alveo help #8545

Merged
merged 6 commits into from
Oct 23, 2024

Conversation

aktondak
Copy link
Collaborator

@aktondak aktondak commented Oct 17, 2024

Problem solved by the commit

xrt-smi advanced --help prints Alveo help which is incorrect

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

https://jira.xilinx.com/browse/CR-1217061
Discovered through internal testing

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

The problem was solved by setting m_device member and using to print specific help.
--report had multiple other bugs which are resolved with this :

  1. mode was used as an option which showed --mode as a user configurable option in help. This was incorrect.
  2. "--report clocks" was not being passed to sub-option making the parser show undefined behavior.
  3. There were no prints of what reports can be generated. Added "clocks" and "preemption" as valid reports.

Risks (if any) associated the changes in the commit

None

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

Tested on kracken board :
Before fix

Y:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi advanced --help

COMMAND: advanced

DESCRIPTION: Low level command operations.

USAGE: xrt-smi advanced [ --read-mem | --write-mem | --report ] [--help]

OPTIONS:
 Common:
  --help             - Help to use this sub-command
 AIE:
  --report           - Hidden reports
 Alveo:
  --read-mem         - Read from the given memory address
  --write-mem        - Write to a given memory address


GLOBAL OPTIONS:
  --verbose          - Turn on verbosity
  --batch            - Enable batch mode (disables escape characters)
  --force            - When possible, force an operation

After fix

Y:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi advanced --help

COMMAND: advanced

DESCRIPTION: Low level command operations.

USAGE: xrt-smi advanced [ --report ] [--help] [-d arg]

OPTIONS:
  -d, --device       - The Bus:Device.Function (e.g., 0000:d8:00.0) device of interest
  --help             - Help to use this sub-command
  --report           - Hidden reports


GLOBAL OPTIONS:
  --verbose          - Turn on verbosity
  --batch            - Enable batch mode (disables escape characters)
  --force            - When possible, force an operation

Documentation impact (if any)

None

Akshay Tondak added 2 commits October 16, 2024 19:05
Signed-off-by: Akshay Tondak <[email protected]>
Signed-off-by: Akshay Tondak <[email protected]>
@gbuildx
Copy link
Collaborator

gbuildx commented Oct 17, 2024

Can one of the admins verify this patch?

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.

You tried out all possible combinations for this? Passing in invalid values as well?

Signed-off-by: Akshay Tondak <[email protected]>
@aktondak
Copy link
Collaborator Author

You tried out all possible combinations for this? Passing in invalid values as well?

Passing an invalid value yields unrecognized arguments error :

Z:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi advanced --report clock
ERROR: Unrecognized arguments:
  clock


COMMAND: advanced

DESCRIPTION: Low level command operations.

USAGE: xrt-smi advanced [ --read-mem | --write-mem | --report ] [--help] [-d arg]

OPTIONS:
 Common:
  -d, --device       - The Bus:Device.Function (e.g., 0000:d8:00.0) device of interest
  --help             - Help to use this sub-command
 AIE:
  --report           - Reports to generate: clocks, preemption
 Alveo:
  --read-mem         - Read from the given memory address
  --write-mem        - Write to a given memory address


GLOBAL OPTIONS:
  --verbose          - Turn on verbosity
  --batch            - Enable batch mode (disables escape characters)
  --force            - When possible, force an operation
ERROR: operation canceled

@@ -46,6 +46,7 @@ SubCmdAdvanced::SubCmdAdvanced(bool _isHidden, bool _isDepricated, bool _isPreli
setIsPreliminary(_isPreliminary);

m_commonOptions.add_options()
("device,d", boost::program_options::value<decltype(m_device)>(&m_device), "The Bus:Device.Function (e.g., 0000:d8:00.0) device of interest")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this device. "xrt-smi advanced --device <>" Is not a valid command. We have devices inside the suboptions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can print device specific help only when m_device is populated.
SubCmdProgram class also has a m_device which works on the OptionOption mechanism, similar to advanced.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, please make sure that all possible option combos work after this change!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only clocks and preemption are used as valida reports right now. Generating preemption exits silently and clocks works fine :

Z:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi advanced --report preemption

Z:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi advanced --report clocks

Clocks
  H Clock                : 800 MHz
  MP-NPU Clock           : 400 MHz

@AShivangi
Copy link
Collaborator

You tried out all possible combinations for this? Passing in invalid values as well?

Passing an invalid value yields unrecognized arguments error :

Z:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi advanced --report clock
ERROR: Unrecognized arguments:
  clock


COMMAND: advanced

DESCRIPTION: Low level command operations.

USAGE: xrt-smi advanced [ --read-mem | --write-mem | --report ] [--help] [-d arg]

OPTIONS:
 Common:
  -d, --device       - The Bus:Device.Function (e.g., 0000:d8:00.0) device of interest
  --help             - Help to use this sub-command
 AIE:
  --report           - Reports to generate: clocks, preemption
 Alveo:
  --read-mem         - Read from the given memory address
  --write-mem        - Write to a given memory address


GLOBAL OPTIONS:
  --verbose          - Turn on verbosity
  --batch            - Enable batch mode (disables escape characters)
  --force            - When possible, force an operation
ERROR: operation canceled

Why is unrecognized option showing both aie and alveo?

@aktondak
Copy link
Collaborator Author

You tried out all possible combinations for this? Passing in invalid values as well?

Passing an invalid value yields unrecognized arguments error :

Z:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi advanced --report clock
ERROR: Unrecognized arguments:
  clock


COMMAND: advanced

DESCRIPTION: Low level command operations.

USAGE: xrt-smi advanced [ --read-mem | --write-mem | --report ] [--help] [-d arg]

OPTIONS:
 Common:
  -d, --device       - The Bus:Device.Function (e.g., 0000:d8:00.0) device of interest
  --help             - Help to use this sub-command
 AIE:
  --report           - Reports to generate: clocks, preemption
 Alveo:
  --read-mem         - Read from the given memory address
  --write-mem        - Write to a given memory address


GLOBAL OPTIONS:
  --verbose          - Turn on verbosity
  --batch            - Enable batch mode (disables escape characters)
  --force            - When possible, force an operation
ERROR: operation canceled

Why is unrecognized option showing both aie and alveo?

Apologies, this is outdated.
The updated behavior prints Ryzen help only :

Z:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi advanced --report clock

ERROR: Invalid report value: 'clock'
: invalid argument

COMMAND: advanced

DESCRIPTION: Reports to generate: clocks, preemption

USAGE:  advanced --report [--help] [-d arg]

OPTIONS:
  --report           - Reports to generate: clocks, preemption
  -d, --device       - The Bus:Device.Function (e.g., 0000:d8:00.0) device of interest
  --help             - Help to use this sub-command


GLOBAL OPTIONS:
  --verbose          - Turn on verbosity
  --batch            - Enable batch mode (disables escape characters)
  --force            - When possible, force an operation

@aktondak aktondak requested a review from rchane October 22, 2024 18:09
Copy link
Collaborator

@rchane rchane left a comment

Choose a reason for hiding this comment

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

Output seems good after changes!

@stsoe stsoe merged commit 4bc431f into Xilinx:master Oct 23, 2024
18 checks passed
@aktondak aktondak deleted the CR-1217061 branch October 23, 2024 06:30
parthash0804 pushed a commit to parthash0804/XRT that referenced this pull request Oct 29, 2024
* CR-1217061 revert

Signed-off-by: Akshay Tondak <[email protected]>

* CR-1217061 fix

Signed-off-by: Akshay Tondak <[email protected]>

* Adding missing line

Signed-off-by: Akshay Tondak <[email protected]>

* Adding missing code

Signed-off-by: Akshay Tondak <[email protected]>

* Taking care of empty error handling

Signed-off-by: Akshay Tondak <[email protected]>

---------

Signed-off-by: Akshay Tondak <[email protected]>
Co-authored-by: Akshay Tondak <[email protected]>
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.

5 participants