VITIS-9706: xrt support for adf event apis #7926

saumyag-xilinx · 2024-02-06T06:32:44Z

Problem solved by the commit

https://jira.xilinx.com/browse/VITIS-9706
Providing XRT support for Event APIs.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

None

How problem was solved, alternative solutions (if any) and why they were rejected

Implemented an Event class whose constructor xrt::aie::event is used to create event object.
The member function start_profiling() is used to start performance counters in AI Engine as per the profiling option passed as an argument.
The read_profiling() member function will return the current performance counter value associated with the event handle.
The stop_profiling() member function stops the performance profiling associated with the event handle.

Risks (if any) associated the changes in the commit

Low

What has been tested and how, request additional testing if necessary

Tested for an application that calculates throughout

Documentation impact (if any)

NA

chvamshi-xilinx

Once event is created, we should be able to start , stop and read. Please make requested changes

chvamshi-xilinx · 2024-02-11T08:33:21Z

src/runtime_src/core/include/xrt/xrt_aie.h

+{
+public:
+
+  event(const xrt::device& device, int option, const std::string& port1_name, const std::string& port2_name, uint32_t value);


instead of constructor taking all the variables, Can you have constructor which takes only device class
and have another member function to start the profiling.

src/runtime_src/core/common/api/aie/xrt_graph.cpp

chvamshi-xilinx · 2024-02-11T08:41:45Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+
+}
+
+int get_event_handle(const std::shared_ptr<xrt::aie::event_impl>& event_ptr) {


Why is this extra function required? you can directly get handle using event_ptr->getHandle() function.

chvamshi-xilinx · 2024-02-11T08:42:00Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+    device->stop_profiling(handle);
+  }
+
+  int getHandle() const { return handle; }


naming convention.. Use get_handle instead of getHandle

chvamshi-xilinx · 2024-02-11T08:48:27Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

-static uint64_t
-read_profiling(xrtDeviceHandle dhdl, int phdl)
+static std::shared_ptr<xrt::aie::event_impl>
+start_profiling(xrtDeviceHandle dhdl, int option, const char *port1_name, const char *port2_name, uint32_t value)


these can be part of event_impl class functions once you make all other changes.

chvamshi-xilinx · 2024-02-14T10:48:58Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+    if(event_hdl == invalid_handle)
+      throw xrt_core::error(-EINVAL, "Not a valid event handle");
+    device->stop_profiling(event_hdl);
+    event_hdl= invalid_handle;


indentation ..

chvamshi-xilinx · 2024-02-14T10:49:19Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+  using handle = int;
+  static constexpr handle invalid_handle = -1;
+
+  event_impl(std::shared_ptr<xrt_core::device> dev, handle hdl)


dont need to send hdl.

chvamshi-xilinx · 2024-02-14T10:49:43Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+
+event::
+event(const xrt::device& device)
+  : impl(open_event(device))


chvamshi-xilinx · 2024-02-14T10:49:58Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

-    return start_profiling(handle, option, port1Name, port2Name, value);
+    auto impl = open_event(handle);
+    auto hdl = impl->start_profiling(option, port1Name, port2Name, value);
+    impl->get_handle() = hdl;


dont require this line

chvamshi-xilinx · 2024-02-14T10:50:24Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+      return it->second->read_profiling();
+    }
+    else
+        throw xrt_core::error(-EINVAL, "No such event handle"); 


indentation

chvamshi-xilinx · 2024-02-14T10:50:39Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+        event_cache.erase(pHandle);
+    }
+    else
+	throw xrt_core::error(-EINVAL, "No such event handle");   


indentation

chvamshi-xilinx · 2024-02-14T10:51:34Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

-  auto device = xrt_core::device_int::get_core_device(dhdl);
-  return device->read_profiling(phdl);
+  auto core_device = xrt_core::device_int::get_core_device(dhdl);
+  auto phdl = std::make_shared<xrt::aie::event_impl>(core_device, xrt::aie::event_impl::invalid_handle);


