* [PATCH] ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime
@ 2019-08-13 10:45 Peter Ujfalusi
2019-08-14 18:14 ` Pierre-Louis Bossart
2019-08-15 17:14 ` Applied "ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime" to the asoc tree Mark Brown
0 siblings, 2 replies; 6+ messages in thread
From: Peter Ujfalusi @ 2019-08-13 10:45 UTC (permalink / raw)
To: broonie, lgirdwood
Cc: alsa-devel, lars, ckeepax, pierre-louis.bossart,
kuninori.morimoto.gx
The pcm_mutex is used to prevent concurrent execution of snd_pcm_ops
callbacks. This works fine most of the cases but it can not handle setups
when the same DAI is used by different rtd, for example:
pcm3168a have two DAIs: one for Playback and one for Capture.
If the codec is connected to a single CPU DAI we need to have two dai_link
to support both playback and capture.
In this case the snd_pcm_ops callbacks can be executed in parallel causing
unexpected races in DAI drivers.
By moving the pcm_mutex up to card level this can be solved
while - hopefully - not breaking other setups.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
Hi,
I have tested the patch on several boards. it fixes the issue with single card
with multiple dai_links where the CPU side (mcasp) is the same.
However I can not test with anything which use DPCM. It would be great if the
patch would gather few tested-by from folks having access to more complicated
setups.
The backround of this patch:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-August/153864.html
Regards,
Peter
include/sound/soc.h | 6 +++--
sound/soc/soc-compress.c | 48 ++++++++++++++++++++--------------------
sound/soc/soc-core.c | 2 +-
sound/soc/soc-pcm.c | 36 +++++++++++++++---------------
4 files changed, 47 insertions(+), 45 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6ac6481b4882..ec2dbe0ac2aa 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -990,6 +990,10 @@ struct snd_soc_card {
struct mutex mutex;
struct mutex dapm_mutex;
+ /* Mutex for PCM operations */
+ struct mutex pcm_mutex;
+ enum snd_soc_pcm_subclass pcm_subclass;
+
spinlock_t dpcm_lock;
bool instantiated;
@@ -1107,8 +1111,6 @@ struct snd_soc_pcm_runtime {
struct device *dev;
struct snd_soc_card *card;
struct snd_soc_dai_link *dai_link;
- struct mutex pcm_mutex;
- enum snd_soc_pcm_subclass pcm_subclass;
struct snd_pcm_ops ops;
unsigned int params_select; /* currently selected param for dai link */
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 289211069a1e..9e54d8ae6d2c 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -80,7 +80,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cpu_dai->driver->cops && cpu_dai->driver->cops->startup) {
ret = cpu_dai->driver->cops->startup(cstream, cpu_dai);
@@ -108,7 +108,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
snd_soc_runtime_activate(rtd, cstream->direction);
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return 0;
@@ -118,7 +118,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -224,7 +224,7 @@ static void close_delayed_work(struct work_struct *work)
container_of(work, struct snd_soc_pcm_runtime, delayed_work.work);
struct snd_soc_dai *codec_dai = rtd->codec_dai;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
dev_dbg(rtd->dev,
"Compress ASoC: pop wq checking: %s status: %s waiting: %s\n",
@@ -239,7 +239,7 @@ static void close_delayed_work(struct work_struct *work)
SND_SOC_DAPM_STREAM_STOP);
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
}
static int soc_compr_free(struct snd_compr_stream *cstream)
@@ -249,7 +249,7 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
struct snd_soc_dai *codec_dai = rtd->codec_dai;
int stream;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cstream->direction == SND_COMPRESS_PLAYBACK)
stream = SNDRV_PCM_STREAM_PLAYBACK;
@@ -292,7 +292,7 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
SND_SOC_DAPM_STREAM_STOP);
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return 0;
}
@@ -375,7 +375,7 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
ret = soc_compr_components_trigger(cstream, cmd);
if (ret < 0)
@@ -394,7 +394,7 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
}
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -480,7 +480,7 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
/*
* First we call set_params for the CPU DAI, then the component
@@ -514,14 +514,14 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
/* cancel any delayed stream shutdown that is pending */
rtd->pop_wait = 0;
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
cancel_delayed_work_sync(&rtd->delayed_work);
return 0;
err:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -593,7 +593,7 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cpu_dai->driver->cops && cpu_dai->driver->cops->get_params) {
ret = cpu_dai->driver->cops->get_params(cstream, params, cpu_dai);
@@ -613,7 +613,7 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
}
err:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -625,7 +625,7 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
struct snd_soc_rtdcom_list *rtdcom;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -638,7 +638,7 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
break;
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -650,7 +650,7 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
struct snd_soc_rtdcom_list *rtdcom;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -664,7 +664,7 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
break;
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -676,7 +676,7 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cpu_dai->driver->cops && cpu_dai->driver->cops->ack) {
ret = cpu_dai->driver->cops->ack(cstream, bytes, cpu_dai);
@@ -697,7 +697,7 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
}
err:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -710,7 +710,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
int ret = 0;
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cpu_dai->driver->cops && cpu_dai->driver->cops->pointer)
cpu_dai->driver->cops->pointer(cstream, tstamp, cpu_dai);
@@ -726,7 +726,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
break;
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -738,7 +738,7 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
struct snd_soc_rtdcom_list *rtdcom;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -751,7 +751,7 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
break;
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index bb1e9e2c4ff4..7f2064889a23 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1344,7 +1344,6 @@ static int soc_post_component_init(struct snd_soc_pcm_runtime *rtd,
rtd->dev->groups = soc_dev_attr_groups;
dev_set_name(rtd->dev, "%s", name);
dev_set_drvdata(rtd->dev, rtd);
- mutex_init(&rtd->pcm_mutex);
INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_PLAYBACK].be_clients);
INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_CAPTURE].be_clients);
INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_PLAYBACK].fe_clients);
@@ -2395,6 +2394,7 @@ int snd_soc_register_card(struct snd_soc_card *card)
card->instantiated = 0;
mutex_init(&card->mutex);
mutex_init(&card->dapm_mutex);
+ mutex_init(&card->pcm_mutex);
spin_lock_init(&card->dpcm_lock);
return snd_soc_bind_card(card);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 77c986fe08d0..93bb4a347779 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -36,7 +36,7 @@
* Increments the active count for all the DAIs and components attached to a PCM
* runtime. Should typically be called when a stream is opened.
*
- * Must be called with the rtd->pcm_mutex being held
+ * Must be called with the rtd->card->pcm_mutex being held
*/
void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
{
@@ -44,7 +44,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
struct snd_soc_dai *codec_dai;
int i;
- lockdep_assert_held(&rtd->pcm_mutex);
+ lockdep_assert_held(&rtd->card->pcm_mutex);
if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
cpu_dai->playback_active++;
@@ -72,7 +72,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
* Decrements the active count for all the DAIs and components attached to a PCM
* runtime. Should typically be called when a stream is closed.
*
- * Must be called with the rtd->pcm_mutex being held
+ * Must be called with the rtd->card->pcm_mutex being held
*/
void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
{
@@ -80,7 +80,7 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
struct snd_soc_dai *codec_dai;
int i;
- lockdep_assert_held(&rtd->pcm_mutex);
+ lockdep_assert_held(&rtd->card->pcm_mutex);
if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
cpu_dai->playback_active--;
@@ -506,7 +506,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
pm_runtime_get_sync(component->dev);
}
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
/* startup the audio subsystem */
ret = snd_soc_dai_startup(cpu_dai, substream);
@@ -604,7 +604,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
snd_soc_runtime_activate(rtd, substream->stream);
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return 0;
config_err:
@@ -623,7 +623,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
snd_soc_dai_shutdown(cpu_dai, substream);
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -653,7 +653,7 @@ static void close_delayed_work(struct work_struct *work)
container_of(work, struct snd_soc_pcm_runtime, delayed_work.work);
struct snd_soc_dai *codec_dai = rtd->codec_dais[0];
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
dev_dbg(rtd->dev, "ASoC: pop wq checking: %s status: %s waiting: %s\n",
codec_dai->driver->playback.stream_name,
@@ -667,7 +667,7 @@ static void close_delayed_work(struct work_struct *work)
SND_SOC_DAPM_STREAM_STOP);
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
}
static void codec2codec_close_delayed_work(struct work_struct *work)
@@ -694,7 +694,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
struct snd_soc_dai *codec_dai;
int i;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
snd_soc_runtime_deactivate(rtd, substream->stream);
@@ -738,7 +738,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
SND_SOC_DAPM_STREAM_STOP);
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -771,7 +771,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
struct snd_soc_dai *codec_dai;
int i, ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (rtd->dai_link->ops->prepare) {
ret = rtd->dai_link->ops->prepare(substream);
@@ -826,7 +826,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
snd_soc_dai_digital_mute(cpu_dai, 0, substream->stream);
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -876,7 +876,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
struct snd_soc_dai *codec_dai;
int i, ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (rtd->dai_link->ops->hw_params) {
ret = rtd->dai_link->ops->hw_params(substream, params);
if (ret < 0) {
@@ -962,7 +962,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
if (ret)
goto component_err;
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
component_err:
@@ -986,7 +986,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
if (rtd->dai_link->ops->hw_free)
rtd->dai_link->ops->hw_free(substream);
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -1001,7 +1001,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
int i;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
/* clear the corresponding DAIs parameters when going to be inactive */
if (cpu_dai->active == 1) {
@@ -1043,7 +1043,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
snd_soc_dai_hw_free(cpu_dai, substream);
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return 0;
}
--
Peter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime
2019-08-13 10:45 [PATCH] ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime Peter Ujfalusi
@ 2019-08-14 18:14 ` Pierre-Louis Bossart
2019-08-15 7:07 ` Peter Ujfalusi
2019-08-15 17:14 ` Applied "ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime" to the asoc tree Mark Brown
1 sibling, 1 reply; 6+ messages in thread
From: Pierre-Louis Bossart @ 2019-08-14 18:14 UTC (permalink / raw)
To: Peter Ujfalusi, broonie, lgirdwood
Cc: alsa-devel, lars, ckeepax, kuninori.morimoto.gx
On 8/13/19 5:45 AM, Peter Ujfalusi wrote:
> The pcm_mutex is used to prevent concurrent execution of snd_pcm_ops
> callbacks. This works fine most of the cases but it can not handle setups
> when the same DAI is used by different rtd, for example:
> pcm3168a have two DAIs: one for Playback and one for Capture.
> If the codec is connected to a single CPU DAI we need to have two dai_link
> to support both playback and capture.
>
> In this case the snd_pcm_ops callbacks can be executed in parallel causing
> unexpected races in DAI drivers.
>
> By moving the pcm_mutex up to card level this can be solved
> while - hopefully - not breaking other setups.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
> Hi,
>
> I have tested the patch on several boards. it fixes the issue with single card
> with multiple dai_links where the CPU side (mcasp) is the same.
>
> However I can not test with anything which use DPCM. It would be great if the
> patch would gather few tested-by from folks having access to more complicated
> setups.
Actually you *can* test by submitting a PR for SOF, it'll trigger some
tests on Intel platforms using DPCM. It's not going to test anything
related to the compressed API but it's better than nothing.
I took this patch and created one PR as an example
https://github.com/thesofproject/linux/pull/1132
Will share results when I have them.
-Pierre
>
> The backround of this patch:
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-August/153864.html
>
> Regards,
> Peter
>
> include/sound/soc.h | 6 +++--
> sound/soc/soc-compress.c | 48 ++++++++++++++++++++--------------------
> sound/soc/soc-core.c | 2 +-
> sound/soc/soc-pcm.c | 36 +++++++++++++++---------------
> 4 files changed, 47 insertions(+), 45 deletions(-)
>
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 6ac6481b4882..ec2dbe0ac2aa 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -990,6 +990,10 @@ struct snd_soc_card {
> struct mutex mutex;
> struct mutex dapm_mutex;
>
> + /* Mutex for PCM operations */
> + struct mutex pcm_mutex;
> + enum snd_soc_pcm_subclass pcm_subclass;
> +
> spinlock_t dpcm_lock;
>
> bool instantiated;
> @@ -1107,8 +1111,6 @@ struct snd_soc_pcm_runtime {
> struct device *dev;
> struct snd_soc_card *card;
> struct snd_soc_dai_link *dai_link;
> - struct mutex pcm_mutex;
> - enum snd_soc_pcm_subclass pcm_subclass;
> struct snd_pcm_ops ops;
>
> unsigned int params_select; /* currently selected param for dai link */
> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
> index 289211069a1e..9e54d8ae6d2c 100644
> --- a/sound/soc/soc-compress.c
> +++ b/sound/soc/soc-compress.c
> @@ -80,7 +80,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> int ret;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> if (cpu_dai->driver->cops && cpu_dai->driver->cops->startup) {
> ret = cpu_dai->driver->cops->startup(cstream, cpu_dai);
> @@ -108,7 +108,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
>
> snd_soc_runtime_activate(rtd, cstream->direction);
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
>
> return 0;
>
> @@ -118,7 +118,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
> if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
> cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
> out:
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -224,7 +224,7 @@ static void close_delayed_work(struct work_struct *work)
> container_of(work, struct snd_soc_pcm_runtime, delayed_work.work);
> struct snd_soc_dai *codec_dai = rtd->codec_dai;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> dev_dbg(rtd->dev,
> "Compress ASoC: pop wq checking: %s status: %s waiting: %s\n",
> @@ -239,7 +239,7 @@ static void close_delayed_work(struct work_struct *work)
> SND_SOC_DAPM_STREAM_STOP);
> }
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> }
>
> static int soc_compr_free(struct snd_compr_stream *cstream)
> @@ -249,7 +249,7 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
> struct snd_soc_dai *codec_dai = rtd->codec_dai;
> int stream;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> if (cstream->direction == SND_COMPRESS_PLAYBACK)
> stream = SNDRV_PCM_STREAM_PLAYBACK;
> @@ -292,7 +292,7 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
> SND_SOC_DAPM_STREAM_STOP);
> }
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return 0;
> }
>
> @@ -375,7 +375,7 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> int ret;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> ret = soc_compr_components_trigger(cstream, cmd);
> if (ret < 0)
> @@ -394,7 +394,7 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
> }
>
> out:
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -480,7 +480,7 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> int ret;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> /*
> * First we call set_params for the CPU DAI, then the component
> @@ -514,14 +514,14 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
>
> /* cancel any delayed stream shutdown that is pending */
> rtd->pop_wait = 0;
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
>
> cancel_delayed_work_sync(&rtd->delayed_work);
>
> return 0;
>
> err:
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -593,7 +593,7 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> int ret = 0;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> if (cpu_dai->driver->cops && cpu_dai->driver->cops->get_params) {
> ret = cpu_dai->driver->cops->get_params(cstream, params, cpu_dai);
> @@ -613,7 +613,7 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
> }
>
> err:
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -625,7 +625,7 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
> struct snd_soc_rtdcom_list *rtdcom;
> int ret = 0;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> for_each_rtdcom(rtd, rtdcom) {
> component = rtdcom->component;
> @@ -638,7 +638,7 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
> break;
> }
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -650,7 +650,7 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
> struct snd_soc_rtdcom_list *rtdcom;
> int ret = 0;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> for_each_rtdcom(rtd, rtdcom) {
> component = rtdcom->component;
> @@ -664,7 +664,7 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
> break;
> }
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -676,7 +676,7 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> int ret = 0;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> if (cpu_dai->driver->cops && cpu_dai->driver->cops->ack) {
> ret = cpu_dai->driver->cops->ack(cstream, bytes, cpu_dai);
> @@ -697,7 +697,7 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
> }
>
> err:
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -710,7 +710,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
> int ret = 0;
> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> if (cpu_dai->driver->cops && cpu_dai->driver->cops->pointer)
> cpu_dai->driver->cops->pointer(cstream, tstamp, cpu_dai);
> @@ -726,7 +726,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
> break;
> }
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -738,7 +738,7 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
> struct snd_soc_rtdcom_list *rtdcom;
> int ret = 0;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> for_each_rtdcom(rtd, rtdcom) {
> component = rtdcom->component;
> @@ -751,7 +751,7 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
> break;
> }
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index bb1e9e2c4ff4..7f2064889a23 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1344,7 +1344,6 @@ static int soc_post_component_init(struct snd_soc_pcm_runtime *rtd,
> rtd->dev->groups = soc_dev_attr_groups;
> dev_set_name(rtd->dev, "%s", name);
> dev_set_drvdata(rtd->dev, rtd);
> - mutex_init(&rtd->pcm_mutex);
> INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_PLAYBACK].be_clients);
> INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_CAPTURE].be_clients);
> INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_PLAYBACK].fe_clients);
> @@ -2395,6 +2394,7 @@ int snd_soc_register_card(struct snd_soc_card *card)
> card->instantiated = 0;
> mutex_init(&card->mutex);
> mutex_init(&card->dapm_mutex);
> + mutex_init(&card->pcm_mutex);
> spin_lock_init(&card->dpcm_lock);
>
> return snd_soc_bind_card(card);
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 77c986fe08d0..93bb4a347779 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -36,7 +36,7 @@
> * Increments the active count for all the DAIs and components attached to a PCM
> * runtime. Should typically be called when a stream is opened.
> *
> - * Must be called with the rtd->pcm_mutex being held
> + * Must be called with the rtd->card->pcm_mutex being held
> */
> void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
> {
> @@ -44,7 +44,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
> struct snd_soc_dai *codec_dai;
> int i;
>
> - lockdep_assert_held(&rtd->pcm_mutex);
> + lockdep_assert_held(&rtd->card->pcm_mutex);
>
> if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> cpu_dai->playback_active++;
> @@ -72,7 +72,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
> * Decrements the active count for all the DAIs and components attached to a PCM
> * runtime. Should typically be called when a stream is closed.
> *
> - * Must be called with the rtd->pcm_mutex being held
> + * Must be called with the rtd->card->pcm_mutex being held
> */
> void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
> {
> @@ -80,7 +80,7 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
> struct snd_soc_dai *codec_dai;
> int i;
>
> - lockdep_assert_held(&rtd->pcm_mutex);
> + lockdep_assert_held(&rtd->card->pcm_mutex);
>
> if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> cpu_dai->playback_active--;
> @@ -506,7 +506,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
> pm_runtime_get_sync(component->dev);
> }
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> /* startup the audio subsystem */
> ret = snd_soc_dai_startup(cpu_dai, substream);
> @@ -604,7 +604,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
>
> snd_soc_runtime_activate(rtd, substream->stream);
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return 0;
>
> config_err:
> @@ -623,7 +623,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
>
> snd_soc_dai_shutdown(cpu_dai, substream);
> out:
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
>
> for_each_rtdcom(rtd, rtdcom) {
> component = rtdcom->component;
> @@ -653,7 +653,7 @@ static void close_delayed_work(struct work_struct *work)
> container_of(work, struct snd_soc_pcm_runtime, delayed_work.work);
> struct snd_soc_dai *codec_dai = rtd->codec_dais[0];
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> dev_dbg(rtd->dev, "ASoC: pop wq checking: %s status: %s waiting: %s\n",
> codec_dai->driver->playback.stream_name,
> @@ -667,7 +667,7 @@ static void close_delayed_work(struct work_struct *work)
> SND_SOC_DAPM_STREAM_STOP);
> }
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> }
>
> static void codec2codec_close_delayed_work(struct work_struct *work)
> @@ -694,7 +694,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
> struct snd_soc_dai *codec_dai;
> int i;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> snd_soc_runtime_deactivate(rtd, substream->stream);
>
> @@ -738,7 +738,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
> SND_SOC_DAPM_STREAM_STOP);
> }
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
>
> for_each_rtdcom(rtd, rtdcom) {
> component = rtdcom->component;
> @@ -771,7 +771,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
> struct snd_soc_dai *codec_dai;
> int i, ret = 0;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> if (rtd->dai_link->ops->prepare) {
> ret = rtd->dai_link->ops->prepare(substream);
> @@ -826,7 +826,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
> snd_soc_dai_digital_mute(cpu_dai, 0, substream->stream);
>
> out:
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -876,7 +876,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> struct snd_soc_dai *codec_dai;
> int i, ret = 0;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
> if (rtd->dai_link->ops->hw_params) {
> ret = rtd->dai_link->ops->hw_params(substream, params);
> if (ret < 0) {
> @@ -962,7 +962,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> if (ret)
> goto component_err;
> out:
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
>
> component_err:
> @@ -986,7 +986,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> if (rtd->dai_link->ops->hw_free)
> rtd->dai_link->ops->hw_free(substream);
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return ret;
> }
>
> @@ -1001,7 +1001,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
> bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> int i;
>
> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>
> /* clear the corresponding DAIs parameters when going to be inactive */
> if (cpu_dai->active == 1) {
> @@ -1043,7 +1043,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
>
> snd_soc_dai_hw_free(cpu_dai, substream);
>
> - mutex_unlock(&rtd->pcm_mutex);
> + mutex_unlock(&rtd->card->pcm_mutex);
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime
2019-08-14 18:14 ` Pierre-Louis Bossart
@ 2019-08-15 7:07 ` Peter Ujfalusi
2019-08-15 13:56 ` Pierre-Louis Bossart
0 siblings, 1 reply; 6+ messages in thread
From: Peter Ujfalusi @ 2019-08-15 7:07 UTC (permalink / raw)
To: Pierre-Louis Bossart, broonie, lgirdwood
Cc: alsa-devel, lars, ckeepax, kuninori.morimoto.gx
On 14/08/2019 21.14, Pierre-Louis Bossart wrote:
>
>
> On 8/13/19 5:45 AM, Peter Ujfalusi wrote:
>> The pcm_mutex is used to prevent concurrent execution of snd_pcm_ops
>> callbacks. This works fine most of the cases but it can not handle setups
>> when the same DAI is used by different rtd, for example:
>> pcm3168a have two DAIs: one for Playback and one for Capture.
>> If the codec is connected to a single CPU DAI we need to have two
>> dai_link
>> to support both playback and capture.
>>
>> In this case the snd_pcm_ops callbacks can be executed in parallel
>> causing
>> unexpected races in DAI drivers.
>>
>> By moving the pcm_mutex up to card level this can be solved
>> while - hopefully - not breaking other setups.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>> Hi,
>>
>> I have tested the patch on several boards. it fixes the issue with
>> single card
>> with multiple dai_links where the CPU side (mcasp) is the same.
>>
>> However I can not test with anything which use DPCM. It would be great
>> if the
>> patch would gather few tested-by from folks having access to more
>> complicated
>> setups.
>
> Actually you *can* test by submitting a PR for SOF, it'll trigger some
> tests on Intel platforms using DPCM. It's not going to test anything
> related to the compressed API but it's better than nothing.
Good to know and thanks.
I would not thought of abusing the SOF project to run tests, not that I
know how to trigger the right tests ;)
>
> I took this patch and created one PR as an example
> https://github.com/thesofproject/linux/pull/1132
Should I be worried because of your comment there saying 'I have no idea
why the BYT_NOCODEC mode fails, there's no information provided.' ?
>
> Will share results when I have them.
Thank you!
> -Pierre
>
>
>>
>> The backround of this patch:
>> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-August/153864.html
>>
>>
>> Regards,
>> Peter
>>
>> include/sound/soc.h | 6 +++--
>> sound/soc/soc-compress.c | 48 ++++++++++++++++++++--------------------
>> sound/soc/soc-core.c | 2 +-
>> sound/soc/soc-pcm.c | 36 +++++++++++++++---------------
>> 4 files changed, 47 insertions(+), 45 deletions(-)
>>
>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>> index 6ac6481b4882..ec2dbe0ac2aa 100644
>> --- a/include/sound/soc.h
>> +++ b/include/sound/soc.h
>> @@ -990,6 +990,10 @@ struct snd_soc_card {
>> struct mutex mutex;
>> struct mutex dapm_mutex;
>> + /* Mutex for PCM operations */
>> + struct mutex pcm_mutex;
>> + enum snd_soc_pcm_subclass pcm_subclass;
>> +
>> spinlock_t dpcm_lock;
>> bool instantiated;
>> @@ -1107,8 +1111,6 @@ struct snd_soc_pcm_runtime {
>> struct device *dev;
>> struct snd_soc_card *card;
>> struct snd_soc_dai_link *dai_link;
>> - struct mutex pcm_mutex;
>> - enum snd_soc_pcm_subclass pcm_subclass;
>> struct snd_pcm_ops ops;
>> unsigned int params_select; /* currently selected param for
>> dai link */
>> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
>> index 289211069a1e..9e54d8ae6d2c 100644
>> --- a/sound/soc/soc-compress.c
>> +++ b/sound/soc/soc-compress.c
>> @@ -80,7 +80,7 @@ static int soc_compr_open(struct snd_compr_stream
>> *cstream)
>> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> int ret;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> if (cpu_dai->driver->cops && cpu_dai->driver->cops->startup) {
>> ret = cpu_dai->driver->cops->startup(cstream, cpu_dai);
>> @@ -108,7 +108,7 @@ static int soc_compr_open(struct snd_compr_stream
>> *cstream)
>> snd_soc_runtime_activate(rtd, cstream->direction);
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return 0;
>> @@ -118,7 +118,7 @@ static int soc_compr_open(struct
>> snd_compr_stream *cstream)
>> if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
>> cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
>> out:
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -224,7 +224,7 @@ static void close_delayed_work(struct
>> work_struct *work)
>> container_of(work, struct snd_soc_pcm_runtime,
>> delayed_work.work);
>> struct snd_soc_dai *codec_dai = rtd->codec_dai;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> dev_dbg(rtd->dev,
>> "Compress ASoC: pop wq checking: %s status: %s waiting: %s\n",
>> @@ -239,7 +239,7 @@ static void close_delayed_work(struct work_struct
>> *work)
>> SND_SOC_DAPM_STREAM_STOP);
>> }
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> }
>> static int soc_compr_free(struct snd_compr_stream *cstream)
>> @@ -249,7 +249,7 @@ static int soc_compr_free(struct snd_compr_stream
>> *cstream)
>> struct snd_soc_dai *codec_dai = rtd->codec_dai;
>> int stream;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> if (cstream->direction == SND_COMPRESS_PLAYBACK)
>> stream = SNDRV_PCM_STREAM_PLAYBACK;
>> @@ -292,7 +292,7 @@ static int soc_compr_free(struct snd_compr_stream
>> *cstream)
>> SND_SOC_DAPM_STREAM_STOP);
>> }
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return 0;
>> }
>> @@ -375,7 +375,7 @@ static int soc_compr_trigger(struct
>> snd_compr_stream *cstream, int cmd)
>> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> int ret;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> ret = soc_compr_components_trigger(cstream, cmd);
>> if (ret < 0)
>> @@ -394,7 +394,7 @@ static int soc_compr_trigger(struct
>> snd_compr_stream *cstream, int cmd)
>> }
>> out:
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -480,7 +480,7 @@ static int soc_compr_set_params(struct
>> snd_compr_stream *cstream,
>> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> int ret;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> /*
>> * First we call set_params for the CPU DAI, then the component
>> @@ -514,14 +514,14 @@ static int soc_compr_set_params(struct
>> snd_compr_stream *cstream,
>> /* cancel any delayed stream shutdown that is pending */
>> rtd->pop_wait = 0;
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> cancel_delayed_work_sync(&rtd->delayed_work);
>> return 0;
>> err:
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -593,7 +593,7 @@ static int soc_compr_get_params(struct
>> snd_compr_stream *cstream,
>> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> int ret = 0;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> if (cpu_dai->driver->cops && cpu_dai->driver->cops->get_params) {
>> ret = cpu_dai->driver->cops->get_params(cstream, params,
>> cpu_dai);
>> @@ -613,7 +613,7 @@ static int soc_compr_get_params(struct
>> snd_compr_stream *cstream,
>> }
>> err:
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -625,7 +625,7 @@ static int soc_compr_get_caps(struct
>> snd_compr_stream *cstream,
>> struct snd_soc_rtdcom_list *rtdcom;
>> int ret = 0;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> for_each_rtdcom(rtd, rtdcom) {
>> component = rtdcom->component;
>> @@ -638,7 +638,7 @@ static int soc_compr_get_caps(struct
>> snd_compr_stream *cstream,
>> break;
>> }
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -650,7 +650,7 @@ static int soc_compr_get_codec_caps(struct
>> snd_compr_stream *cstream,
>> struct snd_soc_rtdcom_list *rtdcom;
>> int ret = 0;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> for_each_rtdcom(rtd, rtdcom) {
>> component = rtdcom->component;
>> @@ -664,7 +664,7 @@ static int soc_compr_get_codec_caps(struct
>> snd_compr_stream *cstream,
>> break;
>> }
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -676,7 +676,7 @@ static int soc_compr_ack(struct snd_compr_stream
>> *cstream, size_t bytes)
>> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> int ret = 0;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> if (cpu_dai->driver->cops && cpu_dai->driver->cops->ack) {
>> ret = cpu_dai->driver->cops->ack(cstream, bytes, cpu_dai);
>> @@ -697,7 +697,7 @@ static int soc_compr_ack(struct snd_compr_stream
>> *cstream, size_t bytes)
>> }
>> err:
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -710,7 +710,7 @@ static int soc_compr_pointer(struct
>> snd_compr_stream *cstream,
>> int ret = 0;
>> struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> if (cpu_dai->driver->cops && cpu_dai->driver->cops->pointer)
>> cpu_dai->driver->cops->pointer(cstream, tstamp, cpu_dai);
>> @@ -726,7 +726,7 @@ static int soc_compr_pointer(struct
>> snd_compr_stream *cstream,
>> break;
>> }
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -738,7 +738,7 @@ static int soc_compr_copy(struct
>> snd_compr_stream *cstream,
>> struct snd_soc_rtdcom_list *rtdcom;
>> int ret = 0;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> for_each_rtdcom(rtd, rtdcom) {
>> component = rtdcom->component;
>> @@ -751,7 +751,7 @@ static int soc_compr_copy(struct snd_compr_stream
>> *cstream,
>> break;
>> }
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>> index bb1e9e2c4ff4..7f2064889a23 100644
>> --- a/sound/soc/soc-core.c
>> +++ b/sound/soc/soc-core.c
>> @@ -1344,7 +1344,6 @@ static int soc_post_component_init(struct
>> snd_soc_pcm_runtime *rtd,
>> rtd->dev->groups = soc_dev_attr_groups;
>> dev_set_name(rtd->dev, "%s", name);
>> dev_set_drvdata(rtd->dev, rtd);
>> - mutex_init(&rtd->pcm_mutex);
>> INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_PLAYBACK].be_clients);
>> INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_CAPTURE].be_clients);
>> INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_PLAYBACK].fe_clients);
>> @@ -2395,6 +2394,7 @@ int snd_soc_register_card(struct snd_soc_card
>> *card)
>> card->instantiated = 0;
>> mutex_init(&card->mutex);
>> mutex_init(&card->dapm_mutex);
>> + mutex_init(&card->pcm_mutex);
>> spin_lock_init(&card->dpcm_lock);
>> return snd_soc_bind_card(card);
>> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
>> index 77c986fe08d0..93bb4a347779 100644
>> --- a/sound/soc/soc-pcm.c
>> +++ b/sound/soc/soc-pcm.c
>> @@ -36,7 +36,7 @@
>> * Increments the active count for all the DAIs and components
>> attached to a PCM
>> * runtime. Should typically be called when a stream is opened.
>> *
>> - * Must be called with the rtd->pcm_mutex being held
>> + * Must be called with the rtd->card->pcm_mutex being held
>> */
>> void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int
>> stream)
>> {
>> @@ -44,7 +44,7 @@ void snd_soc_runtime_activate(struct
>> snd_soc_pcm_runtime *rtd, int stream)
>> struct snd_soc_dai *codec_dai;
>> int i;
>> - lockdep_assert_held(&rtd->pcm_mutex);
>> + lockdep_assert_held(&rtd->card->pcm_mutex);
>> if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> cpu_dai->playback_active++;
>> @@ -72,7 +72,7 @@ void snd_soc_runtime_activate(struct
>> snd_soc_pcm_runtime *rtd, int stream)
>> * Decrements the active count for all the DAIs and components
>> attached to a PCM
>> * runtime. Should typically be called when a stream is closed.
>> *
>> - * Must be called with the rtd->pcm_mutex being held
>> + * Must be called with the rtd->card->pcm_mutex being held
>> */
>> void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int
>> stream)
>> {
>> @@ -80,7 +80,7 @@ void snd_soc_runtime_deactivate(struct
>> snd_soc_pcm_runtime *rtd, int stream)
>> struct snd_soc_dai *codec_dai;
>> int i;
>> - lockdep_assert_held(&rtd->pcm_mutex);
>> + lockdep_assert_held(&rtd->card->pcm_mutex);
>> if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> cpu_dai->playback_active--;
>> @@ -506,7 +506,7 @@ static int soc_pcm_open(struct snd_pcm_substream
>> *substream)
>> pm_runtime_get_sync(component->dev);
>> }
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> /* startup the audio subsystem */
>> ret = snd_soc_dai_startup(cpu_dai, substream);
>> @@ -604,7 +604,7 @@ static int soc_pcm_open(struct snd_pcm_substream
>> *substream)
>> snd_soc_runtime_activate(rtd, substream->stream);
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return 0;
>> config_err:
>> @@ -623,7 +623,7 @@ static int soc_pcm_open(struct snd_pcm_substream
>> *substream)
>> snd_soc_dai_shutdown(cpu_dai, substream);
>> out:
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> for_each_rtdcom(rtd, rtdcom) {
>> component = rtdcom->component;
>> @@ -653,7 +653,7 @@ static void close_delayed_work(struct work_struct
>> *work)
>> container_of(work, struct snd_soc_pcm_runtime,
>> delayed_work.work);
>> struct snd_soc_dai *codec_dai = rtd->codec_dais[0];
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> dev_dbg(rtd->dev, "ASoC: pop wq checking: %s status: %s
>> waiting: %s\n",
>> codec_dai->driver->playback.stream_name,
>> @@ -667,7 +667,7 @@ static void close_delayed_work(struct work_struct
>> *work)
>> SND_SOC_DAPM_STREAM_STOP);
>> }
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> }
>> static void codec2codec_close_delayed_work(struct work_struct *work)
>> @@ -694,7 +694,7 @@ static int soc_pcm_close(struct snd_pcm_substream
>> *substream)
>> struct snd_soc_dai *codec_dai;
>> int i;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> snd_soc_runtime_deactivate(rtd, substream->stream);
>> @@ -738,7 +738,7 @@ static int soc_pcm_close(struct
>> snd_pcm_substream *substream)
>> SND_SOC_DAPM_STREAM_STOP);
>> }
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> for_each_rtdcom(rtd, rtdcom) {
>> component = rtdcom->component;
>> @@ -771,7 +771,7 @@ static int soc_pcm_prepare(struct
>> snd_pcm_substream *substream)
>> struct snd_soc_dai *codec_dai;
>> int i, ret = 0;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> if (rtd->dai_link->ops->prepare) {
>> ret = rtd->dai_link->ops->prepare(substream);
>> @@ -826,7 +826,7 @@ static int soc_pcm_prepare(struct
>> snd_pcm_substream *substream)
>> snd_soc_dai_digital_mute(cpu_dai, 0, substream->stream);
>> out:
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -876,7 +876,7 @@ static int soc_pcm_hw_params(struct
>> snd_pcm_substream *substream,
>> struct snd_soc_dai *codec_dai;
>> int i, ret = 0;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> if (rtd->dai_link->ops->hw_params) {
>> ret = rtd->dai_link->ops->hw_params(substream, params);
>> if (ret < 0) {
>> @@ -962,7 +962,7 @@ static int soc_pcm_hw_params(struct
>> snd_pcm_substream *substream,
>> if (ret)
>> goto component_err;
>> out:
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> component_err:
>> @@ -986,7 +986,7 @@ static int soc_pcm_hw_params(struct
>> snd_pcm_substream *substream,
>> if (rtd->dai_link->ops->hw_free)
>> rtd->dai_link->ops->hw_free(substream);
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return ret;
>> }
>> @@ -1001,7 +1001,7 @@ static int soc_pcm_hw_free(struct
>> snd_pcm_substream *substream)
>> bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
>> int i;
>> - mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>> + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
>> /* clear the corresponding DAIs parameters when going to be
>> inactive */
>> if (cpu_dai->active == 1) {
>> @@ -1043,7 +1043,7 @@ static int soc_pcm_hw_free(struct
>> snd_pcm_substream *substream)
>> snd_soc_dai_hw_free(cpu_dai, substream);
>> - mutex_unlock(&rtd->pcm_mutex);
>> + mutex_unlock(&rtd->card->pcm_mutex);
>> return 0;
>> }
>>
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime
2019-08-15 7:07 ` Peter Ujfalusi
@ 2019-08-15 13:56 ` Pierre-Louis Bossart
2019-08-16 10:49 ` Peter Ujfalusi
0 siblings, 1 reply; 6+ messages in thread
From: Pierre-Louis Bossart @ 2019-08-15 13:56 UTC (permalink / raw)
To: Peter Ujfalusi, broonie, lgirdwood
Cc: alsa-devel, lars, ckeepax, kuninori.morimoto.gx
On 8/15/19 2:07 AM, Peter Ujfalusi wrote:
>
>
> On 14/08/2019 21.14, Pierre-Louis Bossart wrote:
>>
>>
>> On 8/13/19 5:45 AM, Peter Ujfalusi wrote:
>>> The pcm_mutex is used to prevent concurrent execution of snd_pcm_ops
>>> callbacks. This works fine most of the cases but it can not handle setups
>>> when the same DAI is used by different rtd, for example:
>>> pcm3168a have two DAIs: one for Playback and one for Capture.
>>> If the codec is connected to a single CPU DAI we need to have two
>>> dai_link
>>> to support both playback and capture.
>>>
>>> In this case the snd_pcm_ops callbacks can be executed in parallel
>>> causing
>>> unexpected races in DAI drivers.
>>>
>>> By moving the pcm_mutex up to card level this can be solved
>>> while - hopefully - not breaking other setups.
>>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>> Hi,
>>>
>>> I have tested the patch on several boards. it fixes the issue with
>>> single card
>>> with multiple dai_links where the CPU side (mcasp) is the same.
>>>
>>> However I can not test with anything which use DPCM. It would be great
>>> if the
>>> patch would gather few tested-by from folks having access to more
>>> complicated
>>> setups.
>>
>> Actually you *can* test by submitting a PR for SOF, it'll trigger some
>> tests on Intel platforms using DPCM. It's not going to test anything
>> related to the compressed API but it's better than nothing.
>
> Good to know and thanks.
> I would not thought of abusing the SOF project to run tests, not that I
> know how to trigger the right tests ;)
well we will run those tests anyways when we do the weekly merges, and
when in doubt it might be more productive for an ALSA contributor to run
the tests ahead of time than report that things are broken after the
merge (as seen 2 weeks in a row with the module_put and compilation issues)
>
>>
>> I took this patch and created one PR as an example
>> https://github.com/thesofproject/linux/pull/1132
>
> Should I be worried because of your comment there saying 'I have no idea
> why the BYT_NOCODEC mode fails, there's no information provided.' ?
Redoing the tests removes that error, so this looks fine. Must have been
a CI glitch
>> Will share results when I have them.
So far no issues. so FWIW since it's just 1st order automatic testing
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime
2019-08-15 13:56 ` Pierre-Louis Bossart
@ 2019-08-16 10:49 ` Peter Ujfalusi
0 siblings, 0 replies; 6+ messages in thread
From: Peter Ujfalusi @ 2019-08-16 10:49 UTC (permalink / raw)
To: Pierre-Louis Bossart, broonie, lgirdwood
Cc: alsa-devel, lars, ckeepax, kuninori.morimoto.gx
On 15/08/2019 16.56, Pierre-Louis Bossart wrote:
>>> Actually you *can* test by submitting a PR for SOF, it'll trigger some
>>> tests on Intel platforms using DPCM. It's not going to test anything
>>> related to the compressed API but it's better than nothing.
>>
>> Good to know and thanks.
>> I would not thought of abusing the SOF project to run tests, not that I
>> know how to trigger the right tests ;)
>
> well we will run those tests anyways when we do the weekly merges, and
> when in doubt it might be more productive for an ALSA contributor to run
> the tests ahead of time than report that things are broken after the
> merge (as seen 2 weeks in a row with the module_put and compilation issues)
I'll keep in mind, do you have some documentation on the tests?
>>> I took this patch and created one PR as an example
>>> https://github.com/thesofproject/linux/pull/1132
>>
>> Should I be worried because of your comment there saying 'I have no idea
>> why the BYT_NOCODEC mode fails, there's no information provided.' ?
>
> Redoing the tests removes that error, so this looks fine. Must have been
> a CI glitch
Hrm, did it had the same failure w/o my patch? Reordering tests might
just hide a bug somewhere...
>>> Will share results when I have them.
>
> So far no issues. so FWIW since it's just 1st order automatic testing
>
> Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Thank you!
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Applied "ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime" to the asoc tree
2019-08-13 10:45 [PATCH] ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime Peter Ujfalusi
2019-08-14 18:14 ` Pierre-Louis Bossart
@ 2019-08-15 17:14 ` Mark Brown
1 sibling, 0 replies; 6+ messages in thread
From: Mark Brown @ 2019-08-15 17:14 UTC (permalink / raw)
To: Peter Ujfalusi
Cc: alsa-devel, lars, ckeepax, kuninori.morimoto.gx, lgirdwood,
Pierre-Louis Bossart, Mark Brown
The patch
ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime
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 72b745e3ad65deac94ea4eb83262c52ba3ffdb5b Mon Sep 17 00:00:00 2001
From: Peter Ujfalusi <peter.ujfalusi@ti.com>
Date: Tue, 13 Aug 2019 13:45:32 +0300
Subject: [PATCH] ASoC: core: Move pcm_mutex up to card level from
snd_soc_pcm_runtime
The pcm_mutex is used to prevent concurrent execution of snd_pcm_ops
callbacks. This works fine most of the cases but it can not handle setups
when the same DAI is used by different rtd, for example:
pcm3168a have two DAIs: one for Playback and one for Capture.
If the codec is connected to a single CPU DAI we need to have two dai_link
to support both playback and capture.
In this case the snd_pcm_ops callbacks can be executed in parallel causing
unexpected races in DAI drivers.
By moving the pcm_mutex up to card level this can be solved
while - hopefully - not breaking other setups.
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Link: https://lore.kernel.org/r/20190813104532.16669-1-peter.ujfalusi@ti.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
include/sound/soc.h | 6 +++--
sound/soc/soc-compress.c | 48 ++++++++++++++++++++--------------------
sound/soc/soc-core.c | 2 +-
sound/soc/soc-pcm.c | 36 +++++++++++++++---------------
4 files changed, 47 insertions(+), 45 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index b1fe5ebea257..5c841c2ee814 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -988,6 +988,10 @@ struct snd_soc_card {
struct mutex mutex;
struct mutex dapm_mutex;
+ /* Mutex for PCM operations */
+ struct mutex pcm_mutex;
+ enum snd_soc_pcm_subclass pcm_subclass;
+
spinlock_t dpcm_lock;
bool instantiated;
@@ -1116,8 +1120,6 @@ struct snd_soc_pcm_runtime {
struct device *dev;
struct snd_soc_card *card;
struct snd_soc_dai_link *dai_link;
- struct mutex pcm_mutex;
- enum snd_soc_pcm_subclass pcm_subclass;
struct snd_pcm_ops ops;
unsigned int params_select; /* currently selected param for dai link */
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 289211069a1e..9e54d8ae6d2c 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -80,7 +80,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cpu_dai->driver->cops && cpu_dai->driver->cops->startup) {
ret = cpu_dai->driver->cops->startup(cstream, cpu_dai);
@@ -108,7 +108,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
snd_soc_runtime_activate(rtd, cstream->direction);
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return 0;
@@ -118,7 +118,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream)
if (cpu_dai->driver->cops && cpu_dai->driver->cops->shutdown)
cpu_dai->driver->cops->shutdown(cstream, cpu_dai);
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -224,7 +224,7 @@ static void close_delayed_work(struct work_struct *work)
container_of(work, struct snd_soc_pcm_runtime, delayed_work.work);
struct snd_soc_dai *codec_dai = rtd->codec_dai;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
dev_dbg(rtd->dev,
"Compress ASoC: pop wq checking: %s status: %s waiting: %s\n",
@@ -239,7 +239,7 @@ static void close_delayed_work(struct work_struct *work)
SND_SOC_DAPM_STREAM_STOP);
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
}
static int soc_compr_free(struct snd_compr_stream *cstream)
@@ -249,7 +249,7 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
struct snd_soc_dai *codec_dai = rtd->codec_dai;
int stream;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cstream->direction == SND_COMPRESS_PLAYBACK)
stream = SNDRV_PCM_STREAM_PLAYBACK;
@@ -292,7 +292,7 @@ static int soc_compr_free(struct snd_compr_stream *cstream)
SND_SOC_DAPM_STREAM_STOP);
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return 0;
}
@@ -375,7 +375,7 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
ret = soc_compr_components_trigger(cstream, cmd);
if (ret < 0)
@@ -394,7 +394,7 @@ static int soc_compr_trigger(struct snd_compr_stream *cstream, int cmd)
}
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -480,7 +480,7 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
/*
* First we call set_params for the CPU DAI, then the component
@@ -514,14 +514,14 @@ static int soc_compr_set_params(struct snd_compr_stream *cstream,
/* cancel any delayed stream shutdown that is pending */
rtd->pop_wait = 0;
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
cancel_delayed_work_sync(&rtd->delayed_work);
return 0;
err:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -593,7 +593,7 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cpu_dai->driver->cops && cpu_dai->driver->cops->get_params) {
ret = cpu_dai->driver->cops->get_params(cstream, params, cpu_dai);
@@ -613,7 +613,7 @@ static int soc_compr_get_params(struct snd_compr_stream *cstream,
}
err:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -625,7 +625,7 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
struct snd_soc_rtdcom_list *rtdcom;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -638,7 +638,7 @@ static int soc_compr_get_caps(struct snd_compr_stream *cstream,
break;
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -650,7 +650,7 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
struct snd_soc_rtdcom_list *rtdcom;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -664,7 +664,7 @@ static int soc_compr_get_codec_caps(struct snd_compr_stream *cstream,
break;
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -676,7 +676,7 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cpu_dai->driver->cops && cpu_dai->driver->cops->ack) {
ret = cpu_dai->driver->cops->ack(cstream, bytes, cpu_dai);
@@ -697,7 +697,7 @@ static int soc_compr_ack(struct snd_compr_stream *cstream, size_t bytes)
}
err:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -710,7 +710,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
int ret = 0;
struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (cpu_dai->driver->cops && cpu_dai->driver->cops->pointer)
cpu_dai->driver->cops->pointer(cstream, tstamp, cpu_dai);
@@ -726,7 +726,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
break;
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -738,7 +738,7 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
struct snd_soc_rtdcom_list *rtdcom;
int ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -751,7 +751,7 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
break;
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index abe2f47cee6e..b3f820fb53e6 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1360,7 +1360,6 @@ static int soc_post_component_init(struct snd_soc_pcm_runtime *rtd,
rtd->dev->groups = soc_dev_attr_groups;
dev_set_name(rtd->dev, "%s", name);
dev_set_drvdata(rtd->dev, rtd);
- mutex_init(&rtd->pcm_mutex);
INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_PLAYBACK].be_clients);
INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_CAPTURE].be_clients);
INIT_LIST_HEAD(&rtd->dpcm[SNDRV_PCM_STREAM_PLAYBACK].fe_clients);
@@ -2383,6 +2382,7 @@ int snd_soc_register_card(struct snd_soc_card *card)
card->instantiated = 0;
mutex_init(&card->mutex);
mutex_init(&card->dapm_mutex);
+ mutex_init(&card->pcm_mutex);
spin_lock_init(&card->dpcm_lock);
return snd_soc_bind_card(card);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index da657c8179cc..e163dde5eab1 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -36,7 +36,7 @@
* Increments the active count for all the DAIs and components attached to a PCM
* runtime. Should typically be called when a stream is opened.
*
- * Must be called with the rtd->pcm_mutex being held
+ * Must be called with the rtd->card->pcm_mutex being held
*/
void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
{
@@ -44,7 +44,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
struct snd_soc_dai *codec_dai;
int i;
- lockdep_assert_held(&rtd->pcm_mutex);
+ lockdep_assert_held(&rtd->card->pcm_mutex);
if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
cpu_dai->playback_active++;
@@ -72,7 +72,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream)
* Decrements the active count for all the DAIs and components attached to a PCM
* runtime. Should typically be called when a stream is closed.
*
- * Must be called with the rtd->pcm_mutex being held
+ * Must be called with the rtd->card->pcm_mutex being held
*/
void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
{
@@ -80,7 +80,7 @@ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream)
struct snd_soc_dai *codec_dai;
int i;
- lockdep_assert_held(&rtd->pcm_mutex);
+ lockdep_assert_held(&rtd->card->pcm_mutex);
if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
cpu_dai->playback_active--;
@@ -506,7 +506,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
pm_runtime_get_sync(component->dev);
}
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
/* startup the audio subsystem */
ret = snd_soc_dai_startup(cpu_dai, substream);
@@ -604,7 +604,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
snd_soc_runtime_activate(rtd, substream->stream);
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return 0;
config_err:
@@ -623,7 +623,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
snd_soc_dai_shutdown(cpu_dai, substream);
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -653,7 +653,7 @@ static void close_delayed_work(struct work_struct *work)
container_of(work, struct snd_soc_pcm_runtime, delayed_work.work);
struct snd_soc_dai *codec_dai = rtd->codec_dais[0];
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
dev_dbg(rtd->dev, "ASoC: pop wq checking: %s status: %s waiting: %s\n",
codec_dai->driver->playback.stream_name,
@@ -667,7 +667,7 @@ static void close_delayed_work(struct work_struct *work)
SND_SOC_DAPM_STREAM_STOP);
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
}
static void codec2codec_close_delayed_work(struct work_struct *work)
@@ -694,7 +694,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
struct snd_soc_dai *codec_dai;
int i;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
snd_soc_runtime_deactivate(rtd, substream->stream);
@@ -738,7 +738,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
SND_SOC_DAPM_STREAM_STOP);
}
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
for_each_rtdcom(rtd, rtdcom) {
component = rtdcom->component;
@@ -771,7 +771,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
struct snd_soc_dai *codec_dai;
int i, ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (rtd->dai_link->ops->prepare) {
ret = rtd->dai_link->ops->prepare(substream);
@@ -826,7 +826,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
snd_soc_dai_digital_mute(cpu_dai, 0, substream->stream);
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -876,7 +876,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
struct snd_soc_dai *codec_dai;
int i, ret = 0;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
if (rtd->dai_link->ops->hw_params) {
ret = rtd->dai_link->ops->hw_params(substream, params);
if (ret < 0) {
@@ -962,7 +962,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
if (ret)
goto component_err;
out:
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
component_err:
@@ -986,7 +986,7 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
if (rtd->dai_link->ops->hw_free)
rtd->dai_link->ops->hw_free(substream);
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return ret;
}
@@ -1001,7 +1001,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
int i;
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+ mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
/* clear the corresponding DAIs parameters when going to be inactive */
if (cpu_dai->active == 1) {
@@ -1043,7 +1043,7 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
snd_soc_dai_hw_free(cpu_dai, substream);
- mutex_unlock(&rtd->pcm_mutex);
+ mutex_unlock(&rtd->card->pcm_mutex);
return 0;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-16 10:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-13 10:45 [PATCH] ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime Peter Ujfalusi
2019-08-14 18:14 ` Pierre-Louis Bossart
2019-08-15 7:07 ` Peter Ujfalusi
2019-08-15 13:56 ` Pierre-Louis Bossart
2019-08-16 10:49 ` Peter Ujfalusi
2019-08-15 17:14 ` Applied "ASoC: core: Move pcm_mutex up to card level from snd_soc_pcm_runtime" to the asoc tree Mark Brown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.