Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Mark Brown]

On Sun, Mar 04, 2018 at 03:11:09PM +0100, Hans de Goede wrote:
> On 01-03-18 18:17, Mark Brown wrote:

> > Sure, but do such userspaces exist - ChromeOS or something for example?

> ChromeOS has a more or less standard userspace I believe.

No, they've got their own sound server for historical reasons.  There's
Android too of course who definitely do have facilities for compressed
audio offload.


signature.asc
Description: PGP signature

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Mark Brown]

On Sun, Mar 04, 2018 at 03:11:09PM +0100, Hans de Goede wrote:
> On 01-03-18 18:17, Mark Brown wrote:

> > Sure, but do such userspaces exist - ChromeOS or something for example?

> ChromeOS has a more or less standard userspace I believe.

No, they've got their own sound server for historical reasons.  There's
Android too of course who definitely do have facilities for compressed
audio offload.


signature.asc
Description: PGP signature

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

Hi,

On 04-03-18 15:11, Hans de Goede wrote:

Hi,

On 01-03-18 18:17, Mark Brown wrote:

On Thu, Mar 01, 2018 at 05:58:35PM +0100, Hans de Goede wrote:

On 01-03-18 17:48, Mark Brown wrote:



That sounds like what's missing is hookup of whatever the DSP uses to
wake the system when it's getting to the bottom of the buffer?  That's
the normal way this stuff is implemented anyway.



I'm afraid there is a lot more missing, at least from a standard Linux
distro pov, just waking up is not enough, we need to also wakeup
userspace to get the mp3-player (or whatever) to refill the buffer,
but preferably without waking up the GPU, turning on the screen, etc.



AFAIK support for this is currently completely missing, standard Linux
userspace currently treats suspend-2-idle as a a full suspend and any
wakeup as a full wakeup.


Sure, but do such userspaces exist - ChromeOS or something for example?


ChromeOS has a more or less standard userspace I believe.


So AFAICT currently the ignore-suspend flag is currently not useful
and as the commit message mentions IIRC there were added to fix some
issues with suspend/resume in the past.


Is it possible the issue was playback during suspend?


There was an issue with suspend being blocked (so the machine not suspending)
if audio was playing because of one the suspend callbacks returning
an error. I believe the ignore_suspend settings where added as a workaround
for that and that is no longer a problem.


To be precise (I just got a bugzilla email because someone added a
comment) I believe the ignore_suspend settings were added in an
attempt to fix / workaround this bug:

https://bugzilla.kernel.org/show_bug.cgi?id=111481

And not to allow audio to keep playing while suspended.

Regards,

Hans

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

Hi,

On 04-03-18 15:11, Hans de Goede wrote:

Hi,

On 01-03-18 18:17, Mark Brown wrote:

On Thu, Mar 01, 2018 at 05:58:35PM +0100, Hans de Goede wrote:

On 01-03-18 17:48, Mark Brown wrote:



That sounds like what's missing is hookup of whatever the DSP uses to
wake the system when it's getting to the bottom of the buffer?  That's
the normal way this stuff is implemented anyway.



I'm afraid there is a lot more missing, at least from a standard Linux
distro pov, just waking up is not enough, we need to also wakeup
userspace to get the mp3-player (or whatever) to refill the buffer,
but preferably without waking up the GPU, turning on the screen, etc.



AFAIK support for this is currently completely missing, standard Linux
userspace currently treats suspend-2-idle as a a full suspend and any
wakeup as a full wakeup.


Sure, but do such userspaces exist - ChromeOS or something for example?


ChromeOS has a more or less standard userspace I believe.


So AFAICT currently the ignore-suspend flag is currently not useful
and as the commit message mentions IIRC there were added to fix some
issues with suspend/resume in the past.


Is it possible the issue was playback during suspend?


There was an issue with suspend being blocked (so the machine not suspending)
if audio was playing because of one the suspend callbacks returning
an error. I believe the ignore_suspend settings where added as a workaround
for that and that is no longer a problem.


To be precise (I just got a bugzilla email because someone added a
comment) I believe the ignore_suspend settings were added in an
attempt to fix / workaround this bug:

https://bugzilla.kernel.org/show_bug.cgi?id=111481

And not to allow audio to keep playing while suspended.

Regards,

Hans

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

Hi,

On 01-03-18 18:17, Mark Brown wrote:

On Thu, Mar 01, 2018 at 05:58:35PM +0100, Hans de Goede wrote:

On 01-03-18 17:48, Mark Brown wrote:



That sounds like what's missing is hookup of whatever the DSP uses to
wake the system when it's getting to the bottom of the buffer?  That's
the normal way this stuff is implemented anyway.



I'm afraid there is a lot more missing, at least from a standard Linux
distro pov, just waking up is not enough, we need to also wakeup
userspace to get the mp3-player (or whatever) to refill the buffer,
but preferably without waking up the GPU, turning on the screen, etc.



AFAIK support for this is currently completely missing, standard Linux
userspace currently treats suspend-2-idle as a a full suspend and any
wakeup as a full wakeup.


Sure, but do such userspaces exist - ChromeOS or something for example?


ChromeOS has a more or less standard userspace I believe.


So AFAICT currently the ignore-suspend flag is currently not useful
and as the commit message mentions IIRC there were added to fix some
issues with suspend/resume in the past.


Is it possible the issue was playback during suspend?


There was an issue with suspend being blocked (so the machine not suspending)
if audio was playing because of one the suspend callbacks returning
an error. I believe the ignore_suspend settings where added as a workaround
for that and that is no longer a problem.

Pierre-Louis, do you have any input on this / on this patch?

Regards,

Hans

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

Hi,

On 01-03-18 18:17, Mark Brown wrote:

On Thu, Mar 01, 2018 at 05:58:35PM +0100, Hans de Goede wrote:

On 01-03-18 17:48, Mark Brown wrote:



That sounds like what's missing is hookup of whatever the DSP uses to
wake the system when it's getting to the bottom of the buffer?  That's
the normal way this stuff is implemented anyway.



I'm afraid there is a lot more missing, at least from a standard Linux
distro pov, just waking up is not enough, we need to also wakeup
userspace to get the mp3-player (or whatever) to refill the buffer,
but preferably without waking up the GPU, turning on the screen, etc.



AFAIK support for this is currently completely missing, standard Linux
userspace currently treats suspend-2-idle as a a full suspend and any
wakeup as a full wakeup.


Sure, but do such userspaces exist - ChromeOS or something for example?


ChromeOS has a more or less standard userspace I believe.


So AFAICT currently the ignore-suspend flag is currently not useful
and as the commit message mentions IIRC there were added to fix some
issues with suspend/resume in the past.


Is it possible the issue was playback during suspend?


There was an issue with suspend being blocked (so the machine not suspending)
if audio was playing because of one the suspend callbacks returning
an error. I believe the ignore_suspend settings where added as a workaround
for that and that is no longer a problem.

Pierre-Louis, do you have any input on this / on this patch?

Regards,

Hans

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Mark Brown]

On Thu, Mar 01, 2018 at 05:58:35PM +0100, Hans de Goede wrote:
> On 01-03-18 17:48, Mark Brown wrote:

> > That sounds like what's missing is hookup of whatever the DSP uses to
> > wake the system when it's getting to the bottom of the buffer?  That's
> > the normal way this stuff is implemented anyway.

> I'm afraid there is a lot more missing, at least from a standard Linux
> distro pov, just waking up is not enough, we need to also wakeup
> userspace to get the mp3-player (or whatever) to refill the buffer,
> but preferably without waking up the GPU, turning on the screen, etc.

