mtl: introduce mtl_dsp_remove() to fix kmod removal issue by fredoh9 · Pull Request #3679 · thesofproject/linux

fredoh9 · 2022-06-02T23:35:48Z

fixes #3662

sound/soc/sof/intel/mtl.c

sound/soc/sof/intel/hda.c

fredoh9 · 2022-06-03T05:25:14Z

@ranj063 I pushed for adding mtl_dsp_remove() change.

sound/soc/sof/intel/hda.c

sound/soc/sof/intel/mtl.c

ranj063 · 2022-06-03T06:30:40Z

sound/soc/sof/intel/mtl.c

+
+	/* disable cores */
+	if (chip)
+		mtl_dsp_core_power_down(sdev, chip->host_managed_cores_mask);


we could explicitly use primary core mask as we know thats the only one needed for MTL to be powered down

Wondering if we really need to copy this code, just one function that differs. We could have a callback instead and use the default code, as we did for interrupts

@plbossart i thought about that too but we already have core_set_state() and the core_get/core_put. Adding another ops for powering down cores would just add to confusion. Not sure, if it makes sense or not

core_get/core_put is a bit different then what we would need here as those two is also sending IPC message to the firmware and cure_put is not implemented for non TGL.
Here we do not want to send IPC.
struct sof_intel_dsp_desc level power_down_host_managed_cores(struct snd_sof_dev *sdev) or an even more generic set_host_managed_cores_power(struct snd_sof_dev *sdev, bool power_up) might work and can be used to share more code, but I don't have objection for the duplicated code.

@ranj063, @bardliao, should we implement core_get/put for MTL? I think we have all the ingredients, no?

core_get/core_put is a bit different then what we would need here as those two is also sending IPC message to the firmware and cure_put is not implemented for non TGL. Here we do not want to send IPC. struct sof_intel_dsp_desc level power_down_host_managed_cores(struct snd_sof_dev *sdev) or an even more generic set_host_managed_cores_power(struct snd_sof_dev *sdev, bool power_up) might work and can be used to share more code, but I don't have objection for the duplicated code.

@ranj063, @bardliao, should we implement core_get/put for MTL? I think we have all the ingredients, no?

Yes, we need core_get/put for MTL.

@plbossart i thought about that too but we already have core_set_state() and the core_get/core_put. Adding another ops for powering down cores would just add to confusion. Not sure, if it makes sense or not

What's more confusing is the copy of all the initialization flow. That's not really maintainable IMHO, we should try to do something better where the flow remains the same except for one function.

plbossart · 2022-06-06T13:39:40Z

@fredoh9 please make sure the code compiles with W=1 and sparse, see reports

 sound/soc/sof/intel/mtl.c:728:5: error: no previous prototype for ‘mtl_dsp_remove’ [-Werror=missing-prototypes]
  728 | int mtl_dsp_remove(struct snd_sof_dev *sdev)
      |     ^~~~~~~~~~~~~~
  CC [M]  sound/soc/soc-pcm.o

ranj063 · 2022-06-17T16:18:43Z

@fredoh9 can you please update this PR with @plbossart 's suggestion? Maybe we need to add a core_put() op and call it from hda_dsp_remove()

plbossart · 2022-06-22T14:55:30Z

@fredoh9 do you mind completing this work before starting something new? It's not productive if we keep multiple PRs on the fire.

fredoh9 · 2022-06-22T15:33:24Z

@fredoh9 do you mind completing this work before starting something new? It's not productive if we keep multiple PRs on the fire.

I was thinking to finish core get/put from TGL first. Sure, will finish this work also.

fredoh9 · 2022-06-29T23:01:43Z

This is not complete yet, but instead of sitting on it, I need to share the current code with the problem. I'm still based on Rander's SRC PR to test. I can play with SRC pipeline.

[ 6174.531260] sof-audio-pci-intel-mtl 0000:00:1f.3: Core 1 powered up
[ 6174.531261] sof-audio-pci-intel-mtl 0000:00:1f.3: Create widget src.1.1 instance 0 - pipe 1 - core 1
[ 6176.723534] sof-audio-pci-intel-mtl 0000:00:1f.3: Core 1 powered down

When I do sof_remove, i have an ipc error, looking at it.

