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: <email address hidden> (3.3+)
>>>> BugLink: https://bugs.launchpad.net/bugs/1040030
>>>> Signed-off-by: David Henningsson <email address hidden>
>>>> ---
>>>> 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:
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 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.
On 08/22/2012 02:58 PM, Takashi Iwai wrote: /bugs.launchpad .net/bugs/ 1040030 hda/patch_ hdmi.c | 13 +++++++++++++ pci/hda/ patch_hdmi. c b/sound/ pci/hda/ patch_hdmi. c pci/hda/ patch_hdmi. c pci/hda/ patch_hdmi. c hdmi_init( struct hda_codec *codec) hdmi_resume( struct hda_codec *codec) codec_resume_ cache(codec) ; jack_set_ dirty_all( codec); jack_report_ sync(codec) ; jack_set_ dirty_all( ) is already called in codec_resume( ), and snd_hda_ jack_report_ sync() is called in codec_resume I see what you mean... codec_resume_ cache, which is what writes the unsol_enable verbs, jack_report_ sync() in init means that all jacks are
> 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: <email address hidden> (3.3+)
>>>> BugLink: https:/
>>>> Signed-off-by: David Henningsson <email address hidden>
>>>> ---
>>>> sound/pci/
>>>> 1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/sound/
>>>> index 8f23374..6a3ac05 100644
>>>> --- a/sound/
>>>> +++ b/sound/
>>>> @@ -1315,6 +1315,16 @@ static int generic_
>>>> return 0;
>>>> }
>>>>
>>>> +#ifdef CONFIG_PM
>>>> +static int generic_
>>>> +{
>>>> + snd_hda_
>>>> + snd_hda_
>>>> + snd_hda_
>>>> + return 0;
>>>
>>> Hm, is this really needed?
>>>
>>> snd_hda_
>>> hda_call_
>>> 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_
>>
>> I do notice one difference though - the order.
>> snd_hda_
>> 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_
> 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( ) jack_report_ sync() - get_pin_sense codec_resume_ cache() - set_unsol_enable
generic_hdmi_init -> snd_hda_
snd_hda_
With the (theoretical?) race condition being a change of pin sense jack_report_ sync() and snd_hda_ codec_resume_ cache() ,
between snd_hda_
which will not be picked up.
The patch changed the order to:
snd_hda_ codec_resume_ cache() - set_unsol_enable jack_set_ dirty_all( ) jack_report_ sync() - get_pin_sense
snd_hda_
snd_hda_
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.
-- /launchpad. net/~diwic
David Henningsson, Canonical Ltd.
https:/