[PATCH] ASoC: dapm: check suspend state in dapm_dai_check_power()

[PATCH] ASoC: dapm: check suspend state in dapm_dai_check_power()

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Currently, when the system enters suspend with audio playing, since the
DAI widgets are active, the associated CODEC is considered active, and
hence is not transition into a low-power bias state as the system
suspends. This likely causes audio failure after resume, since the CODEC
power has quite possibly been removed during suspend, and the bias level
transitions in the CODEC driver are needed to recover from this. At the
very least, this issue likely causes warnings in the kernel log due to
the CODEC being left on over the suspend transition.

To solve this, modify dapm_dai_check_power() not to consider DAI widgets
active if the system is entering suspend.

Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 sound/soc/soc-dapm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 3507346..bad545a 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1133,7 +1133,7 @@ static int dapm_dai_check_power(struct snd_soc_dapm_widget *w)
 	DAPM_UPDATE_STAT(w, power_checks);
 
 	if (w->active)
-		return w->active;
+		return snd_soc_dapm_suspend_check(w);
 
 	return dapm_generic_check_power(w);
 }
-- 
1.7.10.4

Re: [PATCH] ASoC: dapm: check suspend state in dapm_dai_check_power()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1040 bytes --]

On Mon, Jun 03, 2013 at 11:36:40AM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Currently, when the system enters suspend with audio playing, since the
> DAI widgets are active, the associated CODEC is considered active, and
> hence is not transition into a low-power bias state as the system
> suspends. This likely causes audio failure after resume, since the CODEC
> power has quite possibly been removed during suspend, and the bias level
> transitions in the CODEC driver are needed to recover from this. At the
> very least, this issue likely causes warnings in the kernel log due to
> the CODEC being left on over the suspend transition.
> 
> To solve this, modify dapm_dai_check_power() not to consider DAI widgets
> active if the system is entering suspend.

I would expect us to be dealing with this as part of quiescing the DMA
operations on the links - it's not just the CODEC that needs handling
here.  This feels like it ought to be more joined up somewhere along the
line.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: dapm: check suspend state in dapm_dai_check_power()

[Lars-Peter Clausen]

On 06/03/2013 08:51 PM, Mark Brown wrote:
> On Mon, Jun 03, 2013 at 11:36:40AM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Currently, when the system enters suspend with audio playing, since the
>> DAI widgets are active, the associated CODEC is considered active, and
>> hence is not transition into a low-power bias state as the system
>> suspends. This likely causes audio failure after resume, since the CODEC
>> power has quite possibly been removed during suspend, and the bias level
>> transitions in the CODEC driver are needed to recover from this. At the
>> very least, this issue likely causes warnings in the kernel log due to
>> the CODEC being left on over the suspend transition.
>>
>> To solve this, modify dapm_dai_check_power() not to consider DAI widgets
>> active if the system is entering suspend.
> 
> I would expect us to be dealing with this as part of quiescing the DMA
> operations on the links - it's not just the CODEC that needs handling
> here.  This feels like it ought to be more joined up somewhere along the
> line.

I think it is just an oversight that this is not working properly right now.
In snd_soc_suspend() where we also stop the DMA streams etc the DAI link
widgets are put into suspend, by calling snd_soc_dapm_stream_event(...,
SND_SOC_DAPM_STREAM_SUSPEND). But dapm_dai_check_power() doesn't consider
this and hence stays active and also keeps the whole DAPM context (this is
not CODEC specific) active.

- Lars

Re: [PATCH] ASoC: dapm: check suspend state in dapm_dai_check_power()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 557 bytes --]

On Mon, Jun 03, 2013 at 09:16:36PM +0200, Lars-Peter Clausen wrote:

> I think it is just an oversight that this is not working properly right now.
> In snd_soc_suspend() where we also stop the DMA streams etc the DAI link
> widgets are put into suspend, by calling snd_soc_dapm_stream_event(...,
> SND_SOC_DAPM_STREAM_SUSPEND). But dapm_dai_check_power() doesn't consider
> this and hence stays active and also keeps the whole DAPM context (this is
> not CODEC specific) active.

It's clearly an oversight but I don't think this is a joined up fix for
it.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: dapm: check suspend state in dapm_dai_check_power()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1168 bytes --]

On Mon, Jun 03, 2013 at 07:51:20PM +0100, Mark Brown wrote:

> I would expect us to be dealing with this as part of quiescing the DMA
> operations on the links - it's not just the CODEC that needs handling
> here.  This feels like it ought to be more joined up somewhere along the
> line.

So having grovelled a bit this is of course more annoying than it should
be; the trigger operation is in atomic context and there's nowhere
convenient to react to it outside of suspend and resume.  That said the
active flag is only being used for links started and stopped from
userspace so this change *ought* to be safe to do what you're doing but
it still doesn't seem terribly clever.

Another way of looking at things here is that the DAI widgets are
unusual in that they don't require a complete path to power them as
widgets generally do (including DACs and ADCs).  If we fixed that
instead then everything would work just fine.  This should just be a
case of splitting the playback and capture DAIs into separate widget
types and then using the existing DAC and ADC functions.  I can't recall
why we didn't do that in the first place, it's probably just an
oversight...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]