| Message ID | 20221114113729.1022905-3-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: avs: DSP recovery and resume fixes | expand |
On Mon, 14 Nov 2022 12:37:29 +0100, Cezary Rojewski wrote: > > To improve performance and overall system stability, suspend/resume > operations for ASoC cards always return success status and defer the > actual work. > > Because of that, if a substream fails to resume, userspace may still > attempt to invoke commands on it as from their perspective the operation > completed successfully. Set substream's state to DISCONNECTED to ensure > no further commands are attempted. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> Hm, that might work, but note that, once when the stream is set with the disconnected state, it won't be taken back to the normal state any longer on most sound backends. That is, it'll be gone and won't revive unless you completely unload and reload the whole stuff. If that's the intended behavior, that's fine, of course. So just to make sure. thanks, Takashi
On 2022-11-14 2:02 PM, Takashi Iwai wrote: > On Mon, 14 Nov 2022 12:37:29 +0100, > Cezary Rojewski wrote: >> >> To improve performance and overall system stability, suspend/resume >> operations for ASoC cards always return success status and defer the >> actual work. >> >> Because of that, if a substream fails to resume, userspace may still >> attempt to invoke commands on it as from their perspective the operation >> completed successfully. Set substream's state to DISCONNECTED to ensure >> no further commands are attempted. ... > Hm, that might work, but note that, once when the stream is set with > the disconnected state, it won't be taken back to the normal state any > longer on most sound backends. That is, it'll be gone and won't > revive unless you completely unload and reload the whole stuff. If > that's the intended behavior, that's fine, of course. So just to make > sure. Good point. Our intention: if we fail e.g.: on resume, we would like the framework to invoke ->hw_free() and close us. Right now, if we pretend that everything is okay, invalid actions can be performed on our streams. This all comes down to userspace calling "just" snd_pcm_resume(). If we had an option to opt-in to a _hw_params() + _prepare() + _resume() path, then things would look differently.
On Mon, 14 Nov 2022 15:08:17 +0100, Cezary Rojewski wrote: > > > > On 2022-11-14 2:02 PM, Takashi Iwai wrote: > > On Mon, 14 Nov 2022 12:37:29 +0100, > > Cezary Rojewski wrote: > >> > >> To improve performance and overall system stability, suspend/resume > >> operations for ASoC cards always return success status and defer the > >> actual work. > >> > >> Because of that, if a substream fails to resume, userspace may still > >> attempt to invoke commands on it as from their perspective the operation > >> completed successfully. Set substream's state to DISCONNECTED to ensure > >> no further commands are attempted. > > ... > > > Hm, that might work, but note that, once when the stream is set with > > the disconnected state, it won't be taken back to the normal state any > > longer on most sound backends. That is, it'll be gone and won't > > revive unless you completely unload and reload the whole stuff. If > > that's the intended behavior, that's fine, of course. So just to make > > sure. > > Good point. > > Our intention: if we fail e.g.: on resume, we would like the framework > to invoke ->hw_free() and close us. Right now, if we pretend that > everything is okay, invalid actions can be performed on our > streams. This all comes down to userspace calling "just" > snd_pcm_resume(). If we had an option to opt-in to a _hw_params() + > _prepare() + _resume() path, then things would look differently. So you'd rather like to make the stream appearing and working after re-opening the stream again? Then DISCONNECTED state might be too strong. If the broken state could be recovered by the PCM PREPARE phase, then we may (ab-)use XRUN state, instead. Then application shall re-setup via PCM prepare call. But if hw_free/hw_params is required, it won't work. Other than that, there is no such PCM state that forces to close and re-open, I'm afraid. You can have an internal error state and let the stream returning an error at each operation, instead, too. And, creating a new PCM state is very difficult. It would influence way too much, IMO, as each application code has to be modified to handle the new PCM state. Takashi
On 2022-11-14 3:26 PM, Takashi Iwai wrote: > On Mon, 14 Nov 2022 15:08:17 +0100, > Cezary Rojewski wrote: >> On 2022-11-14 2:02 PM, Takashi Iwai wrote: >>> Hm, that might work, but note that, once when the stream is set with >>> the disconnected state, it won't be taken back to the normal state any >>> longer on most sound backends. That is, it'll be gone and won't >>> revive unless you completely unload and reload the whole stuff. If >>> that's the intended behavior, that's fine, of course. So just to make >>> sure. >> >> Good point. >> >> Our intention: if we fail e.g.: on resume, we would like the framework >> to invoke ->hw_free() and close us. Right now, if we pretend that >> everything is okay, invalid actions can be performed on our >> streams. This all comes down to userspace calling "just" >> snd_pcm_resume(). If we had an option to opt-in to a _hw_params() + >> _prepare() + _resume() path, then things would look differently. > > So you'd rather like to make the stream appearing and working after > re-opening the stream again? Then DISCONNECTED state might be too > strong. > > If the broken state could be recovered by the PCM PREPARE phase, then > we may (ab-)use XRUN state, instead. Then application shall > re-setup via PCM prepare call. But if hw_free/hw_params is required, > it won't work. To resume properly an AudioDSP stream, operations typically found in hw_params() and prepare() need to be re-executed _before_ actual _TRIGGER_RESUME can be called on a stream. Since implementations found in userspace apps such as aplay and pulseaudio first invoke snd_pcm_resume() and then snd_pcm_prepare() if the former fails, one could abuse this flow by doing hw_params-related operations in _resume() and returning a specific error code e.g.: -ESTRPIPE when they succeed (if they fail, the internal error code would be returned instead of course). Prepare tasks are left to snd_pcm_prepare() just like in standard case. I believe such code is a good example of _code smell_ though. First we abuse the error path, second we basically drop the _TRIGGER_RESUME entirely - after all, in such approach it mimics hw_params() and will always fail, either with an internal error code or hardcoded -ESTRPIPE. _TRIGGER_START/STOP would be reused once snd_pcm_prepare() succeeds causing another problem - no differentiation between start and resume and thus lack of information when to or when not to poll DRSM. Thus, we ended up with 'state = DISCONNECTED' if we encounter a firmware issue during PCM-pm. Keeps things sane. > Other than that, there is no such PCM state that forces to close and > re-open, I'm afraid. You can have an internal error state and let the > stream returning an error at each operation, instead, too. > > And, creating a new PCM state is very difficult. It would influence > way too much, IMO, as each application code has to be modified to > handle the new PCM state. Agree, this basically screams not to follow that path. Regards, Czarek
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index ca624fbb5c0d..70442b5212c2 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -934,8 +934,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, rtd = snd_pcm_substream_chip(data->substream); if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { ret = op(dai, data); - if (ret < 0) + if (ret < 0) { + __snd_pcm_set_state(data->substream->runtime, + SNDRV_PCM_STATE_DISCONNECTED); return ret; + } } } @@ -944,8 +947,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, rtd = snd_pcm_substream_chip(data->substream); if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { ret = op(dai, data); - if (ret < 0) + if (ret < 0) { + __snd_pcm_set_state(data->substream->runtime, + SNDRV_PCM_STATE_DISCONNECTED); return ret; + } } } }
To improve performance and overall system stability, suspend/resume operations for ASoC cards always return success status and defer the actual work. Because of that, if a substream fails to resume, userspace may still attempt to invoke commands on it as from their perspective the operation completed successfully. Set substream's state to DISCONNECTED to ensure no further commands are attempted. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- Changes in v2: - __snd_pcm_set_state() replaced direct assignments of PCM state sound/soc/intel/avs/pcm.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)