[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 <linux/delay.h>
> > > > >>>>>    #include <linux/slab.h>
> > > > >>>>>    #include <linux/module.h>
> > > > >>>>> +#include <linux/pm_runtime.h>
> > > > >>>>>    #include <sound/core.h>
> > > > >>>>>    #include <sound/jack.h>
> > > > >>>>>    #include <sound/asoundef.h>
> > > > >>>>> @@ -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(&codec->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.
> 
> Just chatted with Imre a bit, and he clarified the suspend order thing:
> 
> So what happens is that the normal suspend hooks for hda and i915 can
> in theory happen in any order. But i915 reamains powered on until the
> late suspend hook.
> 
> So we can get
> 1. i915 suspend
> 2. hda suspend
> 3. i915 late suspend
> 
> or 
> 1. hda suspend
> 2. i915 suspend
> 3. i915 late suspend
> 
> So in this latter case i915 can call the eld notify hook after hda has
> already suspended. So that at least you would need to handle in your
> side.

OK, for the latter case, we may test a band-aid patch like below.
This will skip the notifier certainly while being suspended.
Xiong, could you check it?


Takashi

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