[alsa-devel] [PATCH] ALSA: hda - fix broken HDMI jack detection after S3

[Takashi Iwai]

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 at kernel.org (3.3+)
> >>>> BugLink: https://bugs.launchpad.net/bugs/1040030
> >>>> Signed-off-by: David Henningsson <david.henningsson at 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