[pulseaudio-discuss,3/3] alsa-ucm: Support Playback/CaptureVolume

Submitted by Arun Raghavan on May 3, 2016, 12:52 p.m.

Details

Message ID 1462279930-15468-4-git-send-email-arun@accosted.net
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Arun Raghavan May 3, 2016, 12:52 p.m.
From: Arun Raghavan <git@arunraghavan.net>

This allows us to support the PlaybackVolume and CaptureVolume commands
in UCM, specifying a mixer control to use for hardware volume control.

This only works with ports corresponding to single devices at the
moment, and doesn't support stacking controls for combination ports.

On the UCM side, this also requires that when disabling the device for
the port, the volume should be reset to some default.

When enabling/disabling combination devices, things are a bit iffy since
we have no way to reset the volume before switching to a combination
device. It would be nice to have a combination-transition-sequence
command in UCM to handle this and other similar cases.

PlaybackSwitch and CaptureSwitch are yet to be implemented.
---
 src/modules/alsa/alsa-sink.c        |  71 ++++++++++++----
 src/modules/alsa/alsa-source.c      |  69 ++++++++++++----
 src/modules/alsa/alsa-ucm.c         | 158 +++++++++++++++++++++++++++++-------
 src/modules/alsa/alsa-ucm.h         |  26 +++++-
 src/modules/alsa/module-alsa-card.c |   4 +-
 5 files changed, 262 insertions(+), 66 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index 2fdebe0..9ee6a29 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -1419,7 +1419,7 @@  static void sink_set_mute_cb(pa_sink *s) {
 static void mixer_volume_init(struct userdata *u) {
     pa_assert(u);
 
-    if (!u->mixer_path->has_volume) {
+    if (!u->mixer_path || !u->mixer_path->has_volume) {
         pa_sink_set_write_volume_callback(u->sink, NULL);
         pa_sink_set_get_volume_callback(u->sink, NULL);
         pa_sink_set_set_volume_callback(u->sink, NULL);
@@ -1454,7 +1454,7 @@  static void mixer_volume_init(struct userdata *u) {
         pa_log_info("Using hardware volume control. Hardware dB scale %s.", u->mixer_path->has_dB ? "supported" : "not supported");
     }
 
-    if (!u->mixer_path->has_mute) {
+    if (!u->mixer_path || !u->mixer_path->has_mute) {
         pa_sink_set_get_mute_callback(u->sink, NULL);
         pa_sink_set_set_mute_callback(u->sink, NULL);
         pa_log_info("Driver does not support hardware mute control, falling back to software mute control.");
@@ -1467,10 +1467,31 @@  static void mixer_volume_init(struct userdata *u) {
 
 static int sink_set_port_ucm_cb(pa_sink *s, pa_device_port *p) {
     struct userdata *u = s->userdata;
+    pa_alsa_ucm_port_data *data;
+
+    data = PA_DEVICE_PORT_DATA(p);
 
     pa_assert(u);
     pa_assert(p);
     pa_assert(u->ucm_context);
+    pa_assert(u->mixer_handle);
+
+    u->mixer_path = data->path;
+    mixer_volume_init(u);
+
+    if (u->mixer_path) {
+        pa_alsa_path_select(u->mixer_path, NULL, u->mixer_handle, s->muted);
+
+        if (s->set_mute)
+            s->set_mute(s);
+        if (s->flags & PA_SINK_DEFERRED_VOLUME) {
+            if (s->write_volume)
+                s->write_volume(s);
+        } else {
+            if (s->set_volume)
+                s->set_volume(s);
+        }
+    }
 
     return pa_alsa_ucm_set_port(u->ucm_context, p, true);
 }
@@ -1894,6 +1915,11 @@  static void find_mixer(struct userdata *u, pa_alsa_mapping *mapping, const char
         return;
     }
 
+    if (u->ucm_context) {
+        /* We just want to open the device */
+        return;
+    }
+
     if (element) {
 
         if (!(u->mixer_path = pa_alsa_path_synthesize(element, PA_ALSA_DIRECTION_OUTPUT)))
@@ -1931,16 +1957,31 @@  static int setup_mixer(struct userdata *u, bool ignore_dB) {
         return 0;
 
     if (u->sink->active_port) {
-        pa_alsa_port_data *data;
+        if (!u->ucm_context) {
+            pa_alsa_port_data *data;
 
-        /* We have a list of supported paths, so let's activate the
-         * one that has been chosen as active */
+            /* We have a list of supported paths, so let's activate the
+             * one that has been chosen as active */
+
+            data = PA_DEVICE_PORT_DATA(u->sink->active_port);
+            u->mixer_path = data->path;
+
+            pa_alsa_path_select(data->path, data->setting, u->mixer_handle, u->sink->muted);
+        } else {
+            pa_alsa_ucm_port_data *data;
 
-        data = PA_DEVICE_PORT_DATA(u->sink->active_port);
-        u->mixer_path = data->path;
+            /* First activate the port on the UCM side */
+            if (pa_alsa_ucm_set_port(u->ucm_context, u->sink->active_port, true) < 0)
+                return -1;
 
-        pa_alsa_path_select(data->path, data->setting, u->mixer_handle, u->sink->muted);
+            data = PA_DEVICE_PORT_DATA(u->sink->active_port);
 
+            /* Now activate volume controls, if any */
+            if (data->path) {
+                u->mixer_path = data->path;
+                pa_alsa_path_select(u->mixer_path, NULL, u->mixer_handle, u->sink->muted);
+            }
+        }
     } else {
 
         if (!u->mixer_path && u->mixer_path_set)
@@ -1950,7 +1991,6 @@  static int setup_mixer(struct userdata *u, bool ignore_dB) {
             /* Hmm, we have only a single path, then let's activate it */
 
             pa_alsa_path_select(u->mixer_path, u->mixer_path->settings, u->mixer_handle, u->sink->muted);
-
         } else
             return 0;
     }
@@ -2244,8 +2284,7 @@  pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
     /* ALSA might tweak the sample spec, so recalculate the frame size */
     frame_size = pa_frame_size(&ss);
 
-    if (!u->ucm_context)
-        find_mixer(u, mapping, pa_modargs_get_value(ma, "control", NULL), ignore_dB);
+    find_mixer(u, mapping, pa_modargs_get_value(ma, "control", NULL), ignore_dB);
 
     pa_sink_new_data_init(&data);
     data.driver = driver;
@@ -2295,7 +2334,7 @@  pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
     }
 
     if (u->ucm_context)
-        pa_alsa_ucm_add_ports(&data.ports, data.proplist, u->ucm_context, true, card);
+        pa_alsa_ucm_add_ports(&data.ports, data.proplist, u->ucm_context, true, card, u->pcm_handle, ignore_dB);
     else if (u->mixer_path_set)
         pa_alsa_add_ports(&data, u->mixer_path_set, card);
 
@@ -2369,10 +2408,7 @@  pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
     if (update_sw_params(u) < 0)
         goto fail;
 
-    if (u->ucm_context) {
-        if (u->sink->active_port && pa_alsa_ucm_set_port(u->ucm_context, u->sink->active_port, true) < 0)
-            goto fail;
-    } else if (setup_mixer(u, ignore_dB) < 0)
+    if (setup_mixer(u, ignore_dB) < 0)
         goto fail;
 
     pa_alsa_dump(PA_LOG_DEBUG, u->pcm_handle);
@@ -2479,7 +2515,8 @@  static void userdata_free(struct userdata *u) {
     if (u->mixer_fdl)
         pa_alsa_fdlist_free(u->mixer_fdl);
 
-    if (u->mixer_path && !u->mixer_path_set)
+    /* Only free the mixer_path if the sink owns it */
+    if (u->mixer_path && !u->mixer_path_set && !u->ucm_context)
         pa_alsa_path_free(u->mixer_path);
 
     if (u->mixer_handle)
diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
index 4683dfe..6b95193 100644
--- a/src/modules/alsa/alsa-source.c
+++ b/src/modules/alsa/alsa-source.c
@@ -1301,7 +1301,7 @@  static void source_set_mute_cb(pa_source *s) {
 static void mixer_volume_init(struct userdata *u) {
     pa_assert(u);
 
-    if (!u->mixer_path->has_volume) {
+    if (!u->mixer_path || !u->mixer_path->has_volume) {
         pa_source_set_write_volume_callback(u->source, NULL);
         pa_source_set_get_volume_callback(u->source, NULL);
         pa_source_set_set_volume_callback(u->source, NULL);
@@ -1336,7 +1336,7 @@  static void mixer_volume_init(struct userdata *u) {
         pa_log_info("Using hardware volume control. Hardware dB scale %s.", u->mixer_path->has_dB ? "supported" : "not supported");
     }
 
-    if (!u->mixer_path->has_mute) {
+    if (!u->mixer_path || !u->mixer_path->has_mute) {
         pa_source_set_get_mute_callback(u->source, NULL);
         pa_source_set_set_mute_callback(u->source, NULL);
         pa_log_info("Driver does not support hardware mute control, falling back to software mute control.");
@@ -1349,10 +1349,31 @@  static void mixer_volume_init(struct userdata *u) {
 
 static int source_set_port_ucm_cb(pa_source *s, pa_device_port *p) {
     struct userdata *u = s->userdata;
+    pa_alsa_ucm_port_data *data;
+
+    data = PA_DEVICE_PORT_DATA(p);
 
     pa_assert(u);
     pa_assert(p);
     pa_assert(u->ucm_context);
+    pa_assert(u->mixer_handle);
+
+    u->mixer_path = data->path;
+    mixer_volume_init(u);
+
+    if (u->mixer_path) {
+        pa_alsa_path_select(u->mixer_path, NULL, u->mixer_handle, s->muted);
+
+        if (s->set_mute)
+            s->set_mute(s);
+        if (s->flags & PA_SINK_DEFERRED_VOLUME) {
+            if (s->write_volume)
+                s->write_volume(s);
+        } else {
+            if (s->set_volume)
+                s->set_volume(s);
+        }
+    }
 
     return pa_alsa_ucm_set_port(u->ucm_context, p, false);
 }
@@ -1610,6 +1631,11 @@  static void find_mixer(struct userdata *u, pa_alsa_mapping *mapping, const char
         return;
     }
 
+    if (u->ucm_context) {
+        /* We just want to open the device */
+        return;
+    }
+
     if (element) {
 
         if (!(u->mixer_path = pa_alsa_path_synthesize(element, PA_ALSA_DIRECTION_INPUT)))
@@ -1647,16 +1673,31 @@  static int setup_mixer(struct userdata *u, bool ignore_dB) {
         return 0;
 
     if (u->source->active_port) {
-        pa_alsa_port_data *data;
+        if (!u->ucm_context) {
+            pa_alsa_port_data *data;
 
-        /* We have a list of supported paths, so let's activate the
-         * one that has been chosen as active */
+            /* We have a list of supported paths, so let's activate the
+             * one that has been chosen as active */
 
-        data = PA_DEVICE_PORT_DATA(u->source->active_port);
-        u->mixer_path = data->path;
+            data = PA_DEVICE_PORT_DATA(u->source->active_port);
+            u->mixer_path = data->path;
 
-        pa_alsa_path_select(data->path, data->setting, u->mixer_handle, u->source->muted);
+            pa_alsa_path_select(data->path, data->setting, u->mixer_handle, u->source->muted);
+        } else {
+            pa_alsa_ucm_port_data *data;
+
+            /* First activate the port on the UCM side */
+            if (pa_alsa_ucm_set_port(u->ucm_context, u->source->active_port, false) < 0)
+                return -1;
 
+            data = PA_DEVICE_PORT_DATA(u->source->active_port);
+
+            /* Now activate volume controls, if any */
+            if (data->path) {
+                u->mixer_path = data->path;
+                pa_alsa_path_select(u->mixer_path, NULL, u->mixer_handle, u->source->muted);
+            }
+        }
     } else {
 
         if (!u->mixer_path && u->mixer_path_set)
@@ -1942,8 +1983,7 @@  pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
     /* ALSA might tweak the sample spec, so recalculate the frame size */
     frame_size = pa_frame_size(&ss);
 
-    if (!u->ucm_context)
-        find_mixer(u, mapping, pa_modargs_get_value(ma, "control", NULL), ignore_dB);
+    find_mixer(u, mapping, pa_modargs_get_value(ma, "control", NULL), ignore_dB);
 
     pa_source_new_data_init(&data);
     data.driver = driver;
@@ -1993,7 +2033,7 @@  pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
     }
 
     if (u->ucm_context)
-        pa_alsa_ucm_add_ports(&data.ports, data.proplist, u->ucm_context, false, card);
+        pa_alsa_ucm_add_ports(&data.ports, data.proplist, u->ucm_context, false, card, u->pcm_handle, ignore_dB);
     else if (u->mixer_path_set)
         pa_alsa_add_ports(&data, u->mixer_path_set, card);
 
@@ -2059,10 +2099,7 @@  pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
     if (update_sw_params(u) < 0)
         goto fail;
 
-    if (u->ucm_context) {
-        if (u->source->active_port && pa_alsa_ucm_set_port(u->ucm_context, u->source->active_port, false) < 0)
-            goto fail;
-    } else if (setup_mixer(u, ignore_dB) < 0)
+    if (setup_mixer(u, ignore_dB) < 0)
         goto fail;
 
     pa_alsa_dump(PA_LOG_DEBUG, u->pcm_handle);
@@ -2151,7 +2188,7 @@  static void userdata_free(struct userdata *u) {
     if (u->mixer_fdl)
         pa_alsa_fdlist_free(u->mixer_fdl);
 
-    if (u->mixer_path && !u->mixer_path_set)
+    if (u->mixer_path && !u->mixer_path_set && !u->ucm_context)
         pa_alsa_path_free(u->mixer_path);
 
     if (u->mixer_handle)
diff --git a/src/modules/alsa/alsa-ucm.c b/src/modules/alsa/alsa-ucm.c
index b42c040..85b0913 100644
--- a/src/modules/alsa/alsa-ucm.c
+++ b/src/modules/alsa/alsa-ucm.c
@@ -81,19 +81,11 @@  static void device_add_hw_mute_jack(pa_alsa_ucm_device *device, pa_alsa_jack *ja
 
 static pa_alsa_ucm_device *verb_find_device(pa_alsa_ucm_verb *verb, const char *device_name);
 
-struct ucm_port {
-    pa_alsa_ucm_config *ucm;
-    pa_device_port *core_port;
-
-    /* A single port will be associated with multiple devices if it represents
-     * a combination of devices. */
-    pa_dynarray *devices; /* pa_alsa_ucm_device */
-};
 
-static void ucm_port_init(struct ucm_port *port, pa_alsa_ucm_config *ucm, pa_device_port *core_port,
-                          pa_alsa_ucm_device **devices, unsigned n_devices);
-static void ucm_port_free(pa_device_port *port);
-static void ucm_port_update_available(struct ucm_port *port);
+static void ucm_port_data_init(pa_alsa_ucm_port_data *port, pa_alsa_ucm_config *ucm, pa_device_port *core_port,
+                               pa_alsa_ucm_device **devices, unsigned n_devices);
+static void ucm_port_data_free(pa_device_port *port);
+static void ucm_port_update_available(pa_alsa_ucm_port_data *port);
 
 static struct ucm_items item[] = {
     {"PlaybackPCM", PA_ALSA_PROP_UCM_SINK},
@@ -297,6 +289,17 @@  static int ucm_get_device_property(
             else
                 pa_log_debug("UCM playback priority %s for device %s error", value, device_name);
         }
+
+        /* Volume control element */
+        if (!device->playback_volume)
+            device->playback_volume = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, pa_xfree,
+                                                          pa_xfree);
+
+        value = pa_proplist_gets(device->proplist, PA_ALSA_PROP_UCM_PLAYBACK_VOLUME);
+        if (value) {
+            pa_hashmap_put(device->playback_volume, pa_xstrdup(pa_proplist_gets(verb->proplist, PA_ALSA_PROP_UCM_NAME)),
+                           pa_xstrdup(value));
+        }
     }
 
     if (device->capture_channels) { /* source device */
@@ -318,6 +321,17 @@  static int ucm_get_device_property(
             else
                 pa_log_debug("UCM capture priority %s for device %s error", value, device_name);
         }
+
+        /* Volume control element */
+        if (!device->capture_volume)
+            device->capture_volume = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, pa_xfree,
+                                                         pa_xfree);
+
+        value = pa_proplist_gets(device->proplist, PA_ALSA_PROP_UCM_CAPTURE_VOLUME);
+        if (value) {
+            pa_hashmap_put(device->capture_volume, pa_xstrdup(pa_proplist_gets(verb->proplist, PA_ALSA_PROP_UCM_NAME)),
+                           pa_xstrdup(value));
+        }
     }
 
     if (PA_UCM_PLAYBACK_PRIORITY_UNSET(device) || PA_UCM_CAPTURE_PRIORITY_UNSET(device)) {
@@ -693,6 +707,46 @@  static int pa_alsa_ucm_device_cmp(const void *a, const void *b) {
     return strcmp(pa_proplist_gets(d1->proplist, PA_ALSA_PROP_UCM_NAME), pa_proplist_gets(d2->proplist, PA_ALSA_PROP_UCM_NAME));
 }
 
+static void probe_volumes(pa_hashmap *hash, snd_pcm_t *pcm_handle, bool ignore_dB) {
+    pa_device_port *port;
+    pa_alsa_path *path;
+    pa_alsa_ucm_port_data *data;
+    snd_mixer_t *mixer_handle;
+    const char *profile;
+    void *state, *state2;
+
+    if (!(mixer_handle = pa_alsa_open_mixer_for_pcm(pcm_handle, NULL))) {
+        pa_log_error("Failed to find a working mixer device.");
+        goto fail;
+    }
+
+    PA_HASHMAP_FOREACH(port, hash, state) {
+        data = PA_DEVICE_PORT_DATA(port);
+
+        PA_HASHMAP_FOREACH_KV(profile, path, data->paths, state2) {
+            if (pa_alsa_path_probe(path, mixer_handle, ignore_dB) < 0) {
+                pa_log_warn("Could not probe path: %s, using s/w volume", data->path->name);
+                pa_hashmap_remove(data->paths, profile);
+            } else
+                pa_log_debug("Set up h/w volume using '%s' for %s:%s", path->name, profile, port->name);
+        }
+    }
+
+    snd_mixer_close(mixer_handle);
+
+    return;
+
+fail:
+    /* We could not probe the paths we created. Free them and revert to software volumes. */
+    PA_HASHMAP_FOREACH(port, hash, state) {
+        data = PA_DEVICE_PORT_DATA(port);
+        pa_hashmap_remove_all(data->paths);
+
+        data->paths = NULL;
+        data->path = NULL;
+    }
+}
+
 static void ucm_add_port_combination(
         pa_hashmap *hash,
         pa_alsa_ucm_mapping_context *context,
@@ -710,7 +764,10 @@  static void ucm_add_port_combination(
     char *name, *desc;
     const char *dev_name;
     const char *direction;
+    const char *profile, *volume;
     pa_alsa_ucm_device *sorted[num], *dev;
+    pa_alsa_ucm_port_data *data;
+    void *state;
 
     for (i = 0; i < num; i++)
         sorted[i] = pdevices[i];
@@ -758,8 +815,6 @@  static void ucm_add_port_combination(
 
     port = pa_hashmap_get(ports, name);
     if (!port) {
-        struct ucm_port *ucm_port;
-
         pa_device_port_new_data port_data;
 
         pa_device_port_new_data_init(&port_data);
@@ -767,17 +822,33 @@  static void ucm_add_port_combination(
         pa_device_port_new_data_set_description(&port_data, desc);
         pa_device_port_new_data_set_direction(&port_data, is_sink ? PA_DIRECTION_OUTPUT : PA_DIRECTION_INPUT);
 
-        port = pa_device_port_new(core, &port_data, sizeof(struct ucm_port));
-        port->impl_free = ucm_port_free;
+        port = pa_device_port_new(core, &port_data, sizeof(pa_alsa_ucm_port_data));
         pa_device_port_new_data_done(&port_data);
 
-        ucm_port = PA_DEVICE_PORT_DATA(port);
-        ucm_port_init(ucm_port, context->ucm, port, pdevices, num);
+        data = PA_DEVICE_PORT_DATA(port);
+        ucm_port_data_init(data, context->ucm, port, pdevices, num);
+        port->impl_free = ucm_port_data_free;
 
         pa_hashmap_put(ports, port->name, port);
         pa_log_debug("Add port %s: %s", port->name, port->description);
     }
 
+    if (num == 1) {
+        /* To keep things simple and not worry about stacking controls, we only support hardware volumes on non-combination
+         * ports. */
+        data = PA_DEVICE_PORT_DATA(port);
+
+        PA_HASHMAP_FOREACH_KV(profile, volume, is_sink ? dev->playback_volume : dev->capture_volume, state) {
+            pa_alsa_path *path = pa_alsa_path_synthesize(volume,
+                    is_sink ? PA_ALSA_DIRECTION_OUTPUT : PA_ALSA_DIRECTION_INPUT);
+
+            if (!path)
+                pa_log_warn("Failed to set up volume control: %s", volume);
+            else
+                pa_hashmap_put(data->paths, pa_xstrdup(profile), path);
+        }
+    }
+
     port->priority = priority;
 
     pa_xfree(name);
@@ -957,7 +1028,9 @@  void pa_alsa_ucm_add_ports(
         pa_proplist *proplist,
         pa_alsa_ucm_mapping_context *context,
         bool is_sink,
-        pa_card *card) {
+        pa_card *card,
+        snd_pcm_t *pcm_handle,
+        bool ignore_dB) {
 
     uint32_t idx;
     char *merged_roles;
@@ -972,6 +1045,9 @@  void pa_alsa_ucm_add_ports(
     /* add ports first */
     pa_alsa_ucm_add_ports_combination(*p, context, is_sink, card->ports, NULL, card->core);
 
+    /* now set up volume paths if any */
+    probe_volumes(*p, pcm_handle, ignore_dB);
+
     /* then set property PA_PROP_DEVICE_INTENDED_ROLES */
     merged_roles = pa_xstrdup(pa_proplist_gets(proplist, PA_PROP_DEVICE_INTENDED_ROLES));
     PA_IDXSET_FOREACH(dev, context->ucm_devices, idx) {
@@ -996,10 +1072,13 @@  void pa_alsa_ucm_add_ports(
 }
 
 /* Change UCM verb and device to match selected card profile */
-int pa_alsa_ucm_set_profile(pa_alsa_ucm_config *ucm, const char *new_profile, const char *old_profile) {
+int pa_alsa_ucm_set_profile(pa_alsa_ucm_config *ucm, pa_card *card, const char *new_profile, const char *old_profile) {
     int ret = 0;
     const char *profile;
     pa_alsa_ucm_verb *verb;
+    pa_device_port *port;
+    pa_alsa_ucm_port_data *data;
+    void *state;
 
     if (new_profile == old_profile)
         return ret;
@@ -1028,6 +1107,12 @@  int pa_alsa_ucm_set_profile(pa_alsa_ucm_config *ucm, const char *new_profile, co
         }
     }
 
+    /* select volume controls on ports */
+    PA_HASHMAP_FOREACH(port, card->ports, state) {
+        data = PA_DEVICE_PORT_DATA(port);
+        data->path = pa_hashmap_get(data->paths, new_profile);
+    }
+
     return ret;
 }
 
@@ -1636,11 +1721,18 @@  static void free_verb(pa_alsa_ucm_verb *verb) {
         if (di->ucm_ports)
             pa_dynarray_free(di->ucm_ports);
 
+        if (di->playback_volume)
+            pa_hashmap_free(di->playback_volume);
+        if (di->capture_volume)
+            pa_hashmap_free(di->capture_volume);
+
         pa_proplist_free(di->proplist);
+
         if (di->conflicting_devices)
             pa_idxset_free(di->conflicting_devices, NULL);
         if (di->supported_devices)
             pa_idxset_free(di->supported_devices, NULL);
+
         pa_xfree(di);
     }
 
@@ -1771,7 +1863,7 @@  void pa_alsa_ucm_roled_stream_end(pa_alsa_ucm_config *ucm, const char *role, pa_
     }
 }
 
-static void device_add_ucm_port(pa_alsa_ucm_device *device, struct ucm_port *port) {
+static void device_add_ucm_port(pa_alsa_ucm_device *device, pa_alsa_ucm_port_data *port) {
     pa_assert(device);
     pa_assert(port);
 
@@ -1799,7 +1891,7 @@  static void device_add_hw_mute_jack(pa_alsa_ucm_device *device, pa_alsa_jack *ja
 }
 
 static void device_set_available(pa_alsa_ucm_device *device, pa_available_t available) {
-    struct ucm_port *port;
+    pa_alsa_ucm_port_data *port;
     unsigned idx;
 
     pa_assert(device);
@@ -1833,8 +1925,8 @@  void pa_alsa_ucm_device_update_available(pa_alsa_ucm_device *device) {
     device_set_available(device, available);
 }
 
-static void ucm_port_init(struct ucm_port *port, pa_alsa_ucm_config *ucm, pa_device_port *core_port,
-                          pa_alsa_ucm_device **devices, unsigned n_devices) {
+static void ucm_port_data_init(pa_alsa_ucm_port_data *port, pa_alsa_ucm_config *ucm, pa_device_port *core_port,
+                               pa_alsa_ucm_device **devices, unsigned n_devices) {
     unsigned i;
 
     pa_assert(ucm);
@@ -1850,11 +1942,14 @@  static void ucm_port_init(struct ucm_port *port, pa_alsa_ucm_config *ucm, pa_dev
         device_add_ucm_port(devices[i], port);
     }
 
+    port->paths = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, pa_xfree,
+                                      (pa_free_cb_t) pa_alsa_path_free);
+
     ucm_port_update_available(port);
 }
 
-static void ucm_port_free(pa_device_port *port) {
-    struct ucm_port *ucm_port;
+static void ucm_port_data_free(pa_device_port *port) {
+    pa_alsa_ucm_port_data *ucm_port;
 
     pa_assert(port);
 
@@ -1862,9 +1957,12 @@  static void ucm_port_free(pa_device_port *port) {
 
     if (ucm_port->devices)
         pa_dynarray_free(ucm_port->devices);
+
+    if (ucm_port->paths)
+        pa_hashmap_free(ucm_port->paths);
 }
 
-static void ucm_port_update_available(struct ucm_port *port) {
+static void ucm_port_update_available(pa_alsa_ucm_port_data *port) {
     pa_alsa_ucm_device *device;
     unsigned idx;
     pa_available_t available = PA_AVAILABLE_YES;
@@ -1896,7 +1994,7 @@  pa_alsa_profile_set* pa_alsa_ucm_add_profile_set(pa_alsa_ucm_config *ucm, pa_cha
     return NULL;
 }
 
-int pa_alsa_ucm_set_profile(pa_alsa_ucm_config *ucm, const char *new_profile, const char *old_profile) {
+int pa_alsa_ucm_set_profile(pa_alsa_ucm_config *ucm, pa_card *card, const char *new_profile, const char *old_profile) {
     return -1;
 }
 
@@ -1909,7 +2007,9 @@  void pa_alsa_ucm_add_ports(
         pa_proplist *proplist,
         pa_alsa_ucm_mapping_context *context,
         bool is_sink,
-        pa_card *card) {
+        pa_card *card,
+        snd_pcm_t *pcm_handle,
+        bool ignore_dB) {
 }
 
 void pa_alsa_ucm_add_ports_combination(
diff --git a/src/modules/alsa/alsa-ucm.h b/src/modules/alsa/alsa-ucm.h
index 53abf3f..f566574 100644
--- a/src/modules/alsa/alsa-ucm.h
+++ b/src/modules/alsa/alsa-ucm.h
@@ -95,10 +95,11 @@  typedef struct pa_alsa_ucm_modifier pa_alsa_ucm_modifier;
 typedef struct pa_alsa_ucm_device pa_alsa_ucm_device;
 typedef struct pa_alsa_ucm_config pa_alsa_ucm_config;
 typedef struct pa_alsa_ucm_mapping_context pa_alsa_ucm_mapping_context;
+typedef struct pa_alsa_ucm_port_data pa_alsa_ucm_port_data;
 
 int pa_alsa_ucm_query_profiles(pa_alsa_ucm_config *ucm, int card_index);
 pa_alsa_profile_set* pa_alsa_ucm_add_profile_set(pa_alsa_ucm_config *ucm, pa_channel_map *default_channel_map);
-int pa_alsa_ucm_set_profile(pa_alsa_ucm_config *ucm, const char *new_profile, const char *old_profile);
+int pa_alsa_ucm_set_profile(pa_alsa_ucm_config *ucm, pa_card *card, const char *new_profile, const char *old_profile);
 
 int pa_alsa_ucm_get_verb(snd_use_case_mgr_t *uc_mgr, const char *verb_name, const char *verb_desc, pa_alsa_ucm_verb **p_verb);
 
@@ -107,7 +108,9 @@  void pa_alsa_ucm_add_ports(
         pa_proplist *proplist,
         pa_alsa_ucm_mapping_context *context,
         bool is_sink,
-        pa_card *card);
+        pa_card *card,
+        snd_pcm_t *pcm_handle,
+        bool ignore_dB);
 void pa_alsa_ucm_add_ports_combination(
         pa_hashmap *hash,
         pa_alsa_ucm_mapping_context *context,
@@ -139,6 +142,11 @@  struct pa_alsa_ucm_device {
     unsigned playback_channels;
     unsigned capture_channels;
 
+    /* These may be different per verb, so we store this as a hashmap of verb -> volume_control. We might eventually want to
+     * make this a hashmap of verb -> per-verb-device-properties-struct. */
+    pa_hashmap *playback_volume;
+    pa_hashmap *capture_volume;
+
     pa_alsa_mapping *playback_mapping;
     pa_alsa_mapping *capture_mapping;
 
@@ -206,4 +214,18 @@  struct pa_alsa_ucm_mapping_context {
     pa_idxset *ucm_modifiers;
 };
 
+struct pa_alsa_ucm_port_data {
+    pa_alsa_ucm_config *ucm;
+    pa_device_port *core_port;
+
+    /* A single port will be associated with multiple devices if it represents
+     * a combination of devices. */
+    pa_dynarray *devices; /* pa_alsa_ucm_device */
+
+    /* profile -> pa_alsa_path for volume control */
+    pa_hashmap *paths;
+    /* Current path, set when activating profile */
+    pa_alsa_path *path;
+};
+
 #endif
diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
index 286cfc9..b743943 100644
--- a/src/modules/alsa/module-alsa-card.c
+++ b/src/modules/alsa/module-alsa-card.c
@@ -238,7 +238,7 @@  static int card_set_profile(pa_card *c, pa_card_profile *new_profile) {
 
     /* if UCM is available for this card then update the verb */
     if (u->use_ucm) {
-        if (pa_alsa_ucm_set_profile(&u->ucm, nd->profile ? nd->profile->name : NULL,
+        if (pa_alsa_ucm_set_profile(&u->ucm, c, nd->profile ? nd->profile->name : NULL,
                     od->profile ? od->profile->name : NULL) < 0) {
             ret = -1;
             goto finish;
@@ -291,7 +291,7 @@  static void init_profile(struct userdata *u) {
 
     if (d->profile && u->use_ucm) {
         /* Set initial verb */
-        if (pa_alsa_ucm_set_profile(ucm, d->profile->name, NULL) < 0) {
+        if (pa_alsa_ucm_set_profile(ucm, u->card, d->profile->name, NULL) < 0) {
             pa_log("Failed to set ucm profile %s", d->profile->name);
             return;
         }

Comments

On Tue, 2016-05-03 at 18:22 +0530, arun@accosted.net wrote:
> @@ -2479,7 +2515,8 @@ static void userdata_free(struct userdata *u) {
>      if (u->mixer_fdl)
>          pa_alsa_fdlist_free(u->mixer_fdl);
>  
> -    if (u->mixer_path && !u->mixer_path_set)
> +    /* Only free the mixer_path if the sink owns it */

This same comment could be added to alsa-source.c.

>  static int source_set_port_ucm_cb(pa_source *s, pa_device_port *p) {
>      struct userdata *u = s->userdata;
> +    pa_alsa_ucm_port_data *data;
> +
> +    data = PA_DEVICE_PORT_DATA(p);
>  
>      pa_assert(u);
>      pa_assert(p);
>      pa_assert(u->ucm_context);
> +    pa_assert(u->mixer_handle);
> +
> +    u->mixer_path = data->path;
> +    mixer_volume_init(u);
> +
> +    if (u->mixer_path) {
> +        pa_alsa_path_select(u->mixer_path, NULL, u->mixer_handle, s->muted);
> +
> +        if (s->set_mute)
> +            s->set_mute(s);
> +        if (s->flags & PA_SINK_DEFERRED_VOLUME) {

Should be PA_SOURCE_DEFERRED_VOLUME.

> @@ -297,6 +289,17 @@ static int ucm_get_device_property(
>              else
>                  pa_log_debug("UCM playback priority %s for device %s error", value, device_name);
>          }
> +
> +        /* Volume control element */
> +        if (!device->playback_volume)
> +            device->playback_volume = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, pa_xfree,
> +                                                          pa_xfree);

I think it would make more sense to create this hashmap when creating
the device object (in ucm_get_devices).

> @@ -318,6 +321,17 @@ static int ucm_get_device_property(
>              else
>                  pa_log_debug("UCM capture priority %s for device %s error", value, device_name);
>          }
> +
> +        /* Volume control element */
> +        if (!device->capture_volume)
> +            device->capture_volume = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, pa_xfree,
> +                                                         pa_xfree);

Same as above.

> +static void probe_volumes(pa_hashmap *hash, snd_pcm_t *pcm_handle, bool ignore_dB) {
> +    pa_device_port *port;
> +    pa_alsa_path *path;
> +    pa_alsa_ucm_port_data *data;
> +    snd_mixer_t *mixer_handle;
> +    const char *profile;
> +    void *state, *state2;
> +
> +    if (!(mixer_handle = pa_alsa_open_mixer_for_pcm(pcm_handle, NULL))) {
> +        pa_log_error("Failed to find a working mixer device.");
> +        goto fail;
> +    }
> +
> +    PA_HASHMAP_FOREACH(port, hash, state) {
> +        data = PA_DEVICE_PORT_DATA(port);
> +
> +        PA_HASHMAP_FOREACH_KV(profile, path, data->paths, state2) {
> +            if (pa_alsa_path_probe(path, mixer_handle, ignore_dB) < 0) {
> +                pa_log_warn("Could not probe path: %s, using s/w volume", data->path->name);
> +                pa_hashmap_remove(data->paths, profile);
> +            } else
> +                pa_log_debug("Set up h/w volume using '%s' for %s:%s", path->name, profile, port->name);
> +        }
> +    }
> +
> +    snd_mixer_close(mixer_handle);
> +
> +    return;
> +
> +fail:
> +    /* We could not probe the paths we created. Free them and revert to software volumes. */
> +    PA_HASHMAP_FOREACH(port, hash, state) {
> +        data = PA_DEVICE_PORT_DATA(port);
> +        pa_hashmap_remove_all(data->paths);
> +
> +        data->paths = NULL;

You only removed the contents of data->paths, you didn't free the
hashmap, so setting data->paths to NULL here leaks the hashmap. I think
it's better to keep the hashmap alive but empty, so this assignment can
just be dropped.

> +        data->path = NULL;

This is unnecessary too, because data->path is set when activating a
profile, and volumes are probed before the initial profile has been
chosen. data->path is already NULL at this point.

> @@ -710,7 +764,10 @@ static void ucm_add_port_combination(
>      char *name, *desc;
>      const char *dev_name;
>      const char *direction;
> +    const char *profile, *volume;

I think "volume" can be a bit confusing name (I tend associate it with
something that contains an actual volume value). I'd prefer "element"
or "volume_element". Or "element_name". Or something like that. Since
the UCM variable name is "PlaybackVolume" or "CaptureVolume", I
understand that "volume" is justifiable from that point of view,
though. I don't mind if you prefer to keep the variable name as is.

> @@ -139,6 +142,11 @@ struct pa_alsa_ucm_device {
>      unsigned playback_channels;
>      unsigned capture_channels;
>  
> +    /* These may be different per verb, so we store this as a hashmap of verb -> volume_control. We might eventually want to
> +     * make this a hashmap of verb -> per-verb-device-properties-struct. */
> +    pa_hashmap *playback_volume;
> +    pa_hashmap *capture_volume;

"playback_volumes"/"capture_volumes" would make more sense to me, since
these variables are containers that can hold multiple values. Bonus
points for including "elements" or "element_names" in the variable
names (that can get a bit long, though)...

-- 
Tanu