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

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(-)

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

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

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