On Mon, 2014-11-03 at 17:54 +0100, Takashi Iwai wrote:
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.
I'll give the patch a try tomorrow and it would be good for Marvell to test tomorrow too (as my system does not XRUN).
Thanks
Liam
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;
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;
}