Reduce overhead when doing SPI transactions #28093

bugobliterator · 2024-09-13T06:48:28Z

Tested on CubeOrangePlus and CubeRedPrimary with Invensensev3 driver
I used debug trace to analyse timeline for read_fifo to check if we have any substantial overhead while doing SPI transactions. It turned out that calling spiStart and spiStop which in turn call dmaStreamAlloc and and dealloc to take substantial amount of CPU call almost 3-4us. Also we were calling Shared DMA mutex lock and unlock when the DMA is not shared between peripherals.
Following changes made SPI transactions much less CPU intensive:

- Do not reset SPI peripheral using spistop and spistart calls if SPI config hasn't changed.
- Also make changes to SPI frequencies for CubeOrangePlus and CubeRedPrimary to match on the bus where possible (minor CPU gain through this, just first part was enough as most transactions are IMU anyways).
- Do not call mutex lock unlock for unshared DMAs.

Results:

CubeRedPrimary:

Before:
SPI1 PRI=181 sp=0x20001AA8 STACK= 792/1464 LOAD=13.4%
SPI4 PRI=181 sp=0x200021C0 STACK= 872/1464 LOAD=13.4%
SPI2 PRI=181 sp=0x20005380 STACK= 896/1464 LOAD=13.3%

After:
SPI1 PRI=181 sp=0x20001B08 STACK= 792/1464 LOAD= 9.7%
SPI4 PRI=181 sp=0x20002220 STACK= 792/1464 LOAD=11.2%
SPI2 PRI=181 sp=0x200053E0 STACK= 840/1464 LOAD= 8.3%

CubeOrangePlus:

Before:
SPI4 PRI=181 sp=0x3002AF58 STACK= 668/1464 LOAD=25.5%
SPI1 PRI=181 sp=0x3002B668 STACK= 668/1464 LOAD=14.5%

After:
SPI4 PRI=181 sp=0x3002AF78 STACK= 668/1464 LOAD=19.2%
SPI1 PRI=181 sp=0x3002B688 STACK= 668/1464 LOAD= 9.5%

10% total CPU time recovered for both CubeOrangePlus and CubeRedPrimary

…fifo count

… performance

andyp1per · 2024-09-13T07:47:40Z

libraries/AP_HAL_ChibiOS/shared_dma.cpp