[  133.643313] sof-audio-pci-intel-mtl 0000:00:1f.3: Fred: core_put core=1 dx_mask=-3
[  133.643375] sof-audio-pci-intel-mtl 0000:00:1f.3: ipc4_tx_msg_unlocked: ipc message send for 0x47000000|0xffff9859 failed: -19

plbossart · 2022-07-12T19:50:31Z

@fredoh9 can we prioritize this, this is a bug with MTL and I'd like to send this fix upstream in this kernel cycle. we have only a couple of days left, thanks!

fredoh9 · 2022-07-20T02:19:18Z

@ranj063 @plbossart @bardliao @RanderWang
Sorry for huge delay. Lost stable ground. Ranjani, thank you for your help/debugging!

sound/soc/sof/intel/hda.c

DSP core power down sequences are different between cavs platforms and MTL. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

hda_power_down_dsp is set for power_down_dsp op for all HDA platforms. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

fredoh9 · 2022-07-21T05:56:14Z

@plbossart @ranj063 @bardliao @RanderWang @yongzhi1 @RDharageswari
I just pushed new set of patches. I was able to test on MTLP_RVP_SDW.

kmod test working. sof_remove.sh -> sof_insert.sh
runtime pm also working. Playback start -> stop, wait for DSP D3 -> start playback.
suspend/resume test also working. This is a bit surprise to me. ./check-suspend-resume.sh -l 2

plbossart

looks mostly good, one question below. thanks @fredoh9

sound/soc/sof/intel/hda.c

sound/soc/sof/intel/mtl.c

For MTL platform, dsp cores need to go power down first then dsp subsystem also need to set power down. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

sound/soc/sof/intel/hda.c

plbossart

two nit-picks below. Thanks @fredoh9

sound/soc/sof/intel/hda.c

Use power_down_dsp op to differentiate power down sequences in platforms. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

ranj063 · 2022-07-21T21:51:05Z

@RanderWang @bardliao could you please take a look at this one?

bardliao · 2022-07-22T02:23:52Z

sound/soc/sof/intel/shim.h

