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;