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

2017-10-11 Thread Paolo Bonzini 
On 10/10/2017 20:32, Martin Schrodt wrote:
> These 3 here are only paaudio.c
> 
> - Remove PA reader/writer threads from paaudio.c, and do IO from the
> audio timer directly.
> - 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.
> 
> and cannot really be separated, unfortunately.

Sure they can! :)  This can actually be a good occasion to learn more of
git.  Say you have this large patch in branch qemu-audio-v1, you can now do

   git checkout -b qemu-audio-v2 qemu-audio-v1^
   git checkout qemu-audio-v1 -- .
   git reset

This will create a branch *without* the patch, while putting the files
*with* the patch in your working tree.

Now you can add your changes gradually to the staging area of git (it is
called the "index"):

  git add -p

This command will ask you interactively about staging each separate
hunk, for files that git already tracks.  When done, you can run

  git diff --staged

to show the diff that is staged for commit.  And also

  git diff

to show the diff that is not staged yet.

If you are happy with the staged changes, run:

  git stash --keep-index

and test them.  If there is a problem, fix it.  Then type

  git commit -a -s

to commit the staged changes to your local branch called qemu-audio-v2
(write a good commit message now that the commit is fresh in your mind!).

Now get back to the full changes:

 git checkout qemu-audio-v1 -- .
 git reset

and restart from the "git add -p" step to get the next part of your
patch.  Remember, each single one of your patches must build and runs
(implementing the next logical step in the feature), and at the last
patch, the feature is complete.

Now format the patches as email messages, and send them to the list.
Standing in the root of your QEMU directory, run the following:

git fetch origin
rm -f -- *.patch
git format-patch --cover-letter --subject-prefix="PATCH v2" \
  origin/master..

This command will generate an email file for each one of your commits.
The patch files will be numbered. The file numbered  is a cover
letter, which you should edit in-place:

* in the Subject: line, summarize the goal of your series,

* in the message body, describe the changes on a broad level.

Now it's time to mail-bomb the list.  You have already used git
send-email here so I won't get into further detail.

(Loosely based on
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers)

Thanks,

Paolo

> The third one is really only a consequence of the removal of the
> additional audio thread.

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

2017-10-11 Thread 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  = _conf.samples,
> -.descr = "buffer size in samples"
> +.valp  = _conf.buffer_size,
> +.descr = "internal buffer size in frames"
>  },
> +{
> +.name  = "TLENGTH",
> +.tag   = AUD_OPT_INT,
> +.valp  = _conf.tlength,
> +.descr = "playback buffer target length in frames"
> +},

What is the difference between these two?

