audio: fast-get: improve userspace compatibility#10524
audio: fast-get: improve userspace compatibility#10524lyakh wants to merge 3 commits intothesofproject:mainfrom
Conversation
Userspace modules should only call mod_fast_get() and mod_fast_put(), which already can cross into the kernel space on their own, so fast_get() and fast_put() themselves don't need to be syscalls. Remove their syscall implementations. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
fast_get.c doesn't have a "trace context" - it doesn't have a DECLARE_TR_CTX() call. Hencs all calls to tr_err() and friends are wrong. Replace them with respective LOG_*() analogs. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
requested review from
abonislawski,
dbaluta,
iuliana-prodan,
jxstelter,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
plbossart and
ranj063
as code owners
There was a problem hiding this comment.
Pull request overview
This PR removes the syscall infrastructure for fast-get/fast-put functions and enhances userspace compatibility by enabling direct buffer sharing between user and kernel spaces. The changes eliminate unnecessary syscall overhead and prepare for more efficient memory management in userspace contexts.
Changes:
- Converted fast-get functions from syscalls to regular functions, removing z_impl_* and z_vrfy_* wrappers
- Added userspace-specific logic to handle memory partition grants for large buffers (> PAGE_SZ/2)
- Updated logging from SOF trace macros (tr_) to standard LOG_ macros with conditional compilation for non-Zephyr builds
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| zephyr/lib/fast-get.c | Removed syscall infrastructure, added userspace memory partition management, updated logging macros |
| src/include/sof/lib/fast-get.h | Simplified API by removing syscall declarations and conditional compilation |
| src/audio/module_adapter/module_adapter.c | Updated include path from platform/platform.h to sof/platform.h for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zephyr/lib/fast-get.c
Outdated
| .size = ALIGN_UP(entry->size, CONFIG_MM_DRV_PAGE_SIZE), | ||
| .attr = K_MEM_PARTITION_P_RO_U_RO | XTENSA_MMU_CACHED_WB, |
There was a problem hiding this comment.
The constant XTENSA_MMU_CACHED_WB is Xtensa-specific but is used without architecture-specific guards. If CONFIG_USERSPACE is enabled on a non-Xtensa platform, this will cause a compilation error. Consider using architecture-specific conditionals or defining a platform-agnostic cache attribute constant. For example, you could use preprocessor conditionals to select the appropriate cache attribute based on the target architecture.
| .size = ALIGN_UP(entry->size, CONFIG_MM_DRV_PAGE_SIZE), | |
| .attr = K_MEM_PARTITION_P_RO_U_RO | XTENSA_MMU_CACHED_WB, | |
| .size = ALIGN_UP(entry->size, CONFIG_MM_DRV_PAGE_SIZE), | |
| #ifdef XTENSA_MMU_CACHED_WB | |
| .attr = K_MEM_PARTITION_P_RO_U_RO | XTENSA_MMU_CACHED_WB, | |
| #else | |
| .attr = K_MEM_PARTITION_P_RO_U_RO, | |
| #endif |
zephyr/lib/fast-get.c
Outdated
| int err = fast_get_access_grant(entry->thread, ret, size); | ||
|
|
||
| if (err < 0) { | ||
| sof_heap_free(NULL, ret); |
There was a problem hiding this comment.
When fast_get_access_grant fails, the allocated memory is freed and ret is set to NULL, but the entry fields (entry->size, entry->sram_ptr, entry->dram_ptr set at line 224) remain set. This leaves the entry in an inconsistent state. Consider resetting the entry with memset(entry, 0, sizeof(*entry)) after freeing the memory to ensure clean error handling, similar to what's done in fast_put at line 278.
| sof_heap_free(NULL, ret); | |
| sof_heap_free(NULL, ret); | |
| memset(entry, 0, sizeof(*entry)); |
zephyr/lib/fast-get.c
Outdated
|
|
||
| ret = sof_heap_alloc(heap, SOF_MEM_FLAG_USER, size, PLATFORM_DCACHE_ALIGN); | ||
| /* | ||
| * If a userspace threads is the first user to fast-get the buffer, an |
There was a problem hiding this comment.
The word "threads" should be "thread" (singular) to match the grammatical context of the sentence.
| * If a userspace threads is the first user to fast-get the buffer, an | |
| * If a userspace thread is the first user to fast-get the buffer, an |
Fix "unused function" Zephyr compilation warnings. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
| #define LOG_DBG(...) do {} while (0) | ||
| #define LOG_INF(...) do {} while (0) | ||
| #define LOG_WRN(...) do {} while (0) | ||
| #define LOG_ERR(...) do {} while (0) |
There was a problem hiding this comment.
As I understand, these are Zephyr macros. I suspect they might be used in other places soon. Do we want to move this else into a separate file that is included when Zephyr is not present?
There was a problem hiding this comment.
@lyakh Also wondering why do we need to ifdef Zephyr, this is zephyr/lib/fast-get.c after all?
There was a problem hiding this comment.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Check the comment in second patch.
| #define LOG_DBG(...) do {} while (0) | ||
| #define LOG_INF(...) do {} while (0) | ||
| #define LOG_WRN(...) do {} while (0) | ||
| #define LOG_ERR(...) do {} while (0) |
There was a problem hiding this comment.
@lyakh Also wondering why do we need to ifdef Zephyr, this is zephyr/lib/fast-get.c after all?
3 participants
remove unneeded syscalls and fix compilation warnings (part of #10469 )