-
Notifications
You must be signed in to change notification settings - Fork 487
XRT-SMI Configure and Examine migration #8906
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
Conversation
Signed-off-by: Akshay Tondak <[email protected]>
Signed-off-by: Akshay Tondak <[email protected]>
Signed-off-by: Akshay Tondak <[email protected]>
shim based flow and separating flow for xbmgmt and xrt-smi Migrate examine to have separate flow for xbmgmt examine and xrt-smi examine Signed-off-by: Akshay Tondak <[email protected]> build fix and clang-tidy fix Signed-off-by: Akshay Tondak <[email protected]> build fix Signed-off-by: Akshay Tondak <[email protected]> Build fix Signed-off-by: Akshay Tondak <[email protected]> build fix Signed-off-by: Akshay Tondak <[email protected]> build fix Signed-off-by: Akshay Tondak <[email protected]> build fix Signed-off-by: Akshay Tondak <[email protected]> Test failure fix Signed-off-by: Akshay Tondak <[email protected]>
Signed-off-by: Akshay Tondak <[email protected]>
Can one of the admins verify this patch? |
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Can you please check the copyright dates and if they need to be updated?
I believe only SubCmdAdvanced is left rn. Once we move that to the shim we can get rid of the json file in xbutil.cpp?
Once SubCmdAdvanced is done. We'll need to make another change where we get the list of subcommands from shim config itself, and then we can remove the xbutil.cpp embedded json. I'm thinking of separating out xrt-smi and xbmgmt there as well (writing a different main_ for xrt-smi) instead of relying on the common main in XBMain.cpp. Will make this change in the next PR. |
Sounds good! |
Signed-off-by: Akshay Tondak <[email protected]>
clang-tidy review says "All clean, LGTM! 👍" |
Problem solved by the commit
This change migrates xrt-smi configure to the new shim based option setting flow. This change also separates out the flow between xbmgmt and xrt-smi since currently, they were both using same code (SubCmd****Internal[.cpp][.h]). Now all xrt-smi subcommands are moved to new flow with subcommand files present under xbutil2 directory. Ideally we should change the subdirectory names as well to xrt-smi.
If need be in future, we should migrate the xbmgmt subcommands as well.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
https://jira.xilinx.com/browse/AIESW-1737
How problem was solved, alternative solutions (if any) and why they were rejected
An alternate to separating out xbmgmt and xrt-smi was to keep them together and migrate only xrt-smi which proved to be untidy and cumbersome.
Risks (if any) associated the changes in the commit
None
What has been tested and how, request additional testing if necessary
Tested all xrt-smi examine and xrt-smi configure commands on krck01
Documentation impact (if any)
none