suspend/resume fix #1479
suspend/resume fix #1479
Conversation
keyonjie
commented
Nov 13, 2019
keyonjie
commented
We might need to check if we are doing d0i3 suspend in platform specific driver, export the helper snd_sof_dsp_d0i3_on_suspend() to make that possible. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
We have switched to use helper snd_sof_dsp_d0i3_on_suspend() to check if D0i3 suspend is on-going, which means the DSP is put to D3 when no d0i3 compatible stream opened even the whole system is entering S0ix. Here correct this in platform suspend/resume routines to make the DSP state matched between core and platform driver. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
keyonjie
requested review from
dbaluta,
lgirdwood and
plbossart
as code owners
|
@keyonjie so are you saying that we dont need to revert your commit which sets the DONE bit with this PR? |
exactly. I have verified that on my cml. |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
Sorry, but at this point I've lost trust in those S0ix and IPC changes.
| int ret; | ||
|
|
||
| if (sdev->s0_suspend) { | ||
| if (snd_sof_dsp_d0i3_on_suspend(sdev)) { |
There was a problem hiding this comment.
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)There was a problem hiding this comment.
I also don't get how the code worked without this patch, and only crapped out with your IPC changes.
More explanations please?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@keyonjie
I am really starting to wonder if we are in spaghetti code territory. Do we really need all ofsdev->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...
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
definitlvely not merging anything until I have a warm and fuzzy feeling on the S0ix directions.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
|
Closing this, it will be included in #1524 soon. |
3 participants
