Hi David,
On 30/03/15 13:36, David Henningsson wrote:
> [...]
>> --- a/src/modules/alsa/alsa-mixer.c
>> +++ b/src/modules/alsa/alsa-mixer.c
>> @@ -4636,12 +4636,16 @@ static pa_device_port*
>> device_port_alsa_init(pa_hashmap *ports, /* card ports */
>> if (!p) {
>> pa_alsa_port_data *data;
>> pa_device_port_new_data port_data;
>> + pa_available_t pa = PA_AVAILABLE_UNKNOWN;
>
> Unnecessary assignment - pa is always set further down.
>
Ok.
>> pa_device_port_new_data_init(&port_data);
>> pa_device_port_new_data_set_name(&port_data, name);
>> pa_device_port_new_data_set_description(&port_data, description);
>> pa_device_port_new_data_set_direction(&port_data,
>> path->direction == PA_ALSA_DIRECTION_OUTPUT ? PA_DIRECTION_OUTPUT :
>> PA_DIRECTION_INPUT);
>>
>> + pa = pa_alsa_availability_from_jacks(path->jacks, NULL, NULL);
>> + pa_device_port_new_data_set_available(&port_data, pa);
>> +
>> p = pa_device_port_new(core, &port_data,
>> sizeof(pa_alsa_port_data));
>> pa_device_port_new_data_done(&port_data);
>> pa_assert(p);
>> diff --git a/src/modules/alsa/alsa-util.c b/src/modules/alsa/alsa-util.c
>> index bb79e71..8ac1ef3 100644
>> --- a/src/modules/alsa/alsa-util.c
>> +++ b/src/modules/alsa/alsa-util.c
>> @@ -1702,3 +1702,55 @@ int pa_alsa_get_hdmi_eld(snd_hctl_elem_t *elem,
>> pa_hdmi_eld *eld) {
>>
>> return 0;
>> }
>> +
>> +pa_available_t pa_alsa_availability_from_jacks(pa_alsa_jack *jacks_llist,
>> pa_device_port *port, pa_card *card) {
>> + pa_alsa_jack *jack;
>> + pa_available_t pa = PA_AVAILABLE_UNKNOWN;
>> + pa_device_port *p;
>> +
>> + PA_LLIST_FOREACH(jack, jacks_llist) {
>> + pa_available_t cpa;
>> +
>> + /* If a port has NOT been provided, it means we will trust the list
>> + * of jacks passed to be the ones we are interested in checking, so
>> + * get the actual port from the jack only when a port has been
>> passed. */
>> + if (port) {
>
> This is a refactoring from u->use_ucm to "if (card)". It works but I found
> it confusing at first. It's because ucm jacks don't have paths, so we have
> to go through the card instead.
>
Yes, it is exactly that.
> Maybe we could comment this a bit better, like putting this above the function:
>
> /*
> * port - can be null, in which case all jacks in the list will be
> * checked.
> * card - null for non-UCM scenarios, where we can use the
> * jack->path->port pointer. Set this to the right card only for UCM
> * jacks.
> */
>
Makes sense, will do that in a follow-up patch.
>> + if (card)
>> + p = pa_hashmap_get(card->ports, jack->name);
>> + else {
>> + if (jack->path)
>> + p = jack->path->port;
>> + else
>> + continue;
>> + }
>> +
>> + if (p != port)
>> + continue;
>> + }
>> +
>> + cpa = jack->plugged_in ? jack->state_plugged :
>> jack->state_unplugged;
>> +
>> + if (cpa == PA_AVAILABLE_NO) {
>
> Nitpick: this is probably not your fault, but indentation of this function
> is inconsistent.
>
It is not my fault, since I took this code from module-asla-card.c, but it's
true that this would be a good opportunity to smooth this. I'll do it.
>> + /* If a plugged-in jack causes the availability to go to NO, it
>> + * should override all other availability information (like a
>> + * blacklist) so set and bail */
>> + if (jack->plugged_in) {
>> + pa = cpa;
>> + break;
>> + }
>> +
>> + /* If the current availablility is unknown go the more precise no,
>> + * but otherwise don't change state */
>> + if (pa == PA_AVAILABLE_UNKNOWN)
>> + pa = cpa;
>> + } else if (cpa == PA_AVAILABLE_YES) {
>> + /* Output is available through at least one jack, so go to that
>> + * level of availability. We still need to continue iterating
>> through
>> + * the jacks in case a jack is plugged in that forces the state
>> to no
>> + */
>> + pa = cpa;
>> + }
>> + }
>> +
>> + return pa;
>> +}
>> diff --git a/src/modules/alsa/alsa-util.h b/src/modules/alsa/alsa-util.h
>> index 8345a0b..dc4b210 100644
>> --- a/src/modules/alsa/alsa-util.h
>> +++ b/src/modules/alsa/alsa-util.h
>> @@ -151,4 +151,6 @@ struct pa_hdmi_eld {
>>
>> int pa_alsa_get_hdmi_eld(snd_hctl_elem_t *elem, pa_hdmi_eld *eld);
>>
>> +pa_available_t pa_alsa_availability_from_jacks(pa_alsa_jack *jacks_llist,
>> pa_device_port *port, pa_card *card);
>> +
>> #endif
>> diff --git a/src/modules/alsa/module-alsa-card.c
>> b/src/modules/alsa/module-alsa-card.c
>> index a7fec04..4032f3b 100644
>> --- a/src/modules/alsa/module-alsa-card.c
>> +++ b/src/modules/alsa/module-alsa-card.c
>> @@ -310,54 +310,6 @@ static void init_profile(struct userdata *u) {
>> am->source = pa_alsa_source_new(u->module, u->modargs,
>> __FILE__, u->card, am);
>> }
>>
>> -static void report_port_state(pa_device_port *p, struct userdata *u) {
>> - void *state;
>> - pa_alsa_jack *jack;
>> - pa_available_t pa = PA_AVAILABLE_UNKNOWN;
>> - pa_device_port *port;
>> -
>> - PA_HASHMAP_FOREACH(jack, u->jacks, state) {
>> - pa_available_t cpa;
>> -
>> - if (u->use_ucm)
>> - port = pa_hashmap_get(u->card->ports, jack->name);
>> - else {
>> - if (jack->path)
>> - port = jack->path->port;
>> - else
>> - continue;
>> - }
>> -
>> - if (p != port)
>> - continue;
>> -
>> - cpa = jack->plugged_in ? jack->state_plugged :
>> jack->state_unplugged;
>> -
>> - if (cpa == PA_AVAILABLE_NO) {
>> - /* If a plugged-in jack causes the availability to go to NO, it
>> - * should override all other availability information (like a
>> - * blacklist) so set and bail */
>> - if (jack->plugged_in) {
>> - pa = cpa;
>> - break;
>> - }
>> -
>> - /* If the current availablility is unknown go the more precise no,
>> - * but otherwise don't change state */
>> - if (pa == PA_AVAILABLE_UNKNOWN)
>> - pa = cpa;
>> - } else if (cpa == PA_AVAILABLE_YES) {
>> - /* Output is available through at least one jack, so go to that
>> - * level of availability. We still need to continue iterating
>> through
>> - * the jacks in case a jack is plugged in that forces the state
>> to no
>> - */
>> - pa = cpa;
>> - }
>> - }
>> -
>> - pa_device_port_set_available(p, pa);
>> -}
>> -
>> static int report_jack_state(snd_mixer_elem_t *melem, unsigned int mask) {
>> struct userdata *u = snd_mixer_elem_get_callback_private(melem);
>> snd_hctl_elem_t *elem = snd_mixer_elem_get_private(melem);
>> @@ -388,13 +340,16 @@ static int report_jack_state(snd_mixer_elem_t
>> *melem, unsigned int mask) {
>> if (u->use_ucm) {
>> pa_assert(u->card->ports);
>> port = pa_hashmap_get(u->card->ports, jack->name);
>> - pa_assert(port);
>> }
>> else {
>> pa_assert(jack->path);
>> - pa_assert_se(port = jack->path->port);
>> + port = jack->path->port;
>> + }
>
> Inconsistent indentation.
>
Will fix it.
>> + if (port) {
>> + pa_card *card = u->use_ucm ? u->card : NULL;
>> + pa_available_t pa =
>> pa_alsa_availability_from_jacks(u->jacks, port, card);
>> + pa_device_port_set_available(port, pa);
>> }
>> - report_port_state(port, u);
>> }
>> return 0;
>> }
>> @@ -738,6 +693,8 @@ int pa__init(pa_module *m) {
>> if ((description = pa_proplist_gets(data.proplist,
>> PA_PROP_DEVICE_DESCRIPTION)))
>> pa_reserve_wrapper_set_application_device_name(reserve,
>> description);
>>
>> + init_jacks(u);
>> +
>> add_profiles(u, data.profiles, data.ports);
>>
>> if (pa_hashmap_isempty(data.profiles)) {
>> @@ -766,7 +723,6 @@ int pa__init(pa_module *m) {
>> u->card->userdata = u;
>> u->card->set_profile = card_set_profile;
>>
>> - init_jacks(u);
>> init_profile(u);
>> init_eld_ctls(u);
>>
>> diff --git a/src/modules/module-switch-on-port-available.c
>> b/src/modules/module-switch-on-port-available.c
>> index 5a6b401..b44819e 100644
>> --- a/src/modules/module-switch-on-port-available.c
>> +++ b/src/modules/module-switch-on-port-available.c
>> @@ -30,6 +30,7 @@
>>
>> struct userdata {
>> pa_hook_slot *available_slot;
>> + pa_hook_slot *card_put_slot;
>> pa_hook_slot *sink_new_slot;
>> pa_hook_slot *source_new_slot;
>> };
>> @@ -246,6 +247,60 @@ static pa_device_port *new_sink_source(pa_hashmap
>> *ports, const char *name) {
>> return p;
>> }
>>
>> +static bool profile_contains_available_ports(pa_card_profile *profile) {
>> + pa_card *card;
>> + pa_device_port *port;
>> + void *state;
>> +
>> + pa_assert(profile);
>> +
>> + card = profile->card;
>> + pa_assert(card);
>> +
>> + PA_HASHMAP_FOREACH(port, card->ports, state) {
>> + if (pa_hashmap_get(port->profiles, profile->name)
>> + && port->available != PA_AVAILABLE_NO)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static pa_card_profile *find_best_profile_with_available_ports(pa_card
>> *card) {
>> + pa_card_profile *profile = NULL;
>> + pa_card_profile *best_profile = NULL;
>> + void *state;
>> +
>> + pa_assert(card);
>> +
>> + PA_HASHMAP_FOREACH(profile, card->profiles, state) {
>> + if (profile->available == PA_AVAILABLE_NO)
>> + continue;
>> +
>> + if (!profile_contains_available_ports(profile))
>> + continue;
>> +
>> + if (!best_profile || profile->priority > best_profile->priority)
>> + best_profile = profile;
>> + }
>> +
>> + return best_profile;
>> +}
>> +
>> +static pa_hook_result_t card_put_hook_callback(pa_core *c, pa_card *card,
>> struct userdata *u) {
>> + pa_card_profile *alt_profile;
>> +
>> + if (profile_contains_available_ports(card->active_profile))
>> + return PA_HOOK_OK;
>
> We had some discussion about moving this to the card_new hook instead. I
> still think that would be a good idea (but feel free to prove me wrong).
>
I don't have any interest in proving you wrong, seriously, but rather in
writing the correct patch here, so I'm open to suggestions.
Furthermore, I was already a bit unsure about this bit because the original
idea was to do this on CARD_NEW, and doing it in CARD_PUT never felt quite
right (I think Arun will agree too), but was the best I could do for now.
> You'd need to change the above to:
>
> if (card->active_profile &&
> profile_contains_available_ports(card->active_profile))
> return PA_HOOK_OK;
>
> ...because card->active_profile isn't always set at that point (but can be,
> in case of module-card-restore).
>
Good point. I totally missed the interactions with the module-card-restore
module here and did this in CARD_PUT because we saw in pa_card_new() that
the active profile was being set there, right after emitting CARD_NEW.
> Also set save_profile to false when you're overwriting module-card-restore's
> suggestion. (Hmm, maybe I need to do the same for save_port in
> sink/source_new callbacks actually!)
>
Will do, thanks for the detailed suggestion!
>> +
>> + /* Try to avoid situations where we could settle on a profile when
>> + there are not available ports that could be actually used. */
>> + if (alt_profile = find_best_profile_with_available_ports(card))
>> + card->active_profile = alt_profile;
>> +
>> + return PA_HOOK_OK;
>> +}
>> +
>> static pa_hook_result_t sink_new_hook_callback(pa_core *c,
>> pa_sink_new_data *new_data, struct userdata *u) {
>>
>> pa_device_port *p = new_sink_source(new_data->ports,
>> new_data->active_port);
>> @@ -276,6 +331,8 @@ int pa__init(pa_module*m) {
>> m->userdata = u = pa_xnew(struct userdata, 1);
>>
>> /* Make sure we are after module-device-restore, so we can overwrite
>> that suggestion if necessary */
>> + u->card_put_slot =
>> pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_CARD_PUT],
>> + PA_HOOK_NORMAL, (pa_hook_cb_t)
>> card_put_hook_callback, u);
>> u->sink_new_slot =
>> pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_NEW],
>> PA_HOOK_NORMAL, (pa_hook_cb_t)
>> sink_new_hook_callback, u);
>> u->source_new_slot =
>> pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SOURCE_NEW],
>> @@ -298,6 +355,8 @@ void pa__done(pa_module*m) {
>>
>> if (u->available_slot)
>> pa_hook_slot_free(u->available_slot);
>> + if (u->card_put_slot)
>> + pa_hook_slot_free(u->card_put_slot);
>> if (u->sink_new_slot)
>> pa_hook_slot_free(u->sink_new_slot);
>> if (u->source_new_slot)
>>
>
>
Thanks,
Mario
