Bug 198503

Created attachment 273675 [details]
Linux messages

Could you confirm that reverting the commit fixes the regression?

Also could you give the alsa-info.sh output?  Run with --no-upload option, and attach to Bugzilla.

Created attachment 273683 [details]
Output of https://www.alsa-project.org/alsa-info.sh with `--no-upload`

(In reply to Takashi Iwai from comment #2)
> Could you confirm that reverting the commit fixes the regression?

No, but I can’t remember these long times from older releases. Unfortunately, using prebuild Linux 4.15-rcX kernels from Ubuntu won’t work, as there was a regression in FTRACE. Rebuilding the Linux kernel will take some time. Let’s see if I get to it before the weekend.

> Also could you give the alsa-info.sh output?  Run with --no-upload option,
> and attach to Bugzilla.

Sure, please find it attached.

With Linux 4.13.0-26-generic from Ubuntu 16.04 LTS, the times are much smaller.

> snd_hda_codec_realtek @ hdaudioC0D0 {snd_hda_codec_realtek} async_device
> (Total Suspend: 23.140 ms Total Resume: 39.798 ms)

Thanks.  Judging from alsa-info.sh output, the commit you mentioned is very unlikely the culprit.  Your codec ID is 0x10ec0256, while the patch changed the behavior for 0x10ec0215, 0x10ec0225, 0x10ec0295 and 0x10ec0299.

It seems happening in alc_update_coef_idx() call in alc256_shutup(), and this was introduced by the commit 4a219ef8f37071ce0993959fe52ffa5034689f42
    ALSA: hda/realtek - Add ALC256 HP depop function

Could you try to swap alc_update_coef_idx() call and snd_hda_codec_write() call?

--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -3154,11 +3154,11 @@ static void alc256_shutup(struct hda_codec *codec)
 	if (hp_pin_sense)
 		msleep(85);
 
+	alc_update_coef_idx(codec, 0x46, 0, 3 << 12); /* 3k pull low control for Headset jack. */
+
 	snd_hda_codec_write(codec, hp_pin, 0,
 			    AC_VERB_SET_PIN_WIDGET_CONTROL, 0x0);
 
-	alc_update_coef_idx(codec, 0x46, 0, 3 << 12); /* 3k pull low control for Headset jack. */
-
 	if (hp_pin_sense)
 		msleep(100);

In anyway, I put Kailang to Cc.  He must know better of this.

(In reply to Takashi Iwai from comment #5)
> Thanks.  Judging from alsa-info.sh output, the commit you mentioned is very
> unlikely the culprit.  Your codec ID is 0x10ec0256, while the patch changed
> the behavior for 0x10ec0215, 0x10ec0225, 0x10ec0295 and 0x10ec0299.
> 
> It seems happening in alc_update_coef_idx() call in alc256_shutup(), and
> this was introduced by the commit 4a219ef8f37071ce0993959fe52ffa5034689f42
>     ALSA: hda/realtek - Add ALC256 HP depop function

This commit is present since Linux 4.13-rc1, so it’s strange that I can’t reproduce it with Linux 4.13. (But maybe it doesn’t happen always.)

> Could you try to swap alc_update_coef_idx() call and snd_hda_codec_write()
> call?
> 
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -3154,11 +3154,11 @@ static void alc256_shutup(struct hda_codec *codec)
>       if (hp_pin_sense)
>               msleep(85);
>  
> +     alc_update_coef_idx(codec, 0x46, 0, 3 << 12); /* 3k pull low control
> for
> Headset jack. */
> +
>       snd_hda_codec_write(codec, hp_pin, 0,
>                           AC_VERB_SET_PIN_WIDGET_CONTROL, 0x0);
>  
> -     alc_update_coef_idx(codec, 0x46, 0, 3 << 12); /* 3k pull low control
> for
> Headset jack. */
> -
>       if (hp_pin_sense)
>               msleep(100);

Ok, I’ll try that.

(In reply to Paul Menzel from comment #7)
> (In reply to Takashi Iwai from comment #5)
> > Thanks.  Judging from alsa-info.sh output, the commit you mentioned is very
> > unlikely the culprit.  Your codec ID is 0x10ec0256, while the patch changed
> > the behavior for 0x10ec0215, 0x10ec0225, 0x10ec0295 and 0x10ec0299.
> > 
> > It seems happening in alc_update_coef_idx() call in alc256_shutup(), and
> > this was introduced by the commit 4a219ef8f37071ce0993959fe52ffa5034689f42
> >     ALSA: hda/realtek - Add ALC256 HP depop function
> 
> This commit is present since Linux 4.13-rc1, so it’s strange that I can’t
> reproduce it with Linux 4.13. (But maybe it doesn’t happen always.)
> 
> > Could you try to swap alc_update_coef_idx() call and snd_hda_codec_write()
> > call?
> > 
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -3154,11 +3154,11 @@ static void alc256_shutup(struct hda_codec *codec)
> >       if (hp_pin_sense)
> >               msleep(85);
> >  
> > +     alc_update_coef_idx(codec, 0x46, 0, 3 << 12); /* 3k pull low control
> > for
> > Headset jack. */
> > +
> >       snd_hda_codec_write(codec, hp_pin, 0,
> >                           AC_VERB_SET_PIN_WIDGET_CONTROL, 0x0);
> >  
> > -     alc_update_coef_idx(codec, 0x46, 0, 3 << 12); /* 3k pull low control
> > for
> > Headset jack. */
> > -
> >       if (hp_pin_sense)
> >               msleep(100);
> 
> Ok, I’ll try that.

With latest master and your patch, the times are down again. Thank you.

I am still confused, why I do not see it with Linux 4.13 though.

So, it just need to shift function alc_update_coef_idx(codec, 0x46, 0, 3 << 12);.

I am also confused with this.

My theory is that the COEF read for the headset doesn't return properly when the pin is off.

So double-check the patch with and without the headphone / headset plugged.

Created attachment 273721 [details]
Output of `alsa-info.sh --no-upload` with headphones plugged in (4.15-rc8+)

(In reply to Takashi Iwai from comment #10)
> My theory is that the COEF read for the headset doesn't return properly when
> the pin is off.
> 
> So double-check the patch with and without the headphone / headset plugged.

Ok, with the headphones jack of the headset plugged in, the times goes up to 200 ms.

> snd_hda_codec_realtek @ hdaudioC0D0 {snd_hda_codec_realtek} async_device
> (Total Suspend: 233.555 ms Total Resume: 256.835 ms)

Please find the output of `./alsa-info.sh --no-upload` attached.

Takashi is right, plugging in the headphone jack with Ubuntu’s Linux 4.13.0-26-generic, suspend time increases.

> snd_hda_codec_realtek @ hdaudioC0D0 {snd_hda_codec_realtek} async_device
> (Total Suspend: 1129.993 ms Total Resume: 256.572 ms)

That level of increase is OK, and it's the designed behavior: alc256_shutup() has a few msleep() calls that are applied only when the headphone is plugged.  It's for avoiding the pop noise, and likely unavoidable.

But, you've never seen over 1000ms like the original report, after the patch?
Then we can take the fix tentatively.  At least I can merge it for 4.16-rc1.

Created attachment 273723 [details]
Fix patch to be merged

(In reply to Takashi Iwai from comment #13)
> That level of increase is OK, and it's the designed behavior:
> alc256_shutup() has a few msleep() calls that are applied only when the
> headphone is plugged.  It's for avoiding the pop noise, and likely
> unavoidable.

Understood. Didn’t know there are pop-noises during suspend too. I’ll hear one during resume, and I’ll submit a separate report for that.

> But, you've never seen over 1000ms like the original report, after the patch?
> Then we can take the fix tentatively.  At least I can merge it for 4.16-rc1.

Yes, in my few tests, I haven’t seen this. Could you please mark it for stable too?

I’ll add another comment, despite sending a message to a mailing list would be better.

Is there a way to parallelize certain things, or split them into different stages. Judging from the HTML files, there are, for example, *suspend*, *suspend late*, and *suspend noirq*.