From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anssi Hannula Subject: Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect Date: Mon, 07 Oct 2013 15:45:09 +0300 Message-ID: <884f0056bbbc27a2f35c581de11a07d9@mail.onse.fi> References: <1380737612-16512-1-git-send-email-anssi.hannula@iki.fi> <73e325dd7c696e2d16946890a66dc838@mail.onse.fi> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: stable-owner@vger.kernel.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org, stable@vger.kernel.org List-Id: alsa-devel@alsa-project.org Takashi Iwai kirjoitti 2013-10-07 13:43: > At Mon, 07 Oct 2013 13:31:17 +0300, > Anssi Hannula wrote: > > Takashi Iwai kirjoitti 2013-10-07 10:48: > > At Wed, 2 Oct 2013 21:13:32 +0300, > > Anssi Hannula wrote: > > > > Currently hdmi_setup_audio_infoframe() reprograms the HDA channel > > mapping only when the infoframe is not up-to-date or the non-PCM flag > > has changed. > > > > However, when just the channel map has been changed, the infoframe may > > still be up-to-date and non-PCM flag may not have changed, so the new > > channel map is not actually programmed into the HDA codec. > > > > Notably, this failing case is also always triggered when the device is > > already in a prepared state and a new channel map is configured while > > changing only the channel positions (for example, plain > > "speaker-test -c2 -m FR,FL"). > > > > Fix that by keeping track of the last programmed channel map and making > > hdmi_setup_audio_infoframe() always update the hardware mapping if the > > channel map has changed. > > > > Hmm, maybe it's easier (and safer) to remove the check and always call > > hdmi_setup_channel_mapping()? Of course, the drawback is the > > unnecessary verbs, but it's not too much. > > Easier, yes. I don't really have that good of an idea how big of a > delay > do 8 write verbs cause, so I'll rely on your judgment :) > > I guess I could replace the writes with read-and-write-if-differs > cycles > when I add the set_slot ops for ATI/AMD, since it seems to be used for > other similar cases (like channel count)? > > Well, this would result in more cost, as read requires a sync. Hmm, so why does e.g. converter channel count set-up use that, then? > If any, we can use the cached write/update. But this assumes that the > hardware never clears the value by some operation internally. It should not, but better be safe than sorry here (at least for stable) :) > > Signed-off-by: Anssi Hannula > > Cc: > > --- > > > > BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be > > called > > at the same time with generic_hdmi_playback_pcm_prepare() that I didn't > > see with a quick look? > > Both of them call hdmi_setup_audio_infoframe() which is not > > thread-safe. > > > > Yeah, we need to introduce a mutex there. > > I guess hdmi_present_sense() is the same, as it can also call > hdmi_setup_audio_infoframe()? > > Hm, looking at the code, we have already pin_eld->lock in > hdmi_present_sense(). This can be used in other places, too. Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons than eld as well (e.g. write verbs to hw), though, so it might be a bit confusing as-is. Better move/rename the lock if we end up using it? Or not? > (And pin_eld->monitor_present change at the beginning of > hdmi_present_sense() should be protected in the mutex, too...) > > BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe() > always > when eld_changed is true? hdmi_setup_audio_infoframe() does not do its > stuff if !monitor_present, so if one starts playback before plugging > in, > the channel mappings etc will never be set even when plug-in happens > (from what I can see, that is - did not test yet). > > Well, there is such a call but currently specific to Haswell because > it hasn't been tested for others. Maybe we can simply get rid of > is_haswell() check there. OK, will test later and provide a patch if it works as expected. > > > > > > thanks, > > > > Takashi > > > > sound/pci/hda/patch_hdmi.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > > index 7ea0245..09f38a1 100644 > > --- a/sound/pci/hda/patch_hdmi.c > > +++ b/sound/pci/hda/patch_hdmi.c > > @@ -74,6 +74,8 @@ struct hdmi_spec_per_pin { > > bool non_pcm; > > bool chmap_set; /* channel-map override by ALSA API? */ > > unsigned char chmap[8]; /* ALSA API channel-map */ > > + bool last_chmap_set; /* channel-map override programmed? */ > > + unsigned char last_chmap[8]; /* last programmed channel-map */ > > char pcm_name[8]; /* filled in build_pcm callbacks */ > > }; > > > > @@ -955,15 +957,21 @@ static void hdmi_setup_audio_infoframe(struct > > hda_codec *codec, > > ai.bytes, sizeof(ai)); > > hdmi_start_infoframe_trans(codec, pin_nid); > > } else { > > - /* For non-pcm audio switch, setup new channel mapping > > - * accordingly */ > > - if (per_pin->non_pcm != non_pcm) > > + /* For non-pcm audio switch or channel-map switch, > > + * setup new channel mapping accordingly */ > > + if (per_pin->non_pcm != non_pcm > > + || per_pin->chmap_set != per_pin->last_chmap_set > > + || (per_pin->chmap_set && memcmp(per_pin->chmap, > > per_pin->last_chmap, > > + ARRAY_SIZE(per_pin->chmap)) != 0)) > > hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca, > > channels, per_pin->chmap, > > per_pin->chmap_set); > > } > > > > per_pin->non_pcm = non_pcm; > > + per_pin->last_chmap_set = per_pin->chmap_set; > > + if (per_pin->chmap_set) > > + memcpy(per_pin->last_chmap, per_pin->chmap, > > ARRAY_SIZE(per_pin->chmap)); > > } > > > > > > -- > > 1.8.1.5 > > >