| Message ID | 20221114113729.1022905-3-cezary.rojewski@intel.com |
|---|---|
| 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
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(-)