Skip to content

suspend/resume fix #1479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
keyonjie wants to merge 2 commits into from
Closed

suspend/resume fix #1479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df6bdc8
ASoC: SOF: export helper snd_sof_dsp_d0i3_on_suspend
keyonjie Nov 13, 2019
8f0df2e
ASoC: SOF: Intel: hda-dsp: fix a mismatch state on suspend/resume
keyonjie Nov 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ bool snd_sof_dsp_d0i3_on_suspend(struct snd_sof_dev *sdev)

return false;
}
EXPORT_SYMBOL(snd_sof_dsp_d0i3_on_suspend);

/*
* FW Panic/fault handling.
Expand Down
4 changes: 2 additions & 2 deletions sound/soc/sof/intel/hda-dsp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,7 +481,7 @@ int hda_dsp_resume(struct snd_sof_dev *sdev)
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
struct pci_dev *pci = to_pci_dev(sdev->dev);

if (sdev->s0_suspend) {
if (snd_sof_dsp_d0i3_on_suspend(sdev)) {
/* restore L1SEN bit */
if (hda->l1_support_changed)
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
Expand Down Expand Up @@ -530,7 +530,7 @@ int hda_dsp_suspend(struct snd_sof_dev *sdev)
struct pci_dev *pci = to_pci_dev(sdev->dev);
int ret;

if (sdev->s0_suspend) {
if (snd_sof_dsp_d0i3_on_suspend(sdev)) {
Copy link
Member

@plbossart plbossart Nov 13, 2019

Choose a reason for hiding this comment

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

@keyonjie

I am really starting to wonder if we are in spaghetti code territory. Do we really need all of

sdev->s0_suspend
spcm->stream[substream->stream].d0i3_compatible
spcm->stream[substream->stream].suspend_ignored
snd_sof_dsp_d0i3_on_suspend(sdev)

Copy link
Member

@plbossart plbossart Nov 13, 2019

Choose a reason for hiding this comment

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

I also don't get how the code worked without this patch, and only crapped out with your IPC changes.

More explanations please?

Copy link
Author

@keyonjie keyonjie Nov 13, 2019

Choose a reason for hiding this comment

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

@keyonjie

I am really starting to wonder if we are in spaghetti code territory. Do we really need all of

sdev->s0_suspend
spcm->stream[substream->stream].d0i3_compatible
spcm->stream[substream->stream].suspend_ignored
snd_sof_dsp_d0i3_on_suspend(sdev)

I feel it a bit nasty also, but I can't remove any of them easily now. I think most of these are initiated from the DAPM PM event for the detect pipeline, once the SOF FW can support pipeline-segment feature, we can refine and clean up things here.

Copy link
Author

@keyonjie keyonjie Nov 13, 2019

Choose a reason for hiding this comment

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

I also don't get how the code worked without this patch, and only crapped out with your IPC changes.

Without the patch here, we will not power off DSP cores when doing S2Idle even without D0I3-compatible streams opened, and then in the subsequent resuming, we will try to power on cores and downloading FW and re-run it, it will fails in the first cl_dsp_init() try, but the retrying mechanism taking effect and we will succeed eventually, the final succeed hid the problems.

More explanations please?

Yes, basically, we need to power down the dsp cores at suspend, otherwise, the next power on to the cores is not resetting the running of the core and the ROM code can't accept and respond to the FW_PURGE IPC. Doing a forced powering down to the DSP cores at the begin of cl_dsp_init() will also work-around this issue.

And actually, the BSW-CYAN issue I have been working on these days is a little relative to this, on BSW, we don't have register/bit to power off the DSP core, which taking a lot of DSP re-booting troubles to us.

Copy link
Member

@plbossart plbossart Nov 13, 2019

Choose a reason for hiding this comment

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

@keyonjie
I am really starting to wonder if we are in spaghetti code territory. Do we really need all of

sdev->s0_suspend
spcm->stream[substream->stream].d0i3_compatible
spcm->stream[substream->stream].suspend_ignored
snd_sof_dsp_d0i3_on_suspend(sdev)

I feel it a bit nasty also, but I can't remove any of them easily now. I think most of these are initiated from the DAPM PM event for the detect pipeline, once the SOF FW can support pipeline-segment feature, we can refine and clean up things here.

the last one was removed by @ranj063 in #1478. but replaced by yet another flag. I get dizzy with all these changes. Maybe I should enforce a 2 day interface stabilization delay...

Copy link
Collaborator

@ranj063 ranj063 Nov 13, 2019

Choose a reason for hiding this comment

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

@plbossart @keyonjie let me spend some time to clean up the S0ix related flags today. Can we please hold off on merging this one until I come back with my proposal?

Copy link
Member

@plbossart plbossart Nov 13, 2019

Choose a reason for hiding this comment

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

definitlvely not merging anything until I have a warm and fuzzy feeling on the S0ix directions.

Copy link
Author

@keyonjie keyonjie Nov 18, 2019

Choose a reason for hiding this comment

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

@plbossart @ranj063 I still suggest take these firstly as they are fixes, and then the cleanup/refining and the subsequent S0 D0i3 support, doing them one by one will make the code easier to be tracked and bisected.

Copy link
Member

@plbossart plbossart Nov 18, 2019

Choose a reason for hiding this comment

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

no, I don't want patches that get modified immediately by another PR. This will look bad when we upstream and I don't want to be the one squashing stuff because developers didn't talk to each other on related topics.

/* enable L1SEN to make sure the system can enter S0Ix */
hda->l1_support_changed =
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
Expand Down