Re: [Qemu-devel] [PATCH v3 15/50] audio: reduce glob_audio_state usage
On Thu, Jan 24, 2019 at 09:12:58PM +0100, Zoltán Kővágó wrote: > On 2019-01-24 12:19, Gerd Hoffmann wrote: > > Hi, > > > >> So, I think with the first part the only open issue is whenever we go > >> with the nested types (i.e. patch #1 as-is) or not. Given that the > >> one-element-structs added in that patch will get additional fields I > >> think the nesting makes sense. > > > > Spoke too soon: scripts/checkpatch.pl flags a bunch of codestyle issues. > > Most of them are about the code style of the old audio subsystem, I > fixed some of them but not everything. IIRC last time it wasn't a > problem, but it was in 2015. Should I go over them again and fix all of > them? The first ones I saw where not a old audio codestyle (which is whitespace-after-function-name mostly) issues but newly introduced ones. A few rules have been added since 2015. Fixing the existing issues due to old audio code style (when changing/moving code) is fine, but not required. Newly added code should follow usual qemu code style. cheers, Gerd
Re: [Qemu-devel] [PATCH v3 15/50] audio: reduce glob_audio_state usage
On 2019-01-24 12:19, Gerd Hoffmann wrote: > Hi, > >> So, I think with the first part the only open issue is whenever we go >> with the nested types (i.e. patch #1 as-is) or not. Given that the >> one-element-structs added in that patch will get additional fields I >> think the nesting makes sense. > > Spoke too soon: scripts/checkpatch.pl flags a bunch of codestyle issues. > Most of them are about the code style of the old audio subsystem, I fixed some of them but not everything. IIRC last time it wasn't a problem, but it was in 2015. Should I go over them again and fix all of them? Regards, Zoltan
Re: [Qemu-devel] [PATCH v3 15/50] audio: reduce glob_audio_state usage
Hi, > So, I think with the first part the only open issue is whenever we go > with the nested types (i.e. patch #1 as-is) or not. Given that the > one-element-structs added in that patch will get additional fields I > think the nesting makes sense. Spoke too soon: scripts/checkpatch.pl flags a bunch of codestyle issues. cheers, Gerd
Re: [Qemu-devel] [PATCH v3 15/50] audio: reduce glob_audio_state usage
On Wed, Jan 23, 2019 at 09:16:07PM +0100, Zoltán Kővágó wrote: > On 2019-01-17 10:22, Gerd Hoffmann wrote: > > On Thu, Jan 17, 2019 at 12:36:48AM +0100, Kővágó, Zoltán wrote: > >> Remove glob_audio_state from functions, where possible without breaking > >> the API. This means that most static functions in audio.c now take an > >> AudioState pointer instead of implicitly using glob_audio_state. Also > >> included a pointer in SWVoice*, HWVoice* structs, so that functions > >> dealing them can know the audio state without having to pass it around > >> separately. > >> > >> This is required in order to support multiple simultaneous audio > >> backends (added in a later commit). > > > > I think it makes sense to split the series into smaller pieces, here > > would be a good place for the split (i.e. patches 1-14 which introduce > > -audiodev will be the first batch, this patch starts the next batch). > > Alright, I think this series is pretty useless without at least the > multiple simultaneous backends part, but if it's easier to review in > smaller chunks, let's do it. Also start merging stuff even though later in the series there are still open issues to be tackled (coreaudio breaking for example). So, I think with the first part the only open issue is whenever we go with the nested types (i.e. patch #1 as-is) or not. Given that the one-element-structs added in that patch will get additional fields I think the nesting makes sense. cheers, Gerd
Re: [Qemu-devel] [PATCH v3 15/50] audio: reduce glob_audio_state usage
On 2019-01-17 10:22, Gerd Hoffmann wrote:
> On Thu, Jan 17, 2019 at 12:36:48AM +0100, Kővágó, Zoltán wrote:
>> Remove glob_audio_state from functions, where possible without breaking
>> the API. This means that most static functions in audio.c now take an
>> AudioState pointer instead of implicitly using glob_audio_state. Also
>> included a pointer in SWVoice*, HWVoice* structs, so that functions
>> dealing them can know the audio state without having to pass it around
>> separately.
>>
>> This is required in order to support multiple simultaneous audio
>> backends (added in a later commit).
>
> I think it makes sense to split the series into smaller pieces, here
> would be a good place for the split (i.e. patches 1-14 which introduce
> -audiodev will be the first batch, this patch starts the next batch).
Alright, I think this series is pretty useless without at least the
multiple simultaneous backends part, but if it's easier to review in
smaller chunks, let's do it.
>
>>
>> Signed-off-by: Kővágó, Zoltán
>> ---
>> audio/audio_int.h | 7 +
>> audio/audio_template.h | 46
>> audio/audio.c | 59 +++---
>> 3 files changed, 56 insertions(+), 56 deletions(-)
>>
>> diff --git a/audio/audio_int.h b/audio/audio_int.h
>> index 66214199f0..2a7556d113 100644
>> --- a/audio/audio_int.h
>> +++ b/audio/audio_int.h
>> @@ -52,6 +52,7 @@ struct audio_pcm_info {
>> typedef struct SWVoiceCap SWVoiceCap;
>>
>> typedef struct HWVoiceOut {
>> +AudioState *s;
>> int enabled;
>> int poll_mode;
>> int pending_disable;
>> @@ -73,6 +74,7 @@ typedef struct HWVoiceOut {
>> } HWVoiceOut;
>>
>> typedef struct HWVoiceIn {
>> +AudioState *s;
>> int enabled;
>> int poll_mode;
>> struct audio_pcm_info info;
>> @@ -94,6 +96,7 @@ typedef struct HWVoiceIn {
>>
>> struct SWVoiceOut {
>> QEMUSoundCard *card;
>> +AudioState *s;
>> struct audio_pcm_info info;
>> t_sample *conv;
>> int64_t ratio;
>> @@ -111,6 +114,7 @@ struct SWVoiceOut {
>>
>> struct SWVoiceIn {
>> QEMUSoundCard *card;
>> +AudioState *s;
>> int active;
>> struct audio_pcm_info info;
>> int64_t ratio;
>> @@ -188,6 +192,9 @@ struct AudioState {
>> int nb_hw_voices_in;
>> int vm_running;
>> int64_t period_ticks;
>> +
>> +bool timer_running;
>> +uint64_t timer_last;
>> };
>>
>> extern const struct mixeng_volume nominal_volume;
>> diff --git a/audio/audio_template.h b/audio/audio_template.h
>> index c1d7207abd..ccf6d810f7 100644
>> --- a/audio/audio_template.h
>> +++ b/audio/audio_template.h
>> @@ -36,9 +36,9 @@
>> #define HWBUF hw->conv_buf
>> #endif
>>
>> -static void glue (audio_init_nb_voices_, TYPE) (struct audio_driver *drv)
>> +static void glue(audio_init_nb_voices_, TYPE)(AudioState *s,
>> + struct audio_driver *drv)
>> {
>> -AudioState *s = &glob_audio_state;
>> int max_voices = glue (drv->max_voices_, TYPE);
>> int voice_size = glue (drv->voice_size_, TYPE);
>>
>> @@ -183,8 +183,8 @@ static void glue (audio_pcm_hw_del_sw_, TYPE) (SW *sw)
>>
>> static void glue (audio_pcm_hw_gc_, TYPE) (HW **hwp)
>> {
>> -AudioState *s = &glob_audio_state;
>> HW *hw = *hwp;
>> +AudioState *s = hw->s;
>>
>> if (!hw->sw_head.lh_first) {
>> #ifdef DAC
>> @@ -199,15 +199,14 @@ static void glue (audio_pcm_hw_gc_, TYPE) (HW **hwp)
>> }
>> }
>>
>> -static HW *glue (audio_pcm_hw_find_any_, TYPE) (HW *hw)
>> +static HW *glue(audio_pcm_hw_find_any_, TYPE)(AudioState *s, HW *hw)
>> {
>> -AudioState *s = &glob_audio_state;
>> return hw ? hw->entries.le_next : glue (s->hw_head_, TYPE).lh_first;
>> }
>>
>> -static HW *glue (audio_pcm_hw_find_any_enabled_, TYPE) (HW *hw)
>> +static HW *glue(audio_pcm_hw_find_any_enabled_, TYPE)(AudioState *s, HW *hw)
>> {
>> -while ((hw = glue (audio_pcm_hw_find_any_, TYPE) (hw))) {
>> +while ((hw = glue(audio_pcm_hw_find_any_, TYPE)(s, hw))) {
>> if (hw->enabled) {
>> return hw;
>> }
>> @@ -215,12 +214,10 @@ static HW *glue (audio_pcm_hw_find_any_enabled_, TYPE)
>> (HW *hw)
>> return NULL;
>> }
>>
>> -static HW *glue (audio_pcm_hw_find_specific_, TYPE) (
>> -HW *hw,
>> -struct audsettings *as
>> -)
>> +static HW *glue(audio_pcm_hw_find_specific_, TYPE)(AudioState *s, HW *hw,
>> + struct audsettings *as)
>> {
>> -while ((hw = glue (audio_pcm_hw_find_any_, TYPE) (hw))) {
>> +while ((hw = glue(audio_pcm_hw_find_any_, TYPE)(s, hw))) {
>> if (audio_pcm_info_eq (&hw->info, as)) {
>> return hw;
>> }
>> @@ -228,10 +225,10 @@ static HW *glue (audio_pcm_hw_find_specific_, TYPE) (
>> return NULL;
>> }
>>
>> -static HW *glue (audio_pcm_hw_add_new_, TYPE) (struct audsettings *as)
>> +static HW *glue(au
Re: [Qemu-devel] [PATCH v3 15/50] audio: reduce glob_audio_state usage
On Thu, Jan 17, 2019 at 12:36:48AM +0100, Kővágó, Zoltán wrote:
> Remove glob_audio_state from functions, where possible without breaking
> the API. This means that most static functions in audio.c now take an
> AudioState pointer instead of implicitly using glob_audio_state. Also
> included a pointer in SWVoice*, HWVoice* structs, so that functions
> dealing them can know the audio state without having to pass it around
> separately.
>
> This is required in order to support multiple simultaneous audio
> backends (added in a later commit).
I think it makes sense to split the series into smaller pieces, here
would be a good place for the split (i.e. patches 1-14 which introduce
-audiodev will be the first batch, this patch starts the next batch).
>
> Signed-off-by: Kővágó, Zoltán
> ---
> audio/audio_int.h | 7 +
> audio/audio_template.h | 46
> audio/audio.c | 59 +++---
> 3 files changed, 56 insertions(+), 56 deletions(-)
>
> diff --git a/audio/audio_int.h b/audio/audio_int.h
> index 66214199f0..2a7556d113 100644
> --- a/audio/audio_int.h
> +++ b/audio/audio_int.h
> @@ -52,6 +52,7 @@ struct audio_pcm_info {
> typedef struct SWVoiceCap SWVoiceCap;
>
> typedef struct HWVoiceOut {
> +AudioState *s;
> int enabled;
> int poll_mode;
> int pending_disable;
> @@ -73,6 +74,7 @@ typedef struct HWVoiceOut {
> } HWVoiceOut;
>
> typedef struct HWVoiceIn {
> +AudioState *s;
> int enabled;
> int poll_mode;
> struct audio_pcm_info info;
> @@ -94,6 +96,7 @@ typedef struct HWVoiceIn {
>
> struct SWVoiceOut {
> QEMUSoundCard *card;
> +AudioState *s;
> struct audio_pcm_info info;
> t_sample *conv;
> int64_t ratio;
> @@ -111,6 +114,7 @@ struct SWVoiceOut {
>
> struct SWVoiceIn {
> QEMUSoundCard *card;
> +AudioState *s;
> int active;
> struct audio_pcm_info info;
> int64_t ratio;
> @@ -188,6 +192,9 @@ struct AudioState {
> int nb_hw_voices_in;
> int vm_running;
> int64_t period_ticks;
> +
> +bool timer_running;
> +uint64_t timer_last;
> };
>
> extern const struct mixeng_volume nominal_volume;
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index c1d7207abd..ccf6d810f7 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -36,9 +36,9 @@
> #define HWBUF hw->conv_buf
> #endif
>
> -static void glue (audio_init_nb_voices_, TYPE) (struct audio_driver *drv)
> +static void glue(audio_init_nb_voices_, TYPE)(AudioState *s,
> + struct audio_driver *drv)
> {
> -AudioState *s = &glob_audio_state;
> int max_voices = glue (drv->max_voices_, TYPE);
> int voice_size = glue (drv->voice_size_, TYPE);
>
> @@ -183,8 +183,8 @@ static void glue (audio_pcm_hw_del_sw_, TYPE) (SW *sw)
>
> static void glue (audio_pcm_hw_gc_, TYPE) (HW **hwp)
> {
> -AudioState *s = &glob_audio_state;
> HW *hw = *hwp;
> +AudioState *s = hw->s;
>
> if (!hw->sw_head.lh_first) {
> #ifdef DAC
> @@ -199,15 +199,14 @@ static void glue (audio_pcm_hw_gc_, TYPE) (HW **hwp)
> }
> }
>
> -static HW *glue (audio_pcm_hw_find_any_, TYPE) (HW *hw)
> +static HW *glue(audio_pcm_hw_find_any_, TYPE)(AudioState *s, HW *hw)
> {
> -AudioState *s = &glob_audio_state;
> return hw ? hw->entries.le_next : glue (s->hw_head_, TYPE).lh_first;
> }
>
> -static HW *glue (audio_pcm_hw_find_any_enabled_, TYPE) (HW *hw)
> +static HW *glue(audio_pcm_hw_find_any_enabled_, TYPE)(AudioState *s, HW *hw)
> {
> -while ((hw = glue (audio_pcm_hw_find_any_, TYPE) (hw))) {
> +while ((hw = glue(audio_pcm_hw_find_any_, TYPE)(s, hw))) {
> if (hw->enabled) {
> return hw;
> }
> @@ -215,12 +214,10 @@ static HW *glue (audio_pcm_hw_find_any_enabled_, TYPE)
> (HW *hw)
> return NULL;
> }
>
> -static HW *glue (audio_pcm_hw_find_specific_, TYPE) (
> -HW *hw,
> -struct audsettings *as
> -)
> +static HW *glue(audio_pcm_hw_find_specific_, TYPE)(AudioState *s, HW *hw,
> + struct audsettings *as)
> {
> -while ((hw = glue (audio_pcm_hw_find_any_, TYPE) (hw))) {
> +while ((hw = glue(audio_pcm_hw_find_any_, TYPE)(s, hw))) {
> if (audio_pcm_info_eq (&hw->info, as)) {
> return hw;
> }
> @@ -228,10 +225,10 @@ static HW *glue (audio_pcm_hw_find_specific_, TYPE) (
> return NULL;
> }
>
> -static HW *glue (audio_pcm_hw_add_new_, TYPE) (struct audsettings *as)
> +static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioState *s,
> + struct audsettings *as)
> {
> HW *hw;
> -AudioState *s = &glob_audio_state;
> struct audio_driver *drv = s->drv;
>
> if (!glue (s->nb_hw_voices_, TYPE)) {
> @@ -255,6 +252,7 @@ static HW *glue (audio_pcm_hw_add_new_, TYPE) (struct
> auds