@@ -87,6 +87,9 @@ void Shared_DMA::unregister()
 // lock one stream
 bool Shared_DMA::lock_stream(uint8_t stream_id)
 {
+    if (!is_shared(stream_id)) {


I'm not sure this is safe. I'm pretty sure the dshot code for instance uses the DMA lock to mediate access, so you would need to also check whether another thread has taken the lock.

@andyp1per can you point to the code where this is done, I don't think Shared DMA is used that way in RCOutput, its only role is to allocate dma for use for dshot and bdshot, as far as I am aware dshot stuff happens entirely from the single thread. What is DMA lock used to mediate access between?

ardupilot/libraries/AP_HAL_ChibiOS/RCOutput.cpp

Line 1628 in 86f51ee

group.dma_handle->lock();

It's mediating access between the _UP DMA channel and the IC DMA channel. Looking at it further it may be ok since these should end up meaning it actually shows up as shared here, but would need some careful testing.

andyp1per · 2024-09-13T07:48:29Z

libraries/AP_HAL_ChibiOS/hwdef/CubeRedPrimary/hwdef.dat

 SPIDEV rm3100           SPI2 DEVID2  RM3100_CS          MODE3  1*MHZ  1*MHZ

 SPIDEV icm20649         SPI4 DEVID1  ICM_2_CS           MODE3  4*MHZ  8*MHZ
 # alternative to icm20649
-SPIDEV icm45686         SPI4 DEVID1  ICM_2_CS           MODE3  2*MHZ  8*MHZ
+SPIDEV icm45686         SPI4 DEVID1  ICM_2_CS           MODE3 20*MHZ 20*MHZ


I thought the slower rate was slower for a reason, i.e. in case of recovery?

@andyp1per the reason is simply legacy, we have been using that frequency in older Sensors.

Yeah, but for instance we needed that to solve a problem on ICM42688 (changed it on Matek for instance), so not 100% convinced this is an older sensor problem.

andyp1per · 2024-09-13T07:49:55Z

libraries/AP_HAL_ChibiOS/SPIDevice.cpp

+        last_spicfg.cfg2 == spicfg.cfg2
+#else
+        last_spicfg.cr1 == spicfg.cr1 &&
+        last_spicfg.cr2 == spicfg.cr2


I feel like you might need to check the DMA lock state here as well. If the lock is shared (and therefore the channel shared), the periperal may need to be shutdown to allow another to use the DMA channel.

@andyp1per we only get to this line near the end of acquire bus which must have been successful.

I guess that then begs the question as to why stop/start is necessary here at all? Also why not put the start() inside the if as well?

andyp1per · 2024-09-21T15:36:34Z

libraries/AP_InertialSensor/AP_InertialSensor_Invensensev3.cpp

-        uint8_t n = MIN(n_samples, INV3_FIFO_BUFFER_LEN);
-        if (!block_read(reg_data, (uint8_t*)fifo_buffer, n * fifo_sample_size)) {
-            goto check_registers;
+    do {


Do you know how much difference this makes? I ask because there is a bug in the v3 driver to do with DMA usage which we should fix - a56660e - that said this also has the cost of two SPI transactions, so I am wondering if there is a way of doing both?

andyp1per

Need a comment for the IMU changes

andyp1per · 2024-09-21T15:37:39Z

libraries/AP_InertialSensor/AP_InertialSensor_Invensensev3.cpp

+        uint8_t n;
+        void *fifo_result = nullptr;
+        if (n_samples == 0) {
+            if (!block_read(reg_counth, (uint8_t*)fifo_buffer, 2 + fifo_sample_size)) {


This needs a comment. I think I understand what you are trying to achieve here, but its not at all clear.

andyp1per · 2024-09-21T15:55:24Z

I did a short flight with just the SPI changes - had no issues and this gives a nice bump of about 5% CPU gain, which with my Dynamic FIFO change (#27841) is giving me about 15% CPU gain

tridge · 2024-09-30T23:33:51Z

@bugobliterator I'd like to discuss this a bit more with you. There are risks to not restarting the SPI peripheral. If (due to eg. a voltage spike) the SPI peripheral gets into a bad state, then with a single sensor bus that state could persist forever and take down the vehicle. We switched to always re-init some time ago when we saw this sort of bad persistent state on I2C peripherals.
I'd really rather fix the real problem which is that we are doing transfers at such a high rate for InvensenseV3

tridge · 2024-10-16T08:33:24Z

libraries/AP_InertialSensor/AP_InertialSensor_Invensensev3.cpp

+        uint8_t n;
+        void *fifo_result = nullptr;
+        if (n_samples == 0) {
+            if (!block_read(reg_counth, (uint8_t*)fifo_buffer, 2 + fifo_sample_size)) {


should be using _dev->transfer() like in InvensenseV2 and V1, to save the stack space and copy, and preventing a watchdog

andyp1per

This needs to be split into SPI changes and IMU changes

bugobliterator added 4 commits September 13, 2024 16:32

AP_HAL_ChibiOS: do not reset SPI periphperal if config remains the same

849e7e9

AP_HAL_ChibiOS: remove mutex lock and unlock for unshared DMAs

b11299d

AP_InertialSensor:Invensensev3: read first fifo frame along with the …

5495593

…fifo count

AP_HAL_ChibiOS: Cube*: make SPI config same where possible to improve…

c8da9c9

… performance

bugobliterator force-pushed the pr-spi-perf branch from 7ed2419 to c8da9c9 Compare September 13, 2024 06:51

bugobliterator requested review from andyp1per and tridge September 13, 2024 06:53

andyp1per reviewed Sep 13, 2024

View reviewed changes

bugobliterator added the DevCallEU label Sep 18, 2024

andyp1per reviewed Sep 21, 2024

View reviewed changes

andyp1per requested changes Sep 21, 2024

View reviewed changes

andyp1per mentioned this pull request Sep 21, 2024

Dynamic FIFO Buffering #27841

Merged

peterbarker added DevCallTopic and removed DevCallEU labels Sep 25, 2024

CraigElder removed the DevCallTopic label Sep 30, 2024

bugobliterator added the DevCallEU label Oct 14, 2024

tridge requested changes Oct 16, 2024

View reviewed changes

peterbarker added DevCallTopic and removed DevCallEU labels Nov 6, 2024

andyp1per requested changes Nov 6, 2024

View reviewed changes

CraigElder removed the DevCallTopic label Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce overhead when doing SPI transactions #28093

Reduce overhead when doing SPI transactions #28093

bugobliterator commented Sep 13, 2024

andyp1per Sep 13, 2024

bugobliterator Sep 17, 2024

andyp1per Sep 17, 2024

andyp1per Sep 13, 2024

bugobliterator Sep 17, 2024

andyp1per Sep 17, 2024

andyp1per Sep 13, 2024

bugobliterator Sep 17, 2024

andyp1per Sep 17, 2024

andyp1per Sep 21, 2024

andyp1per left a comment

andyp1per Sep 21, 2024

andyp1per commented Sep 21, 2024

tridge commented Sep 30, 2024

tridge Oct 16, 2024

andyp1per left a comment

Reduce overhead when doing SPI transactions #28093

Are you sure you want to change the base?

Reduce overhead when doing SPI transactions #28093

Conversation

bugobliterator commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyp1per left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyp1per commented Sep 21, 2024

tridge commented Sep 30, 2024

Choose a reason for hiding this comment

andyp1per left a comment

Choose a reason for hiding this comment