[GitHub] [tvm] janetsc commented on a diff in pull request #13381: [Hexagon] Enable Hexagon User DMA bypass mode

[GitBox <gi...@apache.org>]

janetsc commented on code in PR #13381:
URL: https://github.com/apache/tvm/pull/13381#discussion_r1022106437


##########
src/runtime/hexagon/hexagon_buffer.cc:
##########
@@ -237,8 +236,15 @@ void hexagon_buffer_copy_across_regions(const BufferSet& dest, const BufferSet&
 
   // Finally, do the memory copies.
   for (const auto& copy : macro_copies) {
-    int error_code = hexagon_user_dma_1d_sync(copy.dest, copy.src, copy.num_bytes);
-    CHECK_EQ(error_code, 0);
+    qurt_mem_cache_clean(reinterpret_cast<qurt_addr_t>(copy.dest), copy.num_bytes,
+                         QURT_MEM_CACHE_INVALIDATE, QURT_MEM_DCACHE);
+    qurt_mem_cache_clean(reinterpret_cast<qurt_addr_t>(copy.src), copy.num_bytes,
+                         QURT_MEM_CACHE_INVALIDATE, QURT_MEM_DCACHE);
+    memcpy(copy.dest, copy.src, copy.num_bytes);
+    qurt_mem_cache_clean(reinterpret_cast<qurt_addr_t>(copy.dest), copy.num_bytes,

Review Comment:
   It seems the calls after the copy should not be necessary, particularly for the src buffer.  Are these a temporary measure?



##########
tests/cpp-runtime/hexagon/hexagon_user_dma_tests.cc:
##########
@@ -224,3 +228,124 @@ TEST_F(HexagonUserDMATest, overflow_ring_buffer) {
     ASSERT_EQ(src_char[i], dst_char[i]);
   }
 }
+
+TEST_F(HexagonUserDMATest, sync_dma_bypass) {
+  HexagonBuffer srchb(length, kHexagonAllocAlignment, global_scope);
+  HexagonBuffer dsthb(length, kHexagonAllocAlignment, global_scope);
+  HexagonBuffer vtcmhb(length, kHexagonAllocAlignment, global_vtcm_scope);
+
+  // init src, dst HexagonBuffers
+  srchb.CopyFrom(src, length);
+  dsthb.CopyFrom(dst, length);
+
+  // DDR src -> VTCM
+  ret = user_dma->Copy(queue_id, vtcmhb.GetPointer(), srchb.GetPointer(), length, ENABLE_BYPASS);
+  ASSERT_EQ(ret, DMA_SUCCESS);
+
+  // VTCM -> DDR dst
+  ret = user_dma->Copy(queue_id, dsthb.GetPointer(), vtcmhb.GetPointer(), length, ENABLE_BYPASS);
+  ASSERT_EQ(ret, DMA_SUCCESS);
+
+  // wait for DMAs to complete
+  user_dma->Wait(queue_id, 0);
+
+  // copy answer from dst HexagonBuffer
+  dsthb.CopyTo(dst, length);
+
+  // verify
+  for (uint32_t i = 0; i < length; ++i) {
+    ASSERT_EQ(src_char[i], dst_char[i]);
+  }
+}
+
+TEST_F(HexagonUserDMATest, sync_dma_bypass_vtcm_to_vtcm) {
+  HexagonBuffer srchb(length, kHexagonAllocAlignment, global_scope);
+  HexagonBuffer dsthb(length, kHexagonAllocAlignment, global_scope);
+  HexagonBuffer vtcm1hb(length, kHexagonAllocAlignment, global_vtcm_scope);
+  HexagonBuffer vtcm2hb(length, kHexagonAllocAlignment, global_vtcm_scope);
+
+  // init src, dst HexagonBuffers
+  srchb.CopyFrom(src, length);
+  dsthb.CopyFrom(dst, length);
+
+  // DDR src -> VTCM
+  ret = user_dma->Copy(queue_id, vtcm1hb.GetPointer(), srchb.GetPointer(), length, ENABLE_BYPASS);
+  ASSERT_EQ(ret, DMA_SUCCESS);
+
+  // VTCM -> VTCM
+  // NOTE: Cache bypass is disabled for VTCM -> VTCM transfers
+  ret =
+      user_dma->Copy(queue_id, vtcm2hb.GetPointer(), vtcm1hb.GetPointer(), length, DISABLE_BYPASS);

Review Comment:
   Since this is the enabled test, should it be ENABLE_BYPASS?
   
   Edit - Ah, I see the comment.  But you should be able to say ENABLED, and have it still work.  It just won't actually enable anything.  Or, should it through if you attempt to set bypass if it is vtcm to vtcm?



##########
src/runtime/hexagon/hexagon_vtcm_pool.h:
##########
@@ -70,6 +70,16 @@ class HexagonVtcmPool {
   //! \brief Returns the total number of bytes in this pool
   size_t TotalBytes() { return reinterpret_cast<size_t>(vtcm_size_); }
 
+  bool IsVtcm(void* ptr, unsigned size) {
+    auto char_ptr = static_cast<char*>(ptr);
+    auto char_vtcm = static_cast<char*>(vtcm_data_);
+

Review Comment:
   Add CHECK(vtcm_data_ != nullptr)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org