| Message ID | s5hwpt56v62.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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(). thanks
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. Takashi
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...?
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. 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
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? > > So I believe it's easier to avoid calling the eld_notify callback from > i915 side during its suspend code. > > > Takashi >
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 <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? 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 > > >
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. 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?
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
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 <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. > > 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
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. Takashi
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.
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)