Re: [PATCH] alsaaudio: Set try-poll to false by default

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Monday, March 31, 2025 3:05:24 PM CEST BALATON Zoltan wrote:
> On Sun, 23 Mar 2025, Christian Schoenebeck wrote:
> > On Sunday, March 16, 2025 1:20:46 AM CET BALATON Zoltan wrote:
> >> Quoting Volker Rümelin: "try-poll=on tells the ALSA backend to try to
> >> use an event loop instead of the audio timer. This works most of the
> >> time. But the poll event handler in the ALSA backend has a bug. For
> >> example, if the guest can't provide enough audio frames in time, the
> >> ALSA buffer is only partly full and the event handler will be called
> >> again and again on every iteration of the main loop. This increases
> >> the processor load and the guest has less processor time to provide
> >> new audio frames in time. I have two examples where a guest can't
> >> recover from this situation and the guest seems to hang."
> >>
> >> One reproducer I've found is booting MorphOS demo iso on
> >> qemu-system-ppc -machine pegasos2 -audio alsa which should play a
> >> startup sound but instead it freezes. Even when it does not hang it
> >> plays choppy sound. Volker suggested using command line to set
> >> try-poll=off saying: "The try-poll=off arguments are typically
> >> necessary, because the alsa backend has a design issue with
> >> try-poll=on. If the guest can't provide enough audio frames, it's
> >> really unhelpful to ask for new audio frames on every main loop
> >> iteration until the guest can provide enough audio frames. Timer based
> >> playback doesn't have that problem."
> >>
> >> But users cannot easily find this option and having a non-working
> >> default is really unhelpful so to make life easier just set it to
> >> false by default which works until the issue with the alsa backend can
> >> be fixed.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >> This fixes my issue but if somebody has a better fix I'm open to that
> >> too.
> >>
> >>  audio/alsaaudio.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
> >> index cacae1ea59..9b6c01c0ef 100644
> >> --- a/audio/alsaaudio.c
> >> +++ b/audio/alsaaudio.c
> >> @@ -899,7 +899,7 @@ static void alsa_enable_in(HWVoiceIn *hw, bool enable)
> >>  static void alsa_init_per_direction(AudiodevAlsaPerDirectionOptions *apdo)
> >>  {
> >>      if (!apdo->has_try_poll) {
> >> -        apdo->try_poll = true;
> >> +        apdo->try_poll = false;
> >>          apdo->has_try_poll = true;
> >>      }
> >>  }
> >>
> >
> > Correct me if I am wrong, but AFAICS if polling is not used then no state
> > changes would be handled, no? At least I don't see any snd_pcm_state() call
> > outside of alsa_poll_handler().
> 
> I have no idea but this fixes the problem (and does the same that can be 
> also done from command line but nobody can find that command line option) 
> so unless somebody has a better idea could this be merged as a fix for 
> now?

Well, I understand that if fixes the misbehaviour you encountered. But how
helpful would it be if it then breaks behaviour for other people instead?

I think it would be better to add a 2nd patch that would handle state changes
in callback mode. That would satisfy both groups of people. AFAICS
snd_pcm_state() can be called both in polling mode and callback mode.

Just my 2 cents. Of course it's up to Gerd to decide.

/Christian