alsa-sink/source: set volume to hw immediately if ucm_port changing

Recently we found an issue of output volume on speaker and headphone,
they should have their own volume but in practice they share one
output volume.

This issue happens on the laptops which use the ucm2 sof-hda-dsp,
originally the speaker has output volume A while the headphone has the
output volume B, suppose the speaker is the active port at the moment
and the output volume is A, users plug a headphone to the jack and the
headphone becomes the active port, in this process, ucm_set_port()
calls _disdev/_enadev which triggers the io_mixer_callback(), in the
meanwhile, the module_device_restore will restore the headphone's
volume to B, it will call set_volume_cb() to set the volume to B, but
this value is not written to hw immediately, during the time of
waiting for the B to be written to the hw, the io_mixer_callback()
calls get_volume_cb(), it reads hw volume and gets the volume A, then
it overrides the output volume to A, this results in the headphone
gets the volume A instead of B.

If a machine doesn't use the ucm, this issue will not happen since the
set_port_cb() will not trigger the io_mixer_callback(). If the ports
don't belong to the same sink/source, this issue also doesn't happen.

BugLink: http://bugs.launchpad.net/bugs/1930188
Signed-off-by: Hui Wang <hui.wang@canonical.com>
Part-of: <https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/577>

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index 70b9230..76a710e 100644 (file)
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -1494,6 +1494,7 @@ static void sink_set_volume_cb(pa_sink *s) {
     pa_cvolume r;
     char volume_buf[PA_CVOLUME_SNPRINT_VERBOSE_MAX];
     bool deferred_volume = !!(s->flags & PA_SINK_DEFERRED_VOLUME);
+    bool write_to_hw = !deferred_volume;
 
     pa_assert(u);
     pa_assert(u->mixer_path);
@@ -1502,7 +1503,14 @@ static void sink_set_volume_cb(pa_sink *s) {
     /* Shift up by the base volume */
     pa_sw_cvolume_divide_scalar(&r, &s->real_volume, s->base_volume);
 
-    if (pa_alsa_path_set_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &r, deferred_volume, !deferred_volume) < 0)
+    /* If the set_volume() is called because of ucm active_port changing, the
+     * volume should be written to hw immediately, otherwise this volume will be
+     * overridden by calling get_volume_cb() which is called by
+     * _disdev/_enadev() -> io_mixer_callback() */
+    if (u->ucm_context && s->port_changing)
+       write_to_hw = true;
+
+    if (pa_alsa_path_set_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &r, deferred_volume, write_to_hw) < 0)
         return;
 
     /* Shift down by the base volume, so that 0dB becomes maximum volume */

diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
index 083f928..59cca12 100644 (file)
--- a/src/modules/alsa/alsa-source.c
+++ b/src/modules/alsa/alsa-source.c
@@ -1365,6 +1365,7 @@ static void source_set_volume_cb(pa_source *s) {
     pa_cvolume r;
     char volume_buf[PA_CVOLUME_SNPRINT_VERBOSE_MAX];
     bool deferred_volume = !!(s->flags & PA_SOURCE_DEFERRED_VOLUME);
+    bool write_to_hw = !deferred_volume;
 
     pa_assert(u);
     pa_assert(u->mixer_path);
@@ -1373,7 +1374,14 @@ static void source_set_volume_cb(pa_source *s) {
     /* Shift up by the base volume */
     pa_sw_cvolume_divide_scalar(&r, &s->real_volume, s->base_volume);
 
-    if (pa_alsa_path_set_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &r, deferred_volume, !deferred_volume) < 0)
+    /* If the set_volume() is called because of ucm active_port changing, the
+     * volume should be written to hw immediately, otherwise this volume will be
+     * overridden by calling get_volume_cb() which is called by
+     * _disdev/_enadev() -> io_mixer_callback() */
+    if (u->ucm_context && s->port_changing)
+       write_to_hw = true;
+
+    if (pa_alsa_path_set_volume(u->mixer_path, u->mixer_handle, &s->channel_map, &r, deferred_volume, write_to_hw) < 0)
         return;
 
     /* Shift down by the base volume, so that 0dB becomes maximum volume */

diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index 90dc250..905e1db 100644 (file)
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -3431,6 +3431,8 @@ int pa_sink_set_port(pa_sink *s, const char *name, bool save) {
         return 0;
     }
 
+    s->port_changing = true;
+
     if (s->set_port(s, port) < 0)
         return -PA_ERR_NOENTITY;
 
@@ -3448,6 +3450,8 @@ int pa_sink_set_port(pa_sink *s, const char *name, bool save) {
 
     pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_PORT_CHANGED], s);
 
+    s->port_changing = false;
+
     return 0;
 }
 

diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index c3f5fbc..87bfddd 100644 (file)
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -105,6 +105,7 @@ struct pa_sink {
     bool save_port:1;
     bool save_volume:1;
     bool save_muted:1;
+    bool port_changing:1;
 
     /* Saved volume state while we're in passthrough mode */
     pa_cvolume saved_volume;

diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index efc3640..99d8dde 100644 (file)
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -2696,6 +2696,8 @@ int pa_source_set_port(pa_source *s, const char *name, bool save) {
         return 0;
     }
 
+    s->port_changing = true;
+
     if (s->set_port(s, port) < 0)
         return -PA_ERR_NOENTITY;
 
@@ -2713,6 +2715,8 @@ int pa_source_set_port(pa_source *s, const char *name, bool save) {
 
     pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SOURCE_PORT_CHANGED], s);
 
+    s->port_changing = false;
+
     return 0;
 }
 

diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
index aa45e6d..aa71ee8 100644 (file)
--- a/src/pulsecore/source.h
+++ b/src/pulsecore/source.h
@@ -106,6 +106,7 @@ struct pa_source {
     bool save_port:1;
     bool save_volume:1;
     bool save_muted:1;
+    bool port_changing:1;
 
     /* Saved volume state while we're in passthrough mode */
     pa_cvolume saved_volume;