[alsa-devel] Major reworks on patch_via.c (TESTERS WANTED)
alex dot baldacchino dot alsasub at gmail dot com
alex.baldacchino.alsasub at gmail.com
Sat Jul 2 18:00:27 CEST 2011
2011/6/22 Takashi Iwai <tiwai at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: alsa-info.tar.gz
Type: application/x-gzip
Size: 13898 bytes
Desc: not available
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20110702/d7b27471/attachment-0001.gz
More information about the Alsa-devel
mailing list