Re: Surround speaker connection on Acer 8951G

[Takashi Iwai <tiwai@suse.de>]

On Sun, 01 Sep 2019 21:27:37 +0200,
Sergey 'Jin' Bostandzhyan wrote:
> 
> > 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. 

You just need to override codec->card->longname to some unique string
and use it as UCM profile name.
Check alc1220_fixup_gb_dual_codecs() as an example.

> > > > > {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?

Yes.  You need to set SET_GPIO_MASK=0x02, SET_GPIO_DIRECTION=0x02 and
SET_GPIO_DATA=0x00 for that bit.

> > > 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.

Hm, OK, this amount of changes are acceptable.  The hardware behavior
itself is weird, and we have already tricky code, so it's no problem
to keep some yet another tricky code as long as it's described enough
in the comments and the changelog.

> 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?

This should work, AFAIK.  The *_set_pin_ctl_cache() remembers the last
written value, as its name stands.  That's restored again at the PM
resume, for example.

The PM resume does re-trigger the jack detection callback, so it'll be
written up again in anyway, though.

> 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.

This should be fine as-is.  The shutup_pins() clears pins temporarily
and the pins are resumed to the cached values in return.

One thing to be improved would be to drop the surround jack control.
Adjust the pin config to different value with the fixed pin
connection, so that the auto-parser won't create the "Surround Jack"
control.  This isn't needed by PA or else, otherwise it may be
confusing.


thanks,

Takashi