Skip CSR version check when cra_ring_root doesn't exist

zibaiwan · zibaiwan · commit 71ae4bbf748e · 2022-11-18T12:01:34.000-05:00
1. Parse the new field from the auto-discovery string
2. Changed csr_version to be optional.
3.Skip csr version check if cra_ring_root doesn't exit.
4.Update the forward-compatibility auto-discovery string test. Note:previous test had an issue: The number of fields per device global is fixed (here to 1), not variable. I had to update that as well.
5.Add new unit tests.
diff --git a/include/acl.h b/include/acl.h
@@ -526,6 +526,8 @@ typedef struct acl_device_def_autodiscovery_t {
   // Device global definition.
   std::unordered_map<std::string, acl_device_global_mem_def_t>
       device_global_mem_defs;
+  bool cra_ring_root_exist =
+      true; // Set the default value to true for backwards compatibility flows.
 } acl_device_def_autodiscovery_t;
 
 typedef struct acl_device_def_t {
diff --git a/include/acl_kernel_if.h b/include/acl_kernel_if.h
@@ -50,7 +50,28 @@ typedef struct {
 
   acl_bsp_io io;
 
-  unsigned int csr_version;
+  // csr_version is absent if there is no accelerators or cra_ring_root doesn't
+  // exist
+  std::optional<unsigned int> csr_version;
+
+  // The real kern->cra_ring_root_exist value is initialized in the
+  // acl_kernel_if_update() function (reading the actual auto-discovery string
+  // at that time).
+  // However, the kern->cra_ring_root_exist is referenced in the
+  // acl_kernel_if_post_pll_config_init() to check whether it's OK to read from
+  // the CSR. And acl_kernel_if_post_pll_config_init() can be called prior to
+  // the acl_kernel_if_update(), using a dummy auto-discovery string.
+  // Therefore we must set a default value. (Otherwise, it's reading an
+  // uninitialized value)
+
+  // The reason of setting it to false:
+  // Currently the cra read function will not be called in the early
+  // acl_kernel_if_post_pll_config_init() call, because the kern->num_accel is 0
+  // with dummy auto-discovery string, there is an if statement guard that. With
+  // the default value to false, it will hit the assertions in the cra
+  // read/write functions in case those are accidentally invoked too early,
+  // e.g., in a future code refactoring.
+  bool cra_ring_root_exist = false;
 
   // Depth of hardware kernel invocation queue
   unsigned int *accel_invoc_queue_depth;
diff --git a/src/acl_auto_configure.cpp b/src/acl_auto_configure.cpp
@@ -99,6 +99,25 @@ static bool read_uint_counters(const std::string &str,
   return true;
 }
 
+// Reads the next word in str and converts it into a boolean.
+// Returns true if a valid integer was read or false if an error occurred.
+// pos is updated to the position immediately following the parsed word
+// even if an error occurs.
+static bool read_bool_counters(const std::string &str,
+                               std::string::size_type &pos, bool &val,
+                               std::vector<int> &counters) noexcept {
+  std::string result;
+  pos = read_word(str, pos, result);
+  decrement_section_counters(counters);
+  try {
+    val = static_cast<bool>(std::stoi(result));
+  } catch (const std::exception &e) {
+    UNREFERENCED_PARAMETER(e);
+    return false;
+  }
+  return true;
+}
+
 // Reads the next word in str and converts it into an unsigned.
 // Returns true if a valid integer was read or false if an error occurred.
 // pos is updated to the position immediately following the parsed word
@@ -1052,6 +1071,12 @@ bool acl_load_device_def_from_str(const std::string &config_str,
         config_str, curr_pos, devdef.device_global_mem_defs, counters, err_str);
   }
 
+  // Check whether csr_ring_root exist in the IP.
+  if (result && counters.back() > 0) {
+    result = read_bool_counters(config_str, curr_pos,
+                                devdef.cra_ring_root_exist, counters);
+  }
+
   // forward compatibility: bypassing remaining fields at the end of device
   // description section
   while (result && counters.size() > 0 &&
diff --git a/src/acl_kernel_if.cpp b/src/acl_kernel_if.cpp
@@ -477,6 +477,7 @@ static uintptr_t acl_kernel_cra_set_segment_rom(acl_kernel_if *kern,
 
 static int acl_kernel_cra_read(acl_kernel_if *kern, unsigned int accel_id,
                                unsigned int addr, unsigned int *val) {
+  assert(kern->cra_ring_root_exist);
   uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
   acl_assert_locked_or_sig();
   return acl_kernel_if_read_32b(
@@ -485,6 +486,7 @@ static int acl_kernel_cra_read(acl_kernel_if *kern, unsigned int accel_id,
 
 int acl_kernel_cra_read_64b(acl_kernel_if *kern, unsigned int accel_id,
                             unsigned int addr, uint64_t *val) {
+  assert(kern->cra_ring_root_exist);
   uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
   acl_assert_locked_or_sig();
   return acl_kernel_if_read_64b(
@@ -536,6 +538,7 @@ static int acl_kernel_rom_cra_read_block(acl_kernel_if *kern, unsigned int addr,
 
 static int acl_kernel_cra_write(acl_kernel_if *kern, unsigned int accel_id,
                                 unsigned int addr, unsigned int val) {
+  assert(kern->cra_ring_root_exist);
   uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
   acl_assert_locked_or_sig();
   return acl_kernel_if_write_32b(
@@ -544,6 +547,7 @@ static int acl_kernel_cra_write(acl_kernel_if *kern, unsigned int accel_id,
 
 static int acl_kernel_cra_write_64b(acl_kernel_if *kern, unsigned int accel_id,
                                     unsigned int addr, uint64_t val) {
+  assert(kern->cra_ring_root_exist);
   uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
   acl_assert_locked();
   return acl_kernel_if_write_64b(
@@ -553,6 +557,7 @@ static int acl_kernel_cra_write_64b(acl_kernel_if *kern, unsigned int accel_id,
 static int acl_kernel_cra_write_block(acl_kernel_if *kern,
                                       unsigned int accel_id, unsigned int addr,
                                       unsigned int *val, size_t size) {
+  assert(kern->cra_ring_root_exist);
   uintptr_t segment_offset = acl_kernel_cra_set_segment(kern, accel_id, addr);
   uintptr_t logical_addr =
       kern->accel_csr[accel_id].address + addr - OFFSET_KERNEL_CRA;
@@ -712,8 +717,6 @@ int acl_kernel_if_init(acl_kernel_if *kern, acl_bsp_io bsp_io,
   kern->accel_perf_mon = NULL;
   kern->accel_num_printfs = NULL;
 
-  kern->csr_version = 0;
-
   kern->autorun_profiling_kernel_id = -1;
 
   // The simulator doesn't have any kernel interface information until the aocx
@@ -897,6 +900,11 @@ int acl_kernel_if_update(const acl_device_def_autodiscovery_t &devdef,
 
   acl_kernel_if_close(kern);
 
+  // Initialize whether the cra_ring_root exist in the design
+  kern->cra_ring_root_exist = devdef.cra_ring_root_exist;
+  ACL_KERNEL_IF_DEBUG_MSG(kern, "Number of cra_ring_root : %d\n",
+                          kern->cra_ring_root_exist);
+
   // Setup the PCIe HAL Structures
   kern->num_accel = static_cast<unsigned>(devdef.accel.size());
   ACL_KERNEL_IF_DEBUG_MSG(kern, "Number of Accelerators : %d\n",
@@ -1040,7 +1048,8 @@ int acl_kernel_if_post_pll_config_init(acl_kernel_if *kern) {
     kern->io.printf("HAL Kern Error: Post PLL init: Invalid kernel handle");
     assert(0 && "Invalid kernel handle");
   }
-  if (kern->num_accel > 0) {
+
+  if (kern->num_accel > 0 && kern->cra_ring_root_exist) {
     acl_kernel_cra_read(kern, 0, KERNEL_OFFSET_CSR, &csr);
     version = ACL_KERNEL_READ_BIT_RANGE(csr, KERNEL_CSR_LAST_VERSION_BIT,
                                         KERNEL_CSR_FIRST_VERSION_BIT);
@@ -1054,7 +1063,9 @@ int acl_kernel_if_post_pll_config_init(acl_kernel_if *kern) {
       kern->cra_address_offset = 0;
     }
   } else {
-    kern->csr_version = 0;
+    ACL_KERNEL_IF_DEBUG_MSG(kern,
+                            "No accelerator found or CRA ring root doesn't "
+                            "exist, not setting kern->csr_version\n");
   }
 
   // If environment variables set, configure the profile hardware
@@ -1161,7 +1172,8 @@ void acl_kernel_if_launch_kernel_on_custom_sof(
   // Version is defined in upper 16-bits of the status register and cached
   // in the kern->csr_version field.  The value is cached after PLL init to
   // avoid reading back on every kernel launch, which would add overhead.
-  if (!(kern->csr_version >= CSR_VERSION_ID_18_1 &&
+  if (kern->csr_version.has_value() &&
+      !(kern->csr_version >= CSR_VERSION_ID_18_1 &&
         kern->csr_version <= CSR_VERSION_ID)) {
     kern->io.printf("Hardware CSR version ID differs from version expected by "
                     "software.  Either:\n");
@@ -1253,7 +1265,8 @@ void acl_kernel_if_launch_kernel_on_custom_sof(
       }
     }
 
-    if (kern->csr_version != CSR_VERSION_ID_18_1) {
+    if (kern->csr_version.has_value() &&
+        (kern->csr_version != CSR_VERSION_ID_18_1)) {
       for (uintptr_t p = 0; p < image->arg_value_size; p += sizeof(int)) {
         unsigned int pword = *(unsigned int *)(image->arg_value + p);
         ACL_KERNEL_IF_DEBUG_MSG_VERBOSE(
@@ -1270,7 +1283,9 @@ void acl_kernel_if_launch_kernel_on_custom_sof(
     acl_kernel_cra_write_block(kern, accel_id, offset, (unsigned int *)image_p,
                                image_size_static);
   }
-  if (kern->csr_version != CSR_VERSION_ID_18_1 && image->arg_value_size > 0) {
+
+  if (kern->csr_version.has_value() &&
+      (kern->csr_version != CSR_VERSION_ID_18_1 && image->arg_value_size > 0)) {
     acl_kernel_cra_write_block(
         kern, accel_id, offset + (unsigned int)image_size_static,
         (unsigned int *)image->arg_value, image->arg_value_size);
diff --git a/test/acl_auto_configure_test.cpp b/test/acl_auto_configure_test.cpp
@@ -482,11 +482,11 @@ TEST(auto_configure, many_ok_forward_compatibility) {
   // sections and subsections to check forward compatibility
 
   std::string str(VERSIONIDTOSTR(
-      ACL_AUTO_CONFIGURE_VERSIONID) " 28 "
+      ACL_AUTO_CONFIGURE_VERSIONID) " 29 "
                                     "sample40byterandomhash000000000000000000 "
                                     "a10gx 0 1 15 DDR 2 1 6 0 2147483648 100 "
                                     "100 100 100 200 200 200 200 0 0 0 0 2 "
-                                    "1 name1 1 name2 47 "
+                                    "1 name1 name2 0 0 47 "
                                     "40 external_sort_stage_0 0 128 1 0 0 1 0 "
                                     "1 0 1 10 0 0 4 1 0 0 0 500 500 500 0 0 "
                                     "0 0 1 1 1 3 1 1 1 3 1 0 0 800 800 800 "
@@ -1410,3 +1410,63 @@ TEST(auto_configure, two_streaming_args_and_non_streaming_kernel) {
     CHECK(!args[i].streaming_arg_info_available);
   }
 }
+
+TEST(auto_configure, cra_ring_root_not_exist) {
+  const std::string config_str{
+      "23 50 2ccb683dee8e34a004c1a27e6d722090a8cc684d custom_ipa 0 1 9 0 2 1 2 "
+      "0 2199023255552 3 "
+      "- 0 6 5 ZTSZ4mainE4MyIP_arg_input_a 1 0 8 32768 "
+      "ZTSZ4mainE4MyIP_arg_input_b 1 0 8 32768 ZTSZ4mainE4MyIP_arg_input_c"
+      " 1 0 8 32768 ZTSZ4mainE4MyIP_arg_n 1 0 4 32768 "
+      "ZTSZ4mainE4MyIP_streaming_start 1 0 0 32768 "
+      "ZTSZ4mainE4MyIP_streaming_done"
+      " 0 1 0 32768 0 0 0 0 1 64 _ZTSZ4mainE4MyIP 0 128 1 0 0 1 0 1 0 4 8 2 1 "
+      "8 4 0 0 1 ZTSZ4mainE4MyIP_arg_input_a 8"
+      " 2 1 8 4 0 0 1 ZTSZ4mainE4MyIP_arg_input_b 8 2 1 8 4 0 0 1 "
+      "ZTSZ4mainE4MyIP_arg_input_c"
+      " 8 0 0 4 1 0 0 1 ZTSZ4mainE4MyIP_arg_n 0 0 0 0 1 1 1 3 1 1 1 3 1 1 1 "
+      "ZTSZ4mainE4MyIP_streaming_start"
+      " ZTSZ4mainE4MyIP_streaming_done"};
+
+  acl_device_def_autodiscovery_t devdef;
+  {
+    bool result;
+    std::string err_str;
+    ACL_LOCKED(result =
+                   acl_load_device_def_from_str(config_str, devdef, err_str));
+    std::cerr << err_str;
+    CHECK(result);
+  }
+
+  CHECK_EQUAL(0, devdef.cra_ring_root_exist);
+}
+
+TEST(auto_configure, cra_ring_root_exist) {
+  const std::string config_str{
+      "23 50 2ccb683dee8e34a004c1a27e6d722090a8cc684d custom_ipa 0 1 9 0 2 1 2 "
+      "0 2199023255552 3 "
+      "- 0 6 5 ZTSZ4mainE4MyIP_arg_input_a 1 0 8 32768 "
+      "ZTSZ4mainE4MyIP_arg_input_b 1 0 8 32768 ZTSZ4mainE4MyIP_arg_input_c"
+      " 1 0 8 32768 ZTSZ4mainE4MyIP_arg_n 1 0 4 32768 "
+      "ZTSZ4mainE4MyIP_streaming_start 1 0 0 32768 "
+      "ZTSZ4mainE4MyIP_streaming_done"
+      " 0 1 0 32768 0 0 0 1 1 64 _ZTSZ4mainE4MyIP 0 128 1 0 0 1 0 1 0 4 8 2 1 "
+      "8 4 0 0 1 ZTSZ4mainE4MyIP_arg_input_a 8"
+      " 2 1 8 4 0 0 1 ZTSZ4mainE4MyIP_arg_input_b 8 2 1 8 4 0 0 1 "
+      "ZTSZ4mainE4MyIP_arg_input_c"
+      " 8 0 0 4 1 0 0 1 ZTSZ4mainE4MyIP_arg_n 0 0 0 0 1 1 1 3 1 1 1 3 1 1 1 "
+      "ZTSZ4mainE4MyIP_streaming_start"
+      " ZTSZ4mainE4MyIP_streaming_done"};
+
+  acl_device_def_autodiscovery_t devdef;
+  {
+    bool result;
+    std::string err_str;
+    ACL_LOCKED(result =
+                   acl_load_device_def_from_str(config_str, devdef, err_str));
+    std::cerr << err_str;
+    CHECK(result);
+  }
+
+  CHECK_EQUAL(1, devdef.cra_ring_root_exist);
+}
