From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Henningsson Subject: Re: [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches Date: Tue, 03 Sep 2013 14:36:13 +0200 Message-ID: <5225D7BD.1060005@canonical.com> References: <1378202279-14990-1-git-send-email-tiwai@suse.de> <5225C3B3.8030108@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by alsa0.perex.cz (Postfix) with ESMTP id 707A12619E3 for ; Tue, 3 Sep 2013 14:36:14 +0200 (CEST) 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: Mengdong Lin , alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org On 09/03/2013 02:06 PM, Takashi Iwai wrote: > At Tue, 03 Sep 2013 13:10:43 +0200, > David Henningsson wrote: >> >> On 09/03/2013 11:57 AM, Takashi Iwai wrote: >>> When the transcoder:port mapping on Haswell HDMI/DP audio is changed >>> during the stream playback, the sound gets lost. Typically this >>> problem is seen when the user switches the graphics mode from eDP+DP >>> to DP-only configuration, where CRTC 1 is used for DP in the former >>> while CRTC 0 is used for the latter. >>> >>> The graphics controller notifies the change via the normal ELD update >>> procedure, so we get the intrinsic event. For enabling the sound >>> again, the HDMI audio driver needs to reset the pin and set up the >>> audio infoframe again. >> >> Thanks for working on this! >> >> See a review comment below. >> >>> >>> This patch achieves it by: >>> - keep the current status of channels and info frame setup in per_pin >>> struct, >>> - check the reconnection in the intrinsic event handler, >>> - reset the pin and the re-invoke hdmi_setup_audio_infoframe() >>> accordingly. >>> >>> The hdmi_setup_audio_infoframe() function has been changed, too, so >>> that it can be invoked without passing the substream instance. >>> >>> The patch is mostly based on the work by Mengdong Lin. >>> >>> Cc: Mengdong Lin >>> Cc: >>> Signed-off-by: Takashi Iwai >>> --- >>> sound/pci/hda/patch_hdmi.c | 41 +++++++++++++++++++++++++++++++---------- >>> 1 file changed, 31 insertions(+), 10 deletions(-) >>> >>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >>> index b83b14f..22b5089 100644 >>> --- a/sound/pci/hda/patch_hdmi.c >>> +++ b/sound/pci/hda/patch_hdmi.c >>> @@ -67,6 +67,8 @@ struct hdmi_spec_per_pin { >>> struct delayed_work work; >>> struct snd_kcontrol *eld_ctl; >>> int repoll_count; >>> + bool setup; /* the stream has been set up by prepare callback */ >>> + int channels; /* current number of channels */ >>> bool non_pcm; >>> bool chmap_set; /* channel-map override by ALSA API? */ >>> unsigned char chmap[8]; /* ALSA API channel-map */ >>> @@ -879,18 +881,19 @@ static bool hdmi_infoframe_uptodate(struct hda_codec *codec, hda_nid_t pin_nid, >>> return true; >>> } >>> >>> -static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx, >>> - bool non_pcm, >>> - struct snd_pcm_substream *substream) >>> +static void hdmi_setup_audio_infoframe(struct hda_codec *codec, >>> + struct hdmi_spec_per_pin *per_pin, >>> + bool non_pcm) >>> { >>> - struct hdmi_spec *spec = codec->spec; >>> - struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); >>> hda_nid_t pin_nid = per_pin->pin_nid; >>> - int channels = substream->runtime->channels; >>> + int channels = per_pin->channels; >>> struct hdmi_eld *eld; >>> int ca; >>> union audio_infoframe ai; >>> >>> + if (!channels) >>> + return; >>> + >>> eld = &per_pin->sink_eld; >>> if (!eld->monitor_present) >>> return; >>> @@ -1341,6 +1344,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) >>> eld_changed = true; >>> } >>> if (update_eld) { >>> + bool old_eld_valid = pin_eld->eld_valid; >>> pin_eld->eld_valid = eld->eld_valid; >>> eld_changed = pin_eld->eld_size != eld->eld_size || >>> memcmp(pin_eld->eld_buffer, eld->eld_buffer, >>> @@ -1350,6 +1354,18 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) >>> eld->eld_size); >>> pin_eld->eld_size = eld->eld_size; >>> pin_eld->info = eld->info; >>> + >>> + /* Haswell-specific workaround: re-setup when the transcoder is >>> + * changed during the stream playback >>> + */ >>> + if (codec->vendor_id == 0x80862807 && >>> + eld->eld_valid && !old_eld_valid && per_pin->setup) { >>> + snd_hda_codec_write(codec, pin_nid, 0, >>> + AC_VERB_SET_AMP_GAIN_MUTE, >>> + AMP_OUT_UNMUTE); >> >> If the system is deliberately muted by turning off "IEC958 Playback >> Switch", are we now ignoring that and turning it back on? > > The pin amp is always unmuted. The IEC958 Playback Switch is > controlled via IEC958 status bits instead. Ok. > >> Also, this looks a bit like the workaround I added a while back >> (83f26ad2c909083fa6 - ALSA: hda - fixup D3 pin and right channel mute on >> Haswell HDMI audio) is there a possibility we need to fix up D3 as well >> here? I e, call haswell_verify_pin_D0 rather than just setting the mute? >> Or perhaps move the call to haswell_verify_pin_D0 from hdmi_setup_stream >> to hdme_setup_audio_infoframe ? > > The power state is fine in this case, but just to reset the amp state > by some reason. It's not enough to update the info frame, as far as I > tested. Ok - maybe Mengdong can comment more on this, if it's the same scenario as I was working around earlier (right channel muted and pin in D3), or if this is something different. If it's the same issue, I was thinking that we could 1) skip the extra snd_hda_codec_write here, and 2) move the call to haswell_verify_pin_D0 from hdmi_setup_stream to hdmi_setup_audio_infoframe? Also, if the pin amp is always unmuted, we could speed up haswell_verify_pin_D0 to always unmute instead of read-then-write. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic