Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Fri, 27 Nov 2015 14:45:31 +0100, David Henningsson wrote: > > > > On 2015-11-27 14:38, Takashi Iwai wrote: > > On Fri, 27 Nov 2015 03:55:28 +0100, > > Zhang, Xiong Y wrote: > >> > >>> 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. > > > > Alright, then it's indeed a failure in the sound driver side. > > Below is the official patch I'm going to merge. > > Just a quick question; have you checked that this does not bring back > the original bug the entire patch set was supposed to fix? Do you mean the hotplug handling runtime PM? I tested it locally, but wider ranged tests would be appreciated. In theory, it should work as mentioned in the changelog. Takashi > > > > > thanks, > > > > Takashi > > > > -- 8< -- > > From: Takashi Iwai > > Subject: [PATCH] ALSA: hda - Skip ELD notification during system suspend > > > > The recent addition of ELD notifier for Intel HDMI/DP codec may lead > > the bad codec connection found as kernel messages like below: > > Suspending console(s) (use no_console_suspend to debug) > > hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 > > Pin=6 Presence_Detect=1 ELD_Valid=1 > > snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 > > snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 > > > >snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 > >snd_hda_intel :00:1f.3: azx_get_response timeout, switching to > > polling mode: last cmd=0x206f2f00 > > snd_hda_intel :00:1f.3: No response from codec, disabling MSI: last > > cmd=0x206f2f00 > > snd_hda_intel :00:1f.3: azx_get_response timeout, switching to > > single_cmd mode: last cmd=0x206f2f00 > > azx_single_wait_for_response: 42 callbacks suppressed > > > > This seems appearing when the sound driver went to suspend before i915 > > driver. Then i915 driver disables HDMI/DP audio bit and calls the > > registered notifier, and the HDA codec tries to handle it as a > > hot(un)plug. But since the driver is already in the suspended state, > > it fails miserably. > > > > As this is a sort of spurious wakeup, it can be ignored safely, as > > long as it's delivered during the system suspend. OTOH, if a > > notification comes during the runtime suspend, the situation is > > different: we need to wake up. But during the system suspend, such a > > notification can't be the reason for a wakeup. > > > > This patch addresses it by a simple check of the current sound card > > status. The skipped notification doesn't matter because the HDA > > driver will check the plugged status forcibly at the resume in > > return. > > > > Then, why the card status, not a runtime PM status or else? The HDA > > controller driver is supposed to set the card status to D3 at the > > system suspend but not at the runtime suspend. So we can see it as a > > flag that is set only for the system suspend. Admittedly, it's a bit > > ugly, but it should work well for now. > > > > Reported-and-tested-by: "Zhang, Xiong Y" > > Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify > > events') > > Cc: # v4.3+ > > Signed-off-by: Takashi Iwai > > --- > > sound/pci/hda/patch_hdmi.c | 6 ++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > > index bdb6f226d006..4b6fb668c91c 100644 > > --- a/sound/pci/hda/patch_hdmi.c > > +++ b/sound/pci/hda/patch_hdmi.c > > @@ -2352,6 +2352,12 @@ 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 system suspend (but not in runtime PM); > > +* the state will be updated at resume > > +*/ > > + if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) > > + return; > > + > > check_presence_and_report(codec, pin_nid); > > } > > > > > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On 2015-11-27 14:38, Takashi Iwai wrote: On Fri, 27 Nov 2015 03:55:28 +0100, Zhang, Xiong Y wrote: 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. Alright, then it's indeed a failure in the sound driver side. Below is the official patch I'm going to merge. Just a quick question; have you checked that this does not bring back the original bug the entire patch set was supposed to fix? thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: hda - Skip ELD notification during system suspend The recent addition of ELD notifier for Intel HDMI/DP codec may lead the bad codec connection found as kernel messages like below: Suspending console(s) (use no_console_suspend to debug) hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 snd_hda_intel :00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00 snd_hda_intel :00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00 snd_hda_intel :00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00 azx_single_wait_for_response: 42 callbacks suppressed This seems appearing when the sound driver went to suspend before i915 driver. Then i915 driver disables HDMI/DP audio bit and calls the registered notifier, and the HDA codec tries to handle it as a hot(un)plug. But since the driver is already in the suspended state, it fails miserably. As this is a sort of spurious wakeup, it can be ignored safely, as long as it's delivered during the system suspend. OTOH, if a notification comes during the runtime suspend, the situation is different: we need to wake up. But during the system suspend, such a notification can't be the reason for a wakeup. This patch addresses it by a simple check of the current sound card status. The skipped notification doesn't matter because the HDA driver will check the plugged status forcibly at the resume in return. Then, why the card status, not a runtime PM status or else? The HDA controller driver is supposed to set the card status to D3 at the system suspend but not at the runtime suspend. So we can see it as a flag that is set only for the system suspend. Admittedly, it's a bit ugly, but it should work well for now. Reported-and-tested-by: "Zhang, Xiong Y" Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events') Cc: # v4.3+ Signed-off-by: Takashi Iwai --- sound/pci/hda/patch_hdmi.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..4b6fb668c91c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,12 @@ 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 system suspend (but not in runtime PM); +* the state will be updated at resume +*/ + if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) + return; + check_presence_and_report(codec, pin_nid); } -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Fri, 27 Nov 2015 03:55:28 +0100, Zhang, Xiong Y wrote: > > > 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. Alright, then it's indeed a failure in the sound driver side. Below is the official patch I'm going to merge. thanks, Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] ALSA: hda - Skip ELD notification during system suspend The recent addition of ELD notifier for Intel HDMI/DP codec may lead the bad codec connection found as kernel messages like below: Suspending console(s) (use no_console_suspend to debug) hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 snd_hda_intel :00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00 snd_hda_intel :00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00 snd_hda_intel :00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00 azx_single_wait_for_response: 42 callbacks suppressed This seems appearing when the sound driver went to suspend before i915 driver. Then i915 driver disables HDMI/DP audio bit and calls the registered notifier, and the HDA codec tries to handle it as a hot(un)plug. But since the driver is already in the suspended state, it fails miserably. As this is a sort of spurious wakeup, it can be ignored safely, as long as it's delivered during the system suspend. OTOH, if a notification comes during the runtime suspend, the situation is different: we need to wake up. But during the system suspend, such a notification can't be the reason for a wakeup. This patch addresses it by a simple check of the current sound card status. The skipped notification doesn't matter because the HDA driver will check the plugged status forcibly at the resume in return. Then, why the card status, not a runtime PM status or else? The HDA controller driver is supposed to set the card status to D3 at the system suspend but not at the runtime suspend. So we can see it as a flag that is set only for the system suspend. Admittedly, it's a bit ugly, but it should work well for now. Reported-and-tested-by: "Zhang, Xiong Y" Fixes: 25adc137c546 ('ALSA: hda - Wake the codec up on pin/ELD notify events') Cc: # v4.3+ Signed-off-by: Takashi Iwai --- sound/pci/hda/patch_hdmi.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..4b6fb668c91c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,6 +2352,12 @@ 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 system suspend (but not in runtime PM); +* the state will be updated at resume +*/ + if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) + return; + check_presence_and_report(codec, pin_nid); } -- 2.6.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
> > 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 > > > > > >#include > > > > > >#include > > > > > > +#include > > > > > >#include > > > > > >#include > > > > > >#include > > > > > > @@ -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
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > > > > >#include > > > > >#include > > > > > +#include > > > > >#include > > > > >#include > > > > >#include > > > > > @@ -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 nev
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > > > >#include > > > >#include > > > > +#include > > > >#include > > > >#include > > > >#include > > > > @@ -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 so
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > > >#include > > >#include > > > +#include > > >#include > > >#include > > >#include > > > @@ -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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesk
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > > >>> #include > > >>> #include > > >>> +#include > > >>> #include > > >>> #include > > >>> #include > > >>> @@ -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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > >#include > >#include > > +#include > >#include > >#include > >#include > > @@ -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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > >#include > >#include > > +#include > >#include > >#include > >#include > > @@ -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? -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > > > > > > #include > > > > > > #include > > > > > > +#include > > > > > > #include > > > > > > #include > > > > > > #include > > > > > > @@ -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 > > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 #include #include +#include #include #include #include @@ -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 -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > >>> #include > >>> #include > >>> +#include > >>> #include > >>> #include > >>> #include > >>> @@ -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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 #include #include +#include #include #include #include @@ -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...? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
> 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 > #include > #include > +#include > #include > #include > #include > @@ -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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
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 #include #include +#include #include #include #include @@ -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) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
> > > 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. > > Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Thu, 26 Nov 2015 07:06:56 +0100, Zhang, Xiong Y wrote: > > > On Wed, 25 Nov 2015 11:57:13 +0100, > > Zhang, Xiong Y wrote: > > > > > > > On Wed, 25 Nov 2015 10:56:51 +0100, > > > > Zhang, Xiong Y wrote: > > > > > > > > > > Recently I always see the following error message during S4 or S3 > > > > > resume > > > > with drm-intel-nightly. > > > > > [ 97.778063] PM: Syncing filesystems ... done. > > > > > [ 97.801550] Freezing user space processes ... (elapsed 0.002 > > > > > seconds) > > > > done. > > > > > [ 97.804297] PM: Marking nosave pages: [mem > > 0x-0x0fff] > > > > > [ 97.804302] PM: Marking nosave pages: [mem > > 0x00058000-0x00058fff] > > > > > [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f] > > > > > [ 97.804310] PM: Marking nosave pages: [mem > > 0x84c11000-0x84c12fff] > > > > > [ 97.804312] PM: Marking nosave pages: [mem > > 0x876fc000-0x87746fff] > > > > > [ 97.804317] PM: Marking nosave pages: [mem > > 0x8785e000-0x87fe9fff] > > > > > [ 97.804387] PM: Marking nosave pages: [mem 0x8800-0x] > > > > > [ 97.806363] PM: Basic memory bitmaps created > > > > > [ 97.806409] PM: Preallocating image memory... done (allocated > > 321557 > > > > pages) > > > > > [ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 > > > > MB/s) > > > > > [ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 > > seconds) > > > > done. > > > > > [ 98.151998] Suspending console(s) (use no_console_suspend to > > debug) > > > > > [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi > > hdaudioC0D2: > > > > HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 > > > > > [ 99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > > last > > > > cmd=0x206f2e08 > > > > > [ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size > > is 0, > > > > force 128 > > > > > [ 101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout, > > > > switching to polling mode: last cmd=0x206f2f00 > > > > > [ 102.195492] snd_hda_intel :00:1f.3: No response from codec, > > > > disabling MSI: last cmd=0x206f2f00 > > > > > [ 103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout, > > > > switching to single_cmd mode: last cmd=0x206f2f00 > > > > > [ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed > > > > > > > > > > The bisect result points to this commit. > > > > > I checked this patch and had one question: if i915 driver wake up > > > > > ahead of > > > > snd_hda_intel driver during resume, i915 driver will call audio > > > > driver's > > > > hdmi_present_sense() function through this patch, but the audio > > > > interrupt > > is > > > > disabled at this moment, how could hdmi_present_sense() get the > > response > > > > from codec ? > > > > > > > > Yeah, a bad thing happens there. The fix should be simple like below, > > > > though. Basically the pins are checked in the resume callback in > > > > anyway, so there is no need to handle the notification during PM. > > > > > > > > Could you check whether it works? > > > > > > > > > > > > thanks, > > > > > > > > Takashi > > > > > > Strange, your patch couldn't get away the error message. > > > > OK, then it's not about the race *during* the hd-audio driver > > resuming or such. I guess it's the other way round: the i915 driver > > notifies it unnecessarily while it's getting off. The audio part of > > HDMI controller relies on the i915 power state, so it's screwed up. > > > > Check with drm.debug option to see in which code path it's triggered. > > If it's a call in i915 suspend, the call should be removed not to wake > > up the audio part unnecessarily. > > [Zhang, Xiong Y] it is called in intel_audio_codec_disable() from i915 > suspend. So the call of eld_notifier should be suppressed at suspend. > --- > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index bdb6f226d006..b468fe0e6195 100644 > --- a/sound/pci/hda
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
> On Wed, 25 Nov 2015 11:57:13 +0100, > Zhang, Xiong Y wrote: > > > > > On Wed, 25 Nov 2015 10:56:51 +0100, > > > Zhang, Xiong Y wrote: > > > > > > > > Recently I always see the following error message during S4 or S3 resume > > > with drm-intel-nightly. > > > > [ 97.778063] PM: Syncing filesystems ... done. > > > > [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds) > > > done. > > > > [ 97.804297] PM: Marking nosave pages: [mem > 0x-0x0fff] > > > > [ 97.804302] PM: Marking nosave pages: [mem > 0x00058000-0x00058fff] > > > > [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f] > > > > [ 97.804310] PM: Marking nosave pages: [mem > 0x84c11000-0x84c12fff] > > > > [ 97.804312] PM: Marking nosave pages: [mem > 0x876fc000-0x87746fff] > > > > [ 97.804317] PM: Marking nosave pages: [mem > 0x8785e000-0x87fe9fff] > > > > [ 97.804387] PM: Marking nosave pages: [mem 0x8800-0x] > > > > [ 97.806363] PM: Basic memory bitmaps created > > > > [ 97.806409] PM: Preallocating image memory... done (allocated > 321557 > > > pages) > > > > [ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 > > > MB/s) > > > > [ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 > seconds) > > > done. > > > > [ 98.151998] Suspending console(s) (use no_console_suspend to > debug) > > > > [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi > hdaudioC0D2: > > > HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 > > > > [ 99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, > last > > > cmd=0x206f2e08 > > > > [ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size > is 0, > > > force 128 > > > > [ 101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout, > > > switching to polling mode: last cmd=0x206f2f00 > > > > [ 102.195492] snd_hda_intel :00:1f.3: No response from codec, > > > disabling MSI: last cmd=0x206f2f00 > > > > [ 103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout, > > > switching to single_cmd mode: last cmd=0x206f2f00 > > > > [ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed > > > > > > > > The bisect result points to this commit. > > > > I checked this patch and had one question: if i915 driver wake up ahead > > > > of > > > snd_hda_intel driver during resume, i915 driver will call audio driver's > > > hdmi_present_sense() function through this patch, but the audio interrupt > is > > > disabled at this moment, how could hdmi_present_sense() get the > response > > > from codec ? > > > > > > Yeah, a bad thing happens there. The fix should be simple like below, > > > though. Basically the pins are checked in the resume callback in > > > anyway, so there is no need to handle the notification during PM. > > > > > > Could you check whether it works? > > > > > > > > > thanks, > > > > > > Takashi > > > > Strange, your patch couldn't get away the error message. > > OK, then it's not about the race *during* the hd-audio driver > resuming or such. I guess it's the other way round: the i915 driver > notifies it unnecessarily while it's getting off. The audio part of > HDMI controller relies on the i915 power state, so it's screwed up. > > Check with drm.debug option to see in which code path it's triggered. > If it's a call in i915 suspend, the call should be removed not to wake > up the audio part unnecessarily. [Zhang, Xiong Y] it is called in intel_audio_codec_disable() from i915 suspend. --- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..b468fe0e6195 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,7 +2352,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); + /* don't call notifier during PM; will be checked in resume callback */ + if (atomic_read(&codec->core.in_pm)) +
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Wed, 25 Nov 2015 11:57:13 +0100, Zhang, Xiong Y wrote: > > > On Wed, 25 Nov 2015 10:56:51 +0100, > > Zhang, Xiong Y wrote: > > > > > > Recently I always see the following error message during S4 or S3 resume > > with drm-intel-nightly. > > > [ 97.778063] PM: Syncing filesystems ... done. > > > [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds) > > done. > > > [ 97.804297] PM: Marking nosave pages: [mem 0x-0x0fff] > > > [ 97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff] > > > [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f] > > > [ 97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff] > > > [ 97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff] > > > [ 97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff] > > > [ 97.804387] PM: Marking nosave pages: [mem 0x8800-0x] > > > [ 97.806363] PM: Basic memory bitmaps created > > > [ 97.806409] PM: Preallocating image memory... done (allocated 321557 > > pages) > > > [ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 > > MB/s) > > > [ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 > > > seconds) > > done. > > > [ 98.151998] Suspending console(s) (use no_console_suspend to debug) > > > [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: > > HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 > > > [ 99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > > cmd=0x206f2e08 > > > [ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, > > force 128 > > > [ 101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout, > > switching to polling mode: last cmd=0x206f2f00 > > > [ 102.195492] snd_hda_intel :00:1f.3: No response from codec, > > disabling MSI: last cmd=0x206f2f00 > > > [ 103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout, > > switching to single_cmd mode: last cmd=0x206f2f00 > > > [ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed > > > > > > The bisect result points to this commit. > > > I checked this patch and had one question: if i915 driver wake up ahead of > > snd_hda_intel driver during resume, i915 driver will call audio driver's > > hdmi_present_sense() function through this patch, but the audio interrupt is > > disabled at this moment, how could hdmi_present_sense() get the response > > from codec ? > > > > Yeah, a bad thing happens there. The fix should be simple like below, > > though. Basically the pins are checked in the resume callback in > > anyway, so there is no need to handle the notification during PM. > > > > Could you check whether it works? > > > > > > thanks, > > > > Takashi > > Strange, your patch couldn't get away the error message. OK, then it's not about the race *during* the hd-audio driver resuming or such. I guess it's the other way round: the i915 driver notifies it unnecessarily while it's getting off. The audio part of HDMI controller relies on the i915 power state, so it's screwed up. Check with drm.debug option to see in which code path it's triggered. If it's a call in i915 suspend, the call should be removed not to wake up the audio part unnecessarily. 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. Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
> On Wed, 25 Nov 2015 10:56:51 +0100, > Zhang, Xiong Y wrote: > > > > Recently I always see the following error message during S4 or S3 resume > with drm-intel-nightly. > > [ 97.778063] PM: Syncing filesystems ... done. > > [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds) > done. > > [ 97.804297] PM: Marking nosave pages: [mem 0x-0x0fff] > > [ 97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff] > > [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f] > > [ 97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff] > > [ 97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff] > > [ 97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff] > > [ 97.804387] PM: Marking nosave pages: [mem 0x8800-0x] > > [ 97.806363] PM: Basic memory bitmaps created > > [ 97.806409] PM: Preallocating image memory... done (allocated 321557 > pages) > > [ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 > MB/s) > > [ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 > > seconds) > done. > > [ 98.151998] Suspending console(s) (use no_console_suspend to debug) > > [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: > HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 > > [ 99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > > [ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, > force 128 > > [ 101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout, > switching to polling mode: last cmd=0x206f2f00 > > [ 102.195492] snd_hda_intel :00:1f.3: No response from codec, > disabling MSI: last cmd=0x206f2f00 > > [ 103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout, > switching to single_cmd mode: last cmd=0x206f2f00 > > [ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed > > > > The bisect result points to this commit. > > I checked this patch and had one question: if i915 driver wake up ahead of > snd_hda_intel driver during resume, i915 driver will call audio driver's > hdmi_present_sense() function through this patch, but the audio interrupt is > disabled at this moment, how could hdmi_present_sense() get the response > from codec ? > > Yeah, a bad thing happens there. The fix should be simple like below, > though. Basically the pins are checked in the resume callback in > anyway, so there is no need to handle the notification during PM. > > Could you check whether it works? > > > thanks, > > Takashi Strange, your patch couldn't get away the error message. thanks > > --- > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index bdb6f226d006..b468fe0e6195 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -2352,7 +2352,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); > + /* don't call notifier during PM; will be checked in resume callback */ > + if (!atomic_read(&codec->core.in_pm)) > + check_presence_and_report(codec, pin_nid); > } > > static int patch_generic_hdmi(struct hda_codec *codec) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Wed, 25 Nov 2015 10:56:51 +0100, Zhang, Xiong Y wrote: > > Recently I always see the following error message during S4 or S3 resume with > drm-intel-nightly. > [ 97.778063] PM: Syncing filesystems ... done. > [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds) done. > [ 97.804297] PM: Marking nosave pages: [mem 0x-0x0fff] > [ 97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff] > [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f] > [ 97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff] > [ 97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff] > [ 97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff] > [ 97.804387] PM: Marking nosave pages: [mem 0x8800-0x] > [ 97.806363] PM: Basic memory bitmaps created > [ 97.806409] PM: Preallocating image memory... done (allocated 321557 pages) > [ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 MB/s) > [ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) > done. > [ 98.151998] Suspending console(s) (use no_console_suspend to debug) > [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI > status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 > [ 99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last > cmd=0x206f2e08 > [ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force > 128 > [ 101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout, > switching to polling mode: last cmd=0x206f2f00 > [ 102.195492] snd_hda_intel :00:1f.3: No response from codec, disabling > MSI: last cmd=0x206f2f00 > [ 103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout, > switching to single_cmd mode: last cmd=0x206f2f00 > [ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed > > The bisect result points to this commit. > I checked this patch and had one question: if i915 driver wake up ahead of > snd_hda_intel driver during resume, i915 driver will call audio driver's > hdmi_present_sense() function through this patch, but the audio interrupt is > disabled at this moment, how could hdmi_present_sense() get the response from > codec ? Yeah, a bad thing happens there. The fix should be simple like below, though. Basically the pins are checked in the resume callback in anyway, so there is no need to handle the notification during PM. Could you check whether it works? thanks, Takashi --- diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index bdb6f226d006..b468fe0e6195 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2352,7 +2352,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); + /* don't call notifier during PM; will be checked in resume callback */ + if (!atomic_read(&codec->core.in_pm)) + check_presence_and_report(codec, pin_nid); } static int patch_generic_hdmi(struct hda_codec *codec) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
Recently I always see the following error message during S4 or S3 resume with drm-intel-nightly. [ 97.778063] PM: Syncing filesystems ... done. [ 97.801550] Freezing user space processes ... (elapsed 0.002 seconds) done. [ 97.804297] PM: Marking nosave pages: [mem 0x-0x0fff] [ 97.804302] PM: Marking nosave pages: [mem 0x00058000-0x00058fff] [ 97.804305] PM: Marking nosave pages: [mem 0x0009e000-0x000f] [ 97.804310] PM: Marking nosave pages: [mem 0x84c11000-0x84c12fff] [ 97.804312] PM: Marking nosave pages: [mem 0x876fc000-0x87746fff] [ 97.804317] PM: Marking nosave pages: [mem 0x8785e000-0x87fe9fff] [ 97.804387] PM: Marking nosave pages: [mem 0x8800-0x] [ 97.806363] PM: Basic memory bitmaps created [ 97.806409] PM: Preallocating image memory... done (allocated 321557 pages) [ 98.150475] PM: Allocated 1286228 kbytes in 0.34 seconds (3783.02 MB/s) [ 98.150476] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 98.151998] Suspending console(s) (use no_console_suspend to debug) [ 98.173485] hdmi_present_sense: snd_hda_codec_hdmi hdaudioC0D2: HDMI status: Codec=2 Pin=6 Presence_Detect=1 ELD_Valid=1 [ 99.178150] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178151] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178152] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178153] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178154] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178155] snd_hda_intel :00:1f.3: spurious response 0x0:0x2, last cmd=0x206f2e08 [ 99.178162] snd_hda_codec_hdmi hdaudioC0D2: HDMI: ELD buf size is 0, force 128 [ 101.189709] snd_hda_intel :00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x206f2f00 [ 102.195492] snd_hda_intel :00:1f.3: No response from codec, disabling MSI: last cmd=0x206f2f00 [ 103.201275] snd_hda_intel :00:1f.3: azx_get_response timeout, switching to single_cmd mode: last cmd=0x206f2f00 [ 103.201396] azx_single_wait_for_response: 42 callbacks suppressed The bisect result points to this commit. I checked this patch and had one question: if i915 driver wake up ahead of snd_hda_intel driver during resume, i915 driver will call audio driver's hdmi_present_sense() function through this patch, but the audio interrupt is disabled at this moment, how could hdmi_present_sense() get the response from codec ? thanks > -Original Message- > From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of > David Henningsson > Sent: Saturday, August 29, 2015 1:03 AM > To: ti...@suse.de; alsa-de...@alsa-project.org; > intel-gfx@lists.freedesktop.org; jani.nik...@linux.intel.com; Yang, Libin; > Vetter, > Daniel > Cc: David Henningsson > Subject: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD > notify > events > > Whenever there is an event from the i915 driver, wake the codec > and recheck plug/unplug + ELD status. > > This fixes the issue with lost unsol events in power save mode, > the codec and controller can now sleep in D3 and still know when > the HDMI monitor has been connected. > > Right now, this might mean we get two callbacks from the same event, > one from the unsol event and one from the i915 driver, but this is > not harmful and can be optimised away in a later patch. > > Reviewed-by: Takashi Iwai > Signed-off-by: David Henningsson > --- > sound/pci/hda/patch_hdmi.c | 22 +- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index a97db5f..acbfbe0 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -37,6 +37,8 @@ > #include > #include > #include > +#include > +#include > #include "hda_codec.h" > #include "hda_local.h" > #include "hda_jack.h" > @@ -144,6 +146,9 @@ struct hdmi_spec { >*/ > struct hda_multi_out multiout; > struct hda_pcm_stream pcm_playback; > + > + /* i915/powerwell (Haswell+/Valleyview+) specific */ > + struct i915_audio_component_audio_ops i915_audio_ops; > }; > > > @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec > *codec) > struct hdmi_spec *spec = codec->spec; > int pin_idx; > > + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) > + snd_hdac_i915_register_notifier(NULL); > + > for (pin_idx = 0; pin_idx < spec->n
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Wed, 02 Sep 2015 10:32:34 +0200, Daniel Vetter wrote: > > On Wed, Sep 02, 2015 at 10:03:55AM +0200, Takashi Iwai wrote: > > On Wed, 02 Sep 2015 10:00:44 +0200, > > Daniel Vetter wrote: > > > > > > On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote: > > > > On Thu, 20 Aug 2015, Takashi Iwai wrote: > > > > > On Thu, 20 Aug 2015 11:41:42 +0200, > > > > > David Henningsson wrote: > > > > >> > > > > >> > > > > >> > > > > >> On 2015-08-20 11:28, Takashi Iwai wrote: > > > > >> > On Wed, 19 Aug 2015 10:48:58 +0200, > > > > >> > David Henningsson wrote: > > > > >> >> > > > > >> >> Whenever there is an event from the i915 driver, wake the codec > > > > >> >> and recheck plug/unplug + ELD status. > > > > >> >> > > > > >> >> This fixes the issue with lost unsol events in power save mode, > > > > >> >> the codec and controller can now sleep in D3 and still know when > > > > >> >> the HDMI monitor has been connected. > > > > >> >> > > > > >> >> Signed-off-by: David Henningsson > > > > >> > > > > > >> > This addition looks fine, but then we'll get double notification > > > > >> > for > > > > >> > the normal hotplug/unplug, one via component ops and another via > > > > >> > unsol > > > > >> > event? > > > > >> > > > > >> Right, in case the unsol event actually works... > > > > >> > > > > >> I would argue that the normal case would be that the controller and > > > > >> codec is in D3 which means that the unsol event never gets through - > > > > >> due > > > > >> to hw limitations - which is what triggered this patch set in the > > > > >> first > > > > >> place. > > > > >> > > > > >> But yes, in some case we might get double notification, but this > > > > >> should > > > > >> not cause any trouble in practice. The unsol event could be turned > > > > >> off, > > > > >> but would it be okay to save that for a later patch set (so I don't > > > > >> miss > > > > >> the upcoming merge window)? > > > > > > > > > > In that case, it should be mentioned in the changelog at least. > > > > > > > > > > This series came a bit too late for the merge window, so I'm not sure > > > > > whether this can get in. I personally find it OK, so take my ack for > > > > > ALSA parts (patch 3/4), but the rest need review and ack from i915 > > > > > guys. And we don't know who to merge this, if any. The changes are > > > > > almost even to i915 and hda. I don't mind either way, via drm or > > > > > sound tree. > > > > > > > > Personally I'm fine with this still going for v4.3 since to me it looks > > > > like any regressions this might cause will be on your side. *grin*. > > > > > > The only bit I want is some decent kerneldoc for the ad-hoc growing > > > i915/hda-intel interfaces. But that can be done in follow-up patches and > > > needs to be coordinated with the audio rate handling series anyway. So ack > > > from my side for all getting merged through alsa trees too. > > > > Are you going to merge Libin's patchset? If yes, it's better to align > > both patchsets through a single tree. It'll make merges easier, as > > both touch the similar place. > > Oh that was an ack for merging it all through your tree. If you punt it to > 4.4 I might ask you for a stable tree to pull in in case I get conflicts. OK, then I'll merge David's patchset now for the upcoming pull request for 4.3-rc1. thanks, Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Wed, Sep 02, 2015 at 10:03:55AM +0200, Takashi Iwai wrote: > On Wed, 02 Sep 2015 10:00:44 +0200, > Daniel Vetter wrote: > > > > On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote: > > > On Thu, 20 Aug 2015, Takashi Iwai wrote: > > > > On Thu, 20 Aug 2015 11:41:42 +0200, > > > > David Henningsson wrote: > > > >> > > > >> > > > >> > > > >> On 2015-08-20 11:28, Takashi Iwai wrote: > > > >> > On Wed, 19 Aug 2015 10:48:58 +0200, > > > >> > David Henningsson wrote: > > > >> >> > > > >> >> Whenever there is an event from the i915 driver, wake the codec > > > >> >> and recheck plug/unplug + ELD status. > > > >> >> > > > >> >> This fixes the issue with lost unsol events in power save mode, > > > >> >> the codec and controller can now sleep in D3 and still know when > > > >> >> the HDMI monitor has been connected. > > > >> >> > > > >> >> Signed-off-by: David Henningsson > > > >> > > > > >> > This addition looks fine, but then we'll get double notification for > > > >> > the normal hotplug/unplug, one via component ops and another via > > > >> > unsol > > > >> > event? > > > >> > > > >> Right, in case the unsol event actually works... > > > >> > > > >> I would argue that the normal case would be that the controller and > > > >> codec is in D3 which means that the unsol event never gets through - > > > >> due > > > >> to hw limitations - which is what triggered this patch set in the > > > >> first > > > >> place. > > > >> > > > >> But yes, in some case we might get double notification, but this > > > >> should > > > >> not cause any trouble in practice. The unsol event could be turned > > > >> off, > > > >> but would it be okay to save that for a later patch set (so I don't > > > >> miss > > > >> the upcoming merge window)? > > > > > > > > In that case, it should be mentioned in the changelog at least. > > > > > > > > This series came a bit too late for the merge window, so I'm not sure > > > > whether this can get in. I personally find it OK, so take my ack for > > > > ALSA parts (patch 3/4), but the rest need review and ack from i915 > > > > guys. And we don't know who to merge this, if any. The changes are > > > > almost even to i915 and hda. I don't mind either way, via drm or > > > > sound tree. > > > > > > Personally I'm fine with this still going for v4.3 since to me it looks > > > like any regressions this might cause will be on your side. *grin*. > > > > The only bit I want is some decent kerneldoc for the ad-hoc growing > > i915/hda-intel interfaces. But that can be done in follow-up patches and > > needs to be coordinated with the audio rate handling series anyway. So ack > > from my side for all getting merged through alsa trees too. > > Are you going to merge Libin's patchset? If yes, it's better to align > both patchsets through a single tree. It'll make merges easier, as > both touch the similar place. Oh that was an ack for merging it all through your tree. If you punt it to 4.4 I might ask you for a stable tree to pull in in case I get conflicts. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Wed, 02 Sep 2015 10:00:44 +0200, Daniel Vetter wrote: > > On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote: > > On Thu, 20 Aug 2015, Takashi Iwai wrote: > > > On Thu, 20 Aug 2015 11:41:42 +0200, > > > David Henningsson wrote: > > >> > > >> > > >> > > >> On 2015-08-20 11:28, Takashi Iwai wrote: > > >> > On Wed, 19 Aug 2015 10:48:58 +0200, > > >> > David Henningsson wrote: > > >> >> > > >> >> Whenever there is an event from the i915 driver, wake the codec > > >> >> and recheck plug/unplug + ELD status. > > >> >> > > >> >> This fixes the issue with lost unsol events in power save mode, > > >> >> the codec and controller can now sleep in D3 and still know when > > >> >> the HDMI monitor has been connected. > > >> >> > > >> >> Signed-off-by: David Henningsson > > >> > > > >> > This addition looks fine, but then we'll get double notification for > > >> > the normal hotplug/unplug, one via component ops and another via unsol > > >> > event? > > >> > > >> Right, in case the unsol event actually works... > > >> > > >> I would argue that the normal case would be that the controller and > > >> codec is in D3 which means that the unsol event never gets through - due > > >> to hw limitations - which is what triggered this patch set in the first > > >> place. > > >> > > >> But yes, in some case we might get double notification, but this should > > >> not cause any trouble in practice. The unsol event could be turned off, > > >> but would it be okay to save that for a later patch set (so I don't miss > > >> the upcoming merge window)? > > > > > > In that case, it should be mentioned in the changelog at least. > > > > > > This series came a bit too late for the merge window, so I'm not sure > > > whether this can get in. I personally find it OK, so take my ack for > > > ALSA parts (patch 3/4), but the rest need review and ack from i915 > > > guys. And we don't know who to merge this, if any. The changes are > > > almost even to i915 and hda. I don't mind either way, via drm or > > > sound tree. > > > > Personally I'm fine with this still going for v4.3 since to me it looks > > like any regressions this might cause will be on your side. *grin*. > > The only bit I want is some decent kerneldoc for the ad-hoc growing > i915/hda-intel interfaces. But that can be done in follow-up patches and > needs to be coordinated with the audio rate handling series anyway. So ack > from my side for all getting merged through alsa trees too. Are you going to merge Libin's patchset? If yes, it's better to align both patchsets through a single tree. It'll make merges easier, as both touch the similar place. thanks, Takashi ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote: > On Thu, 20 Aug 2015, Takashi Iwai wrote: > > On Thu, 20 Aug 2015 11:41:42 +0200, > > David Henningsson wrote: > >> > >> > >> > >> On 2015-08-20 11:28, Takashi Iwai wrote: > >> > On Wed, 19 Aug 2015 10:48:58 +0200, > >> > David Henningsson wrote: > >> >> > >> >> Whenever there is an event from the i915 driver, wake the codec > >> >> and recheck plug/unplug + ELD status. > >> >> > >> >> This fixes the issue with lost unsol events in power save mode, > >> >> the codec and controller can now sleep in D3 and still know when > >> >> the HDMI monitor has been connected. > >> >> > >> >> Signed-off-by: David Henningsson > >> > > >> > This addition looks fine, but then we'll get double notification for > >> > the normal hotplug/unplug, one via component ops and another via unsol > >> > event? > >> > >> Right, in case the unsol event actually works... > >> > >> I would argue that the normal case would be that the controller and > >> codec is in D3 which means that the unsol event never gets through - due > >> to hw limitations - which is what triggered this patch set in the first > >> place. > >> > >> But yes, in some case we might get double notification, but this should > >> not cause any trouble in practice. The unsol event could be turned off, > >> but would it be okay to save that for a later patch set (so I don't miss > >> the upcoming merge window)? > > > > In that case, it should be mentioned in the changelog at least. > > > > This series came a bit too late for the merge window, so I'm not sure > > whether this can get in. I personally find it OK, so take my ack for > > ALSA parts (patch 3/4), but the rest need review and ack from i915 > > guys. And we don't know who to merge this, if any. The changes are > > almost even to i915 and hda. I don't mind either way, via drm or > > sound tree. > > Personally I'm fine with this still going for v4.3 since to me it looks > like any regressions this might cause will be on your side. *grin*. The only bit I want is some decent kerneldoc for the ad-hoc growing i915/hda-intel interfaces. But that can be done in follow-up patches and needs to be coordinated with the audio rate handling series anyway. So ack from my side for all getting merged through alsa trees too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Thu, 20 Aug 2015, Takashi Iwai wrote: > On Thu, 20 Aug 2015 11:41:42 +0200, > David Henningsson wrote: >> >> >> >> On 2015-08-20 11:28, Takashi Iwai wrote: >> > On Wed, 19 Aug 2015 10:48:58 +0200, >> > David Henningsson wrote: >> >> >> >> Whenever there is an event from the i915 driver, wake the codec >> >> and recheck plug/unplug + ELD status. >> >> >> >> This fixes the issue with lost unsol events in power save mode, >> >> the codec and controller can now sleep in D3 and still know when >> >> the HDMI monitor has been connected. >> >> >> >> Signed-off-by: David Henningsson >> > >> > This addition looks fine, but then we'll get double notification for >> > the normal hotplug/unplug, one via component ops and another via unsol >> > event? >> >> Right, in case the unsol event actually works... >> >> I would argue that the normal case would be that the controller and >> codec is in D3 which means that the unsol event never gets through - due >> to hw limitations - which is what triggered this patch set in the first >> place. >> >> But yes, in some case we might get double notification, but this should >> not cause any trouble in practice. The unsol event could be turned off, >> but would it be okay to save that for a later patch set (so I don't miss >> the upcoming merge window)? > > In that case, it should be mentioned in the changelog at least. > > This series came a bit too late for the merge window, so I'm not sure > whether this can get in. I personally find it OK, so take my ack for > ALSA parts (patch 3/4), but the rest need review and ack from i915 > guys. And we don't know who to merge this, if any. The changes are > almost even to i915 and hda. I don't mind either way, via drm or > sound tree. Personally I'm fine with this still going for v4.3 since to me it looks like any regressions this might cause will be on your side. *grin*. BR, Jani. > > In anyway, >Reviewed-by: Takashi Iwai > > > thanks, > > Takashi > >> > >> > >> > thanks, >> > >> > Takashi >> > >> >> --- >> >> sound/pci/hda/patch_hdmi.c | 22 +- >> >> 1 file changed, 21 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >> >> index a97db5f..932292c 100644 >> >> --- a/sound/pci/hda/patch_hdmi.c >> >> +++ b/sound/pci/hda/patch_hdmi.c >> >> @@ -37,6 +37,8 @@ >> >> #include >> >> #include >> >> #include >> >> +#include >> >> +#include >> >> #include "hda_codec.h" >> >> #include "hda_local.h" >> >> #include "hda_jack.h" >> >> @@ -144,6 +146,9 @@ struct hdmi_spec { >> >>*/ >> >> struct hda_multi_out multiout; >> >> struct hda_pcm_stream pcm_playback; >> >> + >> >> + /* i915/powerwell (Haswell+/Valleyview+) specific */ >> >> + struct i915_audio_component_audio_ops i915_audio_ops; >> >> }; >> >> >> >> >> >> @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec >> >> *codec) >> >> struct hdmi_spec *spec = codec->spec; >> >> int pin_idx; >> >> >> >> + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) >> >> + snd_hdac_i915_register_notifier(NULL); >> >> + >> >> for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { >> >> struct hdmi_spec_per_pin *per_pin = get_pin(spec, >> >> pin_idx); >> >> >> >> @@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct >> >> hda_codec *codec, hda_nid_t fg, >> >> snd_hda_codec_set_power_to_all(codec, fg, power_state); >> >> } >> >> >> >> +static void intel_pin_eld_notify(void *audio_ptr, int port, int >> >> port_mst_index) >> >> +{ >> >> + struct hda_codec *codec = audio_ptr; >> >> + int pin_nid = port + 0x04; >> >> + >> >> + check_presence_and_report(codec, pin_nid); >> >> +} >> >> + >> >> static int patch_generic_hdmi(struct hda_codec *codec) >> >> { >> >> struct hdmi_spec *spec; >> >> @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec >> >> *codec) >> >> if (is_valleyview_plus(codec) || is_skylake(codec)) >> >> codec->core.link_power_control = 1; >> >> >> >> - if (is_haswell_plus(codec) || is_valleyview_plus(codec)) >> >> + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { >> >> codec->depop_delay = 0; >> >> + spec->i915_audio_ops.audio_ptr = codec; >> >> + spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify; >> >> + snd_hdac_i915_register_notifier(&spec->i915_audio_ops); >> >> + } >> >> >> >> if (hdmi_parse_codec(codec) < 0) { >> >> codec->spec = NULL; >> >> -- >> >> 1.9.1 >> >> >> > >> >> -- >> David Henningsson, Canonical Ltd. >> https://launchpad.net/~diwic >> -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Thu, 20 Aug 2015 11:41:42 +0200, David Henningsson wrote: > > > > On 2015-08-20 11:28, Takashi Iwai wrote: > > On Wed, 19 Aug 2015 10:48:58 +0200, > > David Henningsson wrote: > >> > >> Whenever there is an event from the i915 driver, wake the codec > >> and recheck plug/unplug + ELD status. > >> > >> This fixes the issue with lost unsol events in power save mode, > >> the codec and controller can now sleep in D3 and still know when > >> the HDMI monitor has been connected. > >> > >> Signed-off-by: David Henningsson > > > > This addition looks fine, but then we'll get double notification for > > the normal hotplug/unplug, one via component ops and another via unsol > > event? > > Right, in case the unsol event actually works... > > I would argue that the normal case would be that the controller and > codec is in D3 which means that the unsol event never gets through - due > to hw limitations - which is what triggered this patch set in the first > place. > > But yes, in some case we might get double notification, but this should > not cause any trouble in practice. The unsol event could be turned off, > but would it be okay to save that for a later patch set (so I don't miss > the upcoming merge window)? In that case, it should be mentioned in the changelog at least. This series came a bit too late for the merge window, so I'm not sure whether this can get in. I personally find it OK, so take my ack for ALSA parts (patch 3/4), but the rest need review and ack from i915 guys. And we don't know who to merge this, if any. The changes are almost even to i915 and hda. I don't mind either way, via drm or sound tree. In anyway, Reviewed-by: Takashi Iwai thanks, Takashi > > > > > > thanks, > > > > Takashi > > > >> --- > >> sound/pci/hda/patch_hdmi.c | 22 +- > >> 1 file changed, 21 insertions(+), 1 deletion(-) > >> > >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > >> index a97db5f..932292c 100644 > >> --- a/sound/pci/hda/patch_hdmi.c > >> +++ b/sound/pci/hda/patch_hdmi.c > >> @@ -37,6 +37,8 @@ > >> #include > >> #include > >> #include > >> +#include > >> +#include > >> #include "hda_codec.h" > >> #include "hda_local.h" > >> #include "hda_jack.h" > >> @@ -144,6 +146,9 @@ struct hdmi_spec { > >> */ > >>struct hda_multi_out multiout; > >>struct hda_pcm_stream pcm_playback; > >> + > >> + /* i915/powerwell (Haswell+/Valleyview+) specific */ > >> + struct i915_audio_component_audio_ops i915_audio_ops; > >> }; > >> > >> > >> @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec > >> *codec) > >>struct hdmi_spec *spec = codec->spec; > >>int pin_idx; > >> > >> + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) > >> + snd_hdac_i915_register_notifier(NULL); > >> + > >>for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { > >>struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); > >> > >> @@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct > >> hda_codec *codec, hda_nid_t fg, > >>snd_hda_codec_set_power_to_all(codec, fg, power_state); > >> } > >> > >> +static void intel_pin_eld_notify(void *audio_ptr, int port, int > >> port_mst_index) > >> +{ > >> + struct hda_codec *codec = audio_ptr; > >> + int pin_nid = port + 0x04; > >> + > >> + check_presence_and_report(codec, pin_nid); > >> +} > >> + > >> static int patch_generic_hdmi(struct hda_codec *codec) > >> { > >>struct hdmi_spec *spec; > >> @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec > >> *codec) > >>if (is_valleyview_plus(codec) || is_skylake(codec)) > >>codec->core.link_power_control = 1; > >> > >> - if (is_haswell_plus(codec) || is_valleyview_plus(codec)) > >> + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { > >>codec->depop_delay = 0; > >> + spec->i915_audio_ops.audio_ptr = codec; > >> + spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify; > >> + snd_hdac_i915_register_notifier(&spec->i915_audio_ops); > >> + } > >> > >>if (hdmi_parse_codec(codec) < 0) { > >>codec->spec = NULL; > >> -- > >> 1.9.1 > >> > > > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On 2015-08-20 11:28, Takashi Iwai wrote: On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote: Whenever there is an event from the i915 driver, wake the codec and recheck plug/unplug + ELD status. This fixes the issue with lost unsol events in power save mode, the codec and controller can now sleep in D3 and still know when the HDMI monitor has been connected. Signed-off-by: David Henningsson This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event? Right, in case the unsol event actually works... I would argue that the normal case would be that the controller and codec is in D3 which means that the unsol event never gets through - due to hw limitations - which is what triggered this patch set in the first place. But yes, in some case we might get double notification, but this should not cause any trouble in practice. The unsol event could be turned off, but would it be okay to save that for a later patch set (so I don't miss the upcoming merge window)? thanks, Takashi --- sound/pci/hda/patch_hdmi.c | 22 +- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..932292c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,8 @@ #include #include #include +#include +#include #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" @@ -144,6 +146,9 @@ struct hdmi_spec { */ struct hda_multi_out multiout; struct hda_pcm_stream pcm_playback; + + /* i915/powerwell (Haswell+/Valleyview+) specific */ + struct i915_audio_component_audio_ops i915_audio_ops; }; @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec) struct hdmi_spec *spec = codec->spec; int pin_idx; + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) + snd_hdac_i915_register_notifier(NULL); + for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); @@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg, snd_hda_codec_set_power_to_all(codec, fg, power_state); } +static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index) +{ + struct hda_codec *codec = audio_ptr; + int pin_nid = port + 0x04; + + check_presence_and_report(codec, pin_nid); +} + static int patch_generic_hdmi(struct hda_codec *codec) { struct hdmi_spec *spec; @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_valleyview_plus(codec) || is_skylake(codec)) codec->core.link_power_control = 1; - if (is_haswell_plus(codec) || is_valleyview_plus(codec)) + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { codec->depop_delay = 0; + spec->i915_audio_ops.audio_ptr = codec; + spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify; + snd_hdac_i915_register_notifier(&spec->i915_audio_ops); + } if (hdmi_parse_codec(codec) < 0) { codec->spec = NULL; -- 1.9.1 -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote: > > Whenever there is an event from the i915 driver, wake the codec > and recheck plug/unplug + ELD status. > > This fixes the issue with lost unsol events in power save mode, > the codec and controller can now sleep in D3 and still know when > the HDMI monitor has been connected. > > Signed-off-by: David Henningsson This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event? thanks, Takashi > --- > sound/pci/hda/patch_hdmi.c | 22 +- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index a97db5f..932292c 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -37,6 +37,8 @@ > #include > #include > #include > +#include > +#include > #include "hda_codec.h" > #include "hda_local.h" > #include "hda_jack.h" > @@ -144,6 +146,9 @@ struct hdmi_spec { >*/ > struct hda_multi_out multiout; > struct hda_pcm_stream pcm_playback; > + > + /* i915/powerwell (Haswell+/Valleyview+) specific */ > + struct i915_audio_component_audio_ops i915_audio_ops; > }; > > > @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec) > struct hdmi_spec *spec = codec->spec; > int pin_idx; > > + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) > + snd_hdac_i915_register_notifier(NULL); > + > for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { > struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); > > @@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec > *codec, hda_nid_t fg, > snd_hda_codec_set_power_to_all(codec, fg, power_state); > } > > +static void intel_pin_eld_notify(void *audio_ptr, int port, int > port_mst_index) > +{ > + struct hda_codec *codec = audio_ptr; > + int pin_nid = port + 0x04; > + > + check_presence_and_report(codec, pin_nid); > +} > + > static int patch_generic_hdmi(struct hda_codec *codec) > { > struct hdmi_spec *spec; > @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) > if (is_valleyview_plus(codec) || is_skylake(codec)) > codec->core.link_power_control = 1; > > - if (is_haswell_plus(codec) || is_valleyview_plus(codec)) > + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { > codec->depop_delay = 0; > + spec->i915_audio_ops.audio_ptr = codec; > + spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify; > + snd_hdac_i915_register_notifier(&spec->i915_audio_ops); > + } > > if (hdmi_parse_codec(codec) < 0) { > codec->spec = NULL; > -- > 1.9.1 > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx