[alsa-devel] Via VT2020: issues with kernel 2.6.38.{2, 3} (alsa 1.0.23) - working with 2.6.33.2 (alsa 1.0.21)

[alex dot baldacchino dot alsasub at gmail dot com]

2011/6/18 Raymond Yau <superquad.vortex2 at gmail.com>:
> 2011/6/8 alex dot baldacchino dot alsasub at gmail dot com
> <alex.baldacchino.alsasub at gmail.com>:
>>
>>
>> Adding a few printk()'s to file "patch_via.c" showed me function
>> via_hp_build() is called before via_build_pcms() (unless front panel
>> is disabled in BIOS, in which case via_hp_build() isn't invoked at
>> all), therefore, when a control "Independent HP" is created, it should
>> be 'visible' by via_build_pcms(); however,
>> snd_hda_find_mixer_ctl(codec, "Independent HP") fails to find it until
>> the following loop is executed inside via_build_controls:
>>
>
>> and via_build_controls() is called after via_build_pcms(), but then
>> it's too late (or seemed to be such) to modify the number of playback
>> substreams. At last, I've tried the following, which worked:
>>
>
> The fix is to check spec->multiout.hp_nid  since "Independent HP" need
> the extra DAC for HP when there is no HP if front panel is disabled by
> BIOS
>
>
>
> static int via_build_pcms(struct hda_codec *codec)
> {
> ...
>
>        info->name = spec->stream_name_analog;
>        info->stream[SNDRV_PCM_STREAM_PLAYBACK] =
>                *(spec->stream_analog_playback);
>        info->stream[SNDRV_PCM_STREAM_PLAYBACK].nid =
>                spec->multiout.dac_nids[0];
> +     info->stream[SNDRV_PCM_STREAM_PLAYBACK].substreams =
> +             spec->multiout.hp_nid ? 2 : 1;
>        info->stream[SNDRV_PCM_STREAM_CAPTURE] = *(spec->stream_analog_capture);
>        info->stream[SNDRV_PCM_STREAM_CAPTURE].nid = spec->adc_nids[0];
>
>        info->stream[SNDRV_PCM_STREAM_PLAYBACK].channels_max =
>                spec->multiout.max_channels;
>
>

It might work, but might also fail, I guess. Even if everything else
went ok during initialization, and multiout.hp_nid were set in
vtxxxx_auto_create_hp_ctls(), via_hp_build() could fail to create an
Independent HP control, because of either a failure allocating memory
for it, when calling via_clone_control():


	knew = via_clone_control(spec, &via_hp_mixer[0]);
	if (knew == NULL)
		return -ENOMEM;

or because the nid corresponding to the Independent HP control hasn't
got enough connections:


	if (spec->codec_type != VT1708) {
		nums = snd_hda_get_connections(codec, nid,
					       conn, HDA_MAX_CONNECTIONS);
		if (nums <= 1)
			return 0;
	}


As I understand it, without a control "Independent HP" with its proper
.get and .put methods, that's not possible to switch between single-
and multi-streaming, thus the problem is here, I think.

But perhaps, if there are not enough connections to switch from/to, it
might happen that autocfg.hp_pins[0] is 0, so that:


static int vtXXXX_parse_auto_config(struct hda_codec *codec)
{
...

      err = vtXXX_auto_create_hp_ctls(spec, spec->autocfg.hp_pins[0]);
...


triggered:


static int vtXXX_auto_create_hp_ctls(struct via_spec *spec, hda_nid_t pin)
{
...

	if (!pin)
		return 0;
...


before spec->multiout.hp_nid is set, but it may also not happen,
because, regardless of support for multi-streaming, there can be
support for a front panel line-out in redirected output (refer to
commit 3d83e577a8206f0f3822a3840e12f76477142ba2 and
ee3c35c0827de02de414d08b2ddcbb910c2263ab, if there is need to solve
that problem in via_hp_build(), it should mean that
spec->multiout.hp_nid is not null in such scenarios).

However, that wouldn't solve the memory failure risk, or the risk
via_hp_build is not called at all because spec->hp_mux wasn't created
(though, this latter risk is fixable by moving assignment to
spec->multiout.hp_nid at the end of vtXXX_auto_create_hp_ctls, and
using local variables before), and anyway, in doubt, I think a safe
choice is to add a proper field, I've called
independent_front_panel_out, to struct via_spec, initialize it to 0
and set it to 1 in via_hp_build() after via_clone_control(spec,
&via_hp_mixer[0]) succeeds, so that in via_build_pcms():


info->stream[SNDRV_PCM_STREAM_PLAYBACK].substreams = 1 +
spec->independent_front_panel_out;


I was using it also in patch_vtXXXX() (for a few codecs, not all) to
create a "Front Mic Boost Volume" control dynamically only when the
front panel exist (but that's wrong, I've modified it to check for
spec->autocfg.hp_pins[0], which seems to be 0 when there's no front
panel seen by the BIOS).

More code is in my last mail for this thread, before this one, I've
changed since then:

front_panel_out renamed independent_front_panel_out, and
vt_add_front_mic_boost() now is:


static int vt_add_front_mic_boost( struct via_spec *spec, const struct
snd_kcontrol_new *fmb_ctl ){
	struct snd_kcontrol_new *knew;

	if( spec->autocfg.hp_pins[0] ){ /* if there is a front audio panel */
		knew = via_clone_control( spec, fmb_ctl );
		if( knew == NULL )
			return -ENOMEM;
		return 1;
	}
	
	return 0;
}


(here, it would be the same to check for spec->multiout.hp_nid being
set, but only if the position of the assignment to
spec->multiout.hp_nid is not moved around in
vtxxx_auto_create_hp_ctls)


> However there is a bug in using subdevice 1 for "Independent HP" and
> subdevice 0 for multi-channels
>
> The channel count reported by snd_pcm_hw_params_get_channels_max() or
> snd_pcm_hw_params_test_channel() for subdevice 1 is also equal to
> spec->multiout.max_channels but this subdevice 1 only support stereo


I haven't understood all of the pcm layer, but I guess that issue
could be hard to fix without touching it at some level, or creating a
separate pcm (struct snd_pcm) to handle multi-outs and hp separately,
with a careful inspection of the code needed to handle them in tandem
(via_playback_pcm_open() and so on).

struct hda_pcm can hold only 2 struct hda_pcm_stream elements (one
actually used for playback, the other for capture):

/* for PCM creation */
struct hda_pcm {
	char *name;
	struct hda_pcm_stream stream[2];
...

and at least snd_hda_attach_pcm() (hda_codec.c) counts on it:

static int snd_hda_attach_pcm(struct hda_codec *codec, struct hda_pcm *pcm)
{
	struct hda_bus *bus = codec->bus;
	struct hda_pcm_stream *info;
	int stream, err;

	if (snd_BUG_ON(!pcm->name))
		return -EINVAL;
	for (stream = 0; stream < 2; stream++) {
		info = &pcm->stream[stream];
		if (info->substreams) {
			err = set_pcm_default_values(codec, info);
			if (err < 0)
				return err;
		}
	}
	return bus->ops.attach_pcm(bus, codec, pcm);
}


Moreover, only one pcm is created for a VIA codec, to handle analog
outputs, plus one if there are digital connections:

(file patch_via.c)

static int via_build_pcms(struct hda_codec *codec)
{
	struct via_spec *spec = codec->spec;
	struct hda_pcm *info = spec->pcm_rec;

	codec->num_pcms = 1;
	codec->pcm_info = info;
....

	if (spec->multiout.dig_out_nid || spec->dig_in_nid) {
		codec->num_pcms++;
		info++;
...


(file hda_codec.c)

int snd_hda_codec_build_pcms(struct hda_codec *codec)
{
	unsigned int pcm;
	int err;
...

	for (pcm = 0; pcm < codec->num_pcms; pcm++) {
		struct hda_pcm *cpcm = &codec->pcm_info[pcm];
		int dev;
...

		if (!cpcm->pcm) {
			dev = get_empty_pcm_device(codec->bus, cpcm->pcm_type);
			if (dev < 0)
				continue; /* no fatal error */
			cpcm->device = dev;
			err = snd_hda_attach_pcm(codec, cpcm);
...


snd_hda_attach_pcm() calls bus->ops.attach_pcm(), that is (in file
hda_intel.c) azx_attach_pcm_stream():


static int
azx_attach_pcm_stream(struct hda_bus *bus, struct hda_codec *codec,
		      struct hda_pcm *cpcm)
{
	struct azx *chip = bus->private_data;
	struct snd_pcm *pcm;
	struct azx_pcm *apcm;
	int pcm_dev = cpcm->device;
	int s, err;
...

	err = snd_pcm_new(chip->card, cpcm->name, pcm_dev,
			  cpcm->stream[SNDRV_PCM_STREAM_PLAYBACK].substreams,
			  cpcm->stream[SNDRV_PCM_STREAM_CAPTURE].substreams,
			  &pcm);
...

	apcm->chip = chip;
	apcm->codec = codec;
	pcm->private_data = apcm;
...

	cpcm->pcm = pcm;
	for (s = 0; s < 2; s++) {
		apcm->hinfo[s] = &cpcm->stream[s];
		if (cpcm->stream[s].substreams)
			snd_pcm_set_ops(pcm, s, &azx_pcm_ops);
	}
...


again counting on two streams only:

(hda_intel.c):

struct azx_pcm {
	struct azx *chip;
	struct hda_codec *codec;
	struct hda_pcm_stream *hinfo[2];
};


(pcm.h):

struct snd_pcm {
...
	struct snd_pcm_str streams[2];
...
	void *private_data;
...


struct snd_pcm_str {
	int stream;				/* stream (direction) */
	struct snd_pcm *pcm;
	/* -- substreams -- */
	unsigned int substream_count;
	unsigned int substream_opened;
	struct snd_pcm_substream *substream;
...


struct snd_pcm_substream {
	struct snd_pcm *pcm;
	struct snd_pcm_str *pstr;
	void *private_data;		/* copied from pcm->private_data */
...

	/* -- hardware operations -- */
	struct snd_pcm_ops *ops;
	/* -- runtime information -- */
	struct snd_pcm_runtime *runtime;

(notice above comment about field private_data, from file)


struct snd_pcm_runtime {
...
	/* -- hardware description -- */
	struct snd_pcm_hardware hw;
	struct snd_pcm_hw_constraints hw_constraints;
...


struct snd_pcm_hardware {
...
	unsigned int channels_min;	/* min channels */
	unsigned int channels_max;	/* max channels */
...

(pcm.c):

int snd_pcm_new(struct snd_card *card, const char *id, int device,
		int playback_count, int capture_count,
	        struct snd_pcm ** rpcm)
{
	struct snd_pcm *pcm;
	int err;
...

	if ((err = snd_pcm_new_stream(pcm, SNDRV_PCM_STREAM_PLAYBACK,
playback_count)) < 0) {
...
	if ((err = snd_pcm_new_stream(pcm, SNDRV_PCM_STREAM_CAPTURE,
capture_count)) < 0) {
...


int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
{
	int idx, err;
	struct snd_pcm_str *pstr = &pcm->streams[stream];
	struct snd_pcm_substream *substream, *prev;
...

	pstr->stream = stream;
	pstr->pcm = pcm;
	pstr->substream_count = substream_count;
...
	for (idx = 0, prev = NULL; idx < substream_count; idx++) {
		substream = kzalloc(sizeof(*substream), GFP_KERNEL);
...

		substream->pcm = pcm;
		substream->pstr = pstr;
		substream->number = idx;
		substream->stream = stream;
....


int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
			     struct file *file,
			     struct snd_pcm_substream **rsubstream)
{
	struct snd_pcm_str * pstr;
	struct snd_pcm_substream *substream;
	struct snd_pcm_runtime *runtime;
...

	runtime = kzalloc(sizeof(*runtime), GFP_KERNEL);
...

	substream->runtime = runtime;
	substream->private_data = pcm->private_data;
...
	*rsubstream = substream;
	return 0;
}



This one called by: (pcm_native.c):


int snd_pcm_open_substream(struct snd_pcm *pcm, int stream,
			   struct file *file,
			   struct snd_pcm_substream **rsubstream)
{
	struct snd_pcm_substream *substream;
	int err;

	err = snd_pcm_attach_substream(pcm, stream, file, &substream);
...

	if ((err = substream->ops->open(substream)) < 0)
		goto error;

	substream->hw_opened = 1;
...

	*rsubstream = substream;
...


called by snd_pcm_open_file(), in turn called by snd_pcm_open(), in
turn called by snd_pcm_{playback, capture}_open(), which are
'exported' as file_operations; substream->ops->open(), as set by
azx_attach_pcm_stream(), is:

(hda_intel.c):

static int azx_pcm_open(struct snd_pcm_substream *substream)
{
	struct azx_pcm *apcm = snd_pcm_substream_chip(substream);
	struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream];
	struct azx *chip = apcm->chip;
	struct azx_dev *azx_dev;
	struct snd_pcm_runtime *runtime = substream->runtime;
...

	runtime->hw.channels_min = hinfo->channels_min;
	runtime->hw.channels_max = hinfo->channels_max;
...


In file pcm.h:

#define snd_pcm_substream_chip(substream) ((substream)->private_data)

since substream->private_data is shared by all substreams of the same
snd_pcm, in case of playback (substream->stream ==
SNDRV_PCM_STREAM_PLAYBACK), hinfo is spec->stream_analog_playback, and
.channel_{min, max} fields hold same values for both multi-out and
Independent HP subdevices (if I'm not mistaken, of course).

If my interpretation above is correct, something should change in that
chain in order to have different values retrieved for different
substreams of the same stream of the same pcm.

On the other hand, using separate pcms could require less invasive,
yet careful, changes (affecting only patch_via.c, or one patch at a
time, to test it better), but could affect any application eventually
counting on analog outputs being handled by the same pcm.

> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>