Address comments on PR

Signed-off-by: rbramand <[email protected]>

src/runtime_src/core/common/api/hw_context_int.h

@@ -44,6 +44,9 @@ XRT_CORE_COMMON_EXPORT
 xrt::hw_context
 create_hw_context_from_implementation(void* hwctx_impl);
 
+// Checks all the modules that are registered with given hw context
+// and returns the module with the given kernel name
+// throws if no module is found with given kernel name
 xrt::module
 get_module(const xrt::hw_context& hwctx, const std::string& kname);
 

src/runtime_src/core/common/api/module_int.h

@@ -1,12 +1,13 @@
 // SPDX-License-Identifier: Apache-2.0
-// Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
+// Copyright (C) 2023-2024 Advanced Micro Devices, Inc. All rights reserved.
 //
 // Xilinx Runtime (XRT) Experimental APIs
 
 #ifndef _XRT_COMMON_MODULE_INT_H_
 #define _XRT_COMMON_MODULE_INT_H_
 
 // This file defines implementation extensions to the XRT Kernel APIs.
+#include "core/common/xclbin_parser.h"
 #include "core/include/experimental/xrt_bo.h"
 #include "core/include/experimental/xrt_module.h"
 
@@ -15,6 +16,10 @@
 #include <string>
 
 namespace xrt_core::module_int {
+struct kernel_info {
+  std::vector<xrt_core::xclbin::kernel_argument> args;
+  xrt_core::xclbin::kernel_properties props;
+};
 
 // Fill in ERT command payload in ELF flow. The payload is after extra_cu_mask
 // and before CU arguments.
@@ -57,13 +62,14 @@ get_ert_opcode(const xrt::module& module);
 void
 dump_scratchpad_mem(const xrt::module& module);
 
-std::string
-get_kernel_signature(const xrt::module& module);
+// Returns kernel info extracted from demangled kernel signature
+// eg : DPU(void*, void*, void*)
+// returns kernel name (DPU), kernel args and kernel properties
+// throws exception if Elf passed has no kernel info
+const kernel_info&
+get_kernel_info(const xrt::module& module);
 
-std::string
-get_kernel_name(const xrt::module& module);
-
-// Get partition size if ELF has info
+// Get partition size if ELF has the info
 uint32_t
 get_partition_size(const xrt::module& module);
 

src/runtime_src/core/common/api/xrt_hw_context.cpp

@@ -70,12 +70,15 @@ class hw_context_impl : public std::enable_shared_from_this<hw_context_impl>
     , m_cfg_param{std::move(cfg_param)}
     , m_mode{mode}
   {
+    // Create module object to parse Elf
     auto module = xrt::module(elf);
-    auto kernel_name = xrt_core::module_int::get_kernel_name(module);
-    m_module_map[kernel_name] = std::move(module);
-
-    m_partition_size = xrt_core::module_int::get_partition_size(m_module_map.begin()->second);
+    // Get partition size and pass it to diver for hw ctx creation
+    m_partition_size = xrt_core::module_int::get_partition_size(module);
     m_hdl = m_core_device->create_hw_context(m_partition_size, m_cfg_param, mode);
+
+    // creation successful, store the module in the map
+    auto kernel_name = xrt_core::module_int::get_kernel_info(module).props.name;
+    m_module_map[kernel_name] = std::move(module);
   }
 
   std::shared_ptr<hw_context_impl>
@@ -110,7 +113,7 @@ class hw_context_impl : public std::enable_shared_from_this<hw_context_impl>
   add_config(const xrt::elf& elf)
   {
     auto module = xrt::module(elf);
-    auto kernel_name = xrt_core::module_int::get_kernel_name(module);
+    auto kernel_name = xrt_core::module_int::get_kernel_info(module).props.name;
     auto part_size = xrt_core::module_int::get_partition_size(module);
 
     // create hw ctx handle if not already created
@@ -127,10 +130,8 @@ class hw_context_impl : public std::enable_shared_from_this<hw_context_impl>
       throw std::runtime_error("can not add config to ctx with different configuration\n");
 
     // add module to map if kernel name is different, else throw
-    for (const auto& m : m_module_map) {
-      if (kernel_name == xrt_core::module_int::get_kernel_name(m.second))
-        throw std::runtime_error("config with kernel already exists, cannot add this config\n");
-    }
+    if (m_module_map.find(kernel_name) != m_module_map.end())
+      throw std::runtime_error("config with kernel already exists, cannot add this config\n");
     m_module_map[kernel_name] = std::move(module);
   }
 
@@ -186,11 +187,10 @@ class hw_context_impl : public std::enable_shared_from_this<hw_context_impl>
   xrt::module
   get_module(const std::string& kname) const
   {
-    for (const auto& m : m_module_map) {
-      if (kname == xrt_core::module_int::get_kernel_name(m.second))
-        return m.second;
-    }
-    throw std::runtime_error("no module found with given kernel name in ctx");
+    auto itr = m_module_map.find(kname);
+    if (itr == m_module_map.end())
+      throw std::runtime_error("no module found with given kernel name in ctx");
+    return itr->second;
   }
 };
 

src/runtime_src/core/common/api/xrt_kernel.cpp

@@ -1306,7 +1306,7 @@ class kernel_impl : public std::enable_shared_from_this<kernel_impl>
   xrt::xclbin::kernel xkernel;         // kernel xclbin metadata
   std::vector<argument> args;          // kernel args sorted by argument index
   std::vector<ipctx> ipctxs;           // CU context locks
-  property_type properties;            // Kernel properties from XML meta
+  const property_type& properties;     // Kernel properties from XML meta
   std::bitset<max_cus> cumask;         // cumask for command execution
   size_t regmap_size = 0;              // CU register map size
   size_t fa_num_inputs = 0;            // Fast adapter number of inputs per meta data
@@ -1505,6 +1505,41 @@ class kernel_impl : public std::enable_shared_from_this<kernel_impl>
     return data;  // no skipping
   }
 
+  xrt::module
+  get_module(const xrt::hw_context& ctx, const std::string& kname)
+  {
+    // ELF use case, identify module from ctx that has given kernel name and
+    // get kernel signature from the module to construct kernel args etc
+    // kernel name will be of format - <kernel_name>:<index>
+    auto i = kname.find(":");
+    if (i == std::string::npos) {
+      // default case - ctrl code 0 will be used
+      name = kname.substr(0, kname.size());
+      m_ctrl_code_index = 0;
+    }
+    else {
+      name = kname.substr(0, i);
+      m_ctrl_code_index = std::stoul(kname.substr(i+1, kname.size()-i-1));
+    }
+
+    return xrt_core::hw_context_int::get_module(ctx, name);
+  }
+
+  const property_type&
+  get_elf_kernel_properties()
+  {
+    // Get kernel info from module
+    const auto& kernel_info = xrt_core::module_int::get_kernel_info(m_module);
+    if (name != kernel_info.props.name)
+      throw std::runtime_error("Kernel name mismatch, incorrect module picked\n");
+
+    // set kernel args
+    for (auto& arg : kernel_info.args)
+      args.emplace_back(arg);
+
+    return kernel_info.props;
+  }
+
   static uint32_t
   create_uid()
   {
@@ -1521,64 +1556,6 @@ class kernel_impl : public std::enable_shared_from_this<kernel_impl>
     throw xrt_core::error("No such kernel '" + nm + "'");
   }
 
-  static std::vector<std::string>
-  split(const std::string& s, char delimiter)
-  {
-    std::vector<std::string> tokens;
-    std::stringstream ss(s);
-    std::string item;
-
-    while (getline(ss, item, delimiter))
-      tokens.push_back(item);
-
-    return tokens;
-  }
-
-  void
-  construct_elf_kernel_args(const std::string& kernel_name)
-  {
-    // kernel signature - name(argtype, argtype ...)
-    size_t start_pos = kernel_name.find('(');
-    size_t end_pos = kernel_name.find(')', start_pos);
-
-    if (start_pos == std::string::npos || end_pos == std::string::npos || start_pos > end_pos)
-      throw std::runtime_error("Failed to construct kernel args");
-
-    std::string argstring = kernel_name.substr(start_pos + 1, end_pos - start_pos - 1);
-    std::vector<std::string> argstrings = split(argstring, ',');
-
-    size_t count = 0;
-    size_t offset = 0;
-    for (const std::string& str : argstrings) {
-      xrt_core::xclbin::kernel_argument arg;
-      arg.name = "argv" + std::to_string(count);
-      arg.hosttype = "no-type";
-      arg.port = "no-port";
-      arg.index = count;
-      arg.offset = offset;
-      arg.dir = xrt_core::xclbin::kernel_argument::direction::input;
-      // if arg has pointer(*) in its name (eg: char*, void*) it is of type global otherwise scalar
-      arg.type = (str.find('*') != std::string::npos)
-               ? xrt_core::xclbin::kernel_argument::argtype::global
-               : xrt_core::xclbin::kernel_argument::argtype::scalar;
-
-      // At present only global args are supported
-      // TODO : Add support for scalar args in ELF flow
-      if (arg.type == xrt_core::xclbin::kernel_argument::argtype::scalar)
-        throw std::runtime_error("scalar args are not yet supported for this kind of kernel");
-      else {
-        // global arg
-        static constexpr size_t global_arg_size = 0x8;
-        arg.size = global_arg_size;        
-
-        offset += global_arg_size;
-      }
-
-      args.emplace_back(arg);
-      count++;
-    }
-  }
-
 public:
   // kernel_type - constructor
   //
