[alsa-devel] [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 <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?

[Zhang, Xiong Y] see the attachment file, it is s4 log. And it is latter case.
> 
> 
> 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);
>  }
> 
[Zhang, Xiong Y] yes, this patch could remove the error message.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: s4
Type: application/octet-stream
Size: 74380 bytes
Desc: s4
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20151127/b28527c2/attachment-0001.obj>