From 36fcfeb21113ff9b25796458c372a03a80638cd0 Mon Sep 17 00:00:00 2001 From: Hui Wang Date: Mon, 7 Jun 2021 16:58:36 +0800 Subject: [PATCH] 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 Part-of: --- src/modules/alsa/alsa-sink.c | 10 +++++++++- src/modules/alsa/alsa-source.c | 10 +++++++++- src/pulsecore/sink.c | 4 ++++ src/pulsecore/sink.h | 1 + src/pulsecore/source.c | 4 ++++ src/pulsecore/source.h | 1 + 6 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c index 70b9230..76a710e 100644 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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; -- 2.7.4