topology: set Speakers PPL running on C1 by default #6928

johnylin76 · 2023-01-10T13:27:14Z

For Intel platforms, C0 is on by default for processes necessary to run in low power cases, while C1..3 for processes with higher loads. This commit moves the default core to C1 for Speakers pipeline.

At the same time, we found this commit can resolve the DSP panic issue on builds with 3P post-processing solutions running on Speakers pipeline.

lgirdwood

@johnylin76 I'm assuming this will also be the configuration for MTL ?
@mwasko fyi.

plbossart · 2023-01-11T16:11:59Z

@johnylin76 I'm assuming this will also be the configuration for MTL ? @mwasko fyi.

aren't we moving to per-module core assignment instead of per-pipeline?

mmaka1 · 2023-01-12T21:18:37Z

@johnylin76 I'm assuming this will also be the configuration for MTL ? @mwasko fyi.

aren't we moving to per-module core assignment instead of per-pipeline?

We are considering a constraint in the core allocation process due to L1 cache vs. uncache memory access performance. But also to avoid issues with global ordering and synchronization of the low latency modules. The current plan is to allocate all LL modules belonging to all pipelines connected as a stream (host - dai in any direction) on a single DSP core to guarantee proper order and L1 cache memory access without need to sync.

plbossart · 2023-01-12T22:55:38Z

@johnylin76 I'm assuming this will also be the configuration for MTL ? @mwasko fyi.

aren't we moving to per-module core assignment instead of per-pipeline?

We are considering a constraint in the core allocation process due to L1 cache vs. uncache memory access performance. But also to avoid issues with global ordering and synchronization of the low latency modules. The current plan is to allocate all LL modules belonging to all pipelines connected as a stream (host - dai in any direction) on a single DSP core to guarantee proper order and L1 cache memory access without need to sync.

Right, but I was referring to DP processing. In the MTL time frame, one would hope that speaker protection doesn't have to run as a LL module - none of the existing 3rd party modules qualify as LL really.

johnylin76 · 2023-01-17T06:01:29Z

Hi @lgirdwood @plbossart
Could you advise the next action for this PR commit? We'll need it in rpl-001 for production builds.
If this is still debatable, is it ok to land in rpl-001 only (skipping mainstream)?
Thanks

lgirdwood · 2023-01-18T13:27:14Z

Hi @lgirdwood @plbossart Could you advise the next action for this PR commit? We'll need it in rpl-001 for production builds. If this is still debatable, is it ok to land in rpl-001 only (skipping mainstream)? Thanks

@johnylin76 I think we should be OK for a main branch waiver this time.
@mwasko any objections to take this direct to rpl-01 without being in main ?

mwasko · 2023-01-19T08:06:01Z

@johnylin76 I would like to see SW experts approvals prior merging the topology change - @plbossart, @ranj063 ?

plbossart · 2023-01-19T13:09:34Z

@johnylin76 I would like to see SW experts approvals prior merging the topology change - @plbossart, @ranj063 ?

SW doesn't really care @mwasko. The information coming from topology is used as is, there's no intelligence beyond tracking which core is used.

plbossart

not completely comfortable with the proposal @johnylin76

plbossart · 2023-01-19T15:13:07Z

tools/topology/topology1/sof-tgl-max98373-rt5682.m4

is the addition of a separate variable from SPK_PLAYBACK_CORE intentional?

This could lead to a case where SMART_AMP_CORE=1 and SPK_PLAYBACK_CORE=0 - not sure if this is desired or that it even works since the two pipelines are connected.

Should this be

define(`SMART_AMP_CORE', SPK_PLAYBACK_CORE)

??

Sure. Thanks for the suggestion.

plbossart · 2023-01-19T15:13:24Z

tools/topology/topology1/m4/dts_codec_adapter.m4

spurious change?

I think so. DTS module doesn't have the particular demand, CA_SCHEDULE_CORE should be assigned as SCHEDULE_CORE in the latter code.

Defines the target core ID for Speakers and Echo Reference pipelines explicitly. When GOOGLE_RTC_PROCESSING is defined, it will be set to the same ID as DMIC48K pipeline. Signed-off-by: Pin-chih Lin <johnylin@google.com>

For Intel platforms, C0 is on by default for processes necessary to run in low power cases, while C1..3 for processes with higher loads. This commit moves the default core to C1 for Speakers pipeline. At the same time, we found this commit can resolve the DSP panic issue on builds with 3P post-processing solutions running on Speakers pipeline. Signed-off-by: Pin-chih Lin <johnylin@google.com>

johnylin76 · 2023-01-20T08:23:45Z

Hi @lgirdwood @plbossart Could you advise the next action for this PR commit? We'll need it in rpl-001 for production builds. If this is still debatable, is it ok to land in rpl-001 only (skipping mainstream)? Thanks

@johnylin76 I think we should be OK for a main branch waiver this time. @mwasko any objections to take this direct to rpl-01 without being in main ?

I think what @lgirdwood suggested here is landing on rpl-001-drop-stable directly? Since this PR is for main I will create a new one on rpl-001-drop-stable.

I just revised the modification a bit and separated to 2 commits. The 1st commit 5d77fa1 only added SPK_PLAYBACK_CORE definition, which is also configurable by makefile argument for flexibility considering. The default Speakers core ID was moved to C1 by the 2nd commit 5d16691.

By doing so we can land the 1st commit on main (and rpl-001-drop-stable) harmlessly, then see whether to land the 2nd one specifically on rpl-001-drop-stable(?)

lgirdwood · 2023-01-26T13:48:13Z

@plbossart updates good for you now ?

lyakh · 2023-01-27T08:50:27Z

2 comments:

Just to make sure we don't forget this: these changes aren't tested by the CI - the PR CI now only tests IPC4 / topology2
This has already been mentioned before, but just to make sure - this doesn't move some pipelines in a connected topology to another core, while keeping other pipelines in the same stream on the original core? Are any other pipelines connected to the one, changed here? If yes - they're also running on core 1, aren't they? So before this change pipelines would run on different cores?

johnylin76 · 2023-01-31T14:13:26Z

2 comments:

Just to make sure we don't forget this: these changes aren't tested by the CI - the PR CI now only tests IPC4 / topology2

Thanks for the note.

This has already been mentioned before, but just to make sure - this doesn't move some pipelines in a connected topology to another core, while keeping other pipelines in the same stream on the original core? Are any other pipelines connected to the one, changed here? If yes - they're also running on core 1, aren't they? So before this change pipelines would run on different cores?

It's confirmed that the connected pipelines run on the same core both before and after this change. For cases having other pipelines connected to Spkr (e.g. GOOGLE_RTC_AUDIO_PROCESSING), SPK_PLAYBACK_CORE will be overwritten to align as the connected pipeline (DMIC0), which is already running on core 1.

johnylin76 · 2023-02-01T06:50:58Z

@lgirdwood @lyakh is it all good to merge now?

lyakh · 2023-02-01T12:02:39Z

@lgirdwood @lyakh is it all good to merge now?

No objections against these changes in principle, but just a note, that IPC3 / topology1 aren't actually supported on "main" any more

lgirdwood · 2023-02-01T16:55:36Z

@lgirdwood @lyakh is it all good to merge now?

No objections against these changes in principle, but just a note, that IPC3 / topology1 aren't actually supported on "main" any more

On Intel platforms, other platforms still support IPC3 and topology1.

johnylin76 requested a review from cujomalainey January 10, 2023 13:27

johnylin76 requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners January 10, 2023 13:27

johnylin76 mentioned this pull request Jan 10, 2023

topology: align core target for dmic16k to dmic01 #6916

Closed

lgirdwood reviewed Jan 11, 2023

View reviewed changes

mwasko approved these changes Jan 19, 2023

View reviewed changes

plbossart reviewed Jan 19, 2023

View reviewed changes

johnylin76 added 2 commits January 20, 2023 15:53

topology1: define SPK_PLAYBACK_CORE for Speakers running core

5d77fa1

Defines the target core ID for Speakers and Echo Reference pipelines explicitly. When GOOGLE_RTC_PROCESSING is defined, it will be set to the same ID as DMIC48K pipeline. Signed-off-by: Pin-chih Lin <johnylin@google.com>

plbossart approved these changes Jan 26, 2023

View reviewed changes

lgirdwood approved these changes Feb 1, 2023

View reviewed changes

lgirdwood merged commit 7f80908 into thesofproject:main Feb 1, 2023

johnylin76 mentioned this pull request Feb 2, 2023

cherry-pick: topology: set Speakers PPL running on C1 by default #7030

Merged

This was referenced Feb 2, 2023

cherry-pick: topology1 fixes #7031

Merged

[BUG][ADL-P] DSP panic on device boot-up or resume-from-suspend for sof-adl-max98357a-rt5682-2way tplg #6964

Closed

topology: set Speakers PPL running on C1 by default #6928

topology: set Speakers PPL running on C1 by default #6928

Uh oh!

Conversation

johnylin76 commented Jan 10, 2023

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Jan 11, 2023

Uh oh!

mmaka1 commented Jan 12, 2023

Uh oh!

plbossart commented Jan 12, 2023

Uh oh!

johnylin76 commented Jan 17, 2023

Uh oh!

lgirdwood commented Jan 18, 2023

Uh oh!

mwasko commented Jan 19, 2023

Uh oh!

plbossart commented Jan 19, 2023

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

johnylin76 Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

plbossart Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

johnylin76 Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

johnylin76 commented Jan 20, 2023

Uh oh!

lgirdwood commented Jan 26, 2023

Uh oh!

lyakh commented Jan 27, 2023

Uh oh!

johnylin76 commented Jan 31, 2023

Uh oh!

johnylin76 commented Feb 1, 2023

Uh oh!

lyakh commented Feb 1, 2023

Uh oh!

lgirdwood commented Feb 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants