[Qemu-devel] [PATCH 0/4] audio/hda: improve windows guest audio quality.

[Qemu-devel] [PATCH 0/4] audio/hda: improve windows guest audio quality.

[Gerd Hoffmann]

Based on a patch by Martin Schrodt.

Gerd Hoffmann (4):
  audio/hda: create millisecond timers that handle IO
  audio/hda: turn some dprintfs into trace points
  audio/hda: tweak timer adjust logic
  audio/hda: detect output buffer overruns and underruns

 include/hw/compat.h   |   7 ++
 hw/audio/hda-codec.c  | 300 ++++++++++++++++++++++++++++++++++++++++++++------
 hw/audio/intel-hda.c  |   7 --
 hw/audio/trace-events |   7 ++
 4 files changed, 283 insertions(+), 38 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/4] audio/hda: create millisecond timers that handle IO

[Gerd Hoffmann]

Currently, the HDA device tries to sync itself with the QEMU audio
backend by waiting for the guest driver to handle buffer completion
interrupts. This causes the backend to often read too much data from the
device, as well as running out of data whenever the guest takes too long
to handle the interrupt.

According to the HDA specification, the guest is also not required to
use interrupts, but can also sync itself by polling the LPIB registers.

This patch will introduce high frequency (1000Hz) timers that interface
with the device and allow for much smoother emulation of the LPIB
registers. Since the timing is now provided by these timers, the need
to wait for buffer completion interrupts also ceases.

Signed-off-by: Martin Schrodt <martin@schrodt.org>
Message-id: 20171015184033.2951-3-martin@schrodt.org

[ kraxel: keep old code for compatibility with older qemu versions,
          add property to switch code paths at runtime ]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/compat.h  |   7 ++
 hw/audio/hda-codec.c | 263 ++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/audio/intel-hda.c |   7 --
 3 files changed, 244 insertions(+), 33 deletions(-)

diff --git a/include/hw/compat.h b/include/hw/compat.h
index 13242b831a..dced6d81bf 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -1,6 +1,13 @@
 #ifndef HW_COMPAT_H
 #define HW_COMPAT_H
 
+#define HW_COMPAT_2_12 \
+    {\
+        .driver   = "hda-audio",\
+        .property = "use-timer",\
+        .value    = "false",\
+    },
+
 #define HW_COMPAT_2_11 \
     {\
         .driver   = "hpet",\
diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index e8aa7842e6..9ee2f3d55f 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/atomic.h"
 #include "hw/hw.h"
 #include "hw/pci/pci.h"
 #include "intel-hda.h"
@@ -126,6 +127,11 @@ static void hda_codec_parse_fmt(uint32_t format, struct audsettings *as)
 #define   PARAM nomixemu
 #include  "hda-codec-common.h"
 
+#define HDA_TIMER_TICKS (SCALE_MS)
+#define MAX_CORR (SCALE_US * 100)
+#define B_SIZE sizeof(st->buf)
+#define B_MASK (sizeof(st->buf) - 1)
+
 /* -------------------------------------------------------------------------- */
 
 static const char *fmt2name[] = {
@@ -154,8 +160,13 @@ struct HDAAudioStream {
         SWVoiceIn *in;
         SWVoiceOut *out;
     } voice;
-    uint8_t buf[HDA_BUFFER_SIZE];
-    uint32_t bpos;
+    uint8_t compat_buf[HDA_BUFFER_SIZE];
+    uint32_t compat_bpos;
+    uint8_t buf[8192]; /* size must be power of two */
+    int64_t rpos;
+    int64_t wpos;
+    QEMUTimer *buft;
+    int64_t buft_start;
 };
 
 #define TYPE_HDA_AUDIO "hda-audio"
@@ -174,55 +185,201 @@ struct HDAAudioState {
     /* properties */
     uint32_t debug;
     bool     mixer;
+    bool     use_timer;
 };
 
+static inline int64_t hda_bytes_per_second(HDAAudioStream *st)
+{
+    return 2 * st->as.nchannels * st->as.freq;
+}
+
+static inline void hda_timer_sync_adjust(HDAAudioStream *st, int64_t target_pos)
+{
+    int64_t corr =
+        NANOSECONDS_PER_SECOND * target_pos / hda_bytes_per_second(st);
+    if (corr > MAX_CORR) {
+        corr = MAX_CORR;
+    } else if (corr < -MAX_CORR) {
+        corr = -MAX_CORR;
+    }
+    atomic_fetch_add(&st->buft_start, corr);
+}
+
+static void hda_audio_input_timer(void *opaque)
+{
+    HDAAudioStream *st = opaque;
+
+    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+
+    int64_t buft_start = atomic_fetch_add(&st->buft_start, 0);
+    int64_t wpos = atomic_fetch_add(&st->wpos, 0);
+    int64_t rpos = atomic_fetch_add(&st->rpos, 0);
+
+    int64_t wanted_rpos = hda_bytes_per_second(st) * (now - buft_start)
+                          / NANOSECONDS_PER_SECOND;
+    wanted_rpos &= -4; /* IMPORTANT! clip to frames */
+
+    if (wanted_rpos <= rpos) {
+        /* we already transmitted the data */
+        goto out_timer;
+    }
+
+    int64_t to_transfer = audio_MIN(wpos - rpos, wanted_rpos - rpos);
+    while (to_transfer) {
+        uint32_t start = (rpos & B_MASK);
+        uint32_t chunk = audio_MIN(B_SIZE - start, to_transfer);
+        int rc = hda_codec_xfer(
+                &st->state->hda, st->stream, false, st->buf + start, chunk);
+        if (!rc) {
+            break;
+        }
+        rpos += chunk;
+        to_transfer -= chunk;
+        atomic_fetch_add(&st->rpos, chunk);
+    }
+
+out_timer:
+
+    if (st->running) {
+        timer_mod_anticipate_ns(st->buft, now + HDA_TIMER_TICKS);
+    }
+}
+
 static void hda_audio_input_cb(void *opaque, int avail)
 {
     HDAAudioStream *st = opaque;
+
+    int64_t wpos = atomic_fetch_add(&st->wpos, 0);
+    int64_t rpos = atomic_fetch_add(&st->rpos, 0);
+
+    int64_t to_transfer = audio_MIN(B_SIZE - (wpos - rpos), avail);
+
+    hda_timer_sync_adjust(st, -((wpos - rpos) + to_transfer - (B_SIZE >> 1)));
+
+    while (to_transfer) {
+        uint32_t start = (uint32_t) (wpos & B_MASK);
+        uint32_t chunk = (uint32_t) audio_MIN(B_SIZE - start, to_transfer);
+        uint32_t read = AUD_read(st->voice.in, st->buf + start, chunk);
+        wpos += read;
+        to_transfer -= read;
+        atomic_fetch_add(&st->wpos, read);
+        if (chunk != read) {
+            break;
+        }
+    }
+}
+
+static void hda_audio_output_timer(void *opaque)
+{
+    HDAAudioStream *st = opaque;
+
+    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+
+    int64_t buft_start = atomic_fetch_add(&st->buft_start, 0);
+    int64_t wpos = atomic_fetch_add(&st->wpos, 0);
+    int64_t rpos = atomic_fetch_add(&st->rpos, 0);
+
+    int64_t wanted_wpos = hda_bytes_per_second(st) * (now - buft_start)
+                          / NANOSECONDS_PER_SECOND;
+    wanted_wpos &= -4; /* IMPORTANT! clip to frames */
+
+    if (wanted_wpos <= wpos) {
+        /* we already received the data */
+        goto out_timer;
+    }
+
+    int64_t to_transfer = audio_MIN(B_SIZE - (wpos - rpos), wanted_wpos - wpos);
+    while (to_transfer) {
+        uint32_t start = (wpos & B_MASK);
+        uint32_t chunk = audio_MIN(B_SIZE - start, to_transfer);
+        int rc = hda_codec_xfer(
+                &st->state->hda, st->stream, true, st->buf + start, chunk);
+        if (!rc) {
+            break;
+        }
+        wpos += chunk;
+        to_transfer -= chunk;
+        atomic_fetch_add(&st->wpos, chunk);
+    }
+
+out_timer:
+
+    if (st->running) {
+        timer_mod_anticipate_ns(st->buft, now + HDA_TIMER_TICKS);
+    }
+}
+
+static void hda_audio_output_cb(void *opaque, int avail)
+{
+    HDAAudioStream *st = opaque;
+
+    int64_t wpos = atomic_fetch_add(&st->wpos, 0);
+    int64_t rpos = atomic_fetch_add(&st->rpos, 0);
+
+    int64_t to_transfer = audio_MIN(wpos - rpos, avail);
+
+    hda_timer_sync_adjust(st, (wpos - rpos) - to_transfer - (B_SIZE >> 1));
+
+    while (to_transfer) {
+        uint32_t start = (uint32_t) (rpos & B_MASK);
+        uint32_t chunk = (uint32_t) audio_MIN(B_SIZE - start, to_transfer);
+        uint32_t written = AUD_write(st->voice.out, st->buf + start, chunk);
+        rpos += written;
+        to_transfer -= written;
+        atomic_fetch_add(&st->rpos, written);
+        if (chunk != written) {
+            break;
+        }
+    }
+}
+
+static void hda_audio_compat_input_cb(void *opaque, int avail)
+{
+    HDAAudioStream *st = opaque;
     int recv = 0;
     int len;
     bool rc;
 
-    while (avail - recv >= sizeof(st->buf)) {
-        if (st->bpos != sizeof(st->buf)) {
-            len = AUD_read(st->voice.in, st->buf + st->bpos,
-                           sizeof(st->buf) - st->bpos);
-            st->bpos += len;
+    while (avail - recv >= sizeof(st->compat_buf)) {
+        if (st->compat_bpos != sizeof(st->compat_buf)) {
+            len = AUD_read(st->voice.in, st->compat_buf + st->compat_bpos,
+                           sizeof(st->compat_buf) - st->compat_bpos);
+            st->compat_bpos += len;
             recv += len;
-            if (st->bpos != sizeof(st->buf)) {
+            if (st->compat_bpos != sizeof(st->compat_buf)) {
                 break;
             }
         }
         rc = hda_codec_xfer(&st->state->hda, st->stream, false,
-                            st->buf, sizeof(st->buf));
+                            st->compat_buf, sizeof(st->compat_buf));
         if (!rc) {
             break;
         }
-        st->bpos = 0;
+        st->compat_bpos = 0;
     }
 }
 
-static void hda_audio_output_cb(void *opaque, int avail)
+static void hda_audio_compat_output_cb(void *opaque, int avail)
 {
     HDAAudioStream *st = opaque;
     int sent = 0;
     int len;
     bool rc;
 
-    while (avail - sent >= sizeof(st->buf)) {
-        if (st->bpos == sizeof(st->buf)) {
+    while (avail - sent >= sizeof(st->compat_buf)) {
+        if (st->compat_bpos == sizeof(st->compat_buf)) {
             rc = hda_codec_xfer(&st->state->hda, st->stream, true,
-                                st->buf, sizeof(st->buf));
+                                st->compat_buf, sizeof(st->compat_buf));
             if (!rc) {
                 break;
             }
-            st->bpos = 0;
+            st->compat_bpos = 0;
         }
-        len = AUD_write(st->voice.out, st->buf + st->bpos,
-                        sizeof(st->buf) - st->bpos);
-        st->bpos += len;
+        len = AUD_write(st->voice.out, st->compat_buf + st->compat_bpos,
+                        sizeof(st->compat_buf) - st->compat_bpos);
+        st->compat_bpos += len;
         sent += len;
-        if (st->bpos != sizeof(st->buf)) {
+        if (st->compat_bpos != sizeof(st->compat_buf)) {
             break;
         }
     }
@@ -239,6 +396,17 @@ static void hda_audio_set_running(HDAAudioStream *st, bool running)
     st->running = running;
     dprint(st->state, 1, "%s: %s (stream %d)\n", st->node->name,
            st->running ? "on" : "off", st->stream);
+    if (st->state->use_timer) {
+        if (running) {
+            int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+            st->rpos = 0;
+            st->wpos = 0;
+            st->buft_start = now;
+            timer_mod_anticipate_ns(st->buft, now + HDA_TIMER_TICKS);
+        } else {
+            timer_del(st->buft);
+        }
+    }
     if (st->output) {
         AUD_set_active_out(st->voice.out, st->running);
     } else {
@@ -274,6 +442,9 @@ static void hda_audio_set_amp(HDAAudioStream *st)
 
 static void hda_audio_setup(HDAAudioStream *st)
 {
+    bool use_timer = st->state->use_timer;
+    audio_callback_fn cb;
+
     if (st->node == NULL) {
         return;
     }
@@ -283,13 +454,25 @@ static void hda_audio_setup(HDAAudioStream *st)
            fmt2name[st->as.fmt], st->as.freq);
 
     if (st->output) {
+        if (use_timer) {
+            cb = hda_audio_output_cb;
+            st->buft = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                    hda_audio_output_timer, st);
+        } else {
+            cb = hda_audio_compat_output_cb;
+        }
         st->voice.out = AUD_open_out(&st->state->card, st->voice.out,
-                                     st->node->name, st,
-                                     hda_audio_output_cb, &st->as);
+                                     st->node->name, st, cb, &st->as);
     } else {
+        if (use_timer) {
+            cb = hda_audio_input_cb;
+            st->buft = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                    hda_audio_input_timer, st);
+        } else {
+            cb = hda_audio_compat_input_cb;
+        }
         st->voice.in = AUD_open_in(&st->state->card, st->voice.in,
-                                   st->node->name, st,
-                                   hda_audio_input_cb, &st->as);
+                                   st->node->name, st, cb, &st->as);
     }
 }
 
@@ -505,7 +688,7 @@ static int hda_audio_init(HDACodecDevice *hda, const struct desc_codec *desc)
                 /* unmute output by default */
                 st->gain_left = QEMU_HDA_AMP_STEPS;
                 st->gain_right = QEMU_HDA_AMP_STEPS;
-                st->bpos = sizeof(st->buf);
+                st->compat_bpos = sizeof(st->compat_buf);
                 st->output = true;
             } else {
                 st->output = false;
@@ -532,6 +715,9 @@ static void hda_audio_exit(HDACodecDevice *hda)
         if (st->node == NULL) {
             continue;
         }
+        if (a->use_timer) {
+            timer_del(st->buft);
+        }
         if (st->output) {
             AUD_close_out(&a->card, st->voice.out);
         } else {
@@ -581,6 +767,26 @@ static void hda_audio_reset(DeviceState *dev)
     }
 }
 
+static bool vmstate_hda_audio_stream_buf_needed(void *opaque)
+{
+    HDAAudioStream *st = opaque;
+    return st->state->use_timer;
+}
+
+static const VMStateDescription vmstate_hda_audio_stream_buf = {
+    .name = "hda-audio-stream/buffer",
+    .version_id = 1,
+    .needed = vmstate_hda_audio_stream_buf_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BUFFER(buf, HDAAudioStream),
+        VMSTATE_INT64(rpos, HDAAudioStream),
+        VMSTATE_INT64(wpos, HDAAudioStream),
+        VMSTATE_TIMER_PTR(buft, HDAAudioStream),
+        VMSTATE_INT64(buft_start, HDAAudioStream),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_hda_audio_stream = {
     .name = "hda-audio-stream",
     .version_id = 1,
@@ -592,9 +798,13 @@ 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_UINT32(compat_bpos, HDAAudioStream),
+        VMSTATE_BUFFER(compat_buf, HDAAudioStream),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_hda_audio_stream_buf,
+        NULL
     }
 };
 
@@ -615,6 +825,7 @@ static const VMStateDescription vmstate_hda_audio = {
 static Property hda_audio_properties[] = {
     DEFINE_PROP_UINT32("debug", HDAAudioState, debug,   0),
     DEFINE_PROP_BOOL("mixer", HDAAudioState, mixer,  true),
+    DEFINE_PROP_BOOL("use-timer", HDAAudioState, use_timer,  true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 948268afd8..23a2cf6484 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -407,13 +407,6 @@ static bool intel_hda_xfer(HDACodecDevice *dev, uint32_t stnr, bool output,
     if (st->bpl == NULL) {
         return false;
     }
-    if (st->ctl & (1 << 26)) {
-        /*
-         * Wait with the next DMA xfer until the guest
-         * has acked the buffer completion interrupt
-         */
-        return false;
-    }
 
     left = len;
     s = st->bentries;
-- 
2.9.3

[Qemu-devel] [PATCH 2/4] audio/hda: turn some dprintfs into trace points

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/audio/hda-codec.c  | 9 ++++-----
 hw/audio/trace-events | 4 ++++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index 9ee2f3d55f..13d4473f18 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -24,6 +24,7 @@
 #include "intel-hda.h"
 #include "intel-hda-defs.h"
 #include "audio/audio.h"
+#include "trace.h"
 
 /* -------------------------------------------------------------------------- */
 
@@ -394,8 +395,7 @@ static void hda_audio_set_running(HDAAudioStream *st, bool running)
         return;
     }
     st->running = running;
-    dprint(st->state, 1, "%s: %s (stream %d)\n", st->node->name,
-           st->running ? "on" : "off", st->stream);
+    trace_hda_audio_running(st->node->name, st->stream, st->running);
     if (st->state->use_timer) {
         if (running) {
             int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
@@ -449,9 +449,8 @@ static void hda_audio_setup(HDAAudioStream *st)
         return;
     }
 
-    dprint(st->state, 1, "%s: format: %d x %s @ %d Hz\n",
-           st->node->name, st->as.nchannels,
-           fmt2name[st->as.fmt], st->as.freq);
+    trace_hda_audio_format(st->node->name, st->as.nchannels,
+                           fmt2name[st->as.fmt], st->as.freq);
 
     if (st->output) {
         if (use_timer) {
diff --git a/hw/audio/trace-events b/hw/audio/trace-events
index fa1646d169..03340b9359 100644
--- a/hw/audio/trace-events
+++ b/hw/audio/trace-events
@@ -17,3 +17,7 @@ milkymist_ac97_in_cb(int avail, uint32_t remaining) "avail %d remaining %u"
 milkymist_ac97_in_cb_transferred(int transferred) "transferred %d"
 milkymist_ac97_out_cb(int free, uint32_t remaining) "free %d remaining %u"
 milkymist_ac97_out_cb_transferred(int transferred) "transferred %d"
+
+# hw/audio/hda-codec.c
+hda_audio_running(const char *stream, int nr, bool running) "st %s, nr %d, run %d"
+hda_audio_format(const char *stream, int chan, const char *fmt, int freq) "st %s, %d x %s @ %d Hz"
-- 
2.9.3

[Qemu-devel] [PATCH 3/4] audio/hda: tweak timer adjust logic

[Gerd Hoffmann]

We have some jitter in the audio timer call frequency and buffer sizes.
So it is rather pointless trying to be very exact, effect is a constant
up+down adjustment.  So adjust only in case we are off too much.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/audio/hda-codec.c  | 20 +++++++++++++-------
 hw/audio/trace-events |  1 +
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index 13d4473f18..f6fb1d0fa0 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -129,7 +129,6 @@ static void hda_codec_parse_fmt(uint32_t format, struct audsettings *as)
 #include  "hda-codec-common.h"
 
 #define HDA_TIMER_TICKS (SCALE_MS)
-#define MAX_CORR (SCALE_US * 100)
 #define B_SIZE sizeof(st->buf)
 #define B_MASK (sizeof(st->buf) - 1)
 
@@ -196,13 +195,20 @@ static inline int64_t hda_bytes_per_second(HDAAudioStream *st)
 
 static inline void hda_timer_sync_adjust(HDAAudioStream *st, int64_t target_pos)
 {
-    int64_t corr =
-        NANOSECONDS_PER_SECOND * target_pos / hda_bytes_per_second(st);
-    if (corr > MAX_CORR) {
-        corr = MAX_CORR;
-    } else if (corr < -MAX_CORR) {
-        corr = -MAX_CORR;
+    int64_t limit = B_SIZE / 8;
+    int64_t corr = 0;
+
+    if (target_pos > limit) {
+        corr = HDA_TIMER_TICKS;
     }
+    if (target_pos < -limit) {
+        corr = -HDA_TIMER_TICKS;
+    }
+    if (corr == 0) {
+        return;
+    }
+
+    trace_hda_audio_adjust(st->node->name, target_pos);
     atomic_fetch_add(&st->buft_start, corr);
 }
 
diff --git a/hw/audio/trace-events b/hw/audio/trace-events
index 03340b9359..30112d97c4 100644
--- a/hw/audio/trace-events
+++ b/hw/audio/trace-events
@@ -21,3 +21,4 @@ milkymist_ac97_out_cb_transferred(int transferred) "transferred %d"
 # hw/audio/hda-codec.c
 hda_audio_running(const char *stream, int nr, bool running) "st %s, nr %d, run %d"
 hda_audio_format(const char *stream, int chan, const char *fmt, int freq) "st %s, %d x %s @ %d Hz"
+hda_audio_adjust(const char *stream, int pos) "st %s, pos %d"
-- 
2.9.3

[Qemu-devel] [PATCH 4/4] audio/hda: detect output buffer overruns and underruns

[Gerd Hoffmann]

If some event caused some larger playback hickup the fine-grained timer
adjust isn't able to recover.  Use a buffer overruns and underruns as
indicator for that.  Reset timer adjust logic in case we detected one.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/audio/hda-codec.c  | 22 ++++++++++++++++++++++
 hw/audio/trace-events |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index f6fb1d0fa0..623021b7c7 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -261,6 +261,13 @@ static void hda_audio_input_cb(void *opaque, int avail)
 
     int64_t to_transfer = audio_MIN(B_SIZE - (wpos - rpos), avail);
 
+    if (to_transfer == 0) {
+        /* reset timer adjust */
+        st->buft_start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        trace_hda_audio_underrun(st->node->name);
+        return;
+    }
+
     hda_timer_sync_adjust(st, -((wpos - rpos) + to_transfer - (B_SIZE >> 1)));
 
     while (to_transfer) {
@@ -325,6 +332,21 @@ static void hda_audio_output_cb(void *opaque, int avail)
 
     int64_t to_transfer = audio_MIN(wpos - rpos, avail);
 
+    if (to_transfer == 0) {
+        /* reset timer adjust */
+        st->buft_start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        trace_hda_audio_underrun(st->node->name);
+        return;
+    }
+    if (wpos - rpos == B_SIZE) {
+        /* drop buffer, reset timer adjust */
+        st->rpos = 0;
+        st->wpos = 0;
+        st->buft_start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        trace_hda_audio_overrun(st->node->name);
+        return;
+    }
+
     hda_timer_sync_adjust(st, (wpos - rpos) - to_transfer - (B_SIZE >> 1));
 
     while (to_transfer) {
diff --git a/hw/audio/trace-events b/hw/audio/trace-events
index 30112d97c4..3b2ea2727b 100644
--- a/hw/audio/trace-events
+++ b/hw/audio/trace-events
@@ -22,3 +22,5 @@ milkymist_ac97_out_cb_transferred(int transferred) "transferred %d"
 hda_audio_running(const char *stream, int nr, bool running) "st %s, nr %d, run %d"
 hda_audio_format(const char *stream, int chan, const char *fmt, int freq) "st %s, %d x %s @ %d Hz"
 hda_audio_adjust(const char *stream, int pos) "st %s, pos %d"
+hda_audio_overrun(const char *stream) "st %s"
+hda_audio_underrun(const char *stream) "st %s"
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/4] audio/hda: turn some dprintfs into trace points

[Philippe Mathieu-Daudé]

On 04/19/2018 10:10 AM, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/audio/hda-codec.c  | 9 ++++-----
>  hw/audio/trace-events | 4 ++++
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> index 9ee2f3d55f..13d4473f18 100644
> --- a/hw/audio/hda-codec.c
> +++ b/hw/audio/hda-codec.c
> @@ -24,6 +24,7 @@
>  #include "intel-hda.h"
>  #include "intel-hda-defs.h"
>  #include "audio/audio.h"
> +#include "trace.h"
>  
>  /* -------------------------------------------------------------------------- */
>  
> @@ -394,8 +395,7 @@ static void hda_audio_set_running(HDAAudioStream *st, bool running)
>          return;
>      }
>      st->running = running;
> -    dprint(st->state, 1, "%s: %s (stream %d)\n", st->node->name,
> -           st->running ? "on" : "off", st->stream);
> +    trace_hda_audio_running(st->node->name, st->stream, st->running);
>      if (st->state->use_timer) {
>          if (running) {
>              int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> @@ -449,9 +449,8 @@ static void hda_audio_setup(HDAAudioStream *st)
>          return;
>      }
>  
> -    dprint(st->state, 1, "%s: format: %d x %s @ %d Hz\n",
> -           st->node->name, st->as.nchannels,
> -           fmt2name[st->as.fmt], st->as.freq);
> +    trace_hda_audio_format(st->node->name, st->as.nchannels,
> +                           fmt2name[st->as.fmt], st->as.freq);
>  
>      if (st->output) {
>          if (use_timer) {
> diff --git a/hw/audio/trace-events b/hw/audio/trace-events
> index fa1646d169..03340b9359 100644
> --- a/hw/audio/trace-events
> +++ b/hw/audio/trace-events
> @@ -17,3 +17,7 @@ milkymist_ac97_in_cb(int avail, uint32_t remaining) "avail %d remaining %u"
>  milkymist_ac97_in_cb_transferred(int transferred) "transferred %d"
>  milkymist_ac97_out_cb(int free, uint32_t remaining) "free %d remaining %u"
>  milkymist_ac97_out_cb_transferred(int transferred) "transferred %d"
> +
> +# hw/audio/hda-codec.c
> +hda_audio_running(const char *stream, int nr, bool running) "st %s, nr %d, run %d"
> +hda_audio_format(const char *stream, int chan, const char *fmt, int freq) "st %s, %d x %s @ %d Hz"
> 

Re: [Qemu-devel] [PATCH 4/4] audio/hda: detect output buffer overruns and underruns

[KONRAD Frederic]

On 04/19/2018 03:10 PM, Gerd Hoffmann wrote:
> If some event caused some larger playback hickup the fine-grained timer
> adjust isn't able to recover.  Use a buffer overruns and underruns as
> indicator for that.  Reset timer adjust logic in case we detected one.

It seems that this patch causes big lockup of my Win10 KVM guest
when it tries to output a sound.
The three previous patches improved considerably the audio
output though!

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/audio/hda-codec.c  | 22 ++++++++++++++++++++++
>   hw/audio/trace-events |  2 ++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> index f6fb1d0fa0..623021b7c7 100644
> --- a/hw/audio/hda-codec.c
> +++ b/hw/audio/hda-codec.c
> @@ -261,6 +261,13 @@ static void hda_audio_input_cb(void *opaque, int avail)
>   
>       int64_t to_transfer = audio_MIN(B_SIZE - (wpos - rpos), avail);
>   
> +    if (to_transfer == 0) {
> +        /* reset timer adjust */
> +        st->buft_start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        trace_hda_audio_underrun(st->node->name);
> +        return;
> +    }
> +
>       hda_timer_sync_adjust(st, -((wpos - rpos) + to_transfer - (B_SIZE >> 1)));
>   
>       while (to_transfer) {
> @@ -325,6 +332,21 @@ static void hda_audio_output_cb(void *opaque, int avail)
>   
>       int64_t to_transfer = audio_MIN(wpos - rpos, avail);
>   
> +    if (to_transfer == 0) {
> +        /* reset timer adjust */
> +        st->buft_start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        trace_hda_audio_underrun(st->node->name);
> +        return;
> +    }

The lock happen here:

hda_audio_output_cb is called a lot and keeps running in
underrun. While the 1kHz callback is called regularly and
always "already have the data". So I wonder if the adjust here is
not confusing the 1kHz callback which refuse to get new data.

Regards,
Fred

Re: [Qemu-devel] [PATCH 4/4] audio/hda: detect output buffer overruns and underruns

[Gerd Hoffmann]

On Mon, Apr 23, 2018 at 11:31:34AM +0200, KONRAD Frederic wrote:
> 
> 
> On 04/19/2018 03:10 PM, Gerd Hoffmann wrote:
> > If some event caused some larger playback hickup the fine-grained timer
> > adjust isn't able to recover.  Use a buffer overruns and underruns as
> > indicator for that.  Reset timer adjust logic in case we detected one.
> 
> It seems that this patch causes big lockup of my Win10 KVM guest
> when it tries to output a sound.
> The three previous patches improved considerably the audio
> output though!

Can you try drop the "underrun" chunks, keeping only the "overrun"?

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 4/4] audio/hda: detect output buffer overruns and underruns

[KONRAD Frederic]

Hi Gerd,

Sorry I didn't had time to try that yet.

I'll try today!

Thanks,
Fred

On 04/25/2018 03:05 PM, Gerd Hoffmann wrote:
> On Mon, Apr 23, 2018 at 11:31:34AM +0200, KONRAD Frederic wrote:
>>
>>
>> On 04/19/2018 03:10 PM, Gerd Hoffmann wrote:
>>> If some event caused some larger playback hickup the fine-grained timer
>>> adjust isn't able to recover.  Use a buffer overruns and underruns as
>>> indicator for that.  Reset timer adjust logic in case we detected one.
>>
>> It seems that this patch causes big lockup of my Win10 KVM guest
>> when it tries to output a sound.
>> The three previous patches improved considerably the audio
>> output though!
> 
> Can you try drop the "underrun" chunks, keeping only the "overrun"?
> 
> thanks,
>    Gerd
> 

Re: [Qemu-devel] [PATCH 4/4] audio/hda: detect output buffer overruns and underruns

[KONRAD Frederic]

On 04/25/2018 03:05 PM, Gerd Hoffmann wrote:
> On Mon, Apr 23, 2018 at 11:31:34AM +0200, KONRAD Frederic wrote:
>>
>>
>> On 04/19/2018 03:10 PM, Gerd Hoffmann wrote:
>>> If some event caused some larger playback hickup the fine-grained timer
>>> adjust isn't able to recover.  Use a buffer overruns and underruns as
>>> indicator for that.  Reset timer adjust logic in case we detected one.
>>
>> It seems that this patch causes big lockup of my Win10 KVM guest
>> when it tries to output a sound.
>> The three previous patches improved considerably the audio
>> output though!
> 
> Can you try drop the "underrun" chunks, keeping only the "overrun"?
> 
> thanks,
>    Gerd
> 

Hi Gerd,

Tested yesterday it seems that both are causing lockup. The
overrun a little less though.

Thanks,
Fred

Re: [Qemu-devel] [PATCH 4/4] audio/hda: detect output buffer overruns and underruns

[Gerd Hoffmann]

On Fri, May 11, 2018 at 11:25:37AM +0200, KONRAD Frederic wrote:
> 
> 
> On 04/25/2018 03:05 PM, Gerd Hoffmann wrote:
> > On Mon, Apr 23, 2018 at 11:31:34AM +0200, KONRAD Frederic wrote:
> > > 
> > > 
> > > On 04/19/2018 03:10 PM, Gerd Hoffmann wrote:
> > > > If some event caused some larger playback hickup the fine-grained timer
> > > > adjust isn't able to recover.  Use a buffer overruns and underruns as
> > > > indicator for that.  Reset timer adjust logic in case we detected one.
> > > 
> > > It seems that this patch causes big lockup of my Win10 KVM guest
> > > when it tries to output a sound.
> > > The three previous patches improved considerably the audio
> > > output though!
> > 
> > Can you try drop the "underrun" chunks, keeping only the "overrun"?
> > 
> > thanks,
> >    Gerd
> > 
> 
> Hi Gerd,
> 
> Tested yesterday it seems that both are causing lockup. The
> overrun a little less though.

Hints how to reproduce this?  Which guest?  Which application?

thanks,
  Gerd