| Message ID | 1429635048-30665-1-git-send-email-martin.wilck@ts.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
Hi ----- Original Message ----- > This patch addresses the following problem: > > Moving the input volume slider 0% to 1% in a Win7 guest causes the > the "gain" values in the emulated HDA codec HW to jump from 0 to 40. > This makes it impossible to control sound volume in the low-gain > range using the windows guest. > This is related to how the Windows HDA codec driver and the QEMU > emulated HW interact. It seems that Windows uses a ~30dB overall > scale for the min-max range of the input volume slider. > > The "right" solution for this problem would be to implement > proper dB scaling in QEMU and the audio backends (such as spice). > > While this clean solution is not available, I suggest to decrease > the dynamic range for the the emulated Amps in the QEMU hda codec. > Experiments showed that with 32dB dynamic range with a 0.5 dB step, > it was possible to set gain values as low as 5 (-29.5dB / 8%). > Actual HW seems to use similar ranges for input amplifiers. > > Signed-off-by: Martin Wilck <martin.wilck@ts.fujitsu.com> > --- > hw/audio/hda-codec-common.h | 23 +++++++++++++++-------- > hw/audio/hda-codec.c | 23 +++++++++++++++++------ > 2 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/hw/audio/hda-codec-common.h b/hw/audio/hda-codec-common.h > index b4fdb51..d59781b 100644 > --- a/hw/audio/hda-codec-common.h > +++ b/hw/audio/hda-codec-common.h > @@ -28,16 +28,22 @@ > #define QEMU_HDA_ID_OUTPUT ((QEMU_HDA_ID_VENDOR << 16) | 0x12) > #define QEMU_HDA_ID_DUPLEX ((QEMU_HDA_ID_VENDOR << 16) | 0x22) > #define QEMU_HDA_ID_MICRO ((QEMU_HDA_ID_VENDOR << 16) | 0x32) > -#define QEMU_HDA_AMP_CAPS \ > +#define QEMU_HDA_AMP_OUT_CAPS \ > (AC_AMPCAP_MUTE | \ > - (QEMU_HDA_AMP_STEPS << AC_AMPCAP_OFFSET_SHIFT) | \ > - (QEMU_HDA_AMP_STEPS << AC_AMPCAP_NUM_STEPS_SHIFT) | \ > - (3 << AC_AMPCAP_STEP_SIZE_SHIFT)) > + (QEMU_HDA_AMP_OUT_STEPS << AC_AMPCAP_OFFSET_SHIFT) | \ > + (QEMU_HDA_AMP_OUT_STEPS << AC_AMPCAP_NUM_STEPS_SHIFT) | \ > + (QEMU_HDA_AMP_OUT_STEP_SIZE << AC_AMPCAP_STEP_SIZE_SHIFT)) > +#define QEMU_HDA_AMP_IN_CAPS \ > + (AC_AMPCAP_MUTE | \ > + (QEMU_HDA_AMP_IN_STEPS << AC_AMPCAP_OFFSET_SHIFT) | \ > + (QEMU_HDA_AMP_IN_STEPS << AC_AMPCAP_NUM_STEPS_SHIFT) | \ > + (QEMU_HDA_AMP_IN_STEP_SIZE << AC_AMPCAP_STEP_SIZE_SHIFT)) > #else > #define QEMU_HDA_ID_OUTPUT ((QEMU_HDA_ID_VENDOR << 16) | 0x11) > #define QEMU_HDA_ID_DUPLEX ((QEMU_HDA_ID_VENDOR << 16) | 0x21) > #define QEMU_HDA_ID_MICRO ((QEMU_HDA_ID_VENDOR << 16) | 0x31) > -#define QEMU_HDA_AMP_CAPS QEMU_HDA_AMP_NONE > +#define QEMU_HDA_AMP_OUT_CAPS QEMU_HDA_AMP_NONE > +#define QEMU_HDA_AMP_IN_CAPS QEMU_HDA_AMP_NONE > #endif > > > @@ -61,7 +67,7 @@ static const desc_param glue(common_params_audio_dac_, > PARAM)[] = { > .val = QEMU_HDA_AMP_NONE, > },{ > .id = AC_PAR_AMP_OUT_CAP, > - .val = QEMU_HDA_AMP_CAPS, > + .val = QEMU_HDA_AMP_OUT_CAPS, > }, > }; > > @@ -86,7 +92,7 @@ static const desc_param glue(common_params_audio_adc_, > PARAM)[] = { > .val = AC_SUPFMT_PCM, > },{ > .id = AC_PAR_AMP_IN_CAP, > - .val = QEMU_HDA_AMP_CAPS, > + .val = QEMU_HDA_AMP_IN_CAPS, > },{ > .id = AC_PAR_AMP_OUT_CAP, > .val = QEMU_HDA_AMP_NONE, I am afraid guest OS won't like it the card description changes after a migration. This will likely need a new VMState field, for the steps, or the amp caps. > @@ -453,4 +459,5 @@ static const desc_codec glue(micro_, PARAM) = { > #undef QEMU_HDA_ID_OUTPUT > #undef QEMU_HDA_ID_DUPLEX > #undef QEMU_HDA_ID_MICRO > -#undef QEMU_HDA_AMP_CAPS > +#undef QEMU_HDA_AMP_IN_CAPS > +#undef QEMU_HDA_AMP_OUT_CAPS > diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c > index 3c03ff5..64b4d98 100644 > --- a/hw/audio/hda-codec.c > +++ b/hw/audio/hda-codec.c > @@ -116,7 +116,17 @@ static void hda_codec_parse_fmt(uint32_t format, struct > audsettings *as) > #define QEMU_HDA_PCM_FORMATS (AC_SUPPCM_BITS_16 | \ > 0x1fc /* 16 -> 96 kHz */) > #define QEMU_HDA_AMP_NONE (0) > -#define QEMU_HDA_AMP_STEPS 0x4a > +/* Amplifier properties */ > +/* Step size: 0: 0.25dB 1: 0.5dB 2: 0.75dB 3: 1dB */ > +/* > + Until we have a way to propagate dB values cleanly to the host > + e.g. through spice, it is better to use a smaller dynamic range > + for input. Here we use 0x40 * 0.5 dB = 32dB. > +*/ > +#define QEMU_HDA_AMP_OUT_STEPS 0x4a > +#define QEMU_HDA_AMP_OUT_STEP_SIZE 3 > +#define QEMU_HDA_AMP_IN_STEPS 0x40 > +#define QEMU_HDA_AMP_IN_STEP_SIZE 1 > > #define PARAM mixemu > #define HDA_MIXER > @@ -258,15 +268,16 @@ static void hda_audio_set_amp(HDAAudioStream *st) > left = st->mute_left ? 0 : st->gain_left; > right = st->mute_right ? 0 : st->gain_right; > > - left = left * 255 / QEMU_HDA_AMP_STEPS; > - right = right * 255 / QEMU_HDA_AMP_STEPS; > - > if (!st->state->mixer) { > return; > } > if (st->output) { > + left = left * 255 / QEMU_HDA_AMP_OUT_STEPS; > + right = right * 255 / QEMU_HDA_AMP_OUT_STEPS; > AUD_set_volume_out(st->voice.out, muted, left, right); > } else { > + left = left * 255 / QEMU_HDA_AMP_IN_STEPS; > + right = right * 255 / QEMU_HDA_AMP_IN_STEPS; > AUD_set_volume_in(st->voice.in, muted, left, right); > } > } > @@ -502,8 +513,8 @@ static int hda_audio_init(HDACodecDevice *hda, const > struct desc_codec *desc) > st->node = node; > if (type == AC_WID_AUD_OUT) { > /* unmute output by default */ > - st->gain_left = QEMU_HDA_AMP_STEPS; > - st->gain_right = QEMU_HDA_AMP_STEPS; > + st->gain_left = QEMU_HDA_AMP_OUT_STEPS; > + st->gain_right = QEMU_HDA_AMP_OUT_STEPS; > st->bpos = sizeof(st->buf); > st->output = true; > } else { > -- > 2.1.0 > >
Hi, > > The "right" solution for this problem would be to implement > > proper dB scaling in QEMU and the audio backends (such as spice). Can we try this please? > > While this clean solution is not available, I suggest to decrease > > the dynamic range for the the emulated Amps in the QEMU hda codec. > > Experiments showed that with 32dB dynamic range with a 0.5 dB step, > > it was possible to set gain values as low as 5 (-29.5dB / 8%). > > Actual HW seems to use similar ranges for input amplifiers. > I am afraid guest OS won't like it the card description changes after a migration. > This will likely need a new VMState field, for the steps, or the amp caps. Correct. This is a guest-visible change, so we have to make this runtime-switchable for compatibility with older qemu versions. This quickly becomes a bit messy, so I'd prefer to avoid this. cheers, Gerd
diff --git a/hw/audio/hda-codec-common.h b/hw/audio/hda-codec-common.h index b4fdb51..d59781b 100644 --- a/hw/audio/hda-codec-common.h +++ b/hw/audio/hda-codec-common.h @@ -28,16 +28,22 @@ #define QEMU_HDA_ID_OUTPUT ((QEMU_HDA_ID_VENDOR << 16) | 0x12) #define QEMU_HDA_ID_DUPLEX ((QEMU_HDA_ID_VENDOR << 16) | 0x22) #define QEMU_HDA_ID_MICRO ((QEMU_HDA_ID_VENDOR << 16) | 0x32) -#define QEMU_HDA_AMP_CAPS \ +#define QEMU_HDA_AMP_OUT_CAPS \ (AC_AMPCAP_MUTE | \ - (QEMU_HDA_AMP_STEPS << AC_AMPCAP_OFFSET_SHIFT) | \ - (QEMU_HDA_AMP_STEPS << AC_AMPCAP_NUM_STEPS_SHIFT) | \ - (3 << AC_AMPCAP_STEP_SIZE_SHIFT)) + (QEMU_HDA_AMP_OUT_STEPS << AC_AMPCAP_OFFSET_SHIFT) | \ + (QEMU_HDA_AMP_OUT_STEPS << AC_AMPCAP_NUM_STEPS_SHIFT) | \ + (QEMU_HDA_AMP_OUT_STEP_SIZE << AC_AMPCAP_STEP_SIZE_SHIFT)) +#define QEMU_HDA_AMP_IN_CAPS \ + (AC_AMPCAP_MUTE | \ + (QEMU_HDA_AMP_IN_STEPS << AC_AMPCAP_OFFSET_SHIFT) | \ + (QEMU_HDA_AMP_IN_STEPS << AC_AMPCAP_NUM_STEPS_SHIFT) | \ + (QEMU_HDA_AMP_IN_STEP_SIZE << AC_AMPCAP_STEP_SIZE_SHIFT)) #else #define QEMU_HDA_ID_OUTPUT ((QEMU_HDA_ID_VENDOR << 16) | 0x11) #define QEMU_HDA_ID_DUPLEX ((QEMU_HDA_ID_VENDOR << 16) | 0x21) #define QEMU_HDA_ID_MICRO ((QEMU_HDA_ID_VENDOR << 16) | 0x31) -#define QEMU_HDA_AMP_CAPS QEMU_HDA_AMP_NONE +#define QEMU_HDA_AMP_OUT_CAPS QEMU_HDA_AMP_NONE +#define QEMU_HDA_AMP_IN_CAPS QEMU_HDA_AMP_NONE #endif @@ -61,7 +67,7 @@ static const desc_param glue(common_params_audio_dac_, PARAM)[] = { .val = QEMU_HDA_AMP_NONE, },{ .id = AC_PAR_AMP_OUT_CAP, - .val = QEMU_HDA_AMP_CAPS, + .val = QEMU_HDA_AMP_OUT_CAPS, }, }; @@ -86,7 +92,7 @@ static const desc_param glue(common_params_audio_adc_, PARAM)[] = { .val = AC_SUPFMT_PCM, },{ .id = AC_PAR_AMP_IN_CAP, - .val = QEMU_HDA_AMP_CAPS, + .val = QEMU_HDA_AMP_IN_CAPS, },{ .id = AC_PAR_AMP_OUT_CAP, .val = QEMU_HDA_AMP_NONE, @@ -453,4 +459,5 @@ static const desc_codec glue(micro_, PARAM) = { #undef QEMU_HDA_ID_OUTPUT #undef QEMU_HDA_ID_DUPLEX #undef QEMU_HDA_ID_MICRO -#undef QEMU_HDA_AMP_CAPS +#undef QEMU_HDA_AMP_IN_CAPS +#undef QEMU_HDA_AMP_OUT_CAPS diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c index 3c03ff5..64b4d98 100644 --- a/hw/audio/hda-codec.c +++ b/hw/audio/hda-codec.c @@ -116,7 +116,17 @@ static void hda_codec_parse_fmt(uint32_t format, struct audsettings *as) #define QEMU_HDA_PCM_FORMATS (AC_SUPPCM_BITS_16 | \ 0x1fc /* 16 -> 96 kHz */) #define QEMU_HDA_AMP_NONE (0) -#define QEMU_HDA_AMP_STEPS 0x4a +/* Amplifier properties */ +/* Step size: 0: 0.25dB 1: 0.5dB 2: 0.75dB 3: 1dB */ +/* + Until we have a way to propagate dB values cleanly to the host + e.g. through spice, it is better to use a smaller dynamic range + for input. Here we use 0x40 * 0.5 dB = 32dB. +*/ +#define QEMU_HDA_AMP_OUT_STEPS 0x4a +#define QEMU_HDA_AMP_OUT_STEP_SIZE 3 +#define QEMU_HDA_AMP_IN_STEPS 0x40 +#define QEMU_HDA_AMP_IN_STEP_SIZE 1 #define PARAM mixemu #define HDA_MIXER @@ -258,15 +268,16 @@ static void hda_audio_set_amp(HDAAudioStream *st) left = st->mute_left ? 0 : st->gain_left; right = st->mute_right ? 0 : st->gain_right; - left = left * 255 / QEMU_HDA_AMP_STEPS; - right = right * 255 / QEMU_HDA_AMP_STEPS; - if (!st->state->mixer) { return; } if (st->output) { + left = left * 255 / QEMU_HDA_AMP_OUT_STEPS; + right = right * 255 / QEMU_HDA_AMP_OUT_STEPS; AUD_set_volume_out(st->voice.out, muted, left, right); } else { + left = left * 255 / QEMU_HDA_AMP_IN_STEPS; + right = right * 255 / QEMU_HDA_AMP_IN_STEPS; AUD_set_volume_in(st->voice.in, muted, left, right); } } @@ -502,8 +513,8 @@ static int hda_audio_init(HDACodecDevice *hda, const struct desc_codec *desc) st->node = node; if (type == AC_WID_AUD_OUT) { /* unmute output by default */ - st->gain_left = QEMU_HDA_AMP_STEPS; - st->gain_right = QEMU_HDA_AMP_STEPS; + st->gain_left = QEMU_HDA_AMP_OUT_STEPS; + st->gain_right = QEMU_HDA_AMP_OUT_STEPS; st->bpos = sizeof(st->buf); st->output = true; } else {
This patch addresses the following problem: Moving the input volume slider 0% to 1% in a Win7 guest causes the the "gain" values in the emulated HDA codec HW to jump from 0 to 40. This makes it impossible to control sound volume in the low-gain range using the windows guest. This is related to how the Windows HDA codec driver and the QEMU emulated HW interact. It seems that Windows uses a ~30dB overall scale for the min-max range of the input volume slider. The "right" solution for this problem would be to implement proper dB scaling in QEMU and the audio backends (such as spice). While this clean solution is not available, I suggest to decrease the dynamic range for the the emulated Amps in the QEMU hda codec. Experiments showed that with 32dB dynamic range with a 0.5 dB step, it was possible to set gain values as low as 5 (-29.5dB / 8%). Actual HW seems to use similar ranges for input amplifiers. Signed-off-by: Martin Wilck <martin.wilck@ts.fujitsu.com> --- hw/audio/hda-codec-common.h | 23 +++++++++++++++-------- hw/audio/hda-codec.c | 23 +++++++++++++++++------ 2 files changed, 32 insertions(+), 14 deletions(-)