Re: [Qemu-devel] [PATCH 06/11] hw/ac97: new support for volume control

Re: [Qemu-devel] [PATCH 06/11] hw/ac97: new support for volume control

[Wayne Gao]

Hi Marc-André,


>>>
        ---
         hw/ac97.c |   79
        +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
         1 files changed, 79 insertions(+), 0 deletions(-)
        
        diff --git a/hw/ac97.c b/hw/ac97.c
        index ba94835..4a7c4ed 100644
        --- a/hw/ac97.c
        +++ b/hw/ac97.c
        @@ -431,6 +431,63 @@ static void reset_voices (AC97LinkState *s,
        uint8_t 
        ;
        
        ...
        
        -- 
        1.7.6.2
>>>

According to my test, my guest OS also can support for volume control
even I don't apply the patch to my code. I run my Ubuntu guest OS via
the following command:

qemu-system-x86_64 -M pc --enable-kvm -m 512 -smp 1 -name Ubuntu
-localtime -boot c -drive file=ubuntu.img,if=virtio,index=0  -vga cirrus
-chardev stdio,id=mon0 -mon chardev=mon0,mode=readline -chardev
socket,id=mon1,host=localhost,port=5554,server,nowait,telnet -mon
chardev=mon1,mode=control -soundhw ac97


By the way, my host OS is also Ubuntu 11.04 and I haven't apply your
patch [PATCH 04/11] and [PATCH 05/11] for hw/ac97.c to my code. Are
there any problems for my test?


Best Regards
Wayne Gao

 

[Qemu-devel] [PATCH 00/11] apply volume on client side

[Marc-André Lureau]

Hi,

This patch series was sent back in September, and reviewed by 
Gerd Hoffmann, who pointed out a few nitpicks in the build
warnings.

cheers

Marc-André Lureau (11):
  audio: add VOICE_VOLUME ctl
  audio: don't apply volume effect if backend has VOICE_VOLUME_CAP
  audio: use a nominal volume of 1^32-1
  hw/ac97: remove USE_MIXER code
  hw/ac97: the volume mask was not always 0x1f
  hw/ac97: new support for volume control
  audio/spice: add support for volume control
  Do not use pa_simple PulseAudio API
  configure: pa_simple is not needed anymore
  Allow controlling volume with PulseAudio backend
  Make mixemu mandatory

 audio/audio.c          |   25 +++-
 audio/audio_int.h      |    6 +
 audio/audio_template.h |    2 +
 audio/mixeng.c         |    6 -
 audio/paaudio.c        |  452 +++++++++++++++++++++++++++++++++++++++++++-----
 audio/spiceaudio.c     |   41 +++++
 configure              |   14 +--
 hw/ac97.c              |  141 ++++++----------
 hw/hda-audio.c         |    4 -
 9 files changed, 531 insertions(+), 160 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 00/11] RFC: apply volume on client stream

[Marc-André Lureau]

Hi,

The qemu volume control experience could be improved. Without mixemu,
the volume control has no effect. A volume applet will just feel
broken (except in some cases where software volume is applied on guest
side, with HDA/Windows7 for example). 

And a few other problems:

 - the guest should have a full-range hw volume control over actual
   audio device.
   See https://bugzilla.redhat.com/show_bug.cgi?id=482089

 - one volume should rule them all (no need to control guest,
   stream and client volumes separately)

 - the client volume should be set to match the guest on connection,
   ex: volume and mute should be restored

 - backend should hopefully be better at applying volume effect
   (PulseAudio rely on HW as much as possible, and then optimized
   routines)

  - ideally, we would sync both ways guest <-> client, that would
    probably require guest agent helper to be notified on update

The flat-volume logic of PulseAudio (and Windows) ensures a stream get
a full-range volume control. Thus, the "guest volume" can rely on its
"client stream volume" to have control over the full volume range.

I propose an audio backend claiming VOICE_VOLUME_CAP should handle
VOICE_VOLUME messages and apply the volume/mute received. In this
case mixeng_clear / mixeng_volume aren't applied.

I modified the PA backend as a proof of concept (I had to drop the
pa_simple API, as I can't see how to modify a stream volume
otherwise).

Spice and spice-gtk client have already been supporting volume control
for a while, but qemu part is the missing bit.

Marc-André Lureau (11):
  audio: add VOICE_VOLUME ctl
  audio: don't apply volume effect if backend has VOICE_VOLUME_CAP
  audio: use a nominal volume of 1^32-1
  hw/ac97: remove USE_MIXER code
  hw/ac97: the volume mask was not always 0x1f
  hw/ac97: new support for volume control
  audio/spice: add support for volume control
  RFC: use full PulseAudio API, largely adapted from pa_simple*
  RFC: configure: pa_simple is not needed anymore
  RFC: allow controlling volume with PulseAudio backend
  RFC: make mixemu mandatory

 audio/audio.c          |   25 +++-
 audio/audio_int.h      |    6 +
 audio/audio_template.h |    2 +
 audio/mixeng.c         |    6 -
 audio/paaudio.c        |  452 +++++++++++++++++++++++++++++++++++++++++++-----
 audio/spiceaudio.c     |   43 +++++
 configure              |   14 +--
 hw/ac97.c              |  141 ++++++----------
 hw/hda-audio.c         |    4 -
 9 files changed, 533 insertions(+), 160 deletions(-)

-- 
1.7.6.2

[Qemu-devel] [PATCH 06/11] hw/ac97: new support for volume control

[Marc-André Lureau]

---
 hw/ac97.c |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index ba94835..4a7c4ed 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -431,6 +431,63 @@ static void reset_voices (AC97LinkState *s, uint8_t active[LAST_INDEX])
     AUD_set_active_in (s->voice_mc, active[MC_INDEX]);
 }
 
+static void get_volume (uint16_t vol, uint16_t mask, int inverse,
+                        int *mute, uint8_t *lvol, uint8_t *rvol)
+{
+  *mute = (vol >> MUTE_SHIFT) & 1;
+  *rvol = (255 * (vol & mask)) / mask;
+  *lvol = (255 * ((vol >> 8) & mask)) / mask;
+  if (inverse) {
+    *rvol = 255 - *rvol;
+    *lvol = 255 - *lvol;
+  }
+}
+
+static void update_combined_volume_out (AC97LinkState *s)
+{
+    uint8_t lvol, rvol, plvol, prvol;
+    int mute, pmute;
+
+    get_volume (mixer_load (s, AC97_Master_Volume_Mute), 0x3f, 1,
+                &mute, &lvol, &rvol);
+    /* FIXME: should be 1f according to spec */
+    get_volume (mixer_load (s, AC97_PCM_Out_Volume_Mute), 0x3f, 1,
+                &pmute, &plvol, &prvol);
+
+    mute = mute | pmute;
+    lvol = (lvol * plvol) / 255;
+    rvol = (rvol * prvol) / 255;
+
+    AUD_set_volume_out (s->voice_po, mute, lvol, rvol);
+}
+
+static void update_volume_in(AC97LinkState *s)
+{
+    uint8_t lvol, rvol;
+    int mute;
+
+    get_volume (mixer_load (s, AC97_Record_Gain_Mute), 0x0f, 0,
+                &mute, &lvol, &rvol);
+
+    AUD_set_volume_in (s->voice_pi, mute, lvol, rvol);
+}
+
+static void set_volume (AC97LinkState *s, int index, uint32_t val)
+{
+    mixer_store (s, index, val);
+    if (index == AC97_Master_Volume_Mute || index == AC97_PCM_Out_Volume_Mute)
+      update_combined_volume_out (s);
+    else if (index == AC97_Record_Gain_Mute)
+      update_volume_in (s);
+}
+
+static void record_select (AC97LinkState *s, uint32_t val)
+{
+    uint8_t rs = val & REC_MASK;
+    uint8_t ls = (val >> 8) & REC_MASK;
+    mixer_store (s, AC97_Record_Select, rs | (ls << 8));
+}
+
 static void mixer_reset (AC97LinkState *s)
 {
     uint8_t active[LAST_INDEX];
@@ -465,6 +522,11 @@ static void mixer_reset (AC97LinkState *s)
     mixer_store (s, AC97_PCM_LR_ADC_Rate         , 0xbb80);
     mixer_store (s, AC97_MIC_ADC_Rate            , 0xbb80);
 
+    record_select (s, 0);
+    set_volume (s, AC97_Master_Volume_Mute, 0x8000);
+    set_volume (s, AC97_PCM_Out_Volume_Mute, 0x8808);
+    set_volume (s, AC97_Line_In_Volume_Mute, 0x8808);
+
     reset_voices (s, active);
 }
 
@@ -523,6 +585,15 @@ static void nam_writew (void *opaque, uint32_t addr, uint32_t val)
         val |= mixer_load (s, index) & 0xf;
         mixer_store (s, index, val);
         break;
+    case AC97_PCM_Out_Volume_Mute:
+    case AC97_Master_Volume_Mute:
+    case AC97_Record_Gain_Mute:
+    case AC97_Line_In_Volume_Mute:
+        set_volume (s, index, val);
+        break;
+    case AC97_Record_Select:
+        record_select (s, val);
+        break;
     case AC97_Vendor_ID1:
     case AC97_Vendor_ID2:
         dolog ("Attempt to write vendor ID to %#x\n", val);
@@ -1075,6 +1146,14 @@ static int ac97_post_load (void *opaque, int version_id)
     uint8_t active[LAST_INDEX];
     AC97LinkState *s = opaque;
 
+    record_select (s, mixer_load (s, AC97_Record_Select));
+    set_volume (s, AC97_Master_Volume_Mute,
+                mixer_load (s, AC97_Master_Volume_Mute));
+    set_volume (s, AC97_PCM_Out_Volume_Mute,
+                mixer_load (s, AC97_PCM_Out_Volume_Mute));
+    set_volume (s, AC97_Line_In_Volume_Mute,
+                mixer_load (s, AC97_Line_In_Volume_Mute));
+
     active[PI_INDEX] = !!(s->bm_regs[PI_INDEX].cr & CR_RPBM);
     active[PO_INDEX] = !!(s->bm_regs[PO_INDEX].cr & CR_RPBM);
     active[MC_INDEX] = !!(s->bm_regs[MC_INDEX].cr & CR_RPBM);
-- 
1.7.6.2

[Qemu-devel] [PATCH 01/11] audio: add VOICE_VOLUME ctl

[Marc-André Lureau]

---
 audio/audio.c     |   12 ++++++++++++
 audio/audio_int.h |    1 +
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 5649075..c759c1f 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2050,17 +2050,29 @@ void AUD_del_capture (CaptureVoiceOut *cap, void *cb_opaque)
 void AUD_set_volume_out (SWVoiceOut *sw, int mute, uint8_t lvol, uint8_t rvol)
 {
     if (sw) {
+        HWVoiceOut *hw = sw->hw;
+
         sw->vol.mute = mute;
         sw->vol.l = nominal_volume.l * lvol / 255;
         sw->vol.r = nominal_volume.r * rvol / 255;
+
+        if (hw->pcm_ops->ctl_out) {
+            hw->pcm_ops->ctl_out (hw, VOICE_VOLUME, sw);
+        }
     }
 }
 
 void AUD_set_volume_in (SWVoiceIn *sw, int mute, uint8_t lvol, uint8_t rvol)
 {
     if (sw) {
+        HWVoiceIn *hw = sw->hw;
+
         sw->vol.mute = mute;
         sw->vol.l = nominal_volume.l * lvol / 255;
         sw->vol.r = nominal_volume.r * rvol / 255;
+
+        if (hw->pcm_ops->ctl_in) {
+            hw->pcm_ops->ctl_in (hw, VOICE_VOLUME, sw);
+        }
     }
 }
diff --git a/audio/audio_int.h b/audio/audio_int.h
index 2003f8b..117f95e 100644
--- a/audio/audio_int.h
+++ b/audio/audio_int.h
@@ -231,6 +231,7 @@ void audio_run (const char *msg);
 
 #define VOICE_ENABLE 1
 #define VOICE_DISABLE 2
+#define VOICE_VOLUME 3
 
 static inline int audio_ring_dist (int dst, int src, int len)
 {
-- 
1.7.6.2

[Qemu-devel] [PATCH 06/11] hw/ac97: new support for volume control

[Marc-André Lureau]

---
 hw/ac97.c |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index ba94835..4a7c4ed 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -431,6 +431,63 @@ static void reset_voices (AC97LinkState *s, uint8_t active[LAST_INDEX])
     AUD_set_active_in (s->voice_mc, active[MC_INDEX]);
 }
 
+static void get_volume (uint16_t vol, uint16_t mask, int inverse,
+                        int *mute, uint8_t *lvol, uint8_t *rvol)
+{
+  *mute = (vol >> MUTE_SHIFT) & 1;
+  *rvol = (255 * (vol & mask)) / mask;
+  *lvol = (255 * ((vol >> 8) & mask)) / mask;
+  if (inverse) {
+    *rvol = 255 - *rvol;
+    *lvol = 255 - *lvol;
+  }
+}
+
+static void update_combined_volume_out (AC97LinkState *s)
+{
+    uint8_t lvol, rvol, plvol, prvol;
+    int mute, pmute;
+
+    get_volume (mixer_load (s, AC97_Master_Volume_Mute), 0x3f, 1,
+                &mute, &lvol, &rvol);
+    /* FIXME: should be 1f according to spec */
+    get_volume (mixer_load (s, AC97_PCM_Out_Volume_Mute), 0x3f, 1,
+                &pmute, &plvol, &prvol);
+
+    mute = mute | pmute;
+    lvol = (lvol * plvol) / 255;
+    rvol = (rvol * prvol) / 255;
+
+    AUD_set_volume_out (s->voice_po, mute, lvol, rvol);
+}
+
+static void update_volume_in(AC97LinkState *s)
+{
+    uint8_t lvol, rvol;
+    int mute;
+
+    get_volume (mixer_load (s, AC97_Record_Gain_Mute), 0x0f, 0,
+                &mute, &lvol, &rvol);
+
+    AUD_set_volume_in (s->voice_pi, mute, lvol, rvol);
+}
+
+static void set_volume (AC97LinkState *s, int index, uint32_t val)
+{
+    mixer_store (s, index, val);
+    if (index == AC97_Master_Volume_Mute || index == AC97_PCM_Out_Volume_Mute)
+      update_combined_volume_out (s);
+    else if (index == AC97_Record_Gain_Mute)
+      update_volume_in (s);
+}
+
+static void record_select (AC97LinkState *s, uint32_t val)
+{
+    uint8_t rs = val & REC_MASK;
+    uint8_t ls = (val >> 8) & REC_MASK;
+    mixer_store (s, AC97_Record_Select, rs | (ls << 8));
+}
+
 static void mixer_reset (AC97LinkState *s)
 {
     uint8_t active[LAST_INDEX];
@@ -465,6 +522,11 @@ static void mixer_reset (AC97LinkState *s)
     mixer_store (s, AC97_PCM_LR_ADC_Rate         , 0xbb80);
     mixer_store (s, AC97_MIC_ADC_Rate            , 0xbb80);
 
+    record_select (s, 0);
+    set_volume (s, AC97_Master_Volume_Mute, 0x8000);
+    set_volume (s, AC97_PCM_Out_Volume_Mute, 0x8808);
+    set_volume (s, AC97_Line_In_Volume_Mute, 0x8808);
+
     reset_voices (s, active);
 }
 
@@ -523,6 +585,15 @@ static void nam_writew (void *opaque, uint32_t addr, uint32_t val)
         val |= mixer_load (s, index) & 0xf;
         mixer_store (s, index, val);
         break;
+    case AC97_PCM_Out_Volume_Mute:
+    case AC97_Master_Volume_Mute:
+    case AC97_Record_Gain_Mute:
+    case AC97_Line_In_Volume_Mute:
+        set_volume (s, index, val);
+        break;
+    case AC97_Record_Select:
+        record_select (s, val);
+        break;
     case AC97_Vendor_ID1:
     case AC97_Vendor_ID2:
         dolog ("Attempt to write vendor ID to %#x\n", val);
@@ -1075,6 +1146,14 @@ static int ac97_post_load (void *opaque, int version_id)
     uint8_t active[LAST_INDEX];
     AC97LinkState *s = opaque;
 
+    record_select (s, mixer_load (s, AC97_Record_Select));
+    set_volume (s, AC97_Master_Volume_Mute,
+                mixer_load (s, AC97_Master_Volume_Mute));
+    set_volume (s, AC97_PCM_Out_Volume_Mute,
+                mixer_load (s, AC97_PCM_Out_Volume_Mute));
+    set_volume (s, AC97_Line_In_Volume_Mute,
+                mixer_load (s, AC97_Line_In_Volume_Mute));
+
     active[PI_INDEX] = !!(s->bm_regs[PI_INDEX].cr & CR_RPBM);
     active[PO_INDEX] = !!(s->bm_regs[PO_INDEX].cr & CR_RPBM);
     active[MC_INDEX] = !!(s->bm_regs[MC_INDEX].cr & CR_RPBM);
-- 
1.7.6.2

[Qemu-devel] [PATCH 06/11] hw/ac97: new support for volume control

[Marc-André Lureau]

---
 hw/ac97.c |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index f7866ed..227233c 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -436,6 +436,63 @@ static void reset_voices (AC97LinkState *s, uint8_t active[LAST_INDEX])
     AUD_set_active_in (s->voice_mc, active[MC_INDEX]);
 }
 
+static void get_volume (uint16_t vol, uint16_t mask, int inverse,
+                        int *mute, uint8_t *lvol, uint8_t *rvol)
+{
+  *mute = (vol >> MUTE_SHIFT) & 1;
+  *rvol = (255 * (vol & mask)) / mask;
+  *lvol = (255 * ((vol >> 8) & mask)) / mask;
+  if (inverse) {
+    *rvol = 255 - *rvol;
+    *lvol = 255 - *lvol;
+  }
+}
+
+static void update_combined_volume_out (AC97LinkState *s)
+{
+    uint8_t lvol, rvol, plvol, prvol;
+    int mute, pmute;
+
+    get_volume (mixer_load (s, AC97_Master_Volume_Mute), 0x3f, 1,
+                &mute, &lvol, &rvol);
+    /* FIXME: should be 1f according to spec */
+    get_volume (mixer_load (s, AC97_PCM_Out_Volume_Mute), 0x3f, 1,
+                &pmute, &plvol, &prvol);
+
+    mute = mute | pmute;
+    lvol = (lvol * plvol) / 255;
+    rvol = (rvol * prvol) / 255;
+
+    AUD_set_volume_out (s->voice_po, mute, lvol, rvol);
+}
+
+static void update_volume_in(AC97LinkState *s)
+{
+    uint8_t lvol, rvol;
+    int mute;
+
+    get_volume (mixer_load (s, AC97_Record_Gain_Mute), 0x0f, 0,
+                &mute, &lvol, &rvol);
+
+    AUD_set_volume_in (s->voice_pi, mute, lvol, rvol);
+}
+
+static void set_volume (AC97LinkState *s, int index, uint32_t val)
+{
+    mixer_store (s, index, val);
+    if (index == AC97_Master_Volume_Mute || index == AC97_PCM_Out_Volume_Mute)
+      update_combined_volume_out (s);
+    else if (index == AC97_Record_Gain_Mute)
+      update_volume_in (s);
+}
+
+static void record_select (AC97LinkState *s, uint32_t val)
+{
+    uint8_t rs = val & REC_MASK;
+    uint8_t ls = (val >> 8) & REC_MASK;
+    mixer_store (s, AC97_Record_Select, rs | (ls << 8));
+}
+
 static void mixer_reset (AC97LinkState *s)
 {
     uint8_t active[LAST_INDEX];
@@ -470,6 +527,11 @@ static void mixer_reset (AC97LinkState *s)
     mixer_store (s, AC97_PCM_LR_ADC_Rate         , 0xbb80);
     mixer_store (s, AC97_MIC_ADC_Rate            , 0xbb80);
 
+    record_select (s, 0);
+    set_volume (s, AC97_Master_Volume_Mute, 0x8000);
+    set_volume (s, AC97_PCM_Out_Volume_Mute, 0x8808);
+    set_volume (s, AC97_Line_In_Volume_Mute, 0x8808);
+
     reset_voices (s, active);
 }
 
@@ -528,6 +590,15 @@ static void nam_writew (void *opaque, uint32_t addr, uint32_t val)
         val |= mixer_load (s, index) & 0xf;
         mixer_store (s, index, val);
         break;
+    case AC97_PCM_Out_Volume_Mute:
+    case AC97_Master_Volume_Mute:
+    case AC97_Record_Gain_Mute:
+    case AC97_Line_In_Volume_Mute:
+        set_volume (s, index, val);
+        break;
+    case AC97_Record_Select:
+        record_select (s, val);
+        break;
     case AC97_Vendor_ID1:
     case AC97_Vendor_ID2:
         dolog ("Attempt to write vendor ID to %#x\n", val);
@@ -1080,6 +1151,14 @@ static int ac97_post_load (void *opaque, int version_id)
     uint8_t active[LAST_INDEX];
     AC97LinkState *s = opaque;
 
+    record_select (s, mixer_load (s, AC97_Record_Select));
+    set_volume (s, AC97_Master_Volume_Mute,
+                mixer_load (s, AC97_Master_Volume_Mute));
+    set_volume (s, AC97_PCM_Out_Volume_Mute,
+                mixer_load (s, AC97_PCM_Out_Volume_Mute));
+    set_volume (s, AC97_Line_In_Volume_Mute,
+                mixer_load (s, AC97_Line_In_Volume_Mute));
+
     active[PI_INDEX] = !!(s->bm_regs[PI_INDEX].cr & CR_RPBM);
     active[PO_INDEX] = !!(s->bm_regs[PO_INDEX].cr & CR_RPBM);
     active[MC_INDEX] = !!(s->bm_regs[MC_INDEX].cr & CR_RPBM);
-- 
1.7.7.6