-
Notifications
You must be signed in to change notification settings - Fork 472
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
VITIS-8548-XRT support to improve AIE status reporting #7619
Conversation
Build failed :( |
Signed-off-by: saumya garg <[email protected]> minor fix for build failure Signed-off-by: saumya garg <[email protected]>
Build Passed! |
_output << fmt4("%d") % "Column" % tile.second.get<int>("column"); | ||
_output << fmt4("%d") % "Row" % tile.second.get<int>("row"); | ||
|
||
if (tile.second.find("column") != tile.second.not_found()) { |
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.
why do we have to add this new check?
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.
This is not needed, I have now removed it
_output << fmt4("%d") % "Column" % tile.second.get<int>("column"); | ||
_output << fmt4("%d") % "Row" % tile.second.get<int>("row"); | ||
|
||
if (tile.second.find("column") != tile.second.not_found()) { |
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.
why do we have to add this new check?
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.
same as above
try { | ||
// On Pcie platforms use driver calls to get AIE Shim info | ||
pt_shim = asd_parser::get_formated_tiles_info(device, asd_parser::aie_tile_type::shim); | ||
|
||
if (pt_shim.empty()) |
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.
why is this new check added?
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.
This check was added because the pt_shim for edge device will get overwritten with this call for pcie platforms. I have now modified it further to check for pcie platform in the catch of "not edge device".
populate_shim_bd_info_aieml(const boost::property_tree::ptree& input_pt, boost::property_tree::ptree& pt_bd){ | ||
std::vector<std::string> bd_info; | ||
|
||
bd_info.push_back("base_address"); |
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.
As you know the vector size already, dont use multiple push_back calls as it is going to reallocate your vector again and again.
Either allocate vector with predefined size and push_back, or use initlizer list.
Use in other places too.
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.
Can you also generate property trees before and after your changes and compare the property tree.
Build failed :( |
Build failed :( |
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, Some small changes requested along with vector initialization.
ptarray.push_back(std::make_pair(std::to_string(i) + "_" + std::to_string(aie_meta.shim_row), | ||
aie_sys_parser::get_parser(aiepart)->aie_sys_read(i, aie_meta.shim_row))); | ||
std::stringstream a; | ||
boost::property_tree::write_json(a,ptarray); |
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.
Please remove extra prints
int col = oshim.get<uint32_t>("col"); | ||
int row = oshim.get<uint32_t>("row"); | ||
|
||
int col = oshim.get<int>("col"); |
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.
why do we need to change from uint32_t to int?
Both of them works, but better not to change this.
} | ||
|
||
try { | ||
// On Pcie platforms use driver calls to get AIE Shim info | ||
pt_shim = asd_parser::get_formated_tiles_info(device, asd_parser::aie_tile_type::shim); | ||
} | ||
catch (const xrt_core::query::no_such_key&) { |
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.
these two catches are same.. you have to remove one catch...
Build Passed! |
Signed-off-by: saumya garg <[email protected]> fix review comment Signed-off-by: saumya garg <[email protected]> fix review comment Signed-off-by: saumya garg <[email protected]> fix review comment Signed-off-by: saumya garg <[email protected]>
Build Passed! |
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. Please generate the json output and verify.
boost::property_tree::ptree bd_details_array; | ||
int i = 0; | ||
for (const auto& value : value_tree) { | ||
std::string val = value.second.data(); |
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.
const std::string
const boost::property_tree::ptree& bd_node = input_pt.get_child("bd"); | ||
|
||
for (const auto& pair : bd_node) { | ||
const std::string& key = pair.first; |
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.
you may need to add a error condition. bd_info size and value_tree size should match,
// This function extract BD information for core tiles of 1st generation aie architecture | ||
void | ||
populate_core_bd_info_aie(const boost::property_tree::ptree& input_pt, boost::property_tree::ptree& pt_bd){ | ||
std::vector<std::string> bd_info(35); |
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.
static std::vector with initialization list is better. Use in other places too.
Signed-off-by: saumya garg <[email protected]> fix review comment Signed-off-by: saumya garg <[email protected]>
boost::property_tree::ptree bd_info_array; | ||
const boost::property_tree::ptree& bd_node = input_pt.get_child("bd"); | ||
|
||
for (const auto& pair : bd_node) { |
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.
While you are here you can modernize and clarify a lot your code in a C++17 way https://en.cppreference.com/w/cpp/language/structured_binding
for (const auto& pair : bd_node) { | |
for (const auto& [ key, value_tree ] : bd_node) { |
Build Passed! |
Signed-off-by: saumya garg <[email protected]> fix review comment Signed-off-by: saumya garg <[email protected]> fix review comment Signed-off-by: saumya garg <[email protected]>
Build Passed! |
Merging as other teams are waiting for this change. |
Problem solved by the commit
https://jira.xilinx.com/browse/VITIS-8548
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
NA
How problem was solved, alternative solutions (if any) and why they were rejected
Reporting bd information (added as sysfs nodes) to the aie reports.
Risks (if any) associated the changes in the commit
Low
What has been tested and how, request additional testing if necessary
Tested on edge platforms. Adding a snippet of the reports below (showing bd info for only 3bds per tile here):
Report of AIE core tile :
xbutil examine -r aie -d
`Aie
Aie_Metadata
GRAPH[ 0] Name : mygraph
Status : unknown
SNo. Core [C:R] Iteration_Memory [C:R] Iteration_Memory_Addresses
[ 0] 24:0 24:1 31460
[ 1] 27:0 27:1 8388
Core [ 0]
Column : 24
Row : 0
Core:
Status : disabled, core_done
Program Counter : 0x000000e6
Link Register : 0x000000d0
Stack Pointer : 0x00037b00
DMA:
MM2S:
Channel:
Id : 0
Channel Status : idle
Queue Size : 0
Queue Status : okay
Current BD : 0
`
Report of aie shim tile:
xbutil examine -r aieshim -d
`AIE
Shim Status
Tile[ 0]
Tile[ 1]
Column : 0
Row : 0
Events:
pl : 1, 106, 115
Tile[ 2]
Column : 1
Row : 0
Events:
pl : 1, 106, 115
Tile[ 3]
Column : 2
Row : 0
DMA:
FIFO:
Counter0 : 0
Counter1 : 0
MM2S:
Channel:
Id : 0
Channel Status : idle
Queue Size : 0
Queue Status : okay
Current BD : 15
`
Report of Aie Mem tile:
xbutil examine -r aiemem -d
AIE
Mem Status
Tile[ 0]
Tile[ 1]
Column : 0
Row : 1
DMA:
FIFO:
MM2S:
Channel:
Id : 0
Channel Status : idle
Queue Size : 0
Queue Status : okay
Current BD : 0
Documentation impact (if any)
NA