> AFAIK support for this is currently completely missing, standard Linux
> userspace currently treats suspend-2-idle as a a full suspend and any
> wakeup as a full wakeup.

Sure, but do such userspaces exist - ChromeOS or something for example?

> So AFAICT currently the ignore-suspend flag is currently not useful
> and as the commit message mentions IIRC there were added to fix some
> issues with suspend/resume in the past.

Is it possible the issue was playback during suspend?


signature.asc
Description: PGP signature

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Mark Brown]

On Thu, Mar 01, 2018 at 05:58:35PM +0100, Hans de Goede wrote:
> On 01-03-18 17:48, Mark Brown wrote:

> > That sounds like what's missing is hookup of whatever the DSP uses to
> > wake the system when it's getting to the bottom of the buffer?  That's
> > the normal way this stuff is implemented anyway.

> I'm afraid there is a lot more missing, at least from a standard Linux
> distro pov, just waking up is not enough, we need to also wakeup
> userspace to get the mp3-player (or whatever) to refill the buffer,
> but preferably without waking up the GPU, turning on the screen, etc.

> AFAIK support for this is currently completely missing, standard Linux
> userspace currently treats suspend-2-idle as a a full suspend and any
> wakeup as a full wakeup.

Sure, but do such userspaces exist - ChromeOS or something for example?

> So AFAICT currently the ignore-suspend flag is currently not useful
> and as the commit message mentions IIRC there were added to fix some
> issues with suspend/resume in the past.

Is it possible the issue was playback during suspend?


signature.asc
Description: PGP signature

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

Hi,

On 01-03-18 17:48, Mark Brown wrote:

On Thu, Mar 01, 2018 at 05:26:06PM +0100, Hans de Goede wrote:

On 01-03-18 17:22, Mark Brown wrote:

On Sun, Feb 18, 2018 at 11:01:45PM +0100, Hans de Goede wrote:



The ignore_suspend settings were added in an attempt to try and fix
suspend / resume issues. But they never fully fixed these and now we've
a proper fix, so lets remove these.



Do these systems not have the ability to continue to play audio through
suspend using the DSP?  That's the sort of thing that ignore_suspend is
normally used for.



In theory yes, in practice if we keep ignore_suspend set then we get
audio for a couple of more seconds after suspend until the buffers
are empty, as we have no code keeping them filled, which is not really
useful, since we don't support this in a meaningful way it is better
to just do a normal full suspend.


That sounds like what's missing is hookup of whatever the DSP uses to
wake the system when it's getting to the bottom of the buffer?  That's
the normal way this stuff is implemented anyway.


I'm afraid there is a lot more missing, at least from a standard Linux
distro pov, just waking up is not enough, we need to also wakeup
userspace to get the mp3-player (or whatever) to refill the buffer,
but preferably without waking up the GPU, turning on the screen, etc.

AFAIK support for this is currently completely missing, standard Linux
userspace currently treats suspend-2-idle as a a full suspend and any
wakeup as a full wakeup.

So AFAICT currently the ignore-suspend flag is currently not useful
and as the commit message mentions IIRC there were added to fix some
issues with suspend/resume in the past.

Regards,

Hans

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

Hi,

On 01-03-18 17:48, Mark Brown wrote:

On Thu, Mar 01, 2018 at 05:26:06PM +0100, Hans de Goede wrote:

On 01-03-18 17:22, Mark Brown wrote:

On Sun, Feb 18, 2018 at 11:01:45PM +0100, Hans de Goede wrote:



The ignore_suspend settings were added in an attempt to try and fix
suspend / resume issues. But they never fully fixed these and now we've
a proper fix, so lets remove these.



Do these systems not have the ability to continue to play audio through
suspend using the DSP?  That's the sort of thing that ignore_suspend is
normally used for.



In theory yes, in practice if we keep ignore_suspend set then we get
audio for a couple of more seconds after suspend until the buffers
are empty, as we have no code keeping them filled, which is not really
useful, since we don't support this in a meaningful way it is better
to just do a normal full suspend.


