Skip to content

Conversation

@johnylin76
Copy link
Contributor

DSP panic is observed during resuming from suspend if the target core id is different between dmic01 (48k) and dmic16k pipeline. It failed to enable target core for widget PIPELINE.*.DMIC0.IN according to kernel log.

This commit aligns the core id for dmic16k to dmic01 (48k).

DSP panic is observed during resuming from suspend if the target
core id is different between dmic01 (48k) and dmic16k pipeline.
It failed to enable target core for widget PIPELINE.*.DMIC0.IN
according to kernel log.

This commit aligns the core id for dmic16k to dmic01 (48k).

Signed-off-by: Pin-chih Lin <johnylin@google.com>
ifdef(`DMIC_DAI_LINK_16k_NAME',`',define(DMIC_DAI_LINK_16k_NAME, `dmic16k'))

dnl Align the core target for dmic16k to dmic01 (48k)
define(DMIC_PIPELINE_16k_CORE_ID, DMIC_PIPELINE_48k_CORE_ID)
Copy link
Member

Choose a reason for hiding this comment

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

well if you want to align, then the 48k pipeline needs to use core 0.

To optimize power consumption the dmic16k + WoV need to be on core0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info @plbossart
Is it because core0 is reserved for WoV as default (which might be in different power state from other cores while suspended)?

Copy link
Member

Choose a reason for hiding this comment

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

core0 is on by default, and then other cores are turned on as needed which increases power consumption. So the recommended practice since forever is to place everything that needs to run in low-power use cases on core0, and use cores1..3 for processing, e.g. speaker protection and noise suppression on capture.

That said, it may be impossible at this point to move all the DMIC48k stuff to core0 if that core0 is already oversubscribed. that would mean different possible alternatives:
a) using your solution with an understanding that there's a power impact for WoV.
b) fixing the root cause and making sure the two paths can be used on different pipelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So theoretically it should be fine for DMIC48K and DMIC16K running on the different core?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's expected in terms of architecture that DMIC16k stuff is on core0 and other DMIC48k stuff is on other cores, but between architecture and implementation we may have a gap. I don't know if the panic you're seeing is by design or really a bug.

@johnylin76
Copy link
Contributor Author

The DSP panic issue during resuming from suspend is 100% reproducible on RPL-001 Waves-integrated build:

linux kernel version: chromeos v5.15
core id target for Speakers and Headphone (w/ Waves module):  0
core id target for DMIC48K: 1
core id target for DMIC16K and keyword detector: 0

But not reproducible on the common build:

linux kernel version: chromeos v5.15
core id target for Speakers and Headphone (no post-processing):  0
core id target for DMIC48K: 1
core id target for DMIC16K and keyword detector: 0

And the issue can be fixed by aligning core id target for DMIC48K and DMIC16K:

linux kernel version: chromeos v5.15
core id target for Speakers and Headphone (w/ Waves module):  0
core id target for DMIC48K: [0|1]
core id target for DMIC16K and keyword detector: same as  DMIC48K

This result makes sense to me since DMIC48K and DMIC16K share the same back-end. However I can't explain why is the DSP panic only on Waves-integrated build with v5.15 kernel, not on common build.
In addition, it seems not reproducible for the same Waves-integrated build with v5.10 kernel+ backporting commit chain.

@sathyap-chrome @cujomalainey any ideas about this?

@lgirdwood
Copy link
Member

Adding @mwasko

@cujomalainey
Copy link
Contributor

@johnylin76 any reason we shouldnt punt speaker to C1 as @plbossart mentioned for low power use we should stay on C0?

@johnylin76
Copy link
Contributor Author

@johnylin76 any reason we shouldnt punt speaker to C1 as @plbossart mentioned for low power use we should stay on C0?

Yes. I think this is the better fix. I also verified that DSP panic won't be reproduced by this topology spec:

linux kernel version: chromeos v5.15
core id target for Speakers (w/ Waves module):  1
core id target for Headphone (w/ Waves module): 0
core id target for DMIC48K: 1
core id target for DMIC16K and keyword detector: 0

That is, we should modify sof-tgl-max98357a-rt5682.m4#L162 from:

ifdef(`GOOGLE_RTC_AUDIO_PROCESSING', `define(`SPK_PLAYBACK_CORE', DMIC_PIPELINE_48k_CORE_ID)', `define(`SPK_PLAYBACK_CORE', `0')')

to

ifdef(`GOOGLE_RTC_AUDIO_PROCESSING', `define(`SPK_PLAYBACK_CORE', DMIC_PIPELINE_48k_CORE_ID)', `define(`SPK_PLAYBACK_CORE', `1')')

@cujomalainey
Copy link
Contributor

@johnylin76 any reason we shouldnt punt speaker to C1 as @plbossart mentioned for low power use we should stay on C0?

Yes. I think this is the better fix. I also verified that DSP panic won't be reproduced by this topology spec:

linux kernel version: chromeos v5.15
core id target for Speakers (w/ Waves module):  1
core id target for Headphone (w/ Waves module): 0
core id target for DMIC48K: 1
core id target for DMIC16K and keyword detector: 0

That is, we should modify sof-tgl-max98357a-rt5682.m4#L162 from:

ifdef(`GOOGLE_RTC_AUDIO_PROCESSING', `define(`SPK_PLAYBACK_CORE', DMIC_PIPELINE_48k_CORE_ID)', `define(`SPK_PLAYBACK_CORE', `0')')

to

ifdef(`GOOGLE_RTC_AUDIO_PROCESSING', `define(`SPK_PLAYBACK_CORE', DMIC_PIPELINE_48k_CORE_ID)', `define(`SPK_PLAYBACK_CORE', `1')')

LGTM, tbh, i almost feel like we need prolog to resolve some of these cases at this point...

@johnylin76
Copy link
Contributor Author

@johnylin76 any reason we shouldnt punt speaker to C1 as @plbossart mentioned for low power use we should stay on C0?

Yes. I think this is the better fix. I also verified that DSP panic won't be reproduced by this topology spec:

linux kernel version: chromeos v5.15
core id target for Speakers (w/ Waves module):  1
core id target for Headphone (w/ Waves module): 0
core id target for DMIC48K: 1
core id target for DMIC16K and keyword detector: 0

That is, we should modify sof-tgl-max98357a-rt5682.m4#L162 from:

ifdef(`GOOGLE_RTC_AUDIO_PROCESSING', `define(`SPK_PLAYBACK_CORE', DMIC_PIPELINE_48k_CORE_ID)', `define(`SPK_PLAYBACK_CORE', `0')')

to

ifdef(`GOOGLE_RTC_AUDIO_PROCESSING', `define(`SPK_PLAYBACK_CORE', DMIC_PIPELINE_48k_CORE_ID)', `define(`SPK_PLAYBACK_CORE', `1')')

LGTM, tbh, i almost feel like we need prolog to resolve some of these cases at this point...

Uploaded #6928 on mainstream for moving Speakers pipeline to C1 by default. Close this PR.

@johnylin76 johnylin76 closed this Jan 10, 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.

4 participants