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 2021/05/11 22:58:41 UTC
[GitHub] [tvm] mehrdadh opened a new pull request #8021: [Fix][microTVM] QEMU RPC issue
mehrdadh opened a new pull request #8021:
URL: https://github.com/apache/tvm/pull/8021
This issue happens when we send a large tensor over RPC to the zephyr qemu target specially when it runs on a fast machine. We fixed the RPC large transfer size for microTVM at #7838, however, #7838 doesn't include RPC test with qemu and didn't catch this problem.
In this PR:
- we add qemu RPC test
- increase the size of ring buffer used for UART in zephyr to be more than TVM_CRT_MAX_PACKET_SIZE_BYTES to fix the UART ring buffer issue
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch merged pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
areusch merged pull request #8021:
URL: https://github.com/apache/tvm/pull/8021
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r631156515
##########
File path: tests/micro/zephyr/conftest.py
##########
@@ -44,6 +44,18 @@ def pytest_addoption(parser):
parser.addoption(
"--west-cmd", default="west", help="Path to `west` command for flashing device."
)
+ parser.addoption(
+ "--tvm-build",
+ action="store_true",
+ default=True,
Review comment:
i think best not to checkin the test with this set default True. Maybe we can make it `--skip-build`? I don't think we have proper dependency tracking here.
##########
File path: tests/micro/zephyr/test_zephyr.py
##########
@@ -391,8 +389,42 @@ def test_byoc_utvm(platform, west_cmd):
model=model,
zephyr_board=zephyr_board,
west_cmd=west_cmd,
+ build_config=build_config,
)
+def _make_large_tensors_sess(model, zephyr_board, west_cmd, shape, build_config):
Review comment:
the "large" part is just from the shape--maybe name as `_make_add_sess_with_shape`?
##########
File path: apps/microtvm/zephyr/demo_runtime/crt/crt_config.h
##########
@@ -42,7 +42,7 @@
#define TVM_CRT_MAX_REGISTERED_MODULES 2
/*! Maximum packet size, in bytes, including the length header. */
-#define TVM_CRT_MAX_PACKET_SIZE_BYTES 8192
+#define TVM_CRT_MAX_PACKET_SIZE_BYTES 4 * 1024
Review comment:
add parens around the value, just in case
##########
File path: apps/microtvm/zephyr/demo_runtime/src/main.c
##########
@@ -214,7 +214,8 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
}
// Ring buffer used to store data read from the UART on rx interrupt.
-#define RING_BUF_SIZE_BYTES 4 * 1024
+// Should be larger than TVM_CRT_MAX_PACKET_SIZE_BYTES
Review comment:
can you comment how much larger, and why?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r641831869
##########
File path: apps/microtvm/zephyr/host_driven/src/main.c
##########
@@ -214,33 +216,37 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
}
// Ring buffer used to store data read from the UART on rx interrupt.
-#define RING_BUF_SIZE_BYTES 4 * 1024
-RING_BUF_DECLARE(uart_rx_rbuf, RING_BUF_SIZE_BYTES);
-
-// Small buffer used to read data from the UART into the ring buffer.
-static uint8_t uart_data[8];
+// TODO(mehrdadh): Explore why offset is required.
+#define RING_BUF_SIZE_BYTES (TVM_CRT_MAX_PACKET_SIZE_BYTES + 100)
+RING_BUF_ITEM_DECLARE_SIZE(uart_rx_rbuf, RING_BUF_SIZE_BYTES);
// UART interrupt callback.
void uart_irq_cb(const struct device* dev, void* user_data) {
- while (uart_irq_update(dev) && uart_irq_is_pending(dev)) {
+ uart_irq_update(dev);
+ if (uart_irq_is_pending(dev)) {
struct ring_buf* rbuf = (struct ring_buf*)user_data;
if (uart_irq_rx_ready(dev) != 0) {
- for (;;) {
- // Read a small chunk of data from the UART.
- int bytes_read = uart_fifo_read(dev, uart_data, sizeof(uart_data));
- if (bytes_read < 0) {
- TVMPlatformAbort((tvm_crt_error_t)0xbeef1);
- } else if (bytes_read == 0) {
- break;
- }
- // Write it into the ring buffer.
- int bytes_written = ring_buf_put(rbuf, uart_data, bytes_read);
- if (bytes_read != bytes_written) {
- TVMPlatformAbort((tvm_crt_error_t)0xbeef2);
- }
- // CHECK_EQ(bytes_read, bytes_written, "bytes_read: %d; bytes_written: %d", bytes_read,
- // bytes_written);
+ uint8_t* data;
+ uint32_t size;
+ size = ring_buf_put_claim(rbuf, &data, RING_BUF_SIZE_BYTES);
Review comment:
This is because in this case we don't need to use an intermediate buffer and we can read/write directly from/to the ring buffer. This is specially important for reading from ring buffer, because before this we had to allocate an extra array and do extra an extra copy of a large array.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r641819875
##########
File path: apps/microtvm/zephyr/host_driven/src/main.c
##########
@@ -214,33 +216,37 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
}
// Ring buffer used to store data read from the UART on rx interrupt.
-#define RING_BUF_SIZE_BYTES 4 * 1024
-RING_BUF_DECLARE(uart_rx_rbuf, RING_BUF_SIZE_BYTES);
-
-// Small buffer used to read data from the UART into the ring buffer.
-static uint8_t uart_data[8];
+// TODO(mehrdadh): Explore why offset is required.
Review comment:
can you explain why TVM_CRT_MAX_PACKET_SIZE_BYTES is required, though? I believe it's not needed on most physical hardware
##########
File path: apps/microtvm/zephyr/host_driven/src/main.c
##########
@@ -214,33 +216,37 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
}
// Ring buffer used to store data read from the UART on rx interrupt.
-#define RING_BUF_SIZE_BYTES 4 * 1024
-RING_BUF_DECLARE(uart_rx_rbuf, RING_BUF_SIZE_BYTES);
-
-// Small buffer used to read data from the UART into the ring buffer.
-static uint8_t uart_data[8];
+// TODO(mehrdadh): Explore why offset is required.
+#define RING_BUF_SIZE_BYTES (TVM_CRT_MAX_PACKET_SIZE_BYTES + 100)
+RING_BUF_ITEM_DECLARE_SIZE(uart_rx_rbuf, RING_BUF_SIZE_BYTES);
// UART interrupt callback.
void uart_irq_cb(const struct device* dev, void* user_data) {
- while (uart_irq_update(dev) && uart_irq_is_pending(dev)) {
+ uart_irq_update(dev);
+ if (uart_irq_is_pending(dev)) {
struct ring_buf* rbuf = (struct ring_buf*)user_data;
if (uart_irq_rx_ready(dev) != 0) {
- for (;;) {
- // Read a small chunk of data from the UART.
- int bytes_read = uart_fifo_read(dev, uart_data, sizeof(uart_data));
- if (bytes_read < 0) {
- TVMPlatformAbort((tvm_crt_error_t)0xbeef1);
- } else if (bytes_read == 0) {
- break;
- }
- // Write it into the ring buffer.
- int bytes_written = ring_buf_put(rbuf, uart_data, bytes_read);
- if (bytes_read != bytes_written) {
- TVMPlatformAbort((tvm_crt_error_t)0xbeef2);
- }
- // CHECK_EQ(bytes_read, bytes_written, "bytes_read: %d; bytes_written: %d", bytes_read,
- // bytes_written);
+ uint8_t* data;
+ uint32_t size;
+ size = ring_buf_put_claim(rbuf, &data, RING_BUF_SIZE_BYTES);
Review comment:
can you add a comment explaining why we do ring_buf_put_claim instead of the other way?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r646803124
##########
File path: apps/microtvm/zephyr/host_driven/src/main.c
##########
@@ -214,33 +216,37 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
}
// Ring buffer used to store data read from the UART on rx interrupt.
-#define RING_BUF_SIZE_BYTES 4 * 1024
-RING_BUF_DECLARE(uart_rx_rbuf, RING_BUF_SIZE_BYTES);
-
-// Small buffer used to read data from the UART into the ring buffer.
-static uint8_t uart_data[8];
+// TODO(mehrdadh): Explore why offset is required.
Review comment:
I checked this on physical hardware and as you mentioned this large value for ring buffer size is only required for testing on QEMU. I added a comment about this. Please let me know if you have any other concern.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r631297626
##########
File path: apps/microtvm/zephyr/demo_runtime/src/main.c
##########
@@ -248,6 +249,9 @@ void uart_irq_cb(const struct device* dev, void* user_data) {
// Used to initialize the UART receiver.
void uart_rx_init(struct ring_buf* rbuf, const struct device* dev) {
+ const struct uart_config config = {
+ .baudrate = UART_BAUDRATE, .parity = 0, .stop_bits = 1, .data_bits = 3, .flow_ctrl = 0};
Review comment:
I've tested your changes against the Disco board and it's alright. So aside the magic `25` in `TVM_CRT_MAX_PACKET_SIZE_BYTES+25` for the RX buffer which you're currently discussing with Andrew and seems to be determined experimentally (maybe leave a comment about it?) the change LGTM.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#issuecomment-847511096
@areusch this is ready to review. PTAL.
thanks!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] gromero commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
gromero commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r631290613
##########
File path: apps/microtvm/zephyr/demo_runtime/src/main.c
##########
@@ -248,6 +249,9 @@ void uart_irq_cb(const struct device* dev, void* user_data) {
// Used to initialize the UART receiver.
void uart_rx_init(struct ring_buf* rbuf, const struct device* dev) {
+ const struct uart_config config = {
+ .baudrate = UART_BAUDRATE, .parity = 0, .stop_bits = 1, .data_bits = 3, .flow_ctrl = 0};
Review comment:
@mehrdadh Hi Mehrdad. I think it would be nice to use the enums from Zephyr here (for instance .data_bits = 3 might be a bit enigmatic for some folks):
```
const struct uart_config config = {
- .baudrate = UART_BAUDRATE, .parity = 0, .stop_bits = 1, .data_bits = 3, .flow_ctrl = 0};
+ .baudrate = UART_BAUDRATE,
+ .parity = UART_CFG_PARITY_NONE,
+ .stop_bits = UART_CFG_STOP_BITS_1,
+ .data_bits = UART_CFG_DATA_BITS_8,
+ .flow_ctrl = UART_CFG_FLOW_CTRL_NONE
+ };
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r637245704
##########
File path: apps/microtvm/zephyr/demo_runtime/src/main.c
##########
@@ -248,6 +249,9 @@ void uart_irq_cb(const struct device* dev, void* user_data) {
// Used to initialize the UART receiver.
void uart_rx_init(struct ring_buf* rbuf, const struct device* dev) {
+ const struct uart_config config = {
+ .baudrate = UART_BAUDRATE, .parity = 0, .stop_bits = 1, .data_bits = 3, .flow_ctrl = 0};
Review comment:
I removed the UART setting since it didn't help with fixing the issue. Still trying to understand why magic number 25 works 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r631242197
##########
File path: apps/microtvm/zephyr/demo_runtime/src/main.c
##########
@@ -214,7 +214,8 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
}
// Ring buffer used to store data read from the UART on rx interrupt.
-#define RING_BUF_SIZE_BYTES 4 * 1024
+// Should be larger than TVM_CRT_MAX_PACKET_SIZE_BYTES
Review comment:
the minimum value is 25 bytes and I don't know why.
Also I tested with different baudrates (default is 115200) to see if that solve the issue, but it didn't help.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r641832061
##########
File path: apps/microtvm/zephyr/host_driven/src/main.c
##########
@@ -214,33 +216,37 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
}
// Ring buffer used to store data read from the UART on rx interrupt.
-#define RING_BUF_SIZE_BYTES 4 * 1024
-RING_BUF_DECLARE(uart_rx_rbuf, RING_BUF_SIZE_BYTES);
-
-// Small buffer used to read data from the UART into the ring buffer.
-static uint8_t uart_data[8];
+// TODO(mehrdadh): Explore why offset is required.
Review comment:
This could be because of qemu only. Let me confirm that and add comment on the file.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] areusch commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
areusch commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r645848740
##########
File path: apps/microtvm/zephyr/host_driven/src/main.c
##########
@@ -214,33 +216,37 @@ tvm_crt_error_t TVMPlatformTimerStop(double* elapsed_time_seconds) {
}
// Ring buffer used to store data read from the UART on rx interrupt.
-#define RING_BUF_SIZE_BYTES 4 * 1024
-RING_BUF_DECLARE(uart_rx_rbuf, RING_BUF_SIZE_BYTES);
-
-// Small buffer used to read data from the UART into the ring buffer.
-static uint8_t uart_data[8];
+// TODO(mehrdadh): Explore why offset is required.
Review comment:
just wanted to follow-up on this one
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mehrdadh commented on a change in pull request #8021: [Fix][microTVM] QEMU RPC issue
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on a change in pull request #8021:
URL: https://github.com/apache/tvm/pull/8021#discussion_r631237594
##########
File path: tests/micro/zephyr/conftest.py
##########
@@ -44,6 +44,18 @@ def pytest_addoption(parser):
parser.addoption(
"--west-cmd", default="west", help="Path to `west` command for flashing device."
)
+ parser.addoption(
+ "--tvm-build",
+ action="store_true",
+ default=True,
Review comment:
changed it to --skip-build and default is false.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org