| Message ID | d93032e20d443aeb567c2dcee9c6d4d166b98d58.1254255997.git.quintela@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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; >
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.
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".
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.
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.
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.
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.
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.
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.
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.
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.
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.
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
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.
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.
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
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.
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;
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(-)