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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
