Skip to content

buffer: extend ability to allocate on specific heap to all functions #10719

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.

Sign up for GitHub

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

Open
kv2019i wants to merge 2 commits into
base: main
Choose a base branch
from
Open

buffer: extend ability to allocate on specific heap to all functions #10719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03c5bb9
buffer: extend ability to allocate on specific heap to all functions
kv2019i Feb 16, 2026
f8e79c2
alloc.h: remove rbrealloc() and rbrealloc_align()
kv2019i Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions posix/include/rtos/alloc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,29 +97,6 @@ static inline void *rballoc(uint32_t flags, size_t bytes)
return rballoc_align(flags, bytes, PLATFORM_DCACHE_ALIGN);
}

/**
* Changes size of the memory block allocated.
* @param ptr Address of the block to resize.
* @param flags Flags, see SOF_MEM_FLAG_...
* @param bytes New size in bytes.
* @param old_bytes Old size in bytes.
* @param alignment Alignment in bytes.
* @return Pointer to the resized memory of NULL if failed.
*/
void *rbrealloc_align(void *ptr, uint32_t flags, size_t bytes,
size_t old_bytes, uint32_t alignment);

/**
* Similar to rballoc_align(), returns resized buffer aligned to
* PLATFORM_DCACHE_ALIGN.
*/
static inline void *rbrealloc(void *ptr, uint32_t flags,
size_t bytes, size_t old_bytes)
{
return rbrealloc_align(ptr, flags, bytes, old_bytes,
PLATFORM_DCACHE_ALIGN);
}

/**
* Frees the memory block.
* @param ptr Pointer to the memory block.
Expand Down
67 changes: 28 additions & 39 deletions src/audio/buffers/comp_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ static void comp_buffer_free(struct sof_audio_buffer *audio_buffer)

struct k_heap *heap = buffer->audio_buffer.heap;

rfree(buffer->stream.addr);
sof_heap_free(heap, buffer->stream.addr);
Copy link
Copy Markdown
Collaborator

@lyakh lyakh Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to me that instead of "adding an ability to specify a heap" this commit moves the actual data buffer allocations to the common module heap (when used) instead of using VMH? Wouldn't it be better to directly move this allocation to vregions? Potentially after #10702 is merged?

Copy link
Copy Markdown
Collaborator Author

@kv2019i kv2019i Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh True, although it's arguably less confusing to use the heap one passes to buffer_alloc(). But I'm ok to sequencing options and can wait for vregions.

sof_heap_free(heap, buffer);
if (heap) {
struct dp_heap_user *mod_heap_user = container_of(heap, struct dp_heap_user, heap);
Expand Down Expand Up @@ -254,7 +254,7 @@ struct comp_buffer *buffer_alloc(struct k_heap *heap, size_t size, uint32_t flag
return NULL;
}

stream_addr = rballoc_align(flags, size, align);
stream_addr = sof_heap_alloc(heap, flags, size, align);
if (!stream_addr) {
tr_err(&buffer_tr, "could not alloc size = %zu bytes of flags = 0x%x",
size, flags);
Expand All @@ -264,7 +264,7 @@ struct comp_buffer *buffer_alloc(struct k_heap *heap, size_t size, uint32_t flag
buffer = buffer_alloc_struct(heap, stream_addr, size, flags, is_shared);
if (!buffer) {
tr_err(&buffer_tr, "could not alloc buffer structure");
rfree(stream_addr);
sof_heap_free(heap, stream_addr);
}

return buffer;
Expand Down Expand Up @@ -292,7 +292,7 @@ struct comp_buffer *buffer_alloc_range(struct k_heap *heap, size_t preferred_siz
preferred_size += minimum_size - preferred_size % minimum_size;

for (size = preferred_size; size >= minimum_size; size -= minimum_size) {
stream_addr = rballoc_align(flags, size, align);
stream_addr = sof_heap_alloc(heap, flags, size, align);
if (stream_addr)
break;
}
Expand All @@ -308,7 +308,7 @@ struct comp_buffer *buffer_alloc_range(struct k_heap *heap, size_t preferred_siz
buffer = buffer_alloc_struct(heap, stream_addr, size, flags, is_shared);
if (!buffer) {
tr_err(&buffer_tr, "could not alloc buffer structure");
rfree(stream_addr);
sof_heap_free(heap, stream_addr);
}

return buffer;
Expand Down Expand Up @@ -341,14 +341,8 @@ int buffer_set_size(struct comp_buffer *buffer, uint32_t size, uint32_t alignmen
if (size == audio_stream_get_size(&buffer->stream))
return 0;

Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In buffer_set_size(), when callers pass alignment==0 (e.g. module_adapter.c and copier_ipcgtw.c do), this now forwards 0 to sof_heap_alloc(). Previously the alignment==0 path used rbrealloc() which implicitly aligned to PLATFORM_DCACHE_ALIGN. To preserve behavior and avoid potentially invalid/insufficient alignment from the underlying heap, map alignment==0 to PLATFORM_DCACHE_ALIGN (or to audio_stream_get_align()/existing stream alignment) before calling sof_heap_alloc().

Suggested change
if (!alignment)
alignment = PLATFORM_DCACHE_ALIGN;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@kv2019i kv2019i Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same semantics are now implemented by sof_heap_alloc() so 0 can be passed forward to it.

if (!alignment)
new_ptr = rbrealloc(audio_stream_get_addr(&buffer->stream),
buffer->flags | SOF_MEM_FLAG_NO_COPY,
size, audio_stream_get_size(&buffer->stream));
else
new_ptr = rbrealloc_align(audio_stream_get_addr(&buffer->stream),
buffer->flags | SOF_MEM_FLAG_NO_COPY, size,
audio_stream_get_size(&buffer->stream), alignment);
new_ptr = sof_heap_alloc(buffer->audio_buffer.heap, buffer->flags, size, alignment);

/* we couldn't allocate bigger chunk */
if (!new_ptr && size > audio_stream_get_size(&buffer->stream)) {
buf_err(buffer, "resize can't alloc %u bytes of flags 0x%x",
Expand All @@ -357,8 +351,10 @@ int buffer_set_size(struct comp_buffer *buffer, uint32_t size, uint32_t alignmen
}

/* use bigger chunk, else just use the old chunk but set smaller */
if (new_ptr)
buffer->stream.addr = new_ptr;
if (new_ptr) {
sof_heap_free(buffer->audio_buffer.heap, audio_stream_get_addr(&buffer->stream));
audio_stream_set_addr(&buffer->stream, new_ptr);
}

buffer_init_stream(buffer, size);

Expand All @@ -368,7 +364,6 @@ int buffer_set_size(struct comp_buffer *buffer, uint32_t size, uint32_t alignmen
int buffer_set_size_range(struct comp_buffer *buffer, size_t preferred_size, size_t minimum_size,
uint32_t alignment)
{
void *ptr = audio_stream_get_addr(&buffer->stream);
const size_t actual_size = audio_stream_get_size(&buffer->stream);
void *new_ptr = NULL;
size_t new_size;
Expand All @@ -389,34 +384,28 @@ int buffer_set_size_range(struct comp_buffer *buffer, size_t preferred_size, siz
if (preferred_size == actual_size)
return 0;

if (!alignment) {
for (new_size = preferred_size; new_size >= minimum_size;
new_size -= minimum_size) {
new_ptr = rbrealloc(ptr, buffer->flags | SOF_MEM_FLAG_NO_COPY,
new_size, actual_size);
if (new_ptr)
break;
}
} else {
for (new_size = preferred_size; new_size >= minimum_size;
new_size -= minimum_size) {
new_ptr = rbrealloc_align(ptr, buffer->flags | SOF_MEM_FLAG_NO_COPY,
new_size, actual_size, alignment);
if (new_ptr)
break;
}
for (new_size = preferred_size; new_size >= minimum_size;
new_size -= minimum_size) {
new_ptr = sof_heap_alloc(buffer->audio_buffer.heap, buffer->flags, new_size, alignment);
if (new_ptr)
Comment on lines +389 to +390
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In buffer_set_size_range(), alignment is forwarded directly to sof_heap_alloc(). When alignment==0, earlier code used rbrealloc() (i.e., PLATFORM_DCACHE_ALIGN via the rbrealloc wrapper). To avoid potentially allocating buffers with weaker-than-expected alignment, consider normalizing alignment==0 to PLATFORM_DCACHE_ALIGN (or the stream’s current alignment) before the allocation loop.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@kv2019i kv2019i Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sof_heap_alloc() implements these semantics for alignment==0, so no need to add a special case for PLATFORM_DCACHE_ALIGN here.

break;
}
Comment on lines +387 to 392
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer_set_size_range(): the resize loop decrements new_size until it drops below minimum_size. If all allocations fail, new_ptr stays NULL and new_size will end up < minimum_size (often 0), but the function later calls buffer_init_stream(buffer, new_size). That can set an invalid size despite the earlier validation. Consider handling the “no allocation succeeded” case explicitly: for grow requests return -ENOMEM without changing the stream; for shrink requests skip allocating a new block and just set the stream size to the chosen target (>= minimum_size).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

@kv2019i kv2019i Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in V2.


/* we couldn't allocate bigger chunk */
if (!new_ptr && new_size > actual_size) {
buf_err(buffer, "resize can't alloc %zu bytes of flags 0x%x", new_size,
buffer->flags);
return -ENOMEM;
/* If no allocation succeeded, check if old buffer is large enough */
if (!new_ptr) {
if (minimum_size > actual_size) {
buf_err(buffer, "resize can't alloc %zu bytes of flags 0x%x",
minimum_size, buffer->flags);
return -ENOMEM;
}
new_size = minimum_size;
Copy link
Copy Markdown
Contributor

@wjablon1 wjablon1 Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this step needed? Is it for the extra alignment as the comment in 380 line says?
/* Align preferred size to a multiple of the minimum size */

Copy link
Copy Markdown
Collaborator Author

@kv2019i kv2019i Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I was struggling with this one as well. As far as I understand the old code, this was a bug in the old code. If the allocation fails (so new_ptr==NULL), but the old allocation is still bigger than the last allocation attempted, the old implementation would set the buffer size to a value that is smaller than what the user asked, but also smaller than what is actually allocated.

This seems wrong, so I'm setting new size to the minimum user asked.

Other alternative would be to set the size of the actual size (allocated).

Not sure which is best (and/or least confusing).

Copy link
Copy Markdown
Contributor

@wjablon1 wjablon1 Apr 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but for the alternative you probably would need to adjust the actual size to meet the mentioned "extra alignment"... and still this is only fallback for the initial allocation failure so I would't bother... better option would be to have a proper realloc function.

}

/* use bigger chunk, else just use the old chunk but set smaller */
if (new_ptr)
buffer->stream.addr = new_ptr;
if (new_ptr) {
sof_heap_free(buffer->audio_buffer.heap, audio_stream_get_addr(&buffer->stream));
audio_stream_set_addr(&buffer->stream, new_ptr);
}

buffer_init_stream(buffer, new_size);

Expand Down
12 changes: 0 additions & 12 deletions src/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,6 @@ void rfree(void *ptr)
free(ptr);
}

void *rbrealloc_align(void *ptr, uint32_t flags, size_t bytes,
size_t old_bytes, uint32_t alignment)
{
void *newptr = aligned_alloc(alignment, bytes);

if (!newptr)
return NULL;
memcpy(newptr, ptr, bytes > old_bytes ? old_bytes : bytes);
free(ptr);
return newptr;
}

/* TODO: all mm_pm_...() routines to be implemented for IMR storage */
uint32_t mm_pm_context_size(void)
{
Expand Down
6 changes: 0 additions & 6 deletions src/platform/library/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,6 @@ void *rballoc_align(uint32_t flags, size_t bytes,
return malloc(bytes);
}

void *rbrealloc_align(void *ptr, uint32_t flags, size_t bytes,
size_t old_bytes, uint32_t alignment)
{
return realloc(ptr, bytes);
}

void *sof_heap_alloc(struct k_heap *heap, uint32_t flags, size_t bytes,
size_t alignment)
{
Expand Down
10 changes: 0 additions & 10 deletions test/cmocka/src/common_mocks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,6 @@ void WEAK *rzalloc(uint32_t flags,
return calloc(bytes, 1);
}

void WEAK *rbrealloc_align(void *ptr, uint32_t flags,
size_t bytes, size_t old_bytes, uint32_t alignment)
{
(void)flags;
(void)old_bytes;
(void)alignment;

return realloc(ptr, bytes);
}

void WEAK *rmalloc_align(uint32_t flags, size_t bytes, uint32_t alignment)
{
(void)flags;
Expand Down
23 changes: 0 additions & 23 deletions zephyr/include/rtos/alloc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,29 +88,6 @@ static inline void *rballoc(uint32_t flags, size_t bytes)
return rballoc_align(flags, bytes, PLATFORM_DCACHE_ALIGN);
}

/**
* Changes size of the memory block allocated.
* @param ptr Address of the block to resize.
* @param flags Flags, see SOF_MEM_FLAG_...
* @param bytes New size in bytes.
* @param old_bytes Old size in bytes.
* @param alignment Alignment in bytes.
* @return Pointer to the resized memory of NULL if failed.
*/
void *rbrealloc_align(void *ptr, uint32_t flags, size_t bytes,
size_t old_bytes, uint32_t alignment);

/**
* Similar to rballoc_align(), returns resized buffer aligned to
* PLATFORM_DCACHE_ALIGN.
*/
static inline void *rbrealloc(void *ptr, uint32_t flags,
size_t bytes, size_t old_bytes)
{
return rbrealloc_align(ptr, flags, bytes, old_bytes,
PLATFORM_DCACHE_ALIGN);
}

/**
* Frees the memory block.
* @param ptr Pointer to the memory block.
Expand Down
30 changes: 0 additions & 30 deletions zephyr/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,36 +493,6 @@ void *rmalloc(uint32_t flags, size_t bytes)
}
EXPORT_SYMBOL(rmalloc);

void *rbrealloc_align(void *ptr, uint32_t flags, size_t bytes,
size_t old_bytes, uint32_t alignment)
{
void *new_ptr;

if (!ptr) {
return rballoc_align(flags, bytes, alignment);
}

/* Original version returns NULL without freeing this memory */
if (!bytes) {
/* TODO: Should we call rfree(ptr); */
tr_err(&zephyr_tr, "realloc failed for 0 bytes");
return NULL;
}

new_ptr = rballoc_align(flags, bytes, alignment);
if (!new_ptr)
return NULL;

if (!(flags & SOF_MEM_FLAG_NO_COPY))
memcpy_s(new_ptr, bytes, ptr, MIN(bytes, old_bytes));

rfree(ptr);

tr_info(&zephyr_tr, "rbealloc: new ptr %p", new_ptr);

return new_ptr;
}

/**
* Similar to rmalloc(), guarantees that returned block is zeroed.
*/
Expand Down
Loading