Patchwork [27/49] ac97: add active to the state

login
register
mail settings
Submitter Juan Quintela
Date Sept. 29, 2009, 8:48 p.m.
Message ID <d93032e20d443aeb567c2dcee9c6d4d166b98d58.1254255997.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/34478/
State Superseded
Headers show

Comments

Juan Quintela - Sept. 29, 2009, 8:48 p.m.
This simplifies reset_voices, that only takes one argument now.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ac97.c |   42 ++++++++++++++++++++----------------------
 1 files changed, 20 insertions(+), 22 deletions(-)
malc - Sept. 30, 2009, 10:37 a.m.
On Tue, 29 Sep 2009, Juan Quintela wrote:

> This simplifies reset_voices, that only takes one argument now.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/ac97.c |   42 ++++++++++++++++++++----------------------
>  1 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ac97.c b/hw/ac97.c
> index 610ca60..da6cb2d 100644
> --- a/hw/ac97.c
> +++ b/hw/ac97.c
> @@ -146,6 +146,13 @@ typedef struct AC97BusMasterRegs {
>      BD bd;
>  } AC97BusMasterRegs;
> 
> +enum {
> +    PI_INDEX = 0,
> +    PO_INDEX,
> +    MC_INDEX,
> +    LAST_INDEX
> +};

And this was moved becasue...?

