[alsa-devel] [PATCH v1 0/3] Allow reconfiguration of clock rate
There is use case in Gstreamer ALSA sink, in case of changed caps Gsreatmer reconfigs and it calls snd_pcm_hw_free() before snd_pcm_prepre(). See gstreamer1.0-plugins-base/ gst-libs/gst/audio/gstaudiobasesink.c: gst_audio_base_sink_setcaps(): - gst_audio_ring_buffer_release() - gst_audio_sink_ring_buffer_release() - gst_alsasink_unprepare() - snd_pcm_hw_free() is called before - gst_audio_ring_buffer_acquire() - gst_audio_sink_ring_buffer_acquire() - gst_alsasink_prepare() - set_hwparams() - snd_pcm_hw_params() - snd_pcm_prepare()
But with current implementation after clock rate is started in .prepare reconfiguration of clock rate is not allowed, unless the stream is stopped.
This patch set by move stop of clock to .hw_free callback, to allow reconfiguration of clock rate.
Jiada Wang (1): ASoC: rsnd: call .hw_{params,free} in pair for same stream
Timo Wischer (2): ASoC: rsnd: Support hw_free() callback at DAI level ASoC: rsnd: Allow reconfiguration of clock rate
sound/soc/sh/rcar/core.c | 22 +++++++++++++-- sound/soc/sh/rcar/rsnd.h | 36 ++++++++++++++++++++---- sound/soc/sh/rcar/ssi.c | 61 +++++++++++++++++++++++++++++----------- sound/soc/sh/rcar/ssiu.c | 3 +- 4 files changed, 96 insertions(+), 26 deletions(-)
From: Timo Wischer twischer@de.adit-jv.com
This patch provides the needed infrastructure to support calling hw_free() at the DAI level. This is for example required to free resources allocated in hw_params() callback.
The modification of __rsnd_mod_add_hw_params does not have any side effects because rsnd_mod_ops::hw_params callback is not used by anyone until now.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com Signed-off-by: Jiada Wang jiada_wang@mentor.com --- sound/soc/sh/rcar/core.c | 16 +++++++++++++++- sound/soc/sh/rcar/rsnd.h | 12 +++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 56e8dae9a15c..bda5b958d0dc 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -1421,6 +1421,20 @@ static int rsnd_hw_params(struct snd_pcm_substream *substream, params_buffer_bytes(hw_params)); }
+static int rsnd_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_dai *dai = rsnd_substream_to_dai(substream); + struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai); + struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream); + int ret; + + ret = rsnd_dai_call(hw_free, io, substream); + if (ret) + return ret; + + return snd_pcm_lib_free_pages(substream); +} + static snd_pcm_uframes_t rsnd_pointer(struct snd_pcm_substream *substream) { struct snd_soc_dai *dai = rsnd_substream_to_dai(substream); @@ -1436,7 +1450,7 @@ static snd_pcm_uframes_t rsnd_pointer(struct snd_pcm_substream *substream) static const struct snd_pcm_ops rsnd_pcm_ops = { .ioctl = snd_pcm_lib_ioctl, .hw_params = rsnd_hw_params, - .hw_free = snd_pcm_lib_free_pages, + .hw_free = rsnd_hw_free, .pointer = rsnd_pointer, };
diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h index 7727add3eb1a..ea6cbaa9743e 100644 --- a/sound/soc/sh/rcar/rsnd.h +++ b/sound/soc/sh/rcar/rsnd.h @@ -327,6 +327,9 @@ struct rsnd_mod_ops { int (*cleanup)(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv); + int (*hw_free)(struct rsnd_mod *mod, + struct rsnd_dai_stream *io, + struct snd_pcm_substream *substream); u32 *(*get_status)(struct rsnd_mod *mod, struct rsnd_dai_stream *io, enum rsnd_mod_type type); @@ -351,12 +354,12 @@ struct rsnd_mod { * * B 0: init 1: quit * C 0: start 1: stop + * D 0: hw_params 1: hw_free * * H is always called (see __rsnd_mod_call) * H 0: probe 1: remove * H 0: pcm_new * H 0: fallback - * H 0: hw_params * H 0: pointer * H 0: prepare * H 0: cleanup @@ -365,12 +368,13 @@ struct rsnd_mod { #define __rsnd_mod_shift_quit 4 #define __rsnd_mod_shift_start 8 #define __rsnd_mod_shift_stop 8 +#define __rsnd_mod_shift_hw_params 12 +#define __rsnd_mod_shift_hw_free 12 #define __rsnd_mod_shift_probe 28 /* always called */ #define __rsnd_mod_shift_remove 28 /* always called */ #define __rsnd_mod_shift_irq 28 /* always called */ #define __rsnd_mod_shift_pcm_new 28 /* always called */ #define __rsnd_mod_shift_fallback 28 /* always called */ -#define __rsnd_mod_shift_hw_params 28 /* always called */ #define __rsnd_mod_shift_pointer 28 /* always called */ #define __rsnd_mod_shift_prepare 28 /* always called */ #define __rsnd_mod_shift_cleanup 28 /* always called */ @@ -383,10 +387,11 @@ struct rsnd_mod { #define __rsnd_mod_add_quit -1 #define __rsnd_mod_add_start 1 #define __rsnd_mod_add_stop -1 +#define __rsnd_mod_add_hw_params 1 +#define __rsnd_mod_add_hw_free -1 #define __rsnd_mod_add_irq 0 #define __rsnd_mod_add_pcm_new 0 #define __rsnd_mod_add_fallback 0 -#define __rsnd_mod_add_hw_params 0 #define __rsnd_mod_add_pointer 0
#define __rsnd_mod_call_probe 0 @@ -402,6 +407,7 @@ struct rsnd_mod { #define __rsnd_mod_call_fallback 0 #define __rsnd_mod_call_hw_params 0 #define __rsnd_mod_call_pointer 0 +#define __rsnd_mod_call_hw_free 1
#define rsnd_mod_to_priv(mod) ((mod)->priv) #define rsnd_mod_power_on(mod) clk_enable((mod)->clk)
The patch
ASoC: rsnd: Support hw_free() callback at DAI level
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.4
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 859fd6cbf1fb32b5428c26f837215c085b8a822e Mon Sep 17 00:00:00 2001
From: Timo Wischer twischer@de.adit-jv.com Date: Mon, 22 Jul 2019 16:24:01 +0900 Subject: [PATCH] ASoC: rsnd: Support hw_free() callback at DAI level
This patch provides the needed infrastructure to support calling hw_free() at the DAI level. This is for example required to free resources allocated in hw_params() callback.
The modification of __rsnd_mod_add_hw_params does not have any side effects because rsnd_mod_ops::hw_params callback is not used by anyone until now.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com Signed-off-by: Jiada Wang jiada_wang@mentor.com Link: https://lore.kernel.org/r/20190722072403.11008-2-jiada_wang@mentor.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/sh/rcar/core.c | 16 +++++++++++++++- sound/soc/sh/rcar/rsnd.h | 12 +++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 56e8dae9a15c..bda5b958d0dc 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -1421,6 +1421,20 @@ static int rsnd_hw_params(struct snd_pcm_substream *substream, params_buffer_bytes(hw_params)); }
+static int rsnd_hw_free(struct snd_pcm_substream *substream) +{ + struct snd_soc_dai *dai = rsnd_substream_to_dai(substream); + struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai); + struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream); + int ret; + + ret = rsnd_dai_call(hw_free, io, substream); + if (ret) + return ret; + + return snd_pcm_lib_free_pages(substream); +} + static snd_pcm_uframes_t rsnd_pointer(struct snd_pcm_substream *substream) { struct snd_soc_dai *dai = rsnd_substream_to_dai(substream); @@ -1436,7 +1450,7 @@ static snd_pcm_uframes_t rsnd_pointer(struct snd_pcm_substream *substream) static const struct snd_pcm_ops rsnd_pcm_ops = { .ioctl = snd_pcm_lib_ioctl, .hw_params = rsnd_hw_params, - .hw_free = snd_pcm_lib_free_pages, + .hw_free = rsnd_hw_free, .pointer = rsnd_pointer, };
diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h index 7727add3eb1a..ea6cbaa9743e 100644 --- a/sound/soc/sh/rcar/rsnd.h +++ b/sound/soc/sh/rcar/rsnd.h @@ -327,6 +327,9 @@ struct rsnd_mod_ops { int (*cleanup)(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv); + int (*hw_free)(struct rsnd_mod *mod, + struct rsnd_dai_stream *io, + struct snd_pcm_substream *substream); u32 *(*get_status)(struct rsnd_mod *mod, struct rsnd_dai_stream *io, enum rsnd_mod_type type); @@ -351,12 +354,12 @@ struct rsnd_mod { * * B 0: init 1: quit * C 0: start 1: stop + * D 0: hw_params 1: hw_free * * H is always called (see __rsnd_mod_call) * H 0: probe 1: remove * H 0: pcm_new * H 0: fallback - * H 0: hw_params * H 0: pointer * H 0: prepare * H 0: cleanup @@ -365,12 +368,13 @@ struct rsnd_mod { #define __rsnd_mod_shift_quit 4 #define __rsnd_mod_shift_start 8 #define __rsnd_mod_shift_stop 8 +#define __rsnd_mod_shift_hw_params 12 +#define __rsnd_mod_shift_hw_free 12 #define __rsnd_mod_shift_probe 28 /* always called */ #define __rsnd_mod_shift_remove 28 /* always called */ #define __rsnd_mod_shift_irq 28 /* always called */ #define __rsnd_mod_shift_pcm_new 28 /* always called */ #define __rsnd_mod_shift_fallback 28 /* always called */ -#define __rsnd_mod_shift_hw_params 28 /* always called */ #define __rsnd_mod_shift_pointer 28 /* always called */ #define __rsnd_mod_shift_prepare 28 /* always called */ #define __rsnd_mod_shift_cleanup 28 /* always called */ @@ -383,10 +387,11 @@ struct rsnd_mod { #define __rsnd_mod_add_quit -1 #define __rsnd_mod_add_start 1 #define __rsnd_mod_add_stop -1 +#define __rsnd_mod_add_hw_params 1 +#define __rsnd_mod_add_hw_free -1 #define __rsnd_mod_add_irq 0 #define __rsnd_mod_add_pcm_new 0 #define __rsnd_mod_add_fallback 0 -#define __rsnd_mod_add_hw_params 0 #define __rsnd_mod_add_pointer 0
#define __rsnd_mod_call_probe 0 @@ -402,6 +407,7 @@ struct rsnd_mod { #define __rsnd_mod_call_fallback 0 #define __rsnd_mod_call_hw_params 0 #define __rsnd_mod_call_pointer 0 +#define __rsnd_mod_call_hw_free 1
#define rsnd_mod_to_priv(mod) ((mod)->priv) #define rsnd_mod_power_on(mod) clk_enable((mod)->clk)
From: Timo Wischer twischer@de.adit-jv.com
Currently after clock rate is started in .prepare reconfiguration of clock rate is not allowed, unless the stream is stopped.
But there is use case in Gstreamer ALSA sink, in case of changed caps Gsreatmer reconfigs and it calls snd_pcm_hw_free() before snd_pcm_prepre(). See gstreamer1.0-plugins-base/ gst-libs/gst/audio/gstaudiobasesink.c: gst_audio_base_sink_setcaps(): - gst_audio_ring_buffer_release() - gst_audio_sink_ring_buffer_release() - gst_alsasink_unprepare() - snd_pcm_hw_free() is called before - gst_audio_ring_buffer_acquire() - gst_audio_sink_ring_buffer_acquire() - gst_alsasink_prepare() - set_hwparams() - snd_pcm_hw_params() - snd_pcm_prepare()
The issue mentioned in this commit can be reproduced with the following aplay patch:
>diff --git a/aplay/aplay.c b/aplay/aplay.c >@@ -2760,6 +2760,8 @@ static void playback_go(int fd, size_t loaded, > header(rtype, name); > set_params(); >+ hwparams.rate = (hwparams.rate == 48000) ? 44100 : 48000; >+ set_params(); > > while (loaded > chunk_bytes && written < count && !in_aborting) > { > if (pcm_write(audiobuf + written, chunk_size) <= 0)
$ aplay -Dplughw:0,0,0 -c8 -fS24_LE -r48000 /dev/zero rcar_sound ec500000.sound: SSI parent/child should use same rate rcar_sound ec500000.sound: ssi[3] : prepare error -22 rcar_sound ec500000.sound: ASoC: cpu DAI prepare error: -22 rsnd_link0: ASoC: prepare FE rsnd_link0 failed
this patch address the issue by stop clock in .hw_free, to allow reconfiguration of clock rate without stop of the stream.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com Signed-off-by: Jiada Wang jiada_wang@mentor.com --- sound/soc/sh/rcar/ssi.c | 53 +++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index f6a7466622ea..f43937d2c588 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -286,7 +286,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, if (rsnd_ssi_is_multi_slave(mod, io)) return 0;
- if (ssi->usrcnt > 0) { + if (ssi->rate) { if (ssi->rate != rate) { dev_err(dev, "SSI parent/child should use same rate\n"); return -EINVAL; @@ -471,13 +471,9 @@ static int rsnd_ssi_init(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv) { - struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); - if (!rsnd_ssi_is_run_mods(mod, io)) return 0;
- ssi->usrcnt++; - rsnd_mod_power_on(mod);
rsnd_ssi_config_init(mod, io); @@ -505,18 +501,8 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod, return -EIO; }
- rsnd_ssi_master_clk_stop(mod, io); - rsnd_mod_power_off(mod);
- ssi->usrcnt--; - - if (!ssi->usrcnt) { - ssi->cr_own = 0; - ssi->cr_mode = 0; - ssi->wsr = 0; - } - return 0; }
@@ -525,6 +511,7 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); struct rsnd_dai *rdai = rsnd_io_to_rdai(io); unsigned int fmt_width = snd_pcm_format_width(params_format(params));
@@ -536,6 +523,11 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, return -EINVAL; }
+ if (!rsnd_ssi_is_run_mods(mod, io)) + return 0; + + ssi->usrcnt++; + return 0; }
@@ -913,6 +905,35 @@ static int rsnd_ssi_prepare(struct rsnd_mod *mod, return rsnd_ssi_master_clk_start(mod, io); }
+static int rsnd_ssi_hw_free(struct rsnd_mod *mod, struct rsnd_dai_stream *io, + struct snd_pcm_substream *substream) +{ + struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); + + if (!rsnd_ssi_is_run_mods(mod, io)) + return 0; + + if (!ssi->usrcnt) { + struct rsnd_priv *priv = rsnd_mod_to_priv(mod); + struct device *dev = rsnd_priv_to_dev(priv); + + dev_err(dev, "%s usrcnt error\n", rsnd_mod_name(mod)); + return -EIO; + } + + rsnd_ssi_master_clk_stop(mod, io); + + ssi->usrcnt--; + + if (!ssi->usrcnt) { + ssi->cr_own = 0; + ssi->cr_mode = 0; + ssi->wsr = 0; + } + + return 0; +} + static struct rsnd_mod_ops rsnd_ssi_pio_ops = { .name = SSI_NAME, .probe = rsnd_ssi_common_probe, @@ -926,6 +947,7 @@ static struct rsnd_mod_ops rsnd_ssi_pio_ops = { .pcm_new = rsnd_ssi_pcm_new, .hw_params = rsnd_ssi_hw_params, .prepare = rsnd_ssi_prepare, + .hw_free = rsnd_ssi_hw_free, .get_status = rsnd_ssi_get_status, };
@@ -1012,6 +1034,7 @@ static struct rsnd_mod_ops rsnd_ssi_dma_ops = { .pcm_new = rsnd_ssi_pcm_new, .fallback = rsnd_ssi_fallback, .hw_params = rsnd_ssi_hw_params, + .hw_free = rsnd_ssi_hw_free, .prepare = rsnd_ssi_prepare, .get_status = rsnd_ssi_get_status, };
Hi Jiada
The solution looks very over-kill to me, especiallyq [3/3] patch is too much to me.
1st, can we start clock at .hw_param, instead of .prepare ? and stop it at .hw_free ?
2nd, can we keep usrcnt setup as-is ? I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not 0 ? similar for rsnd_ssi_master_clk_stop()
static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, struct rsnd_dai_stream *io) { ... if (ssi->rate) return 0; ... }
static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, struct rsnd_dai_stream *io) { ... - if (ssi->usrcnt > 1) + if (ssi->rate == 0) return; ... }
From: Timo Wischer twischer@de.adit-jv.com
Currently after clock rate is started in .prepare reconfiguration of clock rate is not allowed, unless the stream is stopped.
But there is use case in Gstreamer ALSA sink, in case of changed caps Gsreatmer reconfigs and it calls snd_pcm_hw_free() before snd_pcm_prepre(). See gstreamer1.0-plugins-base/ gst-libs/gst/audio/gstaudiobasesink.c: gst_audio_base_sink_setcaps():
- gst_audio_ring_buffer_release()
- gst_audio_sink_ring_buffer_release()
- gst_alsasink_unprepare()
- snd_pcm_hw_free()
is called before
- gst_audio_ring_buffer_acquire()
- gst_audio_sink_ring_buffer_acquire()
- gst_alsasink_prepare()
- set_hwparams()
- snd_pcm_hw_params()
- snd_pcm_prepare()
The issue mentioned in this commit can be reproduced with the following aplay patch:
>diff --git a/aplay/aplay.c b/aplay/aplay.c >@@ -2760,6 +2760,8 @@ static void playback_go(int fd, size_t loaded, > header(rtype, name); > set_params(); >+ hwparams.rate = (hwparams.rate == 48000) ? 44100 : 48000; >+ set_params(); > > while (loaded > chunk_bytes && written < count && !in_aborting) > { > if (pcm_write(audiobuf + written, chunk_size) <= 0)$ aplay -Dplughw:0,0,0 -c8 -fS24_LE -r48000 /dev/zero rcar_sound ec500000.sound: SSI parent/child should use same rate rcar_sound ec500000.sound: ssi[3] : prepare error -22 rcar_sound ec500000.sound: ASoC: cpu DAI prepare error: -22 rsnd_link0: ASoC: prepare FE rsnd_link0 failed
this patch address the issue by stop clock in .hw_free, to allow reconfiguration of clock rate without stop of the stream.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com Signed-off-by: Jiada Wang jiada_wang@mentor.com
sound/soc/sh/rcar/ssi.c | 53 +++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index f6a7466622ea..f43937d2c588 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -286,7 +286,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, if (rsnd_ssi_is_multi_slave(mod, io)) return 0;
- if (ssi->usrcnt > 0) {
- if (ssi->rate) { if (ssi->rate != rate) { dev_err(dev, "SSI parent/child should use same rate\n"); return -EINVAL;
@@ -471,13 +471,9 @@ static int rsnd_ssi_init(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv) {
struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
if (!rsnd_ssi_is_run_mods(mod, io)) return 0;
ssi->usrcnt++;
rsnd_mod_power_on(mod);
rsnd_ssi_config_init(mod, io);
@@ -505,18 +501,8 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod, return -EIO; }
rsnd_ssi_master_clk_stop(mod, io);
rsnd_mod_power_off(mod);
ssi->usrcnt--;
if (!ssi->usrcnt) {
ssi->cr_own = 0;ssi->cr_mode = 0;ssi->wsr = 0;}
return 0;
}
@@ -525,6 +511,7 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) {
- struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); struct rsnd_dai *rdai = rsnd_io_to_rdai(io); unsigned int fmt_width = snd_pcm_format_width(params_format(params));
@@ -536,6 +523,11 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, return -EINVAL; }
- if (!rsnd_ssi_is_run_mods(mod, io))
return 0;- ssi->usrcnt++;
- return 0;
}
@@ -913,6 +905,35 @@ static int rsnd_ssi_prepare(struct rsnd_mod *mod, return rsnd_ssi_master_clk_start(mod, io); }
+static int rsnd_ssi_hw_free(struct rsnd_mod *mod, struct rsnd_dai_stream *io,
struct snd_pcm_substream *substream)+{
- struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
- if (!rsnd_ssi_is_run_mods(mod, io))
return 0;- if (!ssi->usrcnt) {
struct rsnd_priv *priv = rsnd_mod_to_priv(mod);struct device *dev = rsnd_priv_to_dev(priv);dev_err(dev, "%s usrcnt error\n", rsnd_mod_name(mod));return -EIO;- }
- rsnd_ssi_master_clk_stop(mod, io);
- ssi->usrcnt--;
- if (!ssi->usrcnt) {
ssi->cr_own = 0;ssi->cr_mode = 0;ssi->wsr = 0;- }
- return 0;
+}
static struct rsnd_mod_ops rsnd_ssi_pio_ops = { .name = SSI_NAME, .probe = rsnd_ssi_common_probe, @@ -926,6 +947,7 @@ static struct rsnd_mod_ops rsnd_ssi_pio_ops = { .pcm_new = rsnd_ssi_pcm_new, .hw_params = rsnd_ssi_hw_params, .prepare = rsnd_ssi_prepare,
- .hw_free = rsnd_ssi_hw_free, .get_status = rsnd_ssi_get_status,
};
@@ -1012,6 +1034,7 @@ static struct rsnd_mod_ops rsnd_ssi_dma_ops = { .pcm_new = rsnd_ssi_pcm_new, .fallback = rsnd_ssi_fallback, .hw_params = rsnd_ssi_hw_params,
- .hw_free = rsnd_ssi_hw_free, .prepare = rsnd_ssi_prepare, .get_status = rsnd_ssi_get_status,
};
2.19.2
Hi Jiada, again
The solution looks very over-kill to me, especiallyq [3/3] patch is too much to me.
1st, can we start clock at .hw_param, instead of .prepare ? and stop it at .hw_free ?
2nd, can we keep usrcnt setup as-is ? I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not 0 ? similar for rsnd_ssi_master_clk_stop()
In my quick check, I used your [1/3] patch and below. It works for me, but I don't know detail of your test case.
---------- diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index f6a7466..f26ffd1 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -286,19 +286,8 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, if (rsnd_ssi_is_multi_slave(mod, io)) return 0;
- if (ssi->usrcnt > 0) { - if (ssi->rate != rate) { - dev_err(dev, "SSI parent/child should use same rate\n"); - return -EINVAL; - } - - if (ssi->chan != chan) { - dev_err(dev, "SSI parent/child should use same chan\n"); - return -EINVAL; - } - + if (ssi->rate) return 0; - }
if (rsnd_runtime_is_tdm_split(io)) chan = rsnd_io_converted_chan(io); @@ -349,7 +338,7 @@ static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, if (!rsnd_ssi_can_output_clk(mod)) return;
- if (ssi->usrcnt > 1) + if (!ssi->rate) return;
ssi->cr_clk = 0; @@ -539,6 +528,17 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, return 0; }
+static int rsnd_ssi_hw_free(struct rsnd_mod *mod, struct rsnd_dai_stream *io, + struct snd_pcm_substream *substream) +{ + if (!rsnd_ssi_is_run_mods(mod, io)) + return 0; + + rsnd_ssi_master_clk_stop(mod, io); + + return 0; +} + static int rsnd_ssi_start(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv) @@ -925,6 +925,7 @@ static struct rsnd_mod_ops rsnd_ssi_pio_ops = { .pointer = rsnd_ssi_pio_pointer, .pcm_new = rsnd_ssi_pcm_new, .hw_params = rsnd_ssi_hw_params, + .hw_free = rsnd_ssi_hw_free, .prepare = rsnd_ssi_prepare, .get_status = rsnd_ssi_get_status, }; @@ -1012,6 +1013,7 @@ static struct rsnd_mod_ops rsnd_ssi_dma_ops = { .pcm_new = rsnd_ssi_pcm_new, .fallback = rsnd_ssi_fallback, .hw_params = rsnd_ssi_hw_params, + .hw_free = rsnd_ssi_hw_free, .prepare = rsnd_ssi_prepare, .get_status = rsnd_ssi_get_status, }; ----------
Hi Morimoto-san
Sorry for the delayed response
On 2019/07/22 17:41, Kuninori Morimoto wrote:
Hi Jiada
The solution looks very over-kill to me, especiallyq [3/3] patch is too much to me.
1st, can we start clock at .hw_param, instead of .prepare ? and stop it at .hw_free ?
the reasoning to move start of clock from .init to .prepare by commit 4d230d1271064 ("ASoC: rsnd: fixup not to call clk_get/set under non-atomic") is to prevent clk_{get, set_rate} be called from atomic context, since .hw_params is non atomic context, so I think start of clock can be moved from .prepare to .hw_params
2nd, can we keep usrcnt setup as-is ? I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not 0 ?
I don't fully understand your 2nd question, in case of rsnd_ssi_master_clk_stop(), if avoid rsnd_ssi_master_clk_stop() when ssi->rate is 0 by apply following change
static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, struct rsnd_dai_stream *io) { ... - if (ssi->usrcnt > 1) + if (ssi->rate == 0) return; ... }
then when any IO stream with same SSI calls .hw_free, the other IO stream's clock will be stopped too.
Thanks, Jiada
similar for rsnd_ssi_master_clk_stop()
static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, struct rsnd_dai_stream *io) { ... if (ssi->rate) return 0; ... }
static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, struct rsnd_dai_stream *io) { ...
if (ssi->usrcnt > 1)
if (ssi->rate == 0) return;From: Timo Wischer twischer@de.adit-jv.com
Currently after clock rate is started in .prepare reconfiguration of clock rate is not allowed, unless the stream is stopped.
But there is use case in Gstreamer ALSA sink, in case of changed caps Gsreatmer reconfigs and it calls snd_pcm_hw_free() before snd_pcm_prepre(). See gstreamer1.0-plugins-base/ gst-libs/gst/audio/gstaudiobasesink.c: gst_audio_base_sink_setcaps():
- gst_audio_ring_buffer_release()
- gst_audio_sink_ring_buffer_release()
- gst_alsasink_unprepare()
- snd_pcm_hw_free()
is called before
- gst_audio_ring_buffer_acquire()
- gst_audio_sink_ring_buffer_acquire()
- gst_alsasink_prepare()
- set_hwparams()
- snd_pcm_hw_params()
- snd_pcm_prepare()
The issue mentioned in this commit can be reproduced with the following aplay patch:
>diff --git a/aplay/aplay.c b/aplay/aplay.c >@@ -2760,6 +2760,8 @@ static void playback_go(int fd, size_t loaded, > header(rtype, name); > set_params(); >+ hwparams.rate = (hwparams.rate == 48000) ? 44100 : 48000; >+ set_params(); > > while (loaded > chunk_bytes && written < count && !in_aborting) > { > if (pcm_write(audiobuf + written, chunk_size) <= 0)$ aplay -Dplughw:0,0,0 -c8 -fS24_LE -r48000 /dev/zero rcar_sound ec500000.sound: SSI parent/child should use same rate rcar_sound ec500000.sound: ssi[3] : prepare error -22 rcar_sound ec500000.sound: ASoC: cpu DAI prepare error: -22 rsnd_link0: ASoC: prepare FE rsnd_link0 failed
this patch address the issue by stop clock in .hw_free, to allow reconfiguration of clock rate without stop of the stream.
Signed-off-by: Timo Wischer twischer@de.adit-jv.com Signed-off-by: Jiada Wang jiada_wang@mentor.com
sound/soc/sh/rcar/ssi.c | 53 +++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-)
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index f6a7466622ea..f43937d2c588 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -286,7 +286,7 @@ static int rsnd_ssi_master_clk_start(struct rsnd_mod *mod, if (rsnd_ssi_is_multi_slave(mod, io)) return 0;
- if (ssi->usrcnt > 0) {
- if (ssi->rate) { if (ssi->rate != rate) { dev_err(dev, "SSI parent/child should use same rate\n"); return -EINVAL;
@@ -471,13 +471,9 @@ static int rsnd_ssi_init(struct rsnd_mod *mod, struct rsnd_dai_stream *io, struct rsnd_priv *priv) {
struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
if (!rsnd_ssi_is_run_mods(mod, io)) return 0;
ssi->usrcnt++;
rsnd_mod_power_on(mod);
rsnd_ssi_config_init(mod, io);
@@ -505,18 +501,8 @@ static int rsnd_ssi_quit(struct rsnd_mod *mod, return -EIO; }
rsnd_ssi_master_clk_stop(mod, io);
rsnd_mod_power_off(mod);
ssi->usrcnt--;
if (!ssi->usrcnt) {
ssi->cr_own = 0;ssi->cr_mode = 0;ssi->wsr = 0;}
return 0; }
@@ -525,6 +511,7 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) {
- struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod); struct rsnd_dai *rdai = rsnd_io_to_rdai(io); unsigned int fmt_width = snd_pcm_format_width(params_format(params));
@@ -536,6 +523,11 @@ static int rsnd_ssi_hw_params(struct rsnd_mod *mod, return -EINVAL; }
- if (!rsnd_ssi_is_run_mods(mod, io))
return 0;- ssi->usrcnt++;
- return 0; }
@@ -913,6 +905,35 @@ static int rsnd_ssi_prepare(struct rsnd_mod *mod, return rsnd_ssi_master_clk_start(mod, io); }
+static int rsnd_ssi_hw_free(struct rsnd_mod *mod, struct rsnd_dai_stream *io,
struct snd_pcm_substream *substream)+{
- struct rsnd_ssi *ssi = rsnd_mod_to_ssi(mod);
- if (!rsnd_ssi_is_run_mods(mod, io))
return 0;- if (!ssi->usrcnt) {
struct rsnd_priv *priv = rsnd_mod_to_priv(mod);struct device *dev = rsnd_priv_to_dev(priv);dev_err(dev, "%s usrcnt error\n", rsnd_mod_name(mod));return -EIO;- }
- rsnd_ssi_master_clk_stop(mod, io);
- ssi->usrcnt--;
- if (!ssi->usrcnt) {
ssi->cr_own = 0;ssi->cr_mode = 0;ssi->wsr = 0;- }
- return 0;
+}
- static struct rsnd_mod_ops rsnd_ssi_pio_ops = { .name = SSI_NAME, .probe = rsnd_ssi_common_probe,
@@ -926,6 +947,7 @@ static struct rsnd_mod_ops rsnd_ssi_pio_ops = { .pcm_new = rsnd_ssi_pcm_new, .hw_params = rsnd_ssi_hw_params, .prepare = rsnd_ssi_prepare,
- .hw_free = rsnd_ssi_hw_free, .get_status = rsnd_ssi_get_status, };
@@ -1012,6 +1034,7 @@ static struct rsnd_mod_ops rsnd_ssi_dma_ops = { .pcm_new = rsnd_ssi_pcm_new, .fallback = rsnd_ssi_fallback, .hw_params = rsnd_ssi_hw_params,
- .hw_free = rsnd_ssi_hw_free, .prepare = rsnd_ssi_prepare, .get_status = rsnd_ssi_get_status, };
-- 2.19.2
Hi Jiada
2nd, can we keep usrcnt setup as-is ? I guess we can just avoid rsnd_ssi_master_clk_start() if ssi->rate was not 0 ?
I don't fully understand your 2nd question, in case of rsnd_ssi_master_clk_stop(), if avoid rsnd_ssi_master_clk_stop() when ssi->rate is 0 by apply following change
static void rsnd_ssi_master_clk_stop(struct rsnd_mod *mod, struct rsnd_dai_stream *io) { ...
if (ssi->usrcnt > 1)
if (ssi->rate == 0) return;then when any IO stream with same SSI calls .hw_free, the other IO stream's clock will be stopped too.
I think we can find more simple solution if we can find good ideas. For example, how about to add new counter for hw_params/hw_free ? Anyway, [3/3] patch is too much over-kill to me.
And, please don't exchange usrcnt inc/dec position at [2/3]. It is for open/close.
Thank you for your help !! Best regards --- Kuninori Morimoto
Currently usrcnt is {in,de}cremented in .hw_{params,free} callbacks, but .hw_free may be called multiple times without calling .hw_params. this causes the usrcnt be decremented wrongly.
This patch allows .hw_{params,free} to be called only in pairs for the same stream which balances the {in,de}crement of usrcnt.
Signed-off-by: Jiada Wang jiada_wang@mentor.com Signed-off-by: Timo Wischer twischer@de.adit-jv.com --- sound/soc/sh/rcar/core.c | 6 ++++-- sound/soc/sh/rcar/rsnd.h | 24 ++++++++++++++++++++++-- sound/soc/sh/rcar/ssi.c | 8 ++++++-- sound/soc/sh/rcar/ssiu.c | 3 ++- 4 files changed, 34 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index bda5b958d0dc..b9330bdadbd3 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -172,7 +172,8 @@ char *rsnd_mod_name(struct rsnd_mod *mod)
u32 *rsnd_mod_get_status(struct rsnd_mod *mod, struct rsnd_dai_stream *io, - enum rsnd_mod_type type) + enum rsnd_mod_type type, + int flag) { return &mod->status; } @@ -548,7 +549,8 @@ static int rsnd_status_update(u32 *status, enum rsnd_mod_type *types = rsnd_mod_sequence[is_play]; \ for_each_rsnd_mod_arrays(i, mod, io, types, RSND_MOD_MAX) { \ int tmp = 0; \ - u32 *status = mod->ops->get_status(mod, io, types[i]); \ + u32 *status = mod->ops->get_status(mod, io, types[i], \ + __rsnd_mod_flag_##fn); \ int func_call = rsnd_status_update(status, \ __rsnd_mod_shift_##fn, \ __rsnd_mod_add_##fn, \ diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h index ea6cbaa9743e..b4e3e9289f8a 100644 --- a/sound/soc/sh/rcar/rsnd.h +++ b/sound/soc/sh/rcar/rsnd.h @@ -238,6 +238,7 @@ enum rsnd_reg { #define SSI9_BUSIF_DALIGN(i) (SSI9_BUSIF0_DALIGN + (i)) #define SSI_SYS_STATUS(i) (SSI_SYS_STATUS0 + (i))
+#define RSND_STATUS_ON_IO BIT(0)
struct rsnd_priv; struct rsnd_mod; @@ -332,7 +333,8 @@ struct rsnd_mod_ops { struct snd_pcm_substream *substream); u32 *(*get_status)(struct rsnd_mod *mod, struct rsnd_dai_stream *io, - enum rsnd_mod_type type); + enum rsnd_mod_type type, + int flag); int (*id)(struct rsnd_mod *mod); int (*id_sub)(struct rsnd_mod *mod); int (*id_cmd)(struct rsnd_mod *mod); @@ -379,6 +381,22 @@ struct rsnd_mod { #define __rsnd_mod_shift_prepare 28 /* always called */ #define __rsnd_mod_shift_cleanup 28 /* always called */
+#define __rsnd_mod_flag_init 0 +#define __rsnd_mod_flag_quit 0 +#define __rsnd_mod_flag_start 0 +#define __rsnd_mod_flag_stop 0 +#define __rsnd_mod_flag_hw_params RSND_STATUS_ON_IO +#define __rsnd_mod_flag_hw_free RSND_STATUS_ON_IO +#define __rsnd_mod_flag_probe 0 +#define __rsnd_mod_flag_remove 0 +#define __rsnd_mod_flag_irq 0 +#define __rsnd_mod_flag_pcm_new 0 +#define __rsnd_mod_flag_fallback 0 +#define __rsnd_mod_flag_pointer 0 +#define __rsnd_mod_flag_prepare 0 +#define __rsnd_mod_flag_cleanup 0 +#define __rsnd_mod_flag_set_fmt 0 + #define __rsnd_mod_add_probe 0 #define __rsnd_mod_add_remove 0 #define __rsnd_mod_add_prepare 0 @@ -428,7 +446,8 @@ void rsnd_mod_interrupt(struct rsnd_mod *mod, struct rsnd_dai_stream *io)); u32 *rsnd_mod_get_status(struct rsnd_mod *mod, struct rsnd_dai_stream *io, - enum rsnd_mod_type type); + enum rsnd_mod_type type, + int flag); int rsnd_mod_id(struct rsnd_mod *mod); int rsnd_mod_id_raw(struct rsnd_mod *mod); int rsnd_mod_id_sub(struct rsnd_mod *mod); @@ -496,6 +515,7 @@ struct rsnd_dai_stream { u32 converted_rate; /* converted sampling rate */ int converted_chan; /* converted channels */ u32 parent_ssi_status; + u32 status; u32 flags; };
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c index f43937d2c588..89b4029b290b 100644 --- a/sound/soc/sh/rcar/ssi.c +++ b/sound/soc/sh/rcar/ssi.c @@ -681,7 +681,8 @@ static irqreturn_t rsnd_ssi_interrupt(int irq, void *data)
static u32 *rsnd_ssi_get_status(struct rsnd_mod *mod, struct rsnd_dai_stream *io, - enum rsnd_mod_type type) + enum rsnd_mod_type type, + int flag) { /* * SSIP (= SSI parent) needs to be special, otherwise, @@ -711,7 +712,10 @@ static u32 *rsnd_ssi_get_status(struct rsnd_mod *mod, if (type == RSND_MOD_SSIP) return &io->parent_ssi_status;
- return rsnd_mod_get_status(mod, io, type); + if (flag && RSND_STATUS_ON_IO) + return &io->status; + + return rsnd_mod_get_status(mod, io, type, flag); }
/* diff --git a/sound/soc/sh/rcar/ssiu.c b/sound/soc/sh/rcar/ssiu.c index f35d88211887..45e4cd84fbc4 100644 --- a/sound/soc/sh/rcar/ssiu.c +++ b/sound/soc/sh/rcar/ssiu.c @@ -47,7 +47,8 @@ static const int gen3_id[] = { 0, 8, 16, 24, 32, 40, 41, 42, 43, 44 };
static u32 *rsnd_ssiu_get_status(struct rsnd_mod *mod, struct rsnd_dai_stream *io, - enum rsnd_mod_type type) + enum rsnd_mod_type type, + int flag) { struct rsnd_ssiu *ssiu = rsnd_mod_to_ssiu(mod); int busif = rsnd_mod_id_sub(mod);
participants (3)
-
Jiada Wang -
Kuninori Morimoto -
Mark Brown