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

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