[alsa-devel] [PATCH v3] ALSA: compress: fix drain calls blocking other compress functions

[Takashi Iwai]

At Wed, 30 Oct 2013 20:28:11 +0530,
Vinod Koul wrote:
> 
> On Wed, Oct 30, 2013 at 03:47:45PM +0100, Takashi Iwai wrote:
> > At Thu, 24 Oct 2013 16:37:31 +0530,
> > Vinod Koul wrote:
> > > 
> > > The drain and drain_notify callback were blocked by low level driver untill the
> > > draining was complete. Due to this being invoked with big fat mutex held, others
> > > ops like reading timestamp, calling pause, drop were blocked.
> > > 
> > > So to fix this we add a new snd_compr_drain_notify() API. This would be required
> > > to be invoked by low level driver when drain or partial drain has been completed
> > > by the DSP. Thus we make the drain and partial_drain callback as non blocking
> > > and driver returns immediately after notifying DSP.
> > > The waiting is done while relasing the lock so that other ops can go ahead.
> > > 
> > > Signed-off-by: Vinod Koul <vinod.koul at intel.com>
> > > CC: stable at vger.kernel.org
> > > ---
> > > v3:
> > >  call snd_compr_drain_notify from compress_stop()
> > >  rename draining -> drain_wake
> > >  add some comments on state transistion after drain
> > > 
> > > v2:
> > >  fix the 80 line warn
> > >  move the state change to compress_drain()
> > > 
> > > 
> > >  include/sound/compress_driver.h |   12 +++++++++++
> > >  sound/core/compress_offload.c   |   41 ++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 50 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> > > index 9031a26..175ab32 100644
> > > --- a/include/sound/compress_driver.h
> > > +++ b/include/sound/compress_driver.h
> > > @@ -48,6 +48,8 @@ struct snd_compr_ops;
> > >   *	the ring buffer
> > >   * @total_bytes_transferred: cumulative bytes transferred by offload DSP
> > >   * @sleep: poll sleep
> > > + * @wait: drain wait queue
> > > + * @drain_wake: condition for drain wake
> > >   */
> > >  struct snd_compr_runtime {
> > >  	snd_pcm_state_t state;
> > > @@ -59,6 +61,8 @@ struct snd_compr_runtime {
> > >  	u64 total_bytes_available;
> > >  	u64 total_bytes_transferred;
> > >  	wait_queue_head_t sleep;
> > > +	wait_queue_head_t wait;
> > > +	unsigned int drain_wake;
> > >  	void *private_data;
> > >  };
> > >  
> > > @@ -171,4 +175,12 @@ static inline void snd_compr_fragment_elapsed(struct snd_compr_stream *stream)
> > >  	wake_up(&stream->runtime->sleep);
> > >  }
> > >  
> > > +static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> > > +{
> > > +	snd_BUG_ON(!stream);
> > 
> > Should return, otherwise you'll get Oops:
> > 
> > 	if (snd_BUG_ON(!stream))
> > 		return;
> ah, missed that!
> 
> > > +
> > > +	stream->runtime->drain_wake = 1;
> > > +	wake_up(&stream->runtime->wait);
> > > +}
> > > +
> > >  #endif
> > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > index bea523a..3eb47d0 100644
> > > --- a/sound/core/compress_offload.c
> > > +++ b/sound/core/compress_offload.c
> > > @@ -123,6 +123,7 @@ static int snd_compr_open(struct inode *inode, struct file *f)
> > >  	}
> > >  	runtime->state = SNDRV_PCM_STATE_OPEN;
> > >  	init_waitqueue_head(&runtime->sleep);
> > > +	init_waitqueue_head(&runtime->wait);
> > >  	data->stream.runtime = runtime;
> > >  	f->private_data = (void *)data;
> > >  	mutex_lock(&compr->lock);
> > > @@ -682,12 +683,34 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> > >  	if (!retval) {
> > >  		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > >  		wake_up(&stream->runtime->sleep);
> > > +		snd_compr_drain_notify(stream);
> > >  		stream->runtime->total_bytes_available = 0;
> > >  		stream->runtime->total_bytes_transferred = 0;
> > >  	}
> > >  	return retval;
> > >  }
> > >  
> > > +static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
> > > +{
> > > +	/*
> > > +	 * We are called with lock held. So drop the lock while we wait for
> > > +	 * drain complete notfication from the driver
> > > +	 *
> > > +	 * It is expected that driver will notify the drain completion and then
> > > +	 * stream will be moved to SETUP state, even if draining resulted in an
> > > +	 * error. We can trigger next track after this.
> > > +	 */
> > > +	stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
> > > +	mutex_unlock(&stream->device->lock);
> > > +
> > > +	wait_event(stream->runtime->wait, stream->runtime->drain_wake);
> > 
> > Don't you really want the interruption?
> > Or use wait_event_interruptible() and return error appropriately.
> any interruption from usermode should trigger the compress_stop/compress_free

... but how?

> which will unblock this. I dont see the need to have interruptible here

Suppose you're running a single thread program.  Then who triggers the
stop?


Takashi