Re: [Qemu-devel] [PATCH] alsa broken when the host is suspended (or hibernated)

[malc]

On Wed, 29 Jul 2009, malc wrote:

> On Wed, 29 Jul 2009, Rob Landley wrote:
> 
> > On Wednesday 29 July 2009 05:51:06 BjЪЪrn Mork wrote:
> > > audio output fails after resuming a host running a guest using alsa
> > > audio output. Messages such as
> > >
> > >  alsa: Failed to write 882 frames to 0x1804b98
> > >  alsa: Reason: Streams pipe error
> > >
> > > will appear repeatedly in the monitor.  This is caused by alsaaudio.c
> > > not handling ESTRPIPE.  Fix this by calling snd_pcm_resume() on
> > > ESTRPIPE.
> > >
> > > This bug is similar to the vlc bug discussed on
> > > https://trac.videolan.org/vlc/ticket/1286 and the fix is insired by
> > > the patch attached to that bug report
> > >
> > > Signed-off-by: BjЪЪrn Mork <address@hidden>
> > > ---
> > >  audio/alsaaudio.c |   22 ++++++++++++++++++++++
> > >  1 files changed, 22 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/audio/alsaaudio.c b/audio/alsaaudio.c
> > > index d0b7cd0..1c007e5 100644
> > > --- a/audio/alsaaudio.c
> > > +++ b/audio/alsaaudio.c
> > > @@ -503,6 +503,16 @@ static int alsa_recover (snd_pcm_t *handle)
> > >      return 0;
> > >  }
> > >
> > > +static int alsa_resume (snd_pcm_t *handle)
> > > +{
> > > +    int err = snd_pcm_resume (handle);
> > > +    if (err < 0) {
> > > +        alsa_logerr (err, "Failed to resume handle %p\n", handle);
> > > +        return -1;
> > > +    }
> > > +    return 0;
> > > +}
> > > +
> > >  static snd_pcm_sframes_t alsa_get_avail (snd_pcm_t *handle)
> > >  {
> > >      snd_pcm_sframes_t avail;
> > > @@ -580,6 +590,18 @@ static int alsa_run_out (HWVoiceOut *hw)
> > >                      }
> > >                      continue;
> > >
> > > +         case -ESTRPIPE:
> > > +             /* stream is suspended and waiting for an application 
> > > recovery */
> > > +             if (alsa_resume (alsa->handle)) {
> > > +                        alsa_logerr (written, "Failed to write %d
> > > frames\n", +                                     len);
> > > +                        goto exit;
> > > +                    }
> > > +                    if (conf.verbose) {
> > > +                        dolog ("Resuming suspended stream\n");
> > > +                    }
> > > +                    continue;
> > > +
> > >                  case -EAGAIN:
> > >                      goto exit;
> > 
> > Is there a reason the whole patch couldn't just be something like:
> > 
> > +    case -ESTRPIPE:
> > +        /* manually recover after suspend/resume */
> > +        if (snd_pcm_resume(handle) < 0) {
> > +            alsa_logerr(written, "Failed to resume handle %p", handle);
> > +            goto exit;
> > +        } else if (conf.verbose) dolog("Resuming suspended stream\n");
> > +        continue;
> > 
> > Why are you wrapping a single function call, and adding logging and error 
> > recovery both in that wrapper and in the only caller of that wrapper?
> 
> Because that's how it's done when handling xruns etc, i.e. consistent
> with the rest of the code around it. Verbose is what's there for
> users to set and to report the output when asked for it.
> 
> And snd_pcm_resume is not a wrapper it's an ALSA function.

Uh have to apologize the patch that you are replying to does indeed
intorduce a wrapper, but i haven't seen/received it up until now, so
sorry.

-- 
mailto:address@hidden