> +
>  typedef struct AC97LinkState {
>      PCIDevice dev;
>      QEMUSoundCard card;
> @@ -162,6 +169,7 @@ typedef struct AC97LinkState {
>      uint8_t silence[128];
>      uint32_t base[2];
>      int bup_flag;
> +    uint8_t active[LAST_INDEX];

This doesn't belong here, cause the only purpose i can see is to hack
around defficiencies of the new load/savevm APIs.

>  } AC97LinkState;
> 
>  enum {
> @@ -186,13 +194,6 @@ enum {                                          \
>      prefix ## _CR = start + 11                  \
>  }
> 
> -enum {
> -    PI_INDEX = 0,
> -    PO_INDEX,
> -    MC_INDEX,
> -    LAST_INDEX
> -};
> -
>  MKREGS (PI, PI_INDEX * 16);
>  MKREGS (PO, PO_INDEX * 16);
>  MKREGS (MC, MC_INDEX * 16);
> @@ -414,21 +415,21 @@ static void open_voice (AC97LinkState *s, int index, int freq)
>      }
>  }
> 
> -static void reset_voices (AC97LinkState *s, uint8_t active[LAST_INDEX])
> +static void reset_voices (AC97LinkState *s)
>  {
>      uint16_t freq;
> 
>      freq = mixer_load (s, AC97_PCM_LR_ADC_Rate);
>      open_voice (s, PI_INDEX, freq);
> -    AUD_set_active_in (s->voice_pi, active[PI_INDEX]);
> +    AUD_set_active_in (s->voice_pi, s->active[PI_INDEX]);
> 
>      freq = mixer_load (s, AC97_PCM_Front_DAC_Rate);
>      open_voice (s, PO_INDEX, freq);
> -    AUD_set_active_out (s->voice_po, active[PO_INDEX]);
> +    AUD_set_active_out (s->voice_po, s->active[PO_INDEX]);
> 
>      freq = mixer_load (s, AC97_MIC_ADC_Rate);
>      open_voice (s, MC_INDEX, freq);
> -    AUD_set_active_in (s->voice_mc, active[MC_INDEX]);
> +    AUD_set_active_in (s->voice_mc, s->active[MC_INDEX]);
>  }
> 
>  #ifdef USE_MIXER
> @@ -526,11 +527,10 @@ static void record_select (AC97LinkState *s, uint32_t val)
> 
>  static void mixer_reset (AC97LinkState *s)
>  {
> -    uint8_t active[LAST_INDEX];
> 
>      dolog ("mixer_reset\n");
>      memset (s->mixer_data, 0, sizeof (s->mixer_data));
> -    memset (active, 0, sizeof (active));
> +    memset (s->active, 0, sizeof (s->active));
>      mixer_store (s, AC97_Reset                   , 0x0000); /* 6940 */
>      mixer_store (s, AC97_Master_Volume_Mono_Mute , 0x8000);
>      mixer_store (s, AC97_PC_BEEP_Volume_Mute     , 0x0000);
> @@ -564,7 +564,7 @@ static void mixer_reset (AC97LinkState *s)
>      set_volume (s, AC97_PCM_Out_Volume_Mute, AUD_MIXER_PCM    , 0x8808);
>      set_volume (s, AC97_Line_In_Volume_Mute, AUD_MIXER_LINE_IN, 0x8808);
>  #endif
> -    reset_voices (s, active);
> +    reset_voices (s);
>  }
> 
>  /**
> @@ -1170,7 +1170,6 @@ static void po_callback (void *opaque, int free)
>  static void ac97_save (QEMUFile *f, void *opaque)
>  {
>      size_t i;
> -    uint8_t active[LAST_INDEX];
>      AC97LinkState *s = opaque;
> 
>      pci_device_save (&s->dev, f);
> @@ -1194,17 +1193,16 @@ static void ac97_save (QEMUFile *f, void *opaque)
>      }
>      qemu_put_buffer (f, s->mixer_data, sizeof (s->mixer_data));
> 
> -    active[PI_INDEX] = AUD_is_active_in (s->voice_pi) ? 1 : 0;
> -    active[PO_INDEX] = AUD_is_active_out (s->voice_po) ? 1 : 0;
> -    active[MC_INDEX] = AUD_is_active_in (s->voice_mc) ? 1 : 0;
> -    qemu_put_buffer (f, active, sizeof (active));
> +    s->active[PI_INDEX] = AUD_is_active_in (s->voice_pi) ? 1 : 0;
> +    s->active[PO_INDEX] = AUD_is_active_out (s->voice_po) ? 1 : 0;
> +    s->active[MC_INDEX] = AUD_is_active_in (s->voice_mc) ? 1 : 0;
> +    qemu_put_buffer (f, s->active, sizeof (s->active));
>  }
> 
>  static int ac97_load (QEMUFile *f, void *opaque, int version_id)
>  {
>      int ret;
>      size_t i;
> -    uint8_t active[LAST_INDEX];
>      AC97LinkState *s = opaque;
> 
>      if (version_id != 2)
> @@ -1232,7 +1230,7 @@ static int ac97_load (QEMUFile *f, void *opaque, int version_id)
>          qemu_get_be32s (f, &r->bd.ctl_len);
>      }
>      qemu_get_buffer (f, s->mixer_data, sizeof (s->mixer_data));
> -    qemu_get_buffer (f, active, sizeof (active));
> +    qemu_get_buffer (f, s->active, sizeof (s->active));
> 
>  #ifdef USE_MIXER
>      record_select (s, mixer_load (s, AC97_Record_Select));
> @@ -1242,7 +1240,7 @@ static int ac97_load (QEMUFile *f, void *opaque, int version_id)
>      V_ (AC97_Line_In_Volume_Mute, AUD_MIXER_LINE_IN);
>  #undef V_
>  #endif
> -    reset_voices (s, active);
> +    reset_voices (s);
> 
>      s->bup_flag = 0;
>      s->last_samp = 0;
>
Juan Quintela - Sept. 30, 2009, 11:37 a.m.
malc <av1474@comtv.ru> wrote:
> On Tue, 29 Sep 2009, Juan Quintela wrote:
>
>> This simplifies reset_voices, that only takes one argument now.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/ac97.c |   42 ++++++++++++++++++++----------------------
>>  1 files changed, 20 insertions(+), 22 deletions(-)
>> 
>> diff --git a/hw/ac97.c b/hw/ac97.c
>> index 610ca60..da6cb2d 100644
>> --- a/hw/ac97.c
>> +++ b/hw/ac97.c
>> @@ -146,6 +146,13 @@ typedef struct AC97BusMasterRegs {
>>      BD bd;
>>  } AC97BusMasterRegs;
>> 
>> +enum {
>> +    PI_INDEX = 0,
>> +    PO_INDEX,
>> +    MC_INDEX,
>> +    LAST_INDEX
>> +};
>
> And this was moved becasue...?

Because I need LAST_INDEX in next part that you quoted.

>> +
>>  typedef struct AC97LinkState {
>>      PCIDevice dev;
>>      QEMUSoundCard card;
>> @@ -162,6 +169,7 @@ typedef struct AC97LinkState {
>>      uint8_t silence[128];
>>      uint32_t base[2];
>>      int bup_flag;
>> +    uint8_t active[LAST_INDEX];
>
> This doesn't belong here, cause the only purpose i can see is to hack
> around defficiencies of the new load/savevm APIs.

That was supposed to be one of the features, not deficiences.  You can't
sent stuff that it is not in the state. It is "by design" that you can't
sent arbitrary variables.

Later, Juan.
malc - Sept. 30, 2009, 11:47 a.m.
On Wed, 30 Sep 2009, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> > On Tue, 29 Sep 2009, Juan Quintela wrote:
> >
> >> This simplifies reset_voices, that only takes one argument now.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/ac97.c |   42 ++++++++++++++++++++----------------------
> >>  1 files changed, 20 insertions(+), 22 deletions(-)
> >> 
> >> diff --git a/hw/ac97.c b/hw/ac97.c
> >> index 610ca60..da6cb2d 100644
> >> --- a/hw/ac97.c
> >> +++ b/hw/ac97.c
> >> @@ -146,6 +146,13 @@ typedef struct AC97BusMasterRegs {
> >>      BD bd;
> >>  } AC97BusMasterRegs;
> >> 
> >> +enum {
> >> +    PI_INDEX = 0,
> >> +    PO_INDEX,
> >> +    MC_INDEX,
> >> +    LAST_INDEX
> >> +};
> >
> > And this was moved becasue...?
> 
> Because I need LAST_INDEX in next part that you quoted.

Ah, missed that, yes.

> >> +
> >>  typedef struct AC97LinkState {
> >>      PCIDevice dev;
> >>      QEMUSoundCard card;
> >> @@ -162,6 +169,7 @@ typedef struct AC97LinkState {
> >>      uint8_t silence[128];
> >>      uint32_t base[2];
> >>      int bup_flag;
> >> +    uint8_t active[LAST_INDEX];
> >
> > This doesn't belong here, cause the only purpose i can see is to hack
> > around defficiencies of the new load/savevm APIs.
> 
> That was supposed to be one of the features, not deficiences.  You can't
> sent stuff that it is not in the state. It is "by design" that you can't
> sent arbitrary variables.

active doesn't belong in the above structure, it's not used for anything
other than save/loadvm. If this "design" doesn't allow this, either find
another way to accomplish the same or fix the "design".
Juan Quintela - Sept. 30, 2009, 12:05 p.m.
malc <av1474@comtv.ru> wrote:
> On Wed, 30 Sep 2009, Juan Quintela wrote:

> active doesn't belong in the above structure, it's not used for anything
> other than save/loadvm.

It is used for reset, for instance.  We can discuss if it belongs there
or not, but will let that for another day.

> If this "design" doesn't allow this, either find
> another way to accomplish the same or fix the "design".

VMStateDescription is a list of offsets from a known address.  Address
is the one from the state.  That is the design.

Now, back to ac97.  Rest of ac97 don't need the active field, because it
is stored anywhere else (basically where AUD_set_active* store it).

Clearly it is part of ac97 state, becase it is needed for load/save/reset to
work properly.  It just happens that it is stored anywhere else for the
design of the audio system.

At load/save time we don't want to call malloc(), and now we have
descriptions of the state, not functions that save/load the state.
Only sane way of storing this kind of variables is storing them
into the state.

Sorry, there is no way that we are going to have a declarative
description of the state and retain the old functions to do save/load.
You can only get only declarative description or functions, not both.

There are more temporary variables in other devices, and this was the
way it was done there.  Normally I would have called it active_vmstate
to make that explicit, but here, it was also used for reset, that is the
way I named it with the _vmstate suffix.

Later, Juan.
malc - Sept. 30, 2009, 12:17 p.m.
On Wed, 30 Sep 2009, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> > On Wed, 30 Sep 2009, Juan Quintela wrote:
> 
> > active doesn't belong in the above structure, it's not used for anything
> > other than save/loadvm.
> 
> It is used for reset, for instance.  We can discuss if it belongs there
> or not, but will let that for another day.
> 
> > If this "design" doesn't allow this, either find
> > another way to accomplish the same or fix the "design".
> 
> VMStateDescription is a list of offsets from a known address.  Address
> is the one from the state.  That is the design.
> 
> Now, back to ac97.  Rest of ac97 don't need the active field, because it
> is stored anywhere else (basically where AUD_set_active* store it).
> 
> Clearly it is part of ac97 state, becase it is needed for load/save/reset to
> work properly.  It just happens that it is stored anywhere else for the
> design of the audio system.
> 
> At load/save time we don't want to call malloc(), and now we have
> descriptions of the state, not functions that save/load the state.
> Only sane way of storing this kind of variables is storing them
> into the state.
> 
> Sorry, there is no way that we are going to have a declarative
> description of the state and retain the old functions to do save/load.
> You can only get only declarative description or functions, not both.

I don't want to have declarative description to begin with, i was and
sitll am perfectly happy with how it was done before, the monstricity
of the new system is frightening and things are kept being added all
the time.

> 
> There are more temporary variables in other devices, and this was the
> way it was done there.  Normally I would have called it active_vmstate
> to make that explicit, but here, it was also used for reset, that is the
> way I named it with the _vmstate suffix.

In any case the while i can understand the fear of malloc, nobody forces
you to do that you can have a scratch space on the stack, with an some
upper cap, that's the way it's done now anyhow, only the cap is stack size
reserved for the process and not something you have to choose.
Juan Quintela - Sept. 30, 2009, 1:07 p.m.
malc <av1474@comtv.ru> wrote:
> On Wed, 30 Sep 2009, Juan Quintela wrote:
>
>> malc <av1474@comtv.ru> wrote:
>> > On Wed, 30 Sep 2009, Juan Quintela wrote:
>> 
>> > active doesn't belong in the above structure, it's not used for anything
>> > other than save/loadvm.
>> 
>> It is used for reset, for instance.  We can discuss if it belongs there
>> or not, but will let that for another day.
>> 
>> > If this "design" doesn't allow this, either find
>> > another way to accomplish the same or fix the "design".
>> 
>> VMStateDescription is a list of offsets from a known address.  Address
>> is the one from the state.  That is the design.
>> 
>> Now, back to ac97.  Rest of ac97 don't need the active field, because it
>> is stored anywhere else (basically where AUD_set_active* store it).
>> 
>> Clearly it is part of ac97 state, becase it is needed for load/save/reset to
>> work properly.  It just happens that it is stored anywhere else for the
>> design of the audio system.
>> 
>> At load/save time we don't want to call malloc(), and now we have
>> descriptions of the state, not functions that save/load the state.
>> Only sane way of storing this kind of variables is storing them
>> into the state.
>> 
>> Sorry, there is no way that we are going to have a declarative
>> description of the state and retain the old functions to do save/load.
>> You can only get only declarative description or functions, not both.
>
> I don't want to have declarative description to begin with, i was and
> sitll am perfectly happy with how it was done before, the monstricity
> of the new system is frightening and things are kept being added all
> the time.

I can't answer to that.

>> 
>> There are more temporary variables in other devices, and this was the
>> way it was done there.  Normally I would have called it active_vmstate
>> to make that explicit, but here, it was also used for reset, that is the
>> way I named it with the _vmstate suffix.
>
> In any case the while i can understand the fear of malloc, nobody forces
> you to do that you can have a scratch space on the stack, with an some
> upper cap, that's the way it's done now anyhow, only the cap is stack size
> reserved for the process and not something you have to choose.

You call new way a monstrosity, and now propose a solution that makes
the monster bigger?

I still stand behind the patch, and I still want it applied.

Later, Juan.
malc - Sept. 30, 2009, 1:12 p.m.
On Wed, 30 Sep 2009, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> > On Wed, 30 Sep 2009, Juan Quintela wrote:
> >
> >> malc <av1474@comtv.ru> wrote:
> >> > On Wed, 30 Sep 2009, Juan Quintela wrote:
> >> 
> >> > active doesn't belong in the above structure, it's not used for anything
> >> > other than save/loadvm.
> >> 
> >> It is used for reset, for instance.  We can discuss if it belongs there
> >> or not, but will let that for another day.
> >> 
> >> > If this "design" doesn't allow this, either find
> >> > another way to accomplish the same or fix the "design".
> >> 
> >> VMStateDescription is a list of offsets from a known address.  Address
> >> is the one from the state.  That is the design.
> >> 
> >> Now, back to ac97.  Rest of ac97 don't need the active field, because it
> >> is stored anywhere else (basically where AUD_set_active* store it).
> >> 
> >> Clearly it is part of ac97 state, becase it is needed for load/save/reset to
> >> work properly.  It just happens that it is stored anywhere else for the
> >> design of the audio system.
> >> 
> >> At load/save time we don't want to call malloc(), and now we have
> >> descriptions of the state, not functions that save/load the state.
> >> Only sane way of storing this kind of variables is storing them
> >> into the state.
> >> 
> >> Sorry, there is no way that we are going to have a declarative
> >> description of the state and retain the old functions to do save/load.
> >> You can only get only declarative description or functions, not both.
> >
> > I don't want to have declarative description to begin with, i was and
> > sitll am perfectly happy with how it was done before, the monstricity
> > of the new system is frightening and things are kept being added all
> > the time.
> 
> I can't answer to that.
> 
> >> 
> >> There are more temporary variables in other devices, and this was the
> >> way it was done there.  Normally I would have called it active_vmstate
> >> to make that explicit, but here, it was also used for reset, that is the
> >> way I named it with the _vmstate suffix.
> >
> > In any case the while i can understand the fear of malloc, nobody forces
> > you to do that you can have a scratch space on the stack, with an some
> > upper cap, that's the way it's done now anyhow, only the cap is stack size
> > reserved for the process and not something you have to choose.
> 
> You call new way a monstrosity, and now propose a solution that makes
> the monster bigger?
> 
> I still stand behind the patch, and I still want it applied.
> 

And it wont be, not this part of it, not in this state.
Juan Quintela - Sept. 30, 2009, 1:37 p.m.
malc <av1474@comtv.ru> wrote:
>> >> There are more temporary variables in other devices, and this was the
>> >> way it was done there.  Normally I would have called it active_vmstate
>> >> to make that explicit, but here, it was also used for reset, that is the
>> >> way I named it with the _vmstate suffix.
>> >
>> > In any case the while i can understand the fear of malloc, nobody forces
>> > you to do that you can have a scratch space on the stack, with an some
>> > upper cap, that's the way it's done now anyhow, only the cap is stack size
>> > reserved for the process and not something you have to choose.
>> 
>> You call new way a monstrosity, and now propose a solution that makes
>> the monster bigger?
>> 
>> I still stand behind the patch, and I still want it applied.
>> 
>
> And it wont be, not this part of it, not in this state.

Suggestions?  Because you are not telling me that you expect to move
something to the stack, reimplement alloca() and work from there was a
_real suggestion_?

Later, Juan.
malc - Sept. 30, 2009, 1:44 p.m.
On Wed, 30 Sep 2009, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> >> >> There are more temporary variables in other devices, and this was the
> >> >> way it was done there.  Normally I would have called it active_vmstate
> >> >> to make that explicit, but here, it was also used for reset, that is the
> >> >> way I named it with the _vmstate suffix.
> >> >
> >> > In any case the while i can understand the fear of malloc, nobody forces
> >> > you to do that you can have a scratch space on the stack, with an some
> >> > upper cap, that's the way it's done now anyhow, only the cap is stack size
> >> > reserved for the process and not something you have to choose.
> >> 
> >> You call new way a monstrosity, and now propose a solution that makes
> >> the monster bigger?
> >> 
> >> I still stand behind the patch, and I still want it applied.
> >> 
> >
> > And it wont be, not this part of it, not in this state.
> 
> Suggestions?  Because you are not telling me that you expect to move
> something to the stack, reimplement alloca() and work from there was a
> _real suggestion_?
> 

Yet somehow you are explecting me to solve the problems with your design
for you, neat. Alloca can be used but is not strictly necessary.

char buf[UPPER_CAP]; and maybe temp_fields that point to it and survive
the period between pre/post functions, would suffice, but anyhow, it's 
your problem to solve, to reiterate i'm quite happy with what is there
now.
Juan Quintela - Sept. 30, 2009, 1:57 p.m.
malc <av1474@comtv.ru> wrote:
> On Wed, 30 Sep 2009, Juan Quintela wrote:
>
>> malc <av1474@comtv.ru> wrote:
>> >> >> There are more temporary variables in other devices, and this was the
>> >> >> way it was done there.  Normally I would have called it active_vmstate
>> >> >> to make that explicit, but here, it was also used for reset, that is the
>> >> >> way I named it with the _vmstate suffix.
>> >> >
>> >> > In any case the while i can understand the fear of malloc, nobody forces
>> >> > you to do that you can have a scratch space on the stack, with an some
>> >> > upper cap, that's the way it's done now anyhow, only the cap is stack size
>> >> > reserved for the process and not something you have to choose.
>> >> 
>> >> You call new way a monstrosity, and now propose a solution that makes
>> >> the monster bigger?
>> >> 
>> >> I still stand behind the patch, and I still want it applied.
>> >> 
>> >
>> > And it wont be, not this part of it, not in this state.
>> 
>> Suggestions?  Because you are not telling me that you expect to move
>> something to the stack, reimplement alloca() and work from there was a
>> _real suggestion_?
>> 
>
> Yet somehow you are explecting me to solve the problems with your design
> for you, neat. Alloca can be used but is not strictly necessary.
>
> char buf[UPPER_CAP]; and maybe temp_fields that point to it and survive
> the period between pre/post functions, would suffice, but anyhow, it's 
> your problem to solve, to reiterate i'm quite happy with what is there
> now.

No.  Everywhere code changed as I changed in ac97.  And everybody
agrees.  And now you told that everybody else was wrong, and that the
only true way is changing everything else for a worse/uglier solution.
As I told before, you have commit access, you win.

Discussion is at the point:
- you will accept _any_ solution that means not changing ac97.  No
  compromises taken.
- I will not make VMState worse/uglier/more complex just to work-around
  your veto.

Patch don't goes in, you win and I have lost time porting ac97 to VMState.

Later, Juan.
malc - Sept. 30, 2009, 2:53 p.m.
On Wed, 30 Sep 2009, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> > On Wed, 30 Sep 2009, Juan Quintela wrote:
> >
> >> malc <av1474@comtv.ru> wrote:
> >> >> >> There are more temporary variables in other devices, and this was the
> >> >> >> way it was done there.  Normally I would have called it active_vmstate
> >> >> >> to make that explicit, but here, it was also used for reset, that is the
> >> >> >> way I named it with the _vmstate suffix.
> >> >> >
> >> >> > In any case the while i can understand the fear of malloc, nobody forces
> >> >> > you to do that you can have a scratch space on the stack, with an some
> >> >> > upper cap, that's the way it's done now anyhow, only the cap is stack size
> >> >> > reserved for the process and not something you have to choose.
> >> >> 
> >> >> You call new way a monstrosity, and now propose a solution that makes
> >> >> the monster bigger?
> >> >> 
> >> >> I still stand behind the patch, and I still want it applied.
> >> >> 
> >> >
> >> > And it wont be, not this part of it, not in this state.
> >> 
> >> Suggestions?  Because you are not telling me that you expect to move
> >> something to the stack, reimplement alloca() and work from there was a
> >> _real suggestion_?
> >> 
> >
> > Yet somehow you are explecting me to solve the problems with your design
> > for you, neat. Alloca can be used but is not strictly necessary.
> >
> > char buf[UPPER_CAP]; and maybe temp_fields that point to it and survive
> > the period between pre/post functions, would suffice, but anyhow, it's 
> > your problem to solve, to reiterate i'm quite happy with what is there
> > now.
> 
> No.  Everywhere code changed as I changed in ac97.  And everybody
> agrees.  And now you told that everybody else was wrong, and that the
> only true way is changing everything else for a worse/uglier solution.
> As I told before, you have commit access, you win.
> 
> Discussion is at the point:
> - you will accept _any_ solution that means not changing ac97.  No
>   compromises taken.

I dont get this part, if you meant - am i against the patch modulo ac97
changes, then no i'm not, it's the respective maintainers opinion what
matters.

> - I will not make VMState worse/uglier/more complex just to work-around
>   your veto.
> 
> Patch don't goes in, you win and I have lost time porting ac97 to VMState.

Yep.
Juan Quintela - Sept. 30, 2009, 3:02 p.m.
malc <av1474@comtv.ru> wrote:
> On Wed, 30 Sep 2009, Juan Quintela wrote:
>> 
>> Discussion is at the point:
>> - you will accept _any_ solution that means not changing ac97.  No
>>   compromises taken.
>
> I dont get this part, if you meant - am i against the patch modulo ac97
> changes, then no i'm not, it's the respective maintainers opinion what
> matters.

Just to clarify, I just meaned the ac97 patch.

>> - I will not make VMState worse/uglier/more complex just to work-around
>>   your veto.
>> 
>> Patch don't goes in, you win and I have lost time porting ac97 to VMState.
>
> Yep.

Later, Juan.
Anthony Liguori - Sept. 30, 2009, 3:08 p.m.
malc wrote:
>>>> +
>>>>  typedef struct AC97LinkState {
>>>>      PCIDevice dev;
>>>>      QEMUSoundCard card;
>>>> @@ -162,6 +169,7 @@ typedef struct AC97LinkState {
>>>>      uint8_t silence[128];
>>>>      uint32_t base[2];
>>>>      int bup_flag;
>>>> +    uint8_t active[LAST_INDEX];
>>>>         
>>> This doesn't belong here, cause the only purpose i can see is to hack
>>> around defficiencies of the new load/savevm APIs.
>>>       
>> That was supposed to be one of the features, not deficiences.  You can't
>> sent stuff that it is not in the state. It is "by design" that you can't
>> sent arbitrary variables.
>>     
>
> active doesn't belong in the above structure, it's not used for anything
> other than save/loadvm. If this "design" doesn't allow this, either find
> another way to accomplish the same or fix the "design".
>   

Looking briefly at the code, it looks like the active[] array isn't 
technically needed in the savevm state.

I think you could basically do:

AUD_set_active_in(s->voice_pi, !!(s->bm_regs[PI_INDEX].cr & CR_RPBM));
...

Better yet, active[] can be dynamically built from the contents of 
bm_regs->cr so there would be little code change.

So I think we should bump the version of the ac97 format, remove the 
active[] array from the vmstate, and then generate it in a post function 
that can then be passed to reset_voices().

Regards,

Anthony Liguori
Juan Quintela - Sept. 30, 2009, 3:22 p.m.
Anthony Liguori <anthony@codemonkey.ws> wrote:
> malc wrote:
>>>>> +
>>>>>  typedef struct AC97LinkState {
>>>>>      PCIDevice dev;
>>>>>      QEMUSoundCard card;
>>>>> @@ -162,6 +169,7 @@ typedef struct AC97LinkState {
>>>>>      uint8_t silence[128];
>>>>>      uint32_t base[2];
>>>>>      int bup_flag;
>>>>> +    uint8_t active[LAST_INDEX];
>>>>>         
>>>> This doesn't belong here, cause the only purpose i can see is to hack
>>>> around defficiencies of the new load/savevm APIs.
>>>>       
>>> That was supposed to be one of the features, not deficiences.  You can't
>>> sent stuff that it is not in the state. It is "by design" that you can't
>>> sent arbitrary variables.
>>>     
>>
>> active doesn't belong in the above structure, it's not used for anything
>> other than save/loadvm. If this "design" doesn't allow this, either find
>> another way to accomplish the same or fix the "design".
>>   
>
> Looking briefly at the code, it looks like the active[] array isn't
> technically needed in the savevm state.
>
> I think you could basically do:
>
> AUD_set_active_in(s->voice_pi, !!(s->bm_regs[PI_INDEX].cr & CR_RPBM));
> ...

I don't doubt that you are right.  I don't understand that bit of the
code.  active is created in other way, but I don't know if they are
equivalent or not.


> Better yet, active[] can be dynamically built from the contents of
> bm_regs->cr so there would be little code change.
>
> So I think we should bump the version of the ac97 format, remove the
> active[] array from the vmstate, and then generate it in a post
> function that can then be passed to reset_voices().

Malc?

Later, Juan.
malc - Sept. 30, 2009, 3:24 p.m.
On Wed, 30 Sep 2009, Anthony Liguori wrote:

> malc wrote:
> > > > > +
> > > > >  typedef struct AC97LinkState {
> > > > >      PCIDevice dev;
> > > > >      QEMUSoundCard card;
> > > > > @@ -162,6 +169,7 @@ typedef struct AC97LinkState {
> > > > >      uint8_t silence[128];
> > > > >      uint32_t base[2];
> > > > >      int bup_flag;
> > > > > +    uint8_t active[LAST_INDEX];
> > > > >         
> > > > This doesn't belong here, cause the only purpose i can see is to hack
> > > > around defficiencies of the new load/savevm APIs.
> > > >       
> > > That was supposed to be one of the features, not deficiences.  You can't
> > > sent stuff that it is not in the state. It is "by design" that you can't
> > > sent arbitrary variables.
> > >     
> > 
> > active doesn't belong in the above structure, it's not used for anything
> > other than save/loadvm. If this "design" doesn't allow this, either find
> > another way to accomplish the same or fix the "design".
> >   
> 
> Looking briefly at the code, it looks like the active[] array isn't
> technically needed in the savevm state.
> 
> I think you could basically do:
> 
> AUD_set_active_in(s->voice_pi, !!(s->bm_regs[PI_INDEX].cr & CR_RPBM));
> ..
> 
> Better yet, active[] can be dynamically built from the contents of 
> bm_regs->cr so there would be little code change.
> 
> So I think we should bump the version of the ac97 format, remove the 
> active[] array from the vmstate, and then generate it in a post function 
> that can then be passed to reset_voices().

Yes, however, what bothers me though, is that i ended up NOT doing that
when the original code was written, there must have been a reason for me
not doing that, and it completely elludes me at the moment, so no this
isn't better yet, better yet implies someone having to go through the
code and figure out whether it's safe or not.
Anthony Liguori - Sept. 30, 2009, 9:11 p.m.
malc wrote:
> On Wed, 30 Sep 2009, Anthony Liguori wrote:
>   
>> So I think we should bump the version of the ac97 format, remove the 
>> active[] array from the vmstate, and then generate it in a post function 
>> that can then be passed to reset_voices().
>>     
>
> Yes, however, what bothers me though, is that i ended up NOT doing that
> when the original code was written, there must have been a reason for me
> not doing that, and it completely elludes me at the moment, so no this
> isn't better yet, better yet implies someone having to go through the
> code and figure out whether it's safe or not.
>   

I'm pretty convinced it's safe.  The only way it wouldn't be is if it 
was possible for the active_in/active_out state in AUD to be changed 
through a means other than the device itself.  Looking at audio.c, it's 
not possible.

But I also expect that Juan will be testing this device model when he 
resubmits the patch so that should confirm it.

Regards,

Anthony Liguori
malc - Sept. 30, 2009, 9:19 p.m.
On Wed, 30 Sep 2009, Anthony Liguori wrote:

> malc wrote:
> > On Wed, 30 Sep 2009, Anthony Liguori wrote:
> >   
> > > So I think we should bump the version of the ac97 format, remove the
> > > active[] array from the vmstate, and then generate it in a post function
> > > that can then be passed to reset_voices().
> > >     
> > 
> > Yes, however, what bothers me though, is that i ended up NOT doing that
> > when the original code was written, there must have been a reason for me
> > not doing that, and it completely elludes me at the moment, so no this
> > isn't better yet, better yet implies someone having to go through the
> > code and figure out whether it's safe or not.
> >   
> 
> I'm pretty convinced it's safe.  The only way it wouldn't be is if it was
> possible for the active_in/active_out state in AUD to be changed through a
> means other than the device itself.  Looking at audio.c, it's not possible.
> 
> But I also expect that Juan will be testing this device model when he
> resubmits the patch so that should confirm it.

Okay then, given it is tested.

Patch

diff --git a/hw/ac97.c b/hw/ac97.c
index 610ca60..da6cb2d 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -146,6 +146,13 @@  typedef struct AC97BusMasterRegs {
     BD bd;
 } AC97BusMasterRegs;

+enum {
+    PI_INDEX = 0,
+    PO_INDEX,
+    MC_INDEX,
+    LAST_INDEX
+};
+
 typedef struct AC97LinkState {
     PCIDevice dev;
     QEMUSoundCard card;
@@ -162,6 +169,7 @@  typedef struct AC97LinkState {
     uint8_t silence[128];
     uint32_t base[2];
     int bup_flag;
+    uint8_t active[LAST_INDEX];
 } AC97LinkState;

 enum {
@@ -186,13 +194,6 @@  enum {                                          \
     prefix ## _CR = start + 11                  \
 }

-enum {
-    PI_INDEX = 0,
-    PO_INDEX,
-    MC_INDEX,
-    LAST_INDEX
-};
-
 MKREGS (PI, PI_INDEX * 16);
 MKREGS (PO, PO_INDEX * 16);
 MKREGS (MC, MC_INDEX * 16);
@@ -414,21 +415,21 @@  static void open_voice (AC97LinkState *s, int index, int freq)
     }
 }

-static void reset_voices (AC97LinkState *s, uint8_t active[LAST_INDEX])
+static void reset_voices (AC97LinkState *s)
 {
     uint16_t freq;

     freq = mixer_load (s, AC97_PCM_LR_ADC_Rate);
     open_voice (s, PI_INDEX, freq);
-    AUD_set_active_in (s->voice_pi, active[PI_INDEX]);
+    AUD_set_active_in (s->voice_pi, s->active[PI_INDEX]);

     freq = mixer_load (s, AC97_PCM_Front_DAC_Rate);
     open_voice (s, PO_INDEX, freq);
-    AUD_set_active_out (s->voice_po, active[PO_INDEX]);
+    AUD_set_active_out (s->voice_po, s->active[PO_INDEX]);

     freq = mixer_load (s, AC97_MIC_ADC_Rate);
     open_voice (s, MC_INDEX, freq);
-    AUD_set_active_in (s->voice_mc, active[MC_INDEX]);
+    AUD_set_active_in (s->voice_mc, s->active[MC_INDEX]);
 }

 #ifdef USE_MIXER
@@ -526,11 +527,10 @@  static void record_select (AC97LinkState *s, uint32_t val)

 static void mixer_reset (AC97LinkState *s)
 {
-    uint8_t active[LAST_INDEX];

     dolog ("mixer_reset\n");
     memset (s->mixer_data, 0, sizeof (s->mixer_data));
-    memset (active, 0, sizeof (active));
+    memset (s->active, 0, sizeof (s->active));
     mixer_store (s, AC97_Reset                   , 0x0000); /* 6940 */
     mixer_store (s, AC97_Master_Volume_Mono_Mute , 0x8000);
     mixer_store (s, AC97_PC_BEEP_Volume_Mute     , 0x0000);