That sounds like what's missing is hookup of whatever the DSP uses to
wake the system when it's getting to the bottom of the buffer?  That's
the normal way this stuff is implemented anyway.


I'm afraid there is a lot more missing, at least from a standard Linux
distro pov, just waking up is not enough, we need to also wakeup
userspace to get the mp3-player (or whatever) to refill the buffer,
but preferably without waking up the GPU, turning on the screen, etc.

AFAIK support for this is currently completely missing, standard Linux
userspace currently treats suspend-2-idle as a a full suspend and any
wakeup as a full wakeup.

So AFAICT currently the ignore-suspend flag is currently not useful
and as the commit message mentions IIRC there were added to fix some
issues with suspend/resume in the past.

Regards,

Hans

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Mark Brown]

On Thu, Mar 01, 2018 at 05:26:06PM +0100, Hans de Goede wrote:
> On 01-03-18 17:22, Mark Brown wrote:
> > On Sun, Feb 18, 2018 at 11:01:45PM +0100, Hans de Goede wrote:

> > > The ignore_suspend settings were added in an attempt to try and fix
> > > suspend / resume issues. But they never fully fixed these and now we've
> > > a proper fix, so lets remove these.

> > Do these systems not have the ability to continue to play audio through
> > suspend using the DSP?  That's the sort of thing that ignore_suspend is
> > normally used for.

> In theory yes, in practice if we keep ignore_suspend set then we get
> audio for a couple of more seconds after suspend until the buffers
> are empty, as we have no code keeping them filled, which is not really
> useful, since we don't support this in a meaningful way it is better
> to just do a normal full suspend.

That sounds like what's missing is hookup of whatever the DSP uses to
wake the system when it's getting to the bottom of the buffer?  That's
the normal way this stuff is implemented anyway.


signature.asc
Description: PGP signature

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Mark Brown]

On Thu, Mar 01, 2018 at 05:26:06PM +0100, Hans de Goede wrote:
> On 01-03-18 17:22, Mark Brown wrote:
> > On Sun, Feb 18, 2018 at 11:01:45PM +0100, Hans de Goede wrote:

> > > The ignore_suspend settings were added in an attempt to try and fix
> > > suspend / resume issues. But they never fully fixed these and now we've
> > > a proper fix, so lets remove these.

> > Do these systems not have the ability to continue to play audio through
> > suspend using the DSP?  That's the sort of thing that ignore_suspend is
> > normally used for.

> In theory yes, in practice if we keep ignore_suspend set then we get
> audio for a couple of more seconds after suspend until the buffers
> are empty, as we have no code keeping them filled, which is not really
> useful, since we don't support this in a meaningful way it is better
> to just do a normal full suspend.

That sounds like what's missing is hookup of whatever the DSP uses to
wake the system when it's getting to the bottom of the buffer?  That's
the normal way this stuff is implemented anyway.


signature.asc
Description: PGP signature

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

Hi,

On 01-03-18 17:22, Mark Brown wrote:

On Sun, Feb 18, 2018 at 11:01:45PM +0100, Hans de Goede wrote:

The ignore_suspend settings were added in an attempt to try and fix
suspend / resume issues. But they never fully fixed these and now we've
a proper fix, so lets remove these.


Do these systems not have the ability to continue to play audio through
suspend using the DSP?  That's the sort of thing that ignore_suspend is
normally used for.


In theory yes, in practice if we keep ignore_suspend set then we get
audio for a couple of more seconds after suspend until the buffers
are empty, as we have no code keeping them filled, which is not really
useful, since we don't support this in a meaningful way it is better
to just do a normal full suspend.

Regards,

Hans

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

Hi,

On 01-03-18 17:22, Mark Brown wrote:

On Sun, Feb 18, 2018 at 11:01:45PM +0100, Hans de Goede wrote:

