| Message ID | 20200319204947.18963-4-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b0ada40cb80d7e427fb719a4e6029935639fa668 |
| Headers | show |
| Series | ASoC: Intel: boards: Remove ignore_suspend flag from SSP0 dai link | expand |
On 3/19/20 3:49 PM, Cezary Rojewski wrote: > As of commit: > ASoC: soc-core: care .ignore_suspend for Component suspend > > function soc-core::snd_soc_suspend no longer ignores 'ignore_suspend' > flag for dai links. While BE dai link for System Pin is > supposed to follow standard suspend-resume flow, appended > 'ignore_suspend' flag disturbs that flow and causes audio to break > right after resume. Remove the flag to address this. > > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Mark Brown <broonie@kernel.org> > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> we should ask Ben and Curtis @ Google if the changes related to suspend interfere with the wake-on-voice support? Btw the .ignore_suspend is also set in bytcr_rt5640/51 drivers, so wondering if additional devices are broken, or if there's something off about Broadwell in general. Hans, have you heard of any regressions on Baytrail devices? > --- > sound/soc/intel/boards/bdw-rt5677.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c > index a94f498388e1..713ef48b36a8 100644 > --- a/sound/soc/intel/boards/bdw-rt5677.c > +++ b/sound/soc/intel/boards/bdw-rt5677.c > @@ -343,7 +343,6 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = { > .no_pcm = 1, > .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | > SND_SOC_DAIFMT_CBS_CFS, > - .ignore_suspend = 1, > .ignore_pmdown_time = 1, > .be_hw_params_fixup = broadwell_ssp0_fixup, > .ops = &bdw_rt5677_ops, >
On Thu, Mar 19, 2020 at 3:15 PM Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > > > > On 3/19/20 3:49 PM, Cezary Rojewski wrote: > > As of commit: > > ASoC: soc-core: care .ignore_suspend for Component suspend > > > > function soc-core::snd_soc_suspend no longer ignores 'ignore_suspend' > > flag for dai links. While BE dai link for System Pin is > > supposed to follow standard suspend-resume flow, appended > > 'ignore_suspend' flag disturbs that flow and causes audio to break > > right after resume. Remove the flag to address this. > > > > Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > Cc: Dominik Brodowski <linux@dominikbrodowski.net> > > Cc: Mark Brown <broonie@kernel.org> > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > > we should ask Ben and Curtis @ Google if the changes related to suspend > interfere with the wake-on-voice support? I have a samus with me so I can test it but my backlog is definitely growing due to WFH slowness. I will see if I can take a look tomorrow.
On 2020-03-19 23:43, Curtis Malainey wrote: > On Thu, Mar 19, 2020 at 3:15 PM Pierre-Louis Bossart > <pierre-louis.bossart@linux.intel.com> wrote: >> On 3/19/20 3:49 PM, Cezary Rojewski wrote: >>> As of commit: >>> ASoC: soc-core: care .ignore_suspend for Component suspend >>> >>> function soc-core::snd_soc_suspend no longer ignores 'ignore_suspend' >>> flag for dai links. While BE dai link for System Pin is >>> supposed to follow standard suspend-resume flow, appended >>> 'ignore_suspend' flag disturbs that flow and causes audio to break >>> right after resume. Remove the flag to address this. >>> >>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>> Cc: Dominik Brodowski <linux@dominikbrodowski.net> >>> Cc: Mark Brown <broonie@kernel.org> >>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> >> we should ask Ben and Curtis @ Google if the changes related to suspend >> interfere with the wake-on-voice support? > > I have a samus with me so I can test it but my backlog is definitely > growing due to WFH slowness. I will see if I can take a look tomorrow. > Any update? Maybe let's leave bdw-rt5650/ bdw-rt5677 behind so people have more time to test and merge just the broadwell & haswell part. Hmm?
On 3/25/20 8:28 AM, Cezary Rojewski wrote: > On 2020-03-19 23:43, Curtis Malainey wrote: >> On Thu, Mar 19, 2020 at 3:15 PM Pierre-Louis Bossart >> <pierre-louis.bossart@linux.intel.com> wrote: >>> On 3/19/20 3:49 PM, Cezary Rojewski wrote: >>>> As of commit: >>>> ASoC: soc-core: care .ignore_suspend for Component suspend >>>> >>>> function soc-core::snd_soc_suspend no longer ignores 'ignore_suspend' >>>> flag for dai links. While BE dai link for System Pin is >>>> supposed to follow standard suspend-resume flow, appended >>>> 'ignore_suspend' flag disturbs that flow and causes audio to break >>>> right after resume. Remove the flag to address this. >>>> >>>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >>>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>>> Cc: Dominik Brodowski <linux@dominikbrodowski.net> >>>> Cc: Mark Brown <broonie@kernel.org> >>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >>> >>> we should ask Ben and Curtis @ Google if the changes related to suspend >>> interfere with the wake-on-voice support? >> >> I have a samus with me so I can test it but my backlog is definitely >> growing due to WFH slowness. I will see if I can take a look tomorrow. >> > > Any update? > > Maybe let's leave bdw-rt5650/ bdw-rt5677 behind so people have more time > to test and merge just the broadwell & haswell part. Hmm? Can you give us a bit more time (couple of days tops)? we also see a problem with SOF when playing after suspend-resume, and I just thought this might be related to the same issue.
Hi, On 3/19/20 11:14 PM, Pierre-Louis Bossart wrote: > > > On 3/19/20 3:49 PM, Cezary Rojewski wrote: >> As of commit: >> ASoC: soc-core: care .ignore_suspend for Component suspend >> >> function soc-core::snd_soc_suspend no longer ignores 'ignore_suspend' >> flag for dai links. While BE dai link for System Pin is >> supposed to follow standard suspend-resume flow, appended >> 'ignore_suspend' flag disturbs that flow and causes audio to break >> right after resume. Remove the flag to address this. >> >> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> Cc: Dominik Brodowski <linux@dominikbrodowski.net> >> Cc: Mark Brown <broonie@kernel.org> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > > we should ask Ben and Curtis @ Google if the changes related to suspend interfere with the wake-on-voice support? > > Btw the .ignore_suspend is also set in bytcr_rt5640/51 drivers, so wondering if additional devices are broken, or if there's something off about Broadwell in general. Hans, have you heard of any regressions on Baytrail devices? I've just tested 5.6.0 on Bay Trail + a rt5651 codec, using the bytcr_rt5651 machine driver which sets ignore_suspend, as well as on a Cherry Trail + rt5645 device using the chtrt5645 machine driver which does _not_ set ignore suspend. Suspend/resume work fine on both and music playing before suspend continues playing after suspend. Note that the bytcr_rt5651 machine driver also does: snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone"); snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker"); Which the bdw-rt5677 seems to not do... Regards, Hans > >> --- >> sound/soc/intel/boards/bdw-rt5677.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c >> index a94f498388e1..713ef48b36a8 100644 >> --- a/sound/soc/intel/boards/bdw-rt5677.c >> +++ b/sound/soc/intel/boards/bdw-rt5677.c >> @@ -343,7 +343,6 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = { >> .no_pcm = 1, >> .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | >> SND_SOC_DAIFMT_CBS_CFS, >> - .ignore_suspend = 1, >> .ignore_pmdown_time = 1, >> .be_hw_params_fixup = broadwell_ssp0_fixup, >> .ops = &bdw_rt5677_ops, >> >
>> Btw the .ignore_suspend is also set in bytcr_rt5640/51 drivers, so >> wondering if additional devices are broken, or if there's something >> off about Broadwell in general. Hans, have you heard of any >> regressions on Baytrail devices? > > I've just tested 5.6.0 on Bay Trail + a rt5651 codec, > using the bytcr_rt5651 machine driver which sets > ignore_suspend, as well as on a Cherry Trail + rt5645 > device using the chtrt5645 machine driver which does > _not_ set ignore suspend. > > Suspend/resume work fine on both and music playing > before suspend continues playing after suspend. Thanks for testing Hans. I think we should remove those .ignore_suspend from all Baytrail/Cherrytrail drivers, no one ever enabled advanced power management except in very specific Android distributions that are no longer maintained. > > Note that the bytcr_rt5651 machine driver also does: > > snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone"); > snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker"); > > Which the bdw-rt5677 seems to not do... On the bytcr_rt5661, these two lines were added in the initial code in 2016, and it's also part of the legacy byt-rt5640, so it's likely a copy/paste more than a feature added on purpose.
On 2020-03-30 23:39, Hans de Goede wrote: > Hi, > >> On 3/19/20 11:14 PM, Pierre-Louis Bossart wrote: >> >> we should ask Ben and Curtis @ Google if the changes related to >> suspend interfere with the wake-on-voice support? >> >> Btw the .ignore_suspend is also set in bytcr_rt5640/51 drivers, so >> wondering if additional devices are broken, or if there's something >> off about Broadwell in general. Hans, have you heard of any >> regressions on Baytrail devices? > > I've just tested 5.6.0 on Bay Trail + a rt5651 codec, > using the bytcr_rt5651 machine driver which sets > ignore_suspend, as well as on a Cherry Trail + rt5645 > device using the chtrt5645 machine driver which does > _not_ set ignore suspend. > > Suspend/resume work fine on both and music playing > before suspend continues playing after suspend. > > Note that the bytcr_rt5651 machine driver also does: > > snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone"); > snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker"); > > Which the bdw-rt5677 seems to not do... > > Regards, > > Hans > Thanks for your report Hans! As all streams (whether that's playback routed through Headphones or Speaker, or capture) were connected to SSP0 dai link, there is a question to be raised: Is Capture path needed to be up during suspend for broadwell solutions? Guess these two lines you mentioned above have the exact same impact on playback as complete .ignore_suspend flag removal from SSP0 dai link. Don't believe WoV for WPT has been added for SST linux so .ignore_suspend=1 achieves nothing. The offload part is probably just limited to bigger buffer size compared to system pin than anything else. So, nothing prevents from removing .ignore_suspend for SST solutions until WoV is verified. Don't know about SOF side. Pierre, what's your take on this? Czarek
> Don't believe WoV for WPT has been added for SST linux so > .ignore_suspend=1 achieves nothing. The offload part is probably just > limited to bigger buffer size compared to system pin than anything else. > So, nothing prevents from removing .ignore_suspend for SST solutions > until WoV is verified. Don't know about SOF side. > Pierre, what's your take on this? I think on Baytrail and Broadwell we will most likely never enable hotwording on the Intel DSP, and S0i1-Playback isn't planned either. However hotwording is enabled on the RT5677 and there may be clocking dependencies so that the RT5677 remains operational - at least the mclk needs to be on, so the bdw-rt5677 case probably needs more work. While I am at it, I think the notion of .ignore_suspend has an assumption baked in. It will work if the only suspend mode is S0i1. If the platform can choose between S0i1 and S3, then we would still want to suspend all paths in S3, which currently isn't really possible.
Hi, On 3/31/20 12:54 PM, Pierre-Louis Bossart wrote: > >> Don't believe WoV for WPT has been added for SST linux so .ignore_suspend=1 achieves nothing. The offload part is probably just limited to bigger buffer size compared to system pin than anything else. So, nothing prevents from removing .ignore_suspend for SST solutions until WoV is verified. Don't know about SOF side. >> Pierre, what's your take on this? > > I think on Baytrail and Broadwell we will most likely never enable hotwording on the Intel DSP, and S0i1-Playback isn't planned either. > > However hotwording is enabled on the RT5677 and there may be clocking dependencies so that the RT5677 remains operational - at least the mclk needs to be on, so the bdw-rt5677 case probably needs more work. > > While I am at it, I think the notion of .ignore_suspend has an assumption baked in. It will work if the only suspend mode is S0i1. If the platform can choose between S0i1 and S3, then we would still want to suspend all paths in S3, which currently isn't really possible. You can check if S0i1 will be used by using: pm_suspend_default_s2idle() If this returns true then S0i1 will be used, otherwise S3. This is defined in kernel/power/suspend.c and always build, so it should be ok to also use this in non x86 specific code-paths (it will simply always return false there I believe). Regards, Hans
Hi, On 3/31/20 1:49 AM, Pierre-Louis Bossart wrote: > >>> Btw the .ignore_suspend is also set in bytcr_rt5640/51 drivers, so wondering if additional devices are broken, or if there's something off about Broadwell in general. Hans, have you heard of any regressions on Baytrail devices? >> >> I've just tested 5.6.0 on Bay Trail + a rt5651 codec, >> using the bytcr_rt5651 machine driver which sets >> ignore_suspend, as well as on a Cherry Trail + rt5645 >> device using the chtrt5645 machine driver which does >> _not_ set ignore suspend. >> >> Suspend/resume work fine on both and music playing >> before suspend continues playing after suspend. > > Thanks for testing Hans. > > I think we should remove those .ignore_suspend from all Baytrail/Cherrytrail drivers, no one ever enabled advanced power management except in very specific Android distributions that are no longer maintained. I agree, I believe I even submitted patches for that a couple of years ago, but back then I think there was still some hope to get S0i1 playback to work so the patches where not accepted. >> Note that the bytcr_rt5651 machine driver also does: >> >> snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone"); >> snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker"); >> >> Which the bdw-rt5677 seems to not do... > > > On the bytcr_rt5661, these two lines were added in the initial code in 2016, and it's also part of the legacy byt-rt5640, so it's likely a copy/paste more than a feature added on purpose. Could be, we should probably also drop those 2 calls together with dropping the setting of the ignore_suspend flag. Regards, Hans
On 3/31/20 7:12 AM, Hans de Goede wrote: > Hi, > > On 3/31/20 12:54 PM, Pierre-Louis Bossart wrote: >> >>> Don't believe WoV for WPT has been added for SST linux so >>> .ignore_suspend=1 achieves nothing. The offload part is probably just >>> limited to bigger buffer size compared to system pin than anything >>> else. So, nothing prevents from removing .ignore_suspend for SST >>> solutions until WoV is verified. Don't know about SOF side. >>> Pierre, what's your take on this? >> >> I think on Baytrail and Broadwell we will most likely never enable >> hotwording on the Intel DSP, and S0i1-Playback isn't planned either. >> >> However hotwording is enabled on the RT5677 and there may be clocking >> dependencies so that the RT5677 remains operational - at least the >> mclk needs to be on, so the bdw-rt5677 case probably needs more work. >> >> While I am at it, I think the notion of .ignore_suspend has an >> assumption baked in. It will work if the only suspend mode is S0i1. If >> the platform can choose between S0i1 and S3, then we would still want >> to suspend all paths in S3, which currently isn't really possible. > > You can check if S0i1 will be used by using: > > pm_suspend_default_s2idle() > > If this returns true then S0i1 will be used, otherwise S3. > > This is defined in kernel/power/suspend.c and always build, so it > should be ok to also use this in non x86 specific code-paths > (it will simply always return false there I believe). Yes, but what I meant is that when the target is S3, we have no way of disabling those .ignore_suspend that were added for S0i1 usages.
Hi, On 3/30/20 11:39 PM, Hans de Goede wrote: > Hi, > > On 3/19/20 11:14 PM, Pierre-Louis Bossart wrote: >> >> >> On 3/19/20 3:49 PM, Cezary Rojewski wrote: >>> As of commit: >>> ASoC: soc-core: care .ignore_suspend for Component suspend >>> >>> function soc-core::snd_soc_suspend no longer ignores 'ignore_suspend' >>> flag for dai links. While BE dai link for System Pin is >>> supposed to follow standard suspend-resume flow, appended >>> 'ignore_suspend' flag disturbs that flow and causes audio to break >>> right after resume. Remove the flag to address this. >>> >>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> >>> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>> Cc: Dominik Brodowski <linux@dominikbrodowski.net> >>> Cc: Mark Brown <broonie@kernel.org> >>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> >> we should ask Ben and Curtis @ Google if the changes related to suspend interfere with the wake-on-voice support? >> >> Btw the .ignore_suspend is also set in bytcr_rt5640/51 drivers, so wondering if additional devices are broken, or if there's something off about Broadwell in general. Hans, have you heard of any regressions on Baytrail devices? > > I've just tested 5.6.0 on Bay Trail + a rt5651 codec, > using the bytcr_rt5651 machine driver which sets > ignore_suspend, as well as on a Cherry Trail + rt5645 > device using the chtrt5645 machine driver which does > _not_ set ignore suspend. > > Suspend/resume work fine on both and music playing > before suspend continues playing after suspend. > > Note that the bytcr_rt5651 machine driver also does: > > snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone"); > snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker"); > > Which the bdw-rt5677 seems to not do... I just noticed something which is somewhat related to this discussion (and also somewhat off topic). I just noticed on a Bay Trail device with a RT5672 codec and on a Cherry Trail device with a RT5645 codec that if an input / recording audio stream is active while suspending the tablet, then after resume audio is broken, playback seems to work (audio samples get consumed at normal speed) but it is silent. Recording also is broken, not sure if it is broken, or just silent too. When this happens, closing all apps which consume audio and waiting 5 seconds for a runtime-suspend to kick in fixes things. After this both record and playback works again. Any idea what the cause for this problem might be? I can reproduce this in 2 ways: 1. Have the sound capplet of GNOME's control-panel open, this shows a VU meter for the default audio input, this VU meter stops working after a suspend resume and playback also stops working if a suspend/resume is done with the VU meter active when suspending. 2. Start a sound recording in gnome-sound-recorder and then suspend + resume. Regards, Hans
> > I just noticed something which is somewhat related to this > discussion (and also somewhat off topic). > > I just noticed on a Bay Trail device with a RT5672 codec > and on a Cherry Trail device with a RT5645 codec that > if an input / recording audio stream is active while > suspending the tablet, then after resume audio is broken, > playback seems to work (audio samples get consumed at normal > speed) but it is silent. Recording also is broken, not > sure if it is broken, or just silent too. > > When this happens, closing all apps which consume audio > and waiting 5 seconds for a runtime-suspend to kick in > fixes things. After this both record and playback > works again. > > Any idea what the cause for this problem might be? Power management for the Atom/sst stuff is far from clear for me, it uses .prepare/.complete callbacks where snd_soc_suspend()/poweroff()/resume() are invoked, so there's a bit of confusion IMO between that the framework does and what the driver should do. It's also unclear to me why the INFO_RESUME flag was set, since the driver cannot restart from the same position. I would try and triangulate with the more traditional bytcr-rt5640, to rule out a codec-specific or machine driver issue (handling of rt5645 and 5672 was done by different people and the machine driver is quite different). I would also remove the INFO_RESUME, that will force the ALSA core to call the .prepare steps and maybe restore settings that are not applied with the current resume. Either way, it's a bit of a shot in the dark :-( My 2 cents -Pierre
diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c index a94f498388e1..713ef48b36a8 100644 --- a/sound/soc/intel/boards/bdw-rt5677.c +++ b/sound/soc/intel/boards/bdw-rt5677.c @@ -343,7 +343,6 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = { .no_pcm = 1, .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, - .ignore_suspend = 1, .ignore_pmdown_time = 1, .be_hw_params_fixup = broadwell_ssp0_fixup, .ops = &bdw_rt5677_ops,
As of commit: ASoC: soc-core: care .ignore_suspend for Component suspend function soc-core::snd_soc_suspend no longer ignores 'ignore_suspend' flag for dai links. While BE dai link for System Pin is supposed to follow standard suspend-resume flow, appended 'ignore_suspend' flag disturbs that flow and causes audio to break right after resume. Remove the flag to address this. Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Cc: Dominik Brodowski <linux@dominikbrodowski.net> Cc: Mark Brown <broonie@kernel.org> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/boards/bdw-rt5677.c | 1 - 1 file changed, 1 deletion(-)