don't need to unbind sink here? And in the potential error case below? Same in module_unbind()

no, sink/src API is specific to a single component only. "Bind" for sink/source means a connection to a single component and nothing more.
Bind/unbind will be called separately for both components in question, each call on proper core in case of cross core connection. So will be bind/unbind for sink/src APIs
As to error patch: it will NEVER be situation where as_source != NULL && as_sink!=NULL

maybe it will be better to do this in more stright way:

struct bind_info {
  struct ipc4_module_bind_unbind *ipc4_data;
  bool is_source; // will think about better name
  union  {
     struct sof_source *source;
     struct sof_sink *sink;
  }
}

what do you think?

@marcinszkudlinski aha, so as_source and as_sink are mutually exclusive, they cannot both be non-NULL at the same time? Then yes, would be good to make that explicit, at least with an else. And yes, using a union would be another possibility too.

api changed

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

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)....

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

@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().

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);

hm, wasn't the actual meaning of CORE_HEAP that it was per core? Maybe it isn't needed any more?

obsolete name - used to be per heap, now is global, Will change

Changed to MEM_REG_ATTR_SHARED_HEAP

MEM_REG_ATTR_* are defined in Zephyr. Zephyr creates one virtual memory space per each core, one as "shared", one for other purposes (MEM_REG_ATTR_OPPORTUNISTIC_MEMORY ??).
Anyway,

• all spaces are pure virtual and don't use any physical memory banks till memory is allocated, so they can stay in Zephyr.
• In Zephyr all regions are treated in the very same way - in fact no difference at all which region is used
• SOF used to use 3 "per core" areas, now is using one "shared"
• "shared" size is 0x100000 == 1MB, and it is hard-coded in zephyr (should be enough)

maybe it is worth to introduce some kconfig options enabling regions and adjusting sizes, but it requires a PR to Zephyr.

but here in line 239/238 num_regions still counts all cores, which then becomes the iteration count for this loop?

@marcinszkudlinski this seems to still be open - this shouldn't be called in a loop any more, should it?

why not? All of virtual memory tables are still in Zephyr, This test uses all of them and this is fine.
There's no kconfig option in zephyr that turns off any of the virtual memory

@marcinszkudlinski how will it use all of them now, that you've modified vmh_init_heap() to only use heap 0 for all core heaps?

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.

@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().

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

looks like this would break with zephyrproject-rtos/zephyr#91666 . I think we need a Zephyr function to find a region by the attribute.

true! CONFIG_MP_MAX_NUM_CPUS + VIRTUAL_REGION_COUNT as a table size - instead of a single macro - should have never been merged :(

I think we can proceed with Zephyr changes

• I'll add a function finding a region by attribute and core num to zephyr.
• I'll create a PR to SOF using the zephyr function
• PR must be merged together with moving to new zephyr head

sys_mm_drv_query_memory_regions is a standard zephyr function. The API assumes that at the end the array of regions should be freed via sys_mm_drv_query_memory_regions_free. The end of the array is marked with an empty element, so the loop doesn't have to rely on knowledge of its size.

@marcinszkudlinski do you want to do the Zephyr changes incrementally, i.e. we merge this and give it a a few days under CI testing ?

this PR should go first, it works with current zephyr

than, zephyr change should be merged
as the last step - move SOF to new zephyr + changes in SOF (in one PR)