ipc4: replace fw_reg spinlock with mutex#10693
ipc4: replace fw_reg spinlock with mutex#10693kv2019i wants to merge 2 commits intothesofproject:mainfrom
Conversation
To enable use of Zephyr sys/mutex.h interface in SOF audio pipeline code, add a rtos/mutex.h wrapper for this. In Zephyr build, the native Zephyr interface is used and in posix builds (e.g. testbench), a no-op implementation is provided (similarly that already exists for k_mutex). Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
kv2019i
requested review from
abonislawski,
dbaluta,
iuliana-prodan,
lbetlej,
lgirdwood,
lyakh,
mmaka1 and
plbossart
as code owners
|
I'm using this to move audio pipelines to user-space in #10558 , but wanted to submit this early to get review feedback. If ok, I'll proceed to use sys_mutex more widely in the audio pipeline code. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR replaces IPC4 firmware register (fw_reg) synchronization from a per-struct sof Zephyr spinlock to a sys_mutex, with the goal of removing spinlocks from ipc4/dai.c.
Changes:
- Remove
fw_reg_lock(k_spinlock) fromstruct soffor both Zephyr and POSIX builds. - Introduce a file-scope
SYS_MUTEX_DEFINE(fw_reg_lock)insrc/ipc/ipc4/dai.cand usesys_mutex_lock/unlock()around mailbox register accesses. - Add
sys_mutexheader support in Zephyr and a stubsys_muteximplementation for POSIX builds.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/include/rtos/sof.h | Removes IPC4 fw_reg_lock from struct sof. |
| zephyr/include/rtos/mutex.h | Includes Zephyr sys/mutex.h to support sys_mutex. |
| src/ipc/ipc4/dai.c | Replaces k_spin_lock usage with a static sys_mutex for fw register access. |
| src/init/init.c | Removes initialization of the deleted sof->fw_reg_lock spinlock. |
| posix/include/rtos/sof.h | Mirrors Zephyr: removes fw_reg_lock from struct sof. |
| posix/include/rtos/mutex.h | Adds a POSIX-side sys_mutex API stub. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ipc/ipc4/dai.c
Outdated
|
|
||
| LOG_MODULE_DECLARE(ipc, CONFIG_SOF_LOG_LEVEL); | ||
|
|
||
| static SYS_MUTEX_DEFINE(fw_reg_lock); |
There was a problem hiding this comment.
is it now only used by the DAI code? Maybe reflect that in the name? Also, this object isn't accessible from the userspace, so it can be a normal k_mutex or is there a plan to make it userspace-accessible in the future? In fact should we follow the Linux example and define exactly what it is protecting?
There was a problem hiding this comment.
Yes. Not sure why the spinlock was aded to the sof() singleton, but it seems just not needed and cleaner to have a local lock. I have a one-liner patch to export this to user-space with:
-static SYS_MUTEX_DEFINE(fw_reg_lock);
+static APP_SYSUSER_BSS SYS_MUTEX_DEFINE(fw_reg_lock);
Ack on the naming, let me address the naming in V2. I just following the old naming practise, but agreed, this can be improved.
Replace the spinlock-based synchronization mechanism for firmware register access with a mutex-based approach. This change removes the fw_reg_lock spinlock field from the sof structure (both POSIX and Zephyr variants), eliminates the spinlock initialization in primary_core_init(), and updates the dai_dma_release(), dai_release_llp_slot(), and dai_get_unused_llp_slot() functions to use sys_mutex_lock/unlock instead of k_spin_lock/unlock. All SOF IPC and audio pipeline code is run in threads, so using spinlocks is no longer necessary. Using mutex is a best fit and allows the audio pipeline code to be used also in user-space threads. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
kv2019i
force-pushed
the
202604-fw-regs-remove-spinlocks
branch
from
47deec9 to
ead1b8b
Compare
|
V2 pushed:
|
Series to add sys/mutex.h support for SOF audio code and use it to remove spinlocks from ipc4/dai.c code.