* [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 @ 2012-08-22 12:01 David Henningsson 2012-08-22 12:22 ` Takashi Iwai 0 siblings, 1 reply; 9+ messages in thread From: David Henningsson @ 2012-08-22 12:01 UTC (permalink / raw) To: tiwai, alsa-devel, 1040030; +Cc: 3.3+, David Henningsson The HDMI codec (an NVIDIA one in this case) forgot that its pins were unsol enabled, while it was suspended. Therefore jack detection was broken after S3. With this patch, we reenable the unsol events on resume, and also do an extra check afterwards, to see if the HDMI monitor was plugged/unplugged while in S3. Cc: stable@kernel.org (3.3+) BugLink: https://bugs.launchpad.net/bugs/1040030 Signed-off-by: David Henningsson <david.henningsson@canonical.com> --- sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8f23374..6a3ac05 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec) return 0; } +#ifdef CONFIG_PM +static int generic_hdmi_resume(struct hda_codec *codec) +{ + snd_hda_codec_resume_cache(codec); + snd_hda_jack_set_dirty_all(codec); + snd_hda_jack_report_sync(codec); + return 0; +} +#endif + static void generic_hdmi_free(struct hda_codec *codec) { struct hdmi_spec *spec = codec->spec; @@ -1338,6 +1348,9 @@ static const struct hda_codec_ops generic_hdmi_patch_ops = { .build_pcms = generic_hdmi_build_pcms, .build_controls = generic_hdmi_build_controls, .unsol_event = hdmi_unsol_event, +#ifdef CONFIG_PM + .resume = generic_hdmi_resume, +#endif }; static int patch_generic_hdmi(struct hda_codec *codec) -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 2012-08-22 12:01 [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 David Henningsson @ 2012-08-22 12:22 ` Takashi Iwai 2012-08-22 12:39 ` David Henningsson 0 siblings, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2012-08-22 12:22 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, 1040030, 3.3+ At Wed, 22 Aug 2012 14:01:41 +0200, David Henningsson wrote: > > The HDMI codec (an NVIDIA one in this case) forgot that its pins > were unsol enabled, while it was suspended. Therefore jack detection > was broken after S3. > With this patch, we reenable the unsol events on resume, > and also do an extra check afterwards, to see if the HDMI monitor was > plugged/unplugged while in S3. > > Cc: stable@kernel.org (3.3+) > BugLink: https://bugs.launchpad.net/bugs/1040030 > Signed-off-by: David Henningsson <david.henningsson@canonical.com> > --- > sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index 8f23374..6a3ac05 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec) > return 0; > } > > +#ifdef CONFIG_PM > +static int generic_hdmi_resume(struct hda_codec *codec) > +{ > + snd_hda_codec_resume_cache(codec); > + snd_hda_jack_set_dirty_all(codec); > + snd_hda_jack_report_sync(codec); > + return 0; Hm, is this really needed? snd_hda_jack_set_dirty_all() is already called in hda_call_codec_resume(), and snd_hda_jack_report_sync() is called in the init callback. Takashi > +} > +#endif > + > static void generic_hdmi_free(struct hda_codec *codec) > { > struct hdmi_spec *spec = codec->spec; > @@ -1338,6 +1348,9 @@ static const struct hda_codec_ops generic_hdmi_patch_ops = { > .build_pcms = generic_hdmi_build_pcms, > .build_controls = generic_hdmi_build_controls, > .unsol_event = hdmi_unsol_event, > +#ifdef CONFIG_PM > + .resume = generic_hdmi_resume, > +#endif > }; > > static int patch_generic_hdmi(struct hda_codec *codec) > -- > 1.7.9.5 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 2012-08-22 12:22 ` Takashi Iwai @ 2012-08-22 12:39 ` David Henningsson 2012-08-22 12:58 ` Takashi Iwai 0 siblings, 1 reply; 9+ messages in thread From: David Henningsson @ 2012-08-22 12:39 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, 1040030 On 08/22/2012 02:22 PM, Takashi Iwai wrote: > At Wed, 22 Aug 2012 14:01:41 +0200, > David Henningsson wrote: >> >> The HDMI codec (an NVIDIA one in this case) forgot that its pins >> were unsol enabled, while it was suspended. Therefore jack detection >> was broken after S3. >> With this patch, we reenable the unsol events on resume, >> and also do an extra check afterwards, to see if the HDMI monitor was >> plugged/unplugged while in S3. >> >> Cc: stable@kernel.org (3.3+) >> BugLink: https://bugs.launchpad.net/bugs/1040030 >> Signed-off-by: David Henningsson <david.henningsson@canonical.com> >> --- >> sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >> index 8f23374..6a3ac05 100644 >> --- a/sound/pci/hda/patch_hdmi.c >> +++ b/sound/pci/hda/patch_hdmi.c >> @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec) >> return 0; >> } >> >> +#ifdef CONFIG_PM >> +static int generic_hdmi_resume(struct hda_codec *codec) >> +{ >> + snd_hda_codec_resume_cache(codec); >> + snd_hda_jack_set_dirty_all(codec); >> + snd_hda_jack_report_sync(codec); >> + return 0; > > Hm, is this really needed? > > snd_hda_jack_set_dirty_all() is already called in > hda_call_codec_resume(), and snd_hda_jack_report_sync() is called in > the init callback. The tester (who has the hardware) has gone for the day, so I can't really verify different scenarios right now, but after having looked at hda_call_codec_resume I see what you mean... I do notice one difference though - the order. snd_hda_codec_resume_cache, which is what writes the unsol_enable verbs, should probably be before the set_dirty_all / report_sync. If not for anything else, so for the race condition of somebody plugging/unplugging the monitor after checking the jack but before the unsol is enabled. Could it be that the card gives wrong info if unsol is not enabled, or perhaps needs SET_PIN_SENSE in that case? I don't know. But yeah, I'll have to wait with this patch then until I've verified more things. Thanks. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 2012-08-22 12:39 ` David Henningsson @ 2012-08-22 12:58 ` Takashi Iwai 2012-08-22 13:46 ` David Henningsson 0 siblings, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2012-08-22 12:58 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, 1040030 At Wed, 22 Aug 2012 14:39:17 +0200, David Henningsson wrote: > > On 08/22/2012 02:22 PM, Takashi Iwai wrote: > > At Wed, 22 Aug 2012 14:01:41 +0200, > > David Henningsson wrote: > >> > >> The HDMI codec (an NVIDIA one in this case) forgot that its pins > >> were unsol enabled, while it was suspended. Therefore jack detection > >> was broken after S3. > >> With this patch, we reenable the unsol events on resume, > >> and also do an extra check afterwards, to see if the HDMI monitor was > >> plugged/unplugged while in S3. > >> > >> Cc: stable@kernel.org (3.3+) > >> BugLink: https://bugs.launchpad.net/bugs/1040030 > >> Signed-off-by: David Henningsson <david.henningsson@canonical.com> > >> --- > >> sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > >> index 8f23374..6a3ac05 100644 > >> --- a/sound/pci/hda/patch_hdmi.c > >> +++ b/sound/pci/hda/patch_hdmi.c > >> @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec) > >> return 0; > >> } > >> > >> +#ifdef CONFIG_PM > >> +static int generic_hdmi_resume(struct hda_codec *codec) > >> +{ > >> + snd_hda_codec_resume_cache(codec); > >> + snd_hda_jack_set_dirty_all(codec); > >> + snd_hda_jack_report_sync(codec); > >> + return 0; > > > > Hm, is this really needed? > > > > snd_hda_jack_set_dirty_all() is already called in > > hda_call_codec_resume(), and snd_hda_jack_report_sync() is called in > > the init callback. > > The tester (who has the hardware) has gone for the day, so I can't > really verify different scenarios right now, but after having looked at > hda_call_codec_resume I see what you mean... > > I do notice one difference though - the order. > snd_hda_codec_resume_cache, which is what writes the unsol_enable verbs, > should probably be before the set_dirty_all / report_sync. If not for > anything else, so for the race condition of somebody plugging/unplugging > the monitor after checking the jack but before the unsol is enabled. Calling snd_hda_jack_report_sync() in init means that all jacks are checked at first, then the caches are resumed. So this won't change ENABLE_UNSOL verb setups. The bug report doesn't give any details, so it's hard to guess what actually doesn't work. alsa-info.sh output before and after suspend would be helpful to understand. Takashi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 2012-08-22 12:58 ` Takashi Iwai @ 2012-08-22 13:46 ` David Henningsson 2012-08-22 13:52 ` Takashi Iwai 0 siblings, 1 reply; 9+ messages in thread From: David Henningsson @ 2012-08-22 13:46 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, 1040030 On 08/22/2012 02:58 PM, Takashi Iwai wrote: > At Wed, 22 Aug 2012 14:39:17 +0200, > David Henningsson wrote: >> >> On 08/22/2012 02:22 PM, Takashi Iwai wrote: >>> At Wed, 22 Aug 2012 14:01:41 +0200, >>> David Henningsson wrote: >>>> >>>> The HDMI codec (an NVIDIA one in this case) forgot that its pins >>>> were unsol enabled, while it was suspended. Therefore jack detection >>>> was broken after S3. >>>> With this patch, we reenable the unsol events on resume, >>>> and also do an extra check afterwards, to see if the HDMI monitor was >>>> plugged/unplugged while in S3. >>>> >>>> Cc: stable@kernel.org (3.3+) >>>> BugLink: https://bugs.launchpad.net/bugs/1040030 >>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> >>>> --- >>>> sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >>>> index 8f23374..6a3ac05 100644 >>>> --- a/sound/pci/hda/patch_hdmi.c >>>> +++ b/sound/pci/hda/patch_hdmi.c >>>> @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec) >>>> return 0; >>>> } >>>> >>>> +#ifdef CONFIG_PM >>>> +static int generic_hdmi_resume(struct hda_codec *codec) >>>> +{ >>>> + snd_hda_codec_resume_cache(codec); >>>> + snd_hda_jack_set_dirty_all(codec); >>>> + snd_hda_jack_report_sync(codec); >>>> + return 0; >>> >>> Hm, is this really needed? >>> >>> snd_hda_jack_set_dirty_all() is already called in >>> hda_call_codec_resume(), and snd_hda_jack_report_sync() is called in >>> the init callback. >> >> The tester (who has the hardware) has gone for the day, so I can't >> really verify different scenarios right now, but after having looked at >> hda_call_codec_resume I see what you mean... >> >> I do notice one difference though - the order. >> snd_hda_codec_resume_cache, which is what writes the unsol_enable verbs, >> should probably be before the set_dirty_all / report_sync. If not for >> anything else, so for the race condition of somebody plugging/unplugging >> the monitor after checking the jack but before the unsol is enabled. > > Calling snd_hda_jack_report_sync() in init means that all jacks are > checked at first, then the caches are resumed. So this won't change > ENABLE_UNSOL verb setups. I'm not sure I'm following. Here's the order in hda_call_codec_resume: snd_hda_jack_set_dirty_all() generic_hdmi_init -> snd_hda_jack_report_sync() - get_pin_sense snd_hda_codec_resume_cache() - set_unsol_enable With the (theoretical?) race condition being a change of pin sense between snd_hda_jack_report_sync() and snd_hda_codec_resume_cache(), which will not be picked up. The patch changed the order to: snd_hda_codec_resume_cache() - set_unsol_enable snd_hda_jack_set_dirty_all() snd_hda_jack_report_sync() - get_pin_sense Which eliminates that race condition. > The bug report doesn't give any details, so it's hard to guess what > actually doesn't work. alsa-info.sh output before and after suspend > would be helpful to understand. Sorry about that. The machine is not yet released, that's why. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 2012-08-22 13:46 ` David Henningsson @ 2012-08-22 13:52 ` Takashi Iwai 2012-08-22 14:27 ` David Henningsson 0 siblings, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2012-08-22 13:52 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, 1040030 At Wed, 22 Aug 2012 15:46:59 +0200, David Henningsson wrote: > > On 08/22/2012 02:58 PM, Takashi Iwai wrote: > > At Wed, 22 Aug 2012 14:39:17 +0200, > > David Henningsson wrote: > >> > >> On 08/22/2012 02:22 PM, Takashi Iwai wrote: > >>> At Wed, 22 Aug 2012 14:01:41 +0200, > >>> David Henningsson wrote: > >>>> > >>>> The HDMI codec (an NVIDIA one in this case) forgot that its pins > >>>> were unsol enabled, while it was suspended. Therefore jack detection > >>>> was broken after S3. > >>>> With this patch, we reenable the unsol events on resume, > >>>> and also do an extra check afterwards, to see if the HDMI monitor was > >>>> plugged/unplugged while in S3. > >>>> > >>>> Cc: stable@kernel.org (3.3+) > >>>> BugLink: https://bugs.launchpad.net/bugs/1040030 > >>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> > >>>> --- > >>>> sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ > >>>> 1 file changed, 13 insertions(+) > >>>> > >>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > >>>> index 8f23374..6a3ac05 100644 > >>>> --- a/sound/pci/hda/patch_hdmi.c > >>>> +++ b/sound/pci/hda/patch_hdmi.c > >>>> @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec) > >>>> return 0; > >>>> } > >>>> > >>>> +#ifdef CONFIG_PM > >>>> +static int generic_hdmi_resume(struct hda_codec *codec) > >>>> +{ > >>>> + snd_hda_codec_resume_cache(codec); > >>>> + snd_hda_jack_set_dirty_all(codec); > >>>> + snd_hda_jack_report_sync(codec); > >>>> + return 0; > >>> > >>> Hm, is this really needed? > >>> > >>> snd_hda_jack_set_dirty_all() is already called in > >>> hda_call_codec_resume(), and snd_hda_jack_report_sync() is called in > >>> the init callback. > >> > >> The tester (who has the hardware) has gone for the day, so I can't > >> really verify different scenarios right now, but after having looked at > >> hda_call_codec_resume I see what you mean... > >> > >> I do notice one difference though - the order. > >> snd_hda_codec_resume_cache, which is what writes the unsol_enable verbs, > >> should probably be before the set_dirty_all / report_sync. If not for > >> anything else, so for the race condition of somebody plugging/unplugging > >> the monitor after checking the jack but before the unsol is enabled. > > > > Calling snd_hda_jack_report_sync() in init means that all jacks are > > checked at first, then the caches are resumed. So this won't change > > ENABLE_UNSOL verb setups. > > I'm not sure I'm following. Here's the order in hda_call_codec_resume: > > snd_hda_jack_set_dirty_all() > generic_hdmi_init -> snd_hda_jack_report_sync() - get_pin_sense > snd_hda_codec_resume_cache() - set_unsol_enable > > With the (theoretical?) race condition being a change of pin sense > between snd_hda_jack_report_sync() and snd_hda_codec_resume_cache(), > which will not be picked up. In that race, yes. But this should be irrelevant with this bug :) > The patch changed the order to: > > snd_hda_codec_resume_cache() - set_unsol_enable > snd_hda_jack_set_dirty_all() > snd_hda_jack_report_sync() - get_pin_sense > > Which eliminates that race condition. Well, what I'm thinking now is rather to call snd_hda_jack_report_sync() generically in hda_call_codec_resume(). The call doesn't have to be in all places. Ditto for the initialization case. It'll clean up many lines. Takashi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 2012-08-22 13:52 ` Takashi Iwai @ 2012-08-22 14:27 ` David Henningsson 2012-08-22 14:49 ` Takashi Iwai 0 siblings, 1 reply; 9+ messages in thread From: David Henningsson @ 2012-08-22 14:27 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, 1040030 On 08/22/2012 03:52 PM, Takashi Iwai wrote: > At Wed, 22 Aug 2012 15:46:59 +0200, > David Henningsson wrote: >> >> On 08/22/2012 02:58 PM, Takashi Iwai wrote: >>> At Wed, 22 Aug 2012 14:39:17 +0200, >>> David Henningsson wrote: >>>> >>>> On 08/22/2012 02:22 PM, Takashi Iwai wrote: >>>>> At Wed, 22 Aug 2012 14:01:41 +0200, >>>>> David Henningsson wrote: >>>>>> >>>>>> The HDMI codec (an NVIDIA one in this case) forgot that its pins >>>>>> were unsol enabled, while it was suspended. Therefore jack detection >>>>>> was broken after S3. >>>>>> With this patch, we reenable the unsol events on resume, >>>>>> and also do an extra check afterwards, to see if the HDMI monitor was >>>>>> plugged/unplugged while in S3. >>>>>> >>>>>> Cc: stable@kernel.org (3.3+) >>>>>> BugLink: https://bugs.launchpad.net/bugs/1040030 >>>>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> >>>>>> --- >>>>>> sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ >>>>>> 1 file changed, 13 insertions(+) >>>>>> >>>>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c >>>>>> index 8f23374..6a3ac05 100644 >>>>>> --- a/sound/pci/hda/patch_hdmi.c >>>>>> +++ b/sound/pci/hda/patch_hdmi.c >>>>>> @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +#ifdef CONFIG_PM >>>>>> +static int generic_hdmi_resume(struct hda_codec *codec) >>>>>> +{ >>>>>> + snd_hda_codec_resume_cache(codec); >>>>>> + snd_hda_jack_set_dirty_all(codec); >>>>>> + snd_hda_jack_report_sync(codec); >>>>>> + return 0; >>>>> >>>>> Hm, is this really needed? >>>>> >>>>> snd_hda_jack_set_dirty_all() is already called in >>>>> hda_call_codec_resume(), and snd_hda_jack_report_sync() is called in >>>>> the init callback. >>>> >>>> The tester (who has the hardware) has gone for the day, so I can't >>>> really verify different scenarios right now, but after having looked at >>>> hda_call_codec_resume I see what you mean... >>>> >>>> I do notice one difference though - the order. >>>> snd_hda_codec_resume_cache, which is what writes the unsol_enable verbs, >>>> should probably be before the set_dirty_all / report_sync. If not for >>>> anything else, so for the race condition of somebody plugging/unplugging >>>> the monitor after checking the jack but before the unsol is enabled. >>> >>> Calling snd_hda_jack_report_sync() in init means that all jacks are >>> checked at first, then the caches are resumed. So this won't change >>> ENABLE_UNSOL verb setups. >> >> I'm not sure I'm following. Here's the order in hda_call_codec_resume: >> >> snd_hda_jack_set_dirty_all() >> generic_hdmi_init -> snd_hda_jack_report_sync() - get_pin_sense >> snd_hda_codec_resume_cache() - set_unsol_enable >> >> With the (theoretical?) race condition being a change of pin sense >> between snd_hda_jack_report_sync() and snd_hda_codec_resume_cache(), >> which will not be picked up. > > In that race, yes. But this should be irrelevant with this bug :) Yes, the bug needs further verification. >> The patch changed the order to: >> >> snd_hda_codec_resume_cache() - set_unsol_enable >> snd_hda_jack_set_dirty_all() >> snd_hda_jack_report_sync() - get_pin_sense >> >> Which eliminates that race condition. > > Well, what I'm thinking now is rather to call > snd_hda_jack_report_sync() generically in hda_call_codec_resume(). > The call doesn't have to be in all places. > Ditto for the initialization case. It'll clean up many lines. Fine with me - the more code being generic, the better. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 2012-08-22 14:27 ` David Henningsson @ 2012-08-22 14:49 ` Takashi Iwai 2012-08-22 17:00 ` David Henningsson 0 siblings, 1 reply; 9+ messages in thread From: Takashi Iwai @ 2012-08-22 14:49 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, 1040030 At Wed, 22 Aug 2012 16:27:35 +0200, David Henningsson wrote: > > On 08/22/2012 03:52 PM, Takashi Iwai wrote: > > At Wed, 22 Aug 2012 15:46:59 +0200, > > David Henningsson wrote: > >> > >> On 08/22/2012 02:58 PM, Takashi Iwai wrote: > >>> At Wed, 22 Aug 2012 14:39:17 +0200, > >>> David Henningsson wrote: > >>>> > >>>> On 08/22/2012 02:22 PM, Takashi Iwai wrote: > >>>>> At Wed, 22 Aug 2012 14:01:41 +0200, > >>>>> David Henningsson wrote: > >>>>>> > >>>>>> The HDMI codec (an NVIDIA one in this case) forgot that its pins > >>>>>> were unsol enabled, while it was suspended. Therefore jack detection > >>>>>> was broken after S3. > >>>>>> With this patch, we reenable the unsol events on resume, > >>>>>> and also do an extra check afterwards, to see if the HDMI monitor was > >>>>>> plugged/unplugged while in S3. > >>>>>> > >>>>>> Cc: stable@kernel.org (3.3+) > >>>>>> BugLink: https://bugs.launchpad.net/bugs/1040030 > >>>>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com> > >>>>>> --- > >>>>>> sound/pci/hda/patch_hdmi.c | 13 +++++++++++++ > >>>>>> 1 file changed, 13 insertions(+) > >>>>>> > >>>>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > >>>>>> index 8f23374..6a3ac05 100644 > >>>>>> --- a/sound/pci/hda/patch_hdmi.c > >>>>>> +++ b/sound/pci/hda/patch_hdmi.c > >>>>>> @@ -1315,6 +1315,16 @@ static int generic_hdmi_init(struct hda_codec *codec) > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> +#ifdef CONFIG_PM > >>>>>> +static int generic_hdmi_resume(struct hda_codec *codec) > >>>>>> +{ > >>>>>> + snd_hda_codec_resume_cache(codec); > >>>>>> + snd_hda_jack_set_dirty_all(codec); > >>>>>> + snd_hda_jack_report_sync(codec); > >>>>>> + return 0; > >>>>> > >>>>> Hm, is this really needed? > >>>>> > >>>>> snd_hda_jack_set_dirty_all() is already called in > >>>>> hda_call_codec_resume(), and snd_hda_jack_report_sync() is called in > >>>>> the init callback. > >>>> > >>>> The tester (who has the hardware) has gone for the day, so I can't > >>>> really verify different scenarios right now, but after having looked at > >>>> hda_call_codec_resume I see what you mean... > >>>> > >>>> I do notice one difference though - the order. > >>>> snd_hda_codec_resume_cache, which is what writes the unsol_enable verbs, > >>>> should probably be before the set_dirty_all / report_sync. If not for > >>>> anything else, so for the race condition of somebody plugging/unplugging > >>>> the monitor after checking the jack but before the unsol is enabled. > >>> > >>> Calling snd_hda_jack_report_sync() in init means that all jacks are > >>> checked at first, then the caches are resumed. So this won't change > >>> ENABLE_UNSOL verb setups. > >> > >> I'm not sure I'm following. Here's the order in hda_call_codec_resume: > >> > >> snd_hda_jack_set_dirty_all() > >> generic_hdmi_init -> snd_hda_jack_report_sync() - get_pin_sense > >> snd_hda_codec_resume_cache() - set_unsol_enable > >> > >> With the (theoretical?) race condition being a change of pin sense > >> between snd_hda_jack_report_sync() and snd_hda_codec_resume_cache(), > >> which will not be picked up. > > > > In that race, yes. But this should be irrelevant with this bug :) > > Yes, the bug needs further verification. > > >> The patch changed the order to: > >> > >> snd_hda_codec_resume_cache() - set_unsol_enable > >> snd_hda_jack_set_dirty_all() > >> snd_hda_jack_report_sync() - get_pin_sense > >> > >> Which eliminates that race condition. > > > > Well, what I'm thinking now is rather to call > > snd_hda_jack_report_sync() generically in hda_call_codec_resume(). > > The call doesn't have to be in all places. > > Ditto for the initialization case. It'll clean up many lines. > > Fine with me - the more code being generic, the better. OK, here we go. Takashi === From: Takashi Iwai <tiwai@suse.de> Subject: [PATCH] ALSA: hda - Call snd_hda_jack_report_sync() generically in hda_codec.c Instead of calling the jack sync in the init callback of each codec, call it generically at initialization and resume. By calling it at the last of resume sequence, a possible race between the jack sync and the unsol event enablement in the current code will be closed, too. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/pci/hda/hda_codec.c | 2 ++ sound/pci/hda/patch_cirrus.c | 2 -- sound/pci/hda/patch_conexant.c | 1 - sound/pci/hda/patch_hdmi.c | 2 -- sound/pci/hda/patch_realtek.c | 2 -- sound/pci/hda/patch_sigmatel.c | 2 -- sound/pci/hda/patch_via.c | 1 - 7 files changed, 2 insertions(+), 10 deletions(-) diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index a6c34dc..4efd271 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3618,6 +3618,7 @@ static void hda_call_codec_resume(struct hda_codec *codec) snd_hda_codec_resume_amp(codec); snd_hda_codec_resume_cache(codec); } + snd_hda_jack_report_sync(codec); snd_hda_power_down(codec); /* flag down before returning */ } #endif /* CONFIG_PM */ @@ -3663,6 +3664,7 @@ int snd_hda_codec_build_controls(struct hda_codec *codec) err = codec->patch_ops.build_controls(codec); if (err < 0) return err; + snd_hda_jack_report_sync(codec); /* call at the last init point */ return 0; } diff --git a/sound/pci/hda/patch_cirrus.c b/sound/pci/hda/patch_cirrus.c index 0c4c1a6..0bddb3e 100644 --- a/sound/pci/hda/patch_cirrus.c +++ b/sound/pci/hda/patch_cirrus.c @@ -1193,7 +1193,6 @@ static int cs_init(struct hda_codec *codec) init_output(codec); init_input(codec); init_digital(codec); - snd_hda_jack_report_sync(codec); return 0; } @@ -1643,7 +1642,6 @@ static int cs421x_init(struct hda_codec *codec) init_output(codec); init_input(codec); init_cs421x_digital(codec); - snd_hda_jack_report_sync(codec); return 0; } diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c index 5e22a8f..172895a 100644 --- a/sound/pci/hda/patch_conexant.c +++ b/sound/pci/hda/patch_conexant.c @@ -4061,7 +4061,6 @@ static int cx_auto_init(struct hda_codec *codec) cx_auto_init_output(codec); cx_auto_init_input(codec); cx_auto_init_digital(codec); - snd_hda_jack_report_sync(codec); snd_hda_sync_vmaster_hook(&spec->vmaster_mute); return 0; } diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8f23374..d9439c5 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1311,7 +1311,6 @@ static int generic_hdmi_init(struct hda_codec *codec) hdmi_init_pin(codec, pin_nid); snd_hda_jack_detect_enable(codec, pin_nid, pin_nid); } - snd_hda_jack_report_sync(codec); return 0; } @@ -1428,7 +1427,6 @@ static int simple_playback_init(struct hda_codec *codec) snd_hda_codec_write(codec, pin, 0, AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE); snd_hda_jack_detect_enable(codec, pin, pin); - snd_hda_jack_report_sync(codec); return 0; } diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 4f81dd4..ce99cc9 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -2053,8 +2053,6 @@ static int alc_init(struct hda_codec *codec) alc_apply_fixup(codec, ALC_FIXUP_ACT_INIT); - snd_hda_jack_report_sync(codec); - hda_call_check_power_status(codec, 0x01); return 0; } diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c index ea5775a..4352954 100644 --- a/sound/pci/hda/patch_sigmatel.c +++ b/sound/pci/hda/patch_sigmatel.c @@ -4418,8 +4418,6 @@ static int stac92xx_init(struct hda_codec *codec) stac_toggle_power_map(codec, nid, 0); } - snd_hda_jack_report_sync(codec); - /* sync mute LED */ if (spec->gpio_led) { if (spec->vmaster_mute.hook) diff --git a/sound/pci/hda/patch_via.c b/sound/pci/hda/patch_via.c index 4307717..4b0796b 100644 --- a/sound/pci/hda/patch_via.c +++ b/sound/pci/hda/patch_via.c @@ -2815,7 +2815,6 @@ static int via_init(struct hda_codec *codec) via_hp_automute(codec); vt1708_update_hp_work(spec); - snd_hda_jack_report_sync(codec); return 0; } -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 2012-08-22 14:49 ` Takashi Iwai @ 2012-08-22 17:00 ` David Henningsson 0 siblings, 0 replies; 9+ messages in thread From: David Henningsson @ 2012-08-22 17:00 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, 1040030 On 08/22/2012 04:49 PM, Takashi Iwai wrote: > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH] ALSA: hda - Call snd_hda_jack_report_sync() generically in hda_codec.c > > Instead of calling the jack sync in the init callback of each codec, > call it generically at initialization and resume. By calling it at > the last of resume sequence, a possible race between the jack sync and > the unsol event enablement in the current code will be closed, too. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> Acked-by: David Henningsson <david.henningsson@canonical.com> > --- > sound/pci/hda/hda_codec.c | 2 ++ > sound/pci/hda/patch_cirrus.c | 2 -- > sound/pci/hda/patch_conexant.c | 1 - > sound/pci/hda/patch_hdmi.c | 2 -- > sound/pci/hda/patch_realtek.c | 2 -- > sound/pci/hda/patch_sigmatel.c | 2 -- > sound/pci/hda/patch_via.c | 1 - > 7 files changed, 2 insertions(+), 10 deletions(-) > > diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c > index a6c34dc..4efd271 100644 > --- a/sound/pci/hda/hda_codec.c > +++ b/sound/pci/hda/hda_codec.c > @@ -3618,6 +3618,7 @@ static void hda_call_codec_resume(struct hda_codec *codec) > snd_hda_codec_resume_amp(codec); > snd_hda_codec_resume_cache(codec); > } > + snd_hda_jack_report_sync(codec); > snd_hda_power_down(codec); /* flag down before returning */ > } > #endif /* CONFIG_PM */ > @@ -3663,6 +3664,7 @@ int snd_hda_codec_build_controls(struct hda_codec *codec) > err = codec->patch_ops.build_controls(codec); > if (err < 0) > return err; > + snd_hda_jack_report_sync(codec); /* call at the last init point */ > return 0; > } > > diff --git a/sound/pci/hda/patch_cirrus.c b/sound/pci/hda/patch_cirrus.c > index 0c4c1a6..0bddb3e 100644 > --- a/sound/pci/hda/patch_cirrus.c > +++ b/sound/pci/hda/patch_cirrus.c > @@ -1193,7 +1193,6 @@ static int cs_init(struct hda_codec *codec) > init_output(codec); > init_input(codec); > init_digital(codec); > - snd_hda_jack_report_sync(codec); > > return 0; > } > @@ -1643,7 +1642,6 @@ static int cs421x_init(struct hda_codec *codec) > init_output(codec); > init_input(codec); > init_cs421x_digital(codec); > - snd_hda_jack_report_sync(codec); > > return 0; > } > diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c > index 5e22a8f..172895a 100644 > --- a/sound/pci/hda/patch_conexant.c > +++ b/sound/pci/hda/patch_conexant.c > @@ -4061,7 +4061,6 @@ static int cx_auto_init(struct hda_codec *codec) > cx_auto_init_output(codec); > cx_auto_init_input(codec); > cx_auto_init_digital(codec); > - snd_hda_jack_report_sync(codec); > snd_hda_sync_vmaster_hook(&spec->vmaster_mute); > return 0; > } > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index 8f23374..d9439c5 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -1311,7 +1311,6 @@ static int generic_hdmi_init(struct hda_codec *codec) > hdmi_init_pin(codec, pin_nid); > snd_hda_jack_detect_enable(codec, pin_nid, pin_nid); > } > - snd_hda_jack_report_sync(codec); > return 0; > } > > @@ -1428,7 +1427,6 @@ static int simple_playback_init(struct hda_codec *codec) > snd_hda_codec_write(codec, pin, 0, AC_VERB_SET_AMP_GAIN_MUTE, > AMP_OUT_UNMUTE); > snd_hda_jack_detect_enable(codec, pin, pin); > - snd_hda_jack_report_sync(codec); > return 0; > } > > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c > index 4f81dd4..ce99cc9 100644 > --- a/sound/pci/hda/patch_realtek.c > +++ b/sound/pci/hda/patch_realtek.c > @@ -2053,8 +2053,6 @@ static int alc_init(struct hda_codec *codec) > > alc_apply_fixup(codec, ALC_FIXUP_ACT_INIT); > > - snd_hda_jack_report_sync(codec); > - > hda_call_check_power_status(codec, 0x01); > return 0; > } > diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c > index ea5775a..4352954 100644 > --- a/sound/pci/hda/patch_sigmatel.c > +++ b/sound/pci/hda/patch_sigmatel.c > @@ -4418,8 +4418,6 @@ static int stac92xx_init(struct hda_codec *codec) > stac_toggle_power_map(codec, nid, 0); > } > > - snd_hda_jack_report_sync(codec); > - > /* sync mute LED */ > if (spec->gpio_led) { > if (spec->vmaster_mute.hook) > diff --git a/sound/pci/hda/patch_via.c b/sound/pci/hda/patch_via.c > index 4307717..4b0796b 100644 > --- a/sound/pci/hda/patch_via.c > +++ b/sound/pci/hda/patch_via.c > @@ -2815,7 +2815,6 @@ static int via_init(struct hda_codec *codec) > > via_hp_automute(codec); > vt1708_update_hp_work(spec); > - snd_hda_jack_report_sync(codec); > > return 0; > } > -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-08-22 17:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-08-22 12:01 [PATCH] ALSA: hda - fix broken HDMI jack detection after S3 David Henningsson 2012-08-22 12:22 ` Takashi Iwai 2012-08-22 12:39 ` David Henningsson 2012-08-22 12:58 ` Takashi Iwai 2012-08-22 13:46 ` David Henningsson 2012-08-22 13:52 ` Takashi Iwai 2012-08-22 14:27 ` David Henningsson 2012-08-22 14:49 ` Takashi Iwai 2012-08-22 17:00 ` David Henningsson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.