[PATCH 4/7] card: move profile selection after pa_card_new()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2015-10-23 12:56, Tanu Kaskinen wrote:
> I want module-alsa-card to set the availability of unavailable
> profiles before the initial card profile gets selected, so that the
> selection logic can use correct availability information.
> module-alsa-card initializes the jack state after calling
> pa_card_new(), however, and the profile selection happens in
> pa_card_new(). This patch solves that by introducing pa_card_put() and
> moving the profile selection code there.

I think it's a good idea to split card_new into card_new and card_put.

> An alternative solution would have been to move the jack
> initialization to happen before pa_card_new() and use pa_card_new_data
> instead of pa_card in the jack initialization code, but I disliked
> that idea (I want to get rid of the "new data" pattern eventually).
>
> The CARD_NEW hook is used when applying the initial profile policy, so
> that was moved to pa_card_put().

So now CARD_NEW is called from CARD_PUT, and they are called almost 
immediately after one another (the only difference is the card state).

The only module using CARD_NEW and CARD_PUT hooks is 
module-card-restore, so the only reason to move it is to accommodate the 
needs of module-card-restore. And here I'm lost - I'm not understanding 
why you need to move CARD_NEW.

As an option, if we set the state to linked *after* the CARD_PUT hook, 
would that help?

> That required changing the hook data
> from pa_card_new_data to pa_card. module-card-restore now uses
> pa_card_set_profile() instead of pa_card_new_data_set_profile(). That
> required adding a state variable to pa_card, because
> pa_card_set_profile() needs to distinguish between setting the initial
> profile and setting the profile in other situations.
>
> The order in which the initial profile policy is applied is reversed
> in this patch. Previously the first one to set it won, now the last
> one to set it wins. I think this is better, because if you have N
> parties that want to set the profile, we avoid checking N times
> whether someone else has already set the profile.

Is this just a matter of taste or would it be difficult to retain the 
old order? The old order seems more consistent with e g setting ports on 
sinks.

