Re: [Question about DPCM] dpcm_run_new_update races again xrun

[Takashi Iwai <tiwai@suse.de>]

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;