-
Notifications
You must be signed in to change notification settings - Fork 497
[XDP] Initial NPU3 support #9130
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
Signed-off-by: Paul Schumacher <[email protected]>
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 25 out of 99. Check the log or trigger a new build to see more.
@@ -198,14 +205,16 @@ namespace xdp { | |||
uint8_t channelNumber; | |||
uint8_t streamId; | |||
uint8_t burstLength; | |||
uint8_t type; | |||
|
|||
TraceGMIO(uint32_t i, uint8_t col, uint8_t num, |
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.
warning: 6 adjacent parameters of 'TraceGMIO' of convertible types are easily swapped by mistake [bugprone-easily-swappable-parameters]
TraceGMIO(uint32_t i, uint8_t col, uint8_t num,
^
Additional context
src/runtime_src/xdp/profile/database/static_info/aie_constructs.h:209: the first parameter in the range is 'i'
TraceGMIO(uint32_t i, uint8_t col, uint8_t num,
^
src/runtime_src/xdp/profile/database/static_info/aie_constructs.h:210: the last parameter in the range is 't'
uint8_t stream, uint8_t len, uint8_t t = 0)
^
src/runtime_src/xdp/profile/database/static_info/aie_constructs.h:209:
TraceGMIO(uint32_t i, uint8_t col, uint8_t num,
^
src/runtime_src/xdp/profile/database/static_info/aie_constructs.h:209: 'uint32_t' and 'uint8_t' may be implicitly converted: 'uint32_t' (as 'unsigned int') -> 'uint8_t' (as 'unsigned char'), 'uint8_t' (as 'unsigned char') -> 'uint32_t' (as 'unsigned int')
TraceGMIO(uint32_t i, uint8_t col, uint8_t num,
^
@@ -271,8 +280,8 @@ | |||
|
|||
bool port_trace_is_master[NUM_SWITCH_MONITOR_PORTS]; | |||
int8_t port_trace_ids[NUM_SWITCH_MONITOR_PORTS]; | |||
int8_t s2mm_channels[NUM_CHANNEL_SELECTS] = {-1, -1}; | |||
int8_t mm2s_channels[NUM_CHANNEL_SELECTS] = {-1, -1}; | |||
int8_t s2mm_channels[NUM_CHANNEL_SELECTS_MAX] = {-1, -1, -1, -1}; |
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.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
int8_t s2mm_channels[NUM_CHANNEL_SELECTS_MAX] = {-1, -1, -1, -1};
^
int8_t s2mm_channels[NUM_CHANNEL_SELECTS] = {-1, -1}; | ||
int8_t mm2s_channels[NUM_CHANNEL_SELECTS] = {-1, -1}; | ||
int8_t s2mm_channels[NUM_CHANNEL_SELECTS_MAX] = {-1, -1, -1, -1}; | ||
int8_t mm2s_channels[NUM_CHANNEL_SELECTS_MAX] = {-1, -1, -1, -1}; |
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.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
int8_t mm2s_channels[NUM_CHANNEL_SELECTS_MAX] = {-1, -1, -1, -1};
^
{ | ||
for (auto xclbin : currentXclbins) | ||
{ | ||
if (xclbin->aie.valid) | ||
{ | ||
xrt_core::message::send(xrt_core::message::severity_level::debug, "XRT", "Added GMIO trace of ID "+ std::to_string(id) + "."); | ||
xclbin->aie.gmioList.push_back(new TraceGMIO(id, col, num, stream, len)) ; | ||
xclbin->aie.gmioList.push_back(new TraceGMIO(id, col, num, stream, len, type)) ; |
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.
warning: initializing non-owner argument of type 'value_type &&' (aka 'xdp::TraceGMIO *&&') with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
xclbin->aie.gmioList.push_back(new TraceGMIO(id, col, num, stream, len, type)) ;
^
|
||
class AIETraceOffload | ||
{ | ||
class AIETraceOffload : public AIETraceOffloadBase { |
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.
warning: class 'AIETraceOffload' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class AIETraceOffload : public AIETraceOffloadBase {
^
void* deviceHandle; | ||
uint64_t deviceId; | ||
PLDeviceIntf* deviceIntf; | ||
AIETraceLogger* traceLogger; |
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.
warning: member variable 'traceLogger' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
AIETraceLogger* traceLogger;
^
PLDeviceIntf* deviceIntf; | ||
AIETraceLogger* traceLogger; | ||
|
||
bool isPLIO; |
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.
warning: member variable 'isPLIO' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
bool isPLIO;
^
AIETraceLogger* traceLogger; | ||
|
||
bool isPLIO; | ||
uint64_t totalSz; |
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.
warning: member variable 'totalSz' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
uint64_t totalSz;
^
|
||
bool isPLIO; | ||
uint64_t totalSz; | ||
uint64_t numStream; |
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.
warning: member variable 'numStream' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
uint64_t numStream;
^
bool isPLIO; | ||
uint64_t totalSz; | ||
uint64_t numStream; | ||
uint64_t bufAllocSz; |
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.
warning: member variable 'bufAllocSz' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
uint64_t bufAllocSz;
^
Signed-off-by: Paul Schumacher <[email protected]>
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 25 out of 68. Check the log or trigger a new build to see more.
uint64_t totalSz; | ||
uint64_t numStream; | ||
uint64_t bufAllocSz; | ||
std::vector<AIETraceBufferInfo> buffers; |
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.
warning: member variable 'buffers' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
std::vector<AIETraceBufferInfo> buffers;
^
|
||
// Internal use only | ||
// Set this for verbose trace offload | ||
bool m_debug = false; |
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.
warning: member variable 'm_debug' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
bool m_debug = false;
^
bool m_debug = false; | ||
|
||
// Continuous Trace Offload (For PLIO) | ||
bool traceContinuous; |
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.
warning: member variable 'traceContinuous' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
bool traceContinuous;
^
|
||
// Continuous Trace Offload (For PLIO) | ||
bool traceContinuous; | ||
uint64_t offloadIntervalUs; |
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.
warning: member variable 'offloadIntervalUs' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
uint64_t offloadIntervalUs;
^
// Continuous Trace Offload (For PLIO) | ||
bool traceContinuous; | ||
uint64_t offloadIntervalUs; | ||
bool bufferInitialized; |
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.
warning: member variable 'bufferInitialized' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]
bool bufferInitialized;
^
inline void setOffloadIntervalUs(uint64_t v) { offloadIntervalUs = v; } | ||
virtual bool initReadTrace(); | ||
virtual void endReadTrace(); | ||
virtual void startOffload(); |
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.
warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions]
virtual void startOffload(); | |
void startOffload() override; |
virtual bool initReadTrace(); | ||
virtual void endReadTrace(); | ||
virtual void startOffload(); | ||
virtual void stopOffload(); |
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.
warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions]
virtual void stopOffload(); | |
void stopOffload() override; |
|
||
void readTrace(bool final) {mReadTrace(final);}; | ||
bool isTraceBufferFull() {return false;}; | ||
virtual bool isTraceBufferFull() {return false;}; |
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.
warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [cppcoreguidelines-explicit-virtual-functions]
virtual bool isTraceBufferFull() {return false;}; | |
bool isTraceBufferFull() override {return false;}; |
@@ -7,7 +7,7 @@ | |||
#include "aie1_attributes.h" | |||
#include "aie2_attributes.h" | |||
#include "aie2ps_attributes.h" | |||
//#include "npu3_attributes.h" | |||
#include "npu3_attributes.h" |
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.
warning: 'npu3_attributes.h' file not found [clang-diagnostic-error]
#include "npu3_attributes.h"
^
/************************************************************************************* | ||
NPU3 Registers | ||
*************************************************************************************/ | ||
class NPU3UsedRegisters : public UsedRegisters { |
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.
warning: class 'NPU3UsedRegisters' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
class NPU3UsedRegisters : public UsedRegisters {
^
auto channelNumber = gmio_node.second.get<uint8_t>("channel_number"); | ||
|
||
gmio.type = io_type::GMIO; | ||
gmio.type = (ioType < 2) ? io_type::GMIO | ||
: ((ioType == 2) ? io_type::TRACE_DMA : io_type::CONTROL_DMA); |
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.
The type value for trace DMA in the metadata is 3, not 2.
Signed-off-by: Paul Schumacher <[email protected]>
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.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 25 out of 43. Check the log or trigger a new build to see more.
populateRegNameToValueMap(); | ||
populateRegValueToNameMap(); | ||
} | ||
~NPU3UsedRegisters() = default; |
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.
warning: annotate this function with 'override' or (rarely) 'final' [hicpp-use-override]
~NPU3UsedRegisters() = default; | |
~NPU3UsedRegisters() override = default; |
} | ||
~NPU3UsedRegisters() = default; | ||
|
||
void populateProfileRegisters(); |
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.
warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void populateProfileRegisters(); | |
void populateProfileRegisters() override; |
~NPU3UsedRegisters() = default; | ||
|
||
void populateProfileRegisters(); | ||
void populateTraceRegisters(); |
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.
warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void populateTraceRegisters(); | |
void populateTraceRegisters() override; |
|
||
void populateProfileRegisters(); | ||
void populateTraceRegisters(); | ||
void populateRegNameToValueMap(); |
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.
warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void populateRegNameToValueMap(); | |
void populateRegNameToValueMap() override; |
void populateProfileRegisters(); | ||
void populateTraceRegisters(); | ||
void populateRegNameToValueMap(); | ||
void populateRegValueToNameMap(); |
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.
warning: annotate this function with 'override' or (rarely) 'final' [cppcoreguidelines-explicit-virtual-functions]
void populateRegValueToNameMap(); | |
void populateRegValueToNameMap() override; |
#include "xdp/profile/device/aie_trace/client/aie_trace_offload_client.h" | ||
#include "xdp/profile/device/aie_trace/client/aie_trace_offload_npu3.h" |
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.
warning: 'xdp/profile/device/aie_trace/client/aie_trace_offload_npu3.h' file not found [clang-diagnostic-error]
#include "xdp/profile/device/aie_trace/client/aie_trace_offload_npu3.h"
^
} | ||
else if (aie::isAIE2ps(hwGen)) { |
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.
warning: repeated branch body in conditional chain [bugprone-branch-clone]
else if (aie::isAIE2ps(hwGen)) {
^
Additional context
src/runtime_src/xdp/profile/plugin/aie_trace/util/aie_trace_util.cpp:267: end of the original
}
^
src/runtime_src/xdp/profile/plugin/aie_trace/util/aie_trace_util.cpp:268: clone 1 starts here
else if (aie::isNPU3(hwGen)) {
^
XAie_EventSelectDmaChannel(aieDevInst, loc, 1, dmaDir, channel1); | ||
|
||
// Record for runtime config file | ||
config.port_trace_ids[0] = channel0; |
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.
warning: narrowing conversion from 'uint8_t' (aka 'unsigned char') to signed type 'int8_t' (aka 'signed char') is implementation-defined [bugprone-narrowing-conversions]
config.port_trace_ids[0] = channel0;
^
|
||
// Record for runtime config file | ||
config.port_trace_ids[0] = channel0; | ||
config.port_trace_ids[1] = channel1; |
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.
warning: narrowing conversion from 'uint8_t' (aka 'unsigned char') to signed type 'int8_t' (aka 'signed char') is implementation-defined [bugprone-narrowing-conversions]
config.port_trace_ids[1] = channel1;
^
if (aie::isInputSet(type, metricSet)) { | ||
config.port_trace_is_master[0] = true; | ||
config.port_trace_is_master[1] = true; | ||
config.s2mm_channels[0] = channel0; |
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.
warning: narrowing conversion from 'uint8_t' (aka 'unsigned char') to signed type 'int8_t' (aka 'signed char') is implementation-defined [bugprone-narrowing-conversions]
config.s2mm_channels[0] = channel0;
^
Problem solved by the commit
XDP needed support for NPU3
How problem was solved, alternative solutions (if any) and why they were rejected
Added extensive support for NPU3
Risks (if any) associated the changes in the commit
Support affects many files and build environments
What has been tested and how, request additional testing if necessary
Simulation of NPU3
Documentation impact (if any)
New events, metric sets, etc.