@@ -1639,51 +1616,17 @@ class kernel_impl : public std::enable_shared_from_this<kernel_impl>
   }
 
   kernel_impl(std::shared_ptr<device_type> dev, xrt::hw_context ctx, const std::string& nm)
-    : device(std::move(dev))    // share ownership
-    , hwctx(std::move(ctx))     // hw context
-    , hwqueue(hwctx)            // hw queue
+    : device(std::move(dev))                    // share ownership
+    , hwctx(std::move(ctx))                     // hw context
+    , hwqueue(hwctx)                            // hw queue
+    , m_module(get_module(hwctx, nm))           // module object with matching kernel name
+    , properties(get_elf_kernel_properties())   // kernel info present in Elf
     , uid(create_uid())
   {
     XRT_DEBUGF("kernel_impl::kernel_impl(%d)\n", uid);
 
-    // ELF use case, identify module from ctx that has given kernel name and
-    // get kernel signature from the module to construct kernel args etc
-
-    // kernel name will be of format - <kernel_name>:<index>
-    auto i = nm.find(":");
-    if (i == std::string::npos) {
-      // default case - ctrl code 0 will be used
-      name = nm.substr(0, nm.size());
-      m_ctrl_code_index = 0;
-    }
-    else {
-      name = nm.substr(0, i);
-      m_ctrl_code_index = std::stoul(nm.substr(i+1, nm.size()-i-1));
-    }
-
-    m_module = xrt_core::hw_context_int::get_module(hwctx, name);
-    auto demangled_name = xrt_core::module_int::get_kernel_signature(m_module);
-
-    // extract kernel name
-    size_t pos = demangled_name.find('(');
-    if (pos == std::string::npos)
-      throw std::runtime_error("Failed to get kernel - " + nm);
-
-    if (name != demangled_name.substr(0, pos))
-      throw std::runtime_error("Kernel name mismatch, incorrect module picked\n");
-
-    construct_elf_kernel_args(demangled_name);
-
-    // fill kernel properties
-    properties.name = name;
-    properties.type = xrt_core::xclbin::kernel_properties::kernel_type::dpu;
-    properties.counted_auto_restart = xrt_core::xclbin::get_restart_from_ini(name);
-    properties.mailbox = xrt_core::xclbin::get_mailbox_from_ini(name);
-    properties.sw_reset = xrt_core::xclbin::get_sw_reset_from_ini(name);
-
     // amend args with computed data based on kernel protocol
     amend_args();
-
     m_usage_logger->log_kernel_info(device->core_device.get(), hwctx, name, args.size());
   }
 
@@ -2174,7 +2117,7 @@ class run_impl
     auto kcmd = pkt->get_ert_cmd<ert_start_kernel_cmd*>();
     auto payload = kernel->initialize_command(pkt);
     if (kcmd->opcode == ERT_START_DPU || kcmd->opcode == ERT_START_NPU || kcmd->opcode == ERT_START_NPU_PREEMPT ||
-        kcmd->opcode == ERT_START_NPU_PDI_IN_ELF) {
+        kcmd->opcode == ERT_START_NPU_PREEMPT_ELF) {
       auto payload_past_dpu = initialize_dpu(payload);
 
       // adjust count to include the prepended ert_dpu_data structures