Fast mode task config + vendor config set #8005

tobonex · 2023-08-02T20:31:02Z

First PR here, open to heavy criticism.

Adding vendor config set and fmt(fast mode task) config as first steps to fmt implementation. Fixes IPC error on fmt config due to no fmt implementation.

sofci · 2023-08-02T20:32:12Z

Can one of the admins verify this patch?

reply test this please to run this test once

gkbldcig · 2023-08-02T21:18:01Z

Can one of the admins verify this patch?

btian1

Please fix CI issues first

lgirdwood

Needs more decription in the commit messages to explain feature and changes.

src/audio/kpb.c

src/include/ipc4/module.h

src/audio/kpb.c

src/include/ipc4/kpb.h

src/include/sof/audio/kpb.h

src/include/ipc4/module.h

src/audio/kpb.c

src/ipc/ipc4/handler.c

I am out of office in ww33 and ww34. Therefore, I'm dismissing my review, I do not want to block this PR for two weeks. But I still expect my comments to be addressed.

lyakh

not a complete review, let's fix these issues first. Maybe this should be a draft PR for now

src/ipc/ipc4/handler.c

src/audio/kpb.c

lgirdwood · 2023-08-21T12:19:44Z

@lyakh update good for you now ?

kv2019i

Mostly minor style issue. I understand this is based on code with C++ background, so some of the conversion artifacts can be omitted, but I did find at least one odd case of error handling.

As for functionality, I don't know kpb well enough to really do meaningful commentary and would welcome a review round by someone more familiar with kpb.

src/ipc/ipc4/handler.c

src/audio/kpb.c

src/include/ipc4/module.h

src/ipc/ipc4/handler.c

src/include/ipc4/module.h

src/ipc/ipc4/handler.c

src/audio/kpb.c

src/include/sof/audio/kpb.h

btian1 · 2023-09-01T06:25:46Z

src/ipc/ipc4/handler.c

+	}
+	ret = drv->ops.set_large_config(dev, param_id, init_block,
+		final_block, data_off_size, (uint8_t *)(&array));
+	return ret;


return drv->ops.set_large_config(dev, param_id, init_block,
final_block, data_off_size, (uint8_t *)(&array));
ret can be removed.

Fixed this, but ret is still used elsewhere.

btian1 · 2023-09-01T06:29:46Z

src/include/ipc4/kpb.h

+	/* Mic selector for client - sets microphone id for real time sink mic selector
+	 * IPC4-compatible ID - please do not change the number
+	 */
+	KP_BUF_CLIENT_MIC_SELECT = 11,


how about 101? left more extend for this?

It is defined as 11 in reference FW. There are some IPC commands in-between those two that are not defined in sof yet.

tmleman · 2023-09-04T14:24:49Z

src/ipc/ipc4/handler.c

+			/* Align up to a multiple of 4 */
+			tlv = (struct sof_tlv *)((const uint8_t *)tlv +
+					TLV_DATA_OFFSET + ALIGN_UP(tlv->length, 4));


Why can't you use tlv_next?

this tlv_next looks convenient, but...
is does not have the ALIGN_UP. Reference FW aligned it up, so I guess that we also need it here. I do not know all of the possible IPC cases so I'm not sure if it's actually necessary or not.

src/audio/kpb.c

kv2019i · 2023-09-12T05:18:59Z

@tobonex CI is failing on build, can you check:

https://github.com/thesofproject/sof/actions/runs/6151677351/job/16692249787?pr=8005

zep_workspace/sof/src/ipc/ipc4/handler.c: In function 'ipc4_set_large_config_module_instance':
/zep_workspace/sof/src/ipc/ipc4/handler.c:1032:67: error: passing argument 8 of 'ipc4_set_vendor_config_module_instance' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]

kv2019i

A few C++'ism, but I can live with those. The CI fail needs to be fixed before merge, and I'd appreciate some comment on why FMT is needed by KPB. Otherwise looks good to go, thanks @tobonex !

src/audio/kpb.c

tobonex · 2023-09-12T10:14:19Z

Refactored some of the code so that it makes more sense:

removed struct byte_array_simple that I previously added - managed without it
removed some duplicated checks
configure_fast_mode_task function no longer accepts null in cfg argument as it made error checking messy

src/audio/kpb.c

tobonex · 2023-09-14T19:42:00Z

A few C++'ism, but I can live with those. The CI fail needs to be fixed before merge, and I'd appreciate some comment on why FMT is needed by KPB. Otherwise looks good to go, thanks @tobonex !

Originally, in case if WoV event, KPB used FMT to drain history buffer and send this data to Host ASAP. Normal tasks were too slow. We still need it for the same reason.

kv2019i · 2023-09-15T09:04:25Z

@tobonex wrote:

Originally, in case if WoV event, KPB used FMT to drain history buffer and send this data to Host ASAP. Normal tasks were too slow. We still need it for the same reason.

Ack, thanks, this is a sufficient description.
UPDATE: please add this two-sentence-explanation to the second git commit message, thanks!

Add Vendor Config Set, a special case of Large Config Set. Large Config Set handling now checks for this case and extracts extended param_id from ipc payload as param_id and handles the rest of the payload as usual. KPB now uses extended param_id. Necessary for fast mode task configuration in KPB. Signed-off-by: Tobiasz Dryjanski <[email protected]>

This patch implements handling of the configuration IPC for FMT in KPB module. KPB now saves the list of module instances to be processed by FMT. This is the first step for implementing FMT functionality. In case of WoV event, FMT is needed for KPB to drain history buffer and send this data to Host ASAP, as normal tasks are too slow. Signed-off-by: Tobiasz Dryjanski <[email protected]>

lyakh

comments have been addressed, thanks! Haven't checked what exactly changed from previous versions, but at least locations, that I commented on, look better now

lyakh · 2023-09-20T10:43:38Z

src/include/sof/audio/kpb.h

+ *		  |
+ *		fmt.device_list(device_list*)
+ *
+ *


2 empty lines above and below the description should be removed IMHO

lyakh · 2023-09-20T10:48:00Z

src/include/sof/audio/kpb.h

+ *		dev_list.device_list(comp_dev**)
+ *		  ^
+ *		  |
+ *		fmt.device_list(device_list*)


not sure what this structure means. Are these elements of the struct device_list::devs[] array above? Maybe we can find some easier (at least for me) representation. If this is an array, maybe arrange entries in lines

array[] +-------> element 1 +-------> element 2 +... +-------> element N

or something similar. The above looks to me like things pointing to each other, or calling each other, or similar

tobonex changed the title ~~Fast mode task config + vendor config task~~ Fast mode task config + vendor config set Aug 4, 2023

tobonex force-pushed the fmt branch from 4925787 to 406d1aa Compare August 6, 2023 14:39

btian1 reviewed Aug 7, 2023

View reviewed changes

tobonex force-pushed the fmt branch from 406d1aa to 3c8e931 Compare August 7, 2023 10:28

lgirdwood reviewed Aug 7, 2023

View reviewed changes

tobonex force-pushed the fmt branch 4 times, most recently from 45d2bf6 to ca3c6c3 Compare August 8, 2023 12:27

tobonex marked this pull request as ready for review August 9, 2023 08:50

tobonex requested review from fkwasowi, iganakov, RanderWang, marcinszkudlinski, pblaszko, aborisovich, plbossart, mmaka1, lbetlej and dbaluta as code owners August 9, 2023 08:50

tobonex requested review from btian1 and lgirdwood August 9, 2023 13:55

tmleman previously requested changes Aug 10, 2023

View reviewed changes

src/include/ipc4/module.h Outdated Show resolved Hide resolved

tmleman reviewed Aug 10, 2023

View reviewed changes

src/audio/kpb.c Outdated Show resolved Hide resolved

btian1 reviewed Aug 11, 2023

View reviewed changes

src/audio/kpb.c Outdated Show resolved Hide resolved

src/ipc/ipc4/handler.c Outdated Show resolved Hide resolved

tobonex force-pushed the fmt branch from ca3c6c3 to 98bc2c2 Compare August 14, 2023 10:52

lyakh requested changes Aug 14, 2023

View reviewed changes

tobonex force-pushed the fmt branch from 98bc2c2 to 405ce25 Compare August 17, 2023 12:48

kv2019i reviewed Aug 21, 2023

View reviewed changes

tobonex force-pushed the fmt branch from 405ce25 to ccb5694 Compare August 22, 2023 11:11

lgirdwood reviewed Aug 22, 2023

View reviewed changes

src/include/ipc4/module.h Outdated Show resolved Hide resolved

lyakh reviewed Aug 25, 2023

View reviewed changes

btian1 reviewed Sep 1, 2023

View reviewed changes

tmleman reviewed Sep 4, 2023

View reviewed changes

tobonex commented Sep 7, 2023

View reviewed changes

src/audio/kpb.c Outdated Show resolved Hide resolved

tobonex force-pushed the fmt branch from ccb5694 to daf2ce0 Compare September 11, 2023 20:55

kv2019i reviewed Sep 12, 2023

View reviewed changes

tobonex force-pushed the fmt branch from daf2ce0 to ec9bbba Compare September 12, 2023 09:54

lyakh reviewed Sep 12, 2023

View reviewed changes

src/audio/kpb.c Show resolved Hide resolved

src/audio/kpb.c Outdated Show resolved Hide resolved

src/audio/kpb.c Outdated Show resolved Hide resolved

src/audio/kpb.c Outdated Show resolved Hide resolved

lyakh reviewed Sep 13, 2023

View reviewed changes

src/audio/kpb.c Show resolved Hide resolved

src/audio/kpb.c Outdated Show resolved Hide resolved

src/audio/kpb.c Show resolved Hide resolved

tobonex force-pushed the fmt branch from ec9bbba to 6a0b5b5 Compare September 14, 2023 18:52

tobonex added 2 commits September 19, 2023 09:15

tobonex force-pushed the fmt branch from 6a0b5b5 to 5732019 Compare September 19, 2023 14:23

lyakh approved these changes Sep 20, 2023

View reviewed changes

kv2019i approved these changes Sep 20, 2023

View reviewed changes

kv2019i merged commit f070135 into thesofproject:main Sep 20, 2023
40 of 41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast mode task config + vendor config set #8005

Fast mode task config + vendor config set #8005

tobonex commented Aug 2, 2023

sofci commented Aug 2, 2023

gkbldcig commented Aug 2, 2023

btian1 left a comment

lgirdwood left a comment

lyakh left a comment

lgirdwood commented Aug 21, 2023

kv2019i left a comment

btian1 Sep 1, 2023

tobonex Sep 12, 2023

btian1 Sep 1, 2023

tobonex Sep 12, 2023

tmleman Sep 4, 2023

tobonex Sep 12, 2023

kv2019i commented Sep 12, 2023

kv2019i left a comment

tobonex commented Sep 12, 2023

tobonex commented Sep 14, 2023

kv2019i commented Sep 15, 2023 •

edited

Loading

lyakh left a comment

lyakh Sep 20, 2023

lyakh Sep 20, 2023

Fast mode task config + vendor config set #8005

Fast mode task config + vendor config set #8005

Conversation

tobonex commented Aug 2, 2023

sofci commented Aug 2, 2023

gkbldcig commented Aug 2, 2023

btian1 left a comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

lgirdwood commented Aug 21, 2023

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Sep 12, 2023

kv2019i left a comment

Choose a reason for hiding this comment

tobonex commented Sep 12, 2023

tobonex commented Sep 14, 2023

kv2019i commented Sep 15, 2023 • edited Loading

lyakh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Sep 15, 2023 •

edited

Loading