@@ -186,6 +186,7 @@ struct sof_intel_dsp_desc {
 	enum sof_intel_hw_ip_version hw_ip_version;


Probably change the commit message to DSP core power down sequences are different between CAVS platforms and ACE platforms.

bardliao

Only one nitpick from me. We could say ACE platforms instead of MTL compare to CAVS platforms

fredoh9 requested review from ajitkupandey, bardliao, dbaluta, kuanhsuncheng, kv2019i, lgirdwood, libinyang, plbossart, ranj063, ujfalusi and yaochunhung as code owners June 2, 2022 23:35

fredoh9 marked this pull request as draft June 2, 2022 23:36

ranj063 reviewed Jun 3, 2022

View reviewed changes

sound/soc/sof/intel/mtl.c Outdated Show resolved Hide resolved

sound/soc/sof/intel/hda.c Outdated Show resolved Hide resolved

fredoh9 force-pushed the fix/mtl_kmod_removal branch from 79cdcc8 to 068c130 Compare June 3, 2022 05:23

fredoh9 requested a review from ranj063 June 3, 2022 05:25

ranj063 reviewed Jun 3, 2022

View reviewed changes

fredoh9 marked this pull request as ready for review June 3, 2022 21:04

fredoh9 requested a review from lyakh as a code owner June 3, 2022 21:04

fredoh9 force-pushed the fix/mtl_kmod_removal branch from 068c130 to 9f74507 Compare June 3, 2022 21:04

fredoh9 changed the title ~~Fix/mtl kmod removal~~ mtl: introduce mtl_dsp_remove() to fix kmod removal issue Jun 3, 2022

fredoh9 requested a review from ranj063 June 3, 2022 21:06

fredoh9 marked this pull request as draft June 9, 2022 19:06

fredoh9 mentioned this pull request Jun 27, 2022

IPC4: TGL: Add multicore support #3711

Closed

fredoh9 force-pushed the fix/mtl_kmod_removal branch from 9f74507 to c4e2554 Compare June 29, 2022 22:50

fredoh9 force-pushed the fix/mtl_kmod_removal branch from c4e2554 to 2d4c8ba Compare July 20, 2022 02:17

fredoh9 marked this pull request as ready for review July 20, 2022 02:17

fredoh9 requested a review from RanderWang July 20, 2022 02:17

RanderWang reviewed Jul 20, 2022

View reviewed changes

sound/soc/sof/intel/hda.c Show resolved Hide resolved

plbossart reviewed Jul 20, 2022

View reviewed changes

sound/soc/sof/intel/hda.c Outdated Show resolved Hide resolved

fredoh9 added 2 commits July 20, 2022 22:53

ASoC: SOF: Intel: introduce new op to handle dsp power down

8c4e436

DSP core power down sequences are different between cavs platforms and MTL. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

ASoC: SOF: Intel: define and set power_down_dsp op for HDA platforms

813de12

hda_power_down_dsp is set for power_down_dsp op for all HDA platforms. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

fredoh9 force-pushed the fix/mtl_kmod_removal branch from 2d4c8ba to 2cdf727 Compare July 21, 2022 05:53

fredoh9 requested a review from juimonen as a code owner July 21, 2022 05:53

fredoh9 requested review from RanderWang and plbossart July 21, 2022 05:56

plbossart reviewed Jul 21, 2022

View reviewed changes

sound/soc/sof/intel/hda.c Show resolved Hide resolved

ranj063 reviewed Jul 21, 2022

View reviewed changes

sound/soc/sof/intel/mtl.c Outdated Show resolved Hide resolved

ASoC: SOF: Intel: mtl: define and set power_down_dsp op

e52897b

For MTL platform, dsp cores need to go power down first then dsp subsystem also need to set power down. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

fredoh9 force-pushed the fix/mtl_kmod_removal branch from 2cdf727 to 3268bcf Compare July 21, 2022 17:36

ranj063 reviewed Jul 21, 2022

View reviewed changes

sound/soc/sof/intel/hda.c Outdated Show resolved Hide resolved

fredoh9 force-pushed the fix/mtl_kmod_removal branch from 3268bcf to c3c1ff2 Compare July 21, 2022 17:59

plbossart requested changes Jul 21, 2022

View reviewed changes

sound/soc/sof/intel/hda.c Outdated Show resolved Hide resolved

sound/soc/sof/intel/hda.c Outdated Show resolved Hide resolved

ASoC: SOF: Intel: use power_down_dsp op in hda_dsp_remove

d344ded

Use power_down_dsp op to differentiate power down sequences in platforms. Signed-off-by: Fred Oh <fred.oh@linux.intel.com>

fredoh9 force-pushed the fix/mtl_kmod_removal branch from c3c1ff2 to d344ded Compare July 21, 2022 18:05

ranj063 approved these changes Jul 21, 2022

View reviewed changes

plbossart approved these changes Jul 21, 2022

View reviewed changes

RanderWang approved these changes Jul 22, 2022

View reviewed changes

bardliao reviewed Jul 22, 2022

View reviewed changes

bardliao approved these changes Jul 22, 2022

View reviewed changes

ranj063 merged commit 87d75bf into thesofproject:topic/sof-dev Jul 22, 2022

		@@ -186,6 +186,7 @@ struct sof_intel_dsp_desc {
		enum sof_intel_hw_ip_version hw_ip_version;

Conversation

fredoh9 commented Jun 2, 2022 • edited by ranj063 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredoh9 commented Jun 3, 2022

Uh oh!

Uh oh!

Uh oh!

ranj063 Jun 3, 2022

Choose a reason for hiding this comment

Uh oh!

plbossart Jun 3, 2022

Choose a reason for hiding this comment

Uh oh!

ranj063 Jun 3, 2022

Choose a reason for hiding this comment

Uh oh!

ujfalusi Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

bardliao Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

plbossart Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

plbossart commented Jun 6, 2022

Uh oh!

ranj063 commented Jun 17, 2022

Uh oh!

plbossart commented Jun 22, 2022

Uh oh!

fredoh9 commented Jun 22, 2022

Uh oh!

fredoh9 commented Jun 29, 2022

Uh oh!

plbossart commented Jul 12, 2022

Uh oh!

fredoh9 commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredoh9 commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ranj063 commented Jul 21, 2022

Uh oh!

bardliao Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fredoh9 commented Jun 2, 2022 •

edited by ranj063

Loading

fredoh9 commented Jul 20, 2022 •

edited

Loading

fredoh9 commented Jul 21, 2022 •

edited

Loading