Re: [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events

From: Takashi Iwai <tiwai@suse.de>
To: David Henningsson <david.henningsson@canonical.com>
Cc: "Vetter, Daniel" <daniel.vetter@intel.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
Date: Fri, 27 Nov 2015 14:50:31 +0100	[thread overview]
Message-ID: <s5hk2p3sg9k.wl-tiwai@suse.de> (raw)
In-Reply-To: <56585E7B.5010002@canonical.com>

On Fri, 27 Nov 2015 14:45:31 +0100,
David Henningsson wrote:
> 
> 
> 
> On 2015-11-27 14:38, Takashi Iwai wrote:
> > On Fri, 27 Nov 2015 03:55:28 +0100,
> > Zhang, Xiong Y wrote:
> >>
> >>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> >>> index bdb6f226d006..0cd7bb30b045 100644
> >>> --- a/sound/pci/hda/patch_hdmi.c
> >>> +++ b/sound/pci/hda/patch_hdmi.c
> >>> @@ -2352,6 +2352,10 @@ static void intel_pin_eld_notify(void *audio_ptr, int
> >>> port)
> >>>   	struct hda_codec *codec = audio_ptr;
> >>>   	int pin_nid = port + 0x04;
> >>>
> >>> +	/* skip notification during suspend */
> >>> +	if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
> >>> +		return;
> >>> +
> >>>   	check_presence_and_report(codec, pin_nid);
> >>>   }
> >>>
> >> [Zhang, Xiong Y] yes, this patch could remove the error message.
> >
> > Alright, then it's indeed a failure in the sound driver side.
> > Below is the official patch I'm going to merge.
> 
> Just a quick question; have you checked that this does not bring back 
> the original bug the entire patch set was supposed to fix?

Do you mean the hotplug handling runtime PM?  I tested it locally, but
wider ranged tests would be appreciated.  In theory, it should work as
mentioned in the changelog.

Takashi

> 
> >
> > thanks,
> >
> > Takashi
> >
> > -- 8< --
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: hda - Skip ELD notification during system suspend
> >
> > The recent addition of ELD notifier for Intel HDMI/DP codec may lead
> > the bad codec connection found as kernel messages like below:
> >   Suspending console(s) (use no_console_suspend to debug)
> >   hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
> >   snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08
> >   snd_hda_intel 0000:00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08
> >   ....
> >    snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128
> >    snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00
> >   snd_hda_intel 0000:00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00
> >   snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00
> >   azx_single_wait_for_response: 42 callbacks suppressed
> >
> > This seems appearing when the sound driver went to suspend before i915
> > driver.  Then i915 driver disables HDMI/DP audio bit and calls the
> > registered notifier, and the HDA codec tries to handle it as a
> > hot(un)plug.  But since the driver is already in the suspended state,
> > it fails miserably.
> >
> > As this is a sort of spurious wakeup, it can be ignored safely, as
> > long as it's delivered during the system suspend.  OTOH, if a
> > notification comes during the runtime suspend, the situation is
> > different: we need to wake up.  But during the system suspend, such a
> > notification can't be the reason for a wakeup.
> >
> > This patch addresses it by a simple check of the current sound card
> > status.  The skipped notification doesn't matter because the HDA
> > driver will check the plugged status forcibly at the resume in
> > return.
> >
> > Then, why the card status, not a runtime PM status or else?  The HDA
> > controller driver is supposed to set the card status to D3 at the
> > system suspend but not at the runtime suspend.  So we can see it as a
> > flag that is set only for the system suspend.  Admittedly, it's a bit
> > ugly, but it should work well for now.
> >
> > Reported-and-tested-by: "Zhang, Xiong Y" <xiong.y.zhang@intel.com>
> > Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events')
> > Cc: <stable@vger.kernel.org> # v4.3+
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   sound/pci/hda/patch_hdmi.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index bdb6f226d006..4b6fb668c91c 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -2352,6 +2352,12 @@ static void intel_pin_eld_notify(void *audio_ptr, int port)
> >   	struct hda_codec *codec = audio_ptr;
> >   	int pin_nid = port + 0x04;
> >
> > +	/* skip notification during system suspend (but not in runtime PM);
> > +	 * the state will be updated at resume
> > +	 */
> > +	if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0)
> > +		return;
> > +
> >   	check_presence_and_report(codec, pin_nid);
> >   }
> >
> >
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx