[RFC]Suspend/Resume flow for APL - #32
Conversation
|
Here's the dmesg log from the point when suspend occurs until after the firmware boots up. |
|
From kernel log, rmbox should be not work ? so no rmbox message? And I think each internal buffer pointer or component status should be kept ?Or OS would send hw_param and start again? |
|
What happens to all the components created when the topology is loaded before suspend? Should the topology be reloaded after resume? |
|
I am not sure: some memory in DSP is still valid, so it can be used to keep some status |
|
@ranj063 : I don't think the topology should be parsed again on resume, but certainly anything that results in an IPC should be invoked on resume, e.g. SSP settings and pipeline commands (likely why the hw_params fail). I am afraid today we combined token parsing and IPC, maybe not such a good idea in hindsight. |
|
@ranj063 we probably dont want to pasre the topology again, but sof/topology.c does track a lot of the topology structures locally when probing. i.e. we have a list of widgets, controls etc. We should store a pointer to the topology raw structure for each topology object and on resume we iterate through all the topology object lists and send IPCs. |
|
@plbossart @lgirdwood Thanks! Let me re-send the ipc's to restore the pipeline from the topology objects stored in the driver. |
|
@plbossart @lgirdwood , I have updated the commits in this pull request to restore the pipeline at resume. |
|
@ranj063 ok, half way there now. Have a look at the legacy baytrail driver (sound/soc/intel/baytral/*) as it stores PCM playback state prior to S3 and restores at S0. You probably need to duplicate this flow. |
There was a problem hiding this comment.
What about if we have 2 or more pipelines ? If this restores all pipelines then maybe rename func.
There was a problem hiding this comment.
Just wondering if we need all these case statements given all the IPC data uses a standard header (which includes cmd and size). i.e. we cas the private data to the header type to get size and cmd.
There was a problem hiding this comment.
@lgirdwood I started it that way but I kept running into errors with it. The only way I can send the ipc without errors is by having all these case statements.
There was a problem hiding this comment.
ok, interesting, what errors ? The only thing the case statements do is cast types
There was a problem hiding this comment.
@lgirdwood. I've fixed it now. The error I was seeing was because of not casting the comp objects to (void *) while storing them.
There was a problem hiding this comment.
Best to try and send this today as it allows DSP to enter D3 ready state. Any context can be saved later.
There was a problem hiding this comment.
I'd expect most of this function to be in pm.c as generic code. The HW specific part hda_dsp_core_reset_power_down() can be an HW abstracted operation (add ops to the operations callback structure). e.g. we could have snd_sof_dsp_suspend(sdev), snd_sof_dsp_resume(sdev)
There was a problem hiding this comment.
Agree with Liam, this feels too APL-specific.
There was a problem hiding this comment.
@plbossart @lgirdwood I have updated this to use the chip cores_mask to make it generic.
|
@lgirdwood I've just pushed the v3 changes. Now I can resume audio playback normally after suspend/resume. There's an occasional xrun that happens at resume but its been hard to reproduce. |
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Mostly minor things, glad its working now :)
There was a problem hiding this comment.
Btw, we need to be able to handle different core masks, e.g. APL has 2, CNL has 4.
There was a problem hiding this comment.
@lgirdwood sure. I have tested this to work only for APL as yet. I will make the changes for CNL next.
There was a problem hiding this comment.
ok, interesting, what errors ? The only thing the case statements do is cast types
There was a problem hiding this comment.
I'd make the stream suspend a separate function
There was a problem hiding this comment.
Where do we send the IPC start message if we set cmd here ?
There was a problem hiding this comment.
@lgirdwood It is sent right outside the switch/case block where we set the cmd.
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
Looks good overall, still some style issues and my usual quixotic chase of memory leaks. One functional part also to align times with the firmware so that on resume the trace logs aren't completely misleading.
There was a problem hiding this comment.
For the volume case, don't you need to free swidget->private?
There was a problem hiding this comment.
need to free sroute first? this doesn't look consistent error management, or do you assume that all frees are done in an sof_route_unload()?
There was a problem hiding this comment.
you need a more consistent memory allocation/free. Here you free on one error, but not for all the other cases in this function below (look for all the return -EINVAL cases). You should have a set of gotos and deal with errors in a more organized way, or you deal with all the frees in another cleanup routine called on failure.
There was a problem hiding this comment.
oh yes. let me address this. seems like I've missed the route_load case to fix the memory allocation/free.
There was a problem hiding this comment.
well, duh. Can you please update this one, this will create issues with module load/unload and break CI.
There was a problem hiding this comment.
@plbossart sure, I am working on it. It wasnt very straightforward which route is being unloaded from the arguments. I need to study this a bit more to implement it correctly.
There was a problem hiding this comment.
@plbossart @lgirdwood it doesnt look like route_unload ever gets called.
I see a comment in soc-topology.c in the remove_widget() function that suggests that routes must
be removed before the widget itself is removed. But I cant find any references to route_unload.
Also, the arguments to the route_unload() method seem ambiguous. There's no dobj member in snd_soc_dapm_route structure. So I'm not sure how to get a handle to it in order to be able to remove it. Can you please help?
There was a problem hiding this comment.
You will need to deep dive this more as I'm short of time and may also need to modify the API if needed to pass in the extra info.
There was a problem hiding this comment.
Looks like we still have a dependency on a core cleanup?
There was a problem hiding this comment.
need explicit /* fallthrough */ to make tools happy that this is not a programming error
There was a problem hiding this comment.
Thanks for the reminder. I'll add this where appropriate.
There was a problem hiding this comment.
ret is not tested in most cases?
There was a problem hiding this comment.
@plbossart ret is tested right outside the switch/case block to make sure none of the ipc's failed.
There was a problem hiding this comment.
shouldn't there be some IPC sent on suspend, so that e.g. if there is any sort of context saving at the firmware level they'd know about it?
There was a problem hiding this comment.
@plbossart I do send the CTX_SAVE ipc during suspend in the suspend callback but not here as it is not a stream ipc message.
There was a problem hiding this comment.
BTW on resume we'd need to pass the new wall clock time to the firmware so that any trace logs are timestamped with the right values.
There was a problem hiding this comment.
@plbossart good point. Let me add it if we're not doing that already.
|
@plbossart @lgirdwood I've made some more changes to the flow now. |
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Sorry could not see the opens you mentioned ?
There was a problem hiding this comment.
Would also be good to see the widget type failing here swidget->id, we would then have the type and id.
|
@lgirdwood the two opens left are:
|
|
@ranj063 do you want us to review/test now or will the two opens come shortly enough that we want to wait for the update. |
|
@plbossart I should be done with the 2 opens by today and I think it will be better to test after those. But I've made some change regarding which device we register the PM callbacks for. I could use some feedback to make sure I've done the right thing. |
|
@ranj063 can you describe the device change so that the rest of us don't have to reverse-engineer the deltas between patches? |
|
@plbossart this is the commit id for the device change in this patchset: With the callbacks set for the pci device, they never get called as the runtime_pm_enable() is called for the platform device and not the pci device. So setting the callbacks for the platform device fixes the issue. |
|
@ranj063 for the kcontrol restore, iirc baytrail/haswell drivers did this by caching the values in the host. |
|
@lgirdwood, sure. I think I have an idea of how to do this. I will update the PR after I fix it. |
|
@plbossart @lgirdwood I've addressed all opens now. Could I request you to please review the latest version. |
5 participants
This patchset aims to implement the preliminary flow for suspend/resume for APL.
V2: Now the pipeline status is restored at resume by sending the ipc's to create all the components and set up the pipeline. With this change, the DSP is power off at suspend and when it resumes from suspend, audio playback works normally.
But when suspend is invoked while audio playback is in progress, playback does not resume and aplay quits with the error "pcm_write:2011: write error: Input/output error".
V3: changes made to reflect previous comments
Audio playback resumes normally after suspend/resume.
V4: Added support for runtime PM and made changes based on previous feedback.
Now, both runtime PM and suspend/resume work well.
2 tasks still left to do: