diff mbox series

[6/6] ASoC: soc-core: add snd_soc_stream_stop()

Message ID 87o8w7hhds.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show
Series ASoC: soc-core cleanup step8 | expand

Commit Message

Kuninori Morimoto Dec. 17, 2019, 1:26 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

When we stop stream, if it was Playback, we might need to care
about power down time. In such case, we need to use delayed work.

We have same implementation for it at soc-pcm.c and soc-compress.c,
but we don't want to have duplicate code.
This patch adds snd_soc_stream_stop(), and share same code.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/soc.h      |  1 +
 sound/soc/soc-compress.c | 18 +-----------------
 sound/soc/soc-core.c     | 23 +++++++++++++++++++++++
 sound/soc/soc-pcm.c      | 19 +------------------
 4 files changed, 26 insertions(+), 35 deletions(-)

Comments

Pierre-Louis Bossart Dec. 17, 2019, 3:22 a.m. UTC | #1 
Hi Morimoto-san,


> +void snd_soc_stream_stop(struct snd_soc_pcm_runtime *rtd, int stream);

> +void snd_soc_stream_stop(struct snd_soc_pcm_runtime *rtd, int stream)
> +{
> +	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		if (snd_soc_runtime_ignore_pmdown_time(rtd)) {
> +			/* powered down playback stream now */
> +			snd_soc_dapm_stream_event(rtd,
> +						  SNDRV_PCM_STREAM_PLAYBACK,
> +						  SND_SOC_DAPM_STREAM_STOP);
> +		} else {
> +			/* start delayed pop wq here for playback streams */
> +			rtd->pop_wait = 1;
> +			queue_delayed_work(system_power_efficient_wq,
> +					   &rtd->delayed_work,
> +					   msecs_to_jiffies(rtd->pmdown_time));
> +		}
> +	} else {
> +		/* capture streams can be powered down now */
> +		snd_soc_dapm_stream_event(rtd, SNDRV_PCM_STREAM_CAPTURE,
> +					  SND_SOC_DAPM_STREAM_STOP);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(snd_soc_stream_stop);

I am not a big fan of naming conventions for the new helper. We don't 
have any other snd_soc_stream function, but we have a concept of stream 
for SoundWire. we also have snd_soc_dapm_stream_ functions..

snd_soc_dapm_stream_stop() maybe?

thanks
-Pierre
Kuninori Morimoto Dec. 17, 2019, 3:54 a.m. UTC | #2 
Hi Pierre-Louis

Thank you for your review

> > +void snd_soc_stream_stop(struct snd_soc_pcm_runtime *rtd, int stream)
> > +{
> > +	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +		if (snd_soc_runtime_ignore_pmdown_time(rtd)) {
> > +			/* powered down playback stream now */
> > +			snd_soc_dapm_stream_event(rtd,
> > +						  SNDRV_PCM_STREAM_PLAYBACK,
> > +						  SND_SOC_DAPM_STREAM_STOP);
> > +		} else {
> > +			/* start delayed pop wq here for playback streams */
> > +			rtd->pop_wait = 1;
> > +			queue_delayed_work(system_power_efficient_wq,
> > +					   &rtd->delayed_work,
> > +					   msecs_to_jiffies(rtd->pmdown_time));
> > +		}
> > +	} else {
> > +		/* capture streams can be powered down now */
> > +		snd_soc_dapm_stream_event(rtd, SNDRV_PCM_STREAM_CAPTURE,
> > +					  SND_SOC_DAPM_STREAM_STOP);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(snd_soc_stream_stop);
> 
> I am not a big fan of naming conventions for the new helper. We don't
> have any other snd_soc_stream function, but we have a concept of
> stream for SoundWire. we also have snd_soc_dapm_stream_ functions..
> 
> snd_soc_dapm_stream_stop() maybe?

Ahh, make sense !
I will post v2 tomorrow

Thank you for your help !!
Best regards
---
Kuninori Morimoto
Kuninori Morimoto Dec. 17, 2019, 4:41 a.m. UTC | #3 
Hi

> > > +void snd_soc_stream_stop(struct snd_soc_pcm_runtime *rtd, int stream)
> > > +{
> > > +	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > > +		if (snd_soc_runtime_ignore_pmdown_time(rtd)) {
> > > +			/* powered down playback stream now */
> > > +			snd_soc_dapm_stream_event(rtd,
> > > +						  SNDRV_PCM_STREAM_PLAYBACK,
> > > +						  SND_SOC_DAPM_STREAM_STOP);
> > > +		} else {
> > > +			/* start delayed pop wq here for playback streams */
> > > +			rtd->pop_wait = 1;
> > > +			queue_delayed_work(system_power_efficient_wq,
> > > +					   &rtd->delayed_work,
> > > +					   msecs_to_jiffies(rtd->pmdown_time));
> > > +		}
> > > +	} else {
> > > +		/* capture streams can be powered down now */
> > > +		snd_soc_dapm_stream_event(rtd, SNDRV_PCM_STREAM_CAPTURE,
> > > +					  SND_SOC_DAPM_STREAM_STOP);
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL_GPL(snd_soc_stream_stop);

Hmm...
I noticed that soc_compr_free_fe(), dpcm_fe_dai_shutdown() are directly
calling SND_SOC_DAPM_STREAM_STOP without caring pmdown time / delayed work.
Can we use snd_soc_dapm_stream_stop() for these, too ?

	static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
	{
		...
		/* run the stream event for each BE */
=>		dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
		...
	}

	static int soc_compr_free_fe(struct snd_compr_stream *cstream)
	{
		...
=>		dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
		...
	}

Thank you for your help !!
Best regards
---
Kuninori Morimoto
Mark Brown Dec. 17, 2019, 12:13 p.m. UTC | #4 
On Tue, Dec 17, 2019 at 01:41:09PM +0900, Kuninori Morimoto wrote:

> I noticed that soc_compr_free_fe(), dpcm_fe_dai_shutdown() are directly
> calling SND_SOC_DAPM_STREAM_STOP without caring pmdown time / delayed work.
> Can we use snd_soc_dapm_stream_stop() for these, too ?

That does seem like an oversight...
Pierre-Louis Bossart Dec. 17, 2019, 1:56 p.m. UTC | #5 
On 12/17/19 6:13 AM, Mark Brown wrote:
> On Tue, Dec 17, 2019 at 01:41:09PM +0900, Kuninori Morimoto wrote:
> 
>> I noticed that soc_compr_free_fe(), dpcm_fe_dai_shutdown() are directly
>> calling SND_SOC_DAPM_STREAM_STOP without caring pmdown time / delayed work.
>> Can we use snd_soc_dapm_stream_stop() for these, too ?
> 
> That does seem like an oversight...

What would the rationale for deferring a FE shutdown though?

IIRC in the Intel machine drivers we only use .pm_ignore_downtime to 
backends to keep some clocks on, and even that is questionable (the 
clock dependencies should be properly modeled instead of hidden with an 
obscure dailink setting). When I asked why exactly this flag was set in 
most cases the reason why 'because others did so in the past'...
Kuninori Morimoto Dec. 18, 2019, 1:51 a.m. UTC | #6 
Hi Pierre-Louis

> >> I noticed that soc_compr_free_fe(), dpcm_fe_dai_shutdown() are directly
> >> calling SND_SOC_DAPM_STREAM_STOP without caring pmdown time / delayed work.
> >> Can we use snd_soc_dapm_stream_stop() for these, too ?
> > 
> > That does seem like an oversight...
> 
> What would the rationale for deferring a FE shutdown though?
> 
> IIRC in the Intel machine drivers we only use .pm_ignore_downtime to
> backends to keep some clocks on, and even that is questionable (the
> clock dependencies should be properly modeled instead of hidden with
> an obscure dailink setting). When I asked why exactly this flag was
> set in most cases the reason why 'because others did so in the
> past'...

Not only "ignore_pmdown_time", but many flags are used
for such reasons, I guess...
But, it is "driver side setting issue", and same things happen
even if we have properly modeled clock dependencies, I guess unfortunately...

From framework side point of view, it should care settings anyway
if it was added/selected even though if it was added by copy-and-paste.
"Following settings case-by-case" is a little bit strange for me.
Maybe FE which has ignore_pmdown_time has some rationale, somehow ?

So, I will post v2 which renames function name that is pointed from you.
And, post additional patch which cares above +2 STREAM_STOP.
Feel free to post NACK it if you don't like it.
Then, Mark can just ignore it.

Thank you for your help !!
Best regards
---
Kuninori Morimoto
Mark Brown Dec. 18, 2019, 11:50 a.m. UTC | #7 
On Tue, Dec 17, 2019 at 07:56:28AM -0600, Pierre-Louis Bossart wrote:
> On 12/17/19 6:13 AM, Mark Brown wrote:
> > On Tue, Dec 17, 2019 at 01:41:09PM +0900, Kuninori Morimoto wrote:

> > > Can we use snd_soc_dapm_stream_stop() for these, too ?

> > That does seem like an oversight...

> What would the rationale for deferring a FE shutdown though?

> IIRC in the Intel machine drivers we only use .pm_ignore_downtime to
> backends to keep some clocks on, and even that is questionable (the clock
> dependencies should be properly modeled instead of hidden with an obscure
> dailink setting). When I asked why exactly this flag was set in most cases
> the reason why 'because others did so in the past'...

It's there as a hack to keep clocks live or to avoid pops and clicks as
the datastream goes to zero during shutdown if the CODEC doesn't support
mute or otherwise handle the input going away cleanly (which is just a
more general case of keeping the clocks live usually).  You'd do it for
front ends as well to make sure things are consistent.
diff mbox series

Patch

diff --git a/include/sound/soc.h b/include/sound/soc.h
index f0e4f36..575ec11 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1160,6 +1160,7 @@  struct snd_soc_pcm_runtime {
 	for (; ((--i) >= 0) && ((dai) = rtd->codec_dais[i]);)
 
 void snd_soc_close_delayed_work(struct snd_soc_pcm_runtime *rtd);
+void snd_soc_stream_stop(struct snd_soc_pcm_runtime *rtd, int stream);
 
 /* mixer control */
 struct soc_mixer_control {
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 7bd574f..eb19df3 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -240,23 +240,7 @@  static int soc_compr_free(struct snd_compr_stream *cstream)
 	if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
 		cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
 
-	if (cstream->direction == SND_COMPRESS_PLAYBACK) {
-		if (snd_soc_runtime_ignore_pmdown_time(rtd)) {
-			snd_soc_dapm_stream_event(rtd,
-						  SNDRV_PCM_STREAM_PLAYBACK,
-						  SND_SOC_DAPM_STREAM_STOP);
-		} else {
-			rtd->pop_wait = 1;
-			queue_delayed_work(system_power_efficient_wq,
-					   &rtd->delayed_work,
-					   msecs_to_jiffies(rtd->pmdown_time));
-		}
-	} else {
-		/* capture streams can be powered down now */
-		snd_soc_dapm_stream_event(rtd,
-					  SNDRV_PCM_STREAM_CAPTURE,
-					  SND_SOC_DAPM_STREAM_STOP);
-	}
+	snd_soc_stream_stop(rtd, stream);
 
 	mutex_unlock(&rtd->card->pcm_mutex);
 	return 0;
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index f8ed927..28ebfad 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -387,6 +387,29 @@  void snd_soc_close_delayed_work(struct snd_soc_pcm_runtime *rtd)
 }
 EXPORT_SYMBOL_GPL(snd_soc_close_delayed_work);
 
+void snd_soc_stream_stop(struct snd_soc_pcm_runtime *rtd, int stream)
+{
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		if (snd_soc_runtime_ignore_pmdown_time(rtd)) {
+			/* powered down playback stream now */
+			snd_soc_dapm_stream_event(rtd,
+						  SNDRV_PCM_STREAM_PLAYBACK,
+						  SND_SOC_DAPM_STREAM_STOP);
+		} else {
+			/* start delayed pop wq here for playback streams */
+			rtd->pop_wait = 1;
+			queue_delayed_work(system_power_efficient_wq,
+					   &rtd->delayed_work,
+					   msecs_to_jiffies(rtd->pmdown_time));
+		}
+	} else {
+		/* capture streams can be powered down now */
+		snd_soc_dapm_stream_event(rtd, SNDRV_PCM_STREAM_CAPTURE,
+					  SND_SOC_DAPM_STREAM_STOP);
+	}
+}
+EXPORT_SYMBOL_GPL(snd_soc_stream_stop);
+
 static void soc_release_rtd_dev(struct device *dev)
 {
 	/* "dev" means "rtd->dev" */
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index ad908e0..c9ff50e 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -672,24 +672,7 @@  static int soc_pcm_close(struct snd_pcm_substream *substream)
 
 	soc_pcm_components_close(substream, NULL);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
-		if (snd_soc_runtime_ignore_pmdown_time(rtd)) {
-			/* powered down playback stream now */
-			snd_soc_dapm_stream_event(rtd,
-						  SNDRV_PCM_STREAM_PLAYBACK,
-						  SND_SOC_DAPM_STREAM_STOP);
-		} else {
-			/* start delayed pop wq here for playback streams */
-			rtd->pop_wait = 1;
-			queue_delayed_work(system_power_efficient_wq,
-					   &rtd->delayed_work,
-					   msecs_to_jiffies(rtd->pmdown_time));
-		}
-	} else {
-		/* capture streams can be powered down now */
-		snd_soc_dapm_stream_event(rtd, SNDRV_PCM_STREAM_CAPTURE,
-					  SND_SOC_DAPM_STREAM_STOP);
-	}
+	snd_soc_stream_stop(rtd, substream->stream);
 
 	mutex_unlock(&rtd->card->pcm_mutex);