Bug 198503

Summary: alc_read_coefex_idx() takes 1 second during suspend on Dell XPS 13 9360
Product: Drivers Reporter: Paul Menzel (pmenzel+bugzilla.kernel.org)
Component: Sound(ALSA)Assignee: Jaroslav Kysela (perex)
Status: NEW ---    
Severity: normal CC: kailang, nate, pmenzel+bugzilla.kernel.org, superm1, tiwai
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 4.15-rc8 Subsystem:
Regression: No Bisected commit-id:
Attachments: Compressed HTML output of `sudo ./sleepgraph -config config/suspend-callgraph.cfg`
Linux messages
Output of https://www.alsa-project.org/alsa-info.sh with `--no-upload`
Output of `alsa-info.sh --no-upload` with headphones plugged in (4.15-rc8+)
Fix patch to be merged

Description Paul Menzel 2018-01-18 10:32:44 UTC
Created attachment 273673 [details]
Compressed HTML output of `sudo ./sleepgraph -config config/suspend-callgraph.cfg`

On a Dell XPS 13 9360, Benchmarking Linux 4.15-rc8+ with `sleepgraph` [1][2] there is a regression, that `alc_read_coefex_idx()` takes around one second. I assume it might be related to commit 44be77c5 (ALSA: hda - Fix missing COEF init for ALC225/295/299) fixing bug 195457 [3].

Please find the HTML output attached.

[1] https://01.org/suspendresume
[2] https://01.org/suspendresume/community
[3] https://bugzilla.kernel.org/show_bug.cgi?id=195457
Comment 1 Paul Menzel 2018-01-18 10:33:09 UTC
Created attachment 273675 [details]
Linux messages
Comment 2 Takashi Iwai 2018-01-18 11:42:41 UTC
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.
Comment 3 Paul Menzel 2018-01-18 12:52:03 UTC
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.
Comment 4 Paul Menzel 2018-01-18 12:56:29 UTC
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)
Comment 5 Takashi Iwai 2018-01-18 13:32:23 UTC
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);
Comment 6 Takashi Iwai 2018-01-18 13:32:53 UTC
In anyway, I put Kailang to Cc.  He must know better of this.
Comment 7 Paul Menzel 2018-01-18 13:48:19 UTC
(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.
Comment 8 Paul Menzel 2018-01-18 15:40:52 UTC
(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.
Comment 9 Kailiang Yang 2018-01-19 07:16:18 UTC
So, it just need to shift function alc_update_coef_idx(codec, 0x46, 0, 3 << 12);.

I am also confused with this.
Comment 10 Takashi Iwai 2018-01-19 07:35:16 UTC
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.
Comment 11 Paul Menzel 2018-01-19 12:47:21 UTC
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.
Comment 12 Paul Menzel 2018-01-19 12:55:42 UTC
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)
Comment 13 Takashi Iwai 2018-01-19 13:16:37 UTC
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.
Comment 14 Takashi Iwai 2018-01-19 13:24:31 UTC
Created attachment 273723 [details]
Fix patch to be merged
Comment 15 Paul Menzel 2018-01-19 13:37:57 UTC
(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?
Comment 16 Paul Menzel 2018-01-19 14:00:17 UTC
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*.