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