> ---
>   src/modules/alsa/module-alsa-card.c          | 19 ++++---
>   src/modules/bluetooth/module-bluez4-device.c | 18 +++---
>   src/modules/bluetooth/module-bluez5-device.c |  1 +
>   src/modules/macosx/module-coreaudio-device.c |  1 +
>   src/modules/module-card-restore.c            | 24 ++++----
>   src/pulsecore/card.c                         | 83 +++++++++++++++-------------
>   src/pulsecore/card.h                         |  7 +++
>   7 files changed, 87 insertions(+), 66 deletions(-)
>
> diff --git a/src/modules/alsa/module-alsa-card.c b/src/modules/alsa/module-alsa-card.c
> index 0943a17..ad463ec 100644
> --- a/src/modules/alsa/module-alsa-card.c
> +++ b/src/modules/alsa/module-alsa-card.c
> @@ -768,15 +768,6 @@ int pa__init(pa_module *m) {
>           goto fail;
>       }
>
> -    if ((profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
> -        if (pa_hashmap_get(data.profiles, profile))
> -            pa_card_new_data_set_profile(&data, profile);
> -        else {
> -            pa_log("No such profile: %s", profile);
> -            goto fail;
> -        }
> -    }
> -
>       u->card = pa_card_new(m->core, &data);
>       pa_card_new_data_done(&data);
>
> @@ -787,6 +778,16 @@ int pa__init(pa_module *m) {
>       u->card->set_profile = card_set_profile;
>
>       init_jacks(u);
> +    pa_card_put(u->card);
> +
> +    if ((profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
> +        u->card->active_profile = pa_hashmap_get(u->card->profiles, profile);
> +        if (!u->card->active_profile) {
> +            pa_log("No such profile: %s", profile);
> +            goto fail;
> +        }
> +    }
> +
>       init_profile(u);
>       init_eld_ctls(u);
>
> diff --git a/src/modules/bluetooth/module-bluez4-device.c b/src/modules/bluetooth/module-bluez4-device.c
> index 41b436f..b9e2335 100644
> --- a/src/modules/bluetooth/module-bluez4-device.c
> +++ b/src/modules/bluetooth/module-bluez4-device.c
> @@ -2304,15 +2304,6 @@ static int add_card(struct userdata *u) {
>       *d = PA_BLUEZ4_PROFILE_OFF;
>       pa_hashmap_put(data.profiles, p->name, p);
>
> -    if ((default_profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
> -        if (pa_hashmap_get(data.profiles, default_profile))
> -            pa_card_new_data_set_profile(&data, default_profile);
> -        else {
> -            pa_log("Profile '%s' not valid or not supported by device.", default_profile);
> -            return -1;
> -        }
> -    }
> -
>       u->card = pa_card_new(u->core, &data);
>       pa_card_new_data_done(&data);
>
> @@ -2323,6 +2314,15 @@ static int add_card(struct userdata *u) {
>
>       u->card->userdata = u;
>       u->card->set_profile = card_set_profile;
> +    pa_card_put(u->card);
> +
> +    if ((default_profile = pa_modargs_get_value(u->modargs, "profile", NULL))) {
> +        u->card->active_profile = pa_hashmap_get(u->card->profiles, default_profile);
> +        if (!u->card->active_profile) {
> +            pa_log("Profile '%s' not valid or not supported by device.", default_profile);
> +            return -1;
> +        }
> +    }
>
>       d = PA_CARD_PROFILE_DATA(u->card->active_profile);
>
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 19ce2b7..bf81384 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -1959,6 +1959,7 @@ static int add_card(struct userdata *u) {
>
>       u->card->userdata = u;
>       u->card->set_profile = set_profile_cb;
> +    pa_card_put(u->card);
>
>       p = PA_CARD_PROFILE_DATA(u->card->active_profile);
>       u->profile = *p;
> diff --git a/src/modules/macosx/module-coreaudio-device.c b/src/modules/macosx/module-coreaudio-device.c
> index cbf1f27..e100f7a 100644
> --- a/src/modules/macosx/module-coreaudio-device.c
> +++ b/src/modules/macosx/module-coreaudio-device.c
> @@ -765,6 +765,7 @@ int pa__init(pa_module *m) {
>       pa_card_new_data_done(&card_new_data);
>       u->card->userdata = u;
>       u->card->set_profile = card_set_profile;
> +    pa_card_put(u->card);
>
>       u->rtpoll = pa_rtpoll_new();
>       pa_thread_mq_init(&u->thread_mq, m->core->mainloop, u->rtpoll);
> diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
> index 3725d30..90bac30 100644
> --- a/src/modules/module-card-restore.c
> +++ b/src/modules/module-card-restore.c
> @@ -481,34 +481,38 @@ static pa_hook_result_t port_offset_change_callback(pa_core *c, pa_device_port *
>       return PA_HOOK_OK;
>   }
>
> -static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new_data, struct userdata *u) {
> +static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card *card, struct userdata *u) {
>       struct entry *e;
>       void *state;
>       pa_device_port *p;
>       struct port_info *p_info;
>
> -    pa_assert(new_data);
> +    pa_assert(c);
> +    pa_assert(card);
> +    pa_assert(u);
>
> -    if (!(e = entry_read(u, new_data->name)))
> +    if (!(e = entry_read(u, card->name)))
>           return PA_HOOK_OK;
>
>       if (e->profile[0]) {
> -        if (!new_data->active_profile) {
> -            pa_card_new_data_set_profile(new_data, e->profile);
> -            pa_log_info("Restored profile '%s' for card %s.", new_data->active_profile, new_data->name);
> -            new_data->save_profile = true;
> +        pa_card_profile *profile;
>
> +        profile = pa_hashmap_get(card->profiles, e->profile);
> +        if (profile) {
> +            pa_card_set_profile(card, profile, true);
> +            pa_log_info("Restored profile '%s' for card %s.", card->active_profile->name, card->name);
>           } else
> -            pa_log_debug("Not restoring profile for card %s, because already set.", new_data->name);
> +            pa_log_debug("Tried to restore profile %s for card %s, but the card doesn't have such profile.",
> +                         e->profile, card->name);
>       }
>
>       /* Always restore the latency offsets because their
>        * initial value is always 0 */
>
> -    pa_log_info("Restoring port latency offsets for card %s.", new_data->name);
> +    pa_log_info("Restoring port latency offsets for card %s.", card->name);
>
>       PA_HASHMAP_FOREACH(p_info, e->ports, state)
> -        if ((p = pa_hashmap_get(new_data->ports, p_info->name)))
> +        if ((p = pa_hashmap_get(card->ports, p_info->name)))
>               p->latency_offset = p_info->offset;
>
>       entry_free(e);
> diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
> index 4ef7900..67471cc 100644
> --- a/src/pulsecore/card.c
> +++ b/src/pulsecore/card.c
> @@ -139,6 +139,7 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
>       pa_assert(!pa_hashmap_isempty(data->profiles));
>
>       c = pa_xnew0(pa_card, 1);
> +    c->state = PA_CARD_STATE_INIT;
>
>       if (!(name = pa_namereg_register(core, data->name, PA_NAMEREG_CARD, c, data->namereg_fail))) {
>           pa_xfree(c);
> @@ -147,12 +148,6 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
>
>       pa_card_new_data_set_name(data, name);
>
> -    if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_NEW], data) < 0) {
> -        pa_xfree(c);
> -        pa_namereg_unregister(core, name);
> -        return NULL;
> -    }
> -
>       c->core = core;
>       c->name = pa_xstrdup(data->name);
>       c->proplist = pa_proplist_copy(data->proplist);
> @@ -175,38 +170,43 @@ pa_card *pa_card_new(pa_core *core, pa_card_new_data *data) {
>       PA_HASHMAP_FOREACH(port, c->ports, state)
>           port->card = c;
>
> -    if (data->active_profile)
> -        if ((c->active_profile = pa_hashmap_get(c->profiles, data->active_profile)))
> -            c->save_profile = data->save_profile;
> -
> -    if (!c->active_profile) {
> -        PA_HASHMAP_FOREACH(profile, c->profiles, state) {
> -            if (profile->available == PA_AVAILABLE_NO)
> -                continue;
> -
> -            if (!c->active_profile || profile->priority > c->active_profile->priority)
> -                c->active_profile = profile;
> -        }
> -        /* If all profiles are not available, then we still need to pick one */
> -        if (!c->active_profile) {
> -            PA_HASHMAP_FOREACH(profile, c->profiles, state)
> -                if (!c->active_profile || profile->priority > c->active_profile->priority)
> -                    c->active_profile = profile;
> -        }
> -        pa_assert(c->active_profile);
> -    }
> -
>       pa_device_init_description(c->proplist, c);
>       pa_device_init_icon(c->proplist, true);
>       pa_device_init_intended_roles(c->proplist);
>
> -    pa_assert_se(pa_idxset_put(core->cards, c, &c->index) >= 0);
> +    return c;
> +}
> +
> +void pa_card_put(pa_card *card) {
> +    pa_card_profile *profile;
> +    void *state;
>
> -    pa_log_info("Created %u \"%s\"", c->index, c->name);
> -    pa_subscription_post(core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, c->index);
> +    pa_assert(card);
>
> -    pa_hook_fire(&core->hooks[PA_CORE_HOOK_CARD_PUT], c);
> -    return c;
> +    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> +        if (profile->available == PA_AVAILABLE_NO)
> +            continue;
> +
> +        if (!card->active_profile || profile->priority > card->active_profile->priority)
> +            card->active_profile = profile;
> +    }
> +
> +    /* If all profiles are unavailable, then we still need to pick one */
> +    if (!card->active_profile) {
> +        PA_HASHMAP_FOREACH(profile, card->profiles, state)
> +            if (!card->active_profile || profile->priority > card->active_profile->priority)
> +                card->active_profile = profile;
> +    }
> +    pa_assert(card->active_profile);
> +
> +    pa_hook_fire(&card->core->hooks[PA_CORE_HOOK_CARD_NEW], card);
> +
> +    pa_assert_se(pa_idxset_put(card->core->cards, card, &card->index) >= 0);
> +    card->state = PA_CARD_STATE_LINKED;
> +
> +    pa_log_info("Created %u \"%s\"", card->index, card->name);
> +    pa_hook_fire(&card->core->hooks[PA_CORE_HOOK_CARD_PUT], card);
> +    pa_subscription_post(card->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, card->index);
>   }
>
>   void pa_card_free(pa_card *c) {
> @@ -273,17 +273,24 @@ int pa_card_set_profile(pa_card *c, pa_card_profile *profile, bool save) {
>           return 0;
>       }
>
> -    if ((r = c->set_profile(c, profile)) < 0)
> +    /* If we're setting the initial profile, we shouldn't call set_profile(),
> +     * because the implementations don't expect that (for historical reasons).
> +     * We should just set c->active_profile, and the implementations will
> +     * properly set up that profile after pa_card_put() has returned. It would
> +     * be probably good to change this so that also the initial profile can be
> +     * set up in set_profile(), but if set_profile() fails, that would need
> +     * some better handling than what we do here currently. */
> +    if (c->state != PA_CARD_STATE_INIT && (r = c->set_profile(c, profile)) < 0)
>           return r;
>
> -    pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, c->index);
> -
> -    pa_log_info("Changed profile of card %u \"%s\" to %s", c->index, c->name, profile->name);
> -
>       c->active_profile = profile;
>       c->save_profile = save;
>
> -    pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
> +    if (c->state != PA_CARD_STATE_INIT) {
> +        pa_log_info("Changed profile of card %u \"%s\" to %s", c->index, c->name, profile->name);
> +        pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
> +        pa_subscription_post(c->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, c->index);
> +    }
>
>       return 0;
>   }
> diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
> index 1c33958..dbbc1c2 100644
> --- a/src/pulsecore/card.h
> +++ b/src/pulsecore/card.h
> @@ -37,6 +37,11 @@ typedef enum pa_available {
>   #include <pulsecore/module.h>
>   #include <pulsecore/idxset.h>
>
> +typedef enum pa_card_state {
> +    PA_CARD_STATE_INIT,
> +    PA_CARD_STATE_LINKED,
> +} pa_card_state_t;
> +
>   typedef struct pa_card_profile {
>       pa_card *card;
>       char *name;
> @@ -61,6 +66,7 @@ typedef struct pa_card_profile {
>
>   struct pa_card {
>       uint32_t index;
> +    pa_card_state_t state;
>       pa_core *core;
>
>       char *name;
> @@ -115,6 +121,7 @@ void pa_card_new_data_set_profile(pa_card_new_data *data, const char *profile);
>   void pa_card_new_data_done(pa_card_new_data *data);
>
>   pa_card *pa_card_new(pa_core *c, pa_card_new_data *data);
> +void pa_card_put(pa_card *c);
>   void pa_card_free(pa_card *c);
>
>   void pa_card_add_profile(pa_card *c, pa_card_profile *profile);
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux