Repository navigation
Conversation
The state machine and notes don't accurately explain or allow transitions from STREAM_DEPREPARED and STREAM_DISABLED. Add more explanations and allow for more transitions as a result of a trigger_stop(), trigger_suspend() and prepare(), depending on the ALSA/ASoC layer behavior defined by the INFO_RESUME and INFO_PAUSE flags. Also add basic checks to help debug inconsistent states and illegal state machine transitions. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…transition After a system suspend, the ALSA/ASoC core will invoke the .prepare() callback and a TRIGGER_START when INFO_RESUME is not supported. Likewise, when an underflow occurs, the .prepare callback will be invoked. In both cases, the stream can be in DISABLED mode, and will transition into the PREPARED mode. We however don't want the bus bandwidth to be recomputed. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The .prepare() callback is invoked for underflows or during the system resume transition. In the latter case, the context for the ALH PDIs is lost, and the DSP is not initialized properly either, but the bus parameters don't need to be recomputed. Keep track of the TRIGGER_SUSPEND transition with a boolean flag, and use it during the .prepare step to check if the ALH PDIs need to be reconfigured. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
021c790 to
abb4b39
Compare
bardliao
left a comment
bardliao
left a comment
There was a problem hiding this comment.
@plbossart We are closed but still some thing need to be clarify.
|
|
||
| sdw_acquire_bus_lock(stream); | ||
|
|
||
| if (stream->state != SDW_STREAM_DISABLED) { |
There was a problem hiding this comment.
@plbossart prepared <--> deprepared is valid, right? I can see hw_parames -> prepare -> hw_free without ant trigger when pulseaudio on. I.e. No enable/disable between prepare/deprepare. I think it makes sense since the application may only want to configure the stream but not to play it. So I think we can add && stream->state != SDW_STREAM_PREPARED here.
There was a problem hiding this comment.
You are right, but that requires an update to the state machine as well. Will fix.
Nice catch, thanks @bardliao
|
|
||
| ret = _sdw_prepare_stream(stream); | ||
| if (stream->state != SDW_STREAM_CONFIGURED && | ||
| stream->state != SDW_STREAM_DISABLED) { |
There was a problem hiding this comment.
@plbossart Will it be better if we do the check before sdw_acquire_bus_lock() and move state_err: after sdw_release_bus_lock()?
There was a problem hiding this comment.
@bardliao I think we have to use the lock before checking the state.
Put differently, the state is changed when the bus is locked, so it would be odd to allow the state to be checked without taking the lock as well, you could have race conditions when you read a state that becomes immediately inconsistent.
| case SNDRV_PCM_TRIGGER_PAUSE_PUSH: | ||
| case SNDRV_PCM_TRIGGER_SUSPEND: | ||
| dev_err(dai->dev, "%s: %s: suspend\n", __func__, dai->name); | ||
| dma->suspend_reinitialize = true; |
There was a problem hiding this comment.
@plbossart set suspend_reinitialize = true here may not be the best place. The reason we need to reinitialize cdns is when system is suspended. So I think it is better to set suspend_reinitialize = true in intel_suspend(). We may need bardliao@d0247d3
There was a problem hiding this comment.
No, I don't think we can set suspend_reinitialize in intel_suspend. The reason is that we need to reprogram the ALH stream ID, and if you have more than one DAI for a given device then with your patch you would only restore the first ALH stream ID. It's got to be done on a DAI-basis, so the flag has to be stored in the dma structure.
There was a problem hiding this comment.
That said, you are right @bardliao that if the DAI is prepared but not triggered, it still needs to be re-initialized. I guess the solution is a mix of yours and mine, we can't use TRIGGER suspend but can't use the .suspend either since each DAI needs to be reinitialized.
|
alternate proposal on https://github.com/thesofproject/linux/pull/1327/commits |
Alternate solution based on @bardliao 's feedback, that seems to deal with suspend-resume, underflows and pause/play
It still see odd messages on the command line so more testing is needed