[alsa-devel] [Question about DPCM] dpcm_run_new_update races again xrun
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?
The scenario is like this, taking audio playback for example: 1, FE1 <-> BE1, working well for some time. 2, disconnect BE1 and connect BE2(FE1 <-> BE2) 3, during FE1 connecting BE2, FE1 is still streaming data normally. Then an under-run happens. Below are the code sequence.
soc_dpcm_runtime_update() { ... dpcm_connect_be() // connect FE1 & BE2 dpcm_run_new_update(fe, stream) { fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_BE dpcm_run_update_startup(fe, stream) { dpcm_be_dai_startup(fe, stream) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ | // under-run happens (interrupt context) | | snd_pcm_stop(substream) { | | dpcm_fe_dai_trigger(STOP) { | | fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_FE | | // trigger stop FE1, BE2 | | fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_NO | | } | | } | |_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _| dpcm_be_dai_hw_params(fe, stream) dpcm_be_dai_prepare(fe, stream) if(fe state == STOP) return 0; dpcm_be_dai_trigger(fe, ....) fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_NO ... } }
After xrun handler finishes, the FE runtime update staus is UPDATE_NO. Then the following dpcm_be_dai_hw_params & dpcm_be_dai_prepare skip related driver API excuting with this FE runtime status, and return 0 to end runtime-startup.
When user APP(ALSA lib/TinyALSA) detects xrun, usually it will do the substream prepare and write data again. Due to BE dai has not been ready for hw_params, the BE dai can't work properly after substream prepare and trigger start. After that system has no sound and can't be recovered, maybe due to FE1 doesn't know what's going on inside BE2.
The under-run is random, and under-run handler does what's necessary to be done, though trigger-stop a BE which has not been started yet is not good in my opinion.
I did an experiment to avoid be udpate checking in dpcm_be_dai_hw_params by Commenting out "if(!snd_soc_dpcm_be_can_update()}". This change is verified OK since APP will do the prepare & trigger start to resume the under-run. The patch is also attached.
From current code, dpcm_be_dai_hw_params is only callded by fe_hw_params & this
runtime startup. This be update checking might be redundant or unnecessary in current code context.
Could you help to give some suggestions? Please help to correct me if anything wrong. Thanks in advance.
--- sound/soc/soc-pcm.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 002311a..dfeb1e7 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1697,10 +1697,6 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream) struct snd_pcm_substream *be_substream = snd_soc_dpcm_get_substream(be, stream);
- /* is this op for this BE ? */ - if (!snd_soc_dpcm_be_can_update(fe, be, stream)) - continue; - /* only allow hw_params() if no connected FEs are running */ if (!snd_soc_dpcm_can_be_params(fe, be, stream)) continue;
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()
Liam
The scenario is like this, taking audio playback for example: 1, FE1 <-> BE1, working well for some time. 2, disconnect BE1 and connect BE2(FE1 <-> BE2) 3, during FE1 connecting BE2, FE1 is still streaming data normally. Then an under-run happens. Below are the code sequence.
soc_dpcm_runtime_update() { ... dpcm_connect_be() // connect FE1 & BE2 dpcm_run_new_update(fe, stream) { fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_BE dpcm_run_update_startup(fe, stream) { dpcm_be_dai_startup(fe, stream) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ | // under-run happens (interrupt context) | | snd_pcm_stop(substream) { | | dpcm_fe_dai_trigger(STOP) { | | fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_FE | | // trigger stop FE1, BE2 | | fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_NO | | } | | } | |_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _| dpcm_be_dai_hw_params(fe, stream) dpcm_be_dai_prepare(fe, stream) if(fe state == STOP) return 0; dpcm_be_dai_trigger(fe, ....) fe->dpcm.runtime_update = SND_SOC_DPCM_UPDATE_NO ... } }
After xrun handler finishes, the FE runtime update staus is UPDATE_NO. Then the following dpcm_be_dai_hw_params & dpcm_be_dai_prepare skip related driver API excuting with this FE runtime status, and return 0 to end runtime-startup.
When user APP(ALSA lib/TinyALSA) detects xrun, usually it will do the substream prepare and write data again. Due to BE dai has not been ready for hw_params, the BE dai can't work properly after substream prepare and trigger start. After that system has no sound and can't be recovered, maybe due to FE1 doesn't know what's going on inside BE2.
The under-run is random, and under-run handler does what's necessary to be done, though trigger-stop a BE which has not been started yet is not good in my opinion.
I did an experiment to avoid be udpate checking in dpcm_be_dai_hw_params by Commenting out "if(!snd_soc_dpcm_be_can_update()}". This change is verified OK since APP will do the prepare & trigger start to resume the under-run. The patch is also attached.
From current code, dpcm_be_dai_hw_params is only callded by fe_hw_params & this runtime startup. This be update checking might be redundant or unnecessary in current code context.
Could you help to give some suggestions? Please help to correct me if anything wrong. Thanks in advance.
sound/soc/soc-pcm.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 002311a..dfeb1e7 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1697,10 +1697,6 @@ int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream) struct snd_pcm_substream *be_substream = snd_soc_dpcm_get_substream(be, stream);
/* is this op for this BE ? */if (!snd_soc_dpcm_be_can_update(fe, be, stream))continue;- /* only allow hw_params() if no connected FEs are running */ if (!snd_soc_dpcm_can_be_params(fe, be, stream)) continue;
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
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?
Takashi
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; }
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;
}
At Mon, 03 Nov 2014 17:20:12 +0000, Liam Girdwood wrote:
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).
FWIW, below is a simple hack to simulate an XRUN via proc file. Just write any value xrun_injection proc file and it triggers XRUN, e.g. # echo > /proc/asound/card0/pcm0p/sub0/xrun_injection
For simulating the race, you should impose some artificial delay in the function you'll try to conflict, then trigger xrun.
Takashi
--- diff --git a/include/sound/pcm.h b/include/sound/pcm.h index e862497f7556..9e46132529c4 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -416,6 +416,7 @@ struct snd_pcm_substream { struct snd_info_entry *proc_status_entry; struct snd_info_entry *proc_prealloc_entry; struct snd_info_entry *proc_prealloc_max_entry; + struct snd_info_entry *proc_xrun_injection; #endif /* misc flags */ unsigned int hw_opened: 1; diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 42ded997b223..fa442d0504f0 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -478,6 +478,19 @@ static void snd_pcm_substream_proc_status_read(struct snd_info_entry *entry, mutex_unlock(&substream->pcm->open_mutex); }
+static void snd_pcm_xrun_injection_write(struct snd_info_entry *entry, + struct snd_info_buffer *buffer) +{ + struct snd_pcm_substream *substream = entry->private_data; + struct snd_pcm_runtime *runtime; + + snd_pcm_stream_lock_irq(substream); + runtime = substream->runtime; + if (runtime && runtime->status->state == SNDRV_PCM_STATE_RUNNING) + snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); + snd_pcm_stream_unlock_irq(substream); +} + #ifdef CONFIG_SND_PCM_XRUN_DEBUG static void snd_pcm_xrun_debug_read(struct snd_info_entry *entry, struct snd_info_buffer *buffer) @@ -610,6 +623,20 @@ static int snd_pcm_substream_proc_init(struct snd_pcm_substream *substream) } substream->proc_status_entry = entry;
+ entry = snd_info_create_card_entry(card, "xrun_injection", + substream->proc_root); + if (entry) { + entry->private_data = substream; + entry->c.text.read = NULL; + entry->c.text.write = snd_pcm_xrun_injection_write; + entry->mode = S_IFREG | S_IWUSR; + if (snd_info_register(entry) < 0) { + snd_info_free_entry(entry); + entry = NULL; + } + } + substream->proc_status_entry = entry; + return 0; }
@@ -623,6 +650,8 @@ static int snd_pcm_substream_proc_done(struct snd_pcm_substream *substream) substream->proc_sw_params_entry = NULL; snd_info_free_entry(substream->proc_status_entry); substream->proc_status_entry = NULL; + snd_info_free_entry(substream->proc_xrun_injection); + substream->proc_xrun_injection = NULL; snd_info_free_entry(substream->proc_root); substream->proc_root = NULL; return 0;
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: 2014年11月4日 1:20 To: Takashi Iwai Cc: Qiao Zhou; Mark Brown; perex@perex.cz; alsa-devel@alsa-project.org; Wilbur Wang; Chao Xie Subject: Re: [Question about DPCM] dpcm_run_new_update races again xrun
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;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;
}
Takashi, Liam
We have verified again and This patch fixes the race issue. Thanks a lot for your patch and suggestions.
Best Regards Qiao
On Mon, 2014-11-03 at 15:42 +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?
The XRUN trigger will propagate to the BE atm, but the BE DSP DAIs should never really XRUN (since the DSP does the IO) meaning the XRUN will have no influence on the BE.
I guess we could ignore the XRUN on the BE if that's what you are meaning ?
Liam
Takashi
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Mon, 2014-11-03 at 17:16 +0000, Liam Girdwood wrote:
On Mon, 2014-11-03 at 15:42 +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?
The XRUN trigger will propagate to the BE atm, but the BE DSP DAIs should never really XRUN (since the DSP does the IO) meaning the XRUN will have no influence on the BE.
I guess we could ignore the XRUN on the BE if that's what you are meaning ?
Oh, hit send before seeing your last reply. Ignore.
Liam
participants (4)
-
Liam Girdwood -
Liam Girdwood -
Qiao Zhou -
Takashi Iwai