Skip to content

Conversation

@aiChaoSONG
Copy link
Collaborator

@aiChaoSONG aiChaoSONG commented Feb 8, 2023

This patch converts the selector module to use base config extension for module init, which ensures only base_cfg and base_cfg_ext is received in module init, and align the init blob format with other modules.

Signed-off-by: Chao Song [email protected]

The selector module is using the module adapter interface, and it has one input and one output, the input format(base_cfg) is already handled with module adapter, we only need to handle the output format.
@ranj063 I suppose that the nb_input_pins/nb_output_pins in base_cfg_ext could be zero, this help us to save some memory. If it is not the case, please let me know, I will also add the input pin format in comp_data (through the input format will never be used in the fw).

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@ujfalusi @ranj063 pls review

Copy link
Contributor

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

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

@aiChaoSONG, looks good!

@ujfalusi ujfalusi self-requested a review February 9, 2023 10:10
@lgirdwood
Copy link
Member

@ujfalusi good now ?

@aiChaoSONG
Copy link
Collaborator Author

Let's hold on merge until Libin help to verify the patch, the rimage patch is needed for the verification: thesofproject/rimage#134

@libinyang
Copy link
Contributor

Let's hold on merge until Libin help to verify the patch, the rimage patch is needed for the verification: thesofproject/rimage#134

After enabling the base config extension, WoV doesn't work. But it is not this PR's issue. Selector is initialized successfully after enabling base config extension.

@aiChaoSONG
Copy link
Collaborator Author

I am able to test wov feature with this PR + thesofproject/linux#4185 + thesofproject/rimage#135 + #7106 without any hack.

@ranj063 @ujfalusi ping

Copy link
Contributor

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

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

@aiChaoSONG, looks good to my eyes, thank you.

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Looks good, no need to update for the whitespace issue if tests pass and other reviews are ok.

@ranj063
Copy link
Collaborator

ranj063 commented Feb 17, 2023

@aiChaoSONG the internal CI tests are failing. Can you please check?

@aiChaoSONG
Copy link
Collaborator Author

@ranj063 Looks like the quickbuild CI issue, let me retrigger

11:37:09,286 ERROR - Step 'master>SCANS>TESTS>TRIGGER PYTHON SMOKE_IPC4 TESTS' is failed: Step is failed since the triggered build is failed, cancelled, or timed out.

@aiChaoSONG
Copy link
Collaborator Author

SOFCI TEST

@aiChaoSONG
Copy link
Collaborator Author

@wszypelt Internal CI trigger Python IPC4 smoke test failed, could you please help to take a look.

@aiChaoSONG
Copy link
Collaborator Author

Update: rebased on the latest main

@kv2019i
Copy link
Collaborator

kv2019i commented Feb 20, 2023

test_003_01_kd_topology_host_gw test is failing. @aiChaoSONG @wszypelt is this related to PR or unrelated failure?

@aiChaoSONG
Copy link
Collaborator Author

test_003_01_kd_topology_host_gw test is failing. @aiChaoSONG @wszypelt is this related to PR or unrelated failure?

@kv2019i @wszypelt selector module is use in the wov topology, this patch changed the payload for selector, I am afraid the test case needs to be modified accordingly.

@wszypelt
Copy link

@aiChaoSONG @kv2019i I'll take a look at it, but recently the KD tests have been changed to meet the new requirements

@lgirdwood
Copy link
Member

SOFCI TEST

@lgirdwood
Copy link
Member

@aiChaoSONG pls check internal CI if failure persists.

@lgirdwood
Copy link
Member

@aiChaoSONG ok, failed again, can you check internal CI.

@aiChaoSONG
Copy link
Collaborator Author

aiChaoSONG commented Feb 27, 2023

@wszypelt I have changed the payload for selector in this patch, but quickbuild python tests still construct selector payload in the old way, so I think we need to modify the test case accordingly. Could you or someone please help to do this?
Below is the payload foramt for selector initialization:

struct ipc4_base_module_extended_cfg {
	struct ipc4_base_module_cfg base_cfg;
	struct ipc4_base_module_cfg_ext base_cfg_ext;  // nb_input_pins == 1, nb_output_pins == 1
}

Below is the payload format for set_large_config:

struct ipc4_selector_coeffs_config {
	uint16_t rsvd0;
	uint16_t rsvd1;

	/** Mixing coefficients in Q10 fixed point format */
	int16_t coeffs[SEL_SINK_CHANNELS_MAX][SEL_SOURCE_CHANNELS_MAX];
};

Copy link
Contributor

@pblaszko pblaszko left a comment

Choose a reason for hiding this comment

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

This change is breaking compatibility to present IPC4 protocol.
Current MicSelect InitInstance structure is:
{
ipc4_base_module_cfg base_cfg;
ipc4_audio_format out_format;
}

@aiChaoSONG
Copy link
Collaborator Author

@ujfalusi @ranj063 @pblaszko After synced with Poland team, the payload base + output format still need to be supported, because Windows use it, and if we convert it directly to use base + base_ext, we will break the compatibility with windows. Howerver, if we don't do the conversion, the selector can not work on linux side, so does wov feature.
How about aiChaoSONG@2e123b5, which makes the selector be able to handle both init blob.

@lgirdwood
Copy link
Member

@pblaszko @aiChaoSONG are you guys aligned now - do we need to make further changes ?

@aiChaoSONG
Copy link
Collaborator Author

do we need to make further change

I made a patch for the selector to support both base_config+output_format and base_config+base_config_extension initialization payload, aiChaoSONG@2e123b5, I'd like to see if Peter and Ranjani are good with this.
@ujfalusi @ranj063 Ping

@lgirdwood
Copy link
Member

do we need to make further change

I made a patch for the selector to support both base_config+output_format and base_config+base_config_extension initialization payload, aiChaoSONG@2e123b5, I'd like to see if Peter and Ranjani are good with this. @ujfalusi @ranj063 Ping

Ok, we also need to see @pblaszko approval too.

@pblaszko
Copy link
Contributor

pblaszko commented Mar 3, 2023

do we need to make further change

I made a patch for the selector to support both base_config+output_format and base_config+base_config_extension initialization payload, aiChaoSONG@2e123b5, I'd like to see if Peter and Ranjani are good with this. @ujfalusi @ranj063 Ping

Ok, we also need to see @pblaszko approval too.

I'm OK with this approach. I wait for bringing changes to this PR.

This patch extend the selector module to use base
config extension for module init, which ensures only
base_cfg and base_cfg_ext is received in module init
on linux side and align the init blob format with
other modules.

However, on Windows side, the payload base config with
output format style payload is still used, this payload
should be supported before Windows doing the payload
format migration.

Signed-off-by: Chao Song <[email protected]>
@lgirdwood
Copy link
Member

Ok, we also need to see @pblaszko approval too.

I'm OK with this approach. I wait for bringing changes to this PR.

@pblaszko ok, great - please approve yout review.

@lgirdwood lgirdwood merged commit 8e916d5 into thesofproject:main Mar 6, 2023
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.

10 participants