On 19 March 2015 at 00:53, Tanu Kaskinen <tanu.kaskinen at linux.intel.com> wrote:
> On Wed, 2015-03-18 at 12:30 +0530, Arun Raghavan wrote:
>> On Thu, 2015-02-12 at 15:11 +0200, Tanu Kaskinen wrote:
>> > "JackHWMute" is a UCM device value that, if set, indicates that the
>> > jack of the device mutes some other device at the hardware or driver
>> > level. A common example would be a headphone jack that mutes built-in
>> > speakers. PulseAudio should show such auto-muted devices as
>> > unavailable.
>>
>> Wouldn't the "right" way for this to happen be to have the corresponding
>> mute control of the device be set to mute by the driver? Are there
>> circumstances where this might not be possible, or are we basically
>> providing a shortcut to describe hardware behaviour via configuration
>> rather than via the driver (i.e. exposing and updating kcontrols)?
>
> Just muting the speaker control isn't enough to convey that trying to
> unmute the control will be futile. At least on my laptop the speaker
> mute control does nothing when headphones are plugged in. I don't know
> if that's a driver bug or a genuine hardware limitation.
You're right -- it could be either, and we need to be able to deal with that.
>> > diff --git a/src/modules/alsa/alsa-mixer.h b/src/modules/alsa/alsa-mixer.h
>> > index ec39fab..dad1ea2 100644
>> > --- a/src/modules/alsa/alsa-mixer.h
>> > +++ b/src/modules/alsa/alsa-mixer.h
>> > @@ -168,8 +168,22 @@ struct pa_alsa_jack {
>> > pa_alsa_required_t required;
>> > pa_alsa_required_t required_any;
>> > pa_alsa_required_t required_absent;
>> > +
>> > + /* UCM specific: this is the "owner" device of this jack. When the jack is
>> > + * plugged in, the device referenced here gets marked as available, and
>> > + * when the jack is unplugged, the device gets marked as unavailable. */
>> > + pa_alsa_ucm_device *device;
>> > +
>> > + /* UCM specific: if this is non-NULL, then the referenced device gets muted
>> > + * by the hardware (or driver) when this jack is plugged in. The muting can't
>> > + * be overridden by PulseAudio, so the device gets marked as
>> > + * unavailable (so if a device is referenced by this field, the effect is
>> > + * inverse to being referenced by the "device" field). */
>> > + pa_alsa_ucm_device *hw_mute;
>> > };
>>
>> Might make sense to prefix those two with "ucm_" to make the meaning
>> clearer wherever they're used.
>
> Yes, that seems sensible.
>
>> > diff --git a/src/modules/alsa/alsa-ucm.c b/src/modules/alsa/alsa-ucm.c
>> > index ef6adcd..061b8f7 100644
>> > --- a/src/modules/alsa/alsa-ucm.c
>> > +++ b/src/modules/alsa/alsa-ucm.c
>> > @@ -66,6 +66,17 @@
>> >
>> > #ifdef HAVE_ALSA_UCM
>> >
>> > +static void device_set_mute_jack(pa_alsa_ucm_device *device, pa_alsa_jack *jack);
>> > +
>> > +struct port {
>>
>> I'd prefer ucm_port or pa_alsa_ucm_port -- the namespacing means that
>> it's easier to look up stuff with ctags/cscope/whatever shiny thing
>> comes in the future (maybe some day we'll have a way of distinguishing
>> the several struct userdatas that we have too).
>
> I'd be strongly against pa_alsa_ucm_port. It's a private struct, and the
> convention is to not prefix those with pa_, and I think that's a good
> convention. I suppose ucm_port would be ok, but I feel that it's
> redundant, since the file name already implies that everything is ucm
> related (I also have a todo item for removing the ucm_ prefix from some
> of the function names in that file). I'm not familiar with
> ctags/cscope/whatever, so I'm pretty clueless as to how useful the extra
> prefixing would be when using those things.
cscope/ctags would basically make a list of available symbols, and
then you can jump to that symbol if it is mentioned elsewhere by
pressing 'Ctrl-]', or something like that. If there are multiple such
definitions, it'll let you select which one you want based on
file/line number.
I'm not super-fussed about struct ucm_port vs. struct port, but I do
prefer the former if you're not dead against it. I'll leave the final
call on this to you.
>> > @@ -394,9 +406,12 @@ static int ucm_get_devices(pa_alsa_ucm_verb *verb, snd_use_case_mgr_t *uc_mgr) {
>> > for (i = 0; i < num_dev; i += 2) {
>> > pa_alsa_ucm_device *d = pa_xnew0(pa_alsa_ucm_device, 1);
>> >
>> > + d->verb = verb;
>> > d->proplist = pa_proplist_new();
>> > pa_proplist_sets(d->proplist, PA_ALSA_PROP_UCM_NAME, pa_strnull(dev_list[i]));
>> > pa_proplist_sets(d->proplist, PA_ALSA_PROP_UCM_DESCRIPTION, pa_strna(dev_list[i + 1]));
>> > + d->ports = pa_dynarray_new(NULL);
>> > + d->available = true;
>>
>> Shouldn't the default availability be unknown, with actual availability
>> set if we have a corresponding jack?
>
> No, I think all ucm devices should be marked as available, unless
> otherwise indicated by the jacks. The difference to the "normal" alsa
> mixer stuff is that with UCM the configuration is tailored for the
> hardware, while the "normal" alsa mixer stuff always tries to use all
> paths.
>
> ...or that's at least what I told myself when I wrote this code.
> Thinking about this again, that logic doesn't seem to hold water. In the
> normal alsa mixer stuff ports are marked with unknown availability when
> there's no jack control associated with a path, and I suppose there's no
> guarantee with UCM either that there would be a jack control associated
> always with e.g. headphones. I suppose you're right, I should set the
> device availability to unknown if there's no information from jacks.
>
>> > @@ -1591,6 +1820,15 @@ void pa_alsa_ucm_free(pa_alsa_ucm_config *ucm) {
>> > pa_alsa_ucm_verb *vi, *vn;
>> > pa_alsa_jack *ji, *jn;
>> >
>> > + if (ucm->ports) {
>> > + struct port *port;
>> > +
>> > + while ((port = pa_dynarray_last(ucm->ports)))
>> > + port_free(port);
>>
>> Should just pass port_free() as the free function in pa_dynarray_new().
>
> Well, that depends... With the current code port_free() is responsible
> for removing itself from the dynarray, which makes sense, since
> port_new() is responsible for adding the port to the dynarray. I'd
> really like to keep that symmetry.
Setting the free function up front makes it less likely that someone
calling remove on the dynarray will forget to free the instance too,
so IMO setting the free function is just good practice.
>> > @@ -142,10 +155,33 @@ struct pa_alsa_ucm_device {
>> > pa_idxset *conflicting_devices;
>> > pa_idxset *supported_devices;
>> >
>> > + pa_dynarray *ports; /* pa_alsa_ucm_port */
>> > +
>> > + /* These jacks are owned by this device. When these jacks are plugged in,
>> > + * the device is marked as available, and when these jacks are unplugged,
>> > + * the device is marked as unavailable. Not all devices have jacks
>> > + * associated with them; those devices are always marked as available.
>> > + *
>> > + * XXX: It's probably pointless to have separate jacks for input and
>> > + * output. Both jack objects will anyway reference the same alsa
>> > + * kcontrol, so they will always have the same state. */
>> > pa_alsa_jack *input_jack;
>> > pa_alsa_jack *output_jack;
>>
>> I don't know if this actually ever does happen this way, but I can
>> imagine that you might have separate headphone and mic jack kcontrols
>> for a combined physical headset port. If that might be the case, the XXX
>> comment can be removed.
>
> There's no support for such setup. The UCM "spec" doesn't allow separate
> controls for input and output. Perhaps it should, though. I'll raise
> this issue on alsa-devel.
Cool, I see it's been a good discussion already. :-)
>> > @@ -80,10 +78,14 @@ void pa_device_port_set_available(pa_device_port *p, pa_available_t status) {
>> > status == PA_AVAILABLE_NO ? "no" : "unknown");
>> >
>> > /* Post subscriptions to the card which owns us */
>> > - pa_assert_se(core = p->core);
>> > - pa_subscription_post(core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE, p->card->index);
>> > -
>> > - pa_hook_fire(&core->hooks[PA_CORE_HOOK_PORT_AVAILABLE_CHANGED], p);
>> > + /* XXX: We need to check p->card, because this function may be called
>> > + * before the card object has been created. The card object should probably
>> > + * be created before port objects, and then p->card could be non-NULL for
>> > + * the whole lifecycle of pa_device_port. */
>>
>> Mario was working on implementing David's comments from
>> https://bugs.freedesktop.org/show_bug.cgi?id=87002#c18 -- which might
>> make this comment obsolete (but the condition correct).
>
> I don't have a clear picture of what David proposes in that comment, but
> it sounds like ports would still be created before cards. If so, I don't
> know how that would make the comment obsolete, unless you're only
> referring to the "card object should probably be created before port
> objects" part. In that case it's a question of whose "should" we take as
> the truth... In my thinking the "bigger" (the container) object should
> always be created before the "smaller" (the contained) obt, because
> that way the small->big pointer can be constant and never-NULL, which
> helps with robustness and readability.
I misremembered -- that bug involved creating the pa_alsa_jack early,
not about creating the port early. I agree with your thinking here.
Regards,
Arun
