[Qemu-devel] Re: [PATCH 00/20] Port audio to vmstate

[Juan Quintela <quintela@redhat.com>]

malc <av1474@comtv.ru> wrote:
> On Thu, 22 Oct 2009, Juan Quintela wrote:
>

Hi

>> - es1370: the best working with migration.
>> - adlib: I am not able to get sound out of it on any recent Fedora :(
>
> It's an FM chip, trying to play PCM with it just not gonna fly.

That could expain it :)

>>   I disabled dma_running before loading state.  malc can you take a look here?

> Can you please elaborate on what is exactly the problem.

What we have here:

On save_state, we save a state field:
    val = s->dma_running; qemu_put_be32s (f, &val);

Now on load state, we read it to one local variable:
    qemu_get_be32s (f, &dma_running);

My understanding of the code here is that you should never assign
directly s->dma_running, that you should set/clean it through
  cs_reset_voices()
(setting this value has _side-effects_

So far so good.

Now, the whole point of vmstate is:
- state is described declaratively
- and load/save are "kind" of inverse functions. One save
  the value of one field to the state, and the other takes the value
  from the state and put it back in the field.
- Later (post_load), we do any side-effect that we need to do with the
  new loaded state.

Here we can't do this.  How would I do it in any other driver, i.e. (no
audio).  Easy, you just declare dma_running_vmstate in the state, and
in post_load(), you just call cs_reset_voices depending on that value.

You already ruled out that solution on ac97.  Didn't even try here.

No problem, I just read it on s->dma_running(), and on post_load I do:

if (s->dma_running && (s->dregs[Interface_Configuration] & PEN)) {
      s->dma_running = 0;
      cs_reset_voices (s, s->dregs[FS_And_Playback_Data_Format]);
}

And call it a day :).

But then I thought of what happens if you call loadvm from the monitor
while s->dma_running = 1.

And decided that the proper thing to do is to do:

in pre_load()
 - we set dma_running to 0 if if it 1, but do it like in
   cs_reset_voices()

        if (s->dma_running) {
            DMA_release_DREQ (s->dma);
            AUD_set_active_out (s->voice, 0);
        }
        s->dma_running = 0;

And now, in post_load, we know that s->dma_runing is what was comes from
migration, and we can set it to this value.  Do you think that my
explanation is clear?

Discussing this with Anthony on irc, he thinks that dma_running is the
same that  (s->dregs[Interface_Configuration] & PEN), and that we could
remove it from the state, but I am not sure, nad decided not to go that
route (because I am not sure, not because it is not the same.  I don't know).

>> - ac97: from Anthony sugestion, remove the use of active array, it can be
>>         recalculated.  All my testing (muted, not muted, ..., shows that
>>         transmited array is the same one than the recalculated one).
>
> Good.
>
>>   ac97 don't work always with migration.
>> 
>> How did I test:
>> - start a linux guest
>> - inside there play a song (ogg123 foo.ogg)
>> - in the middle of the song do savevm foo
>> - later restart qemu with -loadvm foo option
>> 
>> What did I expect?:
>> - I expected after ending the loadvm to hear the contination of the song.
>> 
>> Actual results:
>> - es1370: the one working better
>> - sb16: lots of underruns, very low sound quality.
>> - ac97: mixed results.  The 1st 5-10 seconds always sound perfect
>>    Then, between 10 and 30 seconds, sometimes it lots syncs and start playing at
>>    full speed, basiaclly no sound ouput. No sound after this is ever generated.
>>    If it is able to generate sound for 30-40 seconds, then sound works correctly.
>>   I tried to debug this, enabled all the audio DEBUG*, but I didn't find what is
>>   happening.
>>   Notice that this happens when I launch from the same savevm image.  Some times
>>   it works well, some times it stops working at 5 seconds, sometimes it stops
>>   working at 10 seconds, and so on.
>>   My theory is that we are not saving all the state that we need, but I have been
>>   unable to found anything obviosu here.  malc, do you have any suggestion?
>
> a. See how it behaves when you use OSS on the guest instead
>    (might need clocking=48000 module parameter)
>
> b. http://mpxplay.sourceforge.net/ is the DOS player which knows about
>    i810 audio, see how it fares there
>> 
>> This took a lot because the ac97 testing, I thouguht the ac97
>> problems were due to my changes.  It just happened that the 1st time
>> that I loaded without my changes it just worked :( Problem can be
>> reproduced as easily without any of my changes.  More work needs to
>> be done to be sure that ac97 migrates correctly, but that is
>> independent of this patch :)
>> 
>
> I don't like vmstate, but if you are dead bent on adding it to audio,
> knock yourself out, IO_READ/WRITE_PROTO will stay. Can't help with
> migration, GUS/Adlib are better tested with DOS and as for cs4231a i
> don't quite understand what you are asking.

ok, will integrate the vmstate changes, and leave the
IO_READ/WRITE_PROTO until there are consensous about that :)

Later, Juan.