remove hdl from constructor

stsoe

Please add comments to minimum public APIs and format the public APIs properly.

stsoe

The public APIs are still not documented or formatted correctly; I am referring to xrt_aie.h.

stsoe

Quite a few requests, please review with @chvamshi-xilinx
You can pick and choose as you like, but the public API comments are most important.

stsoe · 2024-02-23T04:08:22Z

src/runtime_src/core/include/xrt/xrt_aie.h

+   * event() - Constructor from a device
+   *
+   * @param device
+   *  The device on which the profiling event should start


This is a bit too obfuscated. Line 175 is the first that hints at this having something to do with profiling.
The class name event is rather generic, but I think this event is specifically for profiling. Would it be better to make that clear by moving event under namespace xrt::aie::profiling ? And if so then change start/read/stop_profiling to just start/read/stop ?

Sorry for being picky here, but this is a public API that we can't change in the future, so we better get it right the first time.

What happens when you create an event? What is an event?

Noted. Thank you for the suggestion Soren. Also for clarity on the event APIs, I'm referring UG-1076

stsoe · 2024-02-23T04:13:24Z

src/runtime_src/core/include/xrt/xrt_aie.h

+  /**
+   * start_profiling() - Start AIE performance profiling
+   *
+   * @param option
+   *  Profiling option
+   * @param port1_name
+   *  Profiling port 1 name.
+   * @param port2_name
+   *  Profiling port 2 name.
+   * @param value
+   *  The number of bytes to trigger the profiling event.
+   *
+   * This function configures the performance counters in AI Engine by given
+   * port names and value. The port names and value will have different
+   * meanings on different options.
+   */
+  int 
+  start_profiling(int option, const std::string& port1_name, const std::string& port2_name, uint32_t value) const;


This will not work in a public API. What is option? It's an int what happens if I pass my lucky number 17? Kidding aside, we need a class enum here. Also, the other parameters are not clear at all, what is profiling port? What does number of bytes to trigger the profiling event mean? Is 0 bytes good? It's not useable in a public API without a lot more documentation and explanation as to what happens with out or range numbers. How do know what port names to use?

Noted.
I have added an enum class of profiling option. Also, the other parameters like profiling port etc are the PLIO/ GMIO ports here. I am referring (UG-1076) for implementation.

stsoe · 2024-02-23T04:13:57Z

src/runtime_src/core/include/xrt/xrt_aie.h

+  uint64_t
+  read_profiling() const;


What does the profiling counter value represent? I am beginning to wonder who the consumer of these event APIs will be? Will it be end-user from application, or will it be internal XRT (xdp?) using this?

The consumer is the end user from application. To understand the adf APIs I referred UG-1076

stsoe · 2024-02-23T04:17:08Z

src/runtime_src/core/include/xrt/xrt_aie.h

+  stop_profiling() const;
+
+private:
+    std::shared_ptr<event_impl> impl;


If this is meant to be a public class, then maybe use core/include/xrt/detail/pimpl.h as base class; look at other examples.

stsoe · 2024-02-23T04:23:32Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+  ~event_impl()
+  {}
+
+  int 


Suggested change

int

handle

stsoe · 2024-02-23T04:40:35Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+    if (it != event_cache.end()) {
+      return it->second->read_profiling();
+    }


Suggested change

if (it != event_cache.end()) {

return it->second->read_profiling();

}

if (it != event_cache.end())

return it->second->read_profiling();

stsoe · 2024-02-23T04:43:15Z

src/runtime_src/doc/toc/xrt_native_apis.rst

+      :number-lines: 45
+
+           auto graph = xrt::graph(device, xclbin_uuid, "graph_name");
+           auto handle = event.start_profiling(int option, std::string& port1, std::string& port2, int value);


I was hoping this documentation would explain the semantics of option and value :-(

Suggested change

auto handle = event.start_profiling(int option, std::string& port1, std::string& port2, int value);

auto handle = event.start_profiling(option, std::string& port1, std::string& port2, int value);

stsoe · 2024-02-23T04:43:59Z

src/runtime_src/doc/toc/xrt_native_apis.rst

+      :number-lines: 35
+
+        event.stop_profiling();
+        double throughput = (double)output_size_in_bytes / (cycle_count *0.8 * 1e-3); 


Why the cast?

stsoe · 2024-02-23T04:44:33Z

src/runtime_src/doc/toc/xrt_native_apis.rst

+        event.stop_profiling();
+        double throughput = (double)output_size_in_bytes / (cycle_count *0.8 * 1e-3); 
+        //Every AIE cycle is 0.8ns in production board
+        std::cout<<"Throughput of the graph: "<<throughput<<" MB/s"<<std::endl;


Suggested change

std::cout<<"Throughput of the graph: "<<throughput<<" MB/s"<<std::endl;

std::cout << "Throughput of the graph: " << throughput << " MB/s" << std::endl;

stsoe · 2024-02-23T04:44:41Z

src/runtime_src/doc/toc/xrt_native_apis.rst

+
+        event.stop_profiling();
+        double throughput = (double)output_size_in_bytes / (cycle_count *0.8 * 1e-3); 
+        //Every AIE cycle is 0.8ns in production board


Suggested change

//Every AIE cycle is 0.8ns in production board

// Every AIE cycle is 0.8ns in production board

minor fix review suggestions minor change Signed-off-by: saumya garg <[email protected]> minor change Signed-off-by: saumya garg <[email protected]> minor change Signed-off-by: saumya garg <[email protected]>

fix Signed-off-by: saumya garg <[email protected]> fix Signed-off-by: saumya garg <[email protected]>

Signed-off-by: saumya garg <[email protected]>

Signed-off-by: saumya garg <[email protected]> doc update2 Signed-off-by: saumya garg <[email protected]>

Signed-off-by: saumya garg <[email protected]>

Signed-off-by: saumya garg <[email protected]> fix Signed-off-by: saumya garg <[email protected]>

Signed-off-by: saumya garg <[email protected]> fix review comments Signed-off-by: saumya garg <[email protected]>

stsoe

Looks a lot better now. Few more notes.

stsoe · 2024-02-27T16:33:45Z

src/runtime_src/core/include/xrt/xrt_aie.h

+class profiling : public detail::pimpl<profiling_impl>
+{
+public:
+  enum class profiling_option : int { io_total_stream_running_to_idle_cycles = 0, io_stream_start_to_bytes_transferred_cycles = 1, io_stream_start_difference_cycles = 2, io_stream_running_event_count = 3 };


Suggested change

enum class profiling_option : int { io_total_stream_running_to_idle_cycles = 0, io_stream_start_to_bytes_transferred_cycles = 1, io_stream_start_difference_cycles = 2, io_stream_running_event_count = 3 };

enum class profiling_option : int

{

io_total_stream_running_to_idle_cycles = 0,

io_stream_start_to_bytes_transferred_cycles = 1,

io_stream_start_difference_cycles = 2,

io_stream_running_event_count = 3

};

You would maybe want to document these?

/** * @enum profiliing_options - ... * * @var enumerator * What it does * /

I know the enumerators are long, so maybe they explain what needs to be explained?

stsoe · 2024-02-27T16:36:07Z

src/runtime_src/core/include/xrt/xrt_aie.h

+   *  PLIO/GMIO port 1 name.
+   * @param port2_name
+   *  PLIO/GMIO port 2 name.
+   * @param value


I know this is in some user guide. Without providing the hyperlink, you could reference UGxxxx for more detail. Without the UG, I would be baffled on the meaning of these parameters.

stsoe · 2024-02-27T16:37:13Z

src/runtime_src/core/include/xrt/xrt_aie.h

+  stop() const;
+
+private:
+  std::shared_ptr<profiling_impl> impl;


Uh, not needed now that you derived off of detail::pimpl

stsoe · 2024-02-27T16:41:43Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

+  {
+    device->stop_profiling(profiling_hdl);
+  }


Uh, in conjunction with user callling stop() this dtor won't work. I think you need a bit more logic to the state so stop_profiling can bail if it was already called. My point earlier was that you must handle the case where user forgets to call stop() but you must also handle the case where user does call stop() and the handle is invalid.

Destructors cannot throw, so you must have a try/catch if the function that is called can throw.

Signed-off-by: saumya garg <[email protected]> fix review comments Signed-off-by: saumya garg <[email protected]>

chvamshi-xilinx · 2024-03-03T02:14:05Z

src/runtime_src/core/common/api/aie/xrt_graph.cpp

@@ -755,7 +851,16 @@ int
 xrtAIEStartProfiling(xrtDeviceHandle handle, int option, const char *port1Name, const char *port2Name, uint32_t value)
 {
  try {
-    return start_profiling(handle, option, port1Name, port2Name, value);
+    auto event = create_profiling_event(handle);
+    if (option < 0 || option > 3)


magic numbers, can you replace this with proper enums? You can fix it in next PR.

saumyag-xilinx requested review from stsoe, rozumx and uday610 as code owners February 6, 2024 06:32

saumyag-xilinx requested a review from chvamshi-xilinx February 6, 2024 06:33

saumyag-xilinx force-pushed the event branch 3 times, most recently from d953946 to 479e00d Compare February 6, 2024 08:19

chvamshi-xilinx approved these changes Feb 8, 2024

View reviewed changes

chvamshi-xilinx requested changes Feb 11, 2024

View reviewed changes

saumyag-xilinx force-pushed the event branch 2 times, most recently from a621e7c to 97ede86 Compare February 14, 2024 09:46

saumyag-xilinx requested a review from chvamshi-xilinx February 14, 2024 09:49

chvamshi-xilinx reviewed Feb 14, 2024

View reviewed changes

chvamshi-xilinx requested changes Feb 14, 2024

View reviewed changes

saumyag-xilinx requested a review from chvamshi-xilinx February 14, 2024 11:52

stsoe requested changes Feb 15, 2024

View reviewed changes

chvamshi-xilinx approved these changes Feb 15, 2024

View reviewed changes

saumyag-xilinx force-pushed the event branch from 34bc318 to cc9066f Compare February 20, 2024 09:45

saumyag-xilinx requested review from stsoe and chvamshi-xilinx February 20, 2024 09:46

saumyag-xilinx force-pushed the event branch from cc9066f to f4bfac9 Compare February 20, 2024 10:40

stsoe requested changes Feb 20, 2024

View reviewed changes

saumyag-xilinx requested a review from stsoe February 22, 2024 04:55

chvamshi-xilinx approved these changes Feb 22, 2024

View reviewed changes

stsoe requested changes Feb 23, 2024

View reviewed changes

saumyag-xilinx force-pushed the event branch 2 times, most recently from 59e12e6 to 6ce5255 Compare February 23, 2024 16:00

saumya garg added 2 commits February 27, 2024 17:36

event class implementation

c4a73f1

minor fix review suggestions minor change Signed-off-by: saumya garg <[email protected]> minor change Signed-off-by: saumya garg <[email protected]> minor change Signed-off-by: saumya garg <[email protected]>

review-fixes

e71387f

fix Signed-off-by: saumya garg <[email protected]> fix Signed-off-by: saumya garg <[email protected]>

saumya garg added 4 commits February 27, 2024 17:36

fix review comments

dfcc10e

Signed-off-by: saumya garg <[email protected]>

doc update

cb9ada8

Signed-off-by: saumya garg <[email protected]> doc update2 Signed-off-by: saumya garg <[email protected]>

fix review comment

c0afb67

Signed-off-by: saumya garg <[email protected]>

fix review comments

666640a

Signed-off-by: saumya garg <[email protected]> fix Signed-off-by: saumya garg <[email protected]>

saumyag-xilinx force-pushed the event branch from 6ce5255 to 282e04c Compare February 27, 2024 12:07

fix review comments

08b09ba

Signed-off-by: saumya garg <[email protected]> fix review comments Signed-off-by: saumya garg <[email protected]>

saumyag-xilinx force-pushed the event branch from 282e04c to 08b09ba Compare February 27, 2024 12:12

saumyag-xilinx requested a review from stsoe February 27, 2024 12:13

stsoe requested changes Feb 27, 2024

View reviewed changes

fix review comments

fdea9e9

Signed-off-by: saumya garg <[email protected]> fix review comments Signed-off-by: saumya garg <[email protected]>

saumyag-xilinx force-pushed the event branch from 8e6dca1 to fdea9e9 Compare February 28, 2024 09:08

saumyag-xilinx requested a review from stsoe February 28, 2024 09:09

stsoe approved these changes Feb 28, 2024

View reviewed changes

chvamshi-xilinx reviewed Mar 3, 2024

View reviewed changes

chvamshi-xilinx merged commit 275a150 into Xilinx:master Mar 3, 2024
22 checks passed

chvamshi-xilinx mentioned this pull request Apr 29, 2024

Fixing adf::event crash. Regression from last release #8119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VITIS-9706: xrt support for adf event apis #7926

VITIS-9706: xrt support for adf event apis #7926

saumyag-xilinx commented Feb 6, 2024 •

edited

Loading

chvamshi-xilinx left a comment

chvamshi-xilinx Feb 11, 2024

chvamshi-xilinx Feb 11, 2024

chvamshi-xilinx Feb 11, 2024

chvamshi-xilinx Feb 11, 2024

chvamshi-xilinx Feb 14, 2024

chvamshi-xilinx Feb 14, 2024

chvamshi-xilinx Feb 14, 2024

chvamshi-xilinx Feb 14, 2024

chvamshi-xilinx Feb 14, 2024

chvamshi-xilinx Feb 14, 2024

chvamshi-xilinx Feb 14, 2024

stsoe left a comment

stsoe left a comment

stsoe left a comment

stsoe Feb 23, 2024

saumyag-xilinx Feb 23, 2024

stsoe Feb 23, 2024

saumyag-xilinx Feb 23, 2024

stsoe Feb 23, 2024

saumyag-xilinx Feb 23, 2024

stsoe Feb 23, 2024

stsoe Feb 23, 2024

stsoe Feb 23, 2024

stsoe Feb 23, 2024

stsoe Feb 23, 2024

stsoe Feb 23, 2024

stsoe Feb 23, 2024

stsoe left a comment

stsoe Feb 27, 2024

stsoe Feb 27, 2024

stsoe Feb 27, 2024

stsoe Feb 27, 2024

chvamshi-xilinx Mar 3, 2024


		}

		int get_event_handle(const std::shared_ptr<xrt::aie::event_impl>& event_ptr) {

	auto handle = event.start_profiling(int option, std::string& port1, std::string& port2, int value);
	auto handle = event.start_profiling(option, std::string& port1, std::string& port2, int value);

	std::cout<<"Throughput of the graph: "<<throughput<<" MB/s"<<std::endl;
	std::cout << "Throughput of the graph: " << throughput << " MB/s" << std::endl;

	//Every AIE cycle is 0.8ns in production board
	// Every AIE cycle is 0.8ns in production board

VITIS-9706: xrt support for adf event apis #7926

VITIS-9706: xrt support for adf event apis #7926

Conversation

saumyag-xilinx commented Feb 6, 2024 • edited Loading

Problem solved by the commit

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

Risks (if any) associated the changes in the commit

What has been tested and how, request additional testing if necessary

Documentation impact (if any)

chvamshi-xilinx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsoe left a comment

Choose a reason for hiding this comment

stsoe left a comment

Choose a reason for hiding this comment

stsoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saumyag-xilinx commented Feb 6, 2024 •

edited

Loading