Skip to content

rp2: Fix power consumption when sleeping with a timeout. #15398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

projectgus
Copy link
Contributor

@projectgus projectgus commented Jul 3, 2024

Summary

Fixes a regression introduced in 3af006e (part of #13329), where WFE never blocked in mp_wfe_or_timeout() function and would busy-wait instead. This increases power consumption measurably. Originally found when testing #15345

Root cause is that mp_wfe_or_timeout() calls soft timer functions that (after the regression) call recursive_mutex_enter() and recursive_mutex_exit(). Mutex exit calls lock_internal_spin_unlock_with_notify() and the default pico-sdk implementation of this macro issues a SEV which negates the WFE that follows it, meaning the CPU never suspends.

See https://forums.raspberrypi.com/viewtopic.php?p=2233908 for more details.

The fix in this comment adds a custom "nowait" variant mutex that doesn't do WFE/SEV, and uses this one for PendSV. This will use more power when there's contention for the PendSV mutex as the other core will spin, but this shouldn't happen very often.

Additional commit fixes a bug that was masked by this bug, specifically that core1 didn't have any way to resume from WFE (because the soft timer interrupt only triggers on core0).

Testing

Tested on PICO board while measuring USB power consumption. Prior to this fix, time.sleep(1) uses 3-4mA more power than sitting at an idle REPL. After this fix, the power consumption is approximately the same (actually approx 0.3mA higher while sleeping, I assume due to the extra clock or maybe because of USB traffic).

Also ran the full rp2 test suite multiple times. The thread_sleep2 test successfully found the core1 bug mentioned above.

Also connected a PICO-W to a Wi-Fi network.

Trade-offs and Alternatives

This is the third fix I wrote for this issue(!)

  • The first version of this PR replaced the soft timer in mp_wfe_or_timeout() with a dedicated hardware timer alarm. This works fine for single core, but in dual core there's potential for both cores to enter this function at the same time meaning this approach is no longer simple.
  • I also tried making all the recursive mutexes the "nowait" variety, instead of adding a new variant. This works fine, but it means if there is contention over any mutex then the waiting core will spin. Mostly this probably doesn't matter as there won't be a lot of contention, but there are some pathological cases: For example a program where core1 spends most of its time blocked on a lock or similar.

This approach adds a bit more code size, but it means the low power mutex variant is still used everywhere except for PendSV - and there shouldn't be a lot of lock contention over PendSV.

Copy link

github-actions bot commented Jul 3, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   -64 -0.007% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@projectgus projectgus marked this pull request as draft July 3, 2024 03:24
@projectgus

This comment was marked as outdated.

@projectgus projectgus force-pushed the bugfix/rp2_wfe_timeout branch from 9abc812 to e1fedbd Compare July 5, 2024 05:47
@projectgus projectgus marked this pull request as ready for review July 5, 2024 06:23
@projectgus
Copy link
Contributor Author

rp2: -64 -0.007% RPI_PICO_W

Not sure why this is, I assume some lucky linker combo? There is definitely new code being added in this PR.

@dpgeorge
Copy link
Member

dpgeorge commented Jul 5, 2024

I've tested this PR on a Pico and Pico W and the power consumption looks good for machine.idle() and time.sleep_ms(1).

Fixes a regression introduced in 3af006e
where WFE never blocked in `mp_wfe_or_timeout()` function and would
busy-wait instead.  This increases power consumption measurably.

Root cause is that `mp_wfe_or_timeout()` calls soft timer functions that
(after the regression) call `recursive_mutex_enter()` and
`recursive_mutex_exit()`.  The exit calls
`lock_internal_spin_unlock_with_notify()` and the default pico-sdk
implementation of this macro issues a SEV which negates the WFE that
follows it, meaning the CPU never suspends.

See https://forums.raspberrypi.com/viewtopic.php?p=2233908 for more
details.

The fix in this comment adds a custom "nowait" variant mutex that doesn't
do WFE/SEV, and uses this one for PendSV.  This will use more power when
there's contention for the PendSV mutex as the other core will spin, but
this shouldn't happen very often.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
If core1 executes `mp_wfe_or_timeout()` then it needs to receive an
interrupt or a SEV to resume execution, but the soft timer interrupt only
fires on core 0.  This fix adds a SEV to the soft timer interrupt handler.

This issue was masked by the issue fixed in the previous commit, as WFE
previously wasn't suspending properly.

Verified via the existing thread_sleep2 test.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@dpgeorge dpgeorge force-pushed the bugfix/rp2_wfe_timeout branch from e1fedbd to 9db16cf Compare July 23, 2024 06:04
@dpgeorge dpgeorge merged commit 9db16cf into micropython:master Jul 23, 2024
8 checks passed
@projectgus projectgus deleted the bugfix/rp2_wfe_timeout branch November 1, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants