Skip to content

samples: convert smart amp test module to not expect the blob during init with IPC4 - #7042

Merged
lgirdwood merged 1 commit into
thesofproject:mainfrom
ujfalusi:peter/pr/test_module_updates_no_blob_at_init_01
Feb 8, 2023
Merged

samples: convert smart amp test module to not expect the blob during init with IPC4#7042
lgirdwood merged 1 commit into
thesofproject:mainfrom
ujfalusi:peter/pr/test_module_updates_no_blob_at_init_01

Conversation

@ujfalusi

@ujfalusi ujfalusi commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

Hi,

the module initialization for IPC4 does not contain binary (configuration) blob, only base_cfg and optional bae_cfg_ext.
The configuration blob is sent as a large_config message separately.

@kv2019i
kv2019i requested a review from libinyang February 6, 2023 09:50
@lgirdwood

Copy link
Copy Markdown
Member

@wszypelt @lrudyX good to go here ? Dont think this flow is tested in CI yet ?

@wszypelt

wszypelt commented Feb 6, 2023

Copy link
Copy Markdown

@lgirdwood 3 tests on KD was failed, I think it's not good to merge

@ujfalusi

ujfalusi commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

@lgirdwood 3 tests on KD was failed, I think it's not good to merge

To be fair, most of the PRs are failing in a same way, if I read the logs right, no?

@wszypelt

wszypelt commented Feb 6, 2023

Copy link
Copy Markdown

@ujfalusi a lot of PR has a problem with one test and right now I'm trying to understand why, but here the whole module fails.
so I'm not sure if it's the same problem as others ;/

@wszypelt

wszypelt commented Feb 6, 2023

Copy link
Copy Markdown

@ujfalusi @lgirdwood I'm taking FW and will check these tests on another machine, in a moment I will write how it went.

@ujfalusi

ujfalusi commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

@ujfalusi @lgirdwood I'm taking FW and will check these tests on another machine, in a moment I will write how it went.

Right, so, with this patch the configuration blob is not expected within MOD_INIT_INSTANCE message, a separate LARGE_CONFIG should be use to send the configuration (param_id = 2) and the blob itself should be the exact same payload as it was with IPC3, So in the abi header you need to change the magic to 0x34464F53, set they type to 2 and send that with Linux.

@ujfalusi

ujfalusi commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

I was not aware that we had any tests for detect_test at all, like we did not have for the smart_amp_test neither.

@mwasko

mwasko commented Feb 6, 2023

Copy link
Copy Markdown
Contributor

@iganakov please review proposed changes

@mwasko
mwasko requested a review from iganakov February 6, 2023 15:18
@wszypelt

wszypelt commented Feb 6, 2023

Copy link
Copy Markdown

@ujfalusi @lgirdwood
As for the current tests
Unfortunately, all three KD tests fail on this PR. I checked on the same settings, compared it to FW from main. those tests pass there, I also confirmed that the 16bit16bit test is unstable, so I will exclude it from the scope and rerun other PR. The other two KD tests remain.

@mszleszy

mszleszy commented Feb 6, 2023

Copy link
Copy Markdown

@lgirdwood
I'll update tests asap but I need to clarify few things.
KdCfgBlobHeader is being removed from init instance message and it should be send in separate LargeConfigSet.
Should I use IPC4_DETECT_TEST_SET_CONFIG param?
If so the message will contain base config + KdCfgBlobHeader - do we want to keep it this way, change IPC4_DETECT_TEST_SET_CONFIG requirements or use different kd param ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ipc4 repeated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting it, I will use sof_smart_amp_ipc4_config

@iganakov iganakov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me except unused field load_memory_size in sof_detect_test_config. Probably, should be removed.

Comment thread src/samples/audio/smart_amp_test.c Outdated
Comment on lines 88 to 89

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have the macros defined, use them here?

#define SMART_AMP_NUM_IN_PINS		2
#define SMART_AMP_NUM_OUT_PINS		1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, this is why I added the defines ;)

@aiChaoSONG

Copy link
Copy Markdown
Collaborator

The smart amp change in this PR is verified with #6996 and thesofproject/linux#4160, need more time to test the WoV change.

@ujfalusi

ujfalusi commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

I will drop the detect_test change from the PR for now and open a new one for it to get the smart amp feature enabled faster.

During the component create (module init) we are not receiving the blob
containing the sof_smart_amp_config, only the base_cfg and base_cfg_ext is
received.
The sof_smart_amp_config content will be sent via large_config_set at a
later time.

Introduce a new IPC4 only struct to store the base and pin configurations
and handle the IPC3 / IPC4 init differently.

The blob received via large_config_set must be the same format as the blob
with IPC3.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
@ujfalusi
ujfalusi force-pushed the peter/pr/test_module_updates_no_blob_at_init_01 branch from 9ce454b to 9adad6f Compare February 7, 2023 08:01
@ujfalusi

ujfalusi commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

Changes since v1:

  • drop the detect_test patch
  • use the defines in place for the number of pins number
  • remove duplicated 'ipc4' from the IPC4 config struct name

@ujfalusi

ujfalusi commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

@lgirdwood I'll update tests asap but I need to clarify few things. KdCfgBlobHeader is being removed from init instance message and it should be send in separate LargeConfigSet. Should I use IPC4_DETECT_TEST_SET_CONFIG param? If so the message will contain base config + KdCfgBlobHeader - do we want to keep it this way, change IPC4_DETECT_TEST_SET_CONFIG requirements or use different kd param ?

@mszleszy, the detect_test at init will receive the struct ipc4_base_module_cfg only. The configuration is going to arrive via set_large_config in a form of (an unmodified, IPC3 compatible) struct sof_detect_test_config and the param ID for this config_large is IPC4_DETECT_TEST_SET_CONFIG (2).

Fwiw, this is how the blob will look like in topology:

Object.Base.data."detect_test_init" {
	bytes "
	0x53,0x4f,0x46,0x34,0x02,0x00,0x00,0x00,	# magic + param_id
	0x20,0x00,0x00,0x00,0x00,0x00,0x00,0x00,	# size + abi version
	0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,	# unused
	0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,	# unused 

	0x20,0x00,0x00,0x00,0x00,0x00,0x00,0x00,	# the blob itself (struct sof_detect_test_config)
	0x00,0x00,0x00,0x00,0x34,0x08,0x00,0x00,
	0x03,0x00,0x18,0x00,0x00,0x00,0x00,0x00,
	0xD0,0x07,0x00,0x00,0x00,0x00,0x00,0x00"
)

@ujfalusi ujfalusi changed the title samples: convert smart amp and wov test modules to not expect the blob during init with IPC4 samples: convert smart amp test module to not expect the blob during init with IPC4 Feb 7, 2023
@ujfalusi

ujfalusi commented Feb 7, 2023

Copy link
Copy Markdown
Contributor Author

Looks good to me except unused field load_memory_size in sof_detect_test_config. Probably, should be removed.

@iganakov, I have intentionally kept the original struct sof_detect_test_config intact in order to be able to re-use the same blob from IPC3 in IPC4 with only needing to update the sof_abi_hdr in topology and in user space.
We can not drop members from the struct from the middle without breaking existing blobs.

@lgirdwood

Copy link
Copy Markdown
Member

@iganakov @softwarecki updates OK for you ?

@lgirdwood
lgirdwood merged commit bcc1407 into thesofproject:main Feb 8, 2023
@ujfalusi
ujfalusi deleted the peter/pr/test_module_updates_no_blob_at_init_01 branch August 17, 2023 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.