[alsa-devel] [Question about DPCM] dpcm_run_new_update races again xrun
Takashi Iwai
tiwai at suse.de
Mon Nov 3 17:54:10 CET 2014
At Mon, 03 Nov 2014 15:42:16 +0100,
Takashi Iwai wrote:
>
> At Mon, 03 Nov 2014 13:32:04 +0000,
> Liam Girdwood wrote:
> >
> > On Mon, 2014-11-03 at 03:28 -0800, Qiao Zhou wrote:
> > > Hi Mark, Liam
> > >
> > > I met one BE not-function issue when developing DPCM feature, and found
> > > dpcm_run_new_update is not protected well against xrun(interrupt context).
> > > Could you help to give some suggestions?
> > >
> >
> > I'm wondering if this would be better solved by improving the locking so
> > that an XRUN could not run at the same time as the runtime update. Both
> > functions are async, but are only protected by a mutex atm (like the
> > rest of PCM ops except trigger which is atomic). We maybe need to add a
> > spinlock to both runtime_update() and dpcm_fe_dai_trigger()
>
> Be careful, you cannot take spinlock while hw_params, for example.
>
> Why do you need to set runtime_update to NO in the trigger callback in
> the first place? The trigger may influence on the FE streaming state,
> but not on the FE/BE connection state, right?
Thinking of this again, this doesn't look like a good suggestion,
either.
As mentioned, we can't take a lock for the whole lengthy operation
like prepare or hw_params. So, instead of taking a lock, the trigger
should be delayed when some operation is being performed. (I thought
at first just performing FE's trigger and delaying BE later, but FE/BE
trigger can be more complex than that, so it ends up with the whole
delayed trigger.)
The patch below is a quick hack (only compile tested!). It uses PCM's
stream lock for protecting against the race for the state transition,
and the trigger callback checks the state, aborts immediately if there
is a concurrent operation. At the exit of state change, it invokes
the delayed trigger.
Let me know if this really works as imagined. Then I'll cook up a
proper patch.
Takashi
---
diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
index 2883a7a6f9f3..98f2ade0266e 100644
--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -102,6 +102,8 @@ struct snd_soc_dpcm_runtime {
/* state and update */
enum snd_soc_dpcm_update runtime_update;
enum snd_soc_dpcm_state state;
+
+ int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */
};
/* can this BE stop and free */
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 002311afdeaa..fbd570c55a67 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1522,13 +1522,36 @@ static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream)
dpcm_init_runtime_hw(runtime, &cpu_dai_drv->capture);
}
+static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream *substream, int cmd);
+
+/* Set FE's runtime_update state; the state is protected via PCM stream lock
+ * for avoiding the race with trigger callback.
+ * If the state is unset and a trigger is pending while the previous operation,
+ * process the pending trigger action here.
+ */
+static void dpcm_set_fe_update_state(struct snd_soc_pcm_runtime *fe,
+ int stream, enum snd_soc_dpcm_update state)
+{
+ struct snd_pcm_substream *substream =
+ snd_soc_dpcm_get_substream(fe, stream);
+
+ snd_pcm_stream_lock_irq(substream);
+ fe->dpcm[stream].runtime_update = state;
+ if (state == SND_SOC_DPCM_UPDATE_NO && fe->dpcm[stream].trigger_pending) {
+ dpcm_fe_dai_do_trigger(substream,
+ fe->dpcm[stream].trigger_pending - 1);
+ fe->dpcm[stream].trigger_pending = 0;
+ }
+ snd_pcm_stream_unlock_irq(substream);
+}
+
static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
{
struct snd_soc_pcm_runtime *fe = fe_substream->private_data;
struct snd_pcm_runtime *runtime = fe_substream->runtime;
int stream = fe_substream->stream, ret = 0;
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
ret = dpcm_be_dai_startup(fe, fe_substream->stream);
if (ret < 0) {
@@ -1550,13 +1573,13 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
dpcm_set_fe_runtime(fe_substream);
snd_pcm_limit_hw_rates(runtime);
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
return 0;
unwind:
dpcm_be_dai_startup_unwind(fe, fe_substream->stream);
be_err:
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
return ret;
}
@@ -1603,7 +1626,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
struct snd_soc_pcm_runtime *fe = substream->private_data;
int stream = substream->stream;
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
/* shutdown the BEs */
dpcm_be_dai_shutdown(fe, substream->stream);
@@ -1617,7 +1640,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream)
dpcm_dapm_stream_event(fe, stream, SND_SOC_DAPM_STREAM_STOP);
fe->dpcm[stream].state = SND_SOC_DPCM_STATE_CLOSE;
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
return 0;
}
@@ -1665,7 +1688,7 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
int err, stream = substream->stream;
mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
dev_dbg(fe->dev, "ASoC: hw_free FE %s\n", fe->dai_link->name);
@@ -1680,7 +1703,7 @@ static int dpcm_fe_dai_hw_free(struct snd_pcm_substream *substream)
err = dpcm_be_dai_hw_free(fe, stream);
fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_FREE;
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
mutex_unlock(&fe->card->mutex);
return 0;
@@ -1773,7 +1796,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
int ret, stream = substream->stream;
mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
memcpy(&fe->dpcm[substream->stream].hw_params, params,
sizeof(struct snd_pcm_hw_params));
@@ -1796,7 +1819,7 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
fe->dpcm[stream].state = SND_SOC_DPCM_STATE_HW_PARAMS;
out:
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
mutex_unlock(&fe->card->mutex);
return ret;
}
@@ -1910,14 +1933,12 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
}
EXPORT_SYMBOL_GPL(dpcm_be_dai_trigger);
-static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
+static int dpcm_fe_dai_do_trigger(struct snd_pcm_substream *substream, int cmd)
{
struct snd_soc_pcm_runtime *fe = substream->private_data;
int stream = substream->stream, ret;
enum snd_soc_dpcm_trigger trigger = fe->dai_link->trigger[stream];
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
-
switch (trigger) {
case SND_SOC_DPCM_TRIGGER_PRE:
/* call trigger on the frontend before the backend. */
@@ -1928,7 +1949,7 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
ret = soc_pcm_trigger(substream, cmd);
if (ret < 0) {
dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
- goto out;
+ return ret;
}
ret = dpcm_be_dai_trigger(fe, substream->stream, cmd);
@@ -1939,7 +1960,7 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
ret = dpcm_be_dai_trigger(fe, substream->stream, cmd);
if (ret < 0) {
dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
- goto out;
+ return ret;
}
dev_dbg(fe->dev, "ASoC: post trigger FE %s cmd %d\n",
@@ -1956,14 +1977,13 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
ret = soc_pcm_bespoke_trigger(substream, cmd);
if (ret < 0) {
dev_err(fe->dev,"ASoC: trigger FE failed %d\n", ret);
- goto out;
+ return ret;
}
break;
default:
dev_err(fe->dev, "ASoC: invalid trigger cmd %d for %s\n", cmd,
fe->dai_link->name);
- ret = -EINVAL;
- goto out;
+ return -EINVAL;
}
switch (cmd) {
@@ -1979,7 +1999,25 @@ static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
break;
}
-out:
+ return 0;
+}
+
+static int dpcm_fe_dai_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+ struct snd_soc_pcm_runtime *fe = substream->private_data;
+ int stream = substream->stream, ret;
+
+ /* if FE's runtime_update is already set, we're in race;
+ * process this trigger later at exit
+ */
+ if (fe->dpcm[stream].runtime_update != SND_SOC_DPCM_UPDATE_NO) {
+ fe->dpcm[stream].trigger_pending = cmd + 1;
+ return 0; /* delayed, assuming it's successful */
+ }
+
+ /* we're alone, let's trigger */
+ fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+ ret = dpcm_fe_dai_do_trigger(substream, cmd);
fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
return ret;
}
@@ -2027,7 +2065,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
dev_dbg(fe->dev, "ASoC: prepare FE %s\n", fe->dai_link->name);
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_FE;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
/* there is no point preparing this FE if there are no BEs */
if (list_empty(&fe->dpcm[stream].be_clients)) {
@@ -2054,7 +2092,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream)
fe->dpcm[stream].state = SND_SOC_DPCM_STATE_PREPARE;
out:
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
mutex_unlock(&fe->card->mutex);
return ret;
@@ -2201,11 +2239,11 @@ static int dpcm_run_new_update(struct snd_soc_pcm_runtime *fe, int stream)
{
int ret;
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_BE);
ret = dpcm_run_update_startup(fe, stream);
if (ret < 0)
dev_err(fe->dev, "ASoC: failed to startup some BEs\n");
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
return ret;
}
@@ -2214,11 +2252,11 @@ static int dpcm_run_old_update(struct snd_soc_pcm_runtime *fe, int stream)
{
int ret;
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_BE;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_BE);
ret = dpcm_run_update_shutdown(fe, stream);
if (ret < 0)
dev_err(fe->dev, "ASoC: failed to shutdown some BEs\n");
- fe->dpcm[stream].runtime_update = SND_SOC_DPCM_UPDATE_NO;
+ dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO);
return ret;
}
More information about the Alsa-devel
mailing list