Re: Surround speaker connection on Acer 8951G

From: Sergey 'Jin' Bostandzhyan <jin@mediatomb.cc>
To: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
Subject: Re: Surround speaker connection on Acer 8951G
Date: Sun, 1 Sep 2019 21:27:37 +0200	[thread overview]
Message-ID: <20190901192737.GB28125@xn--80adja5bqm.su> (raw)
In-Reply-To: <s5hzhjqzvu5.wl-tiwai@suse.de>

Hi Takashi,

On Fri, Aug 30, 2019 at 02:22:42PM +0200, Takashi Iwai wrote:
> > >From various examples which I found when searching for answers I could see,
> > that in most cases the desired _verb section of the profile is activated
> > via a udev rule, but I was not able to find anything that would be
> > hooked to jack sense to switch between devices? 
> 
> UCM profile is read directly PulseAudio.  If a profile matches, PA
> uses the UCM profile instead of its mixer own parser.

That was indeed the "missing link", I was searching for ALSA and UCM all the
time, but did not think about PulseAudio at all. I was surprised to learn
that PulseAudio parses the UCM profiles directly and that alsaucm is barely
being used. It also seems that PulseAudio supports more configuration strings
than the ones that the alsaucm man page refers to (parser.c).

> With UCM profile, basically it's "out-of-the-box" for the normal use
> case.

I kind of got it to work with UCM. The drawback is, that I will have to manually
define all those profiles that PulseAudio generates by default and also make
sure to add mic and line-out jacks properly, which are currently being ignored
by Pulse, because I did not list them in my UCM conf.

> Other than that, the approach with UCM profile would be simpler.  It
> won't need anything else than what you already showed (pincfg and
> GPIO) in the driver side.

Simpler...yes and no :) From what I have seen, all "default" Pulse profiles are
replaced by the UCM, meaning that if I wanted them, I'd have to replicate
all of them in my conf. It would work though. 

> Forget about the whole auto-mute inside the driver.  This won't work
> without the intrusive changes.

I will paste some code with a simple idea that I had at the end of the
mail, but of course my view may be very limited since I am pretty new to this,
and I may not be seeing the bigger picture. I tried to make it a model quirk 
only solution, avoiding any changes elsewhere in the driver.

> The only remaining question is whether the specific Jack control
> notifies via ALSA control API properly.  You can run like
>   % /usr/sbin/alsactl monitor
> and see which control gets notified when you plug the headhpone jack.
> Just give which jack control corresponds to the actual headphone
> jack.  Then it can be put into the UCM profile.

Jack notifications work properly, I figured it out recently and they work
with my UCM profile as well. My earlier questions were related to jack
detection inside the kernel driver, I simply did not know what to hook up
to and I have incorrectly chosen the automute hook which only added to
confusion. I got this part working now, more on that further down.

> > > > {0x01, AC_VERB_SET_GPIO_DIRECTION, 0x02}
> > > 
> > > Actually this must be paired with the corresponding bit of GPIO_DATA,
> > > too.  Is the bit 0x02 of GPIO_DATA set or cleared?  Usually setting it
> > > turns on the amp, but sometimes inverted.
> > 
> > If I understood everything correctly, then the bit is set, meaning that the
> > GPIO signal is configured as output.
> 
> The GPIO_DIRECTION specifies whether input or output, and the actual
> value is passed via GPIO_DATA.  Both have to be specified, not only
> one.

[...]

> > set(0x01, 0x717,   0x02) # 0x01071702 (SET_GPIO_DIRECTION)
> 
> This needs the paired SET_GPIO_DATA for a proper operation.  Your
> analysis implicit assumes some default value that is already set by
> the hardware.

If I understand it correctly, then "some value" is zero on my hardware:

# hda-verb /dev/snd/hwC0D0 0x01 GET_GPIO_DATA 0x02
nid = 0x1, verb = 0xf15, param = 0x2
value = 0x0

Meanwhile I also figured out that /proc/asound/card0/codec#0 is
providing this info as well:

  IO[1]: enable=0, dir=1, wake=0, sticky=0, data=0, unsol=0

So the value seems to be 0 and I can add an explicit SET_GPIO_DATA verb quirk
to set it in addition to SET_GPIO_DIRECTION, right?

> > If the above is a yes, could you please point me to the right way of getting 
> > triggered upon jack state changes in the driver, since automute hooks seem 
> > not to trigger while something is playing?
> 
> This isn't easy, as repeatedly mentioned, since the parser assumes
> only one role for each pin except for the input/output switching.
> In your case the pin shares for two outputs to be switched, and this
> isn't implemented at all.

I think we talked a bit past each other, based on your reply I understand that 
you were assuming that I am still trying to solve it in the parser in some 
generic way, which as you indeed repeatedly mentioned would not be easy.

At the same time I do not rule out the possibility, that I simply do not
see the "bigger picture" and that you have already accounted for the
proposal that follows :)

Given that this is my first driver hacking adventure, I was aiming for a much
more localized solution, so I was not going to add generic support for
shared pins. Once I learned how to subscribe to jack state
changes I was able to implement the idea that I had inside the model
quirk:

static void alc662_aspire_ethos_mute_speakers(struct hda_codec *codec,
                    struct hda_jack_callback *cb)
{
    /* surround speakers muted automatically when headphones are
     * plugged in, we'll mute/unmute the remaining channels manually:
     * 0x15 - front left/front right
     * 0x18 - front center/ LFE
     * */
    if (snd_hda_jack_detect_state(codec, cb->nid) == HDA_JACK_PRESENT) {
        snd_hda_set_pin_ctl_cache(codec, 0x15, 0);
        snd_hda_set_pin_ctl_cache(codec, 0x18, 0);
    } else {
        snd_hda_set_pin_ctl_cache(codec, 0x15, PIN_OUT);
        snd_hda_set_pin_ctl_cache(codec, 0x18, PIN_OUT);
    }
}

static void alc662_fixup_aspire_ethos_hp(struct hda_codec *codec,
                     const struct hda_fixup *fix, int action)
{
    if (action == HDA_FIXUP_ACT_PRE_PROBE) {
        if (is_jack_detectable(codec, 0x1b)) {
            snd_hda_jack_detect_enable_callback(codec, 0x1b,
                                    alc662_aspire_ethos_mute_speakers);
        }
    }
}

This was the idea that I have been referring to, when I was talking about
"muting in the driver" after I learned that proper automuting based on hp pins
was not possible as you indeed repeatedly stated.

The above seems to work quite well for me and does exactly what I want, 
PulseAudio presents all the autogenerated profiles and handles mic and line 
jacks itself, at the same time all unwanted speakers get muted as soon as I 
plug in my headphones into the jack pin that is shared with my surround
speakers. Of course Pulse does not "know" anything about the headphones and
does not switch profiles, but I don't mind since the user experience is
as expected.

My earlier attempt was to send the pin widget control verbs directly, however
then the pin got reconnected as soon as playback started.
This does not happen when I use snd_hda_set_pin_ctl_cache(), but I am not 
quite sure about the cache, should I use the _cache function or the
uncached one?

Another thing I am not sure about is, if I somehow disrupt power management by 
doing what I do? I saw that for instance restore_shutup_pins() does modify 
those connections as well and I would basically overwrite whatever it did
in the case that the user plugs/unplugs the headphones.

Does the above code snippet look like a direction worth exploring, or is
this "secretly-muting" idea not acceptable to you, meaning that I should really 
stop right here and live with UCM as the only solution?

Kind regards,
Jin