diff mbox

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

Message ID s5hpoyvsgto.wl-tiwai@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Takashi Iwai Nov. 27, 2015, 1:38 p.m. UTC
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.

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(+)

Comments

David Henningsson Nov. 27, 2015, 1:45 p.m. UTC | #1
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?

>
> 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);
>   }
>
>
Takashi Iwai Nov. 27, 2015, 1:50 p.m. UTC | #2
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
>
diff mbox

Patch

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);
 }