topology: sof-tgl-nocodec-ci: change to run smart_amp on core 1

[keyonjie]

[Marc: previous discussion about this was in #4082]

Change to run smart_amp pipelines on DSP core 1, this will help reduce
the core 0 usage and add multi-core to the CI coverage.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

Bring this back for multi-core CI coverage. @plbossart @ranj063 @lgirdwood @marc-hb

[lyakh]

This will change it for all users of the smart amplifier, right? Of which I see at least two TGL platforms in the tree. Do we really want it? You're saying that it's experimental, so, maybe only enable it for a single development topology first?

[keyonjie]

This will change it for all users of the smart amplifier, right? Of which I see at least two TGL platforms in the tree. Do we really want it? You're saying that it's experimental, so, maybe only enable it for a single development topology first?

no, this only change for sof-tgl-nocodec-ci.m4, not for any other smart amplifier users.

[marc-hb]

Already asked in #4141 and again here: is there a README that explains how files in tools/topology/development/ are used and when?

[lyakh]

This will change it for all users of the smart amplifier, right? Of which I see at least two TGL platforms in the tree. Do we really want it? You're saying that it's experimental, so, maybe only enable it for a single development topology first?

no, this only change for sof-tgl-nocodec-ci.m4, not for any other smart amplifier users.

how about sof-tgl-sdw-max98373-rt5682.m4 and sof-tgl-max98373-rt5682.m4 ?

[keyonjie]

This will change it for all users of the smart amplifier, right? Of which I see at least two TGL platforms in the tree. Do we really want it? You're saying that it's experimental, so, maybe only enable it for a single development topology first?

no, this only change for sof-tgl-nocodec-ci.m4, not for any other smart amplifier users.

how about sof-tgl-sdw-max98373-rt5682.m4 and sof-tgl-max98373-rt5682.m4 ?

smart_amp there still run on Core 0.

[lyakh]

smart_amp there still run on Core 0.

oh, sure, sorry, my bad

sorry approved in haste. see comment above

[ranj063]

@keyonjie I dont think this is the right pipeline to be moved to core 1. The DSM pipeline on the chrome devices is the only one that has a volatile kcontrol. I dont know if the mock DSM module has it to or not. But given that we have a restriction that pipelines that contain a volatile kcontrol must be static, I prefer keeping this pipeline on Core 0. If the intent is to have "a" pipeline on core 1 for validating multi-core, lets pick another pipeline please.

[keyonjie]

@keyonjie I dont think this is the right pipeline to be moved to core 1. The DSM pipeline on the chrome devices is the only one that has a volatile kcontrol. I dont know if the mock DSM module has it to or not. But given that we have a restriction that pipelines that contain a volatile kcontrol must be static, I prefer keeping this pipeline on Core 0. If the intent is to have "a" pipeline on core 1 for validating multi-core, lets pick another pipeline please.

um, we preferred the DSM pipelines to run on a slave core as it can mimic the real requirement most, we have 2 pipelines there, and the DSM module could consume quite big MCPS with customer's algorithm.

On the other hand, even though we have to make DSM pipelines be static, we can still put them on slave core, the change here only for TGL nocodec CI coverage, it will not impact any customer ones.

[keyonjie]

Already asked in #4141 and again here: is there a README that explains how files in tools/topology/development/ are used and when?

Unfortunately no, though it will be good if we can add one, that is similar for other folders and .m4 files also.

[lgirdwood]

@keyonjie ok, good to have this back again in CI.

[lgirdwood]

@keyonjie I dont think this is the right pipeline to be moved to core 1. The DSM pipeline on the chrome devices is the only one that has a volatile kcontrol. I dont know if the mock DSM module has it to or not. But given that we have a restriction that pipelines that contain a volatile kcontrol must be static, I prefer keeping this pipeline on Core 0. If the intent is to have "a" pipeline on core 1 for validating multi-core, lets pick another pipeline please.

@ranj063 I'm assuming volatile kcontrols are a blocker for dynamic pipelines today ? Can you describe the issue, do we need an ALSA update or similar here ?

[keyonjie]

This already help us in finding kinds of multi-core issues, see https://sof-ci.sh.intel.com/#/result/planresultdetail/3832.

@lgirdwood let's merge this to speed up multi-core quality improvement.

[lgirdwood]

@keyonjie this is probably merged a bit early, need to see what the blocker is for volatile ALSA kcontrols are here so we can understand the impact.

[keyonjie]

@keyonjie this is probably merged a bit early, need to see what the blocker is for volatile ALSA kcontrols are here so we can understand the impact.

@ranj063 talked this to me, for volatile kcontrols, we need to read back their values from the FW (not the cached one in the driver side), that means the corresponding module/component should be active, we need to keep it's pipeline as a static/resident one, or need to wake up it when the kcontrol reading happens, @ranj063 told me the former choice is an reasonable and easier one.

My point here we should not limit the static pipeline run on core 0 only, what's your opinion @lgirdwood?

[lgirdwood]

@ranj063 @keyonjie kcontrols should return last known value here. i.e. they should switch to cached mode.

[ranj063]

@ranj063 @keyonjie kcontrols should return last known value here. i.e. they should switch to cached mode.

@lgirdwood but then doesnt it defeat the purpose of a volatile kcontrol? We only allow returning the cached vaues for the non-volatile kcontrols.

[marc-hb]

This already help us in finding kinds of multi-core issues, see https://sof-ci.sh.intel.com/#/result/planresultdetail/3832.
@lgirdwood let's merge this to speed up multi-core quality improvement.

I'm not sure what this comment meant.

• Merging to the main branch to find new, unknown issues that reproduce rarely or have no test yet: OK!

• Merging to the main branch when there are known issues that make existing tests fail for everyone and everything else: that would sound pretty bad. That's what other branches are for.

[keyonjie]

This already help us in finding kinds of multi-core issues, see https://sof-ci.sh.intel.com/#/result/planresultdetail/3832.
@lgirdwood let's merge this to speed up multi-core quality improvement.

I'm not sure what this comment meant.

• Merging to the main branch to find new, unknown issues that reproduce rarely or have no test yet: OK!
• Merging to the main branch when there are known issues that make existing tests fail for everyone and everything else: that would sound pretty bad. That's what other branches are for.

The former one.

[keyonjie]

@ranj063 @keyonjie kcontrols should return last known value here. i.e. they should switch to cached mode.

@lgirdwood but then doesnt it defeat the purpose of a volatile kcontrol? We only allow returning the cached vaues for the non-volatile kcontrols.

Can we do that with "if else" check? e.g.

if (pipeline active) 
    read volatile value from FW;
else
    read the value cached in the driver.

IMHO, force a pipeline to be resident (always active) with the purpose just to meet the requirement that we can get volatile kcontrol value always sounds like attending to trifles with neglecting of essentials to me.

[plbossart]

IMHO, force a pipeline to be resident (always active) with the purpose just to meet the requirement that we can get volatile kcontrol value always sounds like attending to trifles with neglecting of essentials to me.

the volatile controls have nothing to do with the current issue reported in #4163, it's a PCM read issue

**arecord: pcm_read:2155: read error: Input/output error

Everyone can have opinions, but what we need is to fix bugs before we worry about other things. let's focus a bit, shall we?

[plbossart]

Issue #4164 also seems related to the multi-core change btw.

[lgirdwood]

@ranj063 @keyonjie kcontrols should return last known value here. i.e. they should switch to cached mode.

@lgirdwood but then doesnt it defeat the purpose of a volatile kcontrol? We only allow returning the cached vaues for the non-volatile kcontrols.

I mean if a volatile control belongs to a pipeline that unloaded or the DSP is in D3 then we just return the cached or a default value otherwise we return the real value.