Add changes to set dtrace control file from xrt::run#9630
Add changes to set dtrace control file from xrt::run#9630chvamshi-xilinx merged 3 commits intoXilinx:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for setting dtrace control files at the xrt::run object level, providing more flexibility compared to the global xrt.ini configuration. This enhancement allows users to use different dtrace control files for different runs or change the control file between runs.
Changes:
- Added new xrt::run constructor and set_dtrace_control_file() API to specify dtrace control file per run
- Updated module implementation to support run-level dtrace control file reinitialization
- Added state checking to prevent dtrace control file changes while a run is in progress
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/runtime_src/core/include/xrt/xrt_kernel.h | Added public API: new run constructor with dtrace_control_file parameter and set_dtrace_control_file() method with comprehensive documentation |
| src/runtime_src/core/common/api/xrt_kernel.cpp | Implemented run-level dtrace control file support with state checking, mutex protection, proper cloning support, and command packet updates |
| src/runtime_src/core/common/api/xrt_module.cpp | Refactored dtrace initialization to support run-level control file paths, added set_dtrace_control_file() override, and separated buffer creation from handle initialization |
| src/runtime_src/core/common/api/module_int.h | Added internal API declaration for set_dtrace_control_file() with clear documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
clang-tidy review says "All clean, LGTM! 👍" |
ce728b1 to
da38aeb
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
xuhz
left a comment
There was a problem hiding this comment.
can we change xrt-runner as well so support this new run flavor?
or in a separate PR?
Separate PR, please, and more specifically, what level of support are you looking for? I assume profile.json control over this, please propose. |
sure Soren, I will send changes related to runner using another PR Let me know if its okay |
| * Throws if the run does not support dtrace flow (non-ELF flow). | ||
| */ | ||
| XRT_API_EXPORT | ||
| run(const kernel& krnl, const std::string& dtrace_control_file); |
There was a problem hiding this comment.
Do we need this constructor? It doesn't feel right. dtrace path is like another argument, but can't be part of set_arg, but the set_dtrace_... function should be enough?
There was a problem hiding this comment.
Yes Soren, I agree. I will remove this constructor
I think it is a profile.json control, not a recipe. The recipe has not path components. It is a binding, but of course not an argument binding. |
agreed. It should a profile.json control |
Signed-off-by: rahul <rbramand@amd.com>
Signed-off-by: rahul <rbramand@amd.com>
Signed-off-by: rahul <rbramand@amd.com>
da38aeb to
f50ab2d
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Problem solved by the commit
Added changes to support setting of dtrace control file from xrt::run object.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
This is an enhancement. Earlier dtrace control file is set using xrt.ini option which is a global and if user wants to change control file for tracing after a run or wanted to use different control file for different runs it was not possible. This PR adds changes to make it possible.
How problem was solved, alternative solutions (if any) and why they were rejected
Provided an API in xrt::run class to set dtrace control file. If the command is submitted or is running an exception is thrown. Also xrt.ini option for setting dtrace control file is still maintained but setting from xrt::run is given preference over it.
Updating only the dtrace buffer address in ert packet when control file is changed so that changes to packet are minimal and updating happens in the API call itself to not add overhead to run.start()
Risks (if any) associated the changes in the commit
Low to moderate
What has been tested and how, request additional testing if necessary
Tested dtrace single column and multi column tests on Telluride
TODO : Need to run model-tests on strix to confirm this change doesn't break anything
Documentation impact (if any)
Added doxygen comments in header as needed.