[PATCH v2 0/2] ASoC: core: Ignoring pmdown_time for playback stream power down

[PATCH v2 0/2] ASoC: core: Ignoring pmdown_time for playback stream power down

[Peter Ujfalusi]

Hello Mark,

Changes since v1 (RFC):
- move the flag to per codec (snd_soc_codec)
- twl6040 to request to ignore the pmdown_time

Add support in core to ignore the pmdown_time when stopping the playback stream.
This will inline the DAPM stream stop event, so it is going to be executed
without delay at stream close time (for playback stream).

Backround: with the McPDM protocol we are sendning not only
the pure audio stream, but OMAP McPDM also transmits
additional information (for example offset cancellation).
If McPDM is stopped prior to the DAC this information will
be not sent to the codec, which can result noise rendered
by the twl6040 codec.

Regards,
Peter
---
Peter Ujfalusi (2):
  ASoC: core: Add flag to ignore pmdown_time at pcm_close
  ASoC: twl6040: Request core to inline the DAPM sequence

 include/sound/soc.h        |    1 +
 sound/soc/codecs/twl6040.c |    1 +
 sound/soc/soc-pcm.c        |   15 +++++++++++----
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
1.7.7

[PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Peter Ujfalusi]

With this flag codec drivers can indicate that it is desired
to ignore the pmdown_time for DAPM shutdown sequence when
playback stream is stopped.
The DAPM sequence will be executed without delay in this case.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/sound/soc.h |    1 +
 sound/soc/soc-pcm.c |   15 +++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 858291d..11cfb59 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -578,6 +578,7 @@ struct snd_soc_codec {
 
 	/* dapm */
 	struct snd_soc_dapm_context dapm;
+	unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs_codec_root;
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 8eb0f07..ee15337 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -319,10 +319,17 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	cpu_dai->runtime = NULL;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		/* start delayed pop wq here for playback streams */
-		codec_dai->pop_wait = 1;
-		schedule_delayed_work(&rtd->delayed_work,
-			msecs_to_jiffies(rtd->pmdown_time));
+		if (unlikely(codec->ignore_pmdown_time)) {
+			/* powered down playback stream now */
+			snd_soc_dapm_stream_event(rtd,
+				codec_dai->driver->playback.stream_name,
+				SND_SOC_DAPM_STREAM_STOP);
+		} else {
+			/* start delayed pop wq here for playback streams */
+			codec_dai->pop_wait = 1;
+			schedule_delayed_work(&rtd->delayed_work,
+				msecs_to_jiffies(rtd->pmdown_time));
+		}
 	} else {
 		/* capture streams can be powered down now */
 		snd_soc_dapm_stream_event(rtd,
-- 
1.7.7

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Babu, Ramesh]

> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -578,6 +578,7 @@ struct snd_soc_codec {
> 
>  	/* dapm */
>  	struct snd_soc_dapm_context dapm;
> +	unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at
> stop */

I feel ignore_pmdown_time needs to be part of snd_soc_dai_driver structure.
It will give flexibility to control individual dai.  Individual dai's can choose to
Ignore power down delay.

Would it make sense?

In my codec, I need to ignore the power down delay for voice dais, but retain the delay
for HiFi dai's.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Mark Brown]

On Fri, Oct 21, 2011 at 11:42:27AM +0530, Babu, Ramesh wrote:

> I feel ignore_pmdown_time needs to be part of snd_soc_dai_driver structure.
> It will give flexibility to control individual dai.  Individual dai's can choose to
> Ignore power down delay.

Why do you need to do this?

> Would it make sense?

> In my codec, I need to ignore the power down delay for voice dais, but retain the delay
> for HiFi dai's.

I'm not sure associating it with DAIs is a good idea, I'd expect any
issues to be with the analouge outputs not DAIs.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Babu, Ramesh]

> > I feel ignore_pmdown_time needs to be part of snd_soc_dai_driver
> structure.
> > It will give flexibility to control individual dai.  Individual dai's
> can choose to
> > Ignore power down delay.
> 
> Why do you need to do this?
> 
> > Would it make sense?
> 
> > In my codec, I need to ignore the power down delay for voice dais,
> but retain the delay
> > for HiFi dai's.
> 
> I'm not sure associating it with DAIs is a good idea, I'd expect any
> issues to be with the analouge outputs not DAIs.

In my device, clock to the codec is coming from modem (for voice call use case).  After voice call termination, application
Calls snd_pcm_close for voice device and then turns off the modem clock.  Since clock is turned-off while
Widgets are still on (power down deferred by pmdown_time), I am noticing the glitches.

I thought, having ignore_pmdown_time for individual codec device would allow flexibility.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Mark Brown]

On Fri, Oct 21, 2011 at 02:40:04PM +0530, Babu, Ramesh wrote:

> In my device, clock to the codec is coming from modem (for voice call use case).  After voice call termination, application
> Calls snd_pcm_close for voice device and then turns off the modem clock.  Since clock is turned-off while
> Widgets are still on (power down deferred by pmdown_time), I am noticing the glitches.

This clearly isn't something that should be set in the CODEC driver,
it's a system integration issue - on a different board there might be no
such requirement.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Babu, Ramesh]

> This clearly isn't something that should be set in the CODEC driver,
> it's a system integration issue - on a different board there might be
> no
> such requirement.

Hmm... Ok. Thanks for clarification.

If it is a board specific requirement, then would it make sense to have it in
dai_link as per Peter Ujfaulsi's RFC [1].

In general, how this kind of situation is handled?

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2011-October/045088.html

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Vinod Koul]

On Fri, 2011-10-21 at 16:28 +0530, Babu, Ramesh wrote:
> > This clearly isn't something that should be set in the CODEC driver,
> > it's a system integration issue - on a different board there might be
> > no
> > such requirement.
> 
> Hmm... Ok. Thanks for clarification.
> 
> If it is a board specific requirement, then would it make sense to have it in
> dai_link as per Peter Ujfaulsi's RFC [1].
> 
> In general, how this kind of situation is handled?
> 
> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2011-October/045088.html
Then, I think it makes sense to make it for snd_soc_dai_link

-- 
~Vinod

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Nallasellan, Singaravelan]

I think the requirement is to delay the power down the link during the  music playback. There is a possibility that next song will start playing sooner. In this case, try to avoid power down and power up the DAI immediately. In case of voice call, it is not expected to have voice call immediately after a call. It can be closed. Phone stack can go ahead and close the modem path but codec path will still be open. This will cause glitches.

How will this scenario be glitch free?


-----Original Message-----
From: alsa-devel-bounces@alsa-project.org [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Babu, Ramesh
Sent: Friday, October 21, 2011 3:59 AM
To: Mark Brown
Cc: Peter Ujfalusi; alsa-devel@alsa-project.org; Liam Girdwood; Misael Lopez Cruz
Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

> This clearly isn't something that should be set in the CODEC driver, 
> it's a system integration issue - on a different board there might be 
> no such requirement.

Hmm... Ok. Thanks for clarification.

If it is a board specific requirement, then would it make sense to have it in dai_link as per Peter Ujfaulsi's RFC [1].

In general, how this kind of situation is handled?

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2011-October/045088.html

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Mark Brown]

On Fri, Oct 21, 2011 at 10:31:40PM +0530, Nallasellan, Singaravelan wrote:

Fix your mailer to word wrap within paragraphs and don't top post when
posting to public mailing lists.

> I think the requirement is to delay the power down the link during the
> music playback. There is a possibility that next song will start
> playing sooner. In this case, try to avoid power down and power up the
> DAI immediately. In case of voice call, it is not expected to have
> voice call immediately after a call. It can be closed. Phone stack can
> go ahead and close the modem path but codec path will still be open.
> This will cause glitches.

You can't decide this on the basis of the DAI, though - the system
integrator may have thought of another use for the DAI.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Nallasellan, Singaravelan]

Agree. System integrator may not like to delay closing modem ports for 6 seconds either. Need to close the ports immediately or delay. 
Do you suggest that we shouldn't delay in the driver and introduce delay in the user pace?
I prefer to remove the delay altogether from the driver. Do you see any issue which might cause glitches?

-----Original Message-----
From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
Sent: Friday, October 21, 2011 12:42 PM
To: Nallasellan, Singaravelan
Cc: Babu, Ramesh; Peter Ujfalusi; alsa-devel@alsa-project.org; Liam Girdwood; Misael Lopez Cruz
Subject: Re: [alsa-devel] [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

On Fri, Oct 21, 2011 at 10:31:40PM +0530, Nallasellan, Singaravelan wrote:

Fix your mailer to word wrap within paragraphs and don't top post when posting to public mailing lists.

> I think the requirement is to delay the power down the link during the 
> music playback. There is a possibility that next song will start 
> playing sooner. In this case, try to avoid power down and power up the 
> DAI immediately. In case of voice call, it is not expected to have 
> voice call immediately after a call. It can be closed. Phone stack can 
> go ahead and close the modem path but codec path will still be open.
> This will cause glitches.

You can't decide this on the basis of the DAI, though - the system integrator may have thought of another use for the DAI.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Mark Brown]

On Sat, Oct 22, 2011 at 01:54:23AM +0530, Nallasellan, Singaravelan wrote:

When I said you shouldn't top post that's exactly what I meant!  This is
basic mailing list etiquette for public Linux lists because for
example...

> Agree. System integrator may not like to delay closing modem ports for 6 seconds either. Need to close the ports immediately or delay. 

...not having any context quoted means the reader can't easily tell wht
it is that you are agreeing with.  You should also fix your mail client
to word wrap within paragraphs as I also mentioned.

> Do you suggest that we shouldn't delay in the driver and introduce delay in the user pace?
> I prefer to remove the delay altogether from the driver. Do you see any issue which might cause glitches?

The delay is very important for CPU use cases as brief gaps in playback
are extremely common there.  The power up and power down are generally
noticable and to be avoided.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Mark Brown]

