[Qemu-devel] [PATCH 0/2] HDA fixes for Windows 7

[Qemu-devel] [PATCH 0/2] HDA fixes for Windows 7

[Marc-André Lureau]

Hi,

There are two related bugs in stream state and xfer handling due to the
conflict of stream number (on Windows 7, input stream and output stream
both use the full range of 1..15 values. on Linux, the handling is
different and it works fine because there is no clash)

The patches handle stream number for input and output stream
distinctively to avoid conflict, and it seems to be in accordance
to the spec.

Marc-André Lureau (2):
  hda: do not mix output and input streams, RHBZ #740493
  hda: do not mix output and input stream states, RHBZ #740493

 hw/hda-audio.c |   15 +++++++++------
 hw/intel-hda.c |   18 ++++++++++--------
 hw/intel-hda.h |    2 +-
 3 files changed, 20 insertions(+), 15 deletions(-)

-- 
1.7.6.2

[Qemu-devel] [PATCH 1/2] hda: do not mix output and input streams, RHBZ #740493

[Marc-André Lureau]

Windows 7 may use the same stream number for input and output.
That will result in lot of garbage on playback.

The hardcoded value of 4 needs to be in sync with GCAP streams
description and IN/OUT registers.
---
 hw/intel-hda.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 4272204..c6a3fec 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -389,14 +389,15 @@ static bool intel_hda_xfer(HDACodecDevice *dev, uint32_t stnr, bool output,
 {
     HDACodecBus *bus = DO_UPCAST(HDACodecBus, qbus, dev->qdev.parent_bus);
     IntelHDAState *d = container_of(bus, IntelHDAState, codecs);
-    IntelHDAStream *st = NULL;
     target_phys_addr_t addr;
     uint32_t s, copy, left;
+    IntelHDAStream *st;
     bool irq = false;
 
-    for (s = 0; s < ARRAY_SIZE(d->st); s++) {
-        if (stnr == ((d->st[s].ctl >> 20) & 0x0f)) {
-            st = d->st + s;
+    st = output ? d->st + 4 : d->st;
+    for (s = 0; s < 4; s++) {
+        if (stnr == ((st[s].ctl >> 20) & 0x0f)) {
+            st = st + s;
             break;
         }
     }
-- 
1.7.6.2

[Qemu-devel] [PATCH 2/2] hda: do not mix output and input stream states, RHBZ #740493

[Marc-André Lureau]

Windows 7 may use the same stream number for input and output.
Current code will confuse streams.

NB: I wonder if this patch breaks migration code because of
this change:
-        VMSTATE_BOOL_ARRAY(running, HDAAudioState, 16),
+        VMSTATE_BOOL_ARRAY(running, HDAAudioState, 2 * 16),
---
 hw/hda-audio.c |   15 +++++++++------
 hw/intel-hda.c |    9 +++++----
 hw/intel-hda.h |    2 +-
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/hda-audio.c b/hw/hda-audio.c
index 03c0a24..dacf290 100644
--- a/hw/hda-audio.c
+++ b/hw/hda-audio.c
@@ -462,7 +462,7 @@ struct HDAAudioState {
     QEMUSoundCard card;
     const desc_codec *desc;
     HDAAudioStream st[4];
-    bool running[16];
+    bool running[2 * 16];
 
     /* properties */
     uint32_t debug;
@@ -659,7 +659,7 @@ static void hda_audio_command(HDACodecDevice *hda, uint32_t nid, uint32_t data)
         st->channel = payload & 0x0f;
         dprint(a, 2, "%s: stream %d, channel %d\n",
                st->node->name, st->stream, st->channel);
-        hda_audio_set_running(st, a->running[st->stream]);
+        hda_audio_set_running(st, a->running[st->output * 16 + st->stream]);
         hda_codec_response(hda, true, 0);
         break;
     case AC_VERB_GET_CONV:
@@ -742,16 +742,19 @@ fail:
     hda_codec_response(hda, true, 0);
 }
 
-static void hda_audio_stream(HDACodecDevice *hda, uint32_t stnr, bool running)
+static void hda_audio_stream(HDACodecDevice *hda, uint32_t stnr, bool running, bool output)
 {
     HDAAudioState *a = DO_UPCAST(HDAAudioState, hda, hda);
     int s;
 
-    a->running[stnr] = running;
+    a->running[output * 16 + stnr] = running;
     for (s = 0; s < ARRAY_SIZE(a->st); s++) {
         if (a->st[s].node == NULL) {
             continue;
         }
+        if (a->st[s].output != output) {
+            continue;
+        }
         if (a->st[s].stream != stnr) {
             continue;
         }
@@ -840,7 +843,7 @@ static int hda_audio_post_load(void *opaque, int version)
         hda_codec_parse_fmt(st->format, &st->as);
         hda_audio_setup(st);
         hda_audio_set_amp(st);
-        hda_audio_set_running(st, a->running[st->stream]);
+        hda_audio_set_running(st, a->running[st->output * 16 + st->stream]);
     }
     return 0;
 }
@@ -870,7 +873,7 @@ static const VMStateDescription vmstate_hda_audio = {
         VMSTATE_STRUCT_ARRAY(st, HDAAudioState, 4, 0,
                              vmstate_hda_audio_stream,
                              HDAAudioStream),
-        VMSTATE_BOOL_ARRAY(running, HDAAudioState, 16),
+        VMSTATE_BOOL_ARRAY(running, HDAAudioState, 2 * 16),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index c6a3fec..f97775c 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -485,7 +485,7 @@ static void intel_hda_parse_bdl(IntelHDAState *d, IntelHDAStream *st)
     st->bp    = 0;
 }
 
-static void intel_hda_notify_codecs(IntelHDAState *d, uint32_t stream, bool running)
+static void intel_hda_notify_codecs(IntelHDAState *d, uint32_t stream, bool running, bool output)
 {
     DeviceState *qdev;
     HDACodecDevice *cdev;
@@ -493,7 +493,7 @@ static void intel_hda_notify_codecs(IntelHDAState *d, uint32_t stream, bool runn
     QLIST_FOREACH(qdev, &d->codecs.qbus.children, sibling) {
         cdev = DO_UPCAST(HDACodecDevice, qdev, qdev);
         if (cdev->info->stream) {
-            cdev->info->stream(cdev, stream, running);
+            cdev->info->stream(cdev, stream, running, output);
         }
     }
 }
@@ -567,6 +567,7 @@ static void intel_hda_set_ics(IntelHDAState *d, const IntelHDAReg *reg, uint32_t
 
 static void intel_hda_set_st_ctl(IntelHDAState *d, const IntelHDAReg *reg, uint32_t old)
 {
+    bool output = reg->stream >= 4;
     IntelHDAStream *st = d->st + reg->stream;
 
     if (st->ctl & 0x01) {
@@ -582,11 +583,11 @@ static void intel_hda_set_st_ctl(IntelHDAState *d, const IntelHDAReg *reg, uint3
             dprint(d, 1, "st #%d: start %d (ring buf %d bytes)\n",
                    reg->stream, stnr, st->cbl);
             intel_hda_parse_bdl(d, st);
-            intel_hda_notify_codecs(d, stnr, true);
+            intel_hda_notify_codecs(d, stnr, true, output);
         } else {
             /* stop */
             dprint(d, 1, "st #%d: stop %d\n", reg->stream, stnr);
-            intel_hda_notify_codecs(d, stnr, false);
+            intel_hda_notify_codecs(d, stnr, false, output);
         }
     }
     intel_hda_update_irq(d);
diff --git a/hw/intel-hda.h b/hw/intel-hda.h
index 4e44e38..65fd2a8 100644
--- a/hw/intel-hda.h
+++ b/hw/intel-hda.h
@@ -34,7 +34,7 @@ struct HDACodecDeviceInfo {
     int (*init)(HDACodecDevice *dev);
     int (*exit)(HDACodecDevice *dev);
     void (*command)(HDACodecDevice *dev, uint32_t nid, uint32_t data);
-    void (*stream)(HDACodecDevice *dev, uint32_t stnr, bool running);
+    void (*stream)(HDACodecDevice *dev, uint32_t stnr, bool running, bool output);
 };
 
 void hda_codec_bus_init(DeviceState *dev, HDACodecBus *bus,
-- 
1.7.6.2

Re: [Qemu-devel] [PATCH 2/2] hda: do not mix output and input stream states, RHBZ #740493

[Juan Quintela]

"Marc-André Lureau" <marcandre.lureau@gmail.com> wrote:
> Windows 7 may use the same stream number for input and output.
> Current code will confuse streams.
>
> NB: I wonder if this patch breaks migration code because of
> this change:
> -        VMSTATE_BOOL_ARRAY(running, HDAAudioState, 16),
> +        VMSTATE_BOOL_ARRAY(running, HDAAudioState, 2 * 16),

It does :-(

Two questions to know how to handle this.
a) My understanding of this patch is that we move from an array of 16
bools representing anything to one array where the 1st 16 represent if
there are input and the 2nd 16's reprosenting if there are output for
that channel.

So, what we should do if we migrate from one old version that only has
16 bools?  My understanding is that copying directly is not gonna work?

b) Just in case it makes "compatibility" easier.

Is the running array normally all false or similar?  If so, we could
just sent the other 16th values only if they are different of false.
(if this is true, it is interesting to put in the 1st slots the ones
that are more probable to be true).

Later, Juan.

Re: [Qemu-devel] [PATCH 2/2] hda: do not mix output and input stream states, RHBZ #740493

[Marc-André Lureau]

Hi

On Tue, Oct 4, 2011 at 7:21 PM, Juan Quintela <quintela@redhat.com> wrote:
> "Marc-André Lureau" <marcandre.lureau@gmail.com> wrote:
>> Windows 7 may use the same stream number for input and output.
>> Current code will confuse streams.
>>
>> NB: I wonder if this patch breaks migration code because of
>> this change:
>> -        VMSTATE_BOOL_ARRAY(running, HDAAudioState, 16),
>> +        VMSTATE_BOOL_ARRAY(running, HDAAudioState, 2 * 16),
>
> It does :-(
>
> Two questions to know how to handle this.
> a) My understanding of this patch is that we move from an array of 16
> bools representing anything to one array where the 1st 16 represent if
> there are input and the 2nd 16's reprosenting if there are output for
> that channel.
>
> So, what we should do if we migrate from one old version that only has
> 16 bools?  My understanding is that copying directly is not gonna work?


We might assume that the array is for output stream, and discard any
running input stream. Or we can be slightly smarter and try to figure
out the running HDAAudioStream (which by the way I don't know why
there are 4 and not 8 or 2) and map this to a input and output arrays
correctly, except in complex cases. For instance, if there is only
running[1] == true but we have output st[0].running and input
st[1].running we can map this correctly. But in complex cases that
won't probably work.

Just to be clear and honest, I am not sure how the HDA code works, I
am new to all this, and I use a lot of guesses and skimming the spec.

So for now, I don't think it will be a regression if we try to do a
smart mapping. I suppose there is a hook after VM migration finish
where we could apply this logic, but can we break the bitstrean and
migrate to newer versions safely?

Other possibility is to avoid the issue alltogether by ignoring
running streams and stopping them. From my tests OS seems to deal with
that situation just fine. Next time you play/record something it will
run again.

> b) Just in case it makes "compatibility" easier.
>
> Is the running array normally all false or similar?  If so, we could
> just sent the other 16th values only if they are different of false.
> (if this is true, it is interesting to put in the 1st slots the ones
> that are more probable to be true).

Yes, they are usually all false but one, if playback or recording on
going. But I am not follwing, could you give an example of how you
would change the array? Wouldn't that break migration?

regards

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 2/2] hda: do not mix output and input stream states, RHBZ #740493

[Gerd Hoffmann]

   Hi,

>> a) My understanding of this patch is that we move from an array of 16
>> bools representing anything to one array where the 1st 16 represent if
>> there are input and the 2nd 16's reprosenting if there are output for
>> that channel.

Correct.

>> So, what we should do if we migrate from one old version that only has
>> 16 bools?  My understanding is that copying directly is not gonna work?

Yes.  Putting output first increases the chance that it works as sound 
playback is used much more than sound recording.

I think hda-audio doesn't need so safe any running state.  intel-hda 
knows which streams are running and it can call 
intel_hda_notify_codecs() for each stream in intel_hda_post_load().  The 
only problem is that intel_hda_post_load() might get called before 
hda-audio state is loaded.

Now how to handle compatibility?  We can have a running_compat[] array 
with the current (broken) semantics, so we can write out the state which 
older qemu versions expect to see.  Also on load running_compat[] will 
be filled, and any state already set by intel_hda_post_load in 
running_real[] (or however we'll name this) will be kept intact.

cheers,
   Gerd