> +{
> +.name  = "FRAGSIZE",
> +.tag   = AUD_OPT_INT,
> +.valp  = _conf.fragsize,
> +.descr = "fragment length of recording device in frames"
> +},
> +#ifdef PA_STREAM_ADJUST_LATENCY
> +{
> +.name  = "ADJUST_LATENCY_OUT",
> +.tag   = AUD_OPT_BOOL,
> +.valp  = _conf.adjust_latency_out,
> +.descr = "let PA adjust latency for playback device"
> +},
> +{
> +.name  = "ADJUST_LATENCY_IN",
> +.tag   = AUD_OPT_BOOL,
> +.valp  = _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

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

2017-10-10 Thread no-reply 
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20171010174538.25074-1-q...@martin.schrodt.org
Subject: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the 
HDA device.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
11d17311c9 Several fixes for the Pulse Audio driver, and the HDA device.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: Several fixes for the Pulse Audio driver, and the HDA 
device
ERROR: open brace '{' following function declarations go on the next line
#34: FILE: audio/audio.c:2070:
+int64_t audio_get_timer_ticks(void) {

ERROR: space prohibited between function name and open parenthesis '('
#146: FILE: audio/paaudio.c:106:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#161: FILE: audio/paaudio.c:118:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#266: FILE: audio/paaudio.c:126:
+static int qpa_run_out (HWVoiceOut *hw, int live)

ERROR: space prohibited between function name and open parenthesis '('
#286: FILE: audio/paaudio.c:139:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#287: FILE: audio/paaudio.c:140:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: space prohibited between function name and open parenthesis '('
#300: FILE: audio/paaudio.c:146:
+samples = (int)(audio_MIN (avail_bytes, max_bytes)) >> hw->info.shift;

ERROR: do not use C99 // comments
#306: FILE: audio/paaudio.c:148:
+//if (avail_bytes < max_bytes) {

ERROR: unnecessary whitespace before a quoted newline
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#308: FILE: audio/paaudio.c:150:
+//}

ERROR: line over 90 characters
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: do not use C99 // comments
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: space prohibited between function name and open parenthesis '('
#324: FILE: audio/paaudio.c:157:
+int convert_samples = audio_MIN (samples, left_till_end_samples);

WARNING: line over 80 characters
#325: FILE: audio/paaudio.c:158:
+size_t convert_bytes_wanted = (size_t) convert_samples << 
hw->info.shift;

WARNING: line over 80 characters
#331: FILE: audio/paaudio.c:163:
+CHECK_SUCCESS_GOTO(pa->g, , convert_bytes == 
convert_bytes_wanted, fail);

ERROR: space prohibited between function name and open parenthesis '('
#339: FILE: audio/paaudio.c:166:
+hw->clip (pa_dst, src, convert_samples);

ERROR: line over 90 characters
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#369: FILE: audio/paaudio.c:177:
+pa_threaded_mainloop_unlock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#389: FILE: audio/paaudio.c:183:
+qpa_logerr (error, "qpa_run_out failed\n");

ERROR: space prohibited between function name and open parenthesis '('
#400: FILE: audio/paaudio.c:192:
+static int qpa_run_in (HWVoiceIn *hw)

ERROR: space prohibited between function name and open parenthesis '('
#415: FILE: audio/paaudio.c:203:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#416: FILE: audio/paaudio.c:204:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: line over 90 characters
#418: FILE: audio/paaudio.c:206:
+size_t bytes_wanted = ((unsigned int)(hw->samples - 
audio_pcm_hw_get_live_in(hw)) <<

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

2017-10-10 Thread no-reply 
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20171010174403.24660-1-mar...@schrodt.org
Subject: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the 
HDA device.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
e49f94ef7b Several fixes for the Pulse Audio driver, and the HDA device.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: Several fixes for the Pulse Audio driver, and the HDA 
device
ERROR: open brace '{' following function declarations go on the next line
#34: FILE: audio/audio.c:2070:
+int64_t audio_get_timer_ticks(void) {

ERROR: space prohibited between function name and open parenthesis '('
#146: FILE: audio/paaudio.c:106:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#161: FILE: audio/paaudio.c:118:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#266: FILE: audio/paaudio.c:126:
+static int qpa_run_out (HWVoiceOut *hw, int live)

ERROR: space prohibited between function name and open parenthesis '('
#286: FILE: audio/paaudio.c:139:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#287: FILE: audio/paaudio.c:140:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: space prohibited between function name and open parenthesis '('
#300: FILE: audio/paaudio.c:146:
+samples = (int)(audio_MIN (avail_bytes, max_bytes)) >> hw->info.shift;

ERROR: do not use C99 // comments
#306: FILE: audio/paaudio.c:148:
+//if (avail_bytes < max_bytes) {

ERROR: unnecessary whitespace before a quoted newline
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#308: FILE: audio/paaudio.c:150:
+//}

ERROR: line over 90 characters
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: do not use C99 // comments
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: space prohibited between function name and open parenthesis '('
#324: FILE: audio/paaudio.c:157:
+int convert_samples = audio_MIN (samples, left_till_end_samples);

WARNING: line over 80 characters
#325: FILE: audio/paaudio.c:158:
+size_t convert_bytes_wanted = (size_t) convert_samples << 
hw->info.shift;

WARNING: line over 80 characters
#331: FILE: audio/paaudio.c:163:
+CHECK_SUCCESS_GOTO(pa->g, , convert_bytes == 
convert_bytes_wanted, fail);

ERROR: space prohibited between function name and open parenthesis '('
#339: FILE: audio/paaudio.c:166:
+hw->clip (pa_dst, src, convert_samples);

ERROR: line over 90 characters
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#369: FILE: audio/paaudio.c:177:
+pa_threaded_mainloop_unlock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#389: FILE: audio/paaudio.c:183:
+qpa_logerr (error, "qpa_run_out failed\n");

ERROR: space prohibited between function name and open parenthesis '('
#400: FILE: audio/paaudio.c:192:
+static int qpa_run_in (HWVoiceIn *hw)

ERROR: space prohibited between function name and open parenthesis '('
#415: FILE: audio/paaudio.c:203:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#416: FILE: audio/paaudio.c:204:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: line over 90 characters
#418: FILE: audio/paaudio.c:206:
+size_t bytes_wanted = ((unsigned int)(hw->samples - 
audio_pcm_hw_get_live_in(hw)) << hw->info.shift);

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

2017-10-10 Thread no-reply 
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20171010171218.14298-1-mar...@schrodt.org
Subject: [Qemu-devel] [PATCH] Several fixes for the Pulse Audio driver, and the 
HDA device.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
14f6aeab34 Several fixes for the Pulse Audio driver, and the HDA device.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: Several fixes for the Pulse Audio driver, and the HDA 
device
ERROR: open brace '{' following function declarations go on the next line
#34: FILE: audio/audio.c:2070:
+int64_t audio_get_timer_ticks(void) {

ERROR: space prohibited between function name and open parenthesis '('
#146: FILE: audio/paaudio.c:106:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#161: FILE: audio/paaudio.c:118:
+*(rerror) = pa_context_errno ((c)->context);\

ERROR: space prohibited between function name and open parenthesis '('
#266: FILE: audio/paaudio.c:126:
+static int qpa_run_out (HWVoiceOut *hw, int live)

ERROR: space prohibited between function name and open parenthesis '('
#286: FILE: audio/paaudio.c:139:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#287: FILE: audio/paaudio.c:140:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: space prohibited between function name and open parenthesis '('
#300: FILE: audio/paaudio.c:146:
+samples = (int)(audio_MIN (avail_bytes, max_bytes)) >> hw->info.shift;

ERROR: do not use C99 // comments
#306: FILE: audio/paaudio.c:148:
+//if (avail_bytes < max_bytes) {

ERROR: unnecessary whitespace before a quoted newline
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#307: FILE: audio/paaudio.c:149:
+//dolog("avail: %d, wanted: %d \n", (int)avail_bytes, (int)max_bytes);

ERROR: do not use C99 // comments
#308: FILE: audio/paaudio.c:150:
+//}

ERROR: line over 90 characters
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: do not use C99 // comments
#312: FILE: audio/paaudio.c:152:
+//dolog("TRANSFER avail: %d bytes, max %d bytes -> %d samples from %d\n", 
(int)avail_bytes, (int)max_bytes, samples, rpos);

ERROR: space prohibited between function name and open parenthesis '('
#324: FILE: audio/paaudio.c:157:
+int convert_samples = audio_MIN (samples, left_till_end_samples);

WARNING: line over 80 characters
#325: FILE: audio/paaudio.c:158:
+size_t convert_bytes_wanted = (size_t) convert_samples << 
hw->info.shift;

WARNING: line over 80 characters
#331: FILE: audio/paaudio.c:163:
+CHECK_SUCCESS_GOTO(pa->g, , convert_bytes == 
convert_bytes_wanted, fail);

ERROR: space prohibited between function name and open parenthesis '('
#339: FILE: audio/paaudio.c:166:
+hw->clip (pa_dst, src, convert_samples);

ERROR: line over 90 characters
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#348: FILE: audio/paaudio.c:168:
+r = pa_stream_write (pa->stream, pa_dst, convert_bytes, NULL, 0LL, 
PA_SEEK_RELATIVE);

ERROR: space prohibited between function name and open parenthesis '('
#369: FILE: audio/paaudio.c:177:
+pa_threaded_mainloop_unlock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#389: FILE: audio/paaudio.c:183:
+qpa_logerr (error, "qpa_run_out failed\n");

ERROR: space prohibited between function name and open parenthesis '('
#400: FILE: audio/paaudio.c:192:
+static int qpa_run_in (HWVoiceIn *hw)

ERROR: space prohibited between function name and open parenthesis '('
#415: FILE: audio/paaudio.c:203:
+pa_threaded_mainloop_lock (pa->g->mainloop);

ERROR: space prohibited between function name and open parenthesis '('
#416: FILE: audio/paaudio.c:204:
+CHECK_DEAD_GOTO (pa->g, pa->stream, , fail);

ERROR: line over 90 characters
#418: FILE: audio/paaudio.c:206:
+size_t bytes_wanted = ((unsigned int)(hw->samples - 
audio_pcm_hw_get_live_in(hw)) << hw->info.shift);

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

2017-10-10 Thread Martin Schrodt 
On 10/10/2017 08:18 PM, Eric Blake wrote:

> That's a lot of changes to be slamming in one patch.  Any chance you can
> split it into a series of smaller patches that are easier to review
> individually?  Perhaps one patch per item in your bulleted list is a
> good start for subdividing this into something that is not so massive.

I could make a separate request with all my commits separated.

The patch is all these squashed:

https://github.com/qemu/qemu/compare/master...spheenik:master

Problem is, and that's why I decided to hand it over like this, is that
the commits are kind of a log of my learning experience, getting to
this, and so not really separated better.

These 3 here are only paaudio.c

- Remove PA reader/writer threads from paaudio.c, and do IO from the
audio timer directly.
- 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.

and cannot really be separated, unfortunately.
The third one is really only a consequence of the removal of the
additional audio thread.

And this one:

- 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.

is hda-codec.c.

Hope that helps,

Martin




signature.asc
Description: OpenPGP digital signature

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

2017-10-10 Thread Eric Blake 
On 10/10/2017 12:44 PM, Martin Schrodt wrote:
> Please see
> 
> https://www.reddit.com/r/VFIO/comments/74vokw/improved_pulse_audio_driver_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.

That's a lot of changes to be slamming in one patch.  Any chance you can
split it into a series of smaller patches that are easier to review
individually?  Perhaps one patch per item in your bulleted list is a
good start for subdividing this into something that is not so massive.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2017-10-10 Thread Martin Schrodt 
Please see

https://www.reddit.com/r/VFIO/comments/74vokw/improved_pulse_audio_driver_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.

Signed-off-by: Martin Schrodt 
---
 audio/audio.c |   4 +
 audio/audio_int.h |   2 +
 audio/paaudio.c   | 640 --
 hw/audio/hda-codec.c  | 218 +---
 hw/audio/intel-hda-defs.h |   1 +
 hw/audio/intel-hda.c  |  14 +-
 6 files changed, 468 insertions(+), 411 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index beafed209b..fba1604c34 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2066,3 +2066,7 @@ void AUD_set_volume_in (SWVoiceIn *sw, int mute, uint8_t 
lvol, uint8_t rvol)
 }
 }
 }
+
+int64_t audio_get_timer_ticks(void) {
+return conf.period.ticks;
+}
diff --git a/audio/audio_int.h b/audio/audio_int.h
index 5bcb1c60e1..2f7fc4f8ac 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -214,6 +214,8 @@ extern struct audio_driver pa_audio_driver;
 extern struct audio_driver spice_audio_driver;
 extern const struct mixeng_volume nominal_volume;
 
+int64_t audio_get_timer_ticks(void);
+
 void audio_pcm_init_info (struct audio_pcm_info *info, struct audsettings *as);
 void audio_pcm_info_clear_buf (struct audio_pcm_info *info, void *buf, int 
len);
 
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.
+ */
 #include "qemu/osdep.h"
-#include "qemu-common.h"
 #include "audio.h"
 
+
 #include 
+#include 
 
 #define AUDIO_CAP "pulseaudio"
 #include "audio_int.h"
-#include "audio_pt_int.h"
 
 typedef struct {
-int samples;
+int buffer_size;
+int tlength;
+int fragsize;
+#ifdef PA_STREAM_ADJUST_LATENCY
+int adjust_latency_out;
+int adjust_latency_in;
+#endif
 char *server;
 char *sink;
 char *source;
@@ -24,30 +52,21 @@ typedef struct {
 
 typedef struct {
 HWVoiceOut hw;
-int done;
-int live;
-int decr;
-int rpos;
 pa_stream *stream;
-void *pcm_buf;
-struct audio_pt pt;
 paaudio *g;
+pa_sample_spec ss;
+pa_buffer_attr ba;
 } PAVoiceOut;
 
 typedef struct {
 HWVoiceIn hw;
-int done;
-int dead;
-int incr;
-int wpos;
 pa_stream *stream;
-void *pcm_buf;
-struct audio_pt pt;
-const void *read_data;
-size_t read_index, read_length;
 paaudio *g;
+pa_sample_spec ss;
+pa_buffer_attr ba;
 } PAVoiceIn;
 
+
 static void qpa_audio_fini(void *opaque);
 
 static void GCC_FMT_ATTR (2, 3) qpa_logerr (int err, const char *fmt, ...)
@@ -84,207 +103,85 @@ static inline int PA_STREAM_IS_GOOD(pa_stream_state_t x)
 #define CHECK_SUCCESS_GOTO(c, rerror, expression, label)\
 do {\
 if (!(expression)) {\
-if (rerror) {   \
-*(rerror) = pa_context_errno ((c)->context);\
-}   \
+*(rerror) = pa_context_errno ((c)->context);\
 goto label; \
 }

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

2017-10-10 Thread qemu 
From: Martin Schrodt 

Please see

https://www.reddit.com/r/VFIO/comments/74vokw/improved_pulse_audio_driver_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.

Signed-off-by: Martin Schrodt 
---
 audio/audio.c |   4 +
 audio/audio_int.h |   2 +
 audio/paaudio.c   | 640 --
 hw/audio/hda-codec.c  | 218 +---
 hw/audio/intel-hda-defs.h |   1 +
 hw/audio/intel-hda.c  |  14 +-
 6 files changed, 468 insertions(+), 411 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index beafed209b..fba1604c34 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2066,3 +2066,7 @@ void AUD_set_volume_in (SWVoiceIn *sw, int mute, uint8_t 
lvol, uint8_t rvol)
 }
 }
 }
+
+int64_t audio_get_timer_ticks(void) {
+return conf.period.ticks;
+}
diff --git a/audio/audio_int.h b/audio/audio_int.h
index 5bcb1c60e1..2f7fc4f8ac 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -214,6 +214,8 @@ extern struct audio_driver pa_audio_driver;
 extern struct audio_driver spice_audio_driver;
 extern const struct mixeng_volume nominal_volume;
 
+int64_t audio_get_timer_ticks(void);
+
 void audio_pcm_init_info (struct audio_pcm_info *info, struct audsettings *as);
 void audio_pcm_info_clear_buf (struct audio_pcm_info *info, void *buf, int 
len);
 
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.
+ */
 #include "qemu/osdep.h"
-#include "qemu-common.h"
 #include "audio.h"
 
+
 #include 
+#include 
 
 #define AUDIO_CAP "pulseaudio"
 #include "audio_int.h"
-#include "audio_pt_int.h"
 
 typedef struct {
-int samples;
+int buffer_size;
+int tlength;
+int fragsize;
+#ifdef PA_STREAM_ADJUST_LATENCY
+int adjust_latency_out;
+int adjust_latency_in;
+#endif
 char *server;
 char *sink;
 char *source;
@@ -24,30 +52,21 @@ typedef struct {
 
 typedef struct {
 HWVoiceOut hw;
-int done;
-int live;
-int decr;
-int rpos;
 pa_stream *stream;
-void *pcm_buf;
-struct audio_pt pt;
 paaudio *g;
+pa_sample_spec ss;
+pa_buffer_attr ba;
 } PAVoiceOut;
 
 typedef struct {
 HWVoiceIn hw;
-int done;
-int dead;
-int incr;
-int wpos;
 pa_stream *stream;
-void *pcm_buf;
-struct audio_pt pt;
-const void *read_data;
-size_t read_index, read_length;
 paaudio *g;
+pa_sample_spec ss;
+pa_buffer_attr ba;
 } PAVoiceIn;
 
+
 static void qpa_audio_fini(void *opaque);
 
 static void GCC_FMT_ATTR (2, 3) qpa_logerr (int err, const char *fmt, ...)
@@ -84,207 +103,85 @@ static inline int PA_STREAM_IS_GOOD(pa_stream_state_t x)
 #define CHECK_SUCCESS_GOTO(c, rerror, expression, label)\
 do {\
 if (!(expression)) {\
-if (rerror) {   \
-*(rerror) = pa_context_errno ((c)->context);\
-}   \
+*(rerror) = pa_context_errno ((c)->context);\
 goto label;

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

2017-10-10 Thread Martin Schrodt 
Please see

https://www.reddit.com/r/VFIO/comments/74vokw/improved_pulse_audio_driver_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.

Signed-off-by: Martin Schrodt 
---
 audio/audio.c |   4 +
 audio/audio_int.h |   2 +
 audio/paaudio.c   | 640 --
 hw/audio/hda-codec.c  | 218 +---
 hw/audio/intel-hda-defs.h |   1 +
 hw/audio/intel-hda.c  |  14 +-
 6 files changed, 468 insertions(+), 411 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index beafed209b..fba1604c34 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2066,3 +2066,7 @@ void AUD_set_volume_in (SWVoiceIn *sw, int mute, uint8_t 
lvol, uint8_t rvol)
 }
 }
 }
+
+int64_t audio_get_timer_ticks(void) {
+return conf.period.ticks;
+}
diff --git a/audio/audio_int.h b/audio/audio_int.h
index 5bcb1c60e1..2f7fc4f8ac 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -214,6 +214,8 @@ extern struct audio_driver pa_audio_driver;
 extern struct audio_driver spice_audio_driver;
 extern const struct mixeng_volume nominal_volume;
 
+int64_t audio_get_timer_ticks(void);
+
 void audio_pcm_init_info (struct audio_pcm_info *info, struct audsettings *as);
 void audio_pcm_info_clear_buf (struct audio_pcm_info *info, void *buf, int 
len);
 
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.
+ */
 #include "qemu/osdep.h"
-#include "qemu-common.h"
 #include "audio.h"
 
+
 #include 
+#include 
 
 #define AUDIO_CAP "pulseaudio"
 #include "audio_int.h"
-#include "audio_pt_int.h"
 
 typedef struct {
-int samples;
+int buffer_size;
+int tlength;
+int fragsize;
+#ifdef PA_STREAM_ADJUST_LATENCY
+int adjust_latency_out;
+int adjust_latency_in;
+#endif
 char *server;
 char *sink;
 char *source;
@@ -24,30 +52,21 @@ typedef struct {
 
 typedef struct {
 HWVoiceOut hw;
-int done;
-int live;
-int decr;
-int rpos;
 pa_stream *stream;
-void *pcm_buf;
-struct audio_pt pt;
 paaudio *g;
+pa_sample_spec ss;
+pa_buffer_attr ba;
 } PAVoiceOut;
 
 typedef struct {
 HWVoiceIn hw;
-int done;
-int dead;
-int incr;
-int wpos;
 pa_stream *stream;
-void *pcm_buf;
-struct audio_pt pt;
-const void *read_data;
-size_t read_index, read_length;
 paaudio *g;
+pa_sample_spec ss;
+pa_buffer_attr ba;
 } PAVoiceIn;
 
+
 static void qpa_audio_fini(void *opaque);
 
 static void GCC_FMT_ATTR (2, 3) qpa_logerr (int err, const char *fmt, ...)
@@ -84,207 +103,85 @@ static inline int PA_STREAM_IS_GOOD(pa_stream_state_t x)
 #define CHECK_SUCCESS_GOTO(c, rerror, expression, label)\
 do {\
 if (!(expression)) {\
-if (rerror) {   \
-*(rerror) = pa_context_errno ((c)->context);\
-}   \
+*(rerror) = pa_context_errno ((c)->context);\
 goto label; \
 }