On Fri, Oct 21, 2011 at 04:28:44PM +0530, Babu, Ramesh wrote:

> If it is a board specific requirement, then would it make sense to have it in
> dai_link as per Peter Ujfaulsi's RFC [1].

Possibly for your case - the case this was originally written for was
rather different, the CODEC has issues which mean it always needs to
skip the delay so having to specify this for every single board is going
to be repetitive.

> In general, how this kind of situation is handled?

Usually by keeping the clocks available.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Vinod Koul]

On Sat, 2011-10-22 at 10:45 +0100, Mark Brown wrote:
> On Fri, Oct 21, 2011 at 04:28:44PM +0530, Babu, Ramesh wrote:
> 
> > If it is a board specific requirement, then would it make sense to have it in
> > dai_link as per Peter Ujfaulsi's RFC [1].
> 
> Possibly for your case - the case this was originally written for was
> rather different, the CODEC has issues which mean it always needs to
> skip the delay so having to specify this for every single board is going
> to be repetitive.
> 
> > In general, how this kind of situation is handled?
> 
> Usually by keeping the clocks available.
That does not seem to be possible here :(

The codec takes the clock from modem. The codec doesn't have control
over it. The voice call termination kills the clock and the DAPM power
down produces noticeable artifacts.

Since this is board property, machine driver should tell DAPM to power
down immediately for this DAI alone. That of course allows us to reap
benefits of DAPM not power cycling codec during close/open.

-- 
~Vinod

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Mark Brown]

On Sat, Oct 22, 2011 at 10:47:20PM +0530, Vinod Koul wrote:
> On Sat, 2011-10-22 at 10:45 +0100, Mark Brown wrote:

> > Usually by keeping the clocks available.

> That does not seem to be possible here :(

Well, I'm sure you can remonstrate with your baseband vendor.  :)

> Since this is board property, machine driver should tell DAPM to power
> down immediately for this DAI alone. That of course allows us to reap
> benefits of DAPM not power cycling codec during close/open.

For this issue adding control in the dai_link is probably good.

Re: [PATCH v2 1/2] ASoC: core: Add flag to ignore pmdown_time at pcm_close

[Vinod Koul]

On Sun, 2011-10-23 at 16:00 +0200, Mark Brown wrote:
> 
> > Since this is board property, machine driver should tell DAPM to
> power
> > down immediately for this DAI alone. That of course allows us to
> reap
> > benefits of DAPM not power cycling codec during close/open.
> 
> For this issue adding control in the dai_link is probably good. 
Yes meant dia_link :)


-- 
~Vinod

[PATCH v2 2/2] ASoC: twl6040: Request core to inline the DAPM sequence

[Peter Ujfalusi]

We need to have as less time between McPDM shutdown,
and power down of the DAC on the twl6040 codec as possible.
Request core to ignore the pmdown_time for the playback
stream.
Backround: with the McPDM protocol we are sendning not only
the pure audio stream, but OMAP McPDM also transmits
additional information (for example offset cancellation).
If McPDM is stopped prior to the DAC this information will
be not sent to the codec, which can result noise rendered
by the twl6040 codec.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/codecs/twl6040.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/twl6040.c b/sound/soc/codecs/twl6040.c
index 6c573c3..73e11f0 100644
--- a/sound/soc/codecs/twl6040.c
+++ b/sound/soc/codecs/twl6040.c
@@ -1504,6 +1504,7 @@ static int twl6040_probe(struct snd_soc_codec *codec)
 
 	priv->codec = codec;
 	codec->control_data = dev_get_drvdata(codec->dev->parent);
+	codec->ignore_pmdown_time = 1;
 
 	if (pdata && pdata->hs_left_step && pdata->hs_right_step) {
 		priv->hs_left_step = pdata->hs_left_step;
-- 
1.7.7

Re: [PATCH v2 0/2] ASoC: core: Ignoring pmdown_time for playback stream power down

[Mark Brown]

On Fri, Oct 14, 2011 at 02:43:32PM +0300, Peter Ujfalusi wrote:

> Changes since v1 (RFC):
> - move the flag to per codec (snd_soc_codec)
> - twl6040 to request to ignore the pmdown_time

I've applied both, thanks.  We will probably want to extend this to be
handlable dynamically - there's a lot of devices will be happy to do
this for speaker but which won't be able to do it unconditionally due to
headphone or line outputs for example.