Skip to content

Cross core Virtual heaps, invalidation of cross core buffers in unbind call #10044

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

Merged
lgirdwood merged 10 commits into from
Jun 25, 2025
Merged

Cross core Virtual heaps, invalidation of cross core buffers in unbind call #10044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
37385b3
buf: extend module bind call API_CALL
marcinszkudlinski Jun 4, 2025
4a0a6e4
buf: extend module unbind call API_CALL
marcinszkudlinski Jun 4, 2025
e47ec84
mod: add bind/unbind hook to sink/source API
marcinszkudlinski Jun 4, 2025
d1ae448
ipc: propagate unbind event in all cases
marcinszkudlinski Jun 5, 2025
0ea4145
buf: add cache invalidation on unbind
marcinszkudlinski Jun 5, 2025
6aabe88
mem: move struct vmh_heap to c file for privacy
marcinszkudlinski Jun 16, 2025
ff22137
mem: use single cross-core virtual heap
marcinszkudlinski Jun 16, 2025
767a836
mem: add mutex protection for alloc and free
marcinszkudlinski Jun 16, 2025
f7d9991
buf: buffer free "virtual destructor"
marcinszkudlinski Jun 11, 2025
419f90c
mem: use Zephyr's macro for virtual memory regions iteration
marcinszkudlinski Jun 24, 2025
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
7 changes: 4 additions & 3 deletions src/audio/buffers/audio_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <stdint.h>
#include <stddef.h>
#include <errno.h>
#include <rtos/panic.h>
#include <rtos/alloc.h>
#include <ipc/stream.h>
#include <sof/audio/audio_buffer.h>
Expand Down Expand Up @@ -92,9 +93,8 @@ void audio_buffer_free(struct sof_audio_buffer *buffer)
audio_buffer_free(buffer->secondary_buffer_sink);
audio_buffer_free(buffer->secondary_buffer_source);
#endif /* CONFIG_PIPELINE_2_0 */
if (buffer->ops->free)
buffer->ops->free(buffer);
rfree(buffer);
/* "virtual destructor": free the buffer internals and buffer memory */
buffer->ops->free(buffer);
}

static
Expand Down Expand Up @@ -196,6 +196,7 @@ void audio_buffer_init(struct sof_audio_buffer *buffer, uint32_t buffer_type, bo
CORE_CHECK_STRUCT_INIT(&buffer, is_shared);
buffer->buffer_type = buffer_type;
buffer->ops = audio_buffer_ops;
assert(audio_buffer_ops->free);
buffer->audio_stream_params = audio_stream_params;
buffer->is_shared = is_shared;

Expand Down
1 change: 1 addition & 0 deletions src/audio/buffers/comp_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ static void comp_buffer_free(struct sof_audio_buffer *audio_buffer)
notifier_unregister_all(NULL, buffer);

rfree(buffer->stream.addr);
rfree(buffer);
}

static struct source_ops comp_buffer_source_ops = {
Expand Down
17 changes: 17 additions & 0 deletions src/audio/buffers/ring_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ static void ring_buffer_free(struct sof_audio_buffer *audio_buffer)
container_of(audio_buffer, struct ring_buffer, audio_buffer);

rfree((__sparse_force void *)ring_buffer->_data_buffer);
rfree(ring_buffer);
}

static void ring_buffer_reset(struct sof_audio_buffer *audio_buffer)
Expand Down Expand Up @@ -239,6 +240,21 @@ static int ring_buffer_release_data(struct sof_source *source, size_t free_size)
return 0;
}

int ring_buffer_module_unbind(struct sof_sink *sink)
{
struct ring_buffer *ring_buffer = ring_buffer_from_sink(sink);

CORE_CHECK_STRUCT(&ring_buffer->audio_buffer);

/* in case of disconnection, invalidate all cache. This method is guaranteed be called on
* core that have been using sink API
*/
ring_buffer_invalidate_shared(ring_buffer, ring_buffer->_data_buffer,
Copy link
Collaborator

@lyakh lyakh Jun 11, 2025

Choose a reason for hiding this comment

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

the name of this function is actually somewhat confusing: usually in SOF "shared" means "uncached"

ring_buffer->data_buffer_size);
Copy link
Collaborator

@lyakh lyakh Jun 6, 2025

Choose a reason for hiding this comment

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

would it be possible to also free the buffer here? might need to unbind it from the other side too if not done yet

Copy link
Contributor Author

@marcinszkudlinski marcinszkudlinski Jun 6, 2025

Choose a reason for hiding this comment

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

good idea. Now free is performed when a module is deleted.

OOOh! just got an idea! It has always been a memory leak!
Bind (create bufer) -> unbind (do nothng - don't delete) --> bind (create another)....

Copy link
Contributor Author

@marcinszkudlinski marcinszkudlinski Jun 6, 2025

Choose a reason for hiding this comment

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

@lyakh @kv2019i just a question - is unbind called for IPC3 also?

Copy link
Collaborator

@kv2019i kv2019i Jun 6, 2025

Choose a reason for hiding this comment

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

@marcinszkudlinski No, I don't think it's called at all. In IPC3, there's just a call to pipeline_disconnect() (for both buffers connected to a component) and this internally calls buffer_detach().

Copy link
Contributor Author

@marcinszkudlinski marcinszkudlinski Jun 12, 2025

Choose a reason for hiding this comment

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

As I checked, buffer free is called in unbind.
It wasn't till this PR because "unbind" call was ignored in most cases

	ret = comp_unbind(src, &unbind_data);

	unbind_data.bind_type = COMP_BIND_TYPE_SOURCE;
	unbind_data.source = audio_buffer_get_source(&buffer->audio_buffer);
	ret1 = comp_unbind(sink, &unbind_data);

	ll_unblock(cross_core_unbind, flags);

	buffer_free(buffer);


return 0;
}

static struct source_ops ring_buffer_source_ops = {
.get_data_available = ring_buffer_get_data_available,
.get_data = ring_buffer_get_data,
Expand All @@ -249,6 +265,7 @@ static struct sink_ops ring_buffer_sink_ops = {
.get_free_size = ring_buffer_get_free_size,
.get_buffer = ring_buffer_get_buffer,
.commit_buffer = ring_buffer_commit_buffer,
.on_unbind = ring_buffer_module_unbind,
};

static const struct audio_buffer_ops audio_buffer_ops = {
Expand Down
8 changes: 4 additions & 4 deletions src/audio/copier/copier.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,9 +1173,9 @@ __cold static int copier_get_hw_params(struct comp_dev *dev, struct sof_ipc_stre
return dai_common_get_hw_params(dd, dev, params, dir);
}

__cold static int copier_bind(struct processing_module *mod, void *data)
__cold static int copier_bind(struct processing_module *mod, struct bind_info *bind_data)
{
const struct ipc4_module_bind_unbind *const bu = (struct ipc4_module_bind_unbind *)data;
const struct ipc4_module_bind_unbind *const bu = bind_data->ipc4_data;
const uint32_t src_id = IPC4_COMP_ID(bu->primary.r.module_id, bu->primary.r.instance_id);
const uint32_t src_queue_id = bu->extension.r.src_queue;
struct copier_data *cd = module_get_private_data(mod);
Expand All @@ -1202,7 +1202,7 @@ __cold static int copier_bind(struct processing_module *mod, void *data)
return -ENODEV;
Copy link
Collaborator

@kv2019i kv2019i Jun 6, 2025

Choose a reason for hiding this comment

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

Btw, Something odd with all the commit messages. They are line-wrapped to under 50 columns...? Look a bit odd in git history compared to other commits in SOF.

}

__cold static int copier_unbind(struct processing_module *mod, void *data)
__cold static int copier_unbind(struct processing_module *mod, struct bind_info *unbind_data)
{
struct copier_data *cd = module_get_private_data(mod);
struct comp_dev *dev = mod->dev;
Expand All @@ -1212,7 +1212,7 @@ __cold static int copier_unbind(struct processing_module *mod, void *data)
if (dev->ipc_config.type == SOF_COMP_DAI) {
struct dai_data *dd = cd->dd[0];

return dai_zephyr_unbind(dd, dev, data);
return dai_zephyr_unbind(dd, dev, unbind_data);
}

return 0;
Expand Down
2 changes: 1 addition & 1 deletion src/audio/copier/dai_copier.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ static inline int dai_zephyr_unbind(struct dai_data *dd, struct comp_dev *dev, v
int dai_zephyr_multi_endpoint_copy(struct dai_data **dd, struct comp_dev *dev,
struct comp_buffer *multi_endpoint_buffer,
int num_endpoints);
int dai_zephyr_unbind(struct dai_data *dd, struct comp_dev *dev, void *data);
int dai_zephyr_unbind(struct dai_data *dd, struct comp_dev *dev, struct bind_info *unbind_data);
#endif


Expand Down
5 changes: 3 additions & 2 deletions src/audio/dai-zephyr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1961,14 +1961,15 @@ static uint64_t dai_get_processed_data(struct comp_dev *dev, uint32_t stream_no,
}

#ifdef CONFIG_IPC_MAJOR_4
__cold int dai_zephyr_unbind(struct dai_data *dd, struct comp_dev *dev, void *data)
__cold
int dai_zephyr_unbind(struct dai_data *dd, struct comp_dev *dev, struct bind_info *unbind_data)
{
struct ipc4_module_bind_unbind *bu;
int buf_id;

assert_can_be_cold();

bu = (struct ipc4_module_bind_unbind *)data;
bu = unbind_data->ipc4_data;
buf_id = IPC4_COMP_ID(bu->extension.r.src_queue, bu->extension.r.dst_queue);

if (dd && dd->local_buffer) {
Expand Down
10 changes: 5 additions & 5 deletions src/audio/kpb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,10 @@ static int kpb_get_attribute(struct comp_dev *dev,
/**
* \brief Initialize KPB sinks when binding.
* \param[in] dev - component device pointer.
* \param[in] data - ipc4 bind/unbind data.
* \param[in] bind_data - bind/unbind data.
* \return: none.
*/
static int kpb_bind(struct comp_dev *dev, void *data)
static int kpb_bind(struct comp_dev *dev, struct bind_info *bind_data)
{
struct comp_data *kpb = comp_get_drvdata(dev);
struct ipc4_module_bind_unbind *bu;
Expand All @@ -345,7 +345,7 @@ static int kpb_bind(struct comp_dev *dev, void *data)

comp_dbg(dev, "kpb_bind()");

bu = (struct ipc4_module_bind_unbind *)data;
bu = bind_data->ipc4_data;
buf_id = IPC4_COMP_ID(bu->extension.r.src_queue, bu->extension.r.dst_queue);

/* We're assuming here that KPB Real Time sink (kpb->sel_sink) is
Expand Down Expand Up @@ -383,15 +383,15 @@ static int kpb_bind(struct comp_dev *dev, void *data)
* \param[in] data - ipc4 bind/unbind data.
* \return: none.
*/
static int kpb_unbind(struct comp_dev *dev, void *data)
static int kpb_unbind(struct comp_dev *dev, struct bind_info *unbind_data)
{
struct comp_data *kpb = comp_get_drvdata(dev);
struct ipc4_module_bind_unbind *bu;
int buf_id;

comp_dbg(dev, "kpb_bind()");

bu = (struct ipc4_module_bind_unbind *)data;
bu = unbind_data->ipc4_data;
buf_id = IPC4_COMP_ID(bu->extension.r.src_queue, bu->extension.r.dst_queue);

/* Reset sinks when unbinding */
Expand Down
10 changes: 4 additions & 6 deletions src/audio/mixin_mixout/mixin_mixout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,17 +784,15 @@ static int mixout_prepare(struct processing_module *mod,
return 0;
}

static int mixout_bind(struct processing_module *mod, void *data)
static int mixout_bind(struct processing_module *mod, struct bind_info *bind_data)
{
struct ipc4_module_bind_unbind *bu;
struct comp_dev *mixin;
struct pending_frames *pending_frames;
int src_id;
struct mixout_data *mixout_data;

comp_dbg(mod->dev, "mixout_bind() %p", data);

bu = (struct ipc4_module_bind_unbind *)data;
bu = bind_data->ipc4_data;
src_id = IPC4_COMP_ID(bu->primary.r.module_id, bu->primary.r.instance_id);

/* we are only interested in bind for mixin -> mixout pair */
Expand Down Expand Up @@ -833,7 +831,7 @@ static int mixout_bind(struct processing_module *mod, void *data)
return 0;
}

static int mixout_unbind(struct processing_module *mod, void *data)
static int mixout_unbind(struct processing_module *mod, struct bind_info *unbind_data)
{
struct ipc4_module_bind_unbind *bu;
struct comp_dev *mixin;
Expand All @@ -843,7 +841,7 @@ static int mixout_unbind(struct processing_module *mod, void *data)

comp_dbg(mod->dev, "mixout_unbind()");

bu = (struct ipc4_module_bind_unbind *)data;
bu = unbind_data->ipc4_data;
src_id = IPC4_COMP_ID(bu->primary.r.module_id, bu->primary.r.instance_id);

/* we are only interested in unbind for mixin -> mixout pair */
Expand Down
42 changes: 36 additions & 6 deletions src/audio/module_adapter/module/generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,22 +493,52 @@ int module_set_configuration(struct processing_module *mod,
}
EXPORT_SYMBOL(module_set_configuration);

int module_bind(struct processing_module *mod, void *data)
int module_bind(struct processing_module *mod, struct bind_info *bind_data)
{
int ret;
const struct module_interface *const ops = mod->dev->drv->adapter_ops;

switch (bind_data->bind_type) {
case COMP_BIND_TYPE_SINK:
ret = sink_bind(bind_data->sink, mod);
break;
case COMP_BIND_TYPE_SOURCE:
ret = source_bind(bind_data->source, mod);
break;
default:
ret = -EINVAL;
}
if (ret)
return ret;

if (ops->bind)
return ops->bind(mod, data);
return 0;
ret = ops->bind(mod, bind_data);

return ret;
}

int module_unbind(struct processing_module *mod, void *data)
int module_unbind(struct processing_module *mod, struct bind_info *unbind_data)
{
int ret;
const struct module_interface *const ops = mod->dev->drv->adapter_ops;

switch (unbind_data->bind_type) {
case COMP_BIND_TYPE_SINK:
ret = sink_unbind(unbind_data->sink);
break;
case COMP_BIND_TYPE_SOURCE:
ret = source_unbind(unbind_data->source);
break;
default:
ret = -EINVAL;
}
if (ret)
return ret;

if (ops->unbind)
return ops->unbind(mod, data);
return 0;
ret = ops->unbind(mod, unbind_data);

return ret;
}

void module_update_buffer_position(struct input_stream_buffer *input_buffers,
Expand Down
8 changes: 4 additions & 4 deletions src/audio/module_adapter/module_adapter_ipc4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,12 +242,12 @@ static bool module_adapter_multi_sink_source_prepare(struct comp_dev *dev)
return false;
}

int module_adapter_bind(struct comp_dev *dev, void *data)
int module_adapter_bind(struct comp_dev *dev, struct bind_info *bind_data)
{
struct processing_module *mod = comp_mod(dev);
int ret;

ret = module_bind(mod, data);
ret = module_bind(mod, bind_data);
if (ret < 0)
return ret;

Expand All @@ -257,12 +257,12 @@ int module_adapter_bind(struct comp_dev *dev, void *data)
}
EXPORT_SYMBOL(module_adapter_bind);

int module_adapter_unbind(struct comp_dev *dev, void *data)
int module_adapter_unbind(struct comp_dev *dev, struct bind_info *unbind_data)
{
struct processing_module *mod = comp_mod(dev);
int ret;

ret = module_unbind(mod, data);
ret = module_unbind(mod, unbind_data);
if (ret < 0)
return ret;

Expand Down
8 changes: 4 additions & 4 deletions src/debug/tester/tester.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,24 +188,24 @@ static int tester_free(struct processing_module *mod)
return ret;
}

static int tester_bind(struct processing_module *mod, void *data)
static int tester_bind(struct processing_module *mod, struct bind_info *bind_data)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->bind)
ret = cd->tester_case_interface->bind(cd->test_case_ctx, mod, data);
ret = cd->tester_case_interface->bind(cd->test_case_ctx, mod, bind_data);

return ret;
}

static int tester_unbind(struct processing_module *mod, void *data)
static int tester_unbind(struct processing_module *mod, struct bind_info *unbind_data)
{
struct tester_module_data *cd = module_get_private_data(mod);
int ret = 0;

if (cd->tester_case_interface->unbind)
ret = cd->tester_case_interface->unbind(cd->test_case_ctx, mod, data);
ret = cd->tester_case_interface->unbind(cd->test_case_ctx, mod, unbind_data);

return ret;
}
Expand Down
4 changes: 2 additions & 2 deletions src/debug/tester/tester.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ struct tester_test_case_interface {
/**
* copy of module bind method, with additional ctx param
*/
int (*bind)(void *ctx, struct processing_module *mod, void *data);
int (*bind)(void *ctx, struct processing_module *mod, struct bind_info *bind_data);

/**
* copy of module unbind method, with additional ctx param
*/
int (*unbind)(void *ctx, struct processing_module *mod, void *data);
int (*unbind)(void *ctx, struct processing_module *mod, struct bind_info *unbind_data);

/**
* copy of module trigger method, with additional ctx param
Expand Down
8 changes: 4 additions & 4 deletions src/idc/idc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,14 @@ static int idc_ipc4_bind(uint32_t comp_id)
{
struct ipc_comp_dev *ipc_dev;
struct idc_payload *payload;
struct ipc4_module_bind_unbind *bu;
struct bind_info *bu;

ipc_dev = ipc_get_comp_by_id(ipc_get(), comp_id);
if (!ipc_dev)
return -ENODEV;

payload = idc_payload_get(*idc_get(), cpu_get_id());
bu = (struct ipc4_module_bind_unbind *)payload;
bu = (struct bind_info *)payload;

return comp_bind(ipc_dev->cd, bu);
}
Expand All @@ -102,14 +102,14 @@ static int idc_ipc4_unbind(uint32_t comp_id)
{
struct ipc_comp_dev *ipc_dev;
struct idc_payload *payload;
struct ipc4_module_bind_unbind *bu;
struct bind_info *bu;

ipc_dev = ipc_get_comp_by_id(ipc_get(), comp_id);
if (!ipc_dev)
return -ENODEV;

payload = idc_payload_get(*idc_get(), cpu_get_id());
bu = (struct ipc4_module_bind_unbind *)payload;
bu = (struct bind_info *)payload;

return comp_unbind(ipc_dev->cd, bu);
}
Expand Down
Loading
Loading