Re: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the HDA device.

[Gerd Hoffmann]

On Tue, 2017-10-10 at 19:12 +0200, Martin Schrodt wrote:
> Please see
> 
> https://www.reddit.com/r/VFIO/comments/74vokw/improved_pulse_audio_dr
> iver_for_qemu/
> 
> for motivation, and more coverage.
> 
> Changes:
> 
> - Remove PA reader/writer threads from paaudio.c, and do IO from the
> audio timer directly.
> - Add 1 millisecond timers which interface with the HDA device,
> pulling and pushing data
>   to and from it, to enable the guest driver to measure DMA timing by
> just looking the
>   LPIB registers.
> - Expose new configurable settings, such as TLENGTH and FRAGSIZE,
> plus settings to
>   enable PA_STREAM_ADJUST_LATENCY for input and output device
> separately.
> - Fix the input delay when first using the input device.

This needs splitting into smaller patches, probably at least one patch
per change listed here.  Also the description of each change should go
directly into the commit message, not into a link pointing to the web.

> diff --git a/audio/paaudio.c b/audio/paaudio.c
> index 65beb6f010..089af32e4d 100644
> --- a/audio/paaudio.c
> +++ b/audio/paaudio.c
> @@ -1,16 +1,44 @@
> -/* public domain */
> +/*
> + * QEMU ALSA audio driver
           ^^^^ ???

> + *
> + * Copyright (c) 2017 Martin Schrodt (spheenik)
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a copy
> + * of this software and associated documentation files (the
> "Software"), to deal
> + * in the Software without restriction, including without limitation
> the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense,
> and/or sell
> + * copies of the Software, and to permit persons to whom the
> Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS IN
> + * THE SOFTWARE.
> + */

Hmm.  Changing the licence of existing files need agreement from all
past authors.  This looks ok though, seems to be just the long version
of "public domain", so no actual change.  Should be a separate patch.

> +#ifdef PA_STREAM_ADJUST_LATENCY
> +    int adjust_latency_out;
> +    int adjust_latency_in;
> +#endif

Do you know which pa version added this?

If it is old enough we maybe can simply require it and don't need the
ifdefs here.

> +//    if (avail_bytes < max_bytes) {
> +//        dolog("avail: %d, wanted: %d \n", (int)avail_bytes,
> (int)max_bytes);
> +//    }

qemu has support for tracepoints (see docs/devel/tracing.txt) which are
 useful for this kind of debug messages.

> @@ -897,11 +779,37 @@ static void qpa_audio_fini (void *opaque)
>  
>  struct audio_option qpa_options[] = {
>      {
> -        .name  = "SAMPLES",
> +        .name  = "INT_BUF_SIZE",
>          .tag   = AUD_OPT_INT,
> -        .valp  = &glob_conf.samples,
> -        .descr = "buffer size in samples"
> +        .valp  = &glob_conf.buffer_size,
> +        .descr = "internal buffer size in frames"
>      },
> +    {
> +        .name  = "TLENGTH",
> +        .tag   = AUD_OPT_INT,
> +        .valp  = &glob_conf.tlength,
> +        .descr = "playback buffer target length in frames"
> +    },

What is the difference between these two?

> +    {
> +        .name  = "FRAGSIZE",
> +        .tag   = AUD_OPT_INT,
> +        .valp  = &glob_conf.fragsize,
> +        .descr = "fragment length of recording device in frames"
> +    },
> +#ifdef PA_STREAM_ADJUST_LATENCY
> +    {
> +        .name  = "ADJUST_LATENCY_OUT",
> +        .tag   = AUD_OPT_BOOL,
> +        .valp  = &glob_conf.adjust_latency_out,
> +        .descr = "let PA adjust latency for playback device"
> +    },
> +    {
> +        .name  = "ADJUST_LATENCY_IN",
> +        .tag   = AUD_OPT_BOOL,
> +        .valp  = &glob_conf.adjust_latency_in,
> +        .descr = "let PA adjust latency for recording device"
> +    },
> +#endif

What are the effects of enabling/disabling these settings?

> @@ -592,8 +733,9 @@ static const VMStateDescription
> vmstate_hda_audio_stream = {
>          VMSTATE_UINT32(gain_right, HDAAudioStream),
>          VMSTATE_BOOL(mute_left, HDAAudioStream),
>          VMSTATE_BOOL(mute_right, HDAAudioStream),
> -        VMSTATE_UINT32(bpos, HDAAudioStream),
>          VMSTATE_BUFFER(buf, HDAAudioStream),
> +        VMSTATE_INT64(rpos, HDAAudioStream),
> +        VMSTATE_INT64(wpos, HDAAudioStream),
>          VMSTATE_END_OF_LIST()
>      }

Oops.  vmstate changes break live migration.
Must figure something for backward compatibility here.

> -    if (st->ctl & (1 << 26)) {
> -        /*
> -         * Wait with the next DMA xfer until the guest
> -         * has acked the buffer completion interrupt
> -         */
> -        return false;
> -    }
> +//    if (st->ctl & (1 << 26)) {
> +//        /*
> +//         * Wait with the next DMA xfer until the guest
> +//         * has acked the buffer completion interrupt
> +//         */
> +//        return false;
> +//    }

Just drop the code, best as separate patch saying why it isn't needed
(any more) in the commit message.  There is always "git revert" in case
it turns out to be wrong, so no need to keep the code commented out.

cheers,
  Gerd