The ignore_suspend settings were added in an attempt to try and fix
suspend / resume issues. But they never fully fixed these and now we've
a proper fix, so lets remove these.


Do these systems not have the ability to continue to play audio through
suspend using the DSP?  That's the sort of thing that ignore_suspend is
normally used for.


In theory yes, in practice if we keep ignore_suspend set then we get
audio for a couple of more seconds after suspend until the buffers
are empty, as we have no code keeping them filled, which is not really
useful, since we don't support this in a meaningful way it is better
to just do a normal full suspend.

Regards,

Hans

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Mark Brown]

On Sun, Feb 18, 2018 at 11:01:45PM +0100, Hans de Goede wrote:
> The ignore_suspend settings were added in an attempt to try and fix
> suspend / resume issues. But they never fully fixed these and now we've
> a proper fix, so lets remove these.

Do these systems not have the ability to continue to play audio through
suspend using the DSP?  That's the sort of thing that ignore_suspend is
normally used for.


signature.asc
Description: PGP signature

Re: [PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Mark Brown]

On Sun, Feb 18, 2018 at 11:01:45PM +0100, Hans de Goede wrote:
> The ignore_suspend settings were added in an attempt to try and fix
> suspend / resume issues. But they never fully fixed these and now we've
> a proper fix, so lets remove these.

Do these systems not have the ability to continue to play audio through
suspend using the DSP?  That's the sort of thing that ignore_suspend is
normally used for.


signature.asc
Description: PGP signature

[PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

The ignore_suspend settings were added in an attempt to try and fix
suspend / resume issues. But they never fully fixed these and now we've
a proper fix, so lets remove these.

Signed-off-by: Hans de Goede 
---
 sound/soc/intel/boards/bytcr_rt5640.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c 
b/sound/soc/intel/boards/bytcr_rt5640.c
index b6a1cfeec830..c506a6e129ca 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -532,9 +532,6 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime 
*runtime)
return ret;
}
 
-   snd_soc_dapm_ignore_suspend(>dapm, "Headphone");
-   snd_soc_dapm_ignore_suspend(>dapm, "Speaker");
-
if (byt_rt5640_quirk & BYT_RT5640_MCLK_EN) {
/*
 * The firmware might enable the clock at
@@ -691,7 +688,6 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = {
.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
| SND_SOC_DAIFMT_CBS_CFS,
.be_hw_params_fixup = byt_rt5640_codec_fixup,
-   .ignore_suspend = 1,
.nonatomic = true,
.dpcm_playback = 1,
.dpcm_capture = 1,
-- 
2.14.3

[PATCH 8/9] ASoC: Intel: bytcr_rt5640: Drop unwanted ignore_suspend settings

[Hans de Goede]

The ignore_suspend settings were added in an attempt to try and fix
suspend / resume issues. But they never fully fixed these and now we've
a proper fix, so lets remove these.

Signed-off-by: Hans de Goede 
---
 sound/soc/intel/boards/bytcr_rt5640.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c 
b/sound/soc/intel/boards/bytcr_rt5640.c
index b6a1cfeec830..c506a6e129ca 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -532,9 +532,6 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime 
*runtime)
return ret;
}
 
-   snd_soc_dapm_ignore_suspend(>dapm, "Headphone");
-   snd_soc_dapm_ignore_suspend(>dapm, "Speaker");
-
if (byt_rt5640_quirk & BYT_RT5640_MCLK_EN) {
/*
 * The firmware might enable the clock at
@@ -691,7 +688,6 @@ static struct snd_soc_dai_link byt_rt5640_dais[] = {
.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF
| SND_SOC_DAIFMT_CBS_CFS,
.be_hw_params_fixup = byt_rt5640_codec_fixup,
-   .ignore_suspend = 1,
.nonatomic = true,
.dpcm_playback = 1,
.dpcm_capture = 1,
-- 
2.14.3