[alsa-devel] [Question about DPCM] dpcm_run_new_update races again xrun
Liam Girdwood
liam.r.girdwood at linux.intel.com
Mon Nov 3 18:20:12 CET 2014
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;
> }
> 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