-
Notifications
You must be signed in to change notification settings - Fork 513
Trace/Profile support using runlist #9283
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
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
| const auto& xdp_exit_runs = xrt_core::hw_context_int::get_xdp_exit_runs(m_hwctx); | ||
| if (xdp_exit_runs.size() > 0) { | ||
| // remove the exit runs from the runlist | ||
| m_runlist.erase(m_runlist.end() - xdp_exit_runs.size(), m_runlist.end()); |
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 'size_type' (aka 'unsigned long') to signed type 'difference_type' (aka 'long') is implementation-defined [bugprone-narrowing-conversions]
m_runlist.erase(m_runlist.end() - xdp_exit_runs.size(), m_runlist.end());
^| // Vector of XDP runs to be added at beginning and end of runlist | ||
| // These runs initializes/configures AI array and collects the profile/trace data | ||
| std::vector<xrt::run> m_xdp_init_runs; | ||
| std::vector<xrt::run> m_xdp_exit_runs; | ||
|
|
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.
I would prefer to see these managed by xdp itself. Is that possible?
I imagine xdp will be calling the register functions, alas it can keep the data in xdp and we don't need register and unregister here in hwctx.
The runlist can make a callback into xdp to get the list of runs to prepend and append.
This would be cleaner and more general; I think.
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.
Hi @stsoe, XDP will create separate "xrt::run" objects for its different plugins (say AIE Trace, AIE Profile). Currently XDP does not have a container to store the different configuration and exit xrt::run objects for the different plugins.
We can create one, but I wanted to understand the benefits of XDP maintaining it as opposed to xrt.
If XRT does not store the run objects from XDP, then XDP will need to be informed about when to send the run objects before XRT calls execute on runlist.
I had a little chat with Rahul. He explained XRT can add new callbacks before runlist execute and XDP can send the vector of custom run objects. In that solution, there would be two new callbacks into XDP (one for init run objects and another for exit run objects). That sounds a bit more complex, as opposed to two new vectors
" std::vectorxrt::run m_xdp_init_runs;
std::vectorxrt::run m_xdp_exit_runs;"
in xrt. If xrt maintains the vector, then XDP can register the run objects during hw context creation (like it does now) and be done.
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.
Basically, I don't want XDP to leave bread crumbs in various XRT objects. We had that a long while back when we supported only OpenCL and Alveo. It took a long time to root out XDP from all the places.
It make sense that XDP wants to append and prepend a runlist, and it makes sense for xrt::runlist to support pre and append. But it doesn't make sense that the code is wired to store these pre and append run objects outside XDP and for the runlist know about init and exit objects that some some reason are stored in the hwctx implementation. Just too strange, IMO.
I really think XDP should manage this without storing objects in XRT coreutil. The hook should be entirely within the implementation of xrt::runlist. Right before submission, a callback into XDP (or anyone else registered) with the runlist, XDP then prepends the init run objects and appends the exit run objects. Is that not possible?
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.
Thanks for explaining in details @stsoe . We discussed with Vamshi and Rahul. XDP will store the run objects it needs to prepend and append to runlist and two callbacks from XRT to XDP will be provided to pass the vector of run objects to XRT.
These callbacks will be executed for all devices that support runlist. However, XDP plans to create custom run objects for some special cases only.
4295df0 to
7d1ba56
Compare
|
Hi @stsoe and @IshitaGhosh, |
stsoe
left a comment
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 we nuke the changes to xrt_hw_context* ?
I think the xrt_kernel.cpp changes are still too xdp specific. Maybe I am asking too much, but how about implementing xrt::runlist::append(xrt::run) and prepend(xrt::run). Then have one callback into any registered functions at execution time (we already appending in execute()) with the runlist? Let xdp pre- and append what it needs.
Hopefully the callback would not be registered in the normal case, hence be a no-op.
Signed-off-by: Rahul Bramandlapalli <rbramand@amd.com>
7d1ba56 to
163f6fc
Compare
Signed-off-by: Rahul Bramandlapalli <rbramand@amd.com>
163f6fc to
b85229f
Compare
|
Closing this PR as firmware team provided a mechanism to disable the new mode |
Problem solved by the commit
Added trace/profile support using runlist.
Earlier XDP hooks were in xrt::hw_context constructor and destructor but now because of recent optimizations we have to move them to xrt::runlist.
Spec for this change is captured in confluence page - https://amd.atlassian.net/wiki/spaces/~rbramand/pages/1117815036/Trace+Profile+flow+proposal+using+runlist
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
While the solution has its own limitations it can act as workaround to collect XDP data
Risks (if any) associated the changes in the commit
Low as the feature works only when ini options related to xdp are enabled
What has been tested and how, request additional testing if necessary
Tested using runlist test case on strix linux and xdp init and exit runs are properly added and removed as expected
Documentation impact (if any)
May be we need to update about limitations on when Profile/Trace data can be captured.