snd_usb_pcm only restores the DAC to D1 state, not D0 state, after resuming from a suspend. Userspace sees a state of SND_PCM_STATE_SUSPENDED, which it will try to resume through snd_pcm_prepare (snd_pcm_resume is -ENOSYS). However, snd_pcm_prepare does not trigger the D0 state transition, leading to the audio device not functioning. More detailed symptom at [1]. The DAC is only kicked out of this state after a call to hw_params, which triggers a transition to D0 state. The attached patch hacks so that the DAC enters D0 state right after resume, which I have tested and fixes the problem. [1]: https://gitlab.freedesktop.org/pipewire/pipewire/-/issues/2707
Created attachment 304395 [details] Hack to confirm that PM is the root cause
So, simply changing the power state at prepare fixes the issue? --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -650,6 +650,10 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) goto unlock; } + ret = snd_usb_pcm_change_state(subs, UAC3_PD_STATE_D0); + if (ret < 0) + goto unlock; + again: if (subs->sync_endpoint) { ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint); hw_params isn't called in a usual resume path, hence the current code failed.
Looks good to me, will test later.
Patch in comment #2 works fine, thanks. Additionally it seems we can remove this call from hw_params since all the setup URBs are flushed only once prepare is called. diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index eec5232f9fb2..b7428a23647e 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -520,10 +520,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, if (ret < 0) goto stop_pipeline; - ret = snd_usb_pcm_change_state(subs, UAC3_PD_STATE_D0); - if (ret < 0) - goto unlock; - if (subs->data_endpoint) { if (snd_usb_endpoint_compatible(chip, subs->data_endpoint, fmt, hw_params))
Well, that's another code path, and in theory, application can issue the hw_params step before the prepare step, and that'll hit a similar problem. So it's still safer to keep the power state change code there, too. We may optimize the power state change by caching the value and checking it before actually executing the USB command, if it really matters, too.
> Well, that's another code path, and in theory, application can issue the > hw_params step before the prepare step, and that'll hit a similar problem. > So it's still safer to keep the power state change code there, too. A call to hw_params() put the stream in SND_PCM_STATE_SETUP state, and this needs to always be followed by a prepare() in order to proceed to starting the stream. Hence, I don't see how we will "hit a similar problem". > We may optimize the power state change by caching the value and checking it > before actually executing the USB command, if it really matters, too. Looking at debug logs it seems it already avoids redundant state changes. So this is more or less a code flow clarity thing. I don't really mind keeping the code if you want to be absolutely safe, so up to you.
(In reply to Tatsuyuki Ishi from comment #6) > > Well, that's another code path, and in theory, application can issue the > > hw_params step before the prepare step, and that'll hit a similar problem. > > So it's still safer to keep the power state change code there, too. > > A call to hw_params() put the stream in SND_PCM_STATE_SETUP state, and this > needs to always be followed by a prepare() in order to proceed to starting > the stream. Hence, I don't see how we will "hit a similar problem". In general, hw_params does set up the hardware parameters, and it implies that the power gets turned on at this stage. Although USB-audio driver tries to avoid touching the hardware parameters at hw_parms call as much as possible, it'd be error-prone to leave in an unknown power state. So yes, I'd like to be in the safer side and keep the power up explicitly there on purpose.
The fix submitted to the upstream: https://lore.kernel.org/all/20230612132818.29486-1-tiwai@suse.de/
Fix live with 6.3.9 or 6.4.