| Message ID | B2A7C617B3AB7F44B7B44C7D374B431E38E5070E70@sc-vexch3.marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Takashi Iwai |
| Headers | show |
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
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
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;
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(-)