You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/08/12 19:50:37 UTC
[GitHub] [tvm] adstraw opened a new pull request, #12411: [Hexagon] Asynchronous DMA support
adstraw opened a new pull request, #12411:
URL: https://github.com/apache/tvm/pull/12411
Adds adds asynchronous DMA support through the Hexagon User DMA engine with unit tests to validate basic functionality. Enables future testing and development for asynchronous memory copy on Hexagon. For now, Hexagon DMA support remains synchronous in nature through existing `hexagon_user_dma_1d_sync` interface which uses asynchronous capable `HexagonUserDMA` class in a synchronous way --- calling `Copy` and `Wait` back to back for each request.
--
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
[GitHub] [tvm] csullivan commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r951993684
##########
tests/cpp-runtime/hexagon/hexagon_user_dma_tests.cc:
##########
@@ -57,9 +57,9 @@ TEST_F(HexagonUserDMATest, wait) {
HexagonUserDMA::Get().Wait(10);
}
Review Comment:
Should we white-box test the ring-buffer impl?
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949709289
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <vector>
+
+#ifndef TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+#define TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+
+namespace tvm {
+namespace runtime {
+namespace hexagon {
+
+#define DMA_SUCCESS 0
+#define DMA_FAILURE -1
+
+class HexagonUserDMA {
+ public:
+ /*!
+ * \brief Initiate DMA to copy memory from source to destination address
+ * \param dst Destination address
+ * \param src Source address
+ * \param length Length in bytes to copy
+ * \returns Status, either DMA_SUCCESS or DMA_FAILURE
+ */
+ int Copy(void* dst, void* src, uint32_t length);
+
+ /*!
+ * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \param max_dmas_in_flight Maximum number of DMAs allowed to be in flight
+ * to satisfy the `Wait` e.g. use `Wait(0)` to wait on "all" outstanding DMAs to complete
+ */
+ void Wait(uint32_t max_dmas_in_flight);
+
+ /*!
+ * \brief Poll the number of DMAs in flight
+ * \returns Number of DMAs in flight
+ */
+ uint32_t Poll();
+
+ //! HexagonUserDMA uses the singleton pattern
+ static HexagonUserDMA& Get() {
+ static HexagonUserDMA* hud = new HexagonUserDMA();
+ return *hud;
+ }
+
+ private:
+ HexagonUserDMA() = default;
+ ~HexagonUserDMA();
+ HexagonUserDMA(const HexagonUserDMA&) = delete;
+ HexagonUserDMA& operator=(const HexagonUserDMA&) = delete;
+ HexagonUserDMA(HexagonUserDMA&&) = delete;
+ HexagonUserDMA& operator=(HexagonUserDMA&&) = delete;
+
+ //! \brief Initializes / resets the Hexagon User DMA engine
+ unsigned int Init();
+
+ //! \brief Calculates the number of DMAs in flight
+ uint32_t DMAsInFlight();
+
+ //! \brief Stores descriptors for all DMAs
+ std::vector<void*> dma_descriptors_;
Review Comment:
Possible issue - @csullivan informs me that the destructor may not get called for Hexagon runtime classes given that we kill the RPC to avoid zombie processes.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949037588
##########
tests/cpp-runtime/hexagon/hexagon_user_dma_tests.cc:
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include "../src/runtime/hexagon/hexagon_user_dma.h"
+
+using namespace tvm::runtime;
+using namespace tvm::runtime::hexagon;
+
+class HexagonUserDMATest : public ::testing::Test {
+ void SetUp() override {
+ src = malloc(length);
+ dst = malloc(length);
+ ASSERT_NE(src, nullptr);
+ ASSERT_NE(dst, nullptr);
+
+ src_char = static_cast<char*>(src);
+ dst_char = static_cast<char*>(dst);
+ for (uint32_t i = 0; i < length; ++i) {
+ src_char[i] = 1;
+ dst_char[i] = 0;
+ }
+ }
+ void TearDown() override {
+ free(src);
+ free(dst);
+ }
+
+ public:
+ int ret{0};
+ void* src{nullptr};
+ void* dst{nullptr};
+ char* src_char{nullptr};
+ char* dst_char{nullptr};
+ uint32_t length{0x400000}; // 4MB
+};
+
+TEST_F(HexagonUserDMATest, wait) {
+ HexagonUserDMA::Get().Wait(0);
+ HexagonUserDMA::Get().Wait(10);
+}
+
+TEST_F(HexagonUserDMATest, poll) { ASSERT_EQ(HexagonUserDMA::Get().Poll(), 0); }
+
+TEST_F(HexagonUserDMATest, bad_copy) {
+ uint64_t bigaddr = 0x100000000;
+ void* src64 = reinterpret_cast<void*>(bigaddr);
+ void* dst64 = reinterpret_cast<void*>(bigaddr);
+ uint32_t biglength = 0x1000000;
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst64, src, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src64, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src, biglength), DMA_SUCCESS);
+}
+
+TEST_F(HexagonUserDMATest, sync_dma) {
+ // kick off 1 DMA
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+
+ // wait for DMA to complete
+ HexagonUserDMA::Get().Wait(0);
+
+ // verify
+ for (uint32_t i = 0; i < length; ++i) {
+ ASSERT_EQ(src_char[i], dst_char[i]);
+ }
+}
+
+TEST_F(HexagonUserDMATest, async_dma) {
+ // kick off 10x duplicate DMAs
+ for (uint32_t i = 0; i < 10; ++i) {
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+ }
+
+ // verify at least 1 DMA in flight
+ // TODO: re-enable when CI runs on hardware - fails on simulator
+ // ASSERT_GT(HexagonUserDMA::Get().Poll(), 0);
Review Comment:
Yes, good thought. Let me see if I can take care of this `TODO` today.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r955489439
##########
tests/cpp-runtime/hexagon/ring_buffer_tests.cc:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
Review Comment:
Talked with @csullivan and his point was less about the `RingBuffer` class and more about the HexagonUserDMA engine and whether there would be any issue if we are overwriting descriptors (even if they are `done`) in a HexagonUserDMA chain. I.e. is it OK to overwrite the head of the descriptor chain once it is done? Perhaps @kparzysz-quic or @arangasa have some comment. A test could not hurt.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r950496671
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
+ }
- free(dma_desc);
+ // set descriptor as new tail
+ dma_descriptors_.push_back(dma_desc);
- if (status == DM0_STATUS_IDLE && done == DESC_DONE_COMPLETE) {
- return DMA_SUCCESS;
+ return DMA_SUCCESS;
+}
+
+void HexagonUserDMA::Wait(uint32_t max_dmas_in_flight) {
+ // wait (forever) until max DMAs in flight <= actual DMAs in flight
+ while (DMAsInFlight() > max_dmas_in_flight) {
+ }
+}
+
+uint32_t HexagonUserDMA::Poll() { return DMAsInFlight(); }
+
+uint32_t HexagonUserDMA::DMAsInFlight() {
+ // poll DMA engine to make sure DMA status is current
+ dmpoll();
+
+ // find the oldest DMA in flight
+ for (; oldest_dma_in_flight_ < dma_descriptors_.size(); ++oldest_dma_in_flight_) {
+ // read the `done` bit from the DMA descriptor and stop if incomplete
+ unsigned int done = dma_desc_get_done(dma_descriptors_[oldest_dma_in_flight_]);
+ if (done == DESC_DONE_INCOMPLETE) {
+ break;
+ }
+ }
+ // total DMAs in flight = total DMAs - oldest DMA in flight
+ return dma_descriptors_.size() - oldest_dma_in_flight_;
+}
+
+HexagonUserDMA::~HexagonUserDMA() {
+ Init(); // reset DMA engine
+ for (auto dma_desc : dma_descriptors_) {
+ free(dma_desc);
}
-#endif
- return DMA_FAILURE;
}
int hexagon_user_dma_1d_sync(void* dst, void* src, uint32_t length) {
Review Comment:
Marking this resolved as I am confident that deferring is the correct choice here. Happy to be corrected if I'm wrong and this is needed now.
--
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
[GitHub] [tvm] csullivan commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r948483578
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
+ }
- free(dma_desc);
+ // set descriptor as new tail
+ dma_descriptors_.push_back(dma_desc);
- if (status == DM0_STATUS_IDLE && done == DESC_DONE_COMPLETE) {
- return DMA_SUCCESS;
+ return DMA_SUCCESS;
+}
+
+void HexagonUserDMA::Wait(uint32_t max_dmas_in_flight) {
+ // wait (forever) until max DMAs in flight <= actual DMAs in flight
+ while (DMAsInFlight() > max_dmas_in_flight) {
Review Comment:
We could add a timeout after some large period (e.g. 10 seconds) that logs a warning. That said, I'm more inclined to leave the infinite spin to start as it will be more evident of an issue than a buried log message.
##########
tests/cpp-runtime/hexagon/hexagon_user_dma_tests.cc:
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include "../src/runtime/hexagon/hexagon_user_dma.h"
+
+using namespace tvm::runtime;
+using namespace tvm::runtime::hexagon;
+
+class HexagonUserDMATest : public ::testing::Test {
+ void SetUp() override {
+ src = malloc(length);
+ dst = malloc(length);
+ ASSERT_NE(src, nullptr);
+ ASSERT_NE(dst, nullptr);
+
+ src_char = static_cast<char*>(src);
+ dst_char = static_cast<char*>(dst);
+ for (uint32_t i = 0; i < length; ++i) {
+ src_char[i] = 1;
+ dst_char[i] = 0;
+ }
+ }
+ void TearDown() override {
+ free(src);
+ free(dst);
+ }
+
+ public:
+ int ret{0};
+ void* src{nullptr};
+ void* dst{nullptr};
+ char* src_char{nullptr};
+ char* dst_char{nullptr};
+ uint32_t length{0x400000}; // 4MB
+};
+
+TEST_F(HexagonUserDMATest, wait) {
+ HexagonUserDMA::Get().Wait(0);
+ HexagonUserDMA::Get().Wait(10);
+}
+
+TEST_F(HexagonUserDMATest, poll) { ASSERT_EQ(HexagonUserDMA::Get().Poll(), 0); }
+
+TEST_F(HexagonUserDMATest, bad_copy) {
+ uint64_t bigaddr = 0x100000000;
+ void* src64 = reinterpret_cast<void*>(bigaddr);
+ void* dst64 = reinterpret_cast<void*>(bigaddr);
+ uint32_t biglength = 0x1000000;
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst64, src, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src64, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src, biglength), DMA_SUCCESS);
+}
+
+TEST_F(HexagonUserDMATest, sync_dma) {
+ // kick off 1 DMA
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+
+ // wait for DMA to complete
+ HexagonUserDMA::Get().Wait(0);
+
+ // verify
+ for (uint32_t i = 0; i < length; ++i) {
+ ASSERT_EQ(src_char[i], dst_char[i]);
+ }
+}
+
+TEST_F(HexagonUserDMATest, async_dma) {
+ // kick off 10x duplicate DMAs
+ for (uint32_t i = 0; i < 10; ++i) {
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+ }
+
+ // verify at least 1 DMA in flight
+ // TODO: re-enable when CI runs on hardware - fails on simulator
+ // ASSERT_GT(HexagonUserDMA::Get().Poll(), 0);
+
+ // wait for at least 1 DMA to complete
+ HexagonUserDMA::Get().Wait(9);
+
+ // verify
+ for (uint32_t i = 0; i < length; ++i) {
+ ASSERT_EQ(src_char[i], dst_char[i]);
+ }
+}
+
+// TODO: Run non-pipelined case with sync DMA and execution time vs. pipelined case
+TEST_F(HexagonUserDMATest, pipeline) {
+ uint32_t pipeline_depth = 4;
+ uint32_t pipeline_length = length / pipeline_depth;
+
+ for (uint32_t i = 0; i < pipeline_depth; ++i) {
+ ret |= HexagonUserDMA::Get().Copy(dst_char + i * pipeline_length,
+ src_char + i * pipeline_length, pipeline_length);
+ }
+
+ HexagonUserDMA::Get().Wait(3);
+ for (uint32_t i = 0; i < pipeline_length; ++i) {
+ dst_char[i]++;
+ }
+
+ HexagonUserDMA::Get().Wait(2);
+ for (uint32_t i = pipeline_length; i < 2 * pipeline_length; ++i) {
+ dst_char[i]++;
+ }
+
+ HexagonUserDMA::Get().Wait(1);
+ for (uint32_t i = 2 * pipeline_length; i < 3 * pipeline_length; ++i) {
+ dst_char[i]++;
+ }
+
+ HexagonUserDMA::Get().Wait(0);
+ for (uint32_t i = 3 * pipeline_length; i < 4 * pipeline_length; ++i) {
+ dst_char[i]++;
+ }
+
+ // verify
+ ASSERT_EQ(ret, 0);
+ for (uint32_t i = 0; i < length; ++i) {
+ ASSERT_EQ(dst_char[i], 2);
+ }
+}
Review Comment:
Wonderful unit testing, as always! 👏
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
Review Comment:
Is there a limit to the number of descriptors we can link before requiring a new dmstart?
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
+ }
- free(dma_desc);
+ // set descriptor as new tail
+ dma_descriptors_.push_back(dma_desc);
- if (status == DM0_STATUS_IDLE && done == DESC_DONE_COMPLETE) {
- return DMA_SUCCESS;
+ return DMA_SUCCESS;
+}
+
+void HexagonUserDMA::Wait(uint32_t max_dmas_in_flight) {
+ // wait (forever) until max DMAs in flight <= actual DMAs in flight
+ while (DMAsInFlight() > max_dmas_in_flight) {
+ }
+}
+
+uint32_t HexagonUserDMA::Poll() { return DMAsInFlight(); }
+
+uint32_t HexagonUserDMA::DMAsInFlight() {
+ // poll DMA engine to make sure DMA status is current
+ dmpoll();
+
+ // find the oldest DMA in flight
+ for (; oldest_dma_in_flight_ < dma_descriptors_.size(); ++oldest_dma_in_flight_) {
+ // read the `done` bit from the DMA descriptor and stop if incomplete
+ unsigned int done = dma_desc_get_done(dma_descriptors_[oldest_dma_in_flight_]);
+ if (done == DESC_DONE_INCOMPLETE) {
+ break;
+ }
+ }
+ // total DMAs in flight = total DMAs - oldest DMA in flight
+ return dma_descriptors_.size() - oldest_dma_in_flight_;
+}
+
+HexagonUserDMA::~HexagonUserDMA() {
+ Init(); // reset DMA engine
+ for (auto dma_desc : dma_descriptors_) {
+ free(dma_desc);
}
-#endif
- return DMA_FAILURE;
}
int hexagon_user_dma_1d_sync(void* dst, void* src, uint32_t length) {
Review Comment:
Can we add `hexagon_user_dma_1d_async` and `hexagon_user_dma_1d_async_wait` equivalents now for use in tensorization?
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -28,55 +30,41 @@ namespace tvm {
namespace runtime {
namespace hexagon {
-int init_hexagon_user_dma() {
-#if __HEXAGON_ARCH__ >= 68
+unsigned int HexagonUserDMA::Init() {
// reset DMA engine
unsigned int status = dmpause() & DM0_STATUS_MASK;
- if (status != DM0_STATUS_IDLE) {
- return DMA_FAILURE;
- }
-#endif
- return DMA_SUCCESS;
+ return status;
}
-int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
-#if __HEXAGON_ARCH__ >= 68
- static int config_dma = init_hexagon_user_dma();
- if (config_dma != DMA_SUCCESS) {
+int HexagonUserDMA::Copy(void* dst, void* src, uint32_t length) {
+ // length limited to 24 bits
+ if (length > DESC_LENGTH_MASK) {
return DMA_FAILURE;
}
- uint64_t src64 = reinterpret_cast<uint64_t>(src);
// source address limited to 32 bits
- if (src64 > DESC_SRC_MASK) {
+ uint64_t src64 = reinterpret_cast<uint64_t>(src);
+ if (!src64 || src64 > DESC_SRC_MASK) {
return DMA_FAILURE;
}
- uint64_t dst64 = reinterpret_cast<uint64_t>(dst);
// destination address limited to 32 bits
- if (dst64 > DESC_DST_MASK) {
- return DMA_FAILURE;
- }
-
- // length limited to 24 bits
- if (length > DESC_LENGTH_MASK) {
+ uint64_t dst64 = reinterpret_cast<uint64_t>(dst);
+ if (!dst64 || dst64 > DESC_DST_MASK) {
return DMA_FAILURE;
}
- uint32_t src32 = src64 & DESC_SRC_MASK;
- uint32_t dst32 = dst64 & DESC_DST_MASK;
+ uint32_t src32 = static_cast<uint32_t>(src64);
+ uint32_t dst32 = static_cast<uint32_t>(dst64);
+ // allocate new descriptor
void* dma_desc = nullptr;
-
int ret = posix_memalign(&dma_desc, DMA_DESC_2D_SIZE, DMA_DESC_2D_SIZE);
- if (ret) {
Review Comment:
Potential backlog item could be to allocate the descriptors from a preallocated pool to avoid runtime allocations for each copy.
##########
tests/cpp-runtime/hexagon/hexagon_user_dma_tests.cc:
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include "../src/runtime/hexagon/hexagon_user_dma.h"
+
+using namespace tvm::runtime;
+using namespace tvm::runtime::hexagon;
+
+class HexagonUserDMATest : public ::testing::Test {
+ void SetUp() override {
+ src = malloc(length);
+ dst = malloc(length);
+ ASSERT_NE(src, nullptr);
+ ASSERT_NE(dst, nullptr);
+
+ src_char = static_cast<char*>(src);
+ dst_char = static_cast<char*>(dst);
+ for (uint32_t i = 0; i < length; ++i) {
+ src_char[i] = 1;
+ dst_char[i] = 0;
+ }
+ }
+ void TearDown() override {
+ free(src);
+ free(dst);
+ }
+
+ public:
+ int ret{0};
+ void* src{nullptr};
+ void* dst{nullptr};
+ char* src_char{nullptr};
+ char* dst_char{nullptr};
+ uint32_t length{0x400000}; // 4MB
+};
+
+TEST_F(HexagonUserDMATest, wait) {
+ HexagonUserDMA::Get().Wait(0);
+ HexagonUserDMA::Get().Wait(10);
+}
+
+TEST_F(HexagonUserDMATest, poll) { ASSERT_EQ(HexagonUserDMA::Get().Poll(), 0); }
+
+TEST_F(HexagonUserDMATest, bad_copy) {
+ uint64_t bigaddr = 0x100000000;
+ void* src64 = reinterpret_cast<void*>(bigaddr);
+ void* dst64 = reinterpret_cast<void*>(bigaddr);
+ uint32_t biglength = 0x1000000;
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst64, src, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src64, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src, biglength), DMA_SUCCESS);
+}
+
+TEST_F(HexagonUserDMATest, sync_dma) {
+ // kick off 1 DMA
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+
+ // wait for DMA to complete
+ HexagonUserDMA::Get().Wait(0);
+
+ // verify
+ for (uint32_t i = 0; i < length; ++i) {
+ ASSERT_EQ(src_char[i], dst_char[i]);
+ }
+}
+
+TEST_F(HexagonUserDMATest, async_dma) {
+ // kick off 10x duplicate DMAs
+ for (uint32_t i = 0; i < 10; ++i) {
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+ }
+
+ // verify at least 1 DMA in flight
+ // TODO: re-enable when CI runs on hardware - fails on simulator
+ // ASSERT_GT(HexagonUserDMA::Get().Poll(), 0);
Review Comment:
Should we re-enable this so it runs on hardware in nightly but skip when running on the simulator? e.g.
```
auto sn = std::getenv("ANDROID_SERIAL_NUMBER");
if (sn && sn == "simulator") { GTEST_SKIP(); }
```
--
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
[GitHub] [tvm] csullivan merged pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
csullivan merged PR #12411:
URL: https://github.com/apache/tvm/pull/12411
--
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
[GitHub] [tvm] adstraw commented on pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on PR #12411:
URL: https://github.com/apache/tvm/pull/12411#issuecomment-1218522712
@kparzysz-quic and @arangasa I put the Hexagon arch check around the calls to the 1D sync DMA flow in the Hexagon Device API and also `HexagonBuffer` class. If we are on a device that does not support DMA we will simply `memcpy` instead. This keeps the arch check out of `HexagonUserDMA` class which I believe is better. I believe this PR is ready for review.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949031702
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
+ }
- free(dma_desc);
+ // set descriptor as new tail
+ dma_descriptors_.push_back(dma_desc);
- if (status == DM0_STATUS_IDLE && done == DESC_DONE_COMPLETE) {
- return DMA_SUCCESS;
+ return DMA_SUCCESS;
+}
+
+void HexagonUserDMA::Wait(uint32_t max_dmas_in_flight) {
+ // wait (forever) until max DMAs in flight <= actual DMAs in flight
+ while (DMAsInFlight() > max_dmas_in_flight) {
Review Comment:
I agree completely. Had the same thought about a buried log message.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r955488224
##########
tests/cpp-runtime/hexagon/ring_buffer_tests.cc:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
Review Comment:
> By the way, it's interesting you were thinking about corner cases because I woke up thinking about the Next vs. Add API as described above. And, I also realized there is a bug in the InFlight method where we might increment the "oldest" ID beyond the "next" ID. Will add a test case that can hit that bug and fix it in the next commit.
This is now fixed.
> There is one issue with this flow that I thought of ... the DMA descriptor at ring buffer slot 0 is "finished" and we simply return a pointer to it and expect that the user will add an "inflight" DMA descriptor at that location. If the user were to call InFlight before adding the "inflight" descriptor then ring buffer slot 0 would be treated as "finished" from the perspective of the RingBuffer class where it is still "inflight" from the perspective of the user.
I decided to leave this as-is and encoded this corner case in a test called `wrap_corner`.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r955507870
##########
tests/cpp-runtime/hexagon/ring_buffer_tests.cc:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
Review Comment:
Added a unit test which overflows the HexagonUserDMA ring buffer for DMA descriptors. The test is named `overflow_ring_buffer`. Also added a return of `DMA_RETRY` from the HexagonUserDMA `Copy` function in case the ring buffer is full. This passes the onus to the user of the class to retry the `Copy`. The other way to handle it is for `Copy` to `Wait` until there is a free slot in the ring buffer to use. Not sure what is best at this moment. I am hoping the unit test provides the desired coverage. Would like to add integration tests in a separate test.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r955269061
##########
tests/cpp-runtime/hexagon/ring_buffer_tests.cc:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
Review Comment:
By the way, it's interesting you were thinking about corner cases because I woke up thinking about the `Next` vs. `Add` API as described above. And, I also realized there is a bug in the `InFlight` method where we might increment the "oldest" ID beyond the "next" ID. Will add a test case that can hit that bug and fix it in the next commit.
--
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
[GitHub] [tvm] kparzysz-quic commented on pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on PR #12411:
URL: https://github.com/apache/tvm/pull/12411#issuecomment-1218076304
The guards are necessary: earlier architectures don't have DMA, and if a program attempts to execute a DMA instruction, it will trigger a hardware exception.
For earlier architectures we could just do a synchronous memcpy.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949707780
##########
tests/cpp-runtime/hexagon/hexagon_user_dma_tests.cc:
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include "../src/runtime/hexagon/hexagon_user_dma.h"
+
+using namespace tvm::runtime;
+using namespace tvm::runtime::hexagon;
+
+class HexagonUserDMATest : public ::testing::Test {
+ void SetUp() override {
+ src = malloc(length);
+ dst = malloc(length);
+ ASSERT_NE(src, nullptr);
+ ASSERT_NE(dst, nullptr);
+
+ src_char = static_cast<char*>(src);
+ dst_char = static_cast<char*>(dst);
+ for (uint32_t i = 0; i < length; ++i) {
+ src_char[i] = 1;
+ dst_char[i] = 0;
+ }
+ }
+ void TearDown() override {
+ free(src);
+ free(dst);
+ }
+
+ public:
+ int ret{0};
+ void* src{nullptr};
+ void* dst{nullptr};
+ char* src_char{nullptr};
+ char* dst_char{nullptr};
+ uint32_t length{0x400000}; // 4MB
+};
+
+TEST_F(HexagonUserDMATest, wait) {
+ HexagonUserDMA::Get().Wait(0);
+ HexagonUserDMA::Get().Wait(10);
+}
+
+TEST_F(HexagonUserDMATest, poll) { ASSERT_EQ(HexagonUserDMA::Get().Poll(), 0); }
+
+TEST_F(HexagonUserDMATest, bad_copy) {
+ uint64_t bigaddr = 0x100000000;
+ void* src64 = reinterpret_cast<void*>(bigaddr);
+ void* dst64 = reinterpret_cast<void*>(bigaddr);
+ uint32_t biglength = 0x1000000;
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst64, src, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src64, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src, biglength), DMA_SUCCESS);
+}
+
+TEST_F(HexagonUserDMATest, sync_dma) {
+ // kick off 1 DMA
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+
+ // wait for DMA to complete
+ HexagonUserDMA::Get().Wait(0);
+
+ // verify
+ for (uint32_t i = 0; i < length; ++i) {
+ ASSERT_EQ(src_char[i], dst_char[i]);
+ }
+}
+
+TEST_F(HexagonUserDMATest, async_dma) {
+ // kick off 10x duplicate DMAs
+ for (uint32_t i = 0; i < 10; ++i) {
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+ }
+
+ // verify at least 1 DMA in flight
+ // TODO: re-enable when CI runs on hardware - fails on simulator
+ // ASSERT_GT(HexagonUserDMA::Get().Poll(), 0);
Review Comment:
The more I think about it, this check for an "in flight" count greater than 0 does not really add any value. On the simulator, the "in flight" count is always 0 as if DMAs complete automatically so the test fails. On hardware, the "in flight" count should be non-zero but we have other ways to validate the polling mechanism is working correctly. I added `async_dma_poll` test case which has the correct coverage, I think. Let me know what you think @csullivan.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949031227
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
+ }
- free(dma_desc);
+ // set descriptor as new tail
+ dma_descriptors_.push_back(dma_desc);
- if (status == DM0_STATUS_IDLE && done == DESC_DONE_COMPLETE) {
- return DMA_SUCCESS;
+ return DMA_SUCCESS;
+}
+
+void HexagonUserDMA::Wait(uint32_t max_dmas_in_flight) {
+ // wait (forever) until max DMAs in flight <= actual DMAs in flight
+ while (DMAsInFlight() > max_dmas_in_flight) {
+ }
+}
+
+uint32_t HexagonUserDMA::Poll() { return DMAsInFlight(); }
+
+uint32_t HexagonUserDMA::DMAsInFlight() {
+ // poll DMA engine to make sure DMA status is current
+ dmpoll();
+
+ // find the oldest DMA in flight
+ for (; oldest_dma_in_flight_ < dma_descriptors_.size(); ++oldest_dma_in_flight_) {
+ // read the `done` bit from the DMA descriptor and stop if incomplete
+ unsigned int done = dma_desc_get_done(dma_descriptors_[oldest_dma_in_flight_]);
+ if (done == DESC_DONE_INCOMPLETE) {
+ break;
+ }
+ }
+ // total DMAs in flight = total DMAs - oldest DMA in flight
+ return dma_descriptors_.size() - oldest_dma_in_flight_;
+}
+
+HexagonUserDMA::~HexagonUserDMA() {
+ Init(); // reset DMA engine
+ for (auto dma_desc : dma_descriptors_) {
+ free(dma_desc);
}
-#endif
- return DMA_FAILURE;
}
int hexagon_user_dma_1d_sync(void* dst, void* src, uint32_t length) {
Review Comment:
My thought was to do this in the next phase of development when we work on TIR lowering to Async DMA. What do you think @csullivan?
--
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
[GitHub] [tvm] csullivan commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r952001668
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -82,12 +95,13 @@ int HexagonUserDMA::Copy(void* dst, void* src, uint32_t length) {
dmstart(dma_desc);
first_dma_ = false;
} else {
- // `dmlink` descriptor to tail
- dmlink(dma_descriptors_.back(), dma_desc);
+ // `dmlink` descriptor to tail descriptor
+ void* tail = GetDescriptorAddr(id_next_dma_desc_ - 1);
+ dmlink(tail, dma_desc);
}
- // set descriptor as new tail
- dma_descriptors_.push_back(dma_desc);
+ // update the ID of the next DMA descriptor
+ id_next_dma_desc_++;
Review Comment:
Interaction with the ring buffer here feels a bit like it should be encapsulated into methods like "AllocDescriptor" "FreeDescriptor" or something like that which manages the internal member `id_next_dma_desc_` and others.
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -82,12 +95,13 @@ int HexagonUserDMA::Copy(void* dst, void* src, uint32_t length) {
dmstart(dma_desc);
first_dma_ = false;
} else {
- // `dmlink` descriptor to tail
- dmlink(dma_descriptors_.back(), dma_desc);
+ // `dmlink` descriptor to tail descriptor
+ void* tail = GetDescriptorAddr(id_next_dma_desc_ - 1);
+ dmlink(tail, dma_desc);
}
- // set descriptor as new tail
- dma_descriptors_.push_back(dma_desc);
+ // update the ID of the next DMA descriptor
+ id_next_dma_desc_++;
Review Comment:
Interaction with the ring buffer here feels a bit like it should be encapsulated into a class with methods like "AllocDescriptor" "FreeDescriptor" or something like that which manages the internal member `id_next_dma_desc_` and others.
--
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
[GitHub] [tvm] arangasa commented on pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
arangasa commented on PR #12411:
URL: https://github.com/apache/tvm/pull/12411#issuecomment-1218034594
> Attention @arangasa, please review. Are the `#if __HEXAGON_ARCH__ >= 68` guards removed with this PR necessary? How do we want the HexagonUserDMA class to behave for earlier architectures?
Hi Krzysztof @kparzysz-quic , Could you please suggest Adam @adstraw ? Thank you.
--
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
[GitHub] [tvm] kparzysz-quic commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949146569
##########
src/runtime/hexagon/hexagon_user_dma_instructions.h:
##########
@@ -24,8 +24,6 @@ namespace tvm {
namespace runtime {
namespace hexagon {
-#if __HEXAGON_ARCH__ >= 68
Review Comment:
Just a comment---
If someone tries to compile this code for a pre-v68 architecture, the inline assembly will cause an error. I guess we can require TVM runtime for Hexagon to be compiled for v68 and above, so these guards can remain removed.
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <vector>
Review Comment:
Please move this below the guard.
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <vector>
+
+#ifndef TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+#define TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+
+namespace tvm {
+namespace runtime {
+namespace hexagon {
+
+#define DMA_SUCCESS 0
+#define DMA_FAILURE -1
+
+class HexagonUserDMA {
+ public:
+ /*!
+ * \brief Initiate DMA to copy memory from source to destination address
+ * \param dst Destination address
+ * \param src Source address
+ * \param length Length in bytes to copy
+ * \returns Status, either DMA_SUCCESS or DMA_FAILURE
+ */
+ int Copy(void* dst, void* src, uint32_t length);
+
+ /*!
+ * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \param max_dmas_in_flight Maximum number of DMAs allowed to be in flight
+ * to satisfy the `Wait` e.g. use `Wait(0)` to wait on "all" outstanding DMAs to complete
+ */
+ void Wait(uint32_t max_dmas_in_flight);
+
+ /*!
+ * \brief Poll the number of DMAs in flight
+ * \returns Number of DMAs in flight
+ */
+ uint32_t Poll();
+
+ //! HexagonUserDMA uses the singleton pattern
+ static HexagonUserDMA& Get() {
+ static HexagonUserDMA* hud = new HexagonUserDMA();
+ return *hud;
+ }
+
+ private:
+ HexagonUserDMA() = default;
+ ~HexagonUserDMA();
+ HexagonUserDMA(const HexagonUserDMA&) = delete;
+ HexagonUserDMA& operator=(const HexagonUserDMA&) = delete;
+ HexagonUserDMA(HexagonUserDMA&&) = delete;
+ HexagonUserDMA& operator=(HexagonUserDMA&&) = delete;
+
+ //! \brief Initializes / resets the Hexagon User DMA engine
+ unsigned int Init();
+
+ //! \brief Calculates the number of DMAs in flight
+ uint32_t DMAsInFlight();
+
+ //! \brief Stores descriptors for all DMAs
+ std::vector<void*> dma_descriptors_;
Review Comment:
Are the descriptors ever deallocated/removed from this list? If not, it can cause a memory leak.
--
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
[GitHub] [tvm] kparzysz-quic commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r950230162
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <vector>
+
+#ifndef TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+#define TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+
+namespace tvm {
+namespace runtime {
+namespace hexagon {
+
+#define DMA_SUCCESS 0
+#define DMA_FAILURE -1
+
+class HexagonUserDMA {
+ public:
+ /*!
+ * \brief Initiate DMA to copy memory from source to destination address
+ * \param dst Destination address
+ * \param src Source address
+ * \param length Length in bytes to copy
+ * \returns Status, either DMA_SUCCESS or DMA_FAILURE
+ */
+ int Copy(void* dst, void* src, uint32_t length);
+
+ /*!
+ * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \param max_dmas_in_flight Maximum number of DMAs allowed to be in flight
+ * to satisfy the `Wait` e.g. use `Wait(0)` to wait on "all" outstanding DMAs to complete
+ */
+ void Wait(uint32_t max_dmas_in_flight);
+
+ /*!
+ * \brief Poll the number of DMAs in flight
+ * \returns Number of DMAs in flight
+ */
+ uint32_t Poll();
+
+ //! HexagonUserDMA uses the singleton pattern
+ static HexagonUserDMA& Get() {
+ static HexagonUserDMA* hud = new HexagonUserDMA();
+ return *hud;
+ }
+
+ private:
+ HexagonUserDMA() = default;
+ ~HexagonUserDMA();
+ HexagonUserDMA(const HexagonUserDMA&) = delete;
+ HexagonUserDMA& operator=(const HexagonUserDMA&) = delete;
+ HexagonUserDMA(HexagonUserDMA&&) = delete;
+ HexagonUserDMA& operator=(HexagonUserDMA&&) = delete;
+
+ //! \brief Initializes / resets the Hexagon User DMA engine
+ unsigned int Init();
+
+ //! \brief Calculates the number of DMAs in flight
+ uint32_t DMAsInFlight();
+
+ //! \brief Stores descriptors for all DMAs
+ std::vector<void*> dma_descriptors_;
Review Comment:
My concern is that there is only one object of this class, so when the program runs long enough the descriptor allocations will add up.
--
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
[GitHub] [tvm] csullivan commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r956442755
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#ifndef TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+#define TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+
+#include "hexagon_common.h"
+#include "hexagon_user_dma_descriptors.h"
+#include "hexagon_user_dma_instructions.h"
+#include "hexagon_user_dma_registers.h"
+#include "ring_buffer.h"
+
+namespace tvm {
+namespace runtime {
+namespace hexagon {
+
+#define DMA_SUCCESS 0
+#define DMA_FAILURE -1
+#define DMA_RETRY 1
Review Comment:
nit: A docstring on the meaning of DMA_RETRY would be helpful for quicker understanding.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r955267553
##########
tests/cpp-runtime/hexagon/ring_buffer_tests.cc:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
Review Comment:
I would change your example slightly to make the descriptor IDs zero based. This is how it works in the code. The first descriptor has ID=0 and uses ring buffer slot 0. The second has ID=1 and uses slot 1. When ID is equivalent to ring buffer size we reuse ring buffer slot 0. And so on.
```
Ringbuffer[size=4]
Initial state:
desc0@0[finished] , desc1@1[finished] , desc2@2[finished] , desc3@3[finished]
New DMA needed:
desc4@0[inflight] , desc1@1[finished] , desc2@2[finished] , desc3@3[finished]
```
The case you have described above should work OK. I believe we can cover it with `RingBuffer` unit tests. I believe existing tests already hit this case, to some degree. I will add a test case to explicitly cover this scenario.
When we go to add `desc4` we first check if the ring buffer is full by checking that `InFlight` count (0) is less than the size of the ring buffer (4). All good. Then, we `GetAddr` of the "next" ID = 4 which reuses ring buffer slot 0. Then, we increment the "next" ID to 5. Then we return a pointer to ring buffer slot 0.
There is one issue with this flow that I thought of ... the DMA descriptor at ring buffer slot 0 is "finished" and we simply return a pointer to it and expect that the user will add an "inflight" DMA descriptor at that location. If the user were to call `InFlight` before adding the "inflight" descriptor then ring buffer slot 0 would be treated as "finished" from the perspective of the `RingBuffer` class where it is still "inflight" from the perspective of the user.
I had initially coded an `Add` function which took a function pointer which would add a T to the RingBuffer rather that `Next` to get around this issue. It felt to complicated but it does expose a gap.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r954407081
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -82,12 +95,13 @@ int HexagonUserDMA::Copy(void* dst, void* src, uint32_t length) {
dmstart(dma_desc);
first_dma_ = false;
} else {
- // `dmlink` descriptor to tail
- dmlink(dma_descriptors_.back(), dma_desc);
+ // `dmlink` descriptor to tail descriptor
+ void* tail = GetDescriptorAddr(id_next_dma_desc_ - 1);
+ dmlink(tail, dma_desc);
}
- // set descriptor as new tail
- dma_descriptors_.push_back(dma_desc);
+ // update the ID of the next DMA descriptor
+ id_next_dma_desc_++;
Review Comment:
Added new RingBuffer class along with testing.
--
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
[GitHub] [tvm] kparzysz-quic commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
kparzysz-quic commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949140383
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
Review Comment:
There is not limit that I'm aware of.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949484536
##########
src/runtime/hexagon/hexagon_user_dma_instructions.h:
##########
@@ -24,8 +24,6 @@ namespace tvm {
namespace runtime {
namespace hexagon {
-#if __HEXAGON_ARCH__ >= 68
Review Comment:
OK, I will revert the checks for `#if __HEXAGON_ARCH__ >= 68` assuming it's a workable plan to require the TVM runtime to be compiled for v68 and above.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r950333067
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <vector>
+
+#ifndef TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+#define TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+
+namespace tvm {
+namespace runtime {
+namespace hexagon {
+
+#define DMA_SUCCESS 0
+#define DMA_FAILURE -1
+
+class HexagonUserDMA {
+ public:
+ /*!
+ * \brief Initiate DMA to copy memory from source to destination address
+ * \param dst Destination address
+ * \param src Source address
+ * \param length Length in bytes to copy
+ * \returns Status, either DMA_SUCCESS or DMA_FAILURE
+ */
+ int Copy(void* dst, void* src, uint32_t length);
+
+ /*!
+ * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \param max_dmas_in_flight Maximum number of DMAs allowed to be in flight
+ * to satisfy the `Wait` e.g. use `Wait(0)` to wait on "all" outstanding DMAs to complete
+ */
+ void Wait(uint32_t max_dmas_in_flight);
+
+ /*!
+ * \brief Poll the number of DMAs in flight
+ * \returns Number of DMAs in flight
+ */
+ uint32_t Poll();
+
+ //! HexagonUserDMA uses the singleton pattern
+ static HexagonUserDMA& Get() {
+ static HexagonUserDMA* hud = new HexagonUserDMA();
+ return *hud;
+ }
+
+ private:
+ HexagonUserDMA() = default;
+ ~HexagonUserDMA();
+ HexagonUserDMA(const HexagonUserDMA&) = delete;
+ HexagonUserDMA& operator=(const HexagonUserDMA&) = delete;
+ HexagonUserDMA(HexagonUserDMA&&) = delete;
+ HexagonUserDMA& operator=(HexagonUserDMA&&) = delete;
+
+ //! \brief Initializes / resets the Hexagon User DMA engine
+ unsigned int Init();
+
+ //! \brief Calculates the number of DMAs in flight
+ uint32_t DMAsInFlight();
+
+ //! \brief Stores descriptors for all DMAs
+ std::vector<void*> dma_descriptors_;
Review Comment:
Maybe a good middle ground would be to `free` descriptors once we discover they are marked `done` in the `DMAsInFlight` method.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r951975770
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <vector>
+
+#ifndef TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+#define TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+
+namespace tvm {
+namespace runtime {
+namespace hexagon {
+
+#define DMA_SUCCESS 0
+#define DMA_FAILURE -1
+
+class HexagonUserDMA {
+ public:
+ /*!
+ * \brief Initiate DMA to copy memory from source to destination address
+ * \param dst Destination address
+ * \param src Source address
+ * \param length Length in bytes to copy
+ * \returns Status, either DMA_SUCCESS or DMA_FAILURE
+ */
+ int Copy(void* dst, void* src, uint32_t length);
+
+ /*!
+ * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \param max_dmas_in_flight Maximum number of DMAs allowed to be in flight
+ * to satisfy the `Wait` e.g. use `Wait(0)` to wait on "all" outstanding DMAs to complete
+ */
+ void Wait(uint32_t max_dmas_in_flight);
+
+ /*!
+ * \brief Poll the number of DMAs in flight
+ * \returns Number of DMAs in flight
+ */
+ uint32_t Poll();
+
+ //! HexagonUserDMA uses the singleton pattern
+ static HexagonUserDMA& Get() {
+ static HexagonUserDMA* hud = new HexagonUserDMA();
+ return *hud;
+ }
+
+ private:
+ HexagonUserDMA() = default;
+ ~HexagonUserDMA();
+ HexagonUserDMA(const HexagonUserDMA&) = delete;
+ HexagonUserDMA& operator=(const HexagonUserDMA&) = delete;
+ HexagonUserDMA(HexagonUserDMA&&) = delete;
+ HexagonUserDMA& operator=(HexagonUserDMA&&) = delete;
+
+ //! \brief Initializes / resets the Hexagon User DMA engine
+ unsigned int Init();
+
+ //! \brief Calculates the number of DMAs in flight
+ uint32_t DMAsInFlight();
+
+ //! \brief Stores descriptors for all DMAs
+ std::vector<void*> dma_descriptors_;
Review Comment:
Marking this resolved with the latest commit which will `use ring buffer to store DMA descriptors`
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r952910319
##########
tests/cpp-runtime/hexagon/hexagon_user_dma_tests.cc:
##########
@@ -0,0 +1,141 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include "../src/runtime/hexagon/hexagon_user_dma.h"
+
+using namespace tvm::runtime;
+using namespace tvm::runtime::hexagon;
+
+class HexagonUserDMATest : public ::testing::Test {
+ void SetUp() override {
+ src = malloc(length);
+ dst = malloc(length);
+ ASSERT_NE(src, nullptr);
+ ASSERT_NE(dst, nullptr);
+
+ src_char = static_cast<char*>(src);
+ dst_char = static_cast<char*>(dst);
+ for (uint32_t i = 0; i < length; ++i) {
+ src_char[i] = 1;
+ dst_char[i] = 0;
+ }
+ }
+ void TearDown() override {
+ free(src);
+ free(dst);
+ }
+
+ public:
+ int ret{0};
+ void* src{nullptr};
+ void* dst{nullptr};
+ char* src_char{nullptr};
+ char* dst_char{nullptr};
+ uint32_t length{0x400000}; // 4MB
+};
+
+TEST_F(HexagonUserDMATest, wait) {
+ HexagonUserDMA::Get().Wait(0);
+ HexagonUserDMA::Get().Wait(10);
+}
+
+TEST_F(HexagonUserDMATest, poll) { ASSERT_EQ(HexagonUserDMA::Get().Poll(), 0); }
+
+TEST_F(HexagonUserDMATest, bad_copy) {
+ uint64_t bigaddr = 0x100000000;
+ void* src64 = reinterpret_cast<void*>(bigaddr);
+ void* dst64 = reinterpret_cast<void*>(bigaddr);
+ uint32_t biglength = 0x1000000;
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst64, src, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src64, length), DMA_SUCCESS);
+ ASSERT_NE(HexagonUserDMA::Get().Copy(dst, src, biglength), DMA_SUCCESS);
+}
+
+TEST_F(HexagonUserDMATest, sync_dma) {
+ // kick off 1 DMA
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+
+ // wait for DMA to complete
+ HexagonUserDMA::Get().Wait(0);
+
+ // verify
+ for (uint32_t i = 0; i < length; ++i) {
+ ASSERT_EQ(src_char[i], dst_char[i]);
+ }
+}
+
+TEST_F(HexagonUserDMATest, async_dma) {
+ // kick off 10x duplicate DMAs
+ for (uint32_t i = 0; i < 10; ++i) {
+ ret = HexagonUserDMA::Get().Copy(dst, src, length);
+ ASSERT_EQ(ret, DMA_SUCCESS);
+ }
+
+ // verify at least 1 DMA in flight
+ // TODO: re-enable when CI runs on hardware - fails on simulator
+ // ASSERT_GT(HexagonUserDMA::Get().Poll(), 0);
Review Comment:
Added a task to the backlog to figure out nightly testing of async behavior on hardware.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949483132
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <vector>
Review Comment:
Good catch, done.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949709014
##########
src/runtime/hexagon/hexagon_user_dma_instructions.h:
##########
@@ -24,8 +24,6 @@ namespace tvm {
namespace runtime {
namespace hexagon {
-#if __HEXAGON_ARCH__ >= 68
Review Comment:
Removed.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949031227
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
+ }
- free(dma_desc);
+ // set descriptor as new tail
+ dma_descriptors_.push_back(dma_desc);
- if (status == DM0_STATUS_IDLE && done == DESC_DONE_COMPLETE) {
- return DMA_SUCCESS;
+ return DMA_SUCCESS;
+}
+
+void HexagonUserDMA::Wait(uint32_t max_dmas_in_flight) {
+ // wait (forever) until max DMAs in flight <= actual DMAs in flight
+ while (DMAsInFlight() > max_dmas_in_flight) {
+ }
+}
+
+uint32_t HexagonUserDMA::Poll() { return DMAsInFlight(); }
+
+uint32_t HexagonUserDMA::DMAsInFlight() {
+ // poll DMA engine to make sure DMA status is current
+ dmpoll();
+
+ // find the oldest DMA in flight
+ for (; oldest_dma_in_flight_ < dma_descriptors_.size(); ++oldest_dma_in_flight_) {
+ // read the `done` bit from the DMA descriptor and stop if incomplete
+ unsigned int done = dma_desc_get_done(dma_descriptors_[oldest_dma_in_flight_]);
+ if (done == DESC_DONE_INCOMPLETE) {
+ break;
+ }
+ }
+ // total DMAs in flight = total DMAs - oldest DMA in flight
+ return dma_descriptors_.size() - oldest_dma_in_flight_;
+}
+
+HexagonUserDMA::~HexagonUserDMA() {
+ Init(); // reset DMA engine
+ for (auto dma_desc : dma_descriptors_) {
+ free(dma_desc);
}
-#endif
- return DMA_FAILURE;
}
int hexagon_user_dma_1d_sync(void* dst, void* src, uint32_t length) {
Review Comment:
My thought was to do this in the next phase of development when we work on TIR lowering to Async DMA.
--
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
[GitHub] [tvm] aryamazaheri commented on pull request #12411: [Hexagon] Asynchronous DMA support
Posted by "aryamazaheri (via GitHub)" <gi...@apache.org>.
aryamazaheri commented on PR #12411:
URL: https://github.com/apache/tvm/pull/12411#issuecomment-1594906085
@adstraw @kparzysz-quic It seems that this PR removed the guard `#if __HEXAGON_ARCH__ >= 68`. Then, has TVM officially discontinued support for v66 DSPs?
--
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
[GitHub] [tvm] adstraw commented on pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on PR #12411:
URL: https://github.com/apache/tvm/pull/12411#issuecomment-1217942268
Attention @arangasa, please review. Are the `#if __HEXAGON_ARCH__ >= 68` guards removed with this PR necessary? How do we want the HexagonUserDMA class to behave for earlier architectures?
--
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
[GitHub] [tvm] csullivan commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
csullivan commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r955115733
##########
tests/cpp-runtime/hexagon/ring_buffer_tests.cc:
##########
@@ -0,0 +1,109 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
Review Comment:
It occurred to me that if we maintain only a single command queue / descriptor chain over the lifetime of a program, that eventually a DMA descriptor that was complete, may be "resurected" by the ring buffer as a new descriptor. E.g.
```
Ringbuffer[size=4]
Initial state:
desc1@0[finished] , desc2@1[finished] , desc3@2[finished] , desc4@3[finished]
New DMA needed:
desc5@0[inflight] , desc2@1[finished] , desc3@2[finished] , desc4@3[finished] , desc5@0[inflight]
```
With the last descriptor occupying the same memory space as the first.
Do you think this will be problematic? An integration tests with the UserDMA class to test this could reassure us.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r950496671
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
+ }
- free(dma_desc);
+ // set descriptor as new tail
+ dma_descriptors_.push_back(dma_desc);
- if (status == DM0_STATUS_IDLE && done == DESC_DONE_COMPLETE) {
- return DMA_SUCCESS;
+ return DMA_SUCCESS;
+}
+
+void HexagonUserDMA::Wait(uint32_t max_dmas_in_flight) {
+ // wait (forever) until max DMAs in flight <= actual DMAs in flight
+ while (DMAsInFlight() > max_dmas_in_flight) {
+ }
+}
+
+uint32_t HexagonUserDMA::Poll() { return DMAsInFlight(); }
+
+uint32_t HexagonUserDMA::DMAsInFlight() {
+ // poll DMA engine to make sure DMA status is current
+ dmpoll();
+
+ // find the oldest DMA in flight
+ for (; oldest_dma_in_flight_ < dma_descriptors_.size(); ++oldest_dma_in_flight_) {
+ // read the `done` bit from the DMA descriptor and stop if incomplete
+ unsigned int done = dma_desc_get_done(dma_descriptors_[oldest_dma_in_flight_]);
+ if (done == DESC_DONE_INCOMPLETE) {
+ break;
+ }
+ }
+ // total DMAs in flight = total DMAs - oldest DMA in flight
+ return dma_descriptors_.size() - oldest_dma_in_flight_;
+}
+
+HexagonUserDMA::~HexagonUserDMA() {
+ Init(); // reset DMA engine
+ for (auto dma_desc : dma_descriptors_) {
+ free(dma_desc);
}
-#endif
- return DMA_FAILURE;
}
int hexagon_user_dma_1d_sync(void* dst, void* src, uint32_t length) {
Review Comment:
Marking this resolved as I am confident that deferring is the correct choice here.
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949031227
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
+ }
- free(dma_desc);
+ // set descriptor as new tail
+ dma_descriptors_.push_back(dma_desc);
- if (status == DM0_STATUS_IDLE && done == DESC_DONE_COMPLETE) {
- return DMA_SUCCESS;
+ return DMA_SUCCESS;
+}
+
+void HexagonUserDMA::Wait(uint32_t max_dmas_in_flight) {
+ // wait (forever) until max DMAs in flight <= actual DMAs in flight
+ while (DMAsInFlight() > max_dmas_in_flight) {
+ }
+}
+
+uint32_t HexagonUserDMA::Poll() { return DMAsInFlight(); }
+
+uint32_t HexagonUserDMA::DMAsInFlight() {
+ // poll DMA engine to make sure DMA status is current
+ dmpoll();
+
+ // find the oldest DMA in flight
+ for (; oldest_dma_in_flight_ < dma_descriptors_.size(); ++oldest_dma_in_flight_) {
+ // read the `done` bit from the DMA descriptor and stop if incomplete
+ unsigned int done = dma_desc_get_done(dma_descriptors_[oldest_dma_in_flight_]);
+ if (done == DESC_DONE_INCOMPLETE) {
+ break;
+ }
+ }
+ // total DMAs in flight = total DMAs - oldest DMA in flight
+ return dma_descriptors_.size() - oldest_dma_in_flight_;
+}
+
+HexagonUserDMA::~HexagonUserDMA() {
+ Init(); // reset DMA engine
+ for (auto dma_desc : dma_descriptors_) {
+ free(dma_desc);
}
-#endif
- return DMA_FAILURE;
}
int hexagon_user_dma_1d_sync(void* dst, void* src, uint32_t length) {
Review Comment:
My thought was to do this in the next phase of development when we work on TIR lowering to Async DMA. What do you think?
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949035189
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -28,55 +30,41 @@ namespace tvm {
namespace runtime {
namespace hexagon {
-int init_hexagon_user_dma() {
-#if __HEXAGON_ARCH__ >= 68
+unsigned int HexagonUserDMA::Init() {
// reset DMA engine
unsigned int status = dmpause() & DM0_STATUS_MASK;
- if (status != DM0_STATUS_IDLE) {
- return DMA_FAILURE;
- }
-#endif
- return DMA_SUCCESS;
+ return status;
}
-int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
-#if __HEXAGON_ARCH__ >= 68
- static int config_dma = init_hexagon_user_dma();
- if (config_dma != DMA_SUCCESS) {
+int HexagonUserDMA::Copy(void* dst, void* src, uint32_t length) {
+ // length limited to 24 bits
+ if (length > DESC_LENGTH_MASK) {
return DMA_FAILURE;
}
- uint64_t src64 = reinterpret_cast<uint64_t>(src);
// source address limited to 32 bits
- if (src64 > DESC_SRC_MASK) {
+ uint64_t src64 = reinterpret_cast<uint64_t>(src);
+ if (!src64 || src64 > DESC_SRC_MASK) {
return DMA_FAILURE;
}
- uint64_t dst64 = reinterpret_cast<uint64_t>(dst);
// destination address limited to 32 bits
- if (dst64 > DESC_DST_MASK) {
- return DMA_FAILURE;
- }
-
- // length limited to 24 bits
- if (length > DESC_LENGTH_MASK) {
+ uint64_t dst64 = reinterpret_cast<uint64_t>(dst);
+ if (!dst64 || dst64 > DESC_DST_MASK) {
return DMA_FAILURE;
}
- uint32_t src32 = src64 & DESC_SRC_MASK;
- uint32_t dst32 = dst64 & DESC_DST_MASK;
+ uint32_t src32 = static_cast<uint32_t>(src64);
+ uint32_t dst32 = static_cast<uint32_t>(dst64);
+ // allocate new descriptor
void* dma_desc = nullptr;
-
int ret = posix_memalign(&dma_desc, DMA_DESC_2D_SIZE, DMA_DESC_2D_SIZE);
- if (ret) {
Review Comment:
Added to the backlog
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949036566
##########
src/runtime/hexagon/hexagon_user_dma.cc:
##########
@@ -90,32 +78,80 @@ int hexagon_user_dma_1d_sync_helper(void* dst, void* src, uint32_t length) {
dma_desc_set_src(dma_desc, src32);
dma_desc_set_dst(dma_desc, dst32);
- dmstart(dma_desc);
- unsigned int status = dmwait() & DM0_STATUS_MASK;
- unsigned int done = dma_desc_get_done(dma_desc);
+ // only for first DMA
+ if (first_dma_) {
+ // reset DMA engine
+ auto status = Init();
+ if (status != DM0_STATUS_IDLE) {
+ return DMA_FAILURE;
+ }
+
+ // `dmstart` first descriptor
+ dmstart(dma_desc);
+ first_dma_ = false;
+ } else {
+ // `dmlink` descriptor to tail
+ dmlink(dma_descriptors_.back(), dma_desc);
Review Comment:
From what I can tell, there is no limit. Perhaps @kparzysz-quic or @arangasa have a comment here?
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r949489424
##########
src/runtime/hexagon/hexagon_user_dma.h:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include <vector>
+
+#ifndef TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+#define TVM_RUNTIME_HEXAGON_HEXAGON_USER_DMA_H_
+
+namespace tvm {
+namespace runtime {
+namespace hexagon {
+
+#define DMA_SUCCESS 0
+#define DMA_FAILURE -1
+
+class HexagonUserDMA {
+ public:
+ /*!
+ * \brief Initiate DMA to copy memory from source to destination address
+ * \param dst Destination address
+ * \param src Source address
+ * \param length Length in bytes to copy
+ * \returns Status, either DMA_SUCCESS or DMA_FAILURE
+ */
+ int Copy(void* dst, void* src, uint32_t length);
+
+ /*!
+ * \brief Wait until the number of DMAs in flight is less than or equal to some maximum
+ * \param max_dmas_in_flight Maximum number of DMAs allowed to be in flight
+ * to satisfy the `Wait` e.g. use `Wait(0)` to wait on "all" outstanding DMAs to complete
+ */
+ void Wait(uint32_t max_dmas_in_flight);
+
+ /*!
+ * \brief Poll the number of DMAs in flight
+ * \returns Number of DMAs in flight
+ */
+ uint32_t Poll();
+
+ //! HexagonUserDMA uses the singleton pattern
+ static HexagonUserDMA& Get() {
+ static HexagonUserDMA* hud = new HexagonUserDMA();
+ return *hud;
+ }
+
+ private:
+ HexagonUserDMA() = default;
+ ~HexagonUserDMA();
+ HexagonUserDMA(const HexagonUserDMA&) = delete;
+ HexagonUserDMA& operator=(const HexagonUserDMA&) = delete;
+ HexagonUserDMA(HexagonUserDMA&&) = delete;
+ HexagonUserDMA& operator=(HexagonUserDMA&&) = delete;
+
+ //! \brief Initializes / resets the Hexagon User DMA engine
+ unsigned int Init();
+
+ //! \brief Calculates the number of DMAs in flight
+ uint32_t DMAsInFlight();
+
+ //! \brief Stores descriptors for all DMAs
+ std::vector<void*> dma_descriptors_;
Review Comment:
Yes, in the destructor:
```
HexagonUserDMA::~HexagonUserDMA() {
Init(); // reset DMA engine
for (auto dma_desc : dma_descriptors_) {
free(dma_desc);
}
```
--
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
[GitHub] [tvm] adstraw commented on a diff in pull request #12411: [Hexagon] Asynchronous DMA support
Posted by GitBox <gi...@apache.org>.
adstraw commented on code in PR #12411:
URL: https://github.com/apache/tvm/pull/12411#discussion_r954406986
##########
tests/cpp-runtime/hexagon/hexagon_user_dma_tests.cc:
##########
@@ -57,9 +57,9 @@ TEST_F(HexagonUserDMATest, wait) {
HexagonUserDMA::Get().Wait(10);
}
Review Comment:
Added new RingBuffer class along with testing.
--
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