@@ -564,7 +564,7 @@  static void mixer_reset (AC97LinkState *s)
     set_volume (s, AC97_PCM_Out_Volume_Mute, AUD_MIXER_PCM    , 0x8808);
     set_volume (s, AC97_Line_In_Volume_Mute, AUD_MIXER_LINE_IN, 0x8808);
 #endif
-    reset_voices (s, active);
+    reset_voices (s);
 }

 /**
@@ -1170,7 +1170,6 @@  static void po_callback (void *opaque, int free)
 static void ac97_save (QEMUFile *f, void *opaque)
 {
     size_t i;
-    uint8_t active[LAST_INDEX];
     AC97LinkState *s = opaque;

     pci_device_save (&s->dev, f);
@@ -1194,17 +1193,16 @@  static void ac97_save (QEMUFile *f, void *opaque)
     }
     qemu_put_buffer (f, s->mixer_data, sizeof (s->mixer_data));

-    active[PI_INDEX] = AUD_is_active_in (s->voice_pi) ? 1 : 0;
-    active[PO_INDEX] = AUD_is_active_out (s->voice_po) ? 1 : 0;
-    active[MC_INDEX] = AUD_is_active_in (s->voice_mc) ? 1 : 0;
-    qemu_put_buffer (f, active, sizeof (active));
+    s->active[PI_INDEX] = AUD_is_active_in (s->voice_pi) ? 1 : 0;
+    s->active[PO_INDEX] = AUD_is_active_out (s->voice_po) ? 1 : 0;
+    s->active[MC_INDEX] = AUD_is_active_in (s->voice_mc) ? 1 : 0;
+    qemu_put_buffer (f, s->active, sizeof (s->active));
 }

 static int ac97_load (QEMUFile *f, void *opaque, int version_id)
 {
     int ret;
     size_t i;
-    uint8_t active[LAST_INDEX];
     AC97LinkState *s = opaque;

     if (version_id != 2)
@@ -1232,7 +1230,7 @@  static int ac97_load (QEMUFile *f, void *opaque, int version_id)
         qemu_get_be32s (f, &r->bd.ctl_len);
     }
     qemu_get_buffer (f, s->mixer_data, sizeof (s->mixer_data));
-    qemu_get_buffer (f, active, sizeof (active));
+    qemu_get_buffer (f, s->active, sizeof (s->active));

 #ifdef USE_MIXER
     record_select (s, mixer_load (s, AC97_Record_Select));
@@ -1242,7 +1240,7 @@  static int ac97_load (QEMUFile *f, void *opaque, int version_id)
     V_ (AC97_Line_In_Volume_Mute, AUD_MIXER_LINE_IN);
 #undef V_
 #endif
-    reset_voices (s, active);
+    reset_voices (s);

     s->bup_flag = 0;
     s->last_samp = 0;