From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anssi Hannula Subject: Re: [PATCH] ALSA: hda - Fix the workaround for conflicting IEC958 controls Date: Sun, 10 Feb 2013 13:05:14 +0200 Message-ID: <51177EEA.7080704@iki.fi> References: <1360363479-15678-1-git-send-email-anssi.hannula@iki.fi> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-gw-out2.cc.tut.fi (mail-gw-out2.cc.tut.fi [130.230.160.33]) by alsa0.perex.cz (Postfix) with ESMTP id 0EB822615D1 for ; Sun, 10 Feb 2013 12:05:15 +0100 (CET) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 10.02.2013 12:38, Takashi Iwai kirjoitti: > At Sat, 9 Feb 2013 00:44:39 +0200, > Anssi Hannula wrote: >> >> Commit dcda5806165c155d90b9aa466a1602cf4726012b ("ALSA: hda - Add >> workaround for conflicting IEC958 controls") added a workaround for >> cards that have both an S/PDIF and an HDMI device, so that S/PDIF IEC958 >> controls will be moved to device=1 on such cards. >> >> However, the workaround did not take it into account that the S/PDIF and >> HDMI devices may be on different codecs of the same card. Currently this >> is always the case, and the workaround therefore fails to work. >> >> Fix the workaround to handle card-wide IEC958 conflicts. >> >> Reported-by: Stephan Raue >> Signed-off-by: Anssi Hannula >> --- >> >> Unfortunately this seems to cause a nasty issue with alsa-lib 1.0.26: >> $ amixer scontrols -c 0 >> ALSA lib simple_none.c:1551:(simple_add1) helem (MIXER,'IEC958 Playback Switch',0,1,0) appears twice or more >> amixer: Mixer hw:0 load error: Invalid argument >> >> The non-simple-mode "amixer controls -c 0" works fine, though. >> >> Not really sure what to do now then, do we revert the workaround >> completely and devise a different workaround/fix for this, or do you >> have some other good ideas? > > If the element isn't really dup'ed, it must be a bug in alsa-lib mixer > abstraction, so it should be fixed there. This is because the simple mixer interface only identifies controls by name+index: http://www.alsa-project.org/alsa-doc/alsa-lib/group___simple_mixer.html So controls that only differ by device (or subdevice?) are considered duplicated. I did look at the code but saw no straight-forward way to fix it, other than to introduce devices (and subdevices) to the simple mixer API (which is used by outside applications). Anyway, wouldn't breaking "old" alsa-lib make this way of fixing it a no-go (the error is fatal and mixer creation fails completely)? > Could you add alsa-info.sh output of this board (at best before and > after your patch)? Here's one after patch (can't get one before patch right now, but I guess it isn't needed since the cause is very clear): http://www.alsa-project.org/db/?f=bb3fc8680b372c0475719d42d7fc8b2bb7bfb4eb (this one has alsa-lib hack applied which ignores the failure to add the control so that the mixer error is non-fatal) > > thanks, > > Takashi > > >> >> >> sound/pci/hda/hda_codec.c | 27 +++++++++++++++------------ >> sound/pci/hda/hda_codec.h | 4 +++- >> 2 files changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c >> index 822df97..fe5d6fc 100644 >> --- a/sound/pci/hda/hda_codec.c >> +++ b/sound/pci/hda/hda_codec.c >> @@ -3135,25 +3135,28 @@ int snd_hda_create_dig_out_ctls(struct hda_codec *codec, >> int idx, dev = 0; >> const int spdif_pcm_dev = 1; >> struct hda_spdif_out *spdif; >> + struct hda_codec *c; >> >> - if (codec->primary_dig_out_type == HDA_PCM_TYPE_HDMI && >> + if (codec->bus->primary_dig_out_type == HDA_PCM_TYPE_HDMI && >> type == HDA_PCM_TYPE_SPDIF) { >> dev = spdif_pcm_dev; >> - } else if (codec->primary_dig_out_type == HDA_PCM_TYPE_SPDIF && >> + } else if (codec->bus->primary_dig_out_type == HDA_PCM_TYPE_SPDIF && >> type == HDA_PCM_TYPE_HDMI) { >> - for (idx = 0; idx < codec->spdif_out.used; idx++) { >> - spdif = snd_array_elem(&codec->spdif_out, idx); >> - for (dig_mix = dig_mixes; dig_mix->name; dig_mix++) { >> - kctl = find_mixer_ctl(codec, dig_mix->name, 0, idx); >> - if (!kctl) >> - break; >> - kctl->id.device = spdif_pcm_dev; >> + list_for_each_entry(c, &codec->bus->codec_list, list) { >> + for (idx = 0; idx < c->spdif_out.used; idx++) { >> + spdif = snd_array_elem(&c->spdif_out, idx); >> + for (dig_mix = dig_mixes; dig_mix->name; dig_mix++) { >> + kctl = find_mixer_ctl(c, dig_mix->name, 0, idx); >> + if (!kctl) >> + break; >> + kctl->id.device = spdif_pcm_dev; >> + } >> } >> } >> - codec->primary_dig_out_type = HDA_PCM_TYPE_HDMI; >> + codec->bus->primary_dig_out_type = HDA_PCM_TYPE_HDMI; >> } >> - if (!codec->primary_dig_out_type) >> - codec->primary_dig_out_type = type; >> + if (!codec->bus->primary_dig_out_type) >> + codec->bus->primary_dig_out_type = type; >> >> idx = find_empty_mixer_ctl_idx(codec, "IEC958 Playback Switch", dev); >> if (idx < 0) { >> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h >> index 8665540..ab807f7 100644 >> --- a/sound/pci/hda/hda_codec.h >> +++ b/sound/pci/hda/hda_codec.h >> @@ -671,6 +671,9 @@ struct hda_bus { >> unsigned int response_reset:1; /* controller was reset */ >> unsigned int in_reset:1; /* during reset operation */ >> unsigned int power_keep_link_on:1; /* don't power off HDA link */ >> + >> + /* primary digital out PCM type */ >> + int primary_dig_out_type; >> }; >> >> /* >> @@ -837,7 +840,6 @@ struct hda_codec { >> struct mutex hash_mutex; >> struct snd_array spdif_out; >> unsigned int spdif_in_enable; /* SPDIF input enable? */ >> - int primary_dig_out_type; /* primary digital out PCM type */ >> const hda_nid_t *slave_dig_outs; /* optional digital out slave widgets */ >> struct snd_array init_pins; /* initial (BIOS) pin configurations */ >> struct snd_array driver_pins; /* pin configs set by codec parser */ >> -- >> 1.7.10 >> > -- Anssi Hannula