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

[Takashi Iwai]

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 
> > 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 :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08
> >   snd_hda_intel :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 :00:1f.3: azx_get_response timeout, switching to 
> > polling mode: last cmd=0x206f2f00
> >   snd_hda_intel :00:1f.3: No response from codec, disabling MSI: last 
> > cmd=0x206f2f00
> >   snd_hda_intel :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" 
> > Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify 
> > events')
> > Cc:  # v4.3+
> > Signed-off-by: Takashi Iwai 
> > ---
> >   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

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

[Takashi Iwai]

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 
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 :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08
 snd_hda_intel :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 :00:1f.3: azx_get_response timeout, switching to polling 
mode: last cmd=0x206f2f00
 snd_hda_intel :00:1f.3: No response from codec, disabling MSI: last 
cmd=0x206f2f00
 snd_hda_intel :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" 
Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events')
Cc:  # v4.3+
Signed-off-by: Takashi Iwai 
---
 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);
 }
 
-- 
2.6.3

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Henningsson]

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 
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 :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08
  snd_hda_intel :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 :00:1f.3: azx_get_response timeout, switching to polling 
mode: last cmd=0x206f2f00
  snd_hda_intel :00:1f.3: No response from codec, disabling MSI: last 
cmd=0x206f2f00
  snd_hda_intel :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" 
Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events')
Cc:  # v4.3+
Signed-off-by: Takashi Iwai 
---
  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

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

[Zhang, Xiong Y]

> 
> On Thu, 26 Nov 2015 16:58:09 +0100,
> Ville Syrjälä wrote:
> >
> > On Thu, Nov 26, 2015 at 04:51:04PM +0100, Takashi Iwai wrote:
> > > On Thu, 26 Nov 2015 16:43:23 +0100,
> > > Ville Syrjälä wrote:
> > > >
> > > > On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
> > > > >
> > > > >
> > > > > On 2015-11-26 16:23, Takashi Iwai wrote:
> > > > > > On Thu, 26 Nov 2015 16:08:06 +0100,
> > > > > > David Henningsson wrote:
> > > > > >>
> > > > > >>
> > > > > >>
> > > > > >> On 2015-11-26 10:24, Takashi Iwai wrote:
> > > > > >>> On Thu, 26 Nov 2015 10:16:17 +0100,
> > > > > >>> Zhang, Xiong Y wrote:
> > > > > 
> > > > > 
> > > > > > On Thu, 26 Nov 2015 08:57:30 +0100,
> > > > > > Zhang, Xiong Y wrote:
> > > > > >>
> > > > > > BTW, I have a patchset to avoid the audio h/w wakeup by a
> new
> > > > > > component ops to get ELD and connection states.  It was
> posted to
> > > > > > alsa-devel shortly ago just as a reference, but this should
> work well
> > > > > > in such a case, too.  The test patches are found in
> test/hdmi-jack
> > > > > > branch of my sound git tree.
> > > > > >>>
> > > > > >>> Did you try this branch (merge onto intel-test-nightly)?
> > > > > >>>
> > > > > >> [Zhang, Xiong Y] Yes, this branch could fix my issue.
> > > > > >
> > > > > > OK, good to hear.  But this isn't for 4.4 or backport, as it's 
> > > > > > more
> > > > > > radical changes.
> > > > > >
> > > > > > Could you check the patch below instead?  This suppresses the
> notifier
> > > > > > handling during suspend.  It's bad, but the hotplug should be 
> > > > > > still
> > > > > > handled via normal unsol event, so it should keep working, good
> enough
> > > > > > as a stop gap.
> > > > > >
> > > > > >
> > > > > > Takashi
> > > > > >
> > > > > > ---
> > > > > > diff --git a/sound/pci/hda/patch_hdmi.c
> b/sound/pci/hda/patch_hdmi.c
> > > > > > index bdb6f226d006..f7c70e2ae65c 100644
> > > > > > --- a/sound/pci/hda/patch_hdmi.c
> > > > > > +++ b/sound/pci/hda/patch_hdmi.c
> > > > > > @@ -33,6 +33,7 @@
> > > > > >#include 
> > > > > >#include 
> > > > > >#include 
> > > > > > +#include 
> > > > > >#include 
> > > > > >#include 
> > > > > >#include 
> > > > > > @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void
> *audio_ptr, int
> > > > > > port)
> > > > > > struct hda_codec *codec = audio_ptr;
> > > > > > int pin_nid = port + 0x04;
> > > > > >
> > > > > > -   check_presence_and_report(codec, pin_nid);
> > > > > > +   if (!atomic_read(>core.in_pm) &&
> > > > > > +   !pm_runtime_suspended(hda_codec_dev(codec)))
> > > > > > +   check_presence_and_report(codec, pin_nid);
> > > > > >}
> > > > > >
> > > > > >static int patch_generic_hdmi(struct hda_codec *codec)
> > > > >  [Zhang, Xiong Y]  this patch couldn't remove the error message.
> So audio controller isn't in Runtime D3 after azx_suspend().
> > > > > >>>
> > > > > >>> OK, then the problem isn't about the HD-audio driver but rather
> i915
> > > > > >>> driver.  As already mentioned, i915 driver shouldn't issue
> eld_notify
> > > > > >>> callback in the suspend code path.
> > > > > >>
> > > > > >> We don't have a complete drm log so I'm only speculating here; but
> the
> > > > > >> HDA log seems to indicate the power well is off when we try to talk
> to
> > > > > >> the HDA controller. That means either the i915 shut it down while 
> > > > > >> we
> > > > > >> were holding a lock on it, or HDA tried to lock it and that didn't 
> > > > > >> make
> > > > > >> it through; in which case the HDA side should handle an error from
> > > > > >> get_power more gracefully...?
> > > > > >
> > > > > > My understanding is that it's triggered *during* i915 suspend.  Then
> > > > > > the path calls the HDA callback, and HDA controller tries to get 
> > > > > > power
> > > > > > and proceeds as it has no idea that it's being shut off.
> > > > >
> > > > > Well; that can also be stopped by letting the get_power call return a
> > > > > failure code in case i915 is trying to suspend itself. That seems more
> > > > > robust to me, as it could potentially avoid other S3 races too...?
> > > > >
> > > > > >
> > > > > > Unfortunately, neither get_power callback or the corresponding HDA
> > > > > > code has a capability to check that state.  So, changing / fixing 
> > > > > > this
> > > > > > there would be nice but very hard.
> > > > >
> > > > > Surely the i915 driver has some "am_i_suspending" variable it can 
> > > > > check
> > > > > in the get_power callback?
> > > >
> > > > I don't understand why you need to do anything special. When the eld
> > > > notify happens during suspend, the hardware is still fully powered up,
> > > > so it should just work(tm) as long as the 

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

[Takashi Iwai]

On Thu, 26 Nov 2015 08:57:30 +0100,
Zhang, Xiong Y wrote:
> 
> > > > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > > > component ops to get ELD and connection states.  It was posted to
> > > > alsa-devel shortly ago just as a reference, but this should work well
> > > > in such a case, too.  The test patches are found in test/hdmi-jack
> > > > branch of my sound git tree.
> > 
> > Did you try this branch (merge onto intel-test-nightly)?
> > 
> [Zhang, Xiong Y] Yes, this branch could fix my issue.

OK, good to hear.  But this isn't for 4.4 or backport, as it's more
radical changes.

Could you check the patch below instead?  This suppresses the notifier
handling during suspend.  It's bad, but the hotplug should be still
handled via normal unsol event, so it should keep working, good enough
as a stop gap.


Takashi

---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index bdb6f226d006..f7c70e2ae65c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int 
port)
struct hda_codec *codec = audio_ptr;
int pin_nid = port + 0x04;
 
-   check_presence_and_report(codec, pin_nid);
+   if (!atomic_read(>core.in_pm) &&
+   !pm_runtime_suspended(hda_codec_dev(codec)))
+   check_presence_and_report(codec, pin_nid);
 }
 
 static int patch_generic_hdmi(struct hda_codec *codec)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhang, Xiong Y]

> On Thu, 26 Nov 2015 08:57:30 +0100,
> Zhang, Xiong Y wrote:
> >
> > > > > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > > > > component ops to get ELD and connection states.  It was posted to
> > > > > alsa-devel shortly ago just as a reference, but this should work well
> > > > > in such a case, too.  The test patches are found in test/hdmi-jack
> > > > > branch of my sound git tree.
> > >
> > > Did you try this branch (merge onto intel-test-nightly)?
> > >
> > [Zhang, Xiong Y] Yes, this branch could fix my issue.
> 
> OK, good to hear.  But this isn't for 4.4 or backport, as it's more
> radical changes.
> 
> Could you check the patch below instead?  This suppresses the notifier
> handling during suspend.  It's bad, but the hotplug should be still
> handled via normal unsol event, so it should keep working, good enough
> as a stop gap.
> 
> 
> Takashi
> 
> ---
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index bdb6f226d006..f7c70e2ae65c 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int
> port)
>   struct hda_codec *codec = audio_ptr;
>   int pin_nid = port + 0x04;
> 
> - check_presence_and_report(codec, pin_nid);
> + if (!atomic_read(>core.in_pm) &&
> + !pm_runtime_suspended(hda_codec_dev(codec)))
> + check_presence_and_report(codec, pin_nid);
>  }
> 
>  static int patch_generic_hdmi(struct hda_codec *codec)
[Zhang, Xiong Y]  this patch couldn't remove the error message. So audio 
controller isn't in Runtime D3 after azx_suspend().

thanks
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Thu, 26 Nov 2015 10:16:17 +0100,
Zhang, Xiong Y wrote:
> 
> 
> > On Thu, 26 Nov 2015 08:57:30 +0100,
> > Zhang, Xiong Y wrote:
> > >
> > > > > > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > > > > > component ops to get ELD and connection states.  It was posted to
> > > > > > alsa-devel shortly ago just as a reference, but this should work 
> > > > > > well
> > > > > > in such a case, too.  The test patches are found in test/hdmi-jack
> > > > > > branch of my sound git tree.
> > > >
> > > > Did you try this branch (merge onto intel-test-nightly)?
> > > >
> > > [Zhang, Xiong Y] Yes, this branch could fix my issue.
> > 
> > OK, good to hear.  But this isn't for 4.4 or backport, as it's more
> > radical changes.
> > 
> > Could you check the patch below instead?  This suppresses the notifier
> > handling during suspend.  It's bad, but the hotplug should be still
> > handled via normal unsol event, so it should keep working, good enough
> > as a stop gap.
> > 
> > 
> > Takashi
> > 
> > ---
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index bdb6f226d006..f7c70e2ae65c 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -33,6 +33,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int
> > port)
> > struct hda_codec *codec = audio_ptr;
> > int pin_nid = port + 0x04;
> > 
> > -   check_presence_and_report(codec, pin_nid);
> > +   if (!atomic_read(>core.in_pm) &&
> > +   !pm_runtime_suspended(hda_codec_dev(codec)))
> > +   check_presence_and_report(codec, pin_nid);
> >  }
> > 
> >  static int patch_generic_hdmi(struct hda_codec *codec)
> [Zhang, Xiong Y]  this patch couldn't remove the error message. So audio 
> controller isn't in Runtime D3 after azx_suspend().

OK, then the problem isn't about the HD-audio driver but rather i915
driver.  As already mentioned, i915 driver shouldn't issue eld_notify
callback in the suspend code path.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Henningsson]

On 2015-11-26 10:24, Takashi Iwai wrote:

On Thu, 26 Nov 2015 10:16:17 +0100,
Zhang, Xiong Y wrote:




On Thu, 26 Nov 2015 08:57:30 +0100,
Zhang, Xiong Y wrote:



BTW, I have a patchset to avoid the audio h/w wakeup by a new
component ops to get ELD and connection states.  It was posted to
alsa-devel shortly ago just as a reference, but this should work well
in such a case, too.  The test patches are found in test/hdmi-jack
branch of my sound git tree.


Did you try this branch (merge onto intel-test-nightly)?


[Zhang, Xiong Y] Yes, this branch could fix my issue.


OK, good to hear.  But this isn't for 4.4 or backport, as it's more
radical changes.

Could you check the patch below instead?  This suppresses the notifier
handling during suspend.  It's bad, but the hotplug should be still
handled via normal unsol event, so it should keep working, good enough
as a stop gap.


Takashi

---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index bdb6f226d006..f7c70e2ae65c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -33,6 +33,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int
port)
struct hda_codec *codec = audio_ptr;
int pin_nid = port + 0x04;

-   check_presence_and_report(codec, pin_nid);
+   if (!atomic_read(>core.in_pm) &&
+   !pm_runtime_suspended(hda_codec_dev(codec)))
+   check_presence_and_report(codec, pin_nid);
  }

  static int patch_generic_hdmi(struct hda_codec *codec)

[Zhang, Xiong Y]  this patch couldn't remove the error message. So audio 
controller isn't in Runtime D3 after azx_suspend().


OK, then the problem isn't about the HD-audio driver but rather i915
driver.  As already mentioned, i915 driver shouldn't issue eld_notify
callback in the suspend code path.


We don't have a complete drm log so I'm only speculating here; but the 
HDA log seems to indicate the power well is off when we try to talk to 
the HDA controller. That means either the i915 shut it down while we 
were holding a lock on it, or HDA tried to lock it and that didn't make 
it through; in which case the HDA side should handle an error from 
get_power more gracefully...?



--
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

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

[Takashi Iwai]

On Thu, 26 Nov 2015 16:08:06 +0100,
David Henningsson wrote:
> 
> 
> 
> On 2015-11-26 10:24, Takashi Iwai wrote:
> > On Thu, 26 Nov 2015 10:16:17 +0100,
> > Zhang, Xiong Y wrote:
> >>
> >>
> >>> On Thu, 26 Nov 2015 08:57:30 +0100,
> >>> Zhang, Xiong Y wrote:
> 
> >>> BTW, I have a patchset to avoid the audio h/w wakeup by a new
> >>> component ops to get ELD and connection states.  It was posted to
> >>> alsa-devel shortly ago just as a reference, but this should work well
> >>> in such a case, too.  The test patches are found in test/hdmi-jack
> >>> branch of my sound git tree.
> >
> > Did you try this branch (merge onto intel-test-nightly)?
> >
>  [Zhang, Xiong Y] Yes, this branch could fix my issue.
> >>>
> >>> OK, good to hear.  But this isn't for 4.4 or backport, as it's more
> >>> radical changes.
> >>>
> >>> Could you check the patch below instead?  This suppresses the notifier
> >>> handling during suspend.  It's bad, but the hotplug should be still
> >>> handled via normal unsol event, so it should keep working, good enough
> >>> as a stop gap.
> >>>
> >>>
> >>> Takashi
> >>>
> >>> ---
> >>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> >>> index bdb6f226d006..f7c70e2ae65c 100644
> >>> --- a/sound/pci/hda/patch_hdmi.c
> >>> +++ b/sound/pci/hda/patch_hdmi.c
> >>> @@ -33,6 +33,7 @@
> >>>   #include 
> >>>   #include 
> >>>   #include 
> >>> +#include 
> >>>   #include 
> >>>   #include 
> >>>   #include 
> >>> @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, 
> >>> int
> >>> port)
> >>>   struct hda_codec *codec = audio_ptr;
> >>>   int pin_nid = port + 0x04;
> >>>
> >>> - check_presence_and_report(codec, pin_nid);
> >>> + if (!atomic_read(>core.in_pm) &&
> >>> + !pm_runtime_suspended(hda_codec_dev(codec)))
> >>> + check_presence_and_report(codec, pin_nid);
> >>>   }
> >>>
> >>>   static int patch_generic_hdmi(struct hda_codec *codec)
> >> [Zhang, Xiong Y]  this patch couldn't remove the error message. So audio 
> >> controller isn't in Runtime D3 after azx_suspend().
> >
> > OK, then the problem isn't about the HD-audio driver but rather i915
> > driver.  As already mentioned, i915 driver shouldn't issue eld_notify
> > callback in the suspend code path.
> 
> We don't have a complete drm log so I'm only speculating here; but the 
> HDA log seems to indicate the power well is off when we try to talk to 
> the HDA controller. That means either the i915 shut it down while we 
> were holding a lock on it, or HDA tried to lock it and that didn't make 
> it through; in which case the HDA side should handle an error from 
> get_power more gracefully...?

My understanding is that it's triggered *during* i915 suspend.  Then
the path calls the HDA callback, and HDA controller tries to get power
and proceeds as it has no idea that it's being shut off.

Unfortunately, neither get_power callback or the corresponding HDA
code has a capability to check that state.  So, changing / fixing this
there would be nice but very hard.

So I believe it's easier to avoid calling the eld_notify callback from
i915 side during its suspend code.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Henningsson]

On 2015-11-26 16:23, Takashi Iwai wrote:

On Thu, 26 Nov 2015 16:08:06 +0100,
David Henningsson wrote:




On 2015-11-26 10:24, Takashi Iwai wrote:

On Thu, 26 Nov 2015 10:16:17 +0100,
Zhang, Xiong Y wrote:




On Thu, 26 Nov 2015 08:57:30 +0100,
Zhang, Xiong Y wrote:



BTW, I have a patchset to avoid the audio h/w wakeup by a new
component ops to get ELD and connection states.  It was posted to
alsa-devel shortly ago just as a reference, but this should work well
in such a case, too.  The test patches are found in test/hdmi-jack
branch of my sound git tree.


Did you try this branch (merge onto intel-test-nightly)?


[Zhang, Xiong Y] Yes, this branch could fix my issue.


OK, good to hear.  But this isn't for 4.4 or backport, as it's more
radical changes.

Could you check the patch below instead?  This suppresses the notifier
handling during suspend.  It's bad, but the hotplug should be still
handled via normal unsol event, so it should keep working, good enough
as a stop gap.


Takashi

---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index bdb6f226d006..f7c70e2ae65c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -33,6 +33,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   #include 
@@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int
port)
struct hda_codec *codec = audio_ptr;
int pin_nid = port + 0x04;

-   check_presence_and_report(codec, pin_nid);
+   if (!atomic_read(>core.in_pm) &&
+   !pm_runtime_suspended(hda_codec_dev(codec)))
+   check_presence_and_report(codec, pin_nid);
   }

   static int patch_generic_hdmi(struct hda_codec *codec)

[Zhang, Xiong Y]  this patch couldn't remove the error message. So audio 
controller isn't in Runtime D3 after azx_suspend().


OK, then the problem isn't about the HD-audio driver but rather i915
driver.  As already mentioned, i915 driver shouldn't issue eld_notify
callback in the suspend code path.


We don't have a complete drm log so I'm only speculating here; but the
HDA log seems to indicate the power well is off when we try to talk to
the HDA controller. That means either the i915 shut it down while we
were holding a lock on it, or HDA tried to lock it and that didn't make
it through; in which case the HDA side should handle an error from
get_power more gracefully...?


My understanding is that it's triggered *during* i915 suspend.  Then
the path calls the HDA callback, and HDA controller tries to get power
and proceeds as it has no idea that it's being shut off.


Well; that can also be stopped by letting the get_power call return a 
failure code in case i915 is trying to suspend itself. That seems more 
robust to me, as it could potentially avoid other S3 races too...?




Unfortunately, neither get_power callback or the corresponding HDA
code has a capability to check that state.  So, changing / fixing this
there would be nice but very hard.


Surely the i915 driver has some "am_i_suspending" variable it can check 
in the get_power callback?




So I believe it's easier to avoid calling the eld_notify callback from
i915 side during its suspend code.


Takashi



--
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

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

[Imre Deak]

On to, 2015-11-26 at 16:29 +0100, David Henningsson wrote:
> 
> On 2015-11-26 16:23, Takashi Iwai wrote:
> > On Thu, 26 Nov 2015 16:08:06 +0100,
> > David Henningsson wrote:
> > > 
> > > 
> > > 
> > > On 2015-11-26 10:24, Takashi Iwai wrote:
> > > > On Thu, 26 Nov 2015 10:16:17 +0100,
> > > > Zhang, Xiong Y wrote:
> > > > > 
> > > > > 
> > > > > > On Thu, 26 Nov 2015 08:57:30 +0100,
> > > > > > Zhang, Xiong Y wrote:
> > > > > > > 
> > > > > > > > > > BTW, I have a patchset to avoid the audio h/w
> > > > > > > > > > wakeup by a new
> > > > > > > > > > component ops to get ELD and connection states.  It
> > > > > > > > > > was posted to
> > > > > > > > > > alsa-devel shortly ago just as a reference, but
> > > > > > > > > > this should work well
> > > > > > > > > > in such a case, too.  The test patches are found in
> > > > > > > > > > test/hdmi-jack
> > > > > > > > > > branch of my sound git tree.
> > > > > > > > 
> > > > > > > > Did you try this branch (merge onto intel-test-
> > > > > > > > nightly)?
> > > > > > > > 
> > > > > > > [Zhang, Xiong Y] Yes, this branch could fix my issue.
> > > > > > 
> > > > > > OK, good to hear.  But this isn't for 4.4 or backport, as
> > > > > > it's more
> > > > > > radical changes.
> > > > > > 
> > > > > > Could you check the patch below instead?  This suppresses
> > > > > > the notifier
> > > > > > handling during suspend.  It's bad, but the hotplug should
> > > > > > be still
> > > > > > handled via normal unsol event, so it should keep working,
> > > > > > good enough
> > > > > > as a stop gap.
> > > > > > 
> > > > > > 
> > > > > > Takashi
> > > > > > 
> > > > > > ---
> > > > > > diff --git a/sound/pci/hda/patch_hdmi.c
> > > > > > b/sound/pci/hda/patch_hdmi.c
> > > > > > index bdb6f226d006..f7c70e2ae65c 100644
> > > > > > --- a/sound/pci/hda/patch_hdmi.c
> > > > > > +++ b/sound/pci/hda/patch_hdmi.c
> > > > > > @@ -33,6 +33,7 @@
> > > > > >    #include 
> > > > > >    #include 
> > > > > >    #include 
> > > > > > +#include 
> > > > > >    #include 
> > > > > >    #include 
> > > > > >    #include 
> > > > > > @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void
> > > > > > *audio_ptr, int
> > > > > > port)
> > > > > >     struct hda_codec *codec = audio_ptr;
> > > > > >     int pin_nid = port + 0x04;
> > > > > > 
> > > > > > -   check_presence_and_report(codec, pin_nid);
> > > > > > +   if (!atomic_read(>core.in_pm) &&
> > > > > > +   !pm_runtime_suspended(hda_codec_dev(codec)))
> > > > > > +   check_presence_and_report(codec, pin_nid);
> > > > > >    }
> > > > > > 
> > > > > >    static int patch_generic_hdmi(struct hda_codec *codec)
> > > > > [Zhang, Xiong Y]  this patch couldn't remove the error
> > > > > message. So audio controller isn't in Runtime D3 after
> > > > > azx_suspend().
> > > > 
> > > > OK, then the problem isn't about the HD-audio driver but rather
> > > > i915
> > > > driver.  As already mentioned, i915 driver shouldn't issue
> > > > eld_notify
> > > > callback in the suspend code path.
> > > 
> > > We don't have a complete drm log so I'm only speculating here;
> > > but the
> > > HDA log seems to indicate the power well is off when we try to
> > > talk to
> > > the HDA controller. That means either the i915 shut it down while
> > > we
> > > were holding a lock on it, or HDA tried to lock it and that
> > > didn't make
> > > it through; in which case the HDA side should handle an error
> > > from
> > > get_power more gracefully...?
> > 
> > My understanding is that it's triggered *during* i915
> > suspend.  Then
> > the path calls the HDA callback, and HDA controller tries to get
> > power
> > and proceeds as it has no idea that it's being shut off.
> 
> Well; that can also be stopped by letting the get_power call return a
> failure code in case i915 is trying to suspend itself. That seems
> more 
> robust to me, as it could potentially avoid other S3 races too...?
> 
> > 
> > Unfortunately, neither get_power callback or the corresponding HDA
> > code has a capability to check that state.  So, changing / fixing
> > this
> > there would be nice but very hard.
> 
> Surely the i915 driver has some "am_i_suspending" variable it can
> check 
> in the get_power callback?

As I understand this happens during the i915 system suspend/resume
hooks are running. There is no reason why the getting a power domain
reference would fail there. In fact we are keeping all power wells for
the whole duration of these callbacks, see the call to
intel_display_set_init_power(true) in i915_drm_suspend()
and i915_drm_resume_early()->intel_power_domains_init_hw(). So not sure
how the audio power well could be off at that time.


> 
> > 
> > So I believe it's easier to avoid calling the eld_notify callback
> > from
> > i915 side during its suspend code.
> > 
> > 
> > Takashi
> > 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
> 
> 
> On 2015-11-26 16:23, Takashi Iwai wrote:
> > On Thu, 26 Nov 2015 16:08:06 +0100,
> > David Henningsson wrote:
> >>
> >>
> >>
> >> On 2015-11-26 10:24, Takashi Iwai wrote:
> >>> On Thu, 26 Nov 2015 10:16:17 +0100,
> >>> Zhang, Xiong Y wrote:
> 
> 
> > On Thu, 26 Nov 2015 08:57:30 +0100,
> > Zhang, Xiong Y wrote:
> >>
> > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > component ops to get ELD and connection states.  It was posted to
> > alsa-devel shortly ago just as a reference, but this should work 
> > well
> > in such a case, too.  The test patches are found in test/hdmi-jack
> > branch of my sound git tree.
> >>>
> >>> Did you try this branch (merge onto intel-test-nightly)?
> >>>
> >> [Zhang, Xiong Y] Yes, this branch could fix my issue.
> >
> > OK, good to hear.  But this isn't for 4.4 or backport, as it's more
> > radical changes.
> >
> > Could you check the patch below instead?  This suppresses the notifier
> > handling during suspend.  It's bad, but the hotplug should be still
> > handled via normal unsol event, so it should keep working, good enough
> > as a stop gap.
> >
> >
> > Takashi
> >
> > ---
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index bdb6f226d006..f7c70e2ae65c 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -33,6 +33,7 @@
> >#include 
> >#include 
> >#include 
> > +#include 
> >#include 
> >#include 
> >#include 
> > @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, 
> > int
> > port)
> > struct hda_codec *codec = audio_ptr;
> > int pin_nid = port + 0x04;
> >
> > -   check_presence_and_report(codec, pin_nid);
> > +   if (!atomic_read(>core.in_pm) &&
> > +   !pm_runtime_suspended(hda_codec_dev(codec)))
> > +   check_presence_and_report(codec, pin_nid);
> >}
> >
> >static int patch_generic_hdmi(struct hda_codec *codec)
>  [Zhang, Xiong Y]  this patch couldn't remove the error message. So audio 
>  controller isn't in Runtime D3 after azx_suspend().
> >>>
> >>> OK, then the problem isn't about the HD-audio driver but rather i915
> >>> driver.  As already mentioned, i915 driver shouldn't issue eld_notify
> >>> callback in the suspend code path.
> >>
> >> We don't have a complete drm log so I'm only speculating here; but the
> >> HDA log seems to indicate the power well is off when we try to talk to
> >> the HDA controller. That means either the i915 shut it down while we
> >> were holding a lock on it, or HDA tried to lock it and that didn't make
> >> it through; in which case the HDA side should handle an error from
> >> get_power more gracefully...?
> >
> > My understanding is that it's triggered *during* i915 suspend.  Then
> > the path calls the HDA callback, and HDA controller tries to get power
> > and proceeds as it has no idea that it's being shut off.
> 
> Well; that can also be stopped by letting the get_power call return a 
> failure code in case i915 is trying to suspend itself. That seems more 
> robust to me, as it could potentially avoid other S3 races too...?
> 
> >
> > Unfortunately, neither get_power callback or the corresponding HDA
> > code has a capability to check that state.  So, changing / fixing this
> > there would be nice but very hard.
> 
> Surely the i915 driver has some "am_i_suspending" variable it can check 
> in the get_power callback?

I don't understand why you need to do anything special. When the eld
notify happens during suspend, the hardware is still fully powered up,
so it should just work(tm) as long as the eld_notify is a synchronous
call and it drops the power reference at the end.

I guess for any get_power after i915 has suspended we'd need to just
reject the get_power call. Or does something force hda to suspend before
and resume after i915 always?

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Thu, 26 Nov 2015 16:29:12 +0100,
David Henningsson wrote:
> 
> 
> 
> On 2015-11-26 16:23, Takashi Iwai wrote:
> > On Thu, 26 Nov 2015 16:08:06 +0100,
> > David Henningsson wrote:
> >>
> >>
> >>
> >> On 2015-11-26 10:24, Takashi Iwai wrote:
> >>> On Thu, 26 Nov 2015 10:16:17 +0100,
> >>> Zhang, Xiong Y wrote:
> 
> 
> > On Thu, 26 Nov 2015 08:57:30 +0100,
> > Zhang, Xiong Y wrote:
> >>
> > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > component ops to get ELD and connection states.  It was posted to
> > alsa-devel shortly ago just as a reference, but this should work 
> > well
> > in such a case, too.  The test patches are found in test/hdmi-jack
> > branch of my sound git tree.
> >>>
> >>> Did you try this branch (merge onto intel-test-nightly)?
> >>>
> >> [Zhang, Xiong Y] Yes, this branch could fix my issue.
> >
> > OK, good to hear.  But this isn't for 4.4 or backport, as it's more
> > radical changes.
> >
> > Could you check the patch below instead?  This suppresses the notifier
> > handling during suspend.  It's bad, but the hotplug should be still
> > handled via normal unsol event, so it should keep working, good enough
> > as a stop gap.
> >
> >
> > Takashi
> >
> > ---
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index bdb6f226d006..f7c70e2ae65c 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -33,6 +33,7 @@
> >#include 
> >#include 
> >#include 
> > +#include 
> >#include 
> >#include 
> >#include 
> > @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, 
> > int
> > port)
> > struct hda_codec *codec = audio_ptr;
> > int pin_nid = port + 0x04;
> >
> > -   check_presence_and_report(codec, pin_nid);
> > +   if (!atomic_read(>core.in_pm) &&
> > +   !pm_runtime_suspended(hda_codec_dev(codec)))
> > +   check_presence_and_report(codec, pin_nid);
> >}
> >
> >static int patch_generic_hdmi(struct hda_codec *codec)
>  [Zhang, Xiong Y]  this patch couldn't remove the error message. So audio 
>  controller isn't in Runtime D3 after azx_suspend().
> >>>
> >>> OK, then the problem isn't about the HD-audio driver but rather i915
> >>> driver.  As already mentioned, i915 driver shouldn't issue eld_notify
> >>> callback in the suspend code path.
> >>
> >> We don't have a complete drm log so I'm only speculating here; but the
> >> HDA log seems to indicate the power well is off when we try to talk to
> >> the HDA controller. That means either the i915 shut it down while we
> >> were holding a lock on it, or HDA tried to lock it and that didn't make
> >> it through; in which case the HDA side should handle an error from
> >> get_power more gracefully...?
> >
> > My understanding is that it's triggered *during* i915 suspend.  Then
> > the path calls the HDA callback, and HDA controller tries to get power
> > and proceeds as it has no idea that it's being shut off.
> 
> Well; that can also be stopped by letting the get_power call return a 
> failure code in case i915 is trying to suspend itself. That seems more 
> robust to me, as it could potentially avoid other S3 races too...?

Would be nice, but it's hard, because as mentioned, many paths
evaluating the return value from get_power can't stop the things
easily.

Also, one thing that came to my mind now: do we have a dependency in
suspend order between i915 and HDA?  Wouldn't it happen that i915
driver goes to suspend while HDA still active?  Then a return check
from get_power doesn't necessarily help because it might hold it.

> > Unfortunately, neither get_power callback or the corresponding HDA
> > code has a capability to check that state.  So, changing / fixing this
> > there would be nice but very hard.
> 
> Surely the i915 driver has some "am_i_suspending" variable it can check 
> in the get_power callback?

Yeah, that's I supposed in below.

> > So I believe it's easier to avoid calling the eld_notify callback from
> > i915 side during its suspend code.

And I think we need a quicker solution for now.  My patchset to use
get_eld callback removes the whole power up/down at notification, so
we won't have this issue.  Thus we need a fix only for 4.3/4.4.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Nov 26, 2015 at 04:23:16PM +0100, Takashi Iwai wrote:
> On Thu, 26 Nov 2015 16:08:06 +0100,
> David Henningsson wrote:
> > 
> > 
> > 
> > On 2015-11-26 10:24, Takashi Iwai wrote:
> > > On Thu, 26 Nov 2015 10:16:17 +0100,
> > > Zhang, Xiong Y wrote:
> > >>
> > >>
> > >>> On Thu, 26 Nov 2015 08:57:30 +0100,
> > >>> Zhang, Xiong Y wrote:
> > 
> > >>> BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > >>> component ops to get ELD and connection states.  It was posted to
> > >>> alsa-devel shortly ago just as a reference, but this should work 
> > >>> well
> > >>> in such a case, too.  The test patches are found in test/hdmi-jack
> > >>> branch of my sound git tree.
> > >
> > > Did you try this branch (merge onto intel-test-nightly)?
> > >
> >  [Zhang, Xiong Y] Yes, this branch could fix my issue.
> > >>>
> > >>> OK, good to hear.  But this isn't for 4.4 or backport, as it's more
> > >>> radical changes.
> > >>>
> > >>> Could you check the patch below instead?  This suppresses the notifier
> > >>> handling during suspend.  It's bad, but the hotplug should be still
> > >>> handled via normal unsol event, so it should keep working, good enough
> > >>> as a stop gap.
> > >>>
> > >>>
> > >>> Takashi
> > >>>
> > >>> ---
> > >>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > >>> index bdb6f226d006..f7c70e2ae65c 100644
> > >>> --- a/sound/pci/hda/patch_hdmi.c
> > >>> +++ b/sound/pci/hda/patch_hdmi.c
> > >>> @@ -33,6 +33,7 @@
> > >>>   #include 
> > >>>   #include 
> > >>>   #include 
> > >>> +#include 
> > >>>   #include 
> > >>>   #include 
> > >>>   #include 
> > >>> @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void *audio_ptr, 
> > >>> int
> > >>> port)
> > >>> struct hda_codec *codec = audio_ptr;
> > >>> int pin_nid = port + 0x04;
> > >>>
> > >>> -   check_presence_and_report(codec, pin_nid);
> > >>> +   if (!atomic_read(>core.in_pm) &&
> > >>> +   !pm_runtime_suspended(hda_codec_dev(codec)))
> > >>> +   check_presence_and_report(codec, pin_nid);
> > >>>   }
> > >>>
> > >>>   static int patch_generic_hdmi(struct hda_codec *codec)
> > >> [Zhang, Xiong Y]  this patch couldn't remove the error message. So audio 
> > >> controller isn't in Runtime D3 after azx_suspend().
> > >
> > > OK, then the problem isn't about the HD-audio driver but rather i915
> > > driver.  As already mentioned, i915 driver shouldn't issue eld_notify
> > > callback in the suspend code path.
> > 
> > We don't have a complete drm log so I'm only speculating here; but the 
> > HDA log seems to indicate the power well is off when we try to talk to 
> > the HDA controller. That means either the i915 shut it down while we 
> > were holding a lock on it, or HDA tried to lock it and that didn't make 
> > it through; in which case the HDA side should handle an error from 
> > get_power more gracefully...?
> 
> My understanding is that it's triggered *during* i915 suspend.  Then
> the path calls the HDA callback, and HDA controller tries to get power
> and proceeds as it has no idea that it's being shut off.
> 
> Unfortunately, neither get_power callback or the corresponding HDA
> code has a capability to check that state.  So, changing / fixing this
> there would be nice but very hard.
> 
> So I believe it's easier to avoid calling the eld_notify callback from
> i915 side during its suspend code.

Not calling eld_notify doesn't really help since when we suspend we do a
normal modeset. And on platforms where the eld notify interrupt stuff
works that will happen. This needs to be solved somewhere else I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Thu, 26 Nov 2015 16:43:23 +0100,
Ville Syrjälä wrote:
> 
> On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
> > 
> > 
> > On 2015-11-26 16:23, Takashi Iwai wrote:
> > > On Thu, 26 Nov 2015 16:08:06 +0100,
> > > David Henningsson wrote:
> > >>
> > >>
> > >>
> > >> On 2015-11-26 10:24, Takashi Iwai wrote:
> > >>> On Thu, 26 Nov 2015 10:16:17 +0100,
> > >>> Zhang, Xiong Y wrote:
> > 
> > 
> > > On Thu, 26 Nov 2015 08:57:30 +0100,
> > > Zhang, Xiong Y wrote:
> > >>
> > > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > > component ops to get ELD and connection states.  It was posted to
> > > alsa-devel shortly ago just as a reference, but this should work 
> > > well
> > > in such a case, too.  The test patches are found in test/hdmi-jack
> > > branch of my sound git tree.
> > >>>
> > >>> Did you try this branch (merge onto intel-test-nightly)?
> > >>>
> > >> [Zhang, Xiong Y] Yes, this branch could fix my issue.
> > >
> > > OK, good to hear.  But this isn't for 4.4 or backport, as it's more
> > > radical changes.
> > >
> > > Could you check the patch below instead?  This suppresses the notifier
> > > handling during suspend.  It's bad, but the hotplug should be still
> > > handled via normal unsol event, so it should keep working, good enough
> > > as a stop gap.
> > >
> > >
> > > Takashi
> > >
> > > ---
> > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > index bdb6f226d006..f7c70e2ae65c 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -33,6 +33,7 @@
> > >#include 
> > >#include 
> > >#include 
> > > +#include 
> > >#include 
> > >#include 
> > >#include 
> > > @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void 
> > > *audio_ptr, int
> > > port)
> > >   struct hda_codec *codec = audio_ptr;
> > >   int pin_nid = port + 0x04;
> > >
> > > - check_presence_and_report(codec, pin_nid);
> > > + if (!atomic_read(>core.in_pm) &&
> > > + !pm_runtime_suspended(hda_codec_dev(codec)))
> > > + check_presence_and_report(codec, pin_nid);
> > >}
> > >
> > >static int patch_generic_hdmi(struct hda_codec *codec)
> >  [Zhang, Xiong Y]  this patch couldn't remove the error message. So 
> >  audio controller isn't in Runtime D3 after azx_suspend().
> > >>>
> > >>> OK, then the problem isn't about the HD-audio driver but rather i915
> > >>> driver.  As already mentioned, i915 driver shouldn't issue eld_notify
> > >>> callback in the suspend code path.
> > >>
> > >> We don't have a complete drm log so I'm only speculating here; but the
> > >> HDA log seems to indicate the power well is off when we try to talk to
> > >> the HDA controller. That means either the i915 shut it down while we
> > >> were holding a lock on it, or HDA tried to lock it and that didn't make
> > >> it through; in which case the HDA side should handle an error from
> > >> get_power more gracefully...?
> > >
> > > My understanding is that it's triggered *during* i915 suspend.  Then
> > > the path calls the HDA callback, and HDA controller tries to get power
> > > and proceeds as it has no idea that it's being shut off.
> > 
> > Well; that can also be stopped by letting the get_power call return a 
> > failure code in case i915 is trying to suspend itself. That seems more 
> > robust to me, as it could potentially avoid other S3 races too...?
> > 
> > >
> > > Unfortunately, neither get_power callback or the corresponding HDA
> > > code has a capability to check that state.  So, changing / fixing this
> > > there would be nice but very hard.
> > 
> > Surely the i915 driver has some "am_i_suspending" variable it can check 
> > in the get_power callback?
> 
> I don't understand why you need to do anything special. When the eld
> notify happens during suspend, the hardware is still fully powered up,
> so it should just work(tm) as long as the eld_notify is a synchronous
> call and it drops the power reference at the end.

Hm, that's the question.  It's never clear so far as we haven't got
any detailed logs.

The symptom implies that the graphics side is off while HDA tries to
execute some verbs.  So the power well is the first suspect.  We reall
need to track down the code path triggering the issue.

> I guess for any get_power after i915 has suspended we'd need to just
> reject the get_power call. Or does something force hda to suspend before
> and resume after i915 always?

The HDA code itself calls get_power at the beginning of the resume.
But not sure whether this suffices for the execution ordering.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

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

[Ville Syrjälä]

On Thu, Nov 26, 2015 at 04:51:04PM +0100, Takashi Iwai wrote:
> On Thu, 26 Nov 2015 16:43:23 +0100,
> Ville Syrjälä wrote:
> > 
> > On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
> > > 
> > > 
> > > On 2015-11-26 16:23, Takashi Iwai wrote:
> > > > On Thu, 26 Nov 2015 16:08:06 +0100,
> > > > David Henningsson wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 2015-11-26 10:24, Takashi Iwai wrote:
> > > >>> On Thu, 26 Nov 2015 10:16:17 +0100,
> > > >>> Zhang, Xiong Y wrote:
> > > 
> > > 
> > > > On Thu, 26 Nov 2015 08:57:30 +0100,
> > > > Zhang, Xiong Y wrote:
> > > >>
> > > > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > > > component ops to get ELD and connection states.  It was posted 
> > > > to
> > > > alsa-devel shortly ago just as a reference, but this should 
> > > > work well
> > > > in such a case, too.  The test patches are found in 
> > > > test/hdmi-jack
> > > > branch of my sound git tree.
> > > >>>
> > > >>> Did you try this branch (merge onto intel-test-nightly)?
> > > >>>
> > > >> [Zhang, Xiong Y] Yes, this branch could fix my issue.
> > > >
> > > > OK, good to hear.  But this isn't for 4.4 or backport, as it's more
> > > > radical changes.
> > > >
> > > > Could you check the patch below instead?  This suppresses the 
> > > > notifier
> > > > handling during suspend.  It's bad, but the hotplug should be still
> > > > handled via normal unsol event, so it should keep working, good 
> > > > enough
> > > > as a stop gap.
> > > >
> > > >
> > > > Takashi
> > > >
> > > > ---
> > > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > > index bdb6f226d006..f7c70e2ae65c 100644
> > > > --- a/sound/pci/hda/patch_hdmi.c
> > > > +++ b/sound/pci/hda/patch_hdmi.c
> > > > @@ -33,6 +33,7 @@
> > > >#include 
> > > >#include 
> > > >#include 
> > > > +#include 
> > > >#include 
> > > >#include 
> > > >#include 
> > > > @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void 
> > > > *audio_ptr, int
> > > > port)
> > > > struct hda_codec *codec = audio_ptr;
> > > > int pin_nid = port + 0x04;
> > > >
> > > > -   check_presence_and_report(codec, pin_nid);
> > > > +   if (!atomic_read(>core.in_pm) &&
> > > > +   !pm_runtime_suspended(hda_codec_dev(codec)))
> > > > +   check_presence_and_report(codec, pin_nid);
> > > >}
> > > >
> > > >static int patch_generic_hdmi(struct hda_codec *codec)
> > >  [Zhang, Xiong Y]  this patch couldn't remove the error message. So 
> > >  audio controller isn't in Runtime D3 after azx_suspend().
> > > >>>
> > > >>> OK, then the problem isn't about the HD-audio driver but rather i915
> > > >>> driver.  As already mentioned, i915 driver shouldn't issue eld_notify
> > > >>> callback in the suspend code path.
> > > >>
> > > >> We don't have a complete drm log so I'm only speculating here; but the
> > > >> HDA log seems to indicate the power well is off when we try to talk to
> > > >> the HDA controller. That means either the i915 shut it down while we
> > > >> were holding a lock on it, or HDA tried to lock it and that didn't make
> > > >> it through; in which case the HDA side should handle an error from
> > > >> get_power more gracefully...?
> > > >
> > > > My understanding is that it's triggered *during* i915 suspend.  Then
> > > > the path calls the HDA callback, and HDA controller tries to get power
> > > > and proceeds as it has no idea that it's being shut off.
> > > 
> > > Well; that can also be stopped by letting the get_power call return a 
> > > failure code in case i915 is trying to suspend itself. That seems more 
> > > robust to me, as it could potentially avoid other S3 races too...?
> > > 
> > > >
> > > > Unfortunately, neither get_power callback or the corresponding HDA
> > > > code has a capability to check that state.  So, changing / fixing this
> > > > there would be nice but very hard.
> > > 
> > > Surely the i915 driver has some "am_i_suspending" variable it can check 
> > > in the get_power callback?
> > 
> > I don't understand why you need to do anything special. When the eld
> > notify happens during suspend, the hardware is still fully powered up,
> > so it should just work(tm) as long as the eld_notify is a synchronous
> > call and it drops the power reference at the end.
> 
> Hm, that's the question.  It's never clear so far as we haven't got
> any detailed logs.
> 
> The symptom implies that the graphics side is off while HDA tries to
> execute some verbs.  So the power well is the first suspect.  We reall
> need to track down the code path triggering the issue.
> 
> > I guess for any get_power after i915 has suspended we'd need to just
> > reject the get_power call. Or does 

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

[Takashi Iwai]

On Thu, 26 Nov 2015 16:58:09 +0100,
Ville Syrjälä wrote:
> 
> On Thu, Nov 26, 2015 at 04:51:04PM +0100, Takashi Iwai wrote:
> > On Thu, 26 Nov 2015 16:43:23 +0100,
> > Ville Syrjälä wrote:
> > > 
> > > On Thu, Nov 26, 2015 at 04:29:12PM +0100, David Henningsson wrote:
> > > > 
> > > > 
> > > > On 2015-11-26 16:23, Takashi Iwai wrote:
> > > > > On Thu, 26 Nov 2015 16:08:06 +0100,
> > > > > David Henningsson wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 2015-11-26 10:24, Takashi Iwai wrote:
> > > > >>> On Thu, 26 Nov 2015 10:16:17 +0100,
> > > > >>> Zhang, Xiong Y wrote:
> > > > 
> > > > 
> > > > > On Thu, 26 Nov 2015 08:57:30 +0100,
> > > > > Zhang, Xiong Y wrote:
> > > > >>
> > > > > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > > > > component ops to get ELD and connection states.  It was 
> > > > > posted to
> > > > > alsa-devel shortly ago just as a reference, but this should 
> > > > > work well
> > > > > in such a case, too.  The test patches are found in 
> > > > > test/hdmi-jack
> > > > > branch of my sound git tree.
> > > > >>>
> > > > >>> Did you try this branch (merge onto intel-test-nightly)?
> > > > >>>
> > > > >> [Zhang, Xiong Y] Yes, this branch could fix my issue.
> > > > >
> > > > > OK, good to hear.  But this isn't for 4.4 or backport, as it's 
> > > > > more
> > > > > radical changes.
> > > > >
> > > > > Could you check the patch below instead?  This suppresses the 
> > > > > notifier
> > > > > handling during suspend.  It's bad, but the hotplug should be 
> > > > > still
> > > > > handled via normal unsol event, so it should keep working, good 
> > > > > enough
> > > > > as a stop gap.
> > > > >
> > > > >
> > > > > Takashi
> > > > >
> > > > > ---
> > > > > diff --git a/sound/pci/hda/patch_hdmi.c 
> > > > > b/sound/pci/hda/patch_hdmi.c
> > > > > index bdb6f226d006..f7c70e2ae65c 100644
> > > > > --- a/sound/pci/hda/patch_hdmi.c
> > > > > +++ b/sound/pci/hda/patch_hdmi.c
> > > > > @@ -33,6 +33,7 @@
> > > > >#include 
> > > > >#include 
> > > > >#include 
> > > > > +#include 
> > > > >#include 
> > > > >#include 
> > > > >#include 
> > > > > @@ -2352,7 +2353,9 @@ static void intel_pin_eld_notify(void 
> > > > > *audio_ptr, int
> > > > > port)
> > > > >   struct hda_codec *codec = audio_ptr;
> > > > >   int pin_nid = port + 0x04;
> > > > >
> > > > > - check_presence_and_report(codec, pin_nid);
> > > > > + if (!atomic_read(>core.in_pm) &&
> > > > > + !pm_runtime_suspended(hda_codec_dev(codec)))
> > > > > + check_presence_and_report(codec, pin_nid);
> > > > >}
> > > > >
> > > > >static int patch_generic_hdmi(struct hda_codec *codec)
> > > >  [Zhang, Xiong Y]  this patch couldn't remove the error message. So 
> > > >  audio controller isn't in Runtime D3 after azx_suspend().
> > > > >>>
> > > > >>> OK, then the problem isn't about the HD-audio driver but rather i915
> > > > >>> driver.  As already mentioned, i915 driver shouldn't issue 
> > > > >>> eld_notify
> > > > >>> callback in the suspend code path.
> > > > >>
> > > > >> We don't have a complete drm log so I'm only speculating here; but 
> > > > >> the
> > > > >> HDA log seems to indicate the power well is off when we try to talk 
> > > > >> to
> > > > >> the HDA controller. That means either the i915 shut it down while we
> > > > >> were holding a lock on it, or HDA tried to lock it and that didn't 
> > > > >> make
> > > > >> it through; in which case the HDA side should handle an error from
> > > > >> get_power more gracefully...?
> > > > >
> > > > > My understanding is that it's triggered *during* i915 suspend.  Then
> > > > > the path calls the HDA callback, and HDA controller tries to get power
> > > > > and proceeds as it has no idea that it's being shut off.
> > > > 
> > > > Well; that can also be stopped by letting the get_power call return a 
> > > > failure code in case i915 is trying to suspend itself. That seems more 
> > > > robust to me, as it could potentially avoid other S3 races too...?
> > > > 
> > > > >
> > > > > Unfortunately, neither get_power callback or the corresponding HDA
> > > > > code has a capability to check that state.  So, changing / fixing this
> > > > > there would be nice but very hard.
> > > > 
> > > > Surely the i915 driver has some "am_i_suspending" variable it can check 
> > > > in the get_power callback?
> > > 
> > > I don't understand why you need to do anything special. When the eld
> > > notify happens during suspend, the hardware is still fully powered up,
> > > so it should just work(tm) as long as the eld_notify is a synchronous
> > > call and it drops the power reference at the end.
> > 
> > Hm, that's the question.  It's never 

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

[Takashi Iwai]

On Thu, 26 Nov 2015 07:06:56 +0100,
Zhang, Xiong Y wrote:
> 
> > On Wed, 25 Nov 2015 11:57:13 +0100,
> > Zhang, Xiong Y wrote:
> > >
> > > > On Wed, 25 Nov 2015 10:56:51 +0100,
> > > > Zhang, Xiong Y wrote:
> > > > >
> > > > > Recently I always see the following error message during S4 or S3 
> > > > > resume
> > > > with drm-intel-nightly.
> > > > > [   97.778063] PM: Syncing filesystems ... done.
> > > > > [   97.801550] Freezing user space processes ... (elapsed 0.002 
> > > > > seconds)
> > > > done.
> > > > > [   97.804297] PM: Marking nosave pages: [mem
> > 0x-0x0fff]
> > > > > [   97.804302] PM: Marking nosave pages: [mem
> > 0x00058000-0x00058fff]
> > > > > [   97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f]
> > > > > [   97.804310] PM: Marking nosave pages: [mem
> > 0x84c11000-0x84c12fff]
> > > > > [   97.804312] PM: Marking nosave pages: [mem
> > 0x876fc000-0x87746fff]
> > > > > [   97.804317] PM: Marking nosave pages: [mem
> > 0x8785e000-0x87fe9fff]
> > > > > [   97.804387] PM: Marking nosave pages: [mem 0x8800-0x]
> > > > > [   97.806363] PM: Basic memory bitmaps created
> > > > > [   97.806409] PM: Preallocating image memory... done (allocated
> > 321557
> > > > pages)
> > > > > [   98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02
> > > > MB/s)
> > > > > [   98.150476] Freezing remaining freezable tasks ... (elapsed 0.001
> > seconds)
> > > > done.
> > > > > [   98.151998] Suspending console(s) (use no_console_suspend to
> > debug)
> > > > > [   98.173485] hdmi_present_sense: snd_hda_codec_hdmi
> > hdaudioC0D2:
> > > > HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
> > > > > [   99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> > last
> > > > cmd=0x206f2e08
> > > > > [   99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size
> > is 0,
> > > > force 128
> > > > > [  101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout,
> > > > switching to polling mode: last cmd=0x206f2f00
> > > > > [  102.195492] snd_hda_intel :00:1f.3: No response from codec,
> > > > disabling MSI: last cmd=0x206f2f00
> > > > > [  103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout,
> > > > switching to single_cmd mode: last cmd=0x206f2f00
> > > > > [  103.201396] azx_single_wait_for_response: 42 callbacks suppressed
> > > > >
> > > > > The bisect result points to this commit.
> > > > > I checked this patch and had one question: if i915 driver wake up 
> > > > > ahead of
> > > > snd_hda_intel driver during resume,  i915 driver will call audio 
> > > > driver's
> > > > hdmi_present_sense() function through this patch, but the audio 
> > > > interrupt
> > is
> > > > disabled at this moment, how could hdmi_present_sense() get the
> > response
> > > > from codec ?
> > > >
> > > > Yeah, a bad thing happens there.  The fix should be simple like below,
> > > > though.  Basically the pins are checked in the resume callback in
> > > > anyway, so there is no need to handle the notification during PM.
> > > >
> > > > Could you check whether it works?
> > > >
> > > >
> > > > thanks,
> > > >
> > > > Takashi
> > >
> > > Strange, your patch couldn't get away the error message.
> > 
> > OK, then it's not about the race *during* the hd-audio driver
> > resuming or such.  I guess it's the other way round: the i915 driver
> > notifies it unnecessarily while it's getting off.  The audio part of
> > HDMI controller relies on the i915 power state, so it's screwed up.
> > 
> > Check with drm.debug option to see in which code path it's triggered.
> > If it's a call in i915 suspend, the call should be removed not to wake
> > up the audio part unnecessarily.
> 
> [Zhang, Xiong Y] it is called in intel_audio_codec_disable() from i915 
> suspend.

So the call of eld_notifier should be suppressed at suspend.

> ---
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index bdb6f226d006..b468fe0e6195 100644
> --- 

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

[Takashi Iwai]

On Wed, 25 Nov 2015 10:56:51 +0100,
Zhang, Xiong Y wrote:
> 
> Recently I always see the following error message during S4 or S3 resume with 
> drm-intel-nightly.
> [   97.778063] PM: Syncing filesystems ... done.
> [   97.801550] Freezing user space processes ... (elapsed 0.002 seconds) done.
> [   97.804297] PM: Marking nosave pages: [mem 0x-0x0fff]
> [   97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff]
> [   97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f]
> [   97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff]
> [   97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff]
> [   97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff]
> [   97.804387] PM: Marking nosave pages: [mem 0x8800-0x]
> [   97.806363] PM: Basic memory bitmaps created
> [   97.806409] PM: Preallocating image memory... done (allocated 321557 pages)
> [   98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 MB/s)
> [   98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) 
> done.
> [   98.151998] Suspending console(s) (use no_console_suspend to debug)
> [   98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI 
> status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
> [   99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
> cmd=0x206f2e08
> [   99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 
> 128
> [  101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> switching to polling mode: last cmd=0x206f2f00
> [  102.195492] snd_hda_intel :00:1f.3: No response from codec, disabling 
> MSI: last cmd=0x206f2f00
> [  103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout, 
> switching to single_cmd mode: last cmd=0x206f2f00
> [  103.201396] azx_single_wait_for_response: 42 callbacks suppressed
> 
> The bisect result points to this commit. 
> I checked this patch and had one question: if i915 driver wake up ahead of 
> snd_hda_intel driver during resume,  i915 driver will call audio driver's 
> hdmi_present_sense() function through this patch, but the audio interrupt is 
> disabled at this moment, how could hdmi_present_sense() get the response from 
> codec ?  

Yeah, a bad thing happens there.  The fix should be simple like below,
though.  Basically the pins are checked in the resume callback in
anyway, so there is no need to handle the notification during PM.

Could you check whether it works?


thanks,

Takashi

---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index bdb6f226d006..b468fe0e6195 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2352,7 +2352,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int 
port)
struct hda_codec *codec = audio_ptr;
int pin_nid = port + 0x04;
 
-   check_presence_and_report(codec, pin_nid);
+   /* don't call notifier during PM; will be checked in resume callback */
+   if (!atomic_read(>core.in_pm))
+   check_presence_and_report(codec, pin_nid);
 }
 
 static int patch_generic_hdmi(struct hda_codec *codec)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhang, Xiong Y]

Recently I always see the following error message during S4 or S3 resume with 
drm-intel-nightly.
[   97.778063] PM: Syncing filesystems ... done.
[   97.801550] Freezing user space processes ... (elapsed 0.002 seconds) done.
[   97.804297] PM: Marking nosave pages: [mem 0x-0x0fff]
[   97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff]
[   97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f]
[   97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff]
[   97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff]
[   97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff]
[   97.804387] PM: Marking nosave pages: [mem 0x8800-0x]
[   97.806363] PM: Basic memory bitmaps created
[   97.806409] PM: Preallocating image memory... done (allocated 321557 pages)
[   98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 MB/s)
[   98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) 
done.
[   98.151998] Suspending console(s) (use no_console_suspend to debug)
[   98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: 
Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
[   99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last 
cmd=0x206f2e08
[   99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 
128
[  101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout, switching 
to polling mode: last cmd=0x206f2f00
[  102.195492] snd_hda_intel :00:1f.3: No response from codec, disabling 
MSI: last cmd=0x206f2f00
[  103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout, switching 
to single_cmd mode: last cmd=0x206f2f00
[  103.201396] azx_single_wait_for_response: 42 callbacks suppressed

The bisect result points to this commit. 
I checked this patch and had one question: if i915 driver wake up ahead of 
snd_hda_intel driver during resume,  i915 driver will call audio driver's 
hdmi_present_sense() function through this patch, but the audio interrupt is 
disabled at this moment, how could hdmi_present_sense() get the response from 
codec ?  

thanks
> -Original Message-
> From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
> David Henningsson
> Sent: Saturday, August 29, 2015 1:03 AM
> To: ti...@suse.de; alsa-de...@alsa-project.org;
> intel-gfx@lists.freedesktop.org; jani.nik...@linux.intel.com; Yang, Libin; 
> Vetter,
> Daniel
> Cc: David Henningsson
> Subject: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD 
> notify
> events
> 
> Whenever there is an event from the i915 driver, wake the codec
> and recheck plug/unplug + ELD status.
> 
> This fixes the issue with lost unsol events in power save mode,
> the codec and controller can now sleep in D3 and still know when
> the HDMI monitor has been connected.
> 
> Right now, this might mean we get two callbacks from the same event,
> one from the unsol event and one from the i915 driver, but this is
> not harmful and can be optimised away in a later patch.
> 
> Reviewed-by: Takashi Iwai <ti...@suse.de>
> Signed-off-by: David Henningsson <david.hennings...@canonical.com>
> ---
>  sound/pci/hda/patch_hdmi.c | 22 +-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index a97db5f..acbfbe0 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -37,6 +37,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include "hda_codec.h"
>  #include "hda_local.h"
>  #include "hda_jack.h"
> @@ -144,6 +146,9 @@ struct hdmi_spec {
>*/
>   struct hda_multi_out multiout;
>   struct hda_pcm_stream pcm_playback;
> +
> + /* i915/powerwell (Haswell+/Valleyview+) specific */
> + struct i915_audio_component_audio_ops i915_audio_ops;
>  };
> 
> 
> @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda

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

[Zhang, Xiong Y]

> On Wed, 25 Nov 2015 10:56:51 +0100,
> Zhang, Xiong Y wrote:
> >
> > Recently I always see the following error message during S4 or S3 resume
> with drm-intel-nightly.
> > [   97.778063] PM: Syncing filesystems ... done.
> > [   97.801550] Freezing user space processes ... (elapsed 0.002 seconds)
> done.
> > [   97.804297] PM: Marking nosave pages: [mem 0x-0x0fff]
> > [   97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff]
> > [   97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f]
> > [   97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff]
> > [   97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff]
> > [   97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff]
> > [   97.804387] PM: Marking nosave pages: [mem 0x8800-0x]
> > [   97.806363] PM: Basic memory bitmaps created
> > [   97.806409] PM: Preallocating image memory... done (allocated 321557
> pages)
> > [   98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02
> MB/s)
> > [   98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 
> > seconds)
> done.
> > [   98.151998] Suspending console(s) (use no_console_suspend to debug)
> > [   98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2:
> HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
> > [   99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> cmd=0x206f2e08
> > [   99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0,
> force 128
> > [  101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout,
> switching to polling mode: last cmd=0x206f2f00
> > [  102.195492] snd_hda_intel :00:1f.3: No response from codec,
> disabling MSI: last cmd=0x206f2f00
> > [  103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout,
> switching to single_cmd mode: last cmd=0x206f2f00
> > [  103.201396] azx_single_wait_for_response: 42 callbacks suppressed
> >
> > The bisect result points to this commit.
> > I checked this patch and had one question: if i915 driver wake up ahead of
> snd_hda_intel driver during resume,  i915 driver will call audio driver's
> hdmi_present_sense() function through this patch, but the audio interrupt is
> disabled at this moment, how could hdmi_present_sense() get the response
> from codec ?
> 
> Yeah, a bad thing happens there.  The fix should be simple like below,
> though.  Basically the pins are checked in the resume callback in
> anyway, so there is no need to handle the notification during PM.
> 
> Could you check whether it works?
> 
> 
> thanks,
> 
> Takashi

Strange, your patch couldn't get away the error message.

thanks
> 
> ---
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index bdb6f226d006..b468fe0e6195 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -2352,7 +2352,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int
> port)
>   struct hda_codec *codec = audio_ptr;
>   int pin_nid = port + 0x04;
> 
> - check_presence_and_report(codec, pin_nid);
> + /* don't call notifier during PM; will be checked in resume callback */
> + if (!atomic_read(>core.in_pm))
> + check_presence_and_report(codec, pin_nid);
>  }
> 
>  static int patch_generic_hdmi(struct hda_codec *codec)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Wed, 25 Nov 2015 11:57:13 +0100,
Zhang, Xiong Y wrote:
> 
> > On Wed, 25 Nov 2015 10:56:51 +0100,
> > Zhang, Xiong Y wrote:
> > >
> > > Recently I always see the following error message during S4 or S3 resume
> > with drm-intel-nightly.
> > > [   97.778063] PM: Syncing filesystems ... done.
> > > [   97.801550] Freezing user space processes ... (elapsed 0.002 seconds)
> > done.
> > > [   97.804297] PM: Marking nosave pages: [mem 0x-0x0fff]
> > > [   97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff]
> > > [   97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f]
> > > [   97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff]
> > > [   97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff]
> > > [   97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff]
> > > [   97.804387] PM: Marking nosave pages: [mem 0x8800-0x]
> > > [   97.806363] PM: Basic memory bitmaps created
> > > [   97.806409] PM: Preallocating image memory... done (allocated 321557
> > pages)
> > > [   98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02
> > MB/s)
> > > [   98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 
> > > seconds)
> > done.
> > > [   98.151998] Suspending console(s) (use no_console_suspend to debug)
> > > [   98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2:
> > HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
> > > [   99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last
> > cmd=0x206f2e08
> > > [   99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0,
> > force 128
> > > [  101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout,
> > switching to polling mode: last cmd=0x206f2f00
> > > [  102.195492] snd_hda_intel :00:1f.3: No response from codec,
> > disabling MSI: last cmd=0x206f2f00
> > > [  103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout,
> > switching to single_cmd mode: last cmd=0x206f2f00
> > > [  103.201396] azx_single_wait_for_response: 42 callbacks suppressed
> > >
> > > The bisect result points to this commit.
> > > I checked this patch and had one question: if i915 driver wake up ahead of
> > snd_hda_intel driver during resume,  i915 driver will call audio driver's
> > hdmi_present_sense() function through this patch, but the audio interrupt is
> > disabled at this moment, how could hdmi_present_sense() get the response
> > from codec ?
> > 
> > Yeah, a bad thing happens there.  The fix should be simple like below,
> > though.  Basically the pins are checked in the resume callback in
> > anyway, so there is no need to handle the notification during PM.
> > 
> > Could you check whether it works?
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> Strange, your patch couldn't get away the error message.

OK, then it's not about the race *during* the hd-audio driver
resuming or such.  I guess it's the other way round: the i915 driver
notifies it unnecessarily while it's getting off.  The audio part of
HDMI controller relies on the i915 power state, so it's screwed up.

Check with drm.debug option to see in which code path it's triggered.
If it's a call in i915 suspend, the call should be removed not to wake
up the audio part unnecessarily.

BTW, I have a patchset to avoid the audio h/w wakeup by a new
component ops to get ELD and connection states.  It was posted to
alsa-devel shortly ago just as a reference, but this should work well
in such a case, too.  The test patches are found in test/hdmi-jack
branch of my sound git tree.


Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhang, Xiong Y]

> > > BTW, I have a patchset to avoid the audio h/w wakeup by a new
> > > component ops to get ELD and connection states.  It was posted to
> > > alsa-devel shortly ago just as a reference, but this should work well
> > > in such a case, too.  The test patches are found in test/hdmi-jack
> > > branch of my sound git tree.
> 
> Did you try this branch (merge onto intel-test-nightly)?
> 
[Zhang, Xiong Y] Yes, this branch could fix my issue.

> 
> Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Zhang, Xiong Y]

> On Wed, 25 Nov 2015 11:57:13 +0100,
> Zhang, Xiong Y wrote:
> >
> > > On Wed, 25 Nov 2015 10:56:51 +0100,
> > > Zhang, Xiong Y wrote:
> > > >
> > > > Recently I always see the following error message during S4 or S3 resume
> > > with drm-intel-nightly.
> > > > [   97.778063] PM: Syncing filesystems ... done.
> > > > [   97.801550] Freezing user space processes ... (elapsed 0.002 seconds)
> > > done.
> > > > [   97.804297] PM: Marking nosave pages: [mem
> 0x-0x0fff]
> > > > [   97.804302] PM: Marking nosave pages: [mem
> 0x00058000-0x00058fff]
> > > > [   97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f]
> > > > [   97.804310] PM: Marking nosave pages: [mem
> 0x84c11000-0x84c12fff]
> > > > [   97.804312] PM: Marking nosave pages: [mem
> 0x876fc000-0x87746fff]
> > > > [   97.804317] PM: Marking nosave pages: [mem
> 0x8785e000-0x87fe9fff]
> > > > [   97.804387] PM: Marking nosave pages: [mem 0x8800-0x]
> > > > [   97.806363] PM: Basic memory bitmaps created
> > > > [   97.806409] PM: Preallocating image memory... done (allocated
> 321557
> > > pages)
> > > > [   98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02
> > > MB/s)
> > > > [   98.150476] Freezing remaining freezable tasks ... (elapsed 0.001
> seconds)
> > > done.
> > > > [   98.151998] Suspending console(s) (use no_console_suspend to
> debug)
> > > > [   98.173485] hdmi_present_sense: snd_hda_codec_hdmi
> hdaudioC0D2:
> > > HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1
> > > > [   99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2,
> last
> > > cmd=0x206f2e08
> > > > [   99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size
> is 0,
> > > force 128
> > > > [  101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout,
> > > switching to polling mode: last cmd=0x206f2f00
> > > > [  102.195492] snd_hda_intel :00:1f.3: No response from codec,
> > > disabling MSI: last cmd=0x206f2f00
> > > > [  103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout,
> > > switching to single_cmd mode: last cmd=0x206f2f00
> > > > [  103.201396] azx_single_wait_for_response: 42 callbacks suppressed
> > > >
> > > > The bisect result points to this commit.
> > > > I checked this patch and had one question: if i915 driver wake up ahead 
> > > > of
> > > snd_hda_intel driver during resume,  i915 driver will call audio driver's
> > > hdmi_present_sense() function through this patch, but the audio interrupt
> is
> > > disabled at this moment, how could hdmi_present_sense() get the
> response
> > > from codec ?
> > >
> > > Yeah, a bad thing happens there.  The fix should be simple like below,
> > > though.  Basically the pins are checked in the resume callback in
> > > anyway, so there is no need to handle the notification during PM.
> > >
> > > Could you check whether it works?
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> >
> > Strange, your patch couldn't get away the error message.
> 
> OK, then it's not about the race *during* the hd-audio driver
> resuming or such.  I guess it's the other way round: the i915 driver
> notifies it unnecessarily while it's getting off.  The audio part of
> HDMI controller relies on the i915 power state, so it's screwed up.
> 
> Check with drm.debug option to see in which code path it's triggered.
> If it's a call in i915 suspend, the call should be removed not to wake
> up the audio part unnecessarily.

[Zhang, Xiong Y] it is called in intel_audio_codec_disable() from i915 suspend.
---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index bdb6f226d006..b468fe0e6195 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2352,7 +2352,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int 
port)
struct hda_codec *codec = audio_ptr;
int pin_nid = port + 0x04;
 
-   check_presence_and_report(codec, pin_nid);
+   /* don't call notifier during PM; will be checked in resume callback */
+   if (atomic_read(>core.in_pm))
+   

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

[Daniel Vetter]

On Wed, Sep 02, 2015 at 10:03:55AM +0200, Takashi Iwai wrote:
> On Wed, 02 Sep 2015 10:00:44 +0200,
> Daniel Vetter wrote:
> > 
> > On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
> > > On Thu, 20 Aug 2015, Takashi Iwai  wrote:
> > > > On Thu, 20 Aug 2015 11:41:42 +0200,
> > > > David Henningsson wrote:
> > > >> 
> > > >> 
> > > >> 
> > > >> On 2015-08-20 11:28, Takashi Iwai wrote:
> > > >> > On Wed, 19 Aug 2015 10:48:58 +0200,
> > > >> > David Henningsson wrote:
> > > >> >>
> > > >> >> Whenever there is an event from the i915 driver, wake the codec
> > > >> >> and recheck plug/unplug + ELD status.
> > > >> >>
> > > >> >> This fixes the issue with lost unsol events in power save mode,
> > > >> >> the codec and controller can now sleep in D3 and still know when
> > > >> >> the HDMI monitor has been connected.
> > > >> >>
> > > >> >> Signed-off-by: David Henningsson 
> > > >> >
> > > >> > This addition looks fine, but then we'll get double notification for
> > > >> > the normal hotplug/unplug, one via component ops and another via 
> > > >> > unsol
> > > >> > event?
> > > >> 
> > > >> Right, in case the unsol event actually works...
> > > >> 
> > > >> I would argue that the normal case would be that the controller and 
> > > >> codec is in D3 which means that the unsol event never gets through - 
> > > >> due 
> > > >> to hw limitations - which is what triggered this patch set in the 
> > > >> first 
> > > >> place.
> > > >> 
> > > >> But yes, in some case we might get double notification, but this 
> > > >> should 
> > > >> not cause any trouble in practice. The unsol event could be turned 
> > > >> off, 
> > > >> but would it be okay to save that for a later patch set (so I don't 
> > > >> miss 
> > > >> the upcoming merge window)?
> > > >
> > > > In that case, it should be mentioned in the changelog at least.
> > > >
> > > > This series came a bit too late for the merge window, so I'm not sure
> > > > whether this can get in.  I personally find it OK, so take my ack for
> > > > ALSA parts (patch 3/4), but the rest need review and ack from i915
> > > > guys.  And we don't know who to merge this, if any.  The changes are
> > > > almost even to i915 and hda.  I don't mind either way, via drm or
> > > > sound tree.
> > > 
> > > Personally I'm fine with this still going for v4.3 since to me it looks
> > > like any regressions this might cause will be on your side. *grin*.
> > 
> > The only bit I want is some decent kerneldoc for the ad-hoc growing
> > i915/hda-intel interfaces. But that can be done in follow-up patches and
> > needs to be coordinated with the audio rate handling series anyway. So ack
> > from my side for all getting merged through alsa trees too.
> 
> Are you going to merge Libin's patchset?  If yes, it's better to align
> both patchsets through a single tree.  It'll make merges easier, as
> both touch the similar place.

Oh that was an ack for merging it all through your tree. If you punt it to
4.4 I might ask you for a stable tree to pull in in case I get conflicts.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
> On Thu, 20 Aug 2015, Takashi Iwai  wrote:
> > On Thu, 20 Aug 2015 11:41:42 +0200,
> > David Henningsson wrote:
> >> 
> >> 
> >> 
> >> On 2015-08-20 11:28, Takashi Iwai wrote:
> >> > On Wed, 19 Aug 2015 10:48:58 +0200,
> >> > David Henningsson wrote:
> >> >>
> >> >> Whenever there is an event from the i915 driver, wake the codec
> >> >> and recheck plug/unplug + ELD status.
> >> >>
> >> >> This fixes the issue with lost unsol events in power save mode,
> >> >> the codec and controller can now sleep in D3 and still know when
> >> >> the HDMI monitor has been connected.
> >> >>
> >> >> Signed-off-by: David Henningsson 
> >> >
> >> > This addition looks fine, but then we'll get double notification for
> >> > the normal hotplug/unplug, one via component ops and another via unsol
> >> > event?
> >> 
> >> Right, in case the unsol event actually works...
> >> 
> >> I would argue that the normal case would be that the controller and 
> >> codec is in D3 which means that the unsol event never gets through - due 
> >> to hw limitations - which is what triggered this patch set in the first 
> >> place.
> >> 
> >> But yes, in some case we might get double notification, but this should 
> >> not cause any trouble in practice. The unsol event could be turned off, 
> >> but would it be okay to save that for a later patch set (so I don't miss 
> >> the upcoming merge window)?
> >
> > In that case, it should be mentioned in the changelog at least.
> >
> > This series came a bit too late for the merge window, so I'm not sure
> > whether this can get in.  I personally find it OK, so take my ack for
> > ALSA parts (patch 3/4), but the rest need review and ack from i915
> > guys.  And we don't know who to merge this, if any.  The changes are
> > almost even to i915 and hda.  I don't mind either way, via drm or
> > sound tree.
> 
> Personally I'm fine with this still going for v4.3 since to me it looks
> like any regressions this might cause will be on your side. *grin*.

The only bit I want is some decent kerneldoc for the ad-hoc growing
i915/hda-intel interfaces. But that can be done in follow-up patches and
needs to be coordinated with the audio rate handling series anyway. So ack
from my side for all getting merged through alsa trees too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Wed, 02 Sep 2015 10:00:44 +0200,
Daniel Vetter wrote:
> 
> On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
> > On Thu, 20 Aug 2015, Takashi Iwai  wrote:
> > > On Thu, 20 Aug 2015 11:41:42 +0200,
> > > David Henningsson wrote:
> > >> 
> > >> 
> > >> 
> > >> On 2015-08-20 11:28, Takashi Iwai wrote:
> > >> > On Wed, 19 Aug 2015 10:48:58 +0200,
> > >> > David Henningsson wrote:
> > >> >>
> > >> >> Whenever there is an event from the i915 driver, wake the codec
> > >> >> and recheck plug/unplug + ELD status.
> > >> >>
> > >> >> This fixes the issue with lost unsol events in power save mode,
> > >> >> the codec and controller can now sleep in D3 and still know when
> > >> >> the HDMI monitor has been connected.
> > >> >>
> > >> >> Signed-off-by: David Henningsson 
> > >> >
> > >> > This addition looks fine, but then we'll get double notification for
> > >> > the normal hotplug/unplug, one via component ops and another via unsol
> > >> > event?
> > >> 
> > >> Right, in case the unsol event actually works...
> > >> 
> > >> I would argue that the normal case would be that the controller and 
> > >> codec is in D3 which means that the unsol event never gets through - due 
> > >> to hw limitations - which is what triggered this patch set in the first 
> > >> place.
> > >> 
> > >> But yes, in some case we might get double notification, but this should 
> > >> not cause any trouble in practice. The unsol event could be turned off, 
> > >> but would it be okay to save that for a later patch set (so I don't miss 
> > >> the upcoming merge window)?
> > >
> > > In that case, it should be mentioned in the changelog at least.
> > >
> > > This series came a bit too late for the merge window, so I'm not sure
> > > whether this can get in.  I personally find it OK, so take my ack for
> > > ALSA parts (patch 3/4), but the rest need review and ack from i915
> > > guys.  And we don't know who to merge this, if any.  The changes are
> > > almost even to i915 and hda.  I don't mind either way, via drm or
> > > sound tree.
> > 
> > Personally I'm fine with this still going for v4.3 since to me it looks
> > like any regressions this might cause will be on your side. *grin*.
> 
> The only bit I want is some decent kerneldoc for the ad-hoc growing
> i915/hda-intel interfaces. But that can be done in follow-up patches and
> needs to be coordinated with the audio rate handling series anyway. So ack
> from my side for all getting merged through alsa trees too.

Are you going to merge Libin's patchset?  If yes, it's better to align
both patchsets through a single tree.  It'll make merges easier, as
both touch the similar place.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Wed, 02 Sep 2015 10:32:34 +0200,
Daniel Vetter wrote:
> 
> On Wed, Sep 02, 2015 at 10:03:55AM +0200, Takashi Iwai wrote:
> > On Wed, 02 Sep 2015 10:00:44 +0200,
> > Daniel Vetter wrote:
> > > 
> > > On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
> > > > On Thu, 20 Aug 2015, Takashi Iwai  wrote:
> > > > > On Thu, 20 Aug 2015 11:41:42 +0200,
> > > > > David Henningsson wrote:
> > > > >> 
> > > > >> 
> > > > >> 
> > > > >> On 2015-08-20 11:28, Takashi Iwai wrote:
> > > > >> > On Wed, 19 Aug 2015 10:48:58 +0200,
> > > > >> > David Henningsson wrote:
> > > > >> >>
> > > > >> >> Whenever there is an event from the i915 driver, wake the codec
> > > > >> >> and recheck plug/unplug + ELD status.
> > > > >> >>
> > > > >> >> This fixes the issue with lost unsol events in power save mode,
> > > > >> >> the codec and controller can now sleep in D3 and still know when
> > > > >> >> the HDMI monitor has been connected.
> > > > >> >>
> > > > >> >> Signed-off-by: David Henningsson 
> > > > >> >
> > > > >> > This addition looks fine, but then we'll get double notification 
> > > > >> > for
> > > > >> > the normal hotplug/unplug, one via component ops and another via 
> > > > >> > unsol
> > > > >> > event?
> > > > >> 
> > > > >> Right, in case the unsol event actually works...
> > > > >> 
> > > > >> I would argue that the normal case would be that the controller and 
> > > > >> codec is in D3 which means that the unsol event never gets through - 
> > > > >> due 
> > > > >> to hw limitations - which is what triggered this patch set in the 
> > > > >> first 
> > > > >> place.
> > > > >> 
> > > > >> But yes, in some case we might get double notification, but this 
> > > > >> should 
> > > > >> not cause any trouble in practice. The unsol event could be turned 
> > > > >> off, 
> > > > >> but would it be okay to save that for a later patch set (so I don't 
> > > > >> miss 
> > > > >> the upcoming merge window)?
> > > > >
> > > > > In that case, it should be mentioned in the changelog at least.
> > > > >
> > > > > This series came a bit too late for the merge window, so I'm not sure
> > > > > whether this can get in.  I personally find it OK, so take my ack for
> > > > > ALSA parts (patch 3/4), but the rest need review and ack from i915
> > > > > guys.  And we don't know who to merge this, if any.  The changes are
> > > > > almost even to i915 and hda.  I don't mind either way, via drm or
> > > > > sound tree.
> > > > 
> > > > Personally I'm fine with this still going for v4.3 since to me it looks
> > > > like any regressions this might cause will be on your side. *grin*.
> > > 
> > > The only bit I want is some decent kerneldoc for the ad-hoc growing
> > > i915/hda-intel interfaces. But that can be done in follow-up patches and
> > > needs to be coordinated with the audio rate handling series anyway. So ack
> > > from my side for all getting merged through alsa trees too.
> > 
> > Are you going to merge Libin's patchset?  If yes, it's better to align
> > both patchsets through a single tree.  It'll make merges easier, as
> > both touch the similar place.
> 
> Oh that was an ack for merging it all through your tree. If you punt it to
> 4.4 I might ask you for a stable tree to pull in in case I get conflicts.

OK, then I'll merge David's patchset now for the upcoming pull request
for 4.3-rc1.


thanks,

Takashi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 20 Aug 2015, Takashi Iwai ti...@suse.de wrote:
 On Thu, 20 Aug 2015 11:41:42 +0200,
 David Henningsson wrote:
 
 
 
 On 2015-08-20 11:28, Takashi Iwai wrote:
  On Wed, 19 Aug 2015 10:48:58 +0200,
  David Henningsson wrote:
 
  Whenever there is an event from the i915 driver, wake the codec
  and recheck plug/unplug + ELD status.
 
  This fixes the issue with lost unsol events in power save mode,
  the codec and controller can now sleep in D3 and still know when
  the HDMI monitor has been connected.
 
  Signed-off-by: David Henningsson david.hennings...@canonical.com
 
  This addition looks fine, but then we'll get double notification for
  the normal hotplug/unplug, one via component ops and another via unsol
  event?
 
 Right, in case the unsol event actually works...
 
 I would argue that the normal case would be that the controller and 
 codec is in D3 which means that the unsol event never gets through - due 
 to hw limitations - which is what triggered this patch set in the first 
 place.
 
 But yes, in some case we might get double notification, but this should 
 not cause any trouble in practice. The unsol event could be turned off, 
 but would it be okay to save that for a later patch set (so I don't miss 
 the upcoming merge window)?

 In that case, it should be mentioned in the changelog at least.

 This series came a bit too late for the merge window, so I'm not sure
 whether this can get in.  I personally find it OK, so take my ack for
 ALSA parts (patch 3/4), but the rest need review and ack from i915
 guys.  And we don't know who to merge this, if any.  The changes are
 almost even to i915 and hda.  I don't mind either way, via drm or
 sound tree.

Personally I'm fine with this still going for v4.3 since to me it looks
like any regressions this might cause will be on your side. *grin*.

BR,
Jani.



 In anyway,
Reviewed-by: Takashi Iwai ti...@suse.de


 thanks,

 Takashi

 
 
  thanks,
 
  Takashi
 
  ---
sound/pci/hda/patch_hdmi.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
 
  diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
  index a97db5f..932292c 100644
  --- a/sound/pci/hda/patch_hdmi.c
  +++ b/sound/pci/hda/patch_hdmi.c
  @@ -37,6 +37,8 @@
#include sound/jack.h
#include sound/asoundef.h
#include sound/tlv.h
  +#include sound/hdaudio.h
  +#include sound/hda_i915.h
#include hda_codec.h
#include hda_local.h
#include hda_jack.h
  @@ -144,6 +146,9 @@ struct hdmi_spec {
 */
struct hda_multi_out multiout;
struct hda_pcm_stream pcm_playback;
  +
  + /* i915/powerwell (Haswell+/Valleyview+) specific */
  + struct i915_audio_component_audio_ops i915_audio_ops;
};
 
 
  @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec 
  *codec)
struct hdmi_spec *spec = codec-spec;
int pin_idx;
 
  + if (is_haswell_plus(codec) || is_valleyview_plus(codec))
  + snd_hdac_i915_register_notifier(NULL);
  +
for (pin_idx = 0; pin_idx  spec-num_pins; pin_idx++) {
struct hdmi_spec_per_pin *per_pin = get_pin(spec, 
  pin_idx);
 
  @@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct 
  hda_codec *codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
}
 
  +static void intel_pin_eld_notify(void *audio_ptr, int port, int 
  port_mst_index)
  +{
  + struct hda_codec *codec = audio_ptr;
  + int pin_nid = port + 0x04;
  +
  + check_presence_and_report(codec, pin_nid);
  +}
  +
static int patch_generic_hdmi(struct hda_codec *codec)
{
struct hdmi_spec *spec;
  @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec 
  *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;
 
  - if (is_haswell_plus(codec) || is_valleyview_plus(codec))
  + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
  + spec-i915_audio_ops.audio_ptr = codec;
  + spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
  + snd_hdac_i915_register_notifier(spec-i915_audio_ops);
  + }
 
if (hdmi_parse_codec(codec)  0) {
codec-spec = NULL;
  --
  1.9.1
 
 
 
 -- 
 David Henningsson, Canonical Ltd.
 https://launchpad.net/~diwic
 

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Henningsson]

Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.

This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.

Right now, this might mean we get two callbacks from the same event,
one from the unsol event and one from the i915 driver, but this is
not harmful and can be optimised away in a later patch.

Reviewed-by: Takashi Iwai ti...@suse.de
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
 sound/pci/hda/patch_hdmi.c | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index a97db5f..acbfbe0 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -37,6 +37,8 @@
 #include sound/jack.h
 #include sound/asoundef.h
 #include sound/tlv.h
+#include sound/hdaudio.h
+#include sound/hda_i915.h
 #include hda_codec.h
 #include hda_local.h
 #include hda_jack.h
@@ -144,6 +146,9 @@ struct hdmi_spec {
 */
struct hda_multi_out multiout;
struct hda_pcm_stream pcm_playback;
+
+   /* i915/powerwell (Haswell+/Valleyview+) specific */
+   struct i915_audio_component_audio_ops i915_audio_ops;
 };
 
 
@@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
struct hdmi_spec *spec = codec-spec;
int pin_idx;
 
+   if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+   snd_hdac_i915_register_notifier(NULL);
+
for (pin_idx = 0; pin_idx  spec-num_pins; pin_idx++) {
struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
 
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec 
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
 }
 
+static void intel_pin_eld_notify(void *audio_ptr, int port)
+{
+   struct hda_codec *codec = audio_ptr;
+   int pin_nid = port + 0x04;
+
+   check_presence_and_report(codec, pin_nid);
+}
+
 static int patch_generic_hdmi(struct hda_codec *codec)
 {
struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;
 
-   if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+   if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
+   spec-i915_audio_ops.audio_ptr = codec;
+   spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
+   snd_hdac_i915_register_notifier(spec-i915_audio_ops);
+   }
 
if (hdmi_parse_codec(codec)  0) {
codec-spec = NULL;
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Thu, 20 Aug 2015 11:41:42 +0200,
David Henningsson wrote:
 
 
 
 On 2015-08-20 11:28, Takashi Iwai wrote:
  On Wed, 19 Aug 2015 10:48:58 +0200,
  David Henningsson wrote:
 
  Whenever there is an event from the i915 driver, wake the codec
  and recheck plug/unplug + ELD status.
 
  This fixes the issue with lost unsol events in power save mode,
  the codec and controller can now sleep in D3 and still know when
  the HDMI monitor has been connected.
 
  Signed-off-by: David Henningsson david.hennings...@canonical.com
 
  This addition looks fine, but then we'll get double notification for
  the normal hotplug/unplug, one via component ops and another via unsol
  event?
 
 Right, in case the unsol event actually works...
 
 I would argue that the normal case would be that the controller and 
 codec is in D3 which means that the unsol event never gets through - due 
 to hw limitations - which is what triggered this patch set in the first 
 place.
 
 But yes, in some case we might get double notification, but this should 
 not cause any trouble in practice. The unsol event could be turned off, 
 but would it be okay to save that for a later patch set (so I don't miss 
 the upcoming merge window)?

In that case, it should be mentioned in the changelog at least.

This series came a bit too late for the merge window, so I'm not sure
whether this can get in.  I personally find it OK, so take my ack for
ALSA parts (patch 3/4), but the rest need review and ack from i915
guys.  And we don't know who to merge this, if any.  The changes are
almost even to i915 and hda.  I don't mind either way, via drm or
sound tree.

In anyway,
   Reviewed-by: Takashi Iwai ti...@suse.de


thanks,

Takashi

 
 
  thanks,
 
  Takashi
 
  ---
sound/pci/hda/patch_hdmi.c | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
 
  diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
  index a97db5f..932292c 100644
  --- a/sound/pci/hda/patch_hdmi.c
  +++ b/sound/pci/hda/patch_hdmi.c
  @@ -37,6 +37,8 @@
#include sound/jack.h
#include sound/asoundef.h
#include sound/tlv.h
  +#include sound/hdaudio.h
  +#include sound/hda_i915.h
#include hda_codec.h
#include hda_local.h
#include hda_jack.h
  @@ -144,6 +146,9 @@ struct hdmi_spec {
  */
 struct hda_multi_out multiout;
 struct hda_pcm_stream pcm_playback;
  +
  +  /* i915/powerwell (Haswell+/Valleyview+) specific */
  +  struct i915_audio_component_audio_ops i915_audio_ops;
};
 
 
  @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec 
  *codec)
 struct hdmi_spec *spec = codec-spec;
 int pin_idx;
 
  +  if (is_haswell_plus(codec) || is_valleyview_plus(codec))
  +  snd_hdac_i915_register_notifier(NULL);
  +
 for (pin_idx = 0; pin_idx  spec-num_pins; pin_idx++) {
 struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
 
  @@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct 
  hda_codec *codec, hda_nid_t fg,
 snd_hda_codec_set_power_to_all(codec, fg, power_state);
}
 
  +static void intel_pin_eld_notify(void *audio_ptr, int port, int 
  port_mst_index)
  +{
  +  struct hda_codec *codec = audio_ptr;
  +  int pin_nid = port + 0x04;
  +
  +  check_presence_and_report(codec, pin_nid);
  +}
  +
static int patch_generic_hdmi(struct hda_codec *codec)
{
 struct hdmi_spec *spec;
  @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec 
  *codec)
 if (is_valleyview_plus(codec) || is_skylake(codec))
 codec-core.link_power_control = 1;
 
  -  if (is_haswell_plus(codec) || is_valleyview_plus(codec))
  +  if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
 codec-depop_delay = 0;
  +  spec-i915_audio_ops.audio_ptr = codec;
  +  spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
  +  snd_hdac_i915_register_notifier(spec-i915_audio_ops);
  +  }
 
 if (hdmi_parse_codec(codec)  0) {
 codec-spec = NULL;
  --
  1.9.1
 
 
 
 -- 
 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

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

[David Henningsson]

On 2015-08-20 11:28, Takashi Iwai wrote:

On Wed, 19 Aug 2015 10:48:58 +0200,
David Henningsson wrote:


Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.

This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.

Signed-off-by: David Henningsson david.hennings...@canonical.com


This addition looks fine, but then we'll get double notification for
the normal hotplug/unplug, one via component ops and another via unsol
event?


Right, in case the unsol event actually works...

I would argue that the normal case would be that the controller and 
codec is in D3 which means that the unsol event never gets through - due 
to hw limitations - which is what triggered this patch set in the first 
place.


But yes, in some case we might get double notification, but this should 
not cause any trouble in practice. The unsol event could be turned off, 
but would it be okay to save that for a later patch set (so I don't miss 
the upcoming merge window)?





thanks,

Takashi


---
  sound/pci/hda/patch_hdmi.c | 22 +-
  1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index a97db5f..932292c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -37,6 +37,8 @@
  #include sound/jack.h
  #include sound/asoundef.h
  #include sound/tlv.h
+#include sound/hdaudio.h
+#include sound/hda_i915.h
  #include hda_codec.h
  #include hda_local.h
  #include hda_jack.h
@@ -144,6 +146,9 @@ struct hdmi_spec {
 */
struct hda_multi_out multiout;
struct hda_pcm_stream pcm_playback;
+
+   /* i915/powerwell (Haswell+/Valleyview+) specific */
+   struct i915_audio_component_audio_ops i915_audio_ops;
  };


@@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
struct hdmi_spec *spec = codec-spec;
int pin_idx;

+   if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+   snd_hdac_i915_register_notifier(NULL);
+
for (pin_idx = 0; pin_idx  spec-num_pins; pin_idx++) {
struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);

@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec 
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
  }

+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
+{
+   struct hda_codec *codec = audio_ptr;
+   int pin_nid = port + 0x04;
+
+   check_presence_and_report(codec, pin_nid);
+}
+
  static int patch_generic_hdmi(struct hda_codec *codec)
  {
struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;

-   if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+   if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
+   spec-i915_audio_ops.audio_ptr = codec;
+   spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
+   snd_hdac_i915_register_notifier(spec-i915_audio_ops);
+   }

if (hdmi_parse_codec(codec)  0) {
codec-spec = NULL;
--
1.9.1





--
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

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

[Takashi Iwai]

On Wed, 19 Aug 2015 10:48:58 +0200,
David Henningsson wrote:
 
 Whenever there is an event from the i915 driver, wake the codec
 and recheck plug/unplug + ELD status.
 
 This fixes the issue with lost unsol events in power save mode,
 the codec and controller can now sleep in D3 and still know when
 the HDMI monitor has been connected.
 
 Signed-off-by: David Henningsson david.hennings...@canonical.com

This addition looks fine, but then we'll get double notification for
the normal hotplug/unplug, one via component ops and another via unsol
event?


thanks,

Takashi

 ---
  sound/pci/hda/patch_hdmi.c | 22 +-
  1 file changed, 21 insertions(+), 1 deletion(-)
 
 diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
 index a97db5f..932292c 100644
 --- a/sound/pci/hda/patch_hdmi.c
 +++ b/sound/pci/hda/patch_hdmi.c
 @@ -37,6 +37,8 @@
  #include sound/jack.h
  #include sound/asoundef.h
  #include sound/tlv.h
 +#include sound/hdaudio.h
 +#include sound/hda_i915.h
  #include hda_codec.h
  #include hda_local.h
  #include hda_jack.h
 @@ -144,6 +146,9 @@ struct hdmi_spec {
*/
   struct hda_multi_out multiout;
   struct hda_pcm_stream pcm_playback;
 +
 + /* i915/powerwell (Haswell+/Valleyview+) specific */
 + struct i915_audio_component_audio_ops i915_audio_ops;
  };
  
  
 @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
   struct hdmi_spec *spec = codec-spec;
   int pin_idx;
  
 + if (is_haswell_plus(codec) || is_valleyview_plus(codec))
 + snd_hdac_i915_register_notifier(NULL);
 +
   for (pin_idx = 0; pin_idx  spec-num_pins; pin_idx++) {
   struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
  
 @@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec 
 *codec, hda_nid_t fg,
   snd_hda_codec_set_power_to_all(codec, fg, power_state);
  }
  
 +static void intel_pin_eld_notify(void *audio_ptr, int port, int 
 port_mst_index)
 +{
 + struct hda_codec *codec = audio_ptr;
 + int pin_nid = port + 0x04;
 +
 + check_presence_and_report(codec, pin_nid);
 +}
 +
  static int patch_generic_hdmi(struct hda_codec *codec)
  {
   struct hdmi_spec *spec;
 @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec)
   if (is_valleyview_plus(codec) || is_skylake(codec))
   codec-core.link_power_control = 1;
  
 - if (is_haswell_plus(codec) || is_valleyview_plus(codec))
 + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
   codec-depop_delay = 0;
 + spec-i915_audio_ops.audio_ptr = codec;
 + spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
 + snd_hdac_i915_register_notifier(spec-i915_audio_ops);
 + }
  
   if (hdmi_parse_codec(codec)  0) {
   codec-spec = NULL;
 -- 
 1.9.1
 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Henningsson]

Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.

This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.

Signed-off-by: David Henningsson david.hennings...@canonical.com
---
 sound/pci/hda/patch_hdmi.c | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index a97db5f..932292c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -37,6 +37,8 @@
 #include sound/jack.h
 #include sound/asoundef.h
 #include sound/tlv.h
+#include sound/hdaudio.h
+#include sound/hda_i915.h
 #include hda_codec.h
 #include hda_local.h
 #include hda_jack.h
@@ -144,6 +146,9 @@ struct hdmi_spec {
 */
struct hda_multi_out multiout;
struct hda_pcm_stream pcm_playback;
+
+   /* i915/powerwell (Haswell+/Valleyview+) specific */
+   struct i915_audio_component_audio_ops i915_audio_ops;
 };
 
 
@@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
struct hdmi_spec *spec = codec-spec;
int pin_idx;
 
+   if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+   snd_hdac_i915_register_notifier(NULL);
+
for (pin_idx = 0; pin_idx  spec-num_pins; pin_idx++) {
struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
 
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec 
*codec, hda_nid_t fg,
snd_hda_codec_set_power_to_all(codec, fg, power_state);
 }
 
+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
+{
+   struct hda_codec *codec = audio_ptr;
+   int pin_nid = port + 0x04;
+
+   check_presence_and_report(codec, pin_nid);
+}
+
 static int patch_generic_hdmi(struct hda_codec *codec)
 {
struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec)
if (is_valleyview_plus(codec) || is_skylake(codec))
codec-core.link_power_control = 1;
 
-   if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+   if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
codec-depop_delay = 0;
+   spec-i915_audio_ops.audio_ptr = codec;
+   spec-i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
+   snd_hdac_i915_register_notifier(spec-i915_audio_ops);
+   }
 
if (hdmi_parse_codec(codec)  0) {
codec-spec = NULL;
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx