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

[malc <av1474@comtv.ru>]

On Fri, 23 Oct 2009, Juan Quintela wrote:

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

Okay, i've looked at the code dma_running is basically a flag which is
sortof redundant but for the lack of better interface tells us whether
we have DREQ asserted, nothing more nothing less.

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

Sorry, i haven't slept in quite a while, so nothing makes any sense
currently, i'll try to do a mental exercise of running it solely
inside my brain sometime later.

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

It's not quite the same as Iconf & PEN (see for instance PPIO
handling, or lack thereof)

[..snip..]

-- 
mailto:av1474@comtv.ru