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

[malc <av1474@comtv.ru>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3044 bytes --]

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 <bjorn@mork.no>
> > ---
> >  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.

-- 
mailto:av1474@comtv.ru