2011/6/22 Takashi Iwai tiwai@suse.de:
At Mon, 20 Jun 2011 17:06:51 +0200, Takashi Iwai wrote:
Hi,
as there have many problems reported for VIA codecs, I started looking at the driver code, and decided to rework on it. [...]
I've installed and tested (from tarballs) both snapshot alsa-driver-20110630 and latest version (alsa-driver-snapshot.tar.gz) - up to commit e5e14681404ec27a422d635284bf564dabde3f81 by Lydia Wang, I think.
I've found problems with both. That's a quite huge change in the implementation from older code, so I need to study it more in depth to understand it better... At the moment I can only describe the problems and give my first impressions on them.
First of all, I can't get sound from the front playback (rear line-out). It would seem there's no problem opening the pcm and setting the stream; If I switch Independent HP mode off, I can listen to anything from my front audio panel (though with the old implementation I could switch it on and off repeatedly and ensure that sound came only from front dac (0x8), now the imux is 'blocked' as busy while playing and it would seem the same stream is sent to hp dac as well, even if its corresponding audio selector switches to front dac - 0x8 - so hp shouldn't get sound by 0xb when in redirected mode; perhaps I should add a few printk calls to snd_hda_multi_out_analog_prepare() or snd_hda_codec_setup_stream() to see what happens for nid 0x8, though I think it works, and maybe I can confirm it indirectly).
Could there be the need to update cached values somewhere in the code? Perhaps where certain nids are un-muted? I had a similar problem (with the old implementation) while trying to create a pair or input muxs to enable/disable the AA-path and avoid getting noisy feedback from my headset mic (such as hearing my breath). For my codec (and a few others - AFAICT, all flavours of VT1718S, all flavours of VT2002P and VT1812) it was a matter of muting/un-muting the input from 0x21 (stereo mixer) to each mixer widget sending output to my green jacks; however, trying to send a direct verb to those widgets I got some problems I solved by calling snd_hda_codec_amp_stereo(), which updates cached values as well (I had sent a mail with code slices to the list, but it might have gone lost, because I cannot find a link in the archives, just in case anyone wished to give it a look). Is there a chance that a few calls to snd_hda_codec_write are not enough alone, in this new implementation of the driver? Though it should be relevant only if retrieving correct values for volumes is important for correct handling of a stream setup (that is, if any decision is took basing on results of a read operation on volumes), otherwise there could be some error in nids' path parsing.
Second: Independent HP mode doesn't work as well - tested with aplay -Dhw:0,2,0, since now a separate pcm device is created for headphones. However, this is easy to fix, since it's just a matter of nid mismatching in set_widgets_power_state_vt1718S():
...
if (spec->hp_independent_mode) { /* PW4 (28h), MW3 (1bh), MUX1(34h), AOW4 (ch) */ parm = AC_PWRST_D3; set_pin_power_state(codec, 0x28, &parm); snd_hda_codec_write(codec, 0x1b, 0, AC_VERB_SET_POWER_STATE, parm); snd_hda_codec_write(codec, 0x34, 0, AC_VERB_SET_POWER_STATE, parm); - snd_hda_codec_write(codec, 0xc, 0, + snd_hda_codec_write(codec, spec->hp_dac_nid, 0, AC_VERB_SET_POWER_STATE, parm); } }
By the way, in old code I had made a few changes to this function basing on following considerations - which could no more apply, so I'm posting the code below just to compare notes:
- this codec (family) has two adc's that seem to be 'almost independent', meaning AIW1 (nid 0x11, coupled with MUX7, nid 0x1f), if not in use, could be in state D3 while AIW0 (nid 0x10, coupled with MUX6, nid 0x1e) is in D0, whereas the opposite should be avoided, since mic boost controls and digital (mic) seem to be bound to AIW0; moreover, MUX7 could be connected to stereo mixer, and this should be taken into account when setting state for 0x21;
- when in redirected mode, hp Pin Widget (nid 0x28) should get input only from front dac (nid 0x8) as selected by Audio Selector 0x34, thus hp dac (0x0c or 0x0b) could be set in D3, and, if nothing is connected to it (to 0x28) both Mixer 0x1b and MUX 0x34 could be set in D3 independently from state of pin 0x24 (front).
static void set_widgets_power_state_vt1718S(struct hda_codec *codec) { struct via_spec *spec = codec->spec; unsigned int imux_is_smixer[ 2 ]; unsigned int parm, parm_mux[ 2 ] = { AC_PWRST_D3, AC_PWRST_D3 }; unsigned int imux_conn[2];
/* get connection for MUX 6/7 (1eh/1fh) */ imux_conn[ 0 ] = snd_hda_codec_read(codec, 0x1e, 0, AC_VERB_GET_CONNECT_SEL, 0x00); imux_conn[ 1 ] = snd_hda_codec_read(codec, 0x1f, 0, AC_VERB_GET_CONNECT_SEL, 0x00); /* MUX 6/7 (1eh/1fh) = stereo mixer ? */ imux_is_smixer[ 0 ] = ( imux_conn[ 0 ] == 5 ); imux_is_smixer[ 1 ] = ( imux_conn[ 1 ] == 5 );
/* inputs */ /* PW 5/6/7 (29h/2ah/2bh) */ parm = AC_PWRST_D3; set_pin_power_state(codec, 0x29, &parm); if( imux_conn[ 0 ] == 3 ) /* MUX6 (1eh) connected to 0x29 */ parm_mux[ 0 ] = parm; if( imux_conn[ 1 ] == 3 ) /* MUX7 (1fh) connected to 0x29 */ parm_mux[ 1 ] = parm;
parm = AC_PWRST_D3; set_pin_power_state(codec, 0x2a, &parm); if( imux_conn[ 0 ] == 2 ) /* MUX6 (1eh) connected to 0x2a */ parm_mux[ 0 ] = parm; if( imux_conn[ 1 ] == 2 ) /* MUX7 (1fh) connected to 0x2a */ parm_mux[ 1 ] = parm;
parm = AC_PWRST_D3; set_pin_power_state(codec, 0x2b, &parm); if( imux_conn[ 0 ] == 1 ) /* MUX6 (1eh) connected to 0x2b */ parm_mux[ 0 ] = parm; if( imux_conn[ 1 ] == 1 ) /* MUX7 (1fh) connected to 0x2b */ parm_mux[ 1 ] = parm;
/* MUX6 (1eh) = stereo mixer */ if( imux_is_smixer[ 0 ] ) parm_mux[ 0 ] = AC_PWRST_D0; /* MUX7 (1fh) = stereo mixer */ if( imux_is_smixer[ 1 ] ) parm_mux[ 1 ] = AC_PWRST_D0; /* this codec has switches for front/rear boost captures and digital mic binded to AIW0 (10h) */ /* therefore, if those are needed when using stuff connected to MUX7 (1fh), MUX6 must be on*/ if( parm_mux[ 1 ] == AC_PWRST_D0 ) parm_mux[ 0 ] = parm_mux[ 1 ];
/* MUX6/7 (1eh/1fh), AIW 0/1 (10h/11h) */ snd_hda_codec_write( codec, 0x1e, 0, AC_VERB_SET_POWER_STATE, parm_mux[ 0 ] ); snd_hda_codec_write( codec, 0x10, 0, AC_VERB_SET_POWER_STATE, parm_mux[ 0 ] ); snd_hda_codec_write( codec, 0x1f, 0, AC_VERB_SET_POWER_STATE, parm_mux[ 1 ] ); snd_hda_codec_write( codec, 0x11, 0, AC_VERB_SET_POWER_STATE, parm_mux[ 1 ] );
/* outputs */ /* PW3 (27h), MW2 (1ah), AOW3 (bh) */ parm = AC_PWRST_D3; set_pin_power_state(codec, 0x27, &parm); snd_hda_codec_write(codec, 0x1a, 0, AC_VERB_SET_POWER_STATE, parm); snd_hda_codec_write(codec, 0xb, 0, AC_VERB_SET_POWER_STATE, parm);
/* PW2 (26h), AOW2 (ah) */ parm = AC_PWRST_D3; set_pin_power_state(codec, 0x26, &parm); if (spec->smart51_enabled) set_pin_power_state(codec, 0x2b, &parm); snd_hda_codec_write(codec, 0xa, 0, AC_VERB_SET_POWER_STATE, parm);
/* PW0 (24h), AOW0 (8h) */ parm = AC_PWRST_D3; set_pin_power_state(codec, 0x24, &parm); if (!spec->hp_independent_mode){ /* check for redirected HP */ unsigned int parm_hp = AC_PWRST_D3; set_pin_power_state( codec, 0x28, &parm_hp ); snd_hda_codec_write( codec, 0x1b, 0, AC_VERB_SET_POWER_STATE, parm_hp ); snd_hda_codec_write( codec, 0x34, 0, AC_VERB_SET_POWER_STATE, parm_hp ); /* in redirected mode, hp_nid isn't in use */ snd_hda_codec_write( codec, spec->multiout.hp_nid, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D3 ); if( parm_hp == AC_PWRST_D0 ) parm = AC_PWRST_D0; } snd_hda_codec_write(codec, 0x8, 0, AC_VERB_SET_POWER_STATE, parm);
/* MW9 (21h), Mw2 (1ah), AOW0 (8h) */ /* don't understand above comment - may guess dependence on front dac, but am unsure... and can't see any dependence on MW 1ah */ snd_hda_codec_write( codec, 0x21, 0, AC_VERB_SET_POWER_STATE, imux_is_smixer[ 0 ] || imux_is_mixer[ 1 ] ? AC_PWRST_D0 : parm );
/* PW1 (25h), AOW1 (9h) */ parm = AC_PWRST_D3; set_pin_power_state(codec, 0x25, &parm); if (spec->smart51_enabled) set_pin_power_state(codec, 0x2a, &parm); snd_hda_codec_write(codec, 0x9, 0, AC_VERB_SET_POWER_STATE, parm);
if (spec->hp_independent_mode) { /* PW4 (28h), MW3 (1bh), MUX1(34h), AOW4 (ch) */ parm = AC_PWRST_D3; set_pin_power_state(codec, 0x28, &parm); snd_hda_codec_write(codec, 0x1b, 0, AC_VERB_SET_POWER_STATE, parm); snd_hda_codec_write(codec, 0x34, 0, AC_VERB_SET_POWER_STATE, parm); snd_hda_codec_write(codec, spec->multiout.hp_nid, 0, AC_VERB_SET_POWER_STATE, parm); } }
(if applicable to actual implementation, any occurrence of spec->multiout.hp_nid should be replaced with spec->hp_dac_nid - the above doesn't yet take care of the possible re-tasking of line-in as side, which I didn't try)
----
But let's come back to current state. There could be an initialization verb missing from array vt1718S_init_verbs and present in old vt1718S_volume_init_verbs, which is "almost" a vendor-specific verb, to un-mute the apparently non-existent (not seen by the driver) connection at index #5 for stereo mixer (0x21):
static const struct hda_verb vt1718S_init_verbs[] = { /* Enable MW0 adjust Gain 5 */ {0x1, 0xfb2, 0x10}, /* Enable Boost Volume backdoor */ {0x1, 0xf88, 0x8}, + /* Unmute front dac connection to stereo mixer */ + {0x21, AC_VERB_SET_AMP_GAIN_MUTE, AMP_IN_UNMUTE(5)},
{ } };
If I remember well what Lydia Wang told me in another thread, 0x21 is connected to a dac (should be the front dac), even if the connection isn't visible through the driver. Without that verb, if I set 'stereo mixer' as first input source and try to record some playback, I get only noise, with such initialization I can record an ongoing playback (eventually mixed with other sound, e.g. from my mic). Such a result tells me nid 0x8 (front dac, the only involved in my recording test) should work properly even if no sound gets to front line-out pin (0x24). A side effect of this, however, is that, depending on volumes settings, whatever widget getting input also by 0x21 (and if such connection isn't muted), will get sound by 0x8 while playing - e.g. I can hear, at a lower volume, a playback setup for the front dac from my headset even when Independent HP is set to ON; on the other hand, lowering volumes attenuates such, but playback may become unrecordable (due to volume being too low).
I'm attaching alsa-info.sh output for both the original version of this new implementation (latest version from tarballs, described at the beginning) and the modified version I'm using to both make Independent HP work and activate that "hidden" connection for stereo mixer. Though, it looks like they don't tell much about the problem with front audio from front pin (I've compiled drivers with verbose debug).
One notably thing is that the new (for codec VT1718S family) Master control misses a few slaves, mainly:
- as expectable, there's neither a "Side Playback Volume" nor a "Side Playback Switch", since there's no grey jack (to support 7.1 systems, my motherboard line-in (0x2a) should be re-tasked as output and get input from the now available 0xc, which is no more used by headphones)
- more noticeably, there's no "Headphone Playback Volume" control. There was one in old implementation, explicitly created by vt1718S_auto_create_hp_ctls(); within that function it was easy to bind that control to the actual hp_nid (replacing any occurrence of 0xc with spec->multiout.hp_nid in all relevant functions, the switch from 0xc to 0xb was complete), given that the only reason to test usability of dac at 0xb for headphone was, initially, to check whether 0xc could be freed for use with 0x2a as side (and ensure, or confirm, this codec family can handle as much as 10 channels - but to confirm it definitely an attempt to retask 0x2a should be made).
Moreover, looking at codec informations, now control "Independent HP" is attached to hp pin, altogether with control "Headphone Playback Switch". This should be somehow more consistent with those codecs not using an intermidiate selector to choose the nid to get sound from (perhaps the majority), so that such control always sticks in same (or corresponding) places. However, this way there is more 'distance' between such control and the 'real' nid it works on.
A final (minor) question/consideration on the code. I've noticed now .put helpers validate data gathered from userspace (typically, passed through ucontrol parameter), which is a good choice to protect against some bug external to the driver; however, if I'm not mistaken, via_mux_enum_put (and corresponding _get) could still be missing something: since any data in ucontrol->id is passed as is to snd_ctl_get_ioffidx, is there any chance that the result could be an invalid index? If that's possible, perhaps such value should be validated as well, comparing it to the max number of mux nids, eventually defined in a constant for easier maintenance, such as:
#define VIA_NUM_MUXS 3
then, in struct via_spec:
- hda_nid_t mux_nids[3]; + hda_nid_t mux_nids[ VIA_NUM_MUXS ]; hda_nid_t aa_mix_nid;
...
struct via_input inputs[AUTO_CFG_MAX_INS + 1]; - unsigned int cur_mux[3]; + unsigned int cur_mux[ VIA_NUM_MUXS ];
then, in via_mux_enum_put() (same for via_mux_enum_get)
either:
if( idx >= VIA_NUM_MUXS ) return -EINVAL;
or:
if( idx >= VIA_NUM_MUXS ) /* recovering from wrong value, assuming the last valid entry was the real target */ idx = VIA_NUM_MUXS -1;
(more applicable for via_mux_enum_get)
The latter strategy suggested by analogy with similar implementation of data correctness control in snd_hda_input_mux_put (previously used) and by comparison with other recovery attempts made in .put helpers, such as double negation of ucontrol->value.enumerated.item[0] in via_independent_hp_put and the single negation in via_pin_power_ctl_{get, put} to always get one of the two only possible values in that contexts (0 or 1). Perhaps the same should be done in via_smart51_put() as well, to 'normalize' *ucontrol->value.integer.value as boolean before assigning it to spec->smart51_enabled, maybe returning immediately if old and new values match against each other (as done in via_independent_hp_put and via_pin_power_ctl_put) -- Possibly, the same might apply to vt1716s_dmic_{get,put} helpers, but only if nid 0x26 had just two connections (I was considering it for the old code too, but I haven't been able to find any info about VT1716S).
Regards,
Alex