| Message ID | s5hfuzsohwl.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > 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.
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); }