[WIP][RFC]codec adapter: move generic mem alloc to component by juimonen · Pull Request #4112 · thesofproject/sof

juimonen · 2021-04-28T18:03:14Z

Codec adapter has a devm_kzalloc type memory allocator to allocate
multiple chunks of memory into a list, which can be then freed without
explicit pointers in codec deletion. Move this generic mechanism to
component class.

Signed-off-by: Jaska Uimonen jaska.uimonen@intel.com

Add support to the IPC pipeline logic to support creation of pipelines using generic IPC ABI. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Add support to allocate memory at the pipeline level so that the all its processing components can use the same resource. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

WIP. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Port latest change to ipc4-handler Signed-off-by: Rander Wang <rander.wang@intel.com>

Signed-off-by: Rander Wang <rander.wang@intel.com>

Ipc3 only uses DIPCIDR to send dsp message, but ipc4 uses both DIPCIDR and DIPCIDD. Now get DIPCIDD data from msg tx_data. Signed-off-by: Rander Wang <rander.wang@intel.com>

Fw creates a dsp ipc message to send status of the last host ipc message. Signed-off-by: Rander Wang <rander.wang@intel.com>

Signed-off-by: Rander Wang <rander.wang@intel.com>

Add alh header file for ipc4. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Codec adapter has a devm_kzalloc type memory allocator to allocate multiple chunks of memory into a list, which can be then freed without explicit pointers in codec deletion. Move this generic mechanism to component class. Signed-off-by: Jaska Uimonen <jaska.uimonen@intel.com>

juimonen · 2021-04-28T18:04:55Z

@lgirdwood heavily simplified... I don't know do we need at all the step by step free of memory individually or can we just always free all memory... usually if memory allocation fails the whole component is in very bad state. I can bring back the more refined method if needed.

dbaluta · 2021-04-28T18:11:30Z


-	cd = codec_allocate_memory(dev, sizeof(struct cadence_codec_data), 0);
+	cd = comp_devm_alloc(dev, sizeof(struct cadence_codec_data), 0);
 	if (!cd) {


I wonder if this shouldn't be better called devm_comp_alloc(dev, ..) similar with its Linux counterpart.

yes sure. Can revisit the naming as I need to shuffle this around anyway...

lgirdwood

Thanks @juimonen. I would make two changes here.

Move it to the allocator logic. i.e. all memory logic in one place.
Associate it with the pipeline rather than a component. We only remove componets when we delete the pipeline and we now support dynamic pipeline so make more sense at this level.

lgirdwood · 2021-04-28T19:24:58Z

@cujomalainey fyi

lgirdwood · 2021-04-28T19:25:29Z

@juimonen this should go into main branch. There are no dependencies.

cujomalainey · 2021-04-28T23:59:18Z

@cujomalainey fyi

interesting, codeowners does not recurse into directories with * thanks for the FYI

cujomalainey · 2021-04-29T00:01:45Z

Thanks for moving this, its an excellent piece of code that just needs to benefit everyone 😃

juimonen · 2021-04-29T12:15:27Z

Thanks @juimonen. I would make two changes here.
Move it to the allocator logic. i.e. all memory logic in one place.
Associate it with the pipeline rather than a component. We only remove componets when we delete the pipeline and we now support dynamic pipeline so make more sense at this level.

ok
I'm not sure I can do that as our driver creates the pipeline component after the components themselves have been created -> I don't necessarily have a pipeline where to store the memory...

lgirdwood · 2021-04-29T13:34:36Z

Thanks @juimonen. I would make two changes here.
Move it to the allocator logic. i.e. all memory logic in one place.
Associate it with the pipeline rather than a component. We only remove componets when we delete the pipeline and we now support dynamic pipeline so make more sense at this level.
ok
I'm not sure I can do that as our driver creates the pipeline component after the components themselves have been created -> I don't necessarily have a pipeline where to store the memory...

@ranj063 is doing some work here #4113, we will need this and a kernel patch to change the pipeline new order.

So maybe part 1 is to align the allocations per module/components, and once kernel is ready we change from module to pipeline alignment, but lets get feedback from @ranj063 first wrt kernel timeline.

@RDharageswari fyi.

cujomalainey · 2021-05-03T21:56:11Z

+ */
+struct comp_dev_mem {
+	void *ptr; /**< A pointr to particular memory block */
+	struct list_item mem_list; /**< list of memory allocated by codec */


@lgirdwood would you have an issue if I built in a feature (behind a config flag) here to track amount allocated by each alloc? That way we call a function and query memory consumed by a component (and possibly identify what section each pointer is point to)

@cujomalainey sound good to me.

gkbldcig · 2021-05-10T03:22:16Z

Can one of the admins verify this patch?

cujomalainey · 2021-06-09T20:13:36Z

ping on this patch, this is something i would like to base a feature off of

lgirdwood · 2021-06-10T10:54:52Z

@juimonen whats' the ETA for this patch ? What's still to do ?

lyakh

It still seems to me, that this isn't a good use of "devm." It's a clear reference to the equally named Linux kernel API, but it doesn't do the same thing. If the automated resource management is planned, we should make sure it gets implemented soon enough, not to leave this namespace confusion for indefinite time...

lyakh · 2021-06-11T06:33:13Z

+	list_for_item_safe(mem_list, _mem_list, &dev->mem.mem_list) {
+ mem = container_of(mem_list, struct comp_dev_mem, mem_list);
+ rfree(mem->ptr);
+ list_item_del(&mem->mem_list);


maybe first delete from the list, then free?

juimonen · 2021-06-11T06:47:04Z

let me work on this next week, was busy this week with other stuff.

kv2019i · 2022-04-20T15:39:41Z

@juimonen Over a year without activity, should this be closed?

lrgirdwoand others added 13 commits April 27, 2021 15:07

ipc: abstraction: abstract pipeline_new() to support other IPC ABIs
6571c72
Add support to the IPC pipeline logic to support creation of pipelines using generic IPC ABI. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

pipeline: add support for pipeline to allocate memory
579fcbb
Add support to allocate memory at the pipeline level so that the all its processing components can use the same resource. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

ipc4: wip
04a82ee
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

ipc: abstraction: Add yet more IPC structures
381f005
WIP. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

ipc4: fix compiling error
ed05c68
Port latest change to ipc4-handler Signed-off-by: Rander Wang <rander.wang@intel.com>

ipc: add fw ready support for ipc4
c45a83c
Signed-off-by: Rander Wang <rander.wang@intel.com>

ipc4: process ipc message for ipc4
1cf4bc3
Ipc3 only uses DIPCIDR to send dsp message, but ipc4 uses both DIPCIDR and DIPCIDD. Now get DIPCIDD data from msg tx_data. Signed-off-by: Rander Wang <rander.wang@intel.com>

ipc4: reply ipc message status to host
70772b4
Fw creates a dsp ipc message to send status of the last host ipc message. Signed-off-by: Rander Wang <rander.wang@intel.com>

ipc4: add dmic & ssp header file for ipc4
337a1a8
Signed-off-by: Rander Wang <rander.wang@intel.com>

ipc4: add alh header file for ipc4
04779a1
Add alh header file for ipc4. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

ipc: abstraction: ipc4 header cleanup
2f9a09a
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

ipc: abstraction: ipc4 use gateway
c71da8f
Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

juimonen requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners April 28, 2021 18:03

dbaluta reviewed Apr 28, 2021
View reviewed changes

lgirdwood requested changes Apr 28, 2021
View reviewed changes

cujomalainey reviewed May 3, 2021
View reviewed changes

lgirdwood force-pushed the lrg/topic/ipc4 branch from c811070 to 6c66268Compare May 12, 2021 16:02

juimonen requested review from bardliao and ranj063 May 12, 2021 16:02

lgirdwood force-pushed the lrg/topic/ipc4 branch 3 times, most recently from 78c3229 to 1ad177dCompare May 17, 2021 16:13

lgirdwood force-pushed the lrg/topic/ipc4 branch 5 times, most recently from 7ff3c3f to b91ed92Compare June 2, 2021 10:50

lgirdwood force-pushed the lrg/topic/ipc4 branch 2 times, most recently from e7a3d6a to 81bcfe5Compare June 9, 2021 19:33

lgirdwood force-pushed the lrg/topic/ipc4 branch from 7d2075b to 022ea8cCompare June 10, 2021 14:57

lyakh reviewed Jun 11, 2021
View reviewed changes

lgirdwood force-pushed the lrg/topic/ipc4 branch from 25412d0 to 0393ee6Compare June 11, 2021 17:18

lgirdwood closed this Apr 20, 2022

Uh oh!

Conversation

juimonen commented Apr 28, 2021

Uh oh!

juimonen commented Apr 28, 2021

Uh oh!

dbalutaApr 28, 2021

Choose a reason for hiding this comment

Uh oh!

juimonenApr 29, 2021

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Apr 28, 2021

Uh oh!

lgirdwood commented Apr 28, 2021

Uh oh!

cujomalainey commented Apr 28, 2021

Uh oh!

cujomalainey commented Apr 29, 2021

Uh oh!

juimonen commented Apr 29, 2021

Uh oh!

lgirdwood commented Apr 29, 2021

Uh oh!

cujomalaineyMay 3, 2021

Choose a reason for hiding this comment

Uh oh!

lgirdwoodMay 10, 2021

Choose a reason for hiding this comment

Uh oh!

gkbldcig commented May 10, 2021

Uh oh!

cujomalainey commented Jun 9, 2021

Uh oh!

lgirdwood commented Jun 10, 2021

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

lyakhJun 11, 2021

Choose a reason for hiding this comment

Uh oh!

juimonen commented Jun 11, 2021

Uh oh!

kv2019i commented Apr 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants