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

[Rob